From fadb7e717c24a431e1a03c697c67068ff41fe3a5 Mon Sep 17 00:00:00 2001 From: zackees Date: Fri, 7 Aug 2026 03:37:28 -0700 Subject: [PATCH] fix(daemon): key project build-lock on NormalizedPath, not raw path (#1274) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- crates/fbuild-daemon/src/context.rs | 15 ++++++-- crates/fbuild-daemon/src/handlers/locks.rs | 41 +++++++++++++++++++--- 2 files changed, 50 insertions(+), 6 deletions(-) diff --git a/crates/fbuild-daemon/src/context.rs b/crates/fbuild-daemon/src/context.rs index f05a96d46..b2abb4209 100644 --- a/crates/fbuild-daemon/src/context.rs +++ b/crates/fbuild-daemon/src/context.rs @@ -5,6 +5,7 @@ use crate::status_manager::StatusManager; use dashmap::DashMap; use fbuild_core::DaemonState; use fbuild_core::install_status::InstallStatus; +use fbuild_core::path::NormalizedPath; use fbuild_serial::SharedSerialManager; use std::path::PathBuf; use std::sync::atomic::{AtomicBool, AtomicU64, AtomicUsize}; @@ -162,7 +163,17 @@ pub struct DaemonContext { /// Latest dependency/package install status emitted by lower-level crates. pub dependency_install: Arc>>, /// Per-project build locks to serialize builds on the same project. - pub project_locks: DashMap>>, + /// + /// Keyed on [`NormalizedPath`], not raw `PathBuf`: the project dir + /// arrives as an unnormalized client-supplied string + /// (`PathBuf::from(&req.project_dir)` in the build/deploy handlers), so + /// two requests naming one project with different casing or slash + /// direction (`C:\proj` vs `c:/proj`) would key distinct raw-path + /// entries and get *different* locks — letting two builds run on the + /// same project concurrently. `NormalizedPath`'s case-folded, + /// slash-normalized key collapses those to one lock. See + /// FastLED/fbuild#1274 (and the #436/#437 identity bug class). + pub project_locks: DashMap>>, /// Device lease manager. /// /// Wrapped in `Arc` (FastLED/fbuild#808) so refresh paths that call @@ -441,7 +452,7 @@ impl DaemonContext { /// Get or create a per-project lock. pub fn project_lock(&self, project_dir: &std::path::Path) -> Arc> { self.project_locks - .entry(project_dir.to_path_buf()) + .entry(NormalizedPath::from(project_dir)) .or_insert_with(|| Arc::new(Mutex::new(()))) .clone() } diff --git a/crates/fbuild-daemon/src/handlers/locks.rs b/crates/fbuild-daemon/src/handlers/locks.rs index 687d0bb51..91b42de9d 100644 --- a/crates/fbuild-daemon/src/handlers/locks.rs +++ b/crates/fbuild-daemon/src/handlers/locks.rs @@ -330,8 +330,9 @@ pub async fn clear_locks( #[cfg(test)] mod tests { use super::*; + use fbuild_core::path::NormalizedPath; use fbuild_serial::{SerialClientMetadata, SerialSession}; - use std::path::PathBuf; + use std::path::{Path, PathBuf}; use tokio::sync::Mutex; fn test_context() -> Arc { @@ -367,7 +368,7 @@ mod tests { #[tokio::test] async fn lock_status_reports_held_project_locks_without_stale_entries() { let ctx = test_context(); - let project = PathBuf::from("/tmp/fbuild-held-project"); + let project = NormalizedPath::from(PathBuf::from("/tmp/fbuild-held-project")); let lock = Arc::new(Mutex::new(())); let guard = lock.lock().await; ctx.project_locks.insert(project.clone(), lock.clone()); @@ -429,8 +430,8 @@ mod tests { #[tokio::test] async fn clear_locks_removes_only_unheld_project_lock_entries() { let ctx = test_context(); - let unheld = PathBuf::from("/tmp/fbuild-unheld-project"); - let held = PathBuf::from("/tmp/fbuild-held-project"); + let unheld = NormalizedPath::from(PathBuf::from("/tmp/fbuild-unheld-project")); + let held = NormalizedPath::from(PathBuf::from("/tmp/fbuild-held-project")); let held_lock = Arc::new(Mutex::new(())); let guard = held_lock.lock().await; ctx.project_locks @@ -449,6 +450,38 @@ mod tests { drop(guard); } + /// Regression for FastLED/fbuild#1274 (identity bug class #436/#437): + /// the build-serialization lock is keyed on the *raw* client-supplied + /// project dir, which is never normalized. Two spellings of one project + /// must resolve to the SAME lock or two builds could run concurrently. + #[tokio::test] + async fn project_lock_keys_on_normalized_identity_not_raw_bytes() { + let ctx = test_context(); + let mixed = ctx.project_lock(Path::new("/Tmp/FBuild/Proj")); + let lower = ctx.project_lock(Path::new("/tmp/fbuild/proj")); + + if cfg!(any(windows, target_os = "macos")) { + // Case-insensitive filesystem: one logical project, so the two + // spellings must share a single serialization lock. Before the + // fix these keyed distinct raw-`PathBuf` entries and returned + // different `Arc`s — the concurrency hole this test guards. + assert!( + Arc::ptr_eq(&mixed, &lower), + "same project in different case must share one lock" + ); + assert_eq!(ctx.project_locks.len(), 1, "must not create a second lock"); + } else { + // Case-sensitive filesystem: these are genuinely different + // directories and correctly get independent locks. + assert!(!Arc::ptr_eq(&mixed, &lower)); + } + + // Re-requesting the exact same path always returns the same lock, + // on every platform. + let again = ctx.project_lock(Path::new("/tmp/fbuild/proj")); + assert!(Arc::ptr_eq(&lower, &again)); + } + #[tokio::test] async fn clear_locks_refuses_live_serial_session_without_force() { let ctx = test_context();