From 00a63e9be5283d851de3f68fa664dec46e152003 Mon Sep 17 00:00:00 2001 From: Aditya Date: Sat, 22 Aug 2026 00:59:24 -0500 Subject: [PATCH] fix(agent): a group with nothing left in it is not a failed kill The sandbox tests failed about once in twenty-five runs on macOS with "kill process group: Operation not permitted (os error 1)", taking whichever test happened to lose the race. It looked like a test problem and was not: the same flake reproduces on main with none of the recent work applied, and it can hit any sandbox test. The sequence is narrow. child_exited_unreaped confirms the child is gone, the pipes are not closed yet, so the loop kills the process group to clear any straggler, and then reports a failure if that kill did not work. By then the group can hold nothing but the unreaped zombie, and macOS answers EPERM where Linux answers ESRCH. ESRCH was already forgiven, on the correct reasoning that a group with nothing in it is the outcome the kill wanted. EPERM is the same fact with a different number, and it was not forgiven, so a command that had already succeeded failed while tidying up something that was no longer there. Both errnos now mean the same thing, behind a named predicate so the reasoning has somewhere to live. Anything else still fails the run. The forgiving direction is the dangerous one here, since it could hide a kill that genuinely did not happen, so the set stays at exactly these two and the test pins that EINVAL is still an error. Twenty-five consecutive runs of the sandbox module are clean where the same loop produced a failure before. That is consistent rather than conclusive at this sample size; the argument that should carry weight is the mechanical one, that the kill is now told the truth about a group it has already outlived. Claude-Session: https://claude.ai/code/session_01KGPVQ8wUG7h36zashWYCp4 --- zorp-agent/src/sandbox/mod.rs | 48 ++++++++++++++++++++++++++++++++++- 1 file changed, 47 insertions(+), 1 deletion(-) diff --git a/zorp-agent/src/sandbox/mod.rs b/zorp-agent/src/sandbox/mod.rs index 6bbfd90..7e4b0a5 100644 --- a/zorp-agent/src/sandbox/mod.rs +++ b/zorp-agent/src/sandbox/mod.rs @@ -254,12 +254,31 @@ fn child_exited_unreaped(pid: i32) -> Result { Ok(unsafe { info.si_pid() } != 0) } +/// Whether a failed group kill means the group had nothing left to kill. +/// +/// ESRCH is the obvious one: no such process group. EPERM is the same +/// situation wearing a different errno. Every member of this group is a +/// process we spawned, so we are always allowed to signal it while it is +/// alive; the kernel only refuses when there is nothing signalable there. +/// On macOS a group whose last member is an exited-but-unreaped child +/// answers EPERM rather than ESRCH, and both callers below reach this +/// after the child is known to have exited. +/// +/// Anything else is a real error and still fails the run. Getting this +/// wrong in the forgiving direction would hide a kill that did not happen; +/// getting it wrong in the strict direction is what produced a flaky test +/// suite, because a command that had already succeeded failed while +/// cleaning up something that was no longer there. +fn kill_error_is_already_gone(error: &io::Error) -> bool { + matches!(error.raw_os_error(), Some(libc::ESRCH) | Some(libc::EPERM)) +} + fn kill_process_group(pgid: i32) -> io::Result<()> { if unsafe { libc::kill(-pgid, libc::SIGKILL) } == 0 { return Ok(()); } let error = io::Error::last_os_error(); - if error.raw_os_error() == Some(libc::ESRCH) { + if kill_error_is_already_gone(&error) { Ok(()) } else { Err(error) @@ -308,6 +327,33 @@ fn contains_ascii_case_insensitive(haystack: &[u8], needle: &[u8]) -> bool { #[cfg(test)] mod tests { use super::*; + + // The three errnos that decide whether a failed group kill is a real + // failure. ESRCH and EPERM both mean the group had nothing left in it + // that we could signal, which is the outcome the kill wanted. EINVAL + // means the call itself was wrong and must still be reported. + // + // EPERM is the one that used to be missing, and it was not theoretical: + // `cargo test -p zorp-agent --lib sandbox::` failed roughly once in + // twenty-five runs on macOS with "kill process group: Operation not + // permitted (os error 1)", taking whichever sandbox test happened to + // lose the race. The child is already confirmed dead by + // `child_exited_unreaped` before this kill is attempted, so on a group + // holding only an unreaped zombie macOS answers EPERM where Linux + // answers ESRCH. Same fact, different errno, and only one of them was + // being forgiven. + #[test] + fn a_group_that_is_already_gone_is_not_a_failed_kill() { + assert!(kill_error_is_already_gone(&io::Error::from_raw_os_error( + libc::ESRCH + ))); + assert!(kill_error_is_already_gone(&io::Error::from_raw_os_error( + libc::EPERM + ))); + assert!(!kill_error_is_already_gone(&io::Error::from_raw_os_error( + libc::EINVAL + ))); + } use std::os::unix::ffi::OsStringExt; use std::sync::Mutex; use std::thread;