fix(daemon): key project build-lock on NormalizedPath (#1274) - #1278
Merged
Conversation
…1274) The per-project build-serialization lock was keyed on the raw, client-supplied project dir (PathBuf::from(&req.project_dir), never normalized). Two requests naming one project with different casing or slash direction (C:\proj vs c:/proj) keyed distinct DashMap entries and received different locks — so two builds could run on the same project concurrently. This is the #436/#437 path-identity bug class as a correctness defect, not a cache annoyance. Key project_locks on fbuild_core::path::NormalizedPath, whose Eq/Hash use the case-folded, slash-normalized, UNC-stripped key. Display stays readable (NormalizedPath derefs to Path; the stored path preserves original casing). Scope is deliberately narrow (see #1274): other path-keyed maps (image_hash_memo, compiler identity cache, LDF walker caches) are safe by construction — daemon-internal, single-source, or canonicalized before insert — and are left as PathBuf. Regression test asserts two spellings of one project share a lock on case-insensitive platforms (Windows/macOS CI) and stay independent on case-sensitive ones. Co-Authored-By: Claude <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe daemon now keys project locks by ChangesProject lock normalization
Estimated code review effort: 2 (Simple) | ~10 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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements the bounded slice from #1274 (the replacement for the closed #1271).
The defect
DaemonContext::project_lockserializes concurrent builds of the same project. It was keyed on the raw client path:So
C:\projandc:/proj(or a UNC-prefixed spelling) key different DashMap entries → different locks → two builds run on one project concurrently. That's the #436/#437 identity bug class, here as a real concurrency hole rather than a stale cache entry.Fix
Key
project_locksonfbuild_core::path::NormalizedPath, whoseEq/Hashcompare the case-folded, slash-normalized, UNC-stripped key. Lookups/removals/iteration are unchanged (NormalizedPathderefs toPath, soto_string_lossy()in the status handler still shows a readable, original-case path).Scope — deliberately narrow (per #1274)
Only
project_locksis migrated. The other path-keyed maps are safe by construction and stayPathBuf:image_hash_memo— keyed on a daemon-derived firmware path, get/insert use the same value.COMPILER_IDENTITY_CACHE— compiler binary path resolved consistently in-process.visited/scan_cache— canonicalized before insert (alreadyban_std_fs_canonicalize-allowlisted).rp2040volumeBTreeSets,library-selectdedup sets — single-source, same-process.Non-goals held: no blanket
PathBuf→NormalizedPathswap, noban_std_pathbufallowlist churn, physical canonicalization untouched.Verification
project_lock_keys_on_normalized_identity_not_raw_bytes: on Windows/macOS the two spellings share one lock (Arc::ptr_eq,len()==1); on Linux they stay independent (correct for a case-sensitive FS). The Win/mac assertion runs on theCheck (windows-latest)/Check (macos-latest)CI jobs.soldr cargo test -p fbuild-daemon --lib project_lock→ 5 passed.soldr cargo clippy -p fbuild-daemon --all-targets -- -D warnings→ clean.Closes #1274.
🤖 Generated with Claude Code
Summary by CodeRabbit