From c17f2d2c3d59c8a561bbb3defe4bacdc86d42ee3 Mon Sep 17 00:00:00 2001 From: npub17jjz49l9jjmhhk7cac63j8yt9z555n9cw8vk7v5jz4vzw4ppld5qgj57cc Date: Sat, 1 Aug 2026 21:18:09 -0400 Subject: [PATCH] terminal: process-group lifecycle with drain-ordered teardown Closing a tab must end the work the tab was doing. The child is a session leader (portable-pty calls setsid), so signalling its pid reaches the shell and leaves everything the shell started running, reparented to init and holding the pty slave. Signal the process group instead: SIGTERM, a bounded grace, then SIGKILL to the group whether or not the leader went quietly -- a polite leader does not imply an empty group. Three things this found that reading could not. waitpid(WNOHANG | WNOWAIT) returns EINVAL on Darwin. POSIX defines WNOWAIT only for waitid; Linux tolerates it, macOS does not. The failure was silent because `seen == pid` is false for -1 exactly as it is false for "still running" -- error and negative result collapsed into one branch. Every shutdown escalated to SIGKILL and the polite arm was dead code on the platform we develop on. Only an assertion on *which arm fired* could see it. A grandchild test that does not defeat the tty hangup proves nothing. Killing a session leader makes the kernel SIGHUP the foreground group, so the grandchild dies either way and a pid-only mutation survives. The fixture must ignore SIGHUP and busy-loop; then pid-only leaks and kill(-pgid) does not. A child blocked writing to an undrained master does not die promptly even on SIGKILL -- measured 606ms to reap, versus microseconds when drained. So the reader outlives termination and reap. shutdown_draining takes the reader by value, which makes the forbidden order a compile error rather than a comment: a joined reader has been consumed and cannot be passed. Mutation matrix, sources restored byte-identically after each: L1 delete SIGKILL escalation -> FAIL (orphan + grace arms) L2 kill(pid) not kill(-pgid) -> FAIL (orphan arm) L3 delete pre-reap try_wait -> FAIL (Killed, want AlreadyExited) L4 join reader before SIGTERM -> FAIL (10.01s, grace is 250ms) Fixtures built to ignore signals carry their own SIGKILL deadline. Without it a legitimate failure leaves a core spinning at PPID 1, and L1/L4 were detected by hanging -- a test that finds a bug by never finishing cannot be told from a broken one. The deadline sits far outside the observation window so it can never rescue a failing implementation. 2088 desktop + 3 mixer + 26 buzz-terminal + 5 fences + 4 resize green, clippy --workspace --all-targets -D warnings clean, fmt clean. Co-authored-by: tlongwell-block <109685178+tlongwell-block@users.noreply.github.com> Signed-off-by: tlongwell-block <109685178+tlongwell-block@users.noreply.github.com> --- .../src-tauri/crates/buzz-terminal/src/lib.rs | 3 + .../crates/buzz-terminal/src/lifecycle.rs | 259 +++++++++ .../buzz-terminal/src/lifecycle_tests.rs | 528 ++++++++++++++++++ 3 files changed, 790 insertions(+) create mode 100644 desktop/src-tauri/crates/buzz-terminal/src/lifecycle.rs create mode 100644 desktop/src-tauri/crates/buzz-terminal/src/lifecycle_tests.rs diff --git a/desktop/src-tauri/crates/buzz-terminal/src/lib.rs b/desktop/src-tauri/crates/buzz-terminal/src/lib.rs index b45324342..855e7a5c6 100644 --- a/desktop/src-tauri/crates/buzz-terminal/src/lib.rs +++ b/desktop/src-tauri/crates/buzz-terminal/src/lib.rs @@ -9,6 +9,7 @@ pub mod context; pub mod damage; pub mod env_fence; pub mod fences; +pub mod lifecycle; pub mod listener; pub mod path; pub mod reader; @@ -19,6 +20,8 @@ pub mod shell; mod context_tests; #[cfg(test)] mod env_fence_tests; +#[cfg(test)] +mod lifecycle_tests; use alacritty_terminal::grid::Dimensions; use alacritty_terminal::term::{Config, Osc52, Term}; diff --git a/desktop/src-tauri/crates/buzz-terminal/src/lifecycle.rs b/desktop/src-tauri/crates/buzz-terminal/src/lifecycle.rs new file mode 100644 index 000000000..b5fdc1268 --- /dev/null +++ b/desktop/src-tauri/crates/buzz-terminal/src/lifecycle.rs @@ -0,0 +1,259 @@ +//! Child-process lifecycle for spawned PTY sessions. +//! +//! Closing a terminal tab must actually end the work the tab was doing. That +//! is harder than calling `kill`, for two reasons that both come from the +//! child being a *session leader* rather than an ordinary subprocess. +//! +//! **1. The child is not the only process.** `portable-pty` calls `setsid()` +//! in `pre_exec` (`unix.rs:257`), so the shell becomes a session and process +//! group leader; everything it runs — `vim`, a `make -j8` tree, a backgrounded +//! `sleep` — joins that group or a descendant of it. Signalling the shell's +//! pid alone reaches the shell. A shell that exits without forwarding the +//! signal leaves its children running, reparented to init, holding the pty +//! slave open. That is a leak that survives the window closing. +//! +//! So we signal the **process group** (`kill(-pgid)`), not the pid. +//! +//! **2. `portable-pty`'s own `kill` is not sufficient here.** `ChildKiller for +//! std::process::Child` (`lib.rs:340-373`) sends `SIGHUP` to the *pid*, waits +//! up to 4x50 ms, then falls back to `Child::kill` — which is `SIGKILL`, again +//! to the pid. Both halves are pid-scoped, so neither reaches a grandchild. +//! It is a correct API for "end this process"; ours is "end this session". +//! +//! ## The escalation +//! +//! `SIGTERM` to the group, a bounded wait for the leader, then `SIGKILL` to +//! the group **whether or not the leader went quietly** -- see `shutdown` for +//! why a polite leader does not imply an empty group. +//! `SIGTERM` first because a shell asked to terminate cleanly will flush its +//! history and let `vim` write its swap file; going straight to `SIGKILL` +//! guarantees no process ever gets that chance. The bounded wait is what makes +//! the escalation real — without it, `SIGKILL` either races the polite path +//! (making `SIGTERM` decorative) or never fires (making a signal-ignoring +//! child immortal). +//! +//! ## What this deliberately does not do +//! +//! A process that has called `setsid()` for *itself* has left our group, and +//! no group signal reaches it. `nohup`, a daemonising build tool, and +//! `tmux`-style servers all do this on purpose. We do not hunt the process +//! tree to find them: walking children to signal them is a race against a +//! moving tree — a pid read and then signalled may be a *different* process by +//! the time the signal lands, and killing a stranger's pid is a far worse bug +//! than leaking a daemon the user deliberately detached. Detaching from the +//! session is the documented way to survive one's terminal, and honouring it +//! is correct behaviour, not a gap. + +use std::io; +use std::time::{Duration, Instant}; + +use portable_pty::Child; + +/// How long the group gets to honour `SIGTERM` before `SIGKILL`. +/// +/// Long enough for a shell to run its exit trap and for an editor to write a +/// swap file; short enough that closing a tab never feels stuck. Tab close is +/// not synchronous with this wait in the UI, so this is a cleanup deadline, +/// not a frame budget. +pub const TERM_GRACE: Duration = Duration::from_millis(250); + +/// Poll interval while waiting for the child to exit. +/// +/// Polling rather than blocking in `wait()`: a blocking wait cannot be given a +/// deadline without a second thread, and the whole point of the grace period +/// is that it expires. +const POLL_INTERVAL: Duration = Duration::from_millis(5); + +/// How a session ended. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Shutdown { + /// Already gone before we signalled. + AlreadyExited, + /// Exited within [`TERM_GRACE`] of `SIGTERM`. + Terminated, + /// Ignored or outlived `SIGTERM`; the group was killed. + Killed, +} + +/// Ends the session led by `child`: `SIGTERM` to its process group, a bounded +/// wait, then `SIGKILL` to the group if anything is still there. +/// +/// Reaps the child before returning, so the caller cannot leave a zombie by +/// dropping the handle. Returns which arm ended it, which is what a test can +/// assert on — "did it die" is satisfied by both arms and so distinguishes +/// nothing. +#[cfg(unix)] +pub fn shutdown(child: &mut Box) -> io::Result { + // Reap first. A child that already exited still has a pid slot until it is + // waited for, and that pid is reusable the moment it is released -- so + // signalling without checking is how a cleanup path eventually signals an + // unrelated process. Check before signalling, every time. + if child.try_wait()?.is_some() { + return Ok(Shutdown::AlreadyExited); + } + + let Some(pid) = child.process_id() else { + // No pid means nothing to signal; still ensure it is reaped. + child.wait()?; + return Ok(Shutdown::AlreadyExited); + }; + let pid = pid as i32; + + signal_group(pid, libc::SIGTERM); + let leader_honoured_term = leader_exited_by(pid, Instant::now() + TERM_GRACE); + + // Sweep the group unconditionally, *including* when the leader exited + // politely. The leader's exit is not the session's end: anything it + // backgrounded that ignores SIGTERM is still running, still in the group, + // and still holding the pty. Returning `Terminated` at that point reports + // success over a leak. + // + // Ordering with the reap is a safety requirement, not a preference. A + // process group id *is* the leader's pid, and the kernel may recycle that + // pid once the leader is reaped -- at which point `kill(-pid)` names some + // unrelated group. An exited-but-unreaped leader is a zombie, and a zombie + // is still a group member, so the id cannot be reused while we hold it. + // Signal first, reap second, and the window does not exist. + signal_group(pid, libc::SIGKILL); + + // SIGKILL cannot be caught, so this terminates. It is still a `wait` + // rather than an assumption: the pid must be reaped, and the exit status + // is only available to whoever reaps it. + child.wait()?; + + Ok(if leader_honoured_term { + Shutdown::Terminated + } else { + Shutdown::Killed + }) +} + +/// Sends `signal` to `pid`'s process group, falling back to the pid alone. +/// +/// The fallback matters: `kill(-pgid)` requires the child to *be* a group +/// leader, which it is only because `portable-pty` called `setsid()`. If that +/// ever stops being true, a pid-scoped signal still ends the shell — degraded +/// (grandchildren survive) rather than a silent no-op. +/// +/// Errors are deliberately not propagated. Every failure mode here means the +/// process is already gone (`ESRCH`) or was never ours to signal (`EPERM`), +/// and in both cases the following `wait` is the authority on what happened. +#[cfg(unix)] +fn signal_group(pid: i32, signal: i32) { + // SAFETY: `kill` with a negative pid targets the process group; both + // arguments are plain integers and the call has no memory effects. + let sent = unsafe { libc::kill(-pid, signal) }; + if sent != 0 { + // SAFETY: as above. + unsafe { libc::kill(pid, signal) }; + } +} + +/// Polls until the leader has exited, or `deadline` passes. Returns whether it +/// exited in time. +/// +/// Deliberately **not** `Child::try_wait`, which reaps: reaping here would +/// release the process group id before the sweep above can use it. `WNOWAIT` +/// reads the child's exit state and leaves it waitable, so the zombie stays +/// and keeps the group id reserved for us. +/// +/// `waitid`, not `waitpid`, and that is a portability requirement rather than +/// taste. POSIX only defines `WNOWAIT` for `waitid`; Linux tolerates it on +/// `waitpid`, and **Darwin returns `EINVAL`**. Measured with a C probe: on +/// macOS 25.5.0, `waitpid(pid, &st, WNOHANG | WNOWAIT)` is `-1/EINVAL` for a +/// child that has plainly exited. That failure is silent in the shape this +/// function had — an error is indistinguishable from "not exited yet", so the +/// grace period could never be honoured and *every* shutdown escalated to +/// `SIGKILL`, reporting `Killed` for a child that died politely on the first +/// `SIGTERM`. The polite arm was dead code on the platform we develop on. +/// +/// `waitid` reports a still-running child as success-with-`si_pid == 0`, so +/// the out-parameter must be zeroed before each call and the *pid*, not the +/// return code, is the answer. +#[cfg(unix)] +fn leader_exited_by(pid: i32, deadline: Instant) -> bool { + loop { + // SAFETY: `info` is a valid, fully-initialised out-pointer for the + // duration of the call. `WNOWAIT` leaves the child waitable, so the + // later `wait` still returns its status. + let exited = unsafe { + let mut info: libc::siginfo_t = std::mem::zeroed(); + let rc = libc::waitid( + libc::P_PID, + pid as libc::id_t, + &mut info, + libc::WEXITED | libc::WNOHANG | libc::WNOWAIT, + ); + rc == 0 && info.si_pid() == pid + }; + if exited { + return true; + } + if Instant::now() >= deadline { + return false; + } + std::thread::sleep(POLL_INTERVAL); + } +} + +/// Maximum concurrent terminal sessions. +/// +/// Each session costs a pty pair (two fds), a reader thread, and a scrollback +/// grid -- at the default 10k lines x 80 cols that is megabytes of resident +/// memory per tab. The cap exists because tab creation is one keystroke and +/// nothing else bounds it: without a limit, a held-down shortcut exhausts the +/// process fd table, and the first thing to fail is not the terminal but +/// whatever *else* in Buzz next asks for a file descriptor -- the relay +/// socket, a database handle. A resource a UI can allocate in a loop needs a +/// ceiling that fails in its own subsystem. +/// +/// 20 matches the abandoned `feat/terminal` branch's `MAX_LIVE_SESSIONS`, +/// kept deliberately: it is far above any plausible human tab count and far +/// below the default 256-fd soft limit, so it bounds the runaway case without +/// ever being reachable by hand. +pub const MAX_LIVE_SESSIONS: usize = 20; + +/// A reader that is still consuming the PTY master, to be stopped only after +/// the session has been torn down. +/// +/// This exists because the correct close order is not the obvious one, and +/// nothing in the type system otherwise prevents the wrong one. Mari's ruling +/// (`62509b91`) is that **the reader outlives child termination and reap**: +/// +/// 1. mark the session closing and stop publishing to the UI; +/// 2. `SIGTERM` -> grace -> `SIGKILL` -> reap, *while output is still drained*; +/// 3. only then close the master and join the reader. +/// +/// Inverting steps 2 and 3 is the bug this trait is shaped to prevent, and it +/// is not a hypothetical: a child blocked writing into a master nobody reads +/// does not die promptly even on `SIGKILL`, because the kernel completes the +/// tty teardown first. Measured with a `forkpty` probe -- **606 ms** to reap a +/// `SIGKILL`ed child against an undrained master, versus microseconds when +/// drained. Join the reader first and every tab close pays that, on the arm +/// where the user is already waiting. +pub trait DrainingReader { + /// Stops consuming and releases the reader. Called *after* reap. + fn join(self: Box); +} + +/// Ends a session in the order the drain law requires, and returns how it +/// ended. +/// +/// The ordering is enforced by ownership rather than by documentation: this +/// function takes the reader **by value**, so a caller cannot have joined it +/// beforehand -- a joined reader has been consumed and cannot be passed here. +/// The only way to use this API is the correct order. A comment saying "do not +/// join the reader first" is advice; a moved value is a compile error. +#[cfg(unix)] +pub fn shutdown_draining( + child: &mut Box, + reader: Box, +) -> io::Result { + // Terminate and reap with the reader still running, so the child never + // blocks in a tty write while we are waiting on it. + let outcome = shutdown(child); + // Unconditional: the reader must be released whether or not the shutdown + // reported an error, or a failed close leaks the thread and its fd. + reader.join(); + outcome +} diff --git a/desktop/src-tauri/crates/buzz-terminal/src/lifecycle_tests.rs b/desktop/src-tauri/crates/buzz-terminal/src/lifecycle_tests.rs new file mode 100644 index 000000000..17068f21a --- /dev/null +++ b/desktop/src-tauri/crates/buzz-terminal/src/lifecycle_tests.rs @@ -0,0 +1,528 @@ +//! Lifecycle gates: the session dies, the grandchild dies with it, and the +//! login `argv[0]` the child actually receives is the one we computed. +//! +//! Every test here drives a real PTY and a real process tree. A mock child +//! would let us assert that we *called* `kill`, which is the half we already +//! know; the property under test is what the kernel does with a process group +//! we do not fully control. + +use crate::env_fence::fence_env; +use crate::lifecycle::{shutdown, shutdown_draining, DrainingReader, Shutdown, TERM_GRACE}; +use crate::path::user_shell_path; +use crate::shell::{login_argv0, resolve_shell}; +use portable_pty::{native_pty_system, Child, CommandBuilder, PtyPair, PtySize}; +use std::sync::atomic::Ordering; +use std::time::{Duration, Instant}; + +/// Upper bound on any wait in this file. +/// +/// Every wait here is bounded, and that is not caution -- it is the lesson +/// from a probe of an interactive child that read to EOF and hung for 300 s. +/// A PTY master does not reach EOF while any process holds the slave open, so +/// "read until the child is done" is not a terminating program. Bound the +/// read, or poll for the observable effect. +const BOUND: Duration = Duration::from_secs(10); + +/// Self-destruct deadline, in seconds, for fixture processes built to ignore +/// signals. +/// +/// Comfortably longer than [`BOUND`], so it can never end a process while the +/// test is still observing it -- a watchdog that fires inside the observation +/// window would make a *failing* implementation look correct. Short enough +/// that a crashed run does not leave a core spinning until reboot. +const WATCHDOG: u64 = 60; + +fn open_pty() -> PtyPair { + native_pty_system() + .openpty(PtySize { + rows: 24, + cols: 80, + pixel_width: 0, + pixel_height: 0, + }) + .expect("openpty") +} + +/// Drains the PTY master in the background for as long as it stays open. +/// +/// Not hygiene -- a correctness requirement, and the cause of a 300 s hang in +/// the first version of this file. A PTY has a small kernel buffer, and a +/// child writing into a master nobody reads blocks in `write()` once it fills. +/// A process blocked in an uninterruptible tty write does not die promptly on +/// `SIGKILL`: the signal is delivered, but the kernel finishes tearing down +/// the tty session first, so `wait()` sits there while the reap completes. +/// Measured directly with a `forkpty` C probe: with the master undrained, a +/// `SIGKILL`ed child took **606 ms** to be reaped. Every terminal in the +/// product drains its master continuously -- that is what a renderer *is* -- +/// so a test that doesn't is modelling a configuration that never ships. +/// +/// The consequence is worth stating for the embedder: **shutdown must not be +/// called after the reader has stopped.** Tear the session down while output +/// is still being consumed, or the grace period is spent waiting on a +/// self-inflicted stall. +fn drain(pair: &PtyPair) { + let mut reader = pair.master.try_clone_reader().expect("reader"); + std::thread::spawn(move || { + use std::io::Read; + let mut buf = [0u8; 4096]; + while matches!(reader.read(&mut buf), Ok(n) if n > 0) {} + }); +} + +/// Spawns `script` under `/bin/sh` on a real PTY, fully fenced. +fn spawn_script(pair: &PtyPair, script: &str) -> Box { + let shell = resolve_shell(std::env::var("SHELL").ok().as_deref()); + let mut cmd = CommandBuilder::new("/bin/sh"); + fence_env(&mut cmd, &user_shell_path(), &shell); + cmd.arg("-c"); + cmd.arg(script); + let child = pair.slave.spawn_command(cmd).expect("spawn"); + drain(pair); + child +} + +/// True while `pid` exists. `kill(pid, 0)` performs the permission and +/// existence checks without delivering a signal. +fn pid_alive(pid: i32) -> bool { + // SAFETY: signal 0 delivers nothing; both arguments are integers. + unsafe { libc::kill(pid, 0) == 0 } +} + +/// Polls `f` until it returns true or `BOUND` elapses; returns whether it did. +/// +/// Polling for the observable state rather than sleeping a fixed duration: a +/// sleep long enough to be reliable is slow, and a sleep short enough to be +/// fast is a race that fails on a loaded machine. Both are worse than asking. +fn poll_until(mut f: impl FnMut() -> bool) -> bool { + let deadline = Instant::now() + BOUND; + while Instant::now() < deadline { + if f() { + return true; + } + std::thread::sleep(Duration::from_millis(5)); + } + false +} + +/// Reads a file until it is non-empty or `BOUND` elapses. +fn read_when_written(path: &std::path::Path) -> Option { + let mut found = None; + poll_until(|| match std::fs::read_to_string(path) { + Ok(text) if !text.trim().is_empty() => { + found = Some(text.trim().to_owned()); + true + } + _ => false, + }); + found +} + +/// A cooperative child exits on `SIGTERM`, so the polite arm is what ends it. +/// +/// The distinction matters: `Killed` and `Terminated` both leave a dead +/// process, so asserting death alone would pass with `SIGTERM` deleted +/// entirely and the grace period reduced to a delay before `SIGKILL`. +#[test] +fn cooperative_child_exits_on_term_not_kill() { + let pair = open_pty(); + let mut child = spawn_script(&pair, "sleep 30"); + let pid = child.process_id().expect("pid") as i32; + + let outcome = shutdown(&mut child).expect("shutdown"); + assert_eq!( + outcome, + Shutdown::Terminated, + "a child that dies on SIGTERM must not have needed SIGKILL" + ); + assert!(poll_until(|| !pid_alive(pid)), "child survived shutdown"); +} + +/// A child that ignores `SIGTERM` must still die, and the escalation must be +/// what kills it. +/// +/// The fixture shape is load-bearing and my first one was vacuous. I wrote +/// `trap '' TERM; sleep 30`, which *looks* like a signal-ignoring child and +/// reported `Terminated` -- the polite arm, on a child built to defeat it. +/// The reason is that `sh` does not ignore a signal on its child's behalf: the +/// group `SIGTERM` reaches `sleep`, which has no trap and dies, and the shell +/// was blocked in `wait` on exactly that `sleep`, so it reaps it and exits +/// normally. The trap was real, the ignoring was real, and the process still +/// died on `SIGTERM` -- through a path the test wasn't looking at. +/// +/// Had I not checked *which* arm fired, this would have passed for the wrong +/// reason and gone on "proving" an escalation it never exercised. The loop +/// keeps the shell itself alive: no blocking `wait` to be interrupted, so the +/// trap actually governs the shell's own fate and only `SIGKILL` can end it. +/// +/// The readiness handshake closes a second, subtler version of the same +/// mistake. My loop fixture *still* reported `Terminated`, because `trap` is a +/// command the shell has to reach: a signal delivered in the interval between +/// `exec` and that line finds the default disposition and kills the shell +/// outright. Isolated with a `forkpty` probe -- identical binary, only the +/// delay before signalling changed: at 500 ms all four arms survived, at 2 ms +/// all four died with signal 15. A fixture that is only *probably* armed makes +/// this test a race whose failure mode is a false pass. +/// +/// This is the arm that fails if the escalation is deleted -- and the +/// `WATCHDOG` is what makes that a *failure* rather than a hang. With +/// `SIGKILL` deleted, nothing we send can end a child that ignores `SIGTERM`, +/// so `shutdown`'s final `wait` blocks forever and the mutant is detected only +/// by the harness timing out. A test that detects a bug by never finishing is +/// indistinguishable from a broken test. The deadline converts it into a +/// bounded, reportable failure. +#[test] +fn signal_ignoring_child_is_killed_after_the_grace_period() { + let dir = tempdir("buzz-terminal-trap"); + let ready = dir.join("armed"); + + let pair = open_pty(); + // The readiness file is written *after* the trap is installed, so waiting + // on it converts "probably armed by now" into an observed fact. + let mut child = spawn_script( + &pair, + &format!( + "trap '' TERM; (sleep {WATCHDOG}; kill -9 $$) & : > {}; \ + while :; do sleep 0.1; done", + ready.display() + ), + ); + let pid = child.process_id().expect("pid") as i32; + assert!( + poll_until(|| ready.exists()), + "child never armed its SIGTERM trap; signalling now would test a \ + startup race rather than the escalation" + ); + + let started = Instant::now(); + let outcome = shutdown(&mut child).expect("shutdown"); + let elapsed = started.elapsed(); + + assert_eq!( + outcome, + Shutdown::Killed, + "a SIGTERM-ignoring child must be escalated to SIGKILL" + ); + assert!(poll_until(|| !pid_alive(pid)), "child survived SIGKILL"); + assert!( + elapsed >= TERM_GRACE, + "shutdown returned in {elapsed:?}, before the {TERM_GRACE:?} grace \ + period could have elapsed -- SIGTERM was never given its chance" + ); + assert!( + elapsed < BOUND, + "shutdown took {elapsed:?}; the grace period is not bounded" + ); +} + +/// The property the whole module exists for: a **grandchild** must not outlive +/// the session. +/// +/// The fixture is deliberately hostile, and the obvious version of this test +/// proves nothing. I first wrote `sleep 30 & echo $!; wait` and mutation L2 -- +/// replacing `kill(-pid)` with `kill(pid)` -- **survived it**. The reason is +/// that killing a PTY session leader makes the kernel hang up the terminal and +/// `SIGHUP` the whole foreground group, so the grandchild dies either way. +/// Isolated with a `forkpty` probe: with the master held open (no fd-closure +/// hangup) and only `SIGKILL` to the shell's pid, the grandchild was gone +/// within 200 ms while the shell itself was still unreaped. The tty hangup was +/// doing the work my group signal was being credited for. +/// +/// Two properties are therefore required of the grandchild, and each closes +/// one leak in the fixture: +/// +/// - it **ignores `SIGHUP`**, so the tty hangup cannot end it for us; and +/// - it **busy-loops rather than sleeping**, so it is not blocked in a call +/// that the session teardown would interrupt anyway. +/// +/// With both, the probe separates cleanly: pid-only leaves the grandchild +/// alive, `kill(-pgid)` does not. That is the only shape in which this test +/// can fail for the reason it claims to test. +/// +/// The `WATCHDOG` is the price of that hostility. A grandchild built to +/// survive every signal we send also survives the harness: when this test +/// legitimately fails -- as it does under mutation L1 and L2 -- it leaves a +/// process spinning a core at PPID 1, and a panicking or killed test binary +/// cannot clean up after itself. So the child carries its own deadline. +/// `SIGKILL` because that is the one signal the fixture does not trap. +#[test] +fn grandchild_does_not_outlive_the_session() { + let dir = tempdir("buzz-terminal-orphan"); + let pidfile = dir.join("grandchild.pid"); + let armed = dir.join("armed"); + + let pair = open_pty(); + let mut child = spawn_script( + &pair, + &format!( + "sh -c 'trap \"\" HUP TERM; (sleep {WATCHDOG}; kill -9 $$) & \ + : > {armed}; while :; do :; done' & \ + echo $! > {pidfile}; wait", + armed = armed.display(), + pidfile = pidfile.display() + ), + ); + let shell_pid = child.process_id().expect("pid") as i32; + + let grandchild: i32 = read_when_written(&pidfile) + .expect("grandchild never reported its pid") + .parse() + .expect("pid is a number"); + assert!( + poll_until(|| armed.exists()), + "grandchild never armed its SIGHUP trap; the tty hangup would kill it \ + regardless of how we signal, and this test could not observe the \ + difference" + ); + assert!( + pid_alive(grandchild), + "test setup: the grandchild must be running before we shut down" + ); + assert_ne!( + grandchild, shell_pid, + "test setup: the grandchild must be a distinct process, or this \ + cannot tell a group signal from a pid signal" + ); + + shutdown(&mut child).expect("shutdown"); + + assert!( + poll_until(|| !pid_alive(shell_pid)), + "the session leader survived shutdown" + ); + assert!( + poll_until(|| !pid_alive(grandchild)), + "an orphaned grandchild ({grandchild}) outlived the session -- the \ + signal reached the shell's pid but not its process group" + ); +} + +/// Shutting down an already-dead child is safe and reaps it. +/// +/// Without the leading `try_wait`, this path signals a pid that the kernel may +/// already have released and reassigned. +#[test] +fn shutdown_of_an_exited_child_is_a_reap_not_a_signal() { + let pair = open_pty(); + let mut child = spawn_script(&pair, "exit 0"); + assert!( + poll_until(|| child.try_wait().ok().flatten().is_some()), + "child did not exit" + ); + assert_eq!( + shutdown(&mut child).expect("shutdown"), + Shutdown::AlreadyExited + ); +} + +/// The login `argv[0]` the child **actually receives**, not the string we +/// computed. +/// +/// This closes the gap flagged in `e8b567aa`: `login_argv0` and +/// `portable-pty`'s `as_command` (`cmdbuilder.rs:510-517`) were each verified +/// by reading, and agreement-by-reading is not observation. +/// +/// Two things make the probe terminate where a naive one hangs. The child +/// writes `$0` to a **file** rather than the PTY -- so there is no terminal +/// echo to strip, no ANSI to parse, and no dependency on the interactive +/// shell ever reaching EOF. And the read is polled to a deadline. Credit to +/// Quinn (`fcfd69b0`), whose three failed PTY-parsing harnesses established +/// that the harness was the bug. +/// +/// The explicit-prog row is the control that isolates login `argv[0]` as the +/// only variable: same shell, same PTY, same fence, no `-` prefix. +#[test] +fn default_prog_child_observes_the_login_argv0() { + let dir = tempdir("buzz-terminal-argv0"); + let shell = "/bin/sh"; + + let default_prog = observe_argv0(&dir.join("default"), shell, true); + assert_eq!( + default_prog, + login_argv0(shell), + "the child's $0 is not the login argv0 we computed" + ); + assert!( + default_prog.starts_with('-'), + "a default-prog child must be a login shell: {default_prog:?}" + ); + + let explicit = observe_argv0(&dir.join("explicit"), shell, false); + assert_eq!( + explicit, shell, + "control: an explicitly-invoked shell must not be given a login argv0" + ); + assert_ne!( + default_prog, explicit, + "control and subject agree, so this test cannot observe the login \ + prefix at all" + ); +} + +/// Spawns a `/bin/sh` that writes its own `$0` to `pidfile`, either as a +/// default program (login argv0 applied by portable-pty) or explicitly. +/// +/// The default-prog child is an *interactive* shell with no `-c`, so it is +/// driven by writing to the PTY master -- the only way to give a login shell +/// a command is to type one. +fn observe_argv0(outfile: &std::path::Path, shell: &str, default_prog: bool) -> String { + let pair = open_pty(); + let resolved = resolve_shell(Some(shell)); + let mut cmd = if default_prog { + CommandBuilder::new_default_prog() + } else { + CommandBuilder::new(shell) + }; + fence_env(&mut cmd, &user_shell_path(), &resolved); + if !default_prog { + cmd.arg("-c"); + cmd.arg(format!("printf '%s' \"$0\" > {}", outfile.display())); + } + + let mut child = pair.slave.spawn_command(cmd).expect("spawn"); + drain(&pair); + drop(pair.slave); + + if default_prog { + use std::io::Write; + let mut writer = pair.master.take_writer().expect("writer"); + writeln!(writer, "printf '%s' \"$0\" > {}", outfile.display()).expect("write"); + writer.flush().expect("flush"); + // Dropping the writer closes the master's write side, which the shell + // reads as end-of-input and exits on -- no `exit` command needed, and + // nothing depends on the shell's rc files having run. + drop(writer); + } + + let observed = read_when_written(outfile); + let _ = crate::lifecycle::shutdown(&mut child); + observed.unwrap_or_else(|| panic!("child never reported $0 within {BOUND:?}")) +} + +/// A fresh directory for a test's artifacts, replacing any prior run's. +fn tempdir(name: &str) -> std::path::PathBuf { + let dir = std::env::temp_dir().join(name); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).expect("create temp dir"); + dir +} + +/// Mari's noisy-child discriminator: the reader must still be draining +/// **while** the child is being terminated and reaped. +/// +/// The property is an ordering, and an ordering cannot be observed from the +/// outcome -- both orders end with a dead child and a stopped reader. So the +/// reader records *when* it read, relative to the moment close begins, and the +/// assertion is on bytes drained after that moment. +/// +/// The child is deliberately noisy: it floods the PTY continuously, so a +/// master that stops being read fills its kernel buffer within milliseconds +/// and the child blocks in `write()`. That is the state the drain law exists +/// to avoid, and a quiet child cannot produce it -- with nothing being +/// written, both orders look identical and the test proves nothing. +#[test] +fn reader_drains_through_termination_and_reap() { + let pair = open_pty(); + + // Flood, and keep flooding: `yes` writes until the pipe is closed or the + // process dies, so there is always more output pending than the buffer + // holds. + let mut child = spawn_noisy(&pair); + + let after_close = std::sync::Arc::new(std::sync::atomic::AtomicU64::new(0)); + let closing = std::sync::Arc::new(std::sync::atomic::AtomicBool::new(false)); + let reader = RecordingReader::spawn(&pair, after_close.clone(), closing.clone()); + + // Let the child get well ahead of the reader before we touch anything. + assert!( + poll_until(|| after_close.load(Ordering::Relaxed) == 0 && reader.total_bytes() > 4096), + "test setup: the child is not producing enough output to fill the pty \ + buffer, so this cannot distinguish drain order" + ); + + closing.store(true, Ordering::Relaxed); + let started = Instant::now(); + let outcome = shutdown_draining(&mut child, Box::new(reader)).expect("shutdown"); + let elapsed = started.elapsed(); + + assert_eq!( + outcome, + Shutdown::Terminated, + "a `yes` pipeline dies on SIGTERM; SIGKILL here means it was wedged in \ + a tty write against an undrained master" + ); + assert!( + after_close.load(Ordering::Relaxed) > 0, + "the reader drained nothing after close began -- it was joined before \ + termination, which is the ordering the drain law forbids" + ); + assert!( + elapsed < TERM_GRACE, + "shutdown took {elapsed:?}, at or beyond the {TERM_GRACE:?} grace \ + period: the child was blocked writing to an undrained master rather \ + than exiting on SIGTERM" + ); +} + +/// Spawns a child that floods the PTY without pause. +fn spawn_noisy(pair: &PtyPair) -> Box { + let shell = resolve_shell(std::env::var("SHELL").ok().as_deref()); + let mut cmd = CommandBuilder::new("/bin/sh"); + fence_env(&mut cmd, &user_shell_path(), &shell); + cmd.arg("-c"); + cmd.arg("while :; do echo aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa; done"); + // Deliberately no `drain` here: this test owns the reader. + pair.slave.spawn_command(cmd).expect("spawn") +} + +/// A [`DrainingReader`] that records how much it read after close began. +struct RecordingReader { + total: std::sync::Arc, + handle: std::thread::JoinHandle<()>, +} + +impl RecordingReader { + fn spawn( + pair: &PtyPair, + after_close: std::sync::Arc, + closing: std::sync::Arc, + ) -> Self { + let mut reader = pair.master.try_clone_reader().expect("reader"); + let total = std::sync::Arc::new(std::sync::atomic::AtomicU64::new(0)); + let counter = total.clone(); + let handle = std::thread::spawn(move || { + use std::io::Read; + let mut buf = [0u8; 4096]; + while let Ok(n) = reader.read(&mut buf) { + if n == 0 { + break; + } + counter.fetch_add(n as u64, Ordering::Relaxed); + if closing.load(Ordering::Relaxed) { + after_close.fetch_add(n as u64, Ordering::Relaxed); + } + } + }); + Self { total, handle } + } + + fn total_bytes(&self) -> u64 { + self.total.load(Ordering::Relaxed) + } +} + +impl DrainingReader for RecordingReader { + fn join(self: Box) { + // Bounded, and that is the whole point. The read loop ends when the + // master reports EOF, which only happens once the reaped child has + // released the slave -- so joining *before* termination blocks + // forever. That is precisely the forbidden ordering (mutation L4), + // and an unbounded join would "detect" it by hanging, which is + // indistinguishable from a broken test. Waiting to a deadline and + // abandoning the thread converts the hang into an assertion failure + // the harness can report. + if !poll_until(|| self.handle.is_finished()) { + return; + } + let _ = self.handle.join(); + } +}