From c6cafceae0ec348294c3ec7535412e40660338f2 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 20 Apr 2026 15:04:08 -0400 Subject: [PATCH 1/8] test: de-flake pr_list and full_http_lifecycle_cancel Two CLI/server tests racing against peer state on the shared fabro server session, surfaced by running the default nextest profile 20 times. pr_list_missing_github_credentials_errors depended on an empty shared store; if pr_view_reads_pull_request_from_store_without_pull_request_json ran first it left a PR record behind and this test hit the credentials-required branch instead of "No pull requests found." The snapshot captured the empty path, but the test name promises the error path. Seed a PullRequestCreated event against the test's own run so the store is guaranteed non-empty and the credentials-required error fires deterministically. full_http_lifecycle_cancel asserted that the cancel response body's pending_control == "cancel", but that field is re-read from the store projection after the worker has been signaled. The worker is sitting at a human gate; on hot CI it can emit a clearing event before the handler re-reads the projection, yielding a legitimate null. Relax the assertion to accept "cancel" or null; durable convergence to failed/cancelled is still asserted below. Co-Authored-By: Claude Opus 4.7 (1M context) --- lib/crates/fabro-cli/tests/it/cmd/pr_list.rs | 59 ++++++++++++++++++- .../tests/it/scenario/lifecycle.rs | 12 +++- 2 files changed, 67 insertions(+), 4 deletions(-) diff --git a/lib/crates/fabro-cli/tests/it/cmd/pr_list.rs b/lib/crates/fabro-cli/tests/it/cmd/pr_list.rs index 66cd60308..9d85918af 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/pr_list.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/pr_list.rs @@ -1,4 +1,13 @@ +#![allow( + clippy::absolute_paths, + reason = "This test module prefers explicit type paths over extra imports." +)] + use fabro_test::{fabro_snapshot, test_context}; +use fabro_types::run_event::PullRequestCreatedProps; +use fabro_types::{EventBody, RunEvent, RunId}; + +use super::support::{server_endpoint, setup_completed_fast_dry_run}; #[test] fn help() { @@ -26,17 +35,61 @@ fn help() { "); } +// Seed a PR event against this test's own run so the store is guaranteed to +// have at least one entry; `fabro pr list` then must load GitHub credentials +// and fail, regardless of what peer tests have left in the shared store. #[test] fn pr_list_missing_github_credentials_errors() { let context = test_context!(); + let run = setup_completed_fast_dry_run(&context); + let run_id: RunId = run.run_id.parse().unwrap(); + + let runtime = tokio::runtime::Runtime::new().unwrap(); + runtime.block_on(async { + let (client, base_url) = + server_endpoint(&context.storage_dir).expect("server endpoint should exist"); + let event = RunEvent { + id: ulid::Ulid::new().to_string(), + ts: chrono::Utc::now(), + run_id, + node_id: None, + node_label: None, + stage_id: None, + parallel_group_id: None, + parallel_branch_id: None, + session_id: None, + parent_session_id: None, + tool_call_id: None, + actor: None, + body: EventBody::PullRequestCreated(PullRequestCreatedProps { + pr_url: "https://github.com/fabro-sh/fabro/pull/123".to_string(), + pr_number: 123, + owner: "fabro-sh".to_string(), + repo: "fabro".to_string(), + base_branch: "main".to_string(), + head_branch: "fabro/run/demo".to_string(), + title: "Map the constellations".to_string(), + draft: false, + }), + }; + client + .post(format!("{base_url}/api/v1/runs/{run_id}/events")) + .json(&event) + .send() + .await + .unwrap() + .error_for_status() + .unwrap(); + }); + let mut cmd = context.command(); cmd.args(["pr", "list"]); fabro_snapshot!(context.filters(), cmd, @" - success: true - exit_code: 0 + success: false + exit_code: 1 ----- stdout ----- - No pull requests found. ----- stderr ----- + error: GitHub credentials required — run `fabro install` or set GITHUB_TOKEN "); } diff --git a/lib/crates/fabro-server/tests/it/scenario/lifecycle.rs b/lib/crates/fabro-server/tests/it/scenario/lifecycle.rs index 2ec100f16..f04300075 100644 --- a/lib/crates/fabro-server/tests/it/scenario/lifecycle.rs +++ b/lib/crates/fabro-server/tests/it/scenario/lifecycle.rs @@ -249,7 +249,17 @@ async fn full_http_lifecycle_cancel() { ) .await; assert_eq!(body["status"], "running"); - assert_eq!(body["pending_control"], "cancel"); + // `pending_control` is computed from the store projection after the cancel + // event is appended AND the worker is signaled. The worker is sitting at a + // human gate; once notified it can emit a clearing event before this + // handler re-reads the projection, so the response can legitimately + // observe either the still-pending "cancel" or a null where the worker + // already consumed it. Durable convergence is asserted below. + let pending_control = &body["pending_control"]; + assert!( + pending_control == "cancel" || pending_control.is_null(), + "expected pending_control to be \"cancel\" or null, got {pending_control}" + ); // Verify the durable store view converges to cancelled failure. let body = wait_for_run_state(&app, &run_id, "failed", "cancelled").await; From 24e7e5af8390554680b17dfc6e8713fb3c8e36af Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 20 Apr 2026 16:24:26 -0400 Subject: [PATCH 2/8] test(harness): add FABRO_TEST_PROBE_LOG timing probes Gated test-harness diagnostic. Writes one tab-separated line per phase of TestContext::new to the path named by FABRO_TEST_PROBE_LOG, using an O_APPEND+single-write-per-line pattern so concurrent test processes do not interleave. Disabled when the env var is unset. Used to isolate the source of a recent test-suite slowdown; removed again at the end of the same change set once verification is done. Co-Authored-By: Claude Opus 4.7 (1M context) --- lib/crates/fabro-test/src/lib.rs | 36 ++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/lib/crates/fabro-test/src/lib.rs b/lib/crates/fabro-test/src/lib.rs index da412c96c..b32c4ffb0 100644 --- a/lib/crates/fabro-test/src/lib.rs +++ b/lib/crates/fabro-test/src/lib.rs @@ -44,6 +44,28 @@ macro_rules! preserve_coverage_env { }}; } +/// Emit a single timing probe line to `$FABRO_TEST_PROBE_LOG` using an +/// O_APPEND open — POSIX guarantees that a single `write` up to +/// `PIPE_BUF` bytes is atomic, so concurrent test processes writing short +/// lines to the same file do not interleave. +fn probe_emit(test: &str, phase: &str, elapsed: std::time::Duration) { + let Ok(path) = std::env::var("FABRO_TEST_PROBE_LOG") else { + return; + }; + use std::io::Write; + // Format the entire line first, then issue exactly one `write` so lines + // from concurrent writers do not interleave (O_APPEND + single syscall + // ≤ PIPE_BUF is atomic per POSIX). + let line = format!("{phase}\t{:.3}\t{test}\n", elapsed.as_secs_f64() * 1000.0); + if let Ok(mut file) = std::fs::OpenOptions::new() + .append(true) + .create(true) + .open(path) + { + let _ = file.write(line.as_bytes()); + } +} + /// Walk up from `start` to find the repo-level `test/` fixtures directory. pub fn find_test_fixtures_dir(start: &Path) -> Option { let mut dir = start; @@ -954,6 +976,13 @@ impl TestContext { .next() .unwrap_or("unknown") .to_string(); + let probe_start = std::time::Instant::now(); + let mut last = probe_start; + let mut probe = |phase: &str| { + let now = std::time::Instant::now(); + probe_emit(&test_name, phase, now.duration_since(last)); + last = now; + }; // Truncate to keep total temp path under Unix socket limit (104 bytes). // Budget: TMPDIR (~49) + prefix + suffix (~6) + /home/fabro-data/fabro.sock // (27) < 104 @@ -962,10 +991,14 @@ impl TestContext { .prefix(&format!(".ft-{label}-")) .tempdir() .expect("failed to create temp dir"); + probe("tempdir"); let root_path = context_root.path().to_path_buf(); let (_, test_run_id, session_paths) = session_paths(); + probe("session_paths"); reap_stale_session_roots(SessionMode::Nextest); + probe("reap_nextest"); reap_stale_session_roots(SessionMode::Process); + probe("reap_process"); with_session_lock(&session_paths.root, || { std::fs::create_dir_all(session_clients_dir(&session_paths.root)).unwrap_or_else( |err| { @@ -995,6 +1028,7 @@ impl TestContext { } write_marker(&session_paths.root); }); + probe("with_session_lock"); let temp_dir = root_path.join("temp"); let home_dir = root_path.join("home"); @@ -1009,6 +1043,8 @@ impl TestContext { &session_paths.server.socket_path, false, ); + probe("sync_home_settings"); + probe_emit(&test_name, "total", probe_start.elapsed()); let temp_dir_str = temp_dir .to_str() .expect("temp_dir should be valid UTF-8 for snapshot filtering"); From da87f978cd7641c6afae22f355c54a01adf4c1c2 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 20 Apr 2026 16:24:42 -0400 Subject: [PATCH 3/8] fix(proc): revert process_running to cheap kill(0) probe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Commit 1ed8e6cbd changed process_running(pid) to shell out to `ps` on every call to distinguish running processes from zombies. That cost ~2 ms per invocation on macOS (fork + exec + wait), and the test harness calls process_running O(tests × markers) times under session flock contention. Across a `cargo nextest run -p fabro-cli` that added up to ~90 s of suite time, and the zombie-aware semantics turned out to have no production caller on Unix (the server's worker-termination loop uses process_group_alive; the CLI stop/status paths don't need zombie detection for a daemon that reparents to init). Restore the pre-1ed8e6cbd body: process_running is now a straight kill(pid, 0) via process_exists on Unix, true on non-unix. Delete unix_process_state (the `ps` helper) and its zombie regression test, since they describe behavior we're rolling back. process_group_alive and its tests are unchanged. Measured on this branch against baseline db953c838: reap_nextest p50: 172 ms -> 0.3 ms TestContext::new: 316 ms mean -> 15 ms mean If a future caller genuinely needs zombie-aware semantics, add it back alongside that caller with a benchmark in context. Co-Authored-By: Claude Opus 4.7 (1M context) --- lib/crates/fabro-proc/src/signal.rs | 70 ++++------------------------- 1 file changed, 8 insertions(+), 62 deletions(-) diff --git a/lib/crates/fabro-proc/src/signal.rs b/lib/crates/fabro-proc/src/signal.rs index a418b6dca..7325c3f3d 100644 --- a/lib/crates/fabro-proc/src/signal.rs +++ b/lib/crates/fabro-proc/src/signal.rs @@ -20,42 +20,15 @@ pub fn process_exists(pid: u32) -> bool { /// Check whether a process with the given PID is still running. /// -/// On Unix, this treats zombie / defunct processes as not running even though -/// they still have a visible PID until their parent reaps them. If the -/// follow-up `ps` probe fails, this falls back to `process_exists(pid)` to -/// preserve the old conservative behavior. +/// On Unix, delegates to `process_exists` (a `kill(pid, 0)` probe). Unreaped +/// zombies count as running here because callers that need to distinguish +/// zombies from live processes are a narrow minority; giving every caller +/// the zombie check would require spawning `ps` on every probe and would +/// dominate test-harness setup time at the scale we run it. If a caller +/// needs zombie-aware semantics, it should be introduced alongside that +/// caller with a benchmark in context. pub fn process_running(pid: u32) -> bool { - #[cfg(unix)] - { - if !process_exists(pid) { - return false; - } - - unix_process_state(pid).is_none_or(|state| !matches!(state, 'Z' | 'z')) - } - #[cfg(not(unix))] - { - process_exists(pid) - } -} - -#[cfg(unix)] -#[expect( - clippy::disallowed_methods, - reason = "Unix process-state detection shells out to ps to distinguish running processes from zombies" -)] -fn unix_process_state(pid: u32) -> Option { - let output = std::process::Command::new("ps") - .args(["-ww", "-o", "stat=", "-p", &pid.to_string()]) - .output() - .ok()?; - if !output.status.success() { - return None; - } - - String::from_utf8_lossy(&output.stdout) - .chars() - .find(|ch| !ch.is_whitespace()) + process_exists(pid) } /// Check whether any process in the given process group is alive. @@ -164,33 +137,6 @@ mod tests { assert!(process_running(std::process::id())); } - #[cfg(unix)] - #[test] - #[expect( - clippy::disallowed_methods, - reason = "process-state test needs to spawn a short-lived child and intentionally leave it unreaped" - )] - fn process_running_returns_false_for_unreaped_zombie_child() { - let mut child = Command::new("sh") - .args(["-c", "exit 0"]) - .spawn() - .expect("short-lived child should spawn"); - let pid = child.id(); - - std::thread::sleep(Duration::from_millis(100)); - - assert!( - process_exists(pid), - "unreaped zombie should still have a visible pid" - ); - assert!( - !process_running(pid), - "unreaped zombie should not count as a running process" - ); - - let _status = child.wait().expect("child should remain waitable"); - } - #[cfg(unix)] #[test] #[expect( From 3885b6751560085dd8c0329f356b12112548a3c0 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 20 Apr 2026 16:27:17 -0400 Subject: [PATCH 4/8] test(harness): amortize stale-session reap across tests reap_stale_session_roots cleans up session roots left over by prior nextest runs that crashed. TestContext::new called it twice per test (once per SessionMode). Under a 721-test fabro-cli suite that was ~1400 reap calls where one would do, accounting for several seconds of per-suite overhead even after the process_running regression was reverted. Gate each call behind a per-process OnceLock so at most one reap runs per SessionMode per test binary. The reap itself (and its internal per-root session lock, which iterates candidate roots) is unchanged; we just stop re-entering it for every TestContext::new. Measured on fabro-cli after this change: reap_nextest probes: 356 calls, 302 under 1 ms (OnceLock fast path), sum 1.4 s (down from Friday's 3.1 s and HEAD's 88.6 s before the process_running revert). Co-Authored-By: Claude Opus 4.7 (1M context) --- lib/crates/fabro-test/src/lib.rs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/lib/crates/fabro-test/src/lib.rs b/lib/crates/fabro-test/src/lib.rs index b32c4ffb0..20d72ec38 100644 --- a/lib/crates/fabro-test/src/lib.rs +++ b/lib/crates/fabro-test/src/lib.rs @@ -328,6 +328,14 @@ fn session_refs() -> &'static Mutex> { SESSION_REFS.get_or_init(|| Mutex::new(HashMap::new())) } +// Gate the stale-session reap so it fires at most once per process. The +// reap scans /tmp/fx/{n-*,p-*} to clean up after a prior nextest run +// that crashed; it's correctness-for-safety work that does not need to +// happen on every TestContext::new. One gate per SessionMode preserves +// the existing call structure without introducing cross-mode coupling. +static NEXTEST_REAPED: OnceLock<()> = OnceLock::new(); +static PROCESS_REAPED: OnceLock<()> = OnceLock::new(); + #[expect( clippy::disallowed_methods, reason = "This synchronous test-support helper uses uuidgen when available to create stable unique case IDs." @@ -995,9 +1003,9 @@ impl TestContext { let root_path = context_root.path().to_path_buf(); let (_, test_run_id, session_paths) = session_paths(); probe("session_paths"); - reap_stale_session_roots(SessionMode::Nextest); + NEXTEST_REAPED.get_or_init(|| reap_stale_session_roots(SessionMode::Nextest)); probe("reap_nextest"); - reap_stale_session_roots(SessionMode::Process); + PROCESS_REAPED.get_or_init(|| reap_stale_session_roots(SessionMode::Process)); probe("reap_process"); with_session_lock(&session_paths.root, || { std::fs::create_dir_all(session_clients_dir(&session_paths.root)).unwrap_or_else( From 2348a1e4837fa34367851775bad3692afbdf4a91 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 20 Apr 2026 16:33:53 -0400 Subject: [PATCH 5/8] test(harness): use advisory locks for peer presence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace PID-based liveness probing in `live_marker_count` with flock advisory-lock presence detection. Each test process opens `/clients/` once, holds LOCK_SH for the lifetime of any live TestContext in the process, and releases it explicitly when `cleanup_session_root` fires at refcount zero. Reapers probe with LOCK_EX | LOCK_NB: success means the previous owner is gone (normal exit, panic, SIGKILL, or zombie — the kernel releases advisory locks at process exit in every case) and the stale marker is removed. Compared to the PID check this was replacing: - Handles PID recycling correctly (the new holder does not inherit the previous owner's advisory lock). - Handles zombies correctly without shelling out to `ps`. - Costs one open + one flock per peer, ~50 us on macOS. The marker handle is stored in a process-scoped `Mutex>` so it can be released and reacquired across the drop-to-zero / rise-from-zero cycles that `session_refs` already implements. Storing the path alongside the handle enables a debug assertion that the process never drifts between session roots. `ClientMarker` and its serde plumbing are removed; the marker file is now empty, its existence and lock state carrying the signal. Full workspace wall-clock after A+B+C: 13.3–13.6 s, down from 20–25 s on HEAD before the fix and comparable to the 14 s Friday baseline despite the intervening +85 tests. Co-Authored-By: Claude Opus 4.7 (1M context) --- lib/crates/fabro-proc/src/flock.rs | 19 +++++ lib/crates/fabro-proc/src/lib.rs | 2 +- lib/crates/fabro-test/src/lib.rs | 127 ++++++++++++++++++++++------- 3 files changed, 117 insertions(+), 31 deletions(-) diff --git a/lib/crates/fabro-proc/src/flock.rs b/lib/crates/fabro-proc/src/flock.rs index 552b143b7..ff048929a 100644 --- a/lib/crates/fabro-proc/src/flock.rs +++ b/lib/crates/fabro-proc/src/flock.rs @@ -20,6 +20,25 @@ pub fn try_flock_exclusive(file: &File) -> io::Result { } } +/// Try to acquire a shared (read) lock on `file` without blocking. +/// +/// Returns `Ok(true)` if the lock was acquired, `Ok(false)` if another +/// process/fd already holds an incompatible lock, and `Err` for unexpected +/// errors. +pub fn try_flock_shared(file: &File) -> io::Result { + // SAFETY: flock() on a valid fd is safe; LOCK_NB makes it non-blocking. + let ret = unsafe { libc::flock(file.as_raw_fd(), libc::LOCK_SH | libc::LOCK_NB) }; + if ret == 0 { + Ok(true) + } else { + let err = io::Error::last_os_error(); + match err.raw_os_error() { + Some(libc::EWOULDBLOCK) => Ok(false), + _ => Err(err), + } + } +} + /// Release any lock held on `file`. pub fn flock_unlock(file: &File) -> io::Result<()> { // SAFETY: flock() with LOCK_UN on a valid fd is safe. diff --git a/lib/crates/fabro-proc/src/lib.rs b/lib/crates/fabro-proc/src/lib.rs index 57f9c3c69..e97162293 100644 --- a/lib/crates/fabro-proc/src/lib.rs +++ b/lib/crates/fabro-proc/src/lib.rs @@ -11,7 +11,7 @@ mod signal; mod title; #[cfg(unix)] -pub use flock::{flock_unlock, try_flock_exclusive}; +pub use flock::{flock_unlock, try_flock_exclusive, try_flock_shared}; #[cfg(target_os = "linux")] pub use pre_exec::pre_exec_pdeathsig; #[cfg(unix)] diff --git a/lib/crates/fabro-test/src/lib.rs b/lib/crates/fabro-test/src/lib.rs index 20d72ec38..c00226225 100644 --- a/lib/crates/fabro-test/src/lib.rs +++ b/lib/crates/fabro-test/src/lib.rs @@ -16,7 +16,6 @@ use assert_cmd::Command; use fabro_config::Storage; use fabro_types::RunId; use regex::Regex; -use serde::Serialize; use serde_json::{Map, Value, json}; use toml::Value as TomlValue; use toml::map::Map as TomlMap; @@ -316,12 +315,6 @@ enum SessionMode { Process, } -#[derive(Debug, Serialize)] -struct ClientMarker { - pid: u32, - touched_at_ms: u128, -} - static SESSION_REFS: OnceLock>> = OnceLock::new(); fn session_refs() -> &'static Mutex> { @@ -336,6 +329,21 @@ fn session_refs() -> &'static Mutex> { static NEXTEST_REAPED: OnceLock<()> = OnceLock::new(); static PROCESS_REAPED: OnceLock<()> = OnceLock::new(); +// Advisory-lock-based peer-presence marker. Each test process opens +// `/clients/` once, holds a shared (LOCK_SH) flock +// for the lifetime of any live TestContext in the process, and +// releases it explicitly in `cleanup_session_root` when the refcount +// drops to zero. Peers detect liveness by attempting LOCK_EX: if it +// succeeds, the owner is gone (normal exit, panic, SIGKILL, or zombie +// — the kernel releases flocks at process exit in every case), and +// the stale marker file is removed. +// +// The slot stores the marker path so subsequent rebounds in the same +// process (e.g., after a drop-to-zero followed by a new TestContext) +// can re-validate the invariant: a process only ever participates in +// one session root. +static MARKER_HANDLE: Mutex> = Mutex::new(None); + #[expect( clippy::disallowed_methods, reason = "This synchronous test-support helper uses uuidgen when available to create stable unique case IDs." @@ -359,13 +367,6 @@ fn test_case_id() -> String { ulid } -fn current_timestamp_ms() -> u128 { - SystemTime::now() - .duration_since(UNIX_EPOCH) - .expect("system time should be after unix epoch") - .as_millis() -} - fn current_pid() -> u32 { std::process::id() } @@ -497,6 +498,18 @@ fn with_session_lock(root: &Path, f: impl FnOnce() -> T) -> T { result } +// Iterate `/clients/` and for each marker file attempt a +// non-blocking exclusive flock. Success means the previous owner has +// released the lock (normal exit, panic, SIGKILL, or zombie — the +// kernel releases flocks at process exit regardless), and the stale +// marker file is removed. `EWOULDBLOCK` means the owner is still +// alive and holding LOCK_SH. +// +// Same-process subtlety: if our own PID's marker file is in the +// listing, we have LOCK_SH on it via `MARKER_HANDLE`. On Linux and +// macOS, flock locks are per-open-file-description, so a fresh +// `open()` here returns an FD that sees the shared lock and correctly +// reports EWOULDBLOCK when asked for LOCK_EX. fn live_marker_count(root: &Path) -> usize { let clients_dir = session_clients_dir(root); let Ok(entries) = std::fs::read_dir(&clients_dir) else { @@ -510,30 +523,73 @@ fn live_marker_count(root: &Path) -> usize { .to_string_lossy() .parse::() .ok() - .map(|pid| (pid, entry.path())) + .map(|_pid| entry.path()) }) - .filter(|(pid, path)| { - if fabro_proc::process_running(*pid) { - true - } else { - let _ = std::fs::remove_file(path); - false + .filter(|path| { + // Open read-write so LOCK_EX has the access mode it expects + // on the widest set of platforms. If the file is missing + // between read_dir and open, treat it as already gone. + let Ok(file) = std::fs::OpenOptions::new() + .read(true) + .write(true) + .open(path) + else { + return false; + }; + match fabro_proc::try_flock_exclusive(&file) { + Ok(true) => { + // Lock acquired: previous owner is gone. Drop the + // file handle (releasing our just-acquired lock) + // and remove the marker. + drop(file); + let _ = std::fs::remove_file(path); + false + } + Ok(false) => true, + Err(_) => true, // conservative: count unexpected errors as alive } }) .count() } +// Open (or reopen, after a session-drop-to-zero) the per-process +// marker file and hold a shared advisory lock on it in +// `MARKER_HANDLE`. Called from inside the `with_session_lock` block +// of `TestContext::new`. fn write_marker(root: &Path) { - let marker = ClientMarker { - pid: current_pid(), - touched_at_ms: current_timestamp_ms(), - }; - let marker_path = session_marker_path(root, marker.pid); + let marker_path = session_marker_path(root, current_pid()); ensure_parent_dir(&marker_path); - let contents = - serde_json::to_vec(&marker).expect("client marker should serialize to JSON bytes"); - std::fs::write(&marker_path, contents) - .unwrap_or_else(|err| panic!("failed to write {}: {err}", marker_path.display())); + + let mut slot = MARKER_HANDLE.lock().expect("MARKER_HANDLE lock poisoned"); + + if let Some((existing_path, _)) = slot.as_ref() { + debug_assert_eq!( + existing_path, &marker_path, + "marker handle path drifted — session root changed mid-process?" + ); + if marker_path.exists() { + return; + } + // Marker was removed (e.g., by a peer's reap) while we thought + // we still owned it. Re-establish by replacing the handle. + slot.take(); + } + + let file = std::fs::OpenOptions::new() + .create(true) + .truncate(true) + .read(true) + .write(true) + .open(&marker_path) + .unwrap_or_else(|err| panic!("failed to open {}: {err}", marker_path.display())); + let acquired = fabro_proc::try_flock_shared(&file) + .unwrap_or_else(|err| panic!("failed to flock {}: {err}", marker_path.display())); + assert!( + acquired, + "unexpected contention acquiring LOCK_SH on freshly created marker {}", + marker_path.display() + ); + *slot = Some((marker_path, file)); } fn managed_storage_settings(storage_dir: &Path, rest: &str) -> String { @@ -928,6 +984,17 @@ fn reap_isolated_servers(root: &Path) { fn cleanup_session_root(root: &Path) { with_session_lock(root, || { + // Release our own advisory lock first so `live_marker_count` + // below can observe that no one is holding the marker file, + // then remove the file. Order matters: if we unlinked before + // dropping the handle, peers would still see our LOCK_SH on + // the (now-unlinked but still open) inode and count us as + // live, preventing the server teardown. + { + let mut slot = MARKER_HANDLE.lock().expect("MARKER_HANDLE lock poisoned"); + // Dropping the File closes the FD and releases LOCK_SH. + slot.take(); + } let marker_path = session_marker_path(root, current_pid()); let _ = std::fs::remove_file(&marker_path); let live_count = live_marker_count(root); From 39744068c91cb35a9025e96e9e605ff00e9849da Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 20 Apr 2026 16:36:31 -0400 Subject: [PATCH 6/8] Revert "test(harness): add FABRO_TEST_PROBE_LOG timing probes" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The probe helper served its purpose in narrowing the recent per-test setup regression to `reap_stale_session_roots`. Remove it now that the underlying cause (process_running shelling out to `ps`) is fixed and the reap is amortized to once per process. The plan was to carry it through verification so Step B could quote reap_nextest numbers, then drop it — this commit is that drop. This reverts commit 24e7e5af8. Co-Authored-By: Claude Opus 4.7 (1M context) --- lib/crates/fabro-test/src/lib.rs | 36 -------------------------------- 1 file changed, 36 deletions(-) diff --git a/lib/crates/fabro-test/src/lib.rs b/lib/crates/fabro-test/src/lib.rs index c00226225..2ece1bb83 100644 --- a/lib/crates/fabro-test/src/lib.rs +++ b/lib/crates/fabro-test/src/lib.rs @@ -43,28 +43,6 @@ macro_rules! preserve_coverage_env { }}; } -/// Emit a single timing probe line to `$FABRO_TEST_PROBE_LOG` using an -/// O_APPEND open — POSIX guarantees that a single `write` up to -/// `PIPE_BUF` bytes is atomic, so concurrent test processes writing short -/// lines to the same file do not interleave. -fn probe_emit(test: &str, phase: &str, elapsed: std::time::Duration) { - let Ok(path) = std::env::var("FABRO_TEST_PROBE_LOG") else { - return; - }; - use std::io::Write; - // Format the entire line first, then issue exactly one `write` so lines - // from concurrent writers do not interleave (O_APPEND + single syscall - // ≤ PIPE_BUF is atomic per POSIX). - let line = format!("{phase}\t{:.3}\t{test}\n", elapsed.as_secs_f64() * 1000.0); - if let Ok(mut file) = std::fs::OpenOptions::new() - .append(true) - .create(true) - .open(path) - { - let _ = file.write(line.as_bytes()); - } -} - /// Walk up from `start` to find the repo-level `test/` fixtures directory. pub fn find_test_fixtures_dir(start: &Path) -> Option { let mut dir = start; @@ -1051,13 +1029,6 @@ impl TestContext { .next() .unwrap_or("unknown") .to_string(); - let probe_start = std::time::Instant::now(); - let mut last = probe_start; - let mut probe = |phase: &str| { - let now = std::time::Instant::now(); - probe_emit(&test_name, phase, now.duration_since(last)); - last = now; - }; // Truncate to keep total temp path under Unix socket limit (104 bytes). // Budget: TMPDIR (~49) + prefix + suffix (~6) + /home/fabro-data/fabro.sock // (27) < 104 @@ -1066,14 +1037,10 @@ impl TestContext { .prefix(&format!(".ft-{label}-")) .tempdir() .expect("failed to create temp dir"); - probe("tempdir"); let root_path = context_root.path().to_path_buf(); let (_, test_run_id, session_paths) = session_paths(); - probe("session_paths"); NEXTEST_REAPED.get_or_init(|| reap_stale_session_roots(SessionMode::Nextest)); - probe("reap_nextest"); PROCESS_REAPED.get_or_init(|| reap_stale_session_roots(SessionMode::Process)); - probe("reap_process"); with_session_lock(&session_paths.root, || { std::fs::create_dir_all(session_clients_dir(&session_paths.root)).unwrap_or_else( |err| { @@ -1103,7 +1070,6 @@ impl TestContext { } write_marker(&session_paths.root); }); - probe("with_session_lock"); let temp_dir = root_path.join("temp"); let home_dir = root_path.join("home"); @@ -1118,8 +1084,6 @@ impl TestContext { &session_paths.server.socket_path, false, ); - probe("sync_home_settings"); - probe_emit(&test_name, "total", probe_start.elapsed()); let temp_dir_str = temp_dir .to_str() .expect("temp_dir should be valid UTF-8 for snapshot filtering"); From 224ce9e21e185fa09b9d92c506af66dc56293d8b Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 20 Apr 2026 16:38:28 -0400 Subject: [PATCH 7/8] test(harness): collapse live_marker_count match into matches! Clippy (match_same_arms) on the Step C rewrite: Ok(false) and Err(_) both mean "treat as alive", so expressing it as `if matches!(..., Ok(true))` reads cleaner and satisfies the lint. Co-Authored-By: Claude Opus 4.7 (1M context) --- lib/crates/fabro-test/src/lib.rs | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/lib/crates/fabro-test/src/lib.rs b/lib/crates/fabro-test/src/lib.rs index 2ece1bb83..b428cc482 100644 --- a/lib/crates/fabro-test/src/lib.rs +++ b/lib/crates/fabro-test/src/lib.rs @@ -514,17 +514,17 @@ fn live_marker_count(root: &Path) -> usize { else { return false; }; - match fabro_proc::try_flock_exclusive(&file) { - Ok(true) => { - // Lock acquired: previous owner is gone. Drop the - // file handle (releasing our just-acquired lock) - // and remove the marker. - drop(file); - let _ = std::fs::remove_file(path); - false - } - Ok(false) => true, - Err(_) => true, // conservative: count unexpected errors as alive + // If the lock is acquired, the previous owner is gone; drop + // the file handle (releasing our just-acquired lock) and + // remove the marker. Anything else (`Ok(false)` meaning + // still held, `Err(_)` for unexpected IO errors) is treated + // conservatively as live. + if matches!(fabro_proc::try_flock_exclusive(&file), Ok(true)) { + drop(file); + let _ = std::fs::remove_file(path); + false + } else { + true } }) .count() From 4c4d4efcda9706857ce9365b4dae9204ff90e69e Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 20 Apr 2026 17:39:52 -0400 Subject: [PATCH 8/8] fix(cli): detect zombies in server stop poll loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `foreground_start_writes_tracing_to_storage_server_log` test consistently took ~10.4 s. 10.3 s of that was spent inside `fabro server stop`, which polls `process_running(pid)` every 100 ms until the server exits. The test's server is spawned as a child of the test process (`child.spawn()`), and the test only reaps it via `child.wait_with_output()` after `fabro server stop` returns. After Step A's revert, `process_running` is a plain `kill(pid, 0)`, which returns true for a zombie — so the poll saw the dead-but-unreaped server as alive and burned the full 10 s timeout. Add `fabro_proc::process_running_strict(pid)` — the same ps-shelling zombie-aware predicate commit 1ed8e6cbd introduced — and use it only in `fabro-cli`'s server stop poll. The hot paths that motivated Step A (test-harness marker scans, daemon-liveness probes) continue to use the cheap `process_running`. The ps cost (~2 ms per call) is paid at most once per 100 ms poll interval and only while the server process still exists. In a normal clean shutdown that's zero calls (process exits before the first poll). In the zombie scenario the loop exits after ~1 poll instead of running out the full timeout. Verified on this branch: cargo nextest run -p fabro-cli -E 'test(foreground_start_writes_tracing)' before: 10.48s, 10.45s, 10.42s after: 0.35s, 0.32s, 0.25s (30x faster) The zombie regression test removed in commit da87f978c returns as `process_running_strict_returns_false_for_unreaped_zombie_child`, and also asserts that the cheap `process_running` keeps its "zombie == alive" semantics so the harness hot paths stay honest. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../fabro-cli/src/commands/server/stop.rs | 11 ++- lib/crates/fabro-proc/src/lib.rs | 2 +- lib/crates/fabro-proc/src/signal.rs | 90 +++++++++++++++++-- 3 files changed, 93 insertions(+), 10 deletions(-) diff --git a/lib/crates/fabro-cli/src/commands/server/stop.rs b/lib/crates/fabro-cli/src/commands/server/stop.rs index 054b61218..09811c6bf 100644 --- a/lib/crates/fabro-cli/src/commands/server/stop.rs +++ b/lib/crates/fabro-cli/src/commands/server/stop.rs @@ -16,17 +16,24 @@ pub(crate) async fn stop_server(storage_dir: &Path, timeout: Duration) -> Result fabro_proc::sigterm(record.pid); + // Use the zombie-aware predicate here: this loop is commonly driven + // against a child of the calling process (tests, install/uninstall + // in-process shutdowns, a foreground-launching shell). A zombie + // child would otherwise satisfy `process_running` until its parent + // waits, causing us to burn the whole `timeout` on an already-dead + // process. The `ps` cost (~2 ms per poll) is trivial compared to + // the 10 s timeout and is only paid while the process still exists. let poll_interval = Duration::from_millis(100); let mut elapsed = Duration::ZERO; while elapsed < timeout { - if !fabro_proc::process_running(record.pid) { + if !fabro_proc::process_running_strict(record.pid) { break; } time::sleep(poll_interval).await; elapsed += poll_interval; } - if fabro_proc::process_running(record.pid) { + if fabro_proc::process_running_strict(record.pid) { fabro_proc::sigkill(record.pid); time::sleep(Duration::from_millis(100)).await; } diff --git a/lib/crates/fabro-proc/src/lib.rs b/lib/crates/fabro-proc/src/lib.rs index e97162293..5fbadb7bf 100644 --- a/lib/crates/fabro-proc/src/lib.rs +++ b/lib/crates/fabro-proc/src/lib.rs @@ -18,7 +18,7 @@ pub use pre_exec::pre_exec_pdeathsig; pub use pre_exec::pre_exec_setpgid; #[cfg(unix)] pub use pre_exec::pre_exec_setsid; -pub use signal::{process_exists, process_group_alive, process_running}; +pub use signal::{process_exists, process_group_alive, process_running, process_running_strict}; #[cfg(unix)] pub use signal::{ sigkill, sigkill_process_group, sigterm, sigterm_process_group, sigusr1, sigusr2, diff --git a/lib/crates/fabro-proc/src/signal.rs b/lib/crates/fabro-proc/src/signal.rs index 7325c3f3d..52c2eef71 100644 --- a/lib/crates/fabro-proc/src/signal.rs +++ b/lib/crates/fabro-proc/src/signal.rs @@ -21,16 +21,60 @@ pub fn process_exists(pid: u32) -> bool { /// Check whether a process with the given PID is still running. /// /// On Unix, delegates to `process_exists` (a `kill(pid, 0)` probe). Unreaped -/// zombies count as running here because callers that need to distinguish -/// zombies from live processes are a narrow minority; giving every caller -/// the zombie check would require spawning `ps` on every probe and would -/// dominate test-harness setup time at the scale we run it. If a caller -/// needs zombie-aware semantics, it should be introduced alongside that -/// caller with a benchmark in context. +/// zombies count as running here because the callers are hot paths (test +/// harness marker scans, daemon-liveness probes) where a zombie window is +/// sub-millisecond and the ~2 ms cost of an authoritative zombie check via +/// `ps` would dominate. For the narrow set of callers that genuinely need +/// to treat zombies as stopped (notably the `fabro server stop` polling +/// loop, which can wait out its full timeout on a zombie child), use +/// `process_running_strict`. pub fn process_running(pid: u32) -> bool { process_exists(pid) } +/// Like `process_running`, but treats zombie / defunct processes as not +/// running. +/// +/// On Unix, follows a cheap `kill(pid, 0)` probe with a `ps` shell-out to +/// read the process state character and excludes `Z`/`z` entries. Falls +/// back to `process_exists(pid)` when the `ps` probe fails, preserving the +/// old conservative behavior. On non-unix, identical to `process_exists`. +/// +/// Prefer `process_running` unless you are polling for a child you cannot +/// `wait()` on — the `ps` invocation costs ~2 ms per call on macOS and is +/// wasted on hot paths that have no zombie exposure. +pub fn process_running_strict(pid: u32) -> bool { + #[cfg(unix)] + { + if !process_exists(pid) { + return false; + } + unix_process_state(pid).is_none_or(|state| !matches!(state, 'Z' | 'z')) + } + #[cfg(not(unix))] + { + process_exists(pid) + } +} + +#[cfg(unix)] +#[expect( + clippy::disallowed_methods, + reason = "Unix process-state detection shells out to ps to distinguish running processes from zombies" +)] +fn unix_process_state(pid: u32) -> Option { + let output = std::process::Command::new("ps") + .args(["-ww", "-o", "stat=", "-p", &pid.to_string()]) + .output() + .ok()?; + if !output.status.success() { + return None; + } + String::from_utf8_lossy(&output.stdout) + .chars() + .find(|ch| !ch.is_whitespace()) +} + /// Check whether any process in the given process group is alive. /// /// On Unix, sends signal 0 to `-pgid` via `kill(2)`. Returns `false` if the @@ -128,13 +172,45 @@ mod tests { use std::process::{Command, Stdio}; use std::time::Duration; - use super::{process_exists, process_group_alive, process_running}; + use super::{process_exists, process_group_alive, process_running, process_running_strict}; use crate::pre_exec::pre_exec_setpgid; #[test] fn process_running_returns_true_for_current_process() { assert!(process_exists(std::process::id())); assert!(process_running(std::process::id())); + assert!(process_running_strict(std::process::id())); + } + + #[cfg(unix)] + #[test] + #[expect( + clippy::disallowed_methods, + reason = "zombie-detection test needs to spawn a short-lived child and intentionally leave it unreaped" + )] + fn process_running_strict_returns_false_for_unreaped_zombie_child() { + let mut child = Command::new("sh") + .args(["-c", "exit 0"]) + .spawn() + .expect("short-lived child should spawn"); + let pid = child.id(); + + std::thread::sleep(Duration::from_millis(100)); + + assert!( + process_exists(pid), + "unreaped zombie should still have a visible pid" + ); + assert!( + process_running(pid), + "cheap process_running treats zombies as alive by design" + ); + assert!( + !process_running_strict(pid), + "process_running_strict should treat zombies as stopped" + ); + + let _status = child.wait().expect("child should remain waitable"); } #[cfg(unix)]