mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-10-08 03:10:26 +00:00
fix(cli): detect zombies in server stop poll loop
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 commit1ed8e6cbdintroduced — 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 commitda87f978creturns 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) <noreply@anthropic.com>
This commit is contained in:
parent
224ce9e21e
commit
4c4d4efcda
3 changed files with 93 additions and 10 deletions
|
|
@ -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;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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<char> {
|
||||
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)]
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue