From 1ed8e6cbd53ebc762f7d9b9e4c4765d8f976e6fb Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 20 Apr 2026 09:04:50 -0400 Subject: [PATCH] fix(server): treat zombie processes as stopped Split raw PID existence from actual process liveness in fabro-proc and switch the server shutdown paths to the running-process predicate. This avoids waiting out stop timeouts for unreaped zombie children while keeping process-group behavior covered by measured regression tests. --- .../fabro-cli/src/commands/server/record.rs | 2 +- .../fabro-cli/src/commands/server/stop.rs | 4 +- lib/crates/fabro-proc/src/lib.rs | 2 +- lib/crates/fabro-proc/src/signal.rs | 150 +++++++++++++++++- lib/crates/fabro-server/src/server.rs | 6 +- lib/crates/fabro-test/src/lib.rs | 10 +- 6 files changed, 160 insertions(+), 14 deletions(-) diff --git a/lib/crates/fabro-cli/src/commands/server/record.rs b/lib/crates/fabro-cli/src/commands/server/record.rs index d48088977..23481d8dd 100644 --- a/lib/crates/fabro-cli/src/commands/server/record.rs +++ b/lib/crates/fabro-cli/src/commands/server/record.rs @@ -48,7 +48,7 @@ pub(crate) fn remove_server_record(path: &Path) { } pub(crate) fn server_record_is_running(record: &ServerRecord) -> bool { - fabro_proc::process_alive(record.pid) && server_process_matches(record) + fabro_proc::process_running(record.pid) && server_process_matches(record) } fn server_record_path(storage_dir: &Path) -> PathBuf { diff --git a/lib/crates/fabro-cli/src/commands/server/stop.rs b/lib/crates/fabro-cli/src/commands/server/stop.rs index dc83f55d1..054b61218 100644 --- a/lib/crates/fabro-cli/src/commands/server/stop.rs +++ b/lib/crates/fabro-cli/src/commands/server/stop.rs @@ -19,14 +19,14 @@ pub(crate) async fn stop_server(storage_dir: &Path, timeout: Duration) -> Result let poll_interval = Duration::from_millis(100); let mut elapsed = Duration::ZERO; while elapsed < timeout { - if !fabro_proc::process_alive(record.pid) { + if !fabro_proc::process_running(record.pid) { break; } time::sleep(poll_interval).await; elapsed += poll_interval; } - if fabro_proc::process_alive(record.pid) { + if fabro_proc::process_running(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 85e240ef5..57f9c3c69 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_alive, process_group_alive}; +pub use signal::{process_exists, process_group_alive, process_running}; #[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 188e60426..04f650030 100644 --- a/lib/crates/fabro-proc/src/signal.rs +++ b/lib/crates/fabro-proc/src/signal.rs @@ -1,8 +1,8 @@ -/// Check whether a process with the given PID is alive. +/// Check whether a process with the given PID currently exists. /// /// On Unix, sends signal 0 via `kill(2)`. Returns `false` if the pid does not /// fit in `i32`. On non-Unix platforms, conservatively returns `true`. -pub fn process_alive(pid: u32) -> bool { +pub fn process_exists(pid: u32) -> bool { #[cfg(unix)] { let Ok(pid) = i32::try_from(pid) else { @@ -18,6 +18,46 @@ pub fn process_alive(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. +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()) +} + /// 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 @@ -104,3 +144,109 @@ pub fn sigusr2(pid: u32) { } } } + +#[cfg(test)] +mod tests { + use std::io::{BufRead, BufReader}; + use std::process::{Command, Stdio}; + use std::time::Duration; + + use super::{process_exists, process_group_alive, process_running}; + + #[test] + fn process_running_returns_true_for_current_process() { + assert!(process_exists(std::process::id())); + 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( + clippy::disallowed_methods, + reason = "process-group test spawns a child in its own process group and observes the group probe" + )] + fn process_group_alive_returns_true_for_running_process_group() { + let mut child = Command::new("sh"); + child + .args(["-c", "sleep 5"]) + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::null()); + crate::pre_exec::pre_exec_setpgid(&mut child); + let mut child = child.spawn().expect("group leader should spawn"); + let pgid = child.id(); + + assert!( + process_group_alive(pgid), + "running process group should count as alive" + ); + + let _ = child.kill(); + let _ = child.wait(); + } + + #[cfg(unix)] + #[test] + #[expect( + clippy::disallowed_methods, + reason = "process-group zombie test uses a short perl helper that forks without reaping its child" + )] + fn process_group_alive_returns_false_for_zombie_only_process_group() { + let mut parent = Command::new("perl"); + parent + .args([ + "-MPOSIX", + "-e", + r#"$|=1; $pid=fork(); die $! unless defined $pid; if(!$pid){ POSIX::setpgid(0,0) or die $!; exit 0 } print "$pid\n"; sleep 5;"#, + ]) + .stdin(Stdio::null()) + .stdout(Stdio::piped()) + .stderr(Stdio::inherit()); + let mut parent = parent.spawn().expect("zombie parent helper should spawn"); + let stdout = parent.stdout.take().expect("helper stdout should be piped"); + let mut lines = BufReader::new(stdout).lines(); + let child_pid = lines + .next() + .expect("helper should print child pid") + .expect("helper child pid should read") + .parse::() + .expect("helper child pid should parse"); + + std::thread::sleep(Duration::from_millis(100)); + + assert!( + !process_group_alive(child_pid), + "zombie-only process group should not count as alive" + ); + + let _ = parent.kill(); + let _ = parent.wait(); + } +} diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index 8ec8229dc..e6422a938 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -3148,15 +3148,15 @@ async fn terminate_worker_for_deletion( fabro_proc::sigterm(worker_pid); let deadline = Instant::now() + grace; - while Instant::now() < deadline && fabro_proc::process_alive(worker_pid) { + while Instant::now() < deadline && fabro_proc::process_running(worker_pid) { sleep(Duration::from_millis(50)).await; } - if fabro_proc::process_alive(worker_pid) { + if fabro_proc::process_running(worker_pid) { fabro_proc::sigkill(worker_pid); let kill_deadline = Instant::now() + Duration::from_secs(1); - while Instant::now() < kill_deadline && fabro_proc::process_alive(worker_pid) { + while Instant::now() < kill_deadline && fabro_proc::process_running(worker_pid) { sleep(Duration::from_millis(50)).await; } } diff --git a/lib/crates/fabro-test/src/lib.rs b/lib/crates/fabro-test/src/lib.rs index 301e41b4d..da412c96c 100644 --- a/lib/crates/fabro-test/src/lib.rs +++ b/lib/crates/fabro-test/src/lib.rs @@ -222,7 +222,7 @@ pub fn stop_pid(pid: u32) { fabro_proc::sigterm(pid); let deadline = std::time::Instant::now() + std::time::Duration::from_secs(5); while std::time::Instant::now() < deadline { - if !fabro_proc::process_alive(pid) { + if !fabro_proc::process_running(pid) { return; } poll_sleep(); @@ -483,7 +483,7 @@ fn live_marker_count(root: &Path) -> usize { .map(|pid| (pid, entry.path())) }) .filter(|(pid, path)| { - if fabro_proc::process_alive(*pid) { + if fabro_proc::process_running(*pid) { true } else { let _ = std::fs::remove_file(path); @@ -749,7 +749,7 @@ fn server_record_pid(storage_dir: &Path) -> Option { } fn server_running(server: &ServerPaths) -> bool { - server_record_pid(&server.storage_dir).is_some_and(fabro_proc::process_alive) + server_record_pid(&server.storage_dir).is_some_and(fabro_proc::process_running) } #[expect( @@ -832,11 +832,11 @@ fn stop_test_server(server: &ServerPaths) { let poll = std::time::Duration::from_millis(50); let timeout = test_server_stop_timeout(); let mut elapsed = std::time::Duration::ZERO; - while elapsed < timeout && fabro_proc::process_alive(pid) { + while elapsed < timeout && fabro_proc::process_running(pid) { std::thread::sleep(poll); elapsed += poll; } - if fabro_proc::process_alive(pid) { + if fabro_proc::process_running(pid) { fabro_proc::sigkill(pid); }