fix(fjall): use patched version of fjall with deadlock fix#930
fix(fjall): use patched version of fjall with deadlock fix#930
Conversation
📝 WalkthroughWalkthroughThe pull request updates the fjall crate dependency from version 3.0.2 to 3.0.3 in Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Cargo.toml`:
- Around line 232-233: The patch for the fjall crate in the Cargo.toml uses a
mutable branch ("recovery/change-flush-queueing"); change it to pin the patch to
an immutable commit by replacing the branch key with rev = "<commit-sha>" (use
the exact commit SHA you want) under the [patch.crates-io] entry for fjall so
the patch refers to a fixed commit rather than a moving branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 28c1bb27-3e3c-4900-83e2-8001f20337ba
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
Cargo.tomlcrates/fjall/Cargo.toml
| [patch.crates-io] | ||
| fjall = { git = "https://github.com/fjall-rs/fjall.git", branch = "recovery/change-flush-queueing" } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Resolving current head of target branch:"
git ls-remote https://github.com/fjall-rs/fjall.git refs/heads/recovery/change-flush-queueing
lockfile="$(fd '^Cargo.lock$' | head -n1)"
if [ -n "${lockfile:-}" ]; then
echo
echo "Current fjall lock entry:"
rg -n 'name = "fjall"|source = "git\+https://github\.com/fjall-rs/fjall\.git' "$lockfile" -A4 -B2
fiRepository: txpipe/dolos
Length of output: 492
🌐 Web query:
Cargo.toml patch.crates-io mutable branch non-reproducible builds best practices
💡 Result:
Problem with mutable branch in [patch.crates-io]
If you patch a crates.io dependency to a git branch (e.g. branch = "main"), the meaning of that dependency can change whenever the branch tip moves. Cargo will lock the exact commit in Cargo.lock at the time of resolution, and it will only change when you update the lockfile (for example via cargo update, deleting Cargo.lock, etc.). (doc.rust-lang.org)
So builds become non-reproducible whenever you don’t reliably use the same lockfile (or you intentionally/accidentally update it).
Best practices (reproducible + least surprise)
-
Prefer pinning the patch to an immutable commit (
rev)
Use a full commit SHA for the patch target:[patch.crates-io] somecrate = { git = "https://github.com/org/somecrate", rev = "…full_sha…" }
Cargo explicitly supports choosing commits via
rev/tag/branch, and usingrevavoids “moving target” issues. (doc.rust-lang.org) -
If you must use a branch (short-lived), treat
Cargo.lockas mandatory
CommitCargo.lock(especially for binaries/apps) and have CI enforce it. Cargo’s own guidance: “When in doubt, checkCargo.lockinto version control.” (doc.rust-lang.org) -
Enforce lockfile determinism in CI
- Use
cargo build --lockedto fail if the lockfile would change. - Use
cargo build --frozento additionally prevent network access (good for hermetic CI). (venea.net)
- Use
-
Keep patches local-only when appropriate
Cargo allows putting[patch]into.cargo/config.tomlor passing it via--config, which is useful for temporary/local patches you don’t want to commit to the repo. (doc.rust-lang.org) -
Avoid relying on
[patch]in published libraries
[patch]is only read from the workspace root; it’s primarily a consumer-side override mechanism, not something downstream users of your published library will automatically inherit. (doc.rust-lang.org)
Practical rule of thumb
- Long-lived / shared builds: pin with
rev(best), commitCargo.lock, and build with--locked(and often--frozen) in CI. (doc.rust-lang.org) - Short-lived experimentation: branch patch is fine, but expect the lockfile to be the real source of reproducibility. (doc.rust-lang.org)
Citations:
- 1: https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html
- 2: https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html
- 3: https://doc.rust-lang.org/cargo/guide/cargo-toml-vs-cargo-lock.html
- 4: https://www.venea.net/man/cargo%281%29?utm_source=openai
- 5: https://doc.rust-lang.org/cargo/reference/overriding-dependencies.html
- 6: https://doc.rust-lang.org/cargo/reference/overriding-dependencies.html
- 7: https://doc.rust-lang.org/cargo/guide/cargo-toml-vs-cargo-lock.html
- 8: https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html
Pin patched fjall to an immutable commit (rev) instead of a branch.
Using a mutable branch in [patch.crates-io] makes the patch target a moving reference, which can silently change behavior when the branch advances. Although Cargo.lock currently locks the exact commit, pinning to rev in Cargo.toml is the best practice—it makes intent explicit and doesn't rely on lockfile enforcement.
Proposed fix
[patch.crates-io]
-fjall = { git = "https://github.com/fjall-rs/fjall.git", branch = "recovery/change-flush-queueing" }
+fjall = { git = "https://github.com/fjall-rs/fjall.git", rev = "2443c7bcf6f53920efef836518d76e865974c4ca" }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [patch.crates-io] | |
| fjall = { git = "https://github.com/fjall-rs/fjall.git", branch = "recovery/change-flush-queueing" } | |
| [patch.crates-io] | |
| fjall = { git = "https://github.com/fjall-rs/fjall.git", rev = "2443c7bcf6f53920efef836518d76e865974c4ca" } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Cargo.toml` around lines 232 - 233, The patch for the fjall crate in the
Cargo.toml uses a mutable branch ("recovery/change-flush-queueing"); change it
to pin the patch to an immutable commit by replacing the branch key with rev =
"<commit-sha>" (use the exact commit SHA you want) under the [patch.crates-io]
entry for fjall so the patch refers to a fixed commit rather than a moving
branch.
Summary by CodeRabbit