From e863d4d51dc15d797d8d8980e8ce426f490ef5c5 Mon Sep 17 00:00:00 2001 From: Maximilian Roos Date: Sun, 23 Aug 2026 11:44:31 -0700 Subject: [PATCH 1/5] =?UTF-8?q?fix:=20make=20TERM=E2=86=92KILL=20escalatio?= =?UTF-8?q?n=20poll=20for=20group=20death=20and=20document=20the=20zombie-?= =?UTF-8?q?probe=20invariant?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit process_group_alive counts an exited-but-unreaped member as alive (Linux: Ok, macOS: EPERM), so on the timed-out-command path — whose caller holds the group leader unreaped — the group read alive for the whole grace and the final SIGKILL always fired. That sweep is provably inert against a dead group (signals to fully-exited processes are discarded), so the fix reframes rather than re-plumbs: the probe is now an early-exit optimization (20ms poll, immediate first probe) and the post-grace SIGKILL is the documented straggler sweep. Callers whose children are reaped concurrently (Cmd::stream's signal forwarder, tether) now return within one poll interval instead of always sleeping the full 200ms per escalation step. Co-Authored-By: Claude Fable 5 --- src/shell_exec.rs | 153 +++++++++++++++++++++++++++++++++++++++------- 1 file changed, 132 insertions(+), 21 deletions(-) diff --git a/src/shell_exec.rs b/src/shell_exec.rs index 53fcfc17d5..738c8fae56 100644 --- a/src/shell_exec.rs +++ b/src/shell_exec.rs @@ -1053,6 +1053,12 @@ fn run_with_timeout_impl( /// that exited on the deadline's other side is a zombie that keeps its pid /// reserved until the caller's own `wait()`. The pid cannot name a different /// process group by the time the signal lands. +/// +/// The same unreaped zombie means the escalation's liveness probe reads the +/// group as alive for the entire grace, so its final SIGKILL fires even when +/// every member exited on the TERM. Accepted: that sweep is a no-op against a +/// dead group (see [`forward_signal_with_escalation`]), and holding the zombie +/// is what pins the pgid. #[cfg(unix)] fn kill_timed_out_tree(pid: u32) { forward_signal_with_escalation(pid as i32, signal_hook::consts::SIGTERM); @@ -2257,6 +2263,12 @@ impl Cmd { // Signal forwarding helpers (Unix only) // ============================================================================ +/// One `killpg(pgid, 0)` liveness probe. Only `ESRCH` proves the group empty; +/// everything else counts as alive. That makes the probe conservative in one +/// specific way: an exited-but-unreaped member (a zombie) still registers — +/// Linux answers `Ok`, macOS `EPERM` — so a group whose members all exited +/// keeps reading alive until someone reaps them. See +/// [`forward_signal_with_escalation`] for why that over-report is safe. #[cfg(unix)] fn process_group_alive(pgid: i32) -> bool { match nix::sys::signal::killpg(nix::unistd::Pid::from_raw(pgid), None) { @@ -2266,10 +2278,25 @@ fn process_group_alive(pgid: i32) -> bool { } } +/// Poll [`process_group_alive`] until the group is gone or `grace` expires, +/// returning `true` when the group died within the grace. The first probe is +/// immediate, so an already-empty group costs no sleep at all, and a group +/// whose members exit (and are reaped) mid-grace is noticed within one poll +/// interval rather than at the deadline. #[cfg(unix)] -fn wait_for_exit(pgid: i32, grace: std::time::Duration) -> bool { - std::thread::sleep(grace); - !process_group_alive(pgid) +fn group_died_within(pgid: i32, grace: Duration) -> bool { + const POLL_INTERVAL: Duration = Duration::from_millis(20); + let deadline = Instant::now() + grace; + loop { + if !process_group_alive(pgid) { + return true; + } + let remaining = deadline.saturating_duration_since(Instant::now()); + if remaining.is_zero() { + return false; + } + std::thread::sleep(remaining.min(POLL_INTERVAL)); + } } /// Single-shot signal delivery to a specific PID. Used in shared-pgroup mode @@ -2288,32 +2315,50 @@ pub fn forward_signal_to_pid(pid: i32, sig: i32) { let _ = nix::sys::signal::kill(nix::unistd::Pid::from_raw(pid), nix_sig); } +/// Signal a child process group and sweep stragglers: send `sig` (SIGINT or +/// SIGTERM; anything else is ignored), give the group a 200 ms grace to exit, +/// and SIGKILL whatever still remains. SIGINT inserts a SIGTERM round (with +/// its own grace) before the SIGKILL, so an interrupted git still runs its +/// TERM-time lockfile cleanup before force-kill. +/// +/// "Still remains" is [`process_group_alive`]'s answer, and that probe counts +/// an exited-but-unreaped member as alive — it cannot tell a zombie from live +/// work. The over-report errs in the safe direction on both sides: +/// +/// - Every member already exited, the leader just isn't reaped yet: the grace +/// runs to its deadline and the final SIGKILL lands on a dead group, which +/// is a no-op — a signal to a fully-exited process is discarded and cannot +/// change its recorded exit status, so TERM-time cleanup that already ran is +/// not undone. [`kill_timed_out_tree`] is permanently in this position: its +/// caller holds the group leader unreaped throughout (see its doc), so on +/// that path the sweep always fires, harmlessly. +/// - A member is genuinely alive at the deadline: the probe is accurate and +/// the SIGKILL is the intended escalation. +/// +/// Callers whose children are reaped concurrently — `Cmd::stream`'s main +/// thread waiting while the signal-forwarder thread runs this, tether's +/// supervisor — get the accurate reading: the poll loop returns at the first +/// probe after the reap, so escalating over a cooperative child costs one +/// poll interval, not the full grace. #[cfg(unix)] pub fn forward_signal_with_escalation(pgid: i32, sig: i32) { + use nix::sys::signal::Signal; + let pgid = nix::unistd::Pid::from_raw(pgid); - let initial_signal = match sig { - signal_hook::consts::SIGINT => nix::sys::signal::Signal::SIGINT, - signal_hook::consts::SIGTERM => nix::sys::signal::Signal::SIGTERM, + let chain: &[Signal] = match sig { + signal_hook::consts::SIGINT => &[Signal::SIGINT, Signal::SIGTERM], + signal_hook::consts::SIGTERM => &[Signal::SIGTERM], _ => return, }; - let _ = nix::sys::signal::killpg(pgid, initial_signal); - - let grace = std::time::Duration::from_millis(200); - // Escalate if process doesn't exit gracefully - if sig == signal_hook::consts::SIGINT { - if !wait_for_exit(pgid.as_raw(), grace) { - let _ = nix::sys::signal::killpg(pgid, nix::sys::signal::Signal::SIGTERM); - if !wait_for_exit(pgid.as_raw(), grace) { - let _ = nix::sys::signal::killpg(pgid, nix::sys::signal::Signal::SIGKILL); - } - } - } else { - // SIGTERM - escalate directly to SIGKILL - if !wait_for_exit(pgid.as_raw(), grace) { - let _ = nix::sys::signal::killpg(pgid, nix::sys::signal::Signal::SIGKILL); + let grace = Duration::from_millis(200); + for step in chain { + let _ = nix::sys::signal::killpg(pgid, *step); + if group_died_within(pgid.as_raw(), grace) { + return; } } + let _ = nix::sys::signal::killpg(pgid, Signal::SIGKILL); } #[cfg(test)] @@ -3103,6 +3148,72 @@ mod tests { // No panic = success (function returns early for unknown signals) } + #[test] + #[cfg(unix)] + fn test_group_died_within_immediate_for_reaped_group() { + // A reaped child leaves an empty group: the first (immediate) probe + // reads ESRCH and the grace loop returns without sleeping. This pins + // the early exit that keeps escalation cheap for the callers whose + // children are reaped concurrently (signal forwarder, tether). + use std::os::unix::process::CommandExt; + let mut cmd = std::process::Command::new("sh"); + cmd.args(["-c", ":"]).process_group(0); + let mut child = cmd.spawn().unwrap(); + let pgid = child.id() as i32; + child.wait().unwrap(); + + let start = Instant::now(); + assert!(super::group_died_within(pgid, Duration::from_millis(200))); + assert!( + start.elapsed() < Duration::from_millis(100), + "an empty group must exit the grace loop on the first probe; took {:?}", + start.elapsed() + ); + } + + #[test] + #[cfg(unix)] + fn test_group_died_within_times_out_on_live_group() { + // Probing our own (live) process group runs the full grace and + // reports the group still alive. + let pgid = nix::unistd::getpgrp().as_raw(); + let grace = Duration::from_millis(50); + let start = Instant::now(); + assert!(!super::group_died_within(pgid, grace)); + assert!(start.elapsed() >= grace); + } + + #[test] + #[cfg(unix)] + fn test_escalation_preserves_term_exit_when_leader_unreaped() { + // Replays `kill_timed_out_tree`'s sequence: the caller holds the group + // leader unreaped while escalating, so the child's TERM-time exit + // leaves a zombie the liveness probe counts as alive, the grace runs + // to its deadline, and the final group SIGKILL fires anyway. The + // recorded exit must still be the SIGTERM — the sweep against an + // already-dead group may not alter it. + use std::os::unix::process::CommandExt; + use std::os::unix::process::ExitStatusExt; + let mut cmd = std::process::Command::new("sleep"); + cmd.arg("30").process_group(0); + let mut child = cmd.spawn().unwrap(); + let pid = child.id() as i32; + + let start = Instant::now(); + super::forward_signal_with_escalation(pid, signal_hook::consts::SIGTERM); + assert!( + start.elapsed() >= Duration::from_millis(200), + "with the leader unreaped the group must read alive for the whole grace" + ); + + let status = child.wait().unwrap(); + assert_eq!( + status.signal(), + Some(signal_hook::consts::SIGTERM), + "the post-grace SIGKILL must not change the recorded cause of death" + ); + } + #[test] fn test_format_stream_full_empty() { assert!(format_stream_full(b"", " ").is_empty()); From 8db9ac85e5ded92c5854e1d48b13ef25d6e57826 Mon Sep 17 00:00:00 2001 From: Maximilian Roos Date: Sun, 23 Aug 2026 11:54:10 -0700 Subject: [PATCH 2/5] test: make the unreaped-leader escalation test scheduling-independent Barrier on the child's stdout EOF (the pipe closes at process exit) instead of racing TERM delivery against the grace: the child exits on its own before escalation starts, so no CI-scheduling assumption remains, and the preserved exit code pins that both the TERM and the post-grace sweep are inert against an exited-but-unreaped group. Co-Authored-By: Claude Fable 5 --- src/shell_exec.rs | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/src/shell_exec.rs b/src/shell_exec.rs index 738c8fae56..d72a7b5954 100644 --- a/src/shell_exec.rs +++ b/src/shell_exec.rs @@ -3185,19 +3185,24 @@ mod tests { #[test] #[cfg(unix)] - fn test_escalation_preserves_term_exit_when_leader_unreaped() { - // Replays `kill_timed_out_tree`'s sequence: the caller holds the group - // leader unreaped while escalating, so the child's TERM-time exit - // leaves a zombie the liveness probe counts as alive, the grace runs - // to its deadline, and the final group SIGKILL fires anyway. The - // recorded exit must still be the SIGTERM — the sweep against an - // already-dead group may not alter it. + fn test_escalation_full_grace_and_inert_sweep_when_leader_unreaped() { + // Replays `kill_timed_out_tree`'s position: the caller holds the group + // leader unreaped while escalating. The child has already exited when + // escalation starts (stdout EOF is the barrier — the pipe closes when + // the process exits, so this needs no scheduling assumptions), but + // nobody reaps it, so the liveness probe counts the zombie, the grace + // runs to its deadline, and the final group SIGKILL fires against the + // dead group. The recorded exit must come through untouched — signals + // to a fully-exited group are discarded. use std::os::unix::process::CommandExt; - use std::os::unix::process::ExitStatusExt; - let mut cmd = std::process::Command::new("sleep"); - cmd.arg("30").process_group(0); + let mut cmd = std::process::Command::new("sh"); + cmd.args(["-c", "exit 7"]) + .stdout(std::process::Stdio::piped()) + .process_group(0); let mut child = cmd.spawn().unwrap(); let pid = child.id() as i32; + let mut eof = Vec::new(); + child.stdout.take().unwrap().read_to_end(&mut eof).unwrap(); let start = Instant::now(); super::forward_signal_with_escalation(pid, signal_hook::consts::SIGTERM); @@ -3208,9 +3213,9 @@ mod tests { let status = child.wait().unwrap(); assert_eq!( - status.signal(), - Some(signal_hook::consts::SIGTERM), - "the post-grace SIGKILL must not change the recorded cause of death" + status.code(), + Some(7), + "the TERM and the post-grace SIGKILL must not alter the recorded exit" ); } From d92f313fdce1bca81d76925da5811a424c012b06 Mon Sep 17 00:00:00 2001 From: Maximilian Roos Date: Sun, 23 Aug 2026 11:56:59 -0700 Subject: [PATCH 3/5] docs: unlink private items from the public escalation docstring Co-Authored-By: Claude Fable 5 --- src/shell_exec.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/shell_exec.rs b/src/shell_exec.rs index d72a7b5954..4d9c8f2ffb 100644 --- a/src/shell_exec.rs +++ b/src/shell_exec.rs @@ -2321,7 +2321,7 @@ pub fn forward_signal_to_pid(pid: i32, sig: i32) { /// its own grace) before the SIGKILL, so an interrupted git still runs its /// TERM-time lockfile cleanup before force-kill. /// -/// "Still remains" is [`process_group_alive`]'s answer, and that probe counts +/// "Still remains" is `process_group_alive`'s answer, and that probe counts /// an exited-but-unreaped member as alive — it cannot tell a zombie from live /// work. The over-report errs in the safe direction on both sides: /// @@ -2329,7 +2329,7 @@ pub fn forward_signal_to_pid(pid: i32, sig: i32) { /// runs to its deadline and the final SIGKILL lands on a dead group, which /// is a no-op — a signal to a fully-exited process is discarded and cannot /// change its recorded exit status, so TERM-time cleanup that already ran is -/// not undone. [`kill_timed_out_tree`] is permanently in this position: its +/// not undone. `kill_timed_out_tree` is permanently in this position: its /// caller holds the group leader unreaped throughout (see its doc), so on /// that path the sweep always fires, harmlessly. /// - A member is genuinely alive at the deadline: the probe is accurate and From 0f1c4a94003325c27a2addbbb5103a194115352f Mon Sep 17 00:00:00 2001 From: Maximilian Roos Date: Sun, 23 Aug 2026 12:00:38 -0700 Subject: [PATCH 4/5] docs: note the SIGSTOP straggler and the reaped-path recycling residual Co-Authored-By: Claude Fable 5 --- src/shell_exec.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/shell_exec.rs b/src/shell_exec.rs index 4d9c8f2ffb..8a0ebb5814 100644 --- a/src/shell_exec.rs +++ b/src/shell_exec.rs @@ -2333,13 +2333,18 @@ pub fn forward_signal_to_pid(pid: i32, sig: i32) { /// caller holds the group leader unreaped throughout (see its doc), so on /// that path the sweep always fires, harmlessly. /// - A member is genuinely alive at the deadline: the probe is accurate and -/// the SIGKILL is the intended escalation. +/// the SIGKILL is the intended escalation. For a SIGSTOP'd member it is the +/// only signal that works — a stopped process runs no TERM handler, so the +/// sweep is what keeps teardown bounded. /// /// Callers whose children are reaped concurrently — `Cmd::stream`'s main /// thread waiting while the signal-forwarder thread runs this, tether's /// supervisor — get the accurate reading: the poll loop returns at the first /// probe after the reap, so escalating over a cooperative child costs one -/// poll interval, not the full grace. +/// poll interval, not the full grace. A reap does unpin the pgid, leaving the +/// microseconds between a probe that read alive and the following `killpg` as +/// the accepted recycling exposure — unchanged from the fixed-sleep +/// predecessor, and shared by every killpg-after-grace design. #[cfg(unix)] pub fn forward_signal_with_escalation(pgid: i32, sig: i32) { use nix::sys::signal::Signal; From d8201994cebea0401b3dc76663f90be3d6588c3e Mon Sep 17 00:00:00 2001 From: Maximilian Roos Date: Sun, 23 Aug 2026 13:52:23 -0700 Subject: [PATCH 5/5] test: demote the empty-group timing bound to a safety net A 30s grace makes returning early structurally distinguishable from sleeping it, so the wall-clock bound no longer races CI scheduling. Co-Authored-By: Claude Fable 5 --- src/shell_exec.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/shell_exec.rs b/src/shell_exec.rs index 8a0ebb5814..4a328bc16e 100644 --- a/src/shell_exec.rs +++ b/src/shell_exec.rs @@ -3168,9 +3168,12 @@ mod tests { child.wait().unwrap(); let start = Instant::now(); - assert!(super::group_died_within(pgid, Duration::from_millis(200))); + // Grace far longer than any plausible scheduling stall, so returning + // early is structurally distinguishable from having slept it, and the + // bound below is a safety net rather than a race. + assert!(super::group_died_within(pgid, Duration::from_secs(30))); assert!( - start.elapsed() < Duration::from_millis(100), + start.elapsed() < Duration::from_secs(5), "an empty group must exit the grace loop on the first probe; took {:?}", start.elapsed() );