Skip to content

fix(daemon): key project build-lock on NormalizedPath (#1274) - #1278

Merged
zackees merged 1 commit into
mainfrom
fix/1274-project-lock-normalized-key
Aug 7, 2026
Merged

fix(daemon): key project build-lock on NormalizedPath (#1274)#1278
zackees merged 1 commit into
mainfrom
fix/1274-project-lock-normalized-key

Conversation

@zackees

@zackees zackees commented Aug 7, 2026

Copy link
Copy Markdown
Member

Implements the bounded slice from #1274 (the replacement for the closed #1271).

The defect

DaemonContext::project_lock serializes concurrent builds of the same project. It was keyed on the raw client path:

// build/deploy/install_deps handlers:
let project_dir = PathBuf::from(&req.project_dir);   // never normalized
...
let lock = ctx.project_lock(&project_dir);           // DashMap<PathBuf, _>

So C:\proj and c:/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_locks on fbuild_core::path::NormalizedPath, whose Eq/Hash compare the case-folded, slash-normalized, UNC-stripped key. Lookups/removals/iteration are unchanged (NormalizedPath derefs to Path, so to_string_lossy() in the status handler still shows a readable, original-case path).

Scope — deliberately narrow (per #1274)

Only project_locks is migrated. The other path-keyed maps are safe by construction and stay PathBuf:

  • 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.
  • LDF walker visited/scan_cache — canonicalized before insert (already ban_std_fs_canonicalize-allowlisted).
  • rp2040 volume BTreeSets, library-select dedup sets — single-source, same-process.

Non-goals held: no blanket PathBufNormalizedPath swap, no ban_std_pathbuf allowlist churn, physical canonicalization untouched.

Verification

  • New regression test 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 the Check (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

  • Bug Fixes
    • Improved project locking so equivalent path spellings consistently share the same lock.
    • Ensured locking behavior respects case-sensitive and case-insensitive filesystem differences.
    • Prevented duplicate locks when the same project is referenced repeatedly.

…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>
@coderabbitai

coderabbitai Bot commented Aug 7, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 5bf0a546-c5b9-4ab8-bc74-7f7a2bddb233

📥 Commits

Reviewing files that changed from the base of the PR and between 59216a5 and fadb7e7.

📒 Files selected for processing (2)
  • crates/fbuild-daemon/src/context.rs
  • crates/fbuild-daemon/src/handlers/locks.rs

📝 Walkthrough

Walkthrough

The daemon now keys project locks by NormalizedPath. Project-lock lookup normalizes incoming paths so equivalent spellings share locks. Tests cover repeated requests and platform-specific case behavior.

Changes

Project lock normalization

Layer / File(s) Summary
Normalized lock storage and lookup
crates/fbuild-daemon/src/context.rs
DaemonContext::project_locks uses NormalizedPath keys. project_lock normalizes each project path before lookup.
Lock identity regression coverage
crates/fbuild-daemon/src/handlers/locks.rs
Existing lock tests use normalized keys. New coverage verifies equivalent path spellings and repeated requests share locks as required by the platform.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes changing daemon project build-lock keys to NormalizedPath.
Linked Issues check ✅ Passed The PR changes project lock identity to NormalizedPath and adds platform-specific regression coverage, satisfying the focused requirements of #1274.
Out of Scope Changes check ✅ Passed The changes are limited to project-lock key normalization and related regression tests, with no unrelated scope identified.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/1274-project-lock-normalized-key

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@zackees
zackees merged commit 23ce6a1 into main Aug 7, 2026
93 checks passed
@zackees
zackees deleted the fix/1274-project-lock-normalized-key branch August 7, 2026 11:02
@fastled-project-sync fastled-project-sync Bot moved this to Triage in FastLED Tracker Aug 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Triage

1 participant