From 7a29f1be7d20036258df5770bcc93a88dd6836ce Mon Sep 17 00:00:00 2001 From: Zach Vorhies Date: Wed, 28 Oct 2026 17:05:08 -0700 Subject: [PATCH] fix(cli): retire a leftover daemon and retry once instead of failing (#1627) Three failed spawn attempts were terminal even when the cause was a leftover daemon from an earlier step still owning the cache root (root-owner.lock busy) or a wedged one owning the endpoint (#1360). #1732 named the cause and #1734 widened the readiness budget; this acts on it. daemon_stop.rs gains SpawnRecovery + plan_spawn_recovery (endpoint answers -> Adopt; silent with a verified live fbuild-daemon PID -> RetireAndRetry; silent with nothing alive -> GiveUp) and retire_unresponsive_for_recovery, which terminates the PID via the same terminate_and_confirm escalation `fbuild daemon stop` uses. start_daemon calls it after the attempts are exhausted and, when a daemon was retired, makes exactly one final spawn. Identity is fail-closed (recorded_daemon_pid requires the image stem to be fbuild-daemon), a daemon that answers is never retired, and the scope is this endpoint only, so a parallel build is safe. Additive: the path is a hard failure today. --- crates/fbuild-cli/src/cli/daemon_stop.rs | 66 +++++++++++++++++++ .../fbuild-cli/src/cli/daemon_stop_tests.rs | 24 +++++++ crates/fbuild-cli/src/daemon_client.rs | 39 ++++++++++- 3 files changed, 128 insertions(+), 1 deletion(-) diff --git a/crates/fbuild-cli/src/cli/daemon_stop.rs b/crates/fbuild-cli/src/cli/daemon_stop.rs index dfce1a8a5..2c82d377a 100644 --- a/crates/fbuild-cli/src/cli/daemon_stop.rs +++ b/crates/fbuild-cli/src/cli/daemon_stop.rs @@ -80,6 +80,72 @@ const GRACEFUL_STOP_BUDGET: std::time::Duration = std::time::Duration::from_secs /// is not going. const TERMINATION_BUDGET: std::time::Duration = std::time::Duration::from_secs(5); +/// What a CLI that has just failed every spawn attempt should do next +/// (FastLED/fbuild#1627). +/// +/// Three failed attempts used to be terminal. Two of the three reported causes +/// are recoverable without a human: the endpoint came up late (`Adopt` — the +/// same adoption the spawn loop already performs before each attempt), or a +/// leftover daemon from an earlier step still owns the cache root / endpoint +/// and is no longer answering (`RetireAndRetry`). Only "nothing is alive and +/// nothing answers" is genuinely hopeless at this point. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum SpawnRecovery { + /// The endpoint answers after all: attach to it, spawn nothing. + Adopt, + /// A PID verified to be an fbuild-daemon is alive while the endpoint is + /// silent. Retire it, then make one final spawn attempt. + RetireAndRetry(u32), + /// Nothing alive to retire and nothing answering: report the failure. + GiveUp, +} + +/// Decide the post-failure step. `live_daemon_pid` comes from +/// [`recorded_daemon_pid`], so it is already fail-closed on process identity. +pub(crate) fn plan_spawn_recovery( + endpoint_healthy: bool, + live_daemon_pid: Option, +) -> SpawnRecovery { + match (endpoint_healthy, live_daemon_pid) { + // Never shoot a daemon that answers: it is serving somebody, and + // adopting it is exactly what the failed spawn was trying to achieve. + (true, _) => SpawnRecovery::Adopt, + (false, Some(pid)) => SpawnRecovery::RetireAndRetry(pid), + (false, None) => SpawnRecovery::GiveUp, + } +} + +/// Retire the unresponsive leftover daemon, if there is one, and report the +/// PID that was retired. +/// +/// Deliberately built on [`recorded_daemon_pid`]'s verified-identity rule and +/// the same terminate-and-confirm escalation `daemon stop` uses, so this +/// codebase still kills daemon processes in exactly one way. A retirement that +/// fails is not fatal — the caller reports its original spawn failure either +/// way, with the retirement noted in the message. +pub(crate) async fn retire_unresponsive_for_recovery(client: &DaemonClient) -> Option { + match plan_spawn_recovery(client.health().await, recorded_daemon_pid()) { + SpawnRecovery::RetireAndRetry(pid) => { + tracing::warn!( + pid, + "every spawn attempt failed and PID {pid} is an unresponsive fbuild-daemon; \ + retiring it before one final attempt (FastLED/fbuild#1627)" + ); + match terminate_and_confirm(pid).await { + Ok(()) => { + clear_daemon_records(); + Some(pid) + } + Err(error) => { + tracing::warn!(pid, %error, "could not retire the unresponsive daemon"); + None + } + } + } + SpawnRecovery::Adopt | SpawnRecovery::GiveUp => None, + } +} + /// `fbuild daemon stop` — stop the daemon for *this* endpoint and report what /// actually happened. /// diff --git a/crates/fbuild-cli/src/cli/daemon_stop_tests.rs b/crates/fbuild-cli/src/cli/daemon_stop_tests.rs index f31b52d22..bfd15ff7c 100644 --- a/crates/fbuild-cli/src/cli/daemon_stop_tests.rs +++ b/crates/fbuild-cli/src/cli/daemon_stop_tests.rs @@ -62,3 +62,27 @@ fn a_live_non_daemon_pid_fails_the_identity_gate() { "the test binary is not an fbuild-daemon, so the gate must reject it" ); } +/// FastLED/fbuild#1627: after every spawn attempt fails, an unresponsive +/// daemon that still owns this endpoint's cache root is retired and retried +/// instead of failing the build outright. +#[test] +fn a_failed_spawn_retires_a_verified_unresponsive_daemon() { + assert_eq!( + plan_spawn_recovery(false, Some(4321)), + SpawnRecovery::RetireAndRetry(4321) + ); +} + +#[test] +fn a_failed_spawn_with_nothing_alive_to_retire_gives_up() { + assert_eq!(plan_spawn_recovery(false, None), SpawnRecovery::GiveUp); +} + +/// A daemon that answers is never retired, however the spawn attempts went: +/// it may be serving a concurrent build, and adopting it is what the failed +/// spawn was trying to do in the first place. +#[test] +fn a_healthy_endpoint_is_adopted_never_retired() { + assert_eq!(plan_spawn_recovery(true, Some(4321)), SpawnRecovery::Adopt); + assert_eq!(plan_spawn_recovery(true, None), SpawnRecovery::Adopt); +} diff --git a/crates/fbuild-cli/src/daemon_client.rs b/crates/fbuild-cli/src/daemon_client.rs index 50b89dfcb..c71f7b3e7 100644 --- a/crates/fbuild-cli/src/daemon_client.rs +++ b/crates/fbuild-cli/src/daemon_client.rs @@ -1255,6 +1255,42 @@ async fn start_daemon(client: &DaemonClient) -> fbuild_core::Result> ); } + // FastLED/fbuild#1627: three failed attempts are not the end when the + // reason is a leftover daemon from an earlier step that still owns the + // cache root (`root-owner.lock busy`) or a wedged one that owns the + // endpoint (#1360). #1732 explained those failures and #1734 widened the + // readiness budget; this retires the verified dead weight and makes + // exactly ONE final attempt, so a CI runner self-heals instead of failing + // a release gate that then needs a manual rerun. The retirement is + // fail-closed on process identity and never touches a daemon that + // answers, so a parallel build on the same machine is safe. + let mut leftover_note = String::new(); + if let Some(retired_pid) = + crate::cli::daemon_cmd::retire_unresponsive_for_recovery(client).await + { + leftover_note = format!( + "\n\nA leftover fbuild-daemon (PID {retired_pid}) was still alive while nothing \ + answered at this endpoint, so it was terminated and one final spawn attempt was \ + made (FastLED/fbuild#1627)." + ); + match spawn_daemon_process().await { + Ok(pid) => { + if client + .wait_until_healthy(std::time::Duration::from_secs( + startup_log::readiness_timeout_secs(), + )) + .await? + { + tracing::info!( + "daemon started (pid {pid}) after retiring unresponsive PID {retired_pid}" + ); + return Ok(Some(pid)); + } + } + Err(e) => tracing::warn!("final daemon spawn attempt after retirement failed: {e}"), + } + } + let readiness_secs = startup_log::readiness_timeout_secs(); Err(fbuild_core::FbuildError::DaemonError(format!( "daemon did not become healthy after 3 spawn attempts \ @@ -1265,9 +1301,10 @@ async fn start_daemon(client: &DaemonClient) -> fbuild_core::Result> version (FastLED/fbuild#1009), so a version-mismatched daemon listens on a different \ port than this CLI polls and can never be reached. When running a locally-built CLI, \ build the daemon too (`soldr cargo build -p fbuild-daemon`) so a sibling binary exists \ - instead of resolving `fbuild-daemon` from PATH.{}", + instead of resolving `fbuild-daemon` from PATH.{}{}", runtime_diagnostic(), wedged_daemon_hint(), + leftover_note, startup_log::startup_problems_note(&spawn_started) ))) }