diff --git a/crates/fbuild-cli/src/cli/daemon_cmd.rs b/crates/fbuild-cli/src/cli/daemon_cmd.rs index 6e3e85a0..55d6b144 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 5ce24875..120719d1 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-library/src/library/esptool.rs b/crates/fbuild-library/src/library/esptool.rs index 57e3f783..7b15375a 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 3c0bc7ef..a2aa010f 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"); diff --git a/crates/fbuild-paths/src/lib.rs b/crates/fbuild-paths/src/lib.rs index affd4df2..6ad1fc32 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,