Enable clippy and disallow duplicate crate versions

.pre-commit-config.yaml

@@ -13,6 +13,13 @@ repos:
         entry: trailing-whitespace-fixer
         language: system
         types: [text]
+      - id: typos
+        name: Typos
+        entry: typos --write-changes
+        language: system
+        types: [text]
+        exclude_types: [diff, json]
+        exclude: ^(libs/opsqueue_python/examples/capitalize_text/lipsum\.txt|libs/opsqueue_python/examples/capitalize_text/more_lipsum\.txt)$
       - id: ruff-check
         name: Ruff linting
         entry: ruff check --fix

Cargo.toml

@@ -27,6 +27,10 @@ restriction = { level = "allow", priority = -1 }
 style = { level = "warn", priority = -1 }
 suspicious = { level = "warn", priority = -1 }
 
+# Prevent duplicate versions of crates from being used in the workspace. If an easy fix is not
+# possible, you can add the crate to the `allowed-duplicate-crates` list in `clippy.toml`.
+multiple_crate_versions = "deny"
+
 [profile.release]
 # Full fat LTO makes a big difference in performance, at the cost of a large
 # compile time hit. You should only use release builds when performance matters.

README.md

@@ -280,7 +280,7 @@ just test-integration`
 ```
 
 This will set up the required steps below automatically.
-Any arguments passsed are passed on to `pytest`, e.g.:
+Any arguments passed are passed on to `pytest`, e.g.:
 
 ```bash
 just test-integration -vvvvvv -s -k

clippy.toml

@@ -0,0 +1,25 @@
+allowed-duplicate-crates = [
+    "cpufeatures",
+    "foldhash",
+    "getrandom",
+    "hashbrown",
+    "jni-sys",
+    "nix",
+    "r-efi",
+    "rand",
+    "rand_chacha",
+    "rand_core",
+    "redox_syscall",
+    "thiserror",
+    "thiserror-impl",
+    "windows-sys",
+    "windows-targets",
+    "windows_aarch64_gnullvm",
+    "windows_aarch64_msvc",
+    "windows_i686_gnu",
+    "windows_i686_gnullvm",
+    "windows_i686_msvc",
+    "windows_x86_64_gnu",
+    "windows_x86_64_gnullvm",
+    "windows_x86_64_msvc",
+]

default.nix

@@ -34,6 +34,7 @@ let
       pkgs.pre-commit
       pkgs.python3Packages.pre-commit-hooks
       pkgs.ruff
+      pkgs.typos
 
       # For compiling the Rust parts
       pkgs.rustToolchain

doc/proposals/strategies_and_metadata.md

@@ -51,7 +51,7 @@ At some point, this will make the other operations, especially insertion, too sl
 In the rest of this document, the strategies and their potential implementation are each described in detail, and some tricks/ideas on making the right tradeoff w.r.t. what indexes to create while keeping the database manageable are given at the end.
 
 Of the above strategies, currently `Random`, `NewestFirst` and `OldestFirst` are implemented, exactly as described below.
-The other strategies, most of which depend on the metadata of a particular submission in one way or another, are not implemented yet. Describing these composable strategies and a potential way to implement them with a balance between preformance and flexibility is the main goal of this proposal.
+The other strategies, most of which depend on the metadata of a particular submission in one way or another, are not implemented yet. Describing these composable strategies and a potential way to implement them with a balance between performance and flexibility is the main goal of this proposal.
 
 ## Assumptions & Invariants
 
@@ -84,10 +84,10 @@ Furthermore, it might be important to note:
 
 Opsqueue's consumer API consists of four main producures:
 - `reserve_chunks(max: PosInt, strategy: Strategy) -> Stream<Chunk>`
-- `complete_chunk(SubmmissionId, ChunkIndex) -> ()`
+- `complete_chunk(SubmissionId, ChunkIndex) -> ()`
 - `retry_or_fail_chunk(SubmissiionId, ChunkIndex) -> ()`
 
-`complete_chunk` and `retry_or_faill_chunk` take a super brief write lock on  the SQLite database to update the state of the particular chunk in its table,
+`complete_chunk` and `retry_or_fail_chunk` take a super brief write lock on  the SQLite database to update the state of the particular chunk in its table,
 either incrementing the number of retries, moving it to `chunks_failed` or `chunks_completed`. In very rare situations (once per submission), they do more work,
 by moving the submission to its result table as well (and for `retry_or_fail_chunk`, moving all remaining chunks of that submissionn as well).
 These two calls never block a consumer however; from the perspective of the consumer they  are fire-and-forget.
@@ -180,7 +180,7 @@ But we can normalize this further:
 
 1. There are two kinds of metadata. 'I want access to extra submission-specific info inside the consumer implementation' which can just be a BLOB, and 'I  want to customize the chhunk reservation strategy', hereafter called 'strategic metadata'.
 2. Strategic metadata is relational in nature. Rather than an arbitrary BLOB, it is a `Map<String, Value>` for each submission, meaning we can store it as submission-key-value triples in a separate metadata table.
-3. For extra efficiency, we can consider 'custom priority' yet separate from tthe other strategic metadata. This  because we probably want to make a different choice w.r.t. providing an index there than for the other kinds of metadata.
+3. For extra efficiency, we can consider 'custom priority' yet separate from the other strategic metadata. This  because we probably want to make a different choice w.r.t. providing an index there than for the other kinds of metadata.
 4. We don't have to support more than a single 'custom priority' field, because if people have a multi-tier priority system they can combine that into a single formula.
 
 The strategic metadata table would look as follows:
@@ -221,7 +221,7 @@ because they can query the metadata table directly, and then join the chunks tab
 
 Combining it with the randomm order however requires either a full table scan of the chunks, or adding a join table `chunks_x_metadata` which will be quite large as it will contain the product of the number of chunks times the number of metadata keys.
 
-We could mitigate the write amplification and larger database size somewhat by storing it in a differrent SQLite 'schema' (a different database file) that we don't persist and that isn't part of the backups, and recreating it on startup. See the 'cheaper indexes' section below for details.
+We could mitigate the write amplification and larger database size somewhat by storing it in a different SQLite 'schema' (a different database file) that we don't persist and that isn't part of the backups, and recreating it on startup. See the 'cheaper indexes' section below for details.
 
 Once we have such index, we could use the 'cutting the deck' technique with it as well, which is really nice.
 

justfile

@@ -86,12 +86,12 @@ lint-heavy: clippy mypy
 # Rust static analysis
 [group('lint')]
 clippy:
-  cargo clippy --no-deps --fix --allow-dirty --allow-staged -- -Dwarnings
-  cargo clippy --no-deps -- -Dwarnings
-  cargo clippy --no-deps --fix --allow-dirty --allow-staged --no-default-features -- -Dwarnings
-  cargo clippy --no-deps --no-default-features -- -Dwarnings
-  cargo clippy --no-deps --fix --allow-dirty --allow-staged --all-features -- -Dwarnings
-  cargo clippy --no-deps --all-features -- -Dwarnings
+  cargo clippy --no-deps --all-targets --fix --allow-dirty --allow-staged -- -Dwarnings
+  cargo clippy --no-deps --all-targets -- -Dwarnings
+  cargo clippy --no-deps --all-targets --no-default-features --fix --allow-dirty --allow-staged -- -Dwarnings
+  cargo clippy --no-deps --all-targets --no-default-features -- -Dwarnings
+  cargo clippy --no-deps --all-targets --all-features --fix --allow-dirty --allow-staged -- -Dwarnings
+  cargo clippy --no-deps --all-targets --all-features -- -Dwarnings
 
 # Python static analysis type-checker
 [group('lint')]

libs/opsqueue_python/Cargo.toml

@@ -37,3 +37,6 @@ once_cell = "1.21.4" # Only currently used for `unsync::OnceCell` as part of PyO
 # Logging/tracing:
 pyo3-log = "0.13.3"
 tracing = { version = "0.1.41", features = ["log"] }
+
+[lints]
+workspace = true

libs/opsqueue_python/python/opsqueue/exceptions.py

@@ -198,7 +198,7 @@ class UnexpectedOpsqueueConsumerServerResponseError(OpsqueueInternalError):
     to a sync method something other than what it had expected.
 
     This may indicate that the client and server versions are not matching,
-    or that there is an implementatoin error in the Opsqueue code.
+    or that there is an implementation error in the Opsqueue code.
     """
 
     pass
@@ -232,15 +232,15 @@ class ChunkStorageError(OpsqueueInternalError):
 
 class InternalConsumerClientError(OpsqueueInternalError):
     """
-    Raised for any othre kind of Consumer client exception
+    Raised for any other kind of Consumer client exception
     """
 
     pass
 
 
 class InternalProducerClientError(OpsqueueInternalError):
     """
-    Raised for any othre kind of Producer client exception
+    Raised for any other kind of Producer client exception
     """
 
     pass

libs/opsqueue_python/python/opsqueue/producer.py

@@ -359,7 +359,7 @@ def get_submission_status(
     def lookup_submission_id_by_prefix(self, prefix: str) -> SubmissionId | None:
         """
         Attempts to find the submission ID if only the prefix of the submission
-        (AKA the path at which the submision's chunks are stored in the object store)
+        (AKA the path at which the submission's chunks are stored in the object store)
         is known.
 
         Returns `None` if no submission could be found.

libs/opsqueue_python/src/async_util.rs

@@ -1,4 +1,4 @@
-//! Helpers to bridge async Rust and PyO3's flavour of async Python
+//! Helpers to bridge async Rust and `PyO3`'s flavour of async Python
 //!
 use pyo3::{Bound, IntoPyObject, PyAny, PyResult, Python};
 use pyo3_async_runtimes::TaskLocals;
@@ -12,7 +12,7 @@ use std::{
 ///
 /// Essentially `py.detach` but for async code.
 ///
-/// Based on https://pyo3.rs/v0.25.1/async-await.html#release-the-gil-across-await
+/// Based on <https://pyo3.rs/v0.25.1/async-await.html#release-the-gil-across-await>
 pub struct AsyncAllowThreads<F>(F);
 
 pub fn async_detach<F>(fut: F) -> AsyncAllowThreads<F>

libs/opsqueue_python/src/common.rs

@@ -39,6 +39,7 @@ impl SubmissionId {
         Ok(SubmissionId { id })
     }
 
+    #[allow(clippy::trivially_copy_pass_by_ref)]
     fn __repr__(&self) -> String {
         format!("SubmissionId(id={})", self.id)
     }
@@ -73,6 +74,7 @@ impl ChunkIndex {
         Ok(ChunkIndex { id })
     }
 
+    #[allow(clippy::trivially_copy_pass_by_ref)]
     fn __repr__(&self) -> String {
         format!("ChunkIndex(id={})", self.id)
     }
@@ -187,7 +189,7 @@ impl From<&Strategy> for strategy::Strategy {
             } => {
                 let underlying = strategy::Strategy::from(underlying.get());
                 strategy::Strategy::PreferDistinct {
-                    meta_key: meta_key.to_string(),
+                    meta_key: meta_key.clone(),
                     underlying: Box::new(underlying),
                 }
             }
@@ -218,27 +220,36 @@ pub struct Chunk {
 }
 
 impl Chunk {
+    /// Build a Python-facing chunk by combining DB metadata and chunk contents.
+    ///
+    /// # Panics
+    ///
+    /// Panics if the chunk content is stored in object storage while the submission
+    /// has no associated prefix.
+    ///
+    /// # Errors
+    ///
+    /// Returns an error if fetching chunk bytes from object storage fails.
     pub async fn from_internal(
         c: chunk::Chunk,
         s: submission::Submission,
         object_store_client: &ObjectStoreClient,
     ) -> Result<Self, ChunkRetrievalError> {
-        let (content, prefix) = match c.input_content {
-            Some(bytes) => (bytes, None),
-            None => {
-                let prefix = s.prefix.unwrap();
-                tracing::debug!(
-                    "Fetching chunk content from object store: submission_id={}, prefix={}, chunk_index={}",
-                    c.submission_id,
-                    prefix,
-                    c.chunk_index
-                );
-                let res = object_store_client
-                    .retrieve_chunk(&prefix, c.chunk_index, ChunkType::Input)
-                    .await?;
-                tracing::debug!("Fetched chunk content: {res:?}");
-                (res, Some(prefix))
-            }
+        let (content, prefix) = if let Some(bytes) = c.input_content {
+            (bytes, None)
+        } else {
+            let prefix = s.prefix.unwrap();
+            tracing::debug!(
+                "Fetching chunk content from object store: submission_id={}, prefix={}, chunk_index={}",
+                c.submission_id,
+                prefix,
+                c.chunk_index
+            );
+            let res = object_store_client
+                .retrieve_chunk(&prefix, c.chunk_index, ChunkType::Input)
+                .await?;
+            tracing::debug!("Fetched chunk content: {res:?}");
+            (res, Some(prefix))
         };
         Ok(Chunk {
             submission_id: c.submission_id.into(),
@@ -273,6 +284,7 @@ pub struct ChunkFailed {
 }
 
 impl ChunkFailed {
+    #[must_use]
     pub fn from_internal(c: chunk::ChunkFailed, _s: &submission::SubmissionFailed) -> Self {
         ChunkFailed {
             submission_id: c.submission_id.into(),
@@ -356,7 +368,9 @@ pub enum SubmissionStatus {
 
 impl From<opsqueue::common::submission::SubmissionStatus> for SubmissionStatus {
     fn from(value: opsqueue::common::submission::SubmissionStatus) -> Self {
-        use opsqueue::common::submission::SubmissionStatus::*;
+        use opsqueue::common::submission::SubmissionStatus::{
+            Cancelled, Completed, Failed, InProgress,
+        };
         match value {
             InProgress(s) => SubmissionStatus::InProgress {
                 submission: s.into(),
@@ -515,7 +529,7 @@ pub enum SubmissionNotCancellable {
 
 impl From<opsqueue::common::errors::SubmissionNotCancellable> for SubmissionNotCancellable {
     fn from(value: opsqueue::common::errors::SubmissionNotCancellable) -> Self {
-        use opsqueue::common::errors::SubmissionNotCancellable::*;
+        use opsqueue::common::errors::SubmissionNotCancellable::{Cancelled, Completed, Failed};
         match value {
             Completed(s) => SubmissionNotCancellable::Completed {
                 submission: s.into(),
@@ -541,6 +555,12 @@ impl SubmissionNotCancellable {
     }
 }
 
+/// Await a future while also reacting to Python interruption signals.
+///
+/// # Errors
+///
+/// Returns the underlying future error, or a fatal Python exception when
+/// an interrupt signal is detected.
 pub async fn run_unless_interrupted<T, E>(
     future: impl IntoFuture<Output = Result<T, E>>,
 ) -> Result<T, E>
@@ -575,7 +595,7 @@ pub async fn check_signals_in_background() -> FatalPythonException {
 /// Note that we very intentionally use the multi-threaded scheduler
 /// but with a single thread.
 ///
-/// We **cannot** use the current_thread scheduler,
+/// We **cannot** use the `current_thread` scheduler,
 /// since that would result in the Python task scheduler (e.g. `asyncio`)
 /// and the Rust task scheduler (`Tokio`) to run on the same thread.
 /// This seems to work fine, until you end up with a Python future
@@ -590,6 +610,11 @@ pub async fn check_signals_in_background() -> FatalPythonException {
 /// 'Tokio scheduler thread' is preemptive,
 /// the same problem now no longer occurs:
 /// Both schedulers are able to make forward progress (even on a 1-CPU machine).
+///
+/// # Panics
+///
+/// Panics if creating the Tokio runtime fails.
+#[must_use]
 pub fn start_runtime() -> Arc<tokio::runtime::Runtime> {
     let runtime = tokio::runtime::Builder::new_multi_thread()
         .worker_threads(1)
@@ -600,12 +625,16 @@ pub fn start_runtime() -> Arc<tokio::runtime::Runtime> {
 }
 
 /// Formats a Python exception
-/// in similar fashion as the traceback.format_exc()
+/// in similar fashion as the `traceback.format_exc()`
 /// would do it.
 ///
 /// Internally acquires the GIL!
 ///
 /// c.f. <https://pyo3.rs/main/doc/pyo3/types/trait.pytracebackmethods>
+///
+/// # Panics
+///
+/// Panics if formatting a Python traceback fails.
 pub fn format_pyerr(err: &PyErr) -> String {
     Python::attach(|py| {
         let msg: Option<String> = (|| {