fix(#1154): soldr-style daemon ownership for cold-cache reset (#1159)#1168
Conversation
…redesign snapshot)
Replace the rejected two-lock (reset-gate + shared lifecycle) design with soldr's daemon ownership strategy: - fbuild_paths::daemon_ownership: version-blind per-cache-root root-owner.lock held for the daemon's entire lifetime, spawn.lock single-flight election, and an advisory root-owner.json claim (pid/exe/version/mode/cache_root_key/port). - fbuild_core::process_identity: pid liveness, executable-identity (fails closed), and bounded terminate helpers copied from soldr. - fbuild-daemon acquires root ownership before starting the embedded zccache service and fails closed if another daemon owns the root. - fbuild clean cache: holds spawn election across the whole reset, validates cache identity, distinguishes busy refusal from lost shutdown, sweeps legacy daemons via daemon-*.port discovery with verified-PID displacement only, takes exclusive root ownership as positive proof before deleting <root>/zccache, and restores the daemon even when cleanup fails. - fbuild_core::file_lock: classify Windows ERROR_LOCK_VIOLATION as contention (was misread as a hard error). - Real-process regression tests: legacy/unverified PID is never signaled; abrupt daemon death releases root ownership. - Docs: narrow the "no file-based locks" rule to the sanctioned daemon-lifecycle exception. Closes #1159. Closes #1154. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe pull request adds daemon cache-root ownership and spawn coordination, introduces ChangesCache reset and benchmark coordination
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@agents/docs/commands-reference.md`:
- Line 15: Escape the pipe separators in the `fbuild clean sketch|all|cache`
command text within the commands-reference table as `\|`, preserving the
three-column table structure and all other row content.
In `@crates/fbuild-core/Cargo.toml`:
- Around line 41-43: Replace the fs2-based locking dependency with Rust
standard-library FileExt locking primitives while preserving the existing
shared/exclusive try-lock API and File::lock_contended_error behavior. Update
the fbuild-core locking implementation to use std::fs::FileExt, then remove fs2
from crates/fbuild-core/Cargo.toml and the root workspace dependencies once no
references remain.
In `@crates/fbuild-core/src/file_lock.rs`:
- Around line 6-59: Replace the fs2-based locking in try_acquire with Rust 1.89+
std::fs::File methods, using try_lock_shared or try_lock according to
FileLockMode and mapping TryLockError::WouldBlock to Ok(None) while propagating
other errors. Remove lock_is_held and the fs2 import, then delete the related
fs2 dependency references from the workspace and Cargo manifests.
- Line 1: Update the file-locking implementation in file_lock.rs to use
std::fs::File native try_lock/try_lock_shared APIs, handling
TryLockError::WouldBlock for contention and removing the hand-rolled
lock_is_held classifier. Remove the fs2 dependency from both workspace and
fbuild-core Cargo.toml manifests while preserving existing lock behavior.
In `@crates/fbuild-core/src/process_identity.rs`:
- Around line 75-87: Update pid_executable_path to satisfy the subprocess-spawn
lint by routing the /bin/ps invocation through
fbuild_core::subprocess::{run_command,run_command_passthrough}, or add an
allow-direct-spawn marker with a concise reason if direct Command usage must
remain. Ensure the ps execution is bounded with the existing supported timeout
mechanism before processing its output.
In `@crates/fbuild-paths/src/daemon_ownership.rs`:
- Line 293: Update the #[ignore] attribute for
root_ownership_is_version_blind_across_processes to include a valid tracking
issue reference or concrete hardware/toolchain requirement, following the
sibling ignore annotations’ convention such as (`#1159`).
- Around line 309-320: Add the required allow-direct-spawn marker comment
immediately above the std::process::Command::new call inside the run_probe
closure, including a concise reason for this intentional subprocess test probe.
Leave the existing command arguments and environment setup unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5800ff9b-ccd3-4b59-ad8b-941dbf797aec
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
.github/workflows/benchmark-build-comparison.ymlCLAUDE.mdCargo.tomlagents/docs/commands-reference.mdbench/blink/README.mdbench/fastled-examples/src/README.mdbench/fastled-examples/src/build_comparison.rsbench/fastled-examples/src/build_comparison_tests.rscrates/fbuild-build-engine/src/framework_core_cache.rscrates/fbuild-cli/src/cli/args.rscrates/fbuild-cli/src/cli/clean.rscrates/fbuild-cli/src/cli/tests.rscrates/fbuild-cli/src/daemon_client.rscrates/fbuild-core/Cargo.tomlcrates/fbuild-core/README.mdcrates/fbuild-core/src/file_lock.rscrates/fbuild-core/src/lib.rscrates/fbuild-core/src/process_identity.rscrates/fbuild-daemon/src/main.rscrates/fbuild-daemon/tests/legacy_daemon_transition.rscrates/fbuild-paths/Cargo.tomlcrates/fbuild-paths/README.mdcrates/fbuild-paths/src/daemon_ownership.rscrates/fbuild-paths/src/lib.rscrates/fbuild-serial/src/ports.rsdocs/architecture/runtime.mddocs/reference/cli.md
| |---|---|---| | ||
| | `fbuild build` | You want to compile firmware for the env specified by `-e <env>` and `<project_dir>`. The default path; cache via daemon. | `fbuild help build` | | ||
| | `fbuild clean sketch|all` | You want to remove one environment/profile's project outputs, optionally including its exact reusable framework-cache entries, without compiling or deploying. | `fbuild help clean`, FastLED/fbuild#1089 | | ||
| | `fbuild clean sketch|all|cache` | You want to remove one target's project outputs, optionally its reusable framework objects; `cache` additionally resets the active mode's global compiler-object cache while retaining installed packages/toolchains. | `fbuild help clean`, FastLED/fbuild#1089, FastLED/fbuild#1154 | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Escape the pipes — the table row is broken (MD056).
The | characters in sketch|all|cache are parsed as column delimiters, so this row expands to 5 cells against a 3-column table and renders incorrectly. Escape them as \|.
Proposed fix
-| `fbuild clean sketch|all|cache` | You want to remove one target's project outputs, optionally its reusable framework objects; `cache` additionally resets the active mode's global compiler-object cache while retaining installed packages/toolchains. | `fbuild help clean`, FastLED/fbuild#1089, FastLED/fbuild#1154 |
+| `fbuild clean sketch\|all\|cache` | You want to remove one target's project outputs, optionally its reusable framework objects; `cache` additionally resets the active mode's global compiler-object cache while retaining installed packages/toolchains. | `fbuild help clean`, FastLED/fbuild#1089, FastLED/fbuild#1154 |📝 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.
| | `fbuild clean sketch|all|cache` | You want to remove one target's project outputs, optionally its reusable framework objects; `cache` additionally resets the active mode's global compiler-object cache while retaining installed packages/toolchains. | `fbuild help clean`, FastLED/fbuild#1089, FastLED/fbuild#1154 | | |
| | `fbuild clean sketch\|all\|cache` | You want to remove one target's project outputs, optionally its reusable framework objects; `cache` additionally resets the active mode's global compiler-object cache while retaining installed packages/toolchains. | `fbuild help clean`, FastLED/fbuild#1089, FastLED/fbuild#1154 | |
🧰 Tools
🪛 LanguageTool
[style] ~15-~15: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...| fbuild clean sketch|all|cache | You want to remove one target's project outputs, op...
(REP_WANT_TO_VB)
🪛 markdownlint-cli2 (0.23.0)
[warning] 15-15: Table column count
Expected: 3; Actual: 5; Too many cells, extra data will be missing
(MD056, table-column-count)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agents/docs/commands-reference.md` at line 15, Escape the pipe separators in
the `fbuild clean sketch|all|cache` command text within the commands-reference
table as `\|`, preserving the three-column table structure and all other row
content.
Source: Linters/SAST tools
| # Cross-process shared/exclusive fbuild-daemon lifecycle gates. These are not | ||
| # used for zccache object access. Locks are released if a process exits. | ||
| fs2 = { workspace = true } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant files =="
git ls-files | rg '(^|/)Cargo\.toml$|file_lock\.rs$|crates/fbuild-core/Cargo\.toml$' | sed -n '1,120p'
echo "== root Cargo manifests around relevant dependencies =="
for f in Cargo.toml crates/fbuild-core/Cargo.toml; do
if [ -f "$f" ]; then
echo "--- $f ---"
cat -n "$f" | sed -n '1,180p'
fi
done
echo "== fs2 references =="
rg -n "fs2|flock|lock" crates/fbuild-core crates/fbuild-* Cargo.toml ci/check_workspace_crates.py ci/hooks/crate_guard.py -S || trueRepository: FastLED/fbuild
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file_lock.rs =="
fd -a 'file_lock\.rs$' . | sed 's#^\./##'
echo "== inspect file_lock.rs files =="
while read -r f; do
echo "--- $f ---"
wc -l "$f"
cat -n "$f" | sed -n '1,220p'
done < <(fd 'file_lock\.rs$' .)Repository: FastLED/fbuild
Length of output: 319
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file_lock.rs =="
wc -l crates/fbuild-core/src/file_lock.rs
cat -n crates/fbuild-core/src/file_lock.rs | sed -n '1,220p'
echo "== fs2 references (targeted) =="
rg -n "fs2|LockFileExt|file_lock" crates/fbuild-core Cargo.toml crates/fbuild-core/Cargo.toml -S || true
echo "== manifest dependency sections (targeted) =="
cat -n Cargo.toml | sed -n '1,180p' | rg -n '^\s*(version|edition|fs2|fbuild-core|workspace|dependencies|members)=|^\s*fs2|^\s*\[work|^\s*\[package|^\s*dependencies|^#|^$' -C 2 || true
cat -n crates/fbuild-core/Cargo.toml | sed -n '1,180p' | rg -n '^\s*(version|edition|fs2|devdependencies?|dependencies|features)=|^\s*fs2|^\s*\[package\]|^\s*\[dependencies\]|^\s*\[dev-dependencies\]|^#|^$' -C 2 || trueRepository: FastLED/fbuild
Length of output: 7102
🌐 Web query:
Rust std::fs::File lock_shared lock_exclusive FileExt lock_shared lock_exclusive stable version
💡 Result:
As of July 23, 2026, the Rust standard library (std::fs::File) provides built-in, stable support for file locking, including lock_shared and lock_exclusive [1][2][3]. These methods were introduced directly into the std::fs::File API and are no longer dependent on third-party crates like fs2 or fs4 for basic file locking functionality [1][4][5]. Key details regarding the standard library's file locking API: Methods: - lock_shared: Acquires a shared (non-exclusive) lock, blocking until acquired [1][3]. - lock: Acquires an exclusive lock, blocking until acquired [3]. - try_lock_shared: Attempts to acquire a shared lock without blocking [1][3]. - try_lock: Attempts to acquire an exclusive lock without blocking [1][3]. - unlock: Releases locks held on the file [3]. Important Considerations: - Advisory vs. Mandatory: These locks may be advisory or mandatory depending on the platform [1][3]. - Platform Specifics: Behavior regarding interactions with read/write operations and other file handles is platform-dependent [1][3]. - Cloned Handles: If a file handle (or its clone) already holds a lock, behavior is unspecified and may cause deadlocks [1]. - Release: Locks are automatically released when the file handle (and any duplicated/inherited descriptors) is closed, or explicitly via the unlock method [1][3]. Historically, Rust developers relied on extension traits like FileExt from crates such as fs2 or fs4 to provide this functionality [4][5]. With the inclusion of these methods in the standard library, this is now a native capability of std::fs::File [1][3].
Citations:
- 1: https://doc.rust-lang.org/stable/std/fs/struct.File.html
- 2: https://dev-doc.rust-lang.org/stable/src/std/fs.rs.html
- 3: https://doc.rust-lang.org/stable/src/std/fs.rs.html
- 4: https://docs.rs/fs2/latest/fs2/trait.FileExt.html
- 5: https://docs.rs/fs4/latest/fs4/fs_err3/trait.FileExt.html
🌐 Web query:
Rust 1.76 std::fs::File lock_shared lock_exclusive try_lock_shared try_lock_exclusive
💡 Result:
As of Rust 1.76, the std::fs::File struct does not include the methods lock_shared, lock_exclusive, try_lock_shared, or try_lock_exclusive [1]. These file-locking features were stabilized in Rust 1.89 [2]. Before this release, developers needing file locking in Rust typically relied on third-party crates such as the 'fs2' or 'fd-lock' crates. In the current stable version of Rust, the std::fs::File struct provides methods like lock, lock_shared, try_lock, and try_lock_shared to handle advisory or mandatory file locking [3][4][2]. These methods are designed to interact with each other and release automatically when the File handle is dropped, though they can also be released explicitly using the unlock method [3][5][2].
Citations:
- 1: https://doc.rust-lang.org/1.76.0/std/fs/struct.File.html
- 2: https://rustbites.com/posts/bite-051/
- 3: https://doc.rust-lang.org/stable/std/fs/struct.File.html
- 4: https://doc.rust-lang.org/nightly/std/fs/struct.File.html
- 5: https://doc.rust-lang.org/std/fs/struct.File.html
Replace fs2 with std file-locking primitives.
fbuild-core targets Rust 1.94.1, so use std::fs::FileExt; covers shared/exclusive try locks, but lock_contended_error() has been File::lock_contended_error() in the std API long enough here; keep the API surface and remove fs2 from crates/fbuild-core/Cargo.toml and the root workspace dependency when it is no longer needed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/fbuild-core/Cargo.toml` around lines 41 - 43, Replace the fs2-based
locking dependency with Rust standard-library FileExt locking primitives while
preserving the existing shared/exclusive try-lock API and
File::lock_contended_error behavior. Update the fbuild-core locking
implementation to use std::fs::FileExt, then remove fs2 from
crates/fbuild-core/Cargo.toml and the root workspace dependencies once no
references remain.
| @@ -0,0 +1,157 @@ | |||
| //! Cross-process locks for fbuild-daemon startup/lifecycle coordination. | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo files =="
git ls-files | rg '(^|/)Cargo.toml$|crates/fbuild-core/src/file_lock.rs' || true
echo
echo "== Cargo manifest snippets =="
for f in Cargo.toml crates/fbuild-core/Cargo.toml; do
if [ -f "$f" ]; then
echo "--- $f"
nl -ba "$f" | sed -n '1,150p'
fi
done
echo
echo "== file_lock.rs =="
if [ -f crates/fbuild-core/src/file_lock.rs ]; then
nl -ba crates/fbuild-core/src/file_lock.rs | sed -n '1,180p'
fi
echo
echo "== search fs2/lock_is_held/try_lock_shared/lock_shared =="
rg -n 'fs2|lock_is_held|try_lock_shared|try_lock_exclusive|lock_shared|lock_exclusive|std::fs::File|TryLockError' . --glob '*.rs' --glob 'Cargo.toml' || trueRepository: FastLED/fbuild
Length of output: 2142
🌐 Web query:
Rust std::fs::File try_lock shared lock TryLockError stabilized 1.89 documentation
💡 Result:
The file locking API for std::fs::File, including the try_lock and try_lock_shared methods, was stabilized in Rust 1.89.0 [1][2][3]. This API introduces advisory file locking, allowing file handles to acquire either exclusive or shared locks [4]. The methods try_lock and try_lock_shared return a Result<, TryLockError> [1][5][6]. The TryLockError enum is also stabilized in 1.89.0 and includes the following variants [5][3]: - TryLockError::Error(io::Error): Indicates that the lock could not be acquired due to an I/O error [5][3]. - TryLockError::WouldBlock: Indicates that the lock could not be acquired because it is already held by another handle or process [5][3]. These methods and the associated error type are documented as stable features starting from Rust 1.89.0 [1][5][3].
Citations:
- 1: https://doc.rust-lang.org/1.89.0/std/fs/struct.File.html
- 2: Stabilize "file_lock" feature rust-lang/rust#142125
- 3: https://dev-doc.rust-lang.org/stable/src/std/fs.rs.html
- 4: Tracking Issue for File lock API rust-lang/rust#130994
- 5: https://doc.rust-lang.org/1.89.0/std/fs/enum.TryLockError.html
- 6: https://doc.rust-lang.org/stable/src/std/fs.rs.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Cargo manifest snippets =="
for f in Cargo.toml crates/fbuild-core/Cargo.toml; do
if [ -f "$f" ]; then
echo "--- $f"
awk '{printf "%6d\t%s\n", NR, $0}' "$f" | sed -n '1,150p'
fi
done
echo
echo "== file_lock.rs =="
if [ -f crates/fbuild-core/src/file_lock.rs ]; then
awk '{printf "%6d\t%s\n", NR, $0}' crates/fbuild-core/src/file_lock.rs | sed -n '1,180p'
fi
echo
echo "== search fs2/lock_is_held/try_lock_shared/lock_shared =="
rg -n 'fs2|lock_is_held|try_lock_shared|try_lock_exclusive|lock_share(d|exclusive)|TryLockError|FileExt' . --glob '*.rs' --glob 'Cargo.toml' || true
echo
echo "== local lock helper outline =="
if command -v rg >/dev/null 2>&1; then
rg -n 'fn .*lock|struct .*Lock|lock\(|try_lock' crates/fbuild-core/src crates/fbuild-daemon/src | head -200 || true
fiRepository: FastLED/fbuild
Length of output: 31166
Replace fs2 file locking with std::fs::File’s native lock API. Rust 1.89 adds stable try_lock/try_lock_shared with TryLockError::WouldBlock handling contention cross-platform, so crates/fbuild-core/src/file_lock.rs can use the std API and remove the hand-rolled lock_is_held classifier. Then remove fs2 from both Cargo.toml and crates/fbuild-core/Cargo.toml.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/fbuild-core/src/file_lock.rs` at line 1, Update the file-locking
implementation in file_lock.rs to use std::fs::File native
try_lock/try_lock_shared APIs, handling TryLockError::WouldBlock for contention
and removing the hand-rolled lock_is_held classifier. Remove the fs2 dependency
from both workspace and fbuild-core Cargo.toml manifests while preserving
existing lock behavior.
| use fs2::FileExt; | ||
| use std::fs::{File, OpenOptions}; | ||
| use std::io; | ||
| use std::path::Path; | ||
| use std::time::{Duration, Instant}; | ||
|
|
||
| #[derive(Clone, Copy, Debug, PartialEq, Eq)] | ||
| pub enum FileLockMode { | ||
| Shared, | ||
| Exclusive, | ||
| } | ||
|
|
||
| #[derive(Debug)] | ||
| pub struct FileLockGuard { | ||
| _file: File, | ||
| } | ||
|
|
||
| /// Try to acquire an OS-released lock on `path`. | ||
| /// | ||
| /// Returns `Ok(None)` when another process holds a conflicting lock. The lock | ||
| /// is released automatically when the guard is dropped or the process exits. | ||
| pub fn try_acquire(path: &Path, mode: FileLockMode) -> io::Result<Option<FileLockGuard>> { | ||
| if let Some(parent) = path.parent() { | ||
| std::fs::create_dir_all(parent)?; | ||
| } | ||
| let file = OpenOptions::new() | ||
| .create(true) | ||
| .truncate(false) | ||
| .read(true) | ||
| .write(true) | ||
| .open(path)?; | ||
| let result = match mode { | ||
| FileLockMode::Shared => FileExt::try_lock_shared(&file), | ||
| FileLockMode::Exclusive => FileExt::try_lock_exclusive(&file), | ||
| }; | ||
| match result { | ||
| Ok(()) => Ok(Some(FileLockGuard { _file: file })), | ||
| Err(error) if lock_is_held(&error) => Ok(None), | ||
| Err(error) => Err(error), | ||
| } | ||
| } | ||
|
|
||
| /// Does this error mean "another process holds a conflicting lock"? | ||
| /// | ||
| /// Unix reports contention as `EWOULDBLOCK` (kind `WouldBlock`), but Windows | ||
| /// `LockFileEx(LOCKFILE_FAIL_IMMEDIATELY)` reports `ERROR_LOCK_VIOLATION` | ||
| /// (os error 33), which std maps to an uncategorized kind — so a kind check | ||
| /// alone misclassifies contention as a hard error on Windows. Also compare | ||
| /// against `fs2::lock_contended_error()` (the canonical per-platform | ||
| /// contention error), mirroring soldr's `lock_is_held`. | ||
| fn lock_is_held(error: &io::Error) -> bool { | ||
| error.kind() == io::ErrorKind::WouldBlock | ||
| || error.raw_os_error() == fs2::lock_contended_error().raw_os_error() | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant Rust std file-lock availability =="
python3 - <<'PY'
# Read-only comparison of the repository MSRV/toolchain baseline and stabilized std::fs::File locks.
import re, pathlib, textwrap
for f in [pathlib.Path("Cargo.toml"), *pathlib.Path(".").glob("crates/*/Cargo.toml")]:
if f.exists():
txt=f.read_text(errors="replace")
for m in re.finditer(r'rust-version\s*=\s*"([^"]+)"|toolchain\s*=.*"([^"]+)"|min-supported-rust-version\s*=.*"([^"]+)"', txt):
print(f"{f}: {m.group(0)}")
PY
echo
echo "== locate file_lock.rs and lock_is_held usage =="
git ls-files | rg '(^|/)file_lock\.rs$|Cargo\.toml$|Cargo\.lock$' || true
echo
rg -n "fs2::FileExt|fs2|lock_contested_error|lock_contended_error|TryLockError|fbuild-core" Cargo.toml crates -S --glob 'Cargo.toml' --glob '*.rs' || true
echo
echo "== file_lock.rs =="
cat -n crates/fbuild-core/src/file_lock.rs
echo
echo "== Rust std docs via cargo if available =="
if command -v rustc >/dev/null 2>&1; then rustc --version; rustc --print sysroot; fiRepository: FastLED/fbuild
Length of output: 11635
🌐 Web query:
Rust std::fs::File try_lock try_lock_shared TryLockError WouldBlock stabilized 1.89.0
💡 Result:
The file locking API for std::fs::File, including the methods try_lock and try_lock_shared and the error enum TryLockError, was officially stabilized in Rust 1.89.0 [1][2][3]. These methods and the associated enum are part of the file_lock feature and provide cross-platform support for advisory file locking [1][4]. The try_lock and try_lock_shared methods return a Result<, TryLockError> [5][1]. The TryLockError enum is defined as follows [6][1]: pub enum TryLockError { Error(io::Error), WouldBlock, } The WouldBlock variant is returned when a lock cannot be acquired because it is currently held by another process or file handle [6][1]. The Error variant captures other I/O-related issues that may occur during the locking attempt [6][1]. This design replaces earlier proposals that used boolean return values, providing a more robust and less error-prone way to handle lock contention [7][4].
Citations:
- 1: https://doc.rust-lang.org/src/std/fs.rs.html
- 2: https://doc.rust-lang.org/1.89.0/std/fs/struct.File.html
- 3: https://doc.rust-lang.org/stable/src/std/fs.rs.html
- 4: Tracking Issue for File lock API rust-lang/rust#130994
- 5: https://doc.rust-lang.org/beta/std/fs/struct.File.html
- 6: https://doc.rust-lang.org/stable/std/fs/enum.TryLockError.html
- 7: Change signature of File::try_lock and File::try_lock_shared rust-lang/rust#139343
Use std::fs::File’s native lock API instead of fs2.
Rust 1.89.0+ already provides try_lock(), try_lock_shared(), lock(), lock_shared(), and unlock() on std::fs::File, with TryLockError::WouldBlock unifying Unix/Windows lock contention. This removes the custom lock_is_held fallback and fs2 dependency from the file-lock path.
Also remove the fs2 workspace/Cargo dependency references after switching try_acquire to the std methods.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/fbuild-core/src/file_lock.rs` around lines 6 - 59, Replace the
fs2-based locking in try_acquire with Rust 1.89+ std::fs::File methods, using
try_lock_shared or try_lock according to FileLockMode and mapping
TryLockError::WouldBlock to Ok(None) while propagating other errors. Remove
lock_is_held and the fs2 import, then delete the related fs2 dependency
references from the workspace and Cargo manifests.
| #[cfg(all(unix, not(target_os = "linux")))] | ||
| pub fn pid_executable_path(pid: u32) -> Option<PathBuf> { | ||
| let output = std::process::Command::new("/bin/ps") | ||
| .args(["-p", &pid.to_string(), "-o", "comm="]) | ||
| .output() | ||
| .ok()?; | ||
| if !output.status.success() { | ||
| return None; | ||
| } | ||
| let image = String::from_utf8(output.stdout).ok()?; | ||
| let image = image.trim(); | ||
| (!image.is_empty()).then(|| PathBuf::from(image)) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Direct subprocess spawn flagged by CI lint.
The pipeline's subprocess-spawn lint fails on this line: std::process::Command::new("/bin/ps") is a direct spawn without the required allow-direct-spawn: <reason> marker or routing through fbuild_core::subprocess::{run_command,run_command_passthrough}. It's also unbounded — no timeout guards the ps invocation.
🔧 Minimal fix: annotate with an allow-marker (or route through the sanctioned helper)
pub fn pid_executable_path(pid: u32) -> Option<PathBuf> {
+ // allow-direct-spawn: reads a single process's `comm` name to verify PID
+ // identity before termination; no untrusted input, bounded one-shot call.
let output = std::process::Command::new("/bin/ps")
.args(["-p", &pid.to_string(), "-o", "comm="])
.output()
.ok()?;📝 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.
| #[cfg(all(unix, not(target_os = "linux")))] | |
| pub fn pid_executable_path(pid: u32) -> Option<PathBuf> { | |
| let output = std::process::Command::new("/bin/ps") | |
| .args(["-p", &pid.to_string(), "-o", "comm="]) | |
| .output() | |
| .ok()?; | |
| if !output.status.success() { | |
| return None; | |
| } | |
| let image = String::from_utf8(output.stdout).ok()?; | |
| let image = image.trim(); | |
| (!image.is_empty()).then(|| PathBuf::from(image)) | |
| } | |
| #[cfg(all(unix, not(target_os = "linux")))] | |
| pub fn pid_executable_path(pid: u32) -> Option<PathBuf> { | |
| // allow-direct-spawn: reads a single process's `comm` name to verify PID | |
| // identity before termination; no untrusted input, bounded one-shot call. | |
| let output = std::process::Command::new("/bin/ps") | |
| .args(["-p", &pid.to_string(), "-o", "comm="]) | |
| .output() | |
| .ok()?; | |
| if !output.status.success() { | |
| return None; | |
| } | |
| let image = String::from_utf8(output.stdout).ok()?; | |
| let image = image.trim(); | |
| (!image.is_empty()).then(|| PathBuf::from(image)) | |
| } |
🧰 Tools
🪛 GitHub Actions: Lint subprocess spawns / 0_Lint subprocess spawns.txt
[error] 77-77: Direct subprocess spawn without allow-direct-spawn: <reason> marker: std::process::Command::new("/bin/ps").
🪛 GitHub Actions: Lint subprocess spawns / Lint subprocess spawns
[error] 77-77: Direct subprocess spawn detected without required allow marker: std::process::Command::new("/bin/ps"). Annotate with a one-line reason (route via fbuild_core::subprocess::{run_command,run_command_passthrough} or add 'allow-direct-spawn: ').
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/fbuild-core/src/process_identity.rs` around lines 75 - 87, Update
pid_executable_path to satisfy the subprocess-spawn lint by routing the /bin/ps
invocation through
fbuild_core::subprocess::{run_command,run_command_passthrough}, or add an
allow-direct-spawn marker with a concise reason if direct Command usage must
remain. Ensure the ps execution is bounded with the existing supported timeout
mechanism before processing its output.
Source: Pipeline failures
| /// blocked, drops the lock, then confirms a second subprocess probe | ||
| /// succeeds. | ||
| #[test] | ||
| #[ignore = "subprocess helper for root_ownership_is_version_blind_across_processes"] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
#[ignore] reason must cite a tracking issue or hardware/toolchain requirement.
The reason describes the helper's purpose but cites neither a tracking issue nor a concrete hardware/toolchain need, unlike the sibling ignores in legacy_daemon_transition.rs (which append (#1159)).
Proposed fix
- #[ignore = "subprocess helper for root_ownership_is_version_blind_across_processes"]
+ #[ignore = "subprocess helper for root_ownership_is_version_blind_across_processes (`#1159`)"]As per coding guidelines: "Every Rust #[ignore] attribute must include a reason string in the form #[ignore = "..."]; the reason must cite a tracking issue or a concrete hardware/toolchain requirement."
📝 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.
| #[ignore = "subprocess helper for root_ownership_is_version_blind_across_processes"] | |
| #[ignore = "subprocess helper for root_ownership_is_version_blind_across_processes (`#1159`)"] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/fbuild-paths/src/daemon_ownership.rs` at line 293, Update the
#[ignore] attribute for root_ownership_is_version_blind_across_processes to
include a valid tracking issue reference or concrete hardware/toolchain
requirement, following the sibling ignore annotations’ convention such as
(`#1159`).
Source: Coding guidelines
| let run_probe = |expected: &str| { | ||
| let output = std::process::Command::new(std::env::current_exe().unwrap()) | ||
| .args([ | ||
| "--ignored", | ||
| "--exact", | ||
| "daemon_ownership::tests::subprocess_probe_root_owner", | ||
| "--nocapture", | ||
| ]) | ||
| .env("FBUILD_TEST_ROOT_OWNER_PATH", &path) | ||
| .env("FBUILD_TEST_ROOT_OWNER_EXPECT", expected) | ||
| .output() | ||
| .expect("run subprocess probe"); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
CI lint failure: direct subprocess spawn needs an allow-direct-spawn marker.
The subprocess-spawn linter fails the build here because std::process::Command::new(std::env::current_exe().unwrap()) is spawned without the required marker (the sibling spawns in legacy_daemon_transition.rs already carry one). Add a one-line reason comment immediately above the spawn.
Proposed fix
let run_probe = |expected: &str| {
+ // allow-direct-spawn: test driver re-invokes its own test binary as a root-owner probe.
let output = std::process::Command::new(std::env::current_exe().unwrap())📝 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.
| let run_probe = |expected: &str| { | |
| let output = std::process::Command::new(std::env::current_exe().unwrap()) | |
| .args([ | |
| "--ignored", | |
| "--exact", | |
| "daemon_ownership::tests::subprocess_probe_root_owner", | |
| "--nocapture", | |
| ]) | |
| .env("FBUILD_TEST_ROOT_OWNER_PATH", &path) | |
| .env("FBUILD_TEST_ROOT_OWNER_EXPECT", expected) | |
| .output() | |
| .expect("run subprocess probe"); | |
| let run_probe = |expected: &str| { | |
| // allow-direct-spawn: test driver re-invokes its own test binary as a root-owner probe. | |
| let output = std::process::Command::new(std::env::current_exe().unwrap()) | |
| .args([ | |
| "--ignored", | |
| "--exact", | |
| "daemon_ownership::tests::subprocess_probe_root_owner", | |
| "--nocapture", | |
| ]) | |
| .env("FBUILD_TEST_ROOT_OWNER_PATH", &path) | |
| .env("FBUILD_TEST_ROOT_OWNER_EXPECT", expected) | |
| .output() | |
| .expect("run subprocess probe"); |
🧰 Tools
🪛 GitHub Actions: Lint subprocess spawns / 0_Lint subprocess spawns.txt
[error] 310-310: Direct subprocess spawn without allow-direct-spawn: <reason> marker: std::process::Command::new(std::env::current_exe().unwrap()).
🪛 GitHub Actions: Lint subprocess spawns / Lint subprocess spawns
[error] 310-310: Direct subprocess spawn detected without required allow marker: std::process::Command::new(std::env::current_exe().unwrap()). Annotate with a one-line reason (route via fbuild_core::subprocess::{run_command,run_command_passthrough} or add 'allow-direct-spawn: ').
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/fbuild-paths/src/daemon_ownership.rs` around lines 309 - 320, Add the
required allow-direct-spawn marker comment immediately above the
std::process::Command::new call inside the run_probe closure, including a
concise reason for this intentional subprocess test probe. Leave the existing
command arguments and environment setup unchanged.
Source: Pipeline failures
1 participant
Implements the #1159 handoff: replaces the rejected two-lock design with soldr's daemon ownership strategy so
fbuild clean cachecan safely clear the shared zccache compiler-object root.What changed
fbuild_paths::daemon_ownership(new): version-blind, per-cache-rootroot-owner.lockheld by the daemon for its entire lifetime;spawn.lockspawn-herd single-flight; advisoryroot-owner.jsonclaim (pid/exe/version/mode/cache_root_key/port) — never authoritative without live-PID + exe-identity verification.fbuild_core::process_identity(new): pid liveness, executable-identity gates (fail closed on unverifiable images), bounded TERM→KILL termination — semantics copied from soldr'slifecycle.rs.ensure_daemon_running— one winner spawns and holds the lock through readiness; losers adopt the winner's daemon.fbuild clean cache: holds spawn election across the whole reset → validates cache identity (refuses cross-mode/FBUILD_DAEMON_PORTmismatches) → graceful stop (busy 409 is a hard refusal; lost response with the daemon actually gone is accepted) → legacy/rollout sweep (daemon-*.portdiscovery + identity-matched HTTP shutdown; claim/pid-file/status-json PID candidates displaced only afterpid_is_alive&& exe-stem verification) → exclusive root ownership as positive proof before deleting<root>/zccache→ daemon restored even when cleanup fails. Idempotent; packages/toolchains/archives retained.fbuild_core::file_lock: WindowsERROR_LOCK_VIOLATIONnow classified as lock contention instead of a hard error (fixed real test failures on Windows).Validation (local, per instruction to skip GHA)
soldr cargo clippy --workspace --all-targets -- -D warnings→ exit 0bash test(full unit suite) → exit 0 (one pre-existing flaky infbuild-packages-fetchstreaming_download_retries_chunk_stalls_five_times_without_outputpassed 3/3 on rerun; crate untouched by this PR)legacy_daemon_transition.rs— unverified/recycled PID never signaled; realfbuild-daemonblocksroot-owner.lockwhile alive and releases it on hard kill (--ignoredrun passed)try_acquire_atseams; real~/.fbuild/*/daemonunchanged after runs)Closes #1159. Closes #1154.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
cacheas a supported scope forfbuild clean.Documentation
Tests