From 9eb6d082db47e150c7aa31bdb8777537e4dd977a Mon Sep 17 00:00:00 2001 From: Jakub Date: Tue, 21 Jul 2026 15:29:29 +0200 Subject: [PATCH] fix(core): stop counting unreaped processes as running MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `is_process_alive` was a bare `kill(pid, 0)` on unix, which succeeds for a zombie — a process that has exited but whose parent has not reaped it. The desktop holds its spawned daemon's `Child` handle without waiting on it, so an exited UI-owned daemon stayed "alive" to every pid probe until that handle was finally reaped. Two consequences. On a clean quit, `wait_for_pid_exit` could never observe its own child's exit, so shutdown sat out the full 3s timeout, logged a bogus "did not exit gracefully" and SIGKILLed a corpse. And a crashed daemon still looked live to `running_daemon_in`, sending startup down the attach branch into a hard error instead of respawning. Every caller means *running* — a zombie answers no requests, serves no port, and holds no lock (its fds close at exit) — so zombies now count as dead. Windows already behaved this way: its process handle is signalled at exit. macOS has no direct zombie predicate, so this asks `proc_pidinfo` for the BSD record, which the kernel refuses with ESRCH once the process is gone. Apple's wrapper collapses the kernel's -1 into a return of 0 and reports the reason through errno, so the return value alone cannot separate "pid is gone" from "the call failed" — EPERM and ENOMEM also return 0. Only ESRCH counts as exited; anything else keeps the pre-fix answer, since declaring a live daemon dead and respawning over it is the worse error. Linux reads the state character from /proc//stat, as bytes rather than a String — `comm` is the raw executable name and need not be valid UTF-8. Other unix targets keep the old behaviour. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Y4Aq3X2s6ftafXzmqaN3A8 --- crates/okena-core/src/process/mod.rs | 119 ++++++++++++++++++++++++++- 1 file changed, 118 insertions(+), 1 deletion(-) diff --git a/crates/okena-core/src/process/mod.rs b/crates/okena-core/src/process/mod.rs index a77f36613..c72243f24 100644 --- a/crates/okena-core/src/process/mod.rs +++ b/crates/okena-core/src/process/mod.rs @@ -91,12 +91,91 @@ pub fn open_url(url: &str) { } } +/// Whether `pid` has exited but not yet been reaped by its parent. +/// +/// macOS: `proc_pidinfo(PROC_PIDTBSDINFO)` fills a record for a live process +/// and fails with `ESRCH` once it has exited — including while it is still a +/// zombie and `kill(pid, 0)` reports it present. +#[cfg(target_os = "macos")] +fn is_zombie(pid: u32) -> bool { + /// `PROC_PIDTBSDINFO` from ``. + const PROC_PIDTBSDINFO: libc::c_int = 3; + + // The record is 136 bytes today; over-size the buffer so a larger one on a + // future release still fits (too small is an error, too large is fine). + let mut info = [0u8; 512]; + let written = unsafe { + // Apple's wrapper collapses the kernel's -1 into a return of 0 and + // reports the reason through errno, so the return value alone cannot + // tell "this pid is gone" from "the call failed". Clear errno first so + // an unrelated earlier failure can't be mistaken for ours. + *libc::__error() = 0; + libc::proc_pidinfo( + pid as libc::c_int, + PROC_PIDTBSDINFO, + 0, + info.as_mut_ptr().cast(), + info.len() as libc::c_int, + ) + }; + if written > 0 { + return false; + } + + // Only "no such process" means the pid has exited. The other ways this + // returns 0 — EPERM for a process we may not inspect, ENOMEM for a buffer + // the kernel considers too small — say nothing about liveness, and we + // already know from `kill(pid, 0)` that the pid exists. Treat those as + // running: keeping the old answer beats declaring a live daemon dead and + // respawning on top of it. + std::io::Error::last_os_error().raw_os_error() == Some(libc::ESRCH) +} + +/// Linux: field 3 of `/proc//stat` is the state character, `Z` for zombie. +/// The preceding `comm` field is parenthesised and may itself contain spaces +/// and parentheses, so the scan starts after its last `)`. +#[cfg(target_os = "linux")] +fn is_zombie(pid: u32) -> bool { + // Read bytes, not a String: `comm` is the raw executable name and need not + // be valid UTF-8, which would otherwise fail the read and report a zombie + // as running. + let Ok(stat) = std::fs::read(format!("/proc/{pid}/stat")) else { + return false; + }; + let stat = String::from_utf8_lossy(&stat); + let Some((_, after_comm)) = stat.rsplit_once(')') else { + return false; + }; + after_comm.split_whitespace().next() == Some("Z") +} + +/// Other unix targets keep the old behaviour: no cheap zombie probe, so a +/// pid in the table counts as alive. +#[cfg(all(unix, not(any(target_os = "macos", target_os = "linux"))))] +fn is_zombie(_pid: u32) -> bool { + false +} + /// Check whether a process with the given PID is still running. +/// +/// A process that has exited but whose parent has not reaped it yet keeps its +/// pid in the table, so `kill(pid, 0)` still succeeds for it. Every caller here +/// means *running* — a zombie answers no requests, serves no port and holds no +/// lock — so zombies count as dead. This matters for processes we spawn +/// ourselves and reap late: the desktop holds its daemon's `Child` handle, so a +/// daemon that exited stayed "alive" to every pid probe until the handle was +/// finally waited on, which made a clean shutdown burn its whole timeout and a +/// crashed daemon look reachable. +/// +/// Windows needs no equivalent: its process handle is signalled at exit. pub fn is_process_alive(pid: u32) -> bool { #[cfg(unix)] { // signal 0 probes for existence without delivering a signal. - unsafe { libc::kill(pid as libc::pid_t, 0) == 0 } + if unsafe { libc::kill(pid as libc::pid_t, 0) } != 0 { + return false; + } + !is_zombie(pid) } #[cfg(windows)] { @@ -303,4 +382,42 @@ mod tests { let out = safe_output(&mut cmd).expect("mock"); assert_eq!(out.stdout, b"mocked"); } + + /// A child that exited but has not been `wait`ed on is a zombie: its pid is + /// still in the table, so `kill(pid, 0)` succeeds. Reporting that as alive + /// made the desktop's shutdown wait sit out its whole timeout on a corpse + /// and a crashed daemon look reachable. + #[cfg(unix)] + #[test] + fn exited_but_unreaped_child_is_not_alive() { + let mut child = std::process::Command::new("/bin/sh") + .args(["-c", "exit 0"]) + .spawn() + .expect("spawn"); + let pid = child.id(); + // Let it exit. Deliberately do NOT reap it yet. + std::thread::sleep(Duration::from_millis(300)); + + let alive = is_process_alive(pid); + let _ = child.wait(); + + assert!(!alive, "an exited, unreaped child must not count as running"); + } + + #[cfg(unix)] + #[test] + fn running_processes_are_alive() { + assert!(is_process_alive(std::process::id()), "this test process"); + + let mut child = std::process::Command::new("/bin/sleep") + .arg("30") + .spawn() + .expect("spawn"); + std::thread::sleep(Duration::from_millis(300)); + let alive = is_process_alive(child.id()); + let _ = child.kill(); + let _ = child.wait(); + + assert!(alive, "a live child must count as running"); + } }