diff --git a/src/supervisor.rs b/src/supervisor.rs index 303575f..b78e507 100644 --- a/src/supervisor.rs +++ b/src/supervisor.rs @@ -129,8 +129,11 @@ pub struct Supervisor { /// leader's zombie is collected. Invisible to `tick` snapshots, so the row /// disappears instantly while the sweep runs behind it. /// - /// Entries remain through `kill_grace` because group emptiness cannot be - /// reliably observed before escalation. `shutdown_all` waits for them. + /// Entries remain through `kill_grace` because observing group emptiness + /// would cost the escalation: the probe (`Task::group_gone`) must reap + /// the leader to see past its zombie, and a reaped group can no longer + /// be KILLed. Entries therefore keep their zombie until `kill_sent`, and + /// `shutdown_all` counts the graveyard instead of probing it. graveyard: Vec, next_id: u64, /// PTY content size (rows already minus the client's status bar). Every task @@ -346,20 +349,24 @@ impl Supervisor { self.graveyard.retain_mut(|t| !t.try_collect()); } - /// Kill every task for the quit path: TERM all groups at once, wait out one - /// shared grace (exiting early after all leaders and graveyard entries are - /// collected), then SIGKILL the stragglers. Blocking is bounded by the - /// grace. Anything the final KILLs don't collect (a leader in - /// uninterruptible sleep) reparents to init when the daemon exits moments - /// later; blocking on it here could wedge shutdown forever. + /// Kill every task for the quit path: TERM all groups at once, wait out + /// one shared grace, then SIGKILL the stragglers. The wait exits early + /// once `swept` proves there is nothing left to wait for; a task's + /// `finished` alone cannot gate it, because leader exit says nothing + /// about the rest of the group (`cmd & exit 0` leaves members behind), + /// and a leader-only predicate KILLed those members the instant the last + /// leader happened to be done, skipping the TERM grace entirely. + /// Blocking is bounded by the grace. Anything the final KILLs don't + /// collect (a leader in uninterruptible sleep) reparents to init when + /// the daemon exits moments later, as do TERM-refusing members of a + /// group whose leader the emptiness probe reaped (see + /// `Task::group_gone`); blocking on either could wedge shutdown forever. fn shutdown_all(&mut self) { for t in &mut self.tasks { t.terminate(); } let deadline = Instant::now() + self.kill_grace; - while (self.tasks.iter().any(|t| t.finished.is_none()) || !self.graveyard.is_empty()) - && Instant::now() < deadline - { + while !self.swept() && Instant::now() < deadline { std::thread::sleep(Duration::from_millis(25)); self.reap(); } @@ -367,6 +374,15 @@ impl Supervisor { self.graveyard.clear(); } + /// Shutdown's exit test: every live task's process group probes gone and + /// the graveyard has drained. Graveyard entries are counted, not probed: + /// probing reaps the leader, and a reaped group forfeits the KILL its + /// pending escalation still owes (`Task::try_collect`'s `kill_sent` gate + /// exists for the same reason); they leave through `reap` as always. + fn swept(&mut self) -> bool { + self.graveyard.is_empty() && self.tasks.iter_mut().all(Task::group_gone) + } + /// One step of the core's own loop: reap exits, then emit a fresh task /// snapshot (plus the watched task's screen). In process the client calls /// this each UI tick; in the daemon it runs on the core's thread and the @@ -1930,6 +1946,81 @@ mod tests { let _ = std::fs::remove_dir_all(&dir); } + /// The defect the group probe fixes: every leader exits at birth after + /// backgrounding a TERM-refusing child, so the old leader-only predicate + /// saw nothing to wait for and Drop KILLed the child instantly. Shutdown + /// must instead hold the full grace while the group probes non-empty; + /// the child, unreachable by KILL once the probe reaped its leader, + /// survives to reparent. + #[test] + fn shutdown_holds_the_grace_for_members_of_an_exited_leader() { + use nix::sys::signal::{Signal, kill}; + let dir = scratch("shutdown_leaderless"); + let (spid, ready) = (dir.join("spid"), dir.join("ready")); + let mut s = sup(24, 80); + s.set_kill_grace(Duration::from_millis(400)); + hello_with_sh(&mut s, dir.clone()); + let id = spawn_ready( + &mut s, + format!( + "trap '' HUP; (trap '' TERM; exec sleep 300) & echo $! > {sp}; echo r > {r}", + sp = spid.display(), + r = ready.display() + ), + dir.clone(), + &ready, + ); + let straggler = read_pid(&spid); + assert!(reap_until(&mut s, Duration::from_secs(5), |s| { + s.tasks.iter().all(|t| t.id != id || t.finished.is_some()) + })); + assert!(kill(straggler, None).is_ok(), "straggler should be alive"); + + let t0 = Instant::now(); + s.apply(Command::Shutdown); + let elapsed = t0.elapsed(); + let survived = kill(straggler, None).is_ok(); + // Clean up the reparented survivor before asserting. + let _ = kill(straggler, Signal::SIGKILL); + assert!( + elapsed >= Duration::from_millis(400), + "shutdown returned in {elapsed:?} with a non-empty group: the grace was skipped" + ); + assert!( + elapsed < Duration::from_secs(2), + "shutdown took {elapsed:?}: not bounded by the 400 ms grace" + ); + assert!( + survived, + "the straggler was KILLed instead of receiving the TERM grace" + ); + let _ = std::fs::remove_dir_all(&dir); + } + + /// Prompt exit, pinned: leaders exited long ago and left empty groups, + /// so shutdown returns in a few probe passes, nowhere near the grace. + #[test] + fn shutdown_is_prompt_when_every_group_is_already_empty() { + let mut s = sup(24, 80); + for _ in 0..2 { + s.apply(Command::Spawn { + command: "true".into(), + cwd: here(), + group: None, + }); + } + assert!(reap_until(&mut s, Duration::from_secs(5), |s| { + s.tasks.len() == 2 && s.tasks.iter().all(|t| t.finished.is_some()) + })); + let t0 = Instant::now(); + s.apply(Command::Shutdown); + assert!( + t0.elapsed() < Duration::from_millis(500), + "shutdown of already-empty groups took {:?}: the early exit is gone", + t0.elapsed() + ); + } + /// Session paths follow the connection's launch context: a hello env /// carrying `FLEETCOM_CONFIG_DIR` decides where save, list, and load look. /// The context env holds *only* the override, so anything this process's diff --git a/src/task.rs b/src/task.rs index c56313b..04ca0af 100644 --- a/src/task.rs +++ b/src/task.rs @@ -223,7 +223,12 @@ pub struct Task { /// Whether the group has received the one SIGKILL escalation. kill_sent: bool, /// Whether the leader has been reaped; its process group must not be - /// signalled afterward because the ID may have been reused. + /// signalled afterward because the ID may have been reused (`terminate` + /// and `force_kill` gate on this). The signal-0 existence probe + /// (`group_gone`) is the one carve-out: it delivers nothing, so a + /// recycled ID cannot be harmed, and its errors are one-sided; ESRCH is + /// conclusive while a stale "exists" only extends a wait that stays + /// bounded by the shutdown grace. reaped: bool, } @@ -433,7 +438,8 @@ impl Task { /// reaping it. `WNOWAIT` leaves the zombie in place, which is what keeps /// the pid (and therefore the pgid) reserved so the group stays signalable /// for the task's whole life; see the `reaped` field. The zombie is - /// collected exactly once, at teardown (`collect`). + /// collected exactly once: at teardown (`collect`), or by the shutdown + /// emptiness probe (`group_gone`). pub fn poll_exit(&mut self) -> io::Result<()> { if self.finished.is_some() || self.reaped { return Ok(()); @@ -716,6 +722,55 @@ impl Task { // clone drops when it exits, closing the queue. self.input_tx.take(); } + + /// Whether this task's process group is observably gone: leader reaped + /// and a signal-0 group probe answering ESRCH. The shutdown wait's exit + /// test; nothing else may call it, because it spends the zombie. + /// + /// The order inside one call is load-bearing. An unreaped zombie leader + /// keeps the group answering kill-style probes regardless of member + /// count (Linux reports it Ok, macOS EPERM, never ESRCH), so emptiness + /// is unobservable until the leader is reaped: reap first, probe second, + /// in the same pass, before the freed pid could plausibly recycle. Later + /// calls re-probe a long-reaped ID, which is safe only because the + /// probe's errors are one-sided: surviving members keep the pgid + /// reserved (a pid still serving as a live group's ID is not reissued), + /// so "exists" stays truthful while anyone remains; a recycled ID + /// misreads only as "exists", a bounded wait, never a stray signal; and + /// ESRCH cannot be wrong, since an ID with no group behind it cannot be + /// this group with members. Real signals get no such carve-out (see + /// `reaped`). + /// + /// The reap spends the pgid reservation `force_kill` relies on: a group + /// that still has members afterward can no longer be KILL-escalated, so + /// TERM-refusing members outlive shutdown and reparent to init. That is + /// the price of observing emptiness at all; the graveyard declines to + /// pay it and keeps its zombies until `kill_sent` (see + /// `Supervisor::reap`). + pub(crate) fn group_gone(&mut self) -> bool { + let Some(pid) = self.pid else { + // No pid was ever known: nothing waitable or signalable exists. + return true; + }; + if self.finished.is_none() { + // A live leader is a live group; the zombie-spending reap below + // must never run before the leader has exited. + return false; + } + if !self.reaped { + self.collect(); + if !self.reaped { + // Transient waitid failure: hold shutdown and retry next pass. + return false; + } + } + // Only ESRCH reads as gone. Ok is a live signalable member; EPERM is + // a member that exists but is beyond our signals. Both hold the wait. + matches!( + killpg(Pid::from_raw(pid as i32), None::), + Err(nix::errno::Errno::ESRCH) + ) + } } impl Drop for Task { @@ -906,6 +961,80 @@ mod tests { assert_eq!(t.exit_code, Some(137)); } + /// The shutdown probe reaps the exited leader, then probes the group in + /// the same pass: a zombie-only group turns gone in that one call. The + /// pre-reap assertions pin why the reap must come first: the zombie + /// alone keeps the group id resolvable for kill-style probes. + #[test] + fn group_gone_reaps_then_probes_past_the_zombie() { + use nix::errno::Errno; + let mut t = spawn(30, "exit 0"); + wait_finished(&mut t); + let pgid = Pid::from_raw(t.pid.expect("spawn always yields a pid") as i32); + // Zombie in place: the probe answer is Ok on Linux, EPERM on macOS, + // never ESRCH, so emptiness is invisible before the reap. + assert_ne!( + killpg(pgid, None::), + Err(Errno::ESRCH), + "an unreaped zombie must keep the group id resolvable" + ); + assert!( + t.group_gone(), + "a zombie-only group must probe gone in one reap+probe pass" + ); + // The probe spent the zombie: the group id no longer resolves. + assert_eq!(killpg(pgid, None::), Err(Errno::ESRCH)); + } + + /// A member that survives the leader holds the probe after the reap, + /// and the probe turns gone once that member dies. + #[test] + fn group_gone_holds_while_a_member_survives() { + use nix::sys::signal::kill; + let dir = std::env::temp_dir().join(format!("fleetcom_task_gone_{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + let spid = dir.join("spid"); + // `trap '' HUP` first: the `&` child must survive its session + // leader's exit to be a straggler (see the terminate test above). + let cmd = format!("trap '' HUP; sleep 300 & echo $! > {}", spid.display()); + let mut t = Task::spawn(31, &cmd, &cmd, &here(), 24, 80, &sh_env(), no_waker()).unwrap(); + wait_finished(&mut t); + let deadline = Instant::now() + Duration::from_secs(5); + let mut straggler = None; + while straggler.is_none() && Instant::now() < deadline { + straggler = std::fs::read_to_string(&spid) + .ok() + .and_then(|s| s.trim().parse::().ok()); + thread::sleep(Duration::from_millis(10)); + } + let straggler = Pid::from_raw(straggler.expect("straggler pid never written")); + + assert!(!t.group_gone(), "a surviving member must hold the probe"); + assert!(t.reaped, "the probe reaps the exited leader to see past it"); + + let _ = kill(straggler, Signal::SIGKILL); + let deadline = Instant::now() + Duration::from_secs(5); + while !t.group_gone() && Instant::now() < deadline { + thread::sleep(Duration::from_millis(10)); + } + assert!( + t.group_gone(), + "the group must probe gone once its last member dies" + ); + let _ = std::fs::remove_dir_all(&dir); + } + + /// `finished` gates the zombie-spending reap: a leader that has not + /// exited is never reaped (or waited on) by the probe. + #[test] + fn group_gone_never_reaps_a_live_leader() { + let mut t = spawn(32, "sleep 300"); + assert!(!t.group_gone(), "a live leader is a live group"); + assert!(!t.reaped, "the probe must not reap a running leader"); + t.terminate(); + } + /// Paste encoding follows the child's DECSET 2004 opt-in: markers only /// when asked for, newline→CR conversion only when not. #[test]