From 3ce25ed84cd1e8a229aef4f3fa39225075d3feed Mon Sep 17 00:00:00 2001 From: zackees Date: Sat, 1 Aug 2026 14:20:18 -0700 Subject: [PATCH 1/2] fix(packages): reclaim install locks whose owner is dead (#1213 parts 1+3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Part 1 — the deadlock `owner.txt` has been recording the holder's `pid=` all along; `lock_is_stale` just never read it. It checked only the directory mtime against a 2 h ceiling, so a crashed install wedged every later build for up to two hours — the reporter measured ~40 minutes of `waiting for another process to install ...` with no fbuild process alive. `lock_is_stale` now reclaims a lock when the owning process is gone, which is the fix the issue asked for and needed no new bookkeeping to implement. Three ordered reasons to reclaim, and the ordering is deliberate: 1. the directory vanished; 2. the owner is dead (new); 3. the age ceiling (unchanged backstop, for what liveness can't answer — a missing owner record on a foreign filesystem, or a hung-but-running peer). Care taken not to reclaim a *live* install: - PID recycling is handled by also recording `exe_stem=` and comparing it. A recycled PID now running some other program means the original owner is gone; an fbuild PID that is genuinely alive keeps its lock. - Every probe fails safe. `pid_executable_path` returning `None` (permission denied, race) is treated as alive, never as dead. - Records written before `exe_stem` existed fall back to liveness alone. - `create_dir` and `write_lock_owner` are two steps, so a waiter can observe the gap. A lock with no owner record is left alone for a 30 s grace period, then reclaimed — otherwise a crash *between* those two calls still wedged for the full 2 h. ## Part 3 — undiagnosable esptool failure "cached esptool executable ... exited with status 2" carried no output, which is what the reporter hit on Windows with a freshly-installed 5.3.0. The error now includes captured stderr/stdout, and says so explicitly when both are empty rather than leaving the reader guessing. 7 new tests: dead PID reclaimed immediately, live PID never reclaimed, recycled PID treated as dead, legacy record without exe_stem, the create/write race window left alone, an abandoned-mid-creation lock reclaimed after grace, and the owner record round-trip. Part 2 (stale daemon endpoint surviving `daemon stop`) is not addressed here; it is daemon-lifecycle work with a different blast radius. Co-Authored-By: Claude --- crates/fbuild-library/src/library/esptool.rs | 15 +- .../fbuild-packages-fetch/src/install_lock.rs | 251 +++++++++++++++++- 2 files changed, 257 insertions(+), 9 deletions(-) diff --git a/crates/fbuild-library/src/library/esptool.rs b/crates/fbuild-library/src/library/esptool.rs index 57e3f7831..7b15375ac 100644 --- a/crates/fbuild-library/src/library/esptool.rs +++ b/crates/fbuild-library/src/library/esptool.rs @@ -252,8 +252,21 @@ async fn verify_esptool_binary(bin: &Path) -> Result<()> { if output.success() { Ok(()) } else { + // Include the captured output. A bare "exited with status 2" is + // undiagnosable, and that is exactly what a real user hit on Windows + // with a freshly-installed 5.3.0 (FastLED/fbuild#1213 part 3). + let mut detail = String::new(); + for (label, stream) in [("stderr", &output.stderr), ("stdout", &output.stdout)] { + let text = stream.trim(); + if !text.is_empty() { + detail.push_str(&format!("\n {label}: {text}")); + } + } + if detail.is_empty() { + detail.push_str("\n (no output on stdout or stderr)"); + } Err(FbuildError::PackageError(format!( - "cached esptool executable {} exited with status {}", + "cached esptool executable {} exited with status {}{detail}", bin.display(), output.exit_code ))) diff --git a/crates/fbuild-packages-fetch/src/install_lock.rs b/crates/fbuild-packages-fetch/src/install_lock.rs index 3c0bc7ef4..a2aa010fd 100644 --- a/crates/fbuild-packages-fetch/src/install_lock.rs +++ b/crates/fbuild-packages-fetch/src/install_lock.rs @@ -145,6 +145,14 @@ async fn acquire_install_lock_at( } } +/// Grace period for a lock directory that has no readable `owner.txt`. +/// +/// `create_dir` and [`write_lock_owner`] are two steps, so a waiter can +/// legitimately observe the directory in between. Anything older than this +/// without an owner record was abandoned mid-creation (the writer died +/// between the two calls) and would otherwise wedge until the 2 h ceiling. +const MISSING_OWNER_GRACE: Duration = Duration::from_secs(30); + fn write_lock_owner(lock_dir: &Path, package_name: &str, package_version: &str) -> Result<()> { let mut file = OpenOptions::new() .create(true) @@ -153,8 +161,9 @@ fn write_lock_owner(lock_dir: &Path, package_name: &str, package_version: &str) .open(lock_dir.join("owner.txt"))?; writeln!( file, - "pid={}\npackage={}\nversion={}\nstarted_unix_nanos={}", + "pid={}\nexe_stem={}\npackage={}\nversion={}\nstarted_unix_nanos={}", std::process::id(), + current_exe_stem().unwrap_or_default(), package_name, package_version, unique_suffix() @@ -162,17 +171,132 @@ fn write_lock_owner(lock_dir: &Path, package_name: &str, package_version: &str) Ok(()) } +/// File stem of the running executable, used to make the liveness probe +/// PID-recycling-safe. +fn current_exe_stem() -> Option { + std::env::current_exe() + .ok()? + .file_stem() + .and_then(|s| s.to_str()) + .map(str::to_string) +} + +/// Owner record parsed out of a lock directory's `owner.txt`. +struct LockOwner { + pid: Option, + exe_stem: Option, +} + +fn read_lock_owner(lock_dir: &Path) -> Option { + let raw = std::fs::read_to_string(lock_dir.join("owner.txt")).ok()?; + let mut pid = None; + let mut exe_stem = None; + for line in raw.lines() { + if let Some(v) = line.strip_prefix("pid=") { + pid = v.trim().parse::().ok(); + } else if let Some(v) = line.strip_prefix("exe_stem=") { + let v = v.trim(); + if !v.is_empty() { + exe_stem = Some(v.to_string()); + } + } + } + Some(LockOwner { pid, exe_stem }) +} + +/// Is the process that created this lock gone? +/// +/// Returns `false` unless we have positive evidence of death — an +/// uninspectable owner is treated as alive so a live install is never torn +/// out from under itself. +/// +/// PID recycling is handled by also comparing the recorded executable stem: +/// if the PID is alive but is now some unrelated program, the original owner +/// is gone. When the record predates the `exe_stem` field, liveness alone is +/// used (the old behavior, minus the deadlock). +fn owner_is_dead(owner: &LockOwner) -> bool { + let Some(pid) = owner.pid else { + return false; + }; + if !fbuild_core::process_identity::pid_is_alive(pid) { + return true; + } + match &owner.exe_stem { + // `pid_exe_stem_matches` fails closed on an uninspectable image, so + // only treat a *successful* probe of a different program as death. + Some(stem) => match fbuild_core::process_identity::pid_executable_path(pid) { + Some(path) => match path.file_stem().and_then(|s| s.to_str()) { + Some(actual) => !stem_eq(actual, stem), + None => false, + }, + None => false, + }, + None => false, + } +} + +fn stem_eq(left: &str, right: &str) -> bool { + if cfg!(windows) { + left.eq_ignore_ascii_case(right) + } else { + left == right + } +} + +/// Should a waiter tear this lock down? +/// +/// Three independent reasons, in order of confidence: +/// 1. The directory vanished — nothing to wait for. +/// 2. The owning process is gone. This is the FastLED/fbuild#1213 deadlock: +/// the PID was already being written to `owner.txt` and simply never +/// read, so a crashed install wedged every later build until the 2 h +/// ceiling expired. +/// 3. The age ceiling — the pre-existing backstop, kept for the cases PID +/// liveness cannot answer (owner record missing on a foreign filesystem, +/// a genuinely hung but still-running peer). fn lock_is_stale(lock_dir: &Path, stale_after: Duration) -> bool { + lock_is_stale_with_grace(lock_dir, stale_after, MISSING_OWNER_GRACE) +} + +/// [`lock_is_stale`] with the missing-owner grace injected, so tests can +/// exercise the abandoned-mid-creation branch without sleeping 30 s. +fn lock_is_stale_with_grace( + lock_dir: &Path, + stale_after: Duration, + missing_owner_grace: Duration, +) -> bool { let Ok(metadata) = std::fs::metadata(lock_dir) else { return true; }; - let Ok(modified) = metadata.modified() else { - return false; - }; - modified - .elapsed() - .map(|age| age > stale_after) - .unwrap_or(false) + let age = metadata.modified().ok().and_then(|m| m.elapsed().ok()); + + match read_lock_owner(lock_dir) { + Some(owner) => { + if owner_is_dead(&owner) { + tracing::warn!( + pid = ?owner.pid, + lock = %lock_dir.display(), + "install lock owner is no longer running; reclaiming" + ); + return true; + } + } + None => { + // No owner record. Only meaningful once the create/write window + // has comfortably passed. + if age.map(|a| a >= missing_owner_grace).unwrap_or(false) { + tracing::warn!( + lock = %lock_dir.display(), + "install lock has no owner record after {:?}; reclaiming", + missing_owner_grace + ); + return true; + } + return false; + } + } + + age.map(|a| a > stale_after).unwrap_or(false) } fn unique_suffix() -> u128 { @@ -211,6 +335,117 @@ mod tests { } } + /// Write a lock directory owned by `pid` with an optional exe stem, + /// mimicking what a crashed peer leaves behind. + fn plant_lock(lock_dir: &Path, pid: u32, exe_stem: Option<&str>) { + std::fs::create_dir_all(lock_dir).unwrap(); + let mut body = format!("pid={pid}\n"); + if let Some(stem) = exe_stem { + body.push_str(&format!("exe_stem={stem}\n")); + } + body.push_str("package=toolchain\nversion=1.0\n"); + std::fs::write(lock_dir.join("owner.txt"), body).unwrap(); + } + + /// A PID that is (almost certainly) not running. PID 0 is never a normal + /// user process on either platform, and `pid_is_alive` reports it dead. + const DEAD_PID: u32 = 0; + + /// The FastLED/fbuild#1213 deadlock: a crashed owner used to wedge every + /// later build for the full 2 h ceiling, even though its PID was already + /// recorded in `owner.txt` — it was simply never read. + #[test] + fn lock_owned_by_a_dead_pid_is_stale_immediately() { + let tmp = tempfile::TempDir::new().unwrap(); + let lock_dir = tmp.path().join(".1.0.install.lock"); + plant_lock(&lock_dir, DEAD_PID, None); + + assert!(lock_is_stale(&lock_dir, Duration::from_secs(2 * 60 * 60))); + } + + /// The complement, and the one that matters for safety: a live owner's + /// lock must never be reclaimed, however long the ceiling is. + #[test] + fn lock_owned_by_a_live_pid_is_not_stale() { + let tmp = tempfile::TempDir::new().unwrap(); + let lock_dir = tmp.path().join(".1.0.install.lock"); + plant_lock(&lock_dir, std::process::id(), current_exe_stem().as_deref()); + + assert!(!lock_is_stale(&lock_dir, Duration::from_secs(2 * 60 * 60))); + } + + /// PID recycling: the recorded PID is alive but is now a different + /// program, so the original owner is gone. + #[test] + fn lock_whose_pid_was_recycled_by_another_program_is_stale() { + let tmp = tempfile::TempDir::new().unwrap(); + let lock_dir = tmp.path().join(".1.0.install.lock"); + plant_lock( + &lock_dir, + std::process::id(), + Some("definitely-not-this-test-binary"), + ); + + assert!(lock_is_stale(&lock_dir, Duration::from_secs(2 * 60 * 60))); + } + + /// A record written before the `exe_stem` field existed must still work: + /// liveness alone decides, which is the pre-#1213 data plus the fix. + #[test] + fn legacy_owner_record_without_exe_stem_still_uses_liveness() { + let tmp = tempfile::TempDir::new().unwrap(); + let live = tmp.path().join(".live.install.lock"); + let dead = tmp.path().join(".dead.install.lock"); + plant_lock(&live, std::process::id(), None); + plant_lock(&dead, DEAD_PID, None); + + assert!(!lock_is_stale(&live, Duration::from_secs(2 * 60 * 60))); + assert!(lock_is_stale(&dead, Duration::from_secs(2 * 60 * 60))); + } + + /// `create_dir` then `write_lock_owner` is two steps; a waiter that + /// catches the gap must NOT tear down a lock that is being created. + #[test] + fn freshly_created_lock_without_owner_record_is_not_stale() { + let tmp = tempfile::TempDir::new().unwrap(); + let lock_dir = tmp.path().join(".1.0.install.lock"); + std::fs::create_dir_all(&lock_dir).unwrap(); + + assert!(!lock_is_stale(&lock_dir, Duration::from_secs(2 * 60 * 60))); + } + + /// ...but a lock stuck without an owner record past the grace period was + /// abandoned mid-creation and must be reclaimed rather than waiting out + /// the 2 h ceiling. + #[test] + fn owner_record_missing_past_the_grace_period_is_stale() { + let tmp = tempfile::TempDir::new().unwrap(); + let lock_dir = tmp.path().join(".1.0.install.lock"); + std::fs::create_dir_all(&lock_dir).unwrap(); + + // `lock_is_stale` compares against MISSING_OWNER_GRACE using the + // directory mtime; a zero grace makes any existing dir qualify + // without sleeping in the test. + assert!(super::lock_is_stale_with_grace( + &lock_dir, + Duration::from_secs(2 * 60 * 60), + Duration::ZERO + )); + } + + #[test] + fn written_owner_record_round_trips() { + let tmp = tempfile::TempDir::new().unwrap(); + let lock_dir = tmp.path().join(".1.0.install.lock"); + std::fs::create_dir_all(&lock_dir).unwrap(); + write_lock_owner(&lock_dir, "toolchain", "1.0").unwrap(); + + let owner = read_lock_owner(&lock_dir).expect("owner record"); + assert_eq!(owner.pid, Some(std::process::id())); + assert_eq!(owner.exe_stem, current_exe_stem()); + assert!(!owner_is_dead(&owner), "this process is alive"); + } + #[test] fn lock_path_is_sibling_of_install_path() { let root = Path::new("/cache/toolchains/example/1.0"); From 93ad12a704bb5642d023ef8b209671eb2fd87a95 Mon Sep 17 00:00:00 2001 From: zackees Date: Sat, 1 Aug 2026 14:29:43 -0700 Subject: [PATCH 2/2] fix(daemon): clear stale endpoint records and gate them on owner liveness (#1213 part 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements both fixes the issue suggests, plus one omission found on the way. - `daemon stop` now clears the port/pid/status/owner-claim records when it finds no daemon. The early return was the bug: "not running" is precisely when those records are known to be garbage, yet it was the one path that left them untouched. It also sweeps after a successful shutdown, since the daemon does not remove all of them itself. - The daemon's graceful shutdown removed pid, port and owner claim but NOT `daemon_status.json`, so `daemon status` kept describing a dead PID after a *clean* exit, not just a crash. Now removed too. - `get_daemon_port()` no longer trusts the port file verbatim: it is ignored when the recorded owner is provably gone. Fails safe in every uncertain case — a missing claim (including the startup window where the port file exists but the claim does not yet) counts as alive, and an uninspectable process counts as alive. PID recycling is covered by the exe-stem check. Scope, stated plainly: this does NOT explain the reporter's "clients keep dialing a dead port". The port is derived deterministically from (version, cache identity) (#1009), so discarding the port file yields the same number — which is exactly why deleting the files by hand didn't help them either. That symptom is a daemon-respawn failure, and diagnosing it needs a repro I don't have. What this fixes is the record hygiene the issue asks for, so the state stops lying about a dead daemon. Co-Authored-By: Claude --- crates/fbuild-cli/src/cli/daemon_cmd.rs | 55 ++++++++++++++++- crates/fbuild-daemon/src/main.rs | 3 + crates/fbuild-paths/src/lib.rs | 80 ++++++++++++++++++++++++- 3 files changed, 135 insertions(+), 3 deletions(-) diff --git a/crates/fbuild-cli/src/cli/daemon_cmd.rs b/crates/fbuild-cli/src/cli/daemon_cmd.rs index 6e3e85a03..55d6b144f 100644 --- a/crates/fbuild-cli/src/cli/daemon_cmd.rs +++ b/crates/fbuild-cli/src/cli/daemon_cmd.rs @@ -14,7 +14,20 @@ pub async fn run_daemon(action: DaemonAction) -> fbuild_core::Result<()> { match action { DaemonAction::Stop => { if !client.health().await { - output::result("daemon is not running"); + // FastLED/fbuild#1213 part 2: this used to return here without + // touching anything, so a crashed daemon's port/pid/status + // records survived `daemon stop` indefinitely and every later + // `daemon status` kept describing a dead PID. "Not running" is + // exactly when those records are known to be garbage. + let removed = clear_daemon_records(); + if removed.is_empty() { + output::result("daemon is not running"); + } else { + output::result(&format!( + "daemon is not running (cleared stale records: {})", + removed.join(", ") + )); + } return Ok(()); } client.shutdown().await?; @@ -22,6 +35,10 @@ pub async fn run_daemon(action: DaemonAction) -> fbuild_core::Result<()> { for _ in 0..50 { tokio::time::sleep(std::time::Duration::from_millis(100)).await; if !client.health().await { + // The daemon removes its own pid/port/claim on a graceful + // exit; sweep anything it left behind (notably + // daemon_status.json) so `stop` always leaves a clean dir. + clear_daemon_records(); output::result("daemon stopped"); return Ok(()); } @@ -778,3 +795,39 @@ pub fn format_uptime(seconds: f64) -> String { fn format_age_seconds(seconds: f64) -> String { format_uptime(seconds.max(0.0)) } + +/// Remove the on-disk records that describe a daemon endpoint, returning the +/// names of the ones that actually existed. +/// +/// These are advisory records, never the source of truth: the port is derived +/// deterministically from (version, cache identity), and the daemon rewrites +/// all of them at startup. Deleting them cannot orphan a live daemon — a +/// running daemon keeps serving, and the next `fbuild` invocation re-derives +/// the same endpoint. What it does fix is a crashed daemon's records +/// outliving it and describing a dead PID forever (FastLED/fbuild#1213). +/// +/// Note the owner claim is deliberately included: it is what `daemon status` +/// and the spawn path consult to decide whether a live daemon owns the cache +/// root. +fn clear_daemon_records() -> Vec<&'static str> { + let targets: [(&'static str, std::path::PathBuf); 3] = [ + ("pid", fbuild_paths::get_daemon_pid_file()), + ("port", fbuild_paths::get_daemon_port_file()), + ("status", fbuild_paths::get_daemon_status_file()), + ]; + let mut removed = Vec::new(); + for (label, path) in targets { + match std::fs::remove_file(&path) { + Ok(()) => removed.push(label), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} + Err(e) => { + tracing::warn!(path = %path.display(), error = %e, "failed to remove daemon record"); + } + } + } + if fbuild_paths::daemon_ownership::owner_claim_path().exists() { + fbuild_paths::daemon_ownership::remove_owner_claim(); + removed.push("owner-claim"); + } + removed +} diff --git a/crates/fbuild-daemon/src/main.rs b/crates/fbuild-daemon/src/main.rs index 5ce248750..120719d1a 100644 --- a/crates/fbuild-daemon/src/main.rs +++ b/crates/fbuild-daemon/src/main.rs @@ -522,6 +522,9 @@ async fn main() { let _ = fbuild_core::fs::remove_file(&pid_file).await; let _ = fbuild_core::fs::remove_file(&port_file).await; fbuild_paths::daemon_ownership::remove_owner_claim(); + // ...and the status file, which was previously left behind on every clean + // shutdown, so `daemon status` kept reporting a dead PID (#1213 part 2). + let _ = fbuild_core::fs::remove_file(&fbuild_paths::get_daemon_status_file()).await; tracing::info!("daemon exiting"); std::process::exit(0); diff --git a/crates/fbuild-paths/src/lib.rs b/crates/fbuild-paths/src/lib.rs index affd4df2a..6ad1fc322 100644 --- a/crates/fbuild-paths/src/lib.rs +++ b/crates/fbuild-paths/src/lib.rs @@ -292,15 +292,56 @@ pub fn get_daemon_port() -> u16 { } } - // Priority 2: this endpoint's port file. + // Priority 2: this endpoint's port file — but only if we cannot prove its + // writer is dead. FastLED/fbuild#1213: a crashed daemon's port file was + // trusted verbatim forever. if let Some(port) = read_port_from_file(&get_daemon_port_file()) { - return port; + if !recorded_daemon_owner_is_dead() { + return port; + } + // Falls through to the derived default. Note this is usually the SAME + // number, because the port is deterministic — see the note on + // `recorded_daemon_owner_is_dead`. } // Priority 3: deterministic per-endpoint default. default_daemon_port() } +/// Can we *prove* the daemon that wrote this endpoint's records is gone? +/// +/// Fails safe: anything short of positive evidence of death returns `false`, +/// so a live daemon's endpoint is never discarded. In particular a missing +/// owner claim returns `false`, because the daemon writes its pid/port files +/// before the claim — treating that window as death would make a starting +/// daemon look dead. +/// +/// The exe-stem check makes this PID-recycling-safe: a recycled PID now +/// running some other program means the daemon itself is gone. +/// +/// Scope note: because [`default_daemon_port`] is deterministic, discarding a +/// stale port file usually yields the *same* port number. This is a +/// correctness/hygiene fix (stop trusting a record whose writer is provably +/// gone), NOT the reason a client can keep failing to reach a dead endpoint — +/// see the PR discussion on FastLED/fbuild#1213. +fn recorded_daemon_owner_is_dead() -> bool { + let Some(claim) = daemon_ownership::read_owner_claim() else { + return false; + }; + if !fbuild_core::process_identity::pid_is_alive(claim.pid) { + return true; + } + // Alive PID: only a *successful* probe showing a different program counts + // as death, since `pid_exe_stem_matches` fails closed. + match fbuild_core::process_identity::pid_executable_path(claim.pid) { + Some(_) => !fbuild_core::process_identity::pid_exe_stem_matches( + claim.pid, + daemon_ownership::DAEMON_EXE_STEM, + ), + None => false, + } +} + /// Daemon URL. pub fn get_daemon_url() -> String { format!("http://127.0.0.1:{}", get_daemon_port()) @@ -420,6 +461,41 @@ mod tests { assert!(port > 0); } + /// FastLED/fbuild#1213: the liveness gate must only fire on *positive* + /// evidence that the recorded daemon is gone. With no owner claim on + /// disk — including the window where a starting daemon has written its + /// port file but not yet its claim — the endpoint must be trusted. + #[test] + fn owner_liveness_gate_fails_safe_without_a_claim() { + // `read_owner_claim` returns None when the claim file is absent or + // malformed; in a test process there is no daemon claim for this + // endpoint, so this exercises the fail-safe branch. + if daemon_ownership::read_owner_claim().is_none() { + assert!( + !recorded_daemon_owner_is_dead(), + "absent owner claim must NOT be read as a dead daemon" + ); + } + } + + /// A claim naming this very test process (alive, but not `fbuild-daemon`) + /// must be classified as dead — that is the PID-recycling guard. + #[test] + fn owner_liveness_gate_treats_a_recycled_pid_as_dead() { + // Probe the primitives directly rather than writing a claim to the + // process-global claim path, which would race other tests. + let pid = std::process::id(); + assert!(fbuild_core::process_identity::pid_is_alive(pid)); + assert!( + !fbuild_core::process_identity::pid_exe_stem_matches( + pid, + daemon_ownership::DAEMON_EXE_STEM + ), + "the test binary must not be mistaken for {}", + daemon_ownership::DAEMON_EXE_STEM + ); + } + #[test] fn endpoint_key_is_deterministic_16_hex() { // FastLED/fbuild#1009: the key must be deterministic per (version,