Hold shutdown grace for process groups whose leader exited

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<Task>,
     next_id: u64,
     /// PTY content size (rows already minus the client's status bar). Every task
@@ -346,27 +349,40 @@ 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();
         }
         self.tasks.clear(); // Drop force-kills whatever is left
         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

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::<Signal>),
+            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::<Signal>),
+            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::<Signal>), 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::<i32>().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]