scriptoriumadmin/fabro
1
0
Fork 0
mirror of https://github.com/fabro-sh/fabro.git synced 2026-08-28 05:27:41 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 4

fix(proc): revert process_running to cheap kill(0) probe

Browse source 
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:🆕  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) <noreply@anthropic.com>
This commit is contained in:
Bryan Helmkamp 2026-04-20 16:24:42 -04:00
parent 24e7e5af83
commit da87f978cd
No known key found for this signature in database
1 changed files with 8 additions and 62 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

70
lib/crates/fabro-proc/src/signal.rs
View file

@ -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<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())
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(