test(cli): pin daemon crash-recovery with a real-process regression test (#1228)

crates/fbuild-cli/tests/README.md

@@ -8,6 +8,13 @@ and message contracts are covered end-to-end.
   HTTP daemon on an ephemeral loopback port, points the CLI at it via
   `FBUILD_DEV_MODE=1` + `FBUILD_DAEMON_PORT`, and asserts the CLI exits
   non-zero when the daemon returns a structured failure response.
+- **`daemon_crash_recovery.rs`** -- regression for FastLED/fbuild#1228.
+  `#[ignore]`-gated (runs under `bash test --full`): spawns the real
+  `fbuild` + sibling `fbuild-daemon` binaries, kills the daemon uncleanly,
+  and asserts the very next CLI invocation respawns a fresh daemon and
+  reaches it instead of redialing the dead endpoint. Skips when a live dev
+  daemon owns the real `~/.fbuild/dev` root or no sibling daemon binary
+  exists.
 - **`ci_command.rs`** -- regression for FastLED/fbuild#242. Spawns the
   compiled `fbuild` binary and asserts that `ci --help` documents the
   PlatformIO-compatible flags (`--board`, `--lib`, `--project-conf`,

crates/fbuild-cli/tests/daemon_crash_recovery.rs

@@ -0,0 +1,182 @@
+//! Real-process regression test for FastLED/fbuild#1228: after a daemon is
+//! killed uncleanly (leaving stale port/pid/status records behind), the very
+//! next CLI invocation must respawn a fresh daemon and reach it — the client
+//! must never sit redialing the dead endpoint.
+//!
+//! This drives the REAL `fbuild` binary, which in turn spawns the REAL
+//! sibling `fbuild-daemon` binary through the production spawn path
+//! (`ensure_daemon_running` → sibling discovery → detached spawn). Isolation
+//! notes, because the production spawn path rebuilds the daemon's
+//! environment from the OS user baseline (`user_baseline_environment`
+//! discards any test-provided `HOME`/`USERPROFILE`):
+//!
+//! - The daemon ALWAYS resolves the real `~/.fbuild/dev/` root. The test
+//!   therefore refuses to run (skips) when that root's `root-owner.lock` is
+//!   held — i.e. when a real dev daemon is alive on this machine — so it can
+//!   never displace or corrupt a daemon it does not own.
+//! - Port and cache are still isolated: `FBUILD_DAEMON_PORT` (a free
+//!   ephemeral port) and `FBUILD_CACHE_DIR` (a tempdir) both survive the
+//!   spawn path's `FBUILD_*` propagation filter.
+//! - `RUNNING_PROCESS_DISABLE=1` pins the legacy direct acquisition path so
+//!   the test deterministically exercises `ensure_direct_daemon_running`
+//!   (the path traced in #1228) rather than broker adoption.
+
+use std::path::PathBuf;
+use std::process::Command;
+use std::time::{Duration, Instant};
+
+use fbuild_core::process_identity::{pid_is_alive, terminate_pid, wait_for_pid_exit};
+use fbuild_paths::daemon_ownership::RootOwnershipGuard;
+
+/// The real user home, the same way the spawned daemon will resolve it.
+fn real_home() -> Option<PathBuf> {
+    let key = if cfg!(windows) { "USERPROFILE" } else { "HOME" };
+    std::env::var_os(key).map(PathBuf::from)
+}
+
+/// `<real home>/.fbuild/dev/daemon/root-owner.lock` — the dev-mode root
+/// ownership lock the spawned daemon will contend for. Mirrors
+/// `fbuild_paths` layout without mutating this process's env (env vars are
+/// process-global and tests run multi-threaded).
+fn dev_root_owner_lock(home: &std::path::Path) -> PathBuf {
+    home.join(".fbuild")
+        .join("dev")
+        .join("daemon")
+        .join("root-owner.lock")
+}
+
+fn free_port() -> u16 {
+    let listener = std::net::TcpListener::bind("127.0.0.1:0").expect("bind ephemeral port");
+    listener.local_addr().expect("local addr").port()
+}
+
+/// Run the fbuild CLI with the test's isolation env; returns (exit ok, stdout).
+fn run_cli(args: &[&str], port: u16, cache_dir: &std::path::Path) -> (bool, String) {
+    let bin = env!("CARGO_BIN_EXE_fbuild");
+    // allow-direct-spawn: test driver invoking the fbuild CLI binary under test.
+    let output = Command::new(bin)
+        .args(args)
+        .env("FBUILD_DEV_MODE", "1")
+        .env("FBUILD_DAEMON_PORT", port.to_string())
+        .env("FBUILD_CACHE_DIR", cache_dir)
+        .env("RUNNING_PROCESS_DISABLE", "1")
+        .output()
+        .expect("spawn fbuild CLI");
+    let stdout = String::from_utf8_lossy(&output.stdout).into_owned();
+    (output.status.success(), stdout)
+}
+
+/// Parse `  PID:     12345` out of `fbuild daemon status` output.
+fn parse_status_pid(status_stdout: &str) -> Option<u32> {
+    status_stdout.lines().find_map(|line| {
+        let rest = line.trim().strip_prefix("PID:")?;
+        rest.trim().parse().ok()
+    })
+}
+
+fn wait_for_health(port: u16, budget: Duration) -> bool {
+    let url = format!("http://127.0.0.1:{port}/health");
+    let deadline = Instant::now() + budget;
+    while Instant::now() < deadline {
+        if let Ok(resp) = reqwest::blocking::get(&url) {
+            if resp.status().is_success() {
+                return true;
+            }
+        }
+        std::thread::sleep(Duration::from_millis(200));
+    }
+    false
+}
+
+#[test]
+#[ignore = "spawns real fbuild + fbuild-daemon binaries (#1228)"]
+fn client_recovers_after_daemon_is_killed_uncleanly() {
+    let Some(home) = real_home() else {
+        eprintln!("skip: no home directory resolvable");
+        return;
+    };
+
+    // The production spawn path resolves the daemon binary as a sibling of
+    // the CLI. Under `cargo test --workspace` (and any full build) it exists;
+    // under an isolated `-p fbuild-cli` test run it may not — skip then.
+    let cli = PathBuf::from(env!("CARGO_BIN_EXE_fbuild"));
+    let daemon_name = if cfg!(windows) {
+        "fbuild-daemon.exe"
+    } else {
+        "fbuild-daemon"
+    };
+    let sibling = cli.parent().map(|d| d.join(daemon_name));
+    if !sibling.as_deref().is_some_and(std::path::Path::exists) {
+        eprintln!("skip: no sibling fbuild-daemon binary at {sibling:?} — build fbuild-daemon first");
+        return;
+    }
+
+    // Never contend with a real dev daemon: the spawned daemon will use the
+    // real ~/.fbuild/dev root (see module docs), so if something already owns
+    // it, running this test would try to displace a daemon we don't own.
+    let lock_path = dev_root_owner_lock(&home);
+    match RootOwnershipGuard::try_acquire_at(&lock_path) {
+        Ok(Some(guard)) => drop(guard), // free — safe to proceed
+        Ok(None) => {
+            eprintln!("skip: a live dev daemon holds {lock_path:?}");
+            return;
+        }
+        Err(err) => {
+            eprintln!("skip: cannot probe {lock_path:?}: {err}");
+            return;
+        }
+    }
+
+    let cache_dir = tempfile::tempdir().expect("temp cache dir");
+    let port = free_port();
+
+    // 1. Bring a daemon up through the production acquisition path.
+    let (ok, _) = run_cli(&["daemon", "restart"], port, cache_dir.path());
+    assert!(ok, "initial `fbuild daemon restart` must succeed");
+    assert!(
+        wait_for_health(port, Duration::from_secs(30)),
+        "daemon never became healthy on port {port}"
+    );
+    let (ok, status) = run_cli(&["daemon", "status"], port, cache_dir.path());
+    assert!(ok, "daemon status must succeed while daemon is up");
+    let old_pid = parse_status_pid(&status)
+        .unwrap_or_else(|| panic!("no PID in daemon status output:\n{status}"));
+    assert!(pid_is_alive(old_pid), "freshly started daemon must be alive");
+
+    // 2. Crash it. This is the #1213/#1228 scenario: unclean death that
+    //    leaves the port/pid/status records in place.
+    terminate_pid(old_pid);
+    assert!(
+        wait_for_pid_exit(old_pid, Duration::from_secs(15)),
+        "daemon (pid {old_pid}) did not die within 15s of terminate_pid"
+    );
+
+    // 3. The very next client invocation must recover: detect the dead
+    //    endpoint, respawn, and reach the fresh daemon. `daemon restart`
+    //    routes through the same `ensure_daemon_running` every build/deploy
+    //    request uses.
+    let (ok, _) = run_cli(&["daemon", "restart"], port, cache_dir.path());
+    assert!(
+        ok,
+        "client invocation after unclean daemon death must respawn and succeed (#1228)"
+    );
+    assert!(
+        wait_for_health(port, Duration::from_secs(30)),
+        "respawned daemon never became healthy on port {port}"
+    );
+    let (ok, status) = run_cli(&["daemon", "status"], port, cache_dir.path());
+    assert!(ok, "daemon status must succeed after recovery");
+    let new_pid = parse_status_pid(&status)
+        .unwrap_or_else(|| panic!("no PID in post-recovery status output:\n{status}"));
+    assert_ne!(new_pid, old_pid, "recovery must have spawned a NEW daemon");
+    assert!(pid_is_alive(new_pid), "respawned daemon must be alive");
+
+    // 4. Cleanup: stop the daemon we spawned (also exercises the #1227
+    //    stale-record clearing) and confirm it exits.
+    let (ok, _) = run_cli(&["daemon", "stop"], port, cache_dir.path());
+    assert!(ok, "daemon stop must succeed");
+    assert!(
+        wait_for_pid_exit(new_pid, Duration::from_secs(15)),
+        "daemon (pid {new_pid}) did not exit within 15s of `daemon stop`"
+    );
+}