diff --git a/docs/active-work.md b/docs/active-work.md index 7df3466..270e9e9 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -60,6 +60,92 @@ git status --short --branch The `git log` command must expose `d152120` or a newer intentional main. If it does not, stop and repair the remote/fetch configuration. +## PTY terminate diagnostic lane — IN REVIEW (PR #176) + +- Portable branch: `githubsucks/pty-terminate-eperm`; worktree + `../pmacs-math-slice`. **PR #176**, base `main`, based on `ccf29e3` + with `c93f9ee` (#175) merged in. +- Approved framing: `docs/process-signal-tolerance-framing.md` + **revision 4**, after three review rounds. +- **Diagnostic only. No disposition change.** Every call that failed + before still fails, with no state transition and no reap-ledger + arming. `src/process.rs` is the only source file touched. +- **Why nothing is fixed:** revisions 1–3 each proposed a *tolerance* + rule and all three were rejected as unsound in the same way — each + concluded something about a process from something that was not about + that process. Rev 1 from an errno alone (EPERM means the caller lacks + permission, not that the id was recycled); rev 2 from `try_wait`, + which observes the spawned **leader** while a PTY signal targets + `-tcgetpgrp(...)`, entities that diverge exactly when job control has + moved the terminal; rev 3 from group-directed **ESRCH**, which proves + only that the selected foreground group vanished. +- **Two facts that killed the original argument.** `group = true` is + *rejected* for PTY mode at spawn (`src/process.rs:1428-1429`), so the + reap ledger never applies to the PTY path at all; and the ledger + comment (`:1075`) says EPERM "cannot happen for our own children" and + drops the entry for **bounded growth** — not a ruling that EPERM means + dead. +- **The CI evidence never established the child had exited.** The probe's + last source statement is a file write and CPython teardown does not + synchronise with it, so no tolerance rule could even be shown to fix + the symptom. That is the whole reason the lane is diagnostic. +- What ships: a failing `kill` now reports five separate facts — target + source, target kind/value, spawn-time group, errno, and the leader's + real `try_wait` state. The test seam injects the **kill result only**, + never the observation, so the real `ChildHandle::try_wait` runs against + the real child. +- **Not "strictly additive".** `try_wait` reaps and caches, so an exited + child may be reaped earlier than otherwise. Safe because + `portable-pty` 0.9.0 returns a `std::process::Child` on Unix and + delegates `try_wait` to it, so `poll_one` still sees the cached + status — pinned by an exactly-one-terminal-event test rather than + assumed. +- Round-1 review fixes: the exited-child tests no longer use a fixed + sleep as proof of exit (nix's `waitid` is unavailable on macOS and + `libc::waitid` needs `unsafe`, which the crate forbids), instead + driving the production diagnostic in a bounded loop until it observes + the exit; and every assertion is now exact message equality built from + the kernel-assigned pid, since the substring forms would have accepted + a hardcoded target or a wrong exit code. +- Bites, all verified rather than assumed: tolerating the failure fails + the disposition test; stubbing the leader observation fails three + tests including the one-event pin; a hardcoded target fails four; a + wrong exit code fails two. +- **The sweep found a real defect in these tests, not a flake.** + `observing_the_leader_does_not_consume_the_exit_event` failed with + "process ProcessId(26) is not running": the pid helper drained for + `Started`, and **`drain_until` ticks**. A tick can observe an + immediately-exiting child and move the record out of `Running`, after + which `signal` never reaches the diagnostic at all, so the bounded + loop spun to its limit. It passed standalone because the drain + returned on `Started` before `poll_one` saw the exit; only load lost + the race. Fast-exiting children now read the pid straight from the + supervisor record (no tick), and the loop fails fast if the record + left `Running`. **Verified under matched load: 0/15 with all 16 cores + saturated, while the old ticking helper fails 1/10 — the fix is + load-bearing.** +- Verification: fmt, `git diff --check`, strict workspace clippy clean; + lib 1,838 + CRDT 2,015 (both +6, exactly the new tests); GPU 202; M4 + 121; bottom-panel 46; compile-mode 67; vterm 9/6/5; **isolated-config + `--no-fail-fast` sweep 3,258 across 93 suites, zero failures**. + Earlier sweeps on this branch showed two failures and then one; the + totals reconcile (3,256/2 → 3,257/1 → 3,258/0, same test count). The + two that were genuinely unrelated — + `read_dir_supersede_cancels_in_flight_predecessor` (known + pre-existing) and + `headless_snapshot_round_trip_summary_restores_the_minimap` — are + load-contention flakes; the second is structurally unreachable from + this diff, since `pmacs-gpu` depends on `pmacs-protocol` and never on + `pmacs`. +- **Parked, each with its reason:** all tolerance rules (need the + evidence this PR produces); `terminate` idempotence for an + already-reaped process (independent fix, different failure, one + feature per PR); and `signal_target`'s read-then-kill of `tcgetpgrp` + — still the most likely real fix site. +- **The lane closes when this merges.** It does not wait for the flake + to recur; the next occurrence carries its own evidence under whoever's + PR, and a Stage B framing follows then. + ## Lean 4 lane (Arc 8) — Stages 1, 2, 3a, 3b, 4a MERGED; 4b is next - **Stages 1, 2, 3a and 3b are MERGED** — #160 (`main` @ `0827dd1`), diff --git a/docs/process-signal-tolerance-framing.md b/docs/process-signal-tolerance-framing.md new file mode 100644 index 0000000..1d3c45d --- /dev/null +++ b/docs/process-signal-tolerance-framing.md @@ -0,0 +1,258 @@ +# Framing — make the PTY terminate failure self-describing (diagnostic only) + +**Revision 4.** Status: awaiting review round 4. Lane: +`pty-terminate-eperm`, worktree `../pmacs-math-slice`, based on +`githubsucks/main` @ `ccf29e3`. + +**Diagnostic only. No disposition changes, no tolerance rules, no +behavioural fix.** Every rule this document proposed across revisions 1 +to 3 is parked (§5). The lane's entire deliverable is that the next +occurrence of the failure explains itself. + +## Revision history + +**Revision 3 → 4**, after review round 3 (two blocking, one major) and +its scope call. All accepted. + +- **Group-directed ESRCH was also unsafe**, for the same reason EPERM + was: it proves the selected *foreground group* vanished, not that the + leader exited. A job-control race — foreground job exits after + `tcgetpgrp` and before `kill`, shell alive and not yet reclaiming the + terminal — would have been reported as success with the leader never + signalled. Rev 3's acceptance 7 pinned that unsafe behaviour. **All + tolerance is parked** (§5). +- **Rev 3's Stage A implemented Stage B.** It declared itself + diagnostic-only, then listed tolerance and bookkeeping acceptances. + Removed. +- **Q#PS6 (already-reaped `terminate` is `Ok`) is parked separately.** + It is an independent behavioural fix answering a different failure; + under one-feature/one-PR it does not ride with instrumentation. +- **"Strictly additive / cannot regress behaviour" was overstated** and + is narrowed (Q#PD3). +- The injected-kill seam is restored as an explicit decision (Q#PD4). + +**Rounds 1–3, for the record.** Rev 1 classified on errno alone and +claimed a live owned child cannot yield EPERM — false. Rev 2 gated on +`try_wait`, which observes the leader while a PTY signal targets the +foreground group — unsound whenever those diverge, and it could not be +shown to fix the observed failure at all. Rev 3 corrected EPERM but left +ESRCH unsafe and mixed the stages. **Three consecutive designs were +wrong in the same direction: each tried to conclude something about a +process from something that was not about that process.** + + +## 0. Coherence impact (COHERENCE §20) + +- **Journey step 8, "Open a terminal"** (§2), teardown half. **No grade + change and no behavioural change** — this lane only improves what a + failure reports. +- **Serves §9 (worker model), failure attribution**, in its most literal + sense: an error that names only an errno cannot be attributed. +- **Interaction islands: none. Config registry: not adopted. + Background-work attribution: unchanged.** +- **No audited claim in COHERENCE.md changes**, so under §25 no + COHERENCE edit rides this PR. + + +## 1. Ground truth (scouted @ `ccf29e3`, re-verified each revision) + +### 1.1 The failure reports an errno and nothing else + +`ProcessSupervisor::signal` (`src/process.rs:921`) maps the `kill` +failure to `format!("kill: {e}")` (`:931`). That string is everything a +reader gets. + +### 1.2 The signal target is not the observation target + +- **Signal target** — `signal_target` (`:687`) returns `-pgrp` for a + PTY, where `pgrp = master.process_group_leader()`: the tty's + **current foreground process group**, read at signal time. +- **Observation target** — `ChildHandle::try_wait` (`:668`) observes the + **spawned leader**. + +They coincide only while the leader owns the terminal. Job control is +precisely the mechanism that makes them diverge, and the PTY path is +**always group-directed by design** — spawn rejects `group = true` for +PTY mode with the rationale that "PTY children already lead their own +session and are signaled group-wide" (`:1428-1429`). + +**This is why every tolerance rule across rev 1–3 failed review**, and +why the diagnostic must record the target and the leader state as +*separate* facts. + +### 1.3 The reap ledger is disjoint from this path + +`tick_reap_ledger` (`:1075`) treats any probe error as "nothing left we +can reach" for **bounded growth**, asserting EPERM "cannot happen for our +own children". It is armed only for `proc.spec.group`, which PTY mode +cannot set. Rev 1's "asymmetry" argument was a misreading; withdrawn. + +### 1.4 The observed failure, and the limits of the evidence + +macOS CI, PR #172 (**docs-only** diff), `Test (macos-latest / luajit)`, +`acc28_child_input_and_the_c_c_escape_work_unchanged_in_a_panel` +([attempt 1](https://github.com/levineuwirth/pmacs/actions/runs/30177276839/attempts/1)): + +``` +in function 'terminate' +cause: ExternalError(Process("kill: EPERM: Operation not permitted")) +``` + +**Established:** the errno, and the call path +(`pmacs.terminal.terminate` → `session.rs:566` → `signal`). + +**Not established:** that the child had exited (the probe's last source +statement is a file write at +`tests/bottom_panel_stage1_acceptance.rs:2239`; CPython teardown follows +and does not synchronise with it); that any pgid was recycled; or what +the signal target actually was. + +**This is the whole reason the lane is diagnostic.** Every candidate fix +needs at least one of those three facts, and none is available. + +### 1.5 Caller inventory + +| Caller | Disposition | +|---|---| +| `src/lsp.rs:1364`, `:2427` | discards (`let _ =`) | +| `src/mcp.rs:1229`, `:1239`, `:1915` | discards (`let _ =`) | +| `src/terminal/session.rs:319`, `:607`, `:635` | discards (`let _ =`) | +| **`src/terminal/session.rs:566`** (propagating at `:577`) | **propagates** as `TerminalError::Process` | +| supervisor-internal `shutdown` path | discards | +| `src/lua_bindings/mod.rs:8150`, `:8164` | propagates to Lua | +| `src/lua_bindings/mod.rs:8717` | propagates (via `session.rs:566`) | +| `src/daemon.rs:4162` | **test-only** `.expect`, not production | + +No test in the repository asserts either error string, so widening the +message breaks nothing. + +### 1.6 `portable-pty` caches the exit status on Unix + +Pinned `portable-pty 0.9.0`: `spawn_command` returns +`std::process::Child` (`unix.rs:228`), and `impl Child for +std::process::Child::try_wait` delegates to +`std::process::Child::try_wait` (`lib.rs:271-277`), which caches into +`self.status`. Both `ChildHandle` variants therefore cache. + + +## 2. Decisions + +### Q#PD1 — what the widened error records + +On a `kill` failure in `signal`, the error carries: + +| Field | Why | +|---|---| +| **target source** — `tcgetpgrp` vs `group` vs `leader-pid` fallback | which branch of `signal_target` (`:687`) ran | +| **target kind and value** — `-pgid` or `pid`, with the number | the entity actually signalled | +| **spawn-time pgid / leader pid** | a divergence from the target is the job-control hypothesis, visible only by comparison | +| **errno** | as today | +| **leader `try_wait` state** — `exited(status)` / `live` / `unobservable(e)` | separates "the leader is gone" from "the group we signalled is gone" — the distinction all three failed designs collapsed | + +Every candidate Stage B rule is decidable from these five together, and +none is decidable from the errno alone. + +### Q#PD2 — the disposition is preserved exactly + +The call still fails, with the same `Err`, in every case. No state +transition changes, no ledger arming changes, no tolerance. A reader +diffing behaviour should find none. + +### Q#PD3 — the honest claim is "no disposition change", not "strictly additive" + +Rev 3 said the diagnostic was only an error-string change and could not +regress behaviour. **That overstated it.** `try_wait` on an exited child +**reaps it and caches the status**, so consulting it in the failure path +is an internal state change: the child may be reaped earlier than it +otherwise would be. + +Observably safe, because both variants cache (§1.6) and `poll_one` +(`:1133`) will still see `Ok(Some(_))` and emit its event. But safe by +argument is not safe by assertion, so the terminate-failure-then-tick +event pin is retained (acceptance 5). + +### Q#PD4 — the injected-kill seam injects the KILL, never the observation + +Acceptance 5 needs a forced `kill` failure while the **real** +`ChildHandle::try_wait` runs against the **real** child. A stubbed +observation would bypass exactly the code path in question. + +So the seam is a test-only override of the *kill attempt's result*, +consumed once by the signal path; everything downstream — target +selection, the observation, the error construction — runs for real. This +also makes the diagnostic's own fields testable without racing the +kernel. + +### Q#PD5 — nothing else lands here + +No tolerance rule, no idempotence change, no `signal_target` change. See +§5. + + +## 3. Bets (falsifiable) + +- **B1 — The five fields are sufficient to discriminate the §1.4 + hypotheses.** Falsified if a recurrence carries all five and still + leaves the cause ambiguous — which would itself be a finding worth + having. +- **B2 — Widening the message breaks no caller.** Evidence: §1.5, and no + test asserts the string. + +*Retracted across revisions and not reinstated:* rev 1's "a live owned +child cannot yield EPERM"; rev 2's "exit observation suffices"; rev 2's +"this removes the failure class"; rev 3's "group ESRCH is safe to +tolerate". + + +## 4. Acceptance + +1. A group-directed `kill` failure produces an error carrying all five + Q#PD1 fields, with the target rendered as `-pgid` and the leader + state distinct from it. +2. A leader-directed `kill` failure does the same, with the target + rendered as `pid` and the target source recorded as the fallback + branch. +3. The leader state renders each of `exited(status)`, `live`, and + `unobservable(e)` correctly. +4. **The disposition is unchanged**: every injected failure still + returns `Err`, with no state transition and no ledger arming + (Q#PD2). Falsified by revert — flipping any arm to `Ok` fails this. +5. **Forced injected kill failure against the real PTY child + observation**, then tick: exactly one exit event, with the correct + status (Q#PD3/Q#PD4). A fully stubbed observation does not satisfy + this and is rejected as vacuous. +6. The existing suites stay green, pinning "no behavioural change" from + the outside. + + +## 5. Parked (not deferred-and-forgotten — each needs its own evidence) + +- **All tolerance rules.** Group-directed EPERM *and* ESRCH both fail on + the §1.2 entity split; leader-directed tolerance is plausible but + unmotivated until evidence shows the fallback branch is ever taken. + Needs Stage A evidence first. +- **Q#PS6, `terminate` on an already-reaped process returning `Ok`.** + Independent behavioural fix, different failure (§1.6 of rev 3), its + own lane under one-feature/one-PR. +- **`signal_target`'s read-then-kill of `tcgetpgrp`** — still the most + likely real fix site, still unframed. +- `terminate` cancelling pending restarts; PTYs in + `pmacs.process.list`; any change to `C-c` delivery. + + +## 6. Gates + +Full suite per `CLAUDE.md`. Touched suites: +`bottom_panel_stage1_acceptance`, the vterm stages, and +`compile_mode_acceptance`. Sweep with `-- --skip basedpyright`. + + +## 7. Branch plan + +`pty-terminate-eperm`, one PR, diagnostic only. This framing is its first +commit; the instrumentation and its tests are the second. + +**The lane then closes.** It does not wait for the flake to recur: the +next occurrence — whenever it happens, under whoever's PR — carries its +own evidence, and Stage B is framed then. Math work proceeds immediately +after this lands. diff --git a/src/process.rs b/src/process.rs index 2b53bf3..9e02629 100644 --- a/src/process.rs +++ b/src/process.rs @@ -470,6 +470,13 @@ pub struct ProcessSupervisor { /// TERM→KILL window used when arming the ledger. Constant /// [`GROUP_TERM_GRACE`] in production; overridable in tests. group_term_grace: Duration, + /// Q#PD4 test seam: forces the next `kill(2)` attempt in + /// [`Self::signal`] to fail with this errno, consumed once. + /// Always `None` in production — there is no way to set it outside + /// `cfg(test)`. It replaces the *kill result only*, so the leader + /// observation still runs against the real child handle; a stubbed + /// observation would bypass the code path under test. + forced_kill_errno: Option, } /// One armed group in the reap ledger. @@ -684,7 +691,50 @@ impl ChildHandle { } } -fn signal_target(proc: &ManagedProcess, pid: u32) -> Result { +/// Which branch of [`signal_target`] chose the target (Q#PD1). +/// +/// Recorded on failure because the branches differ in what a failing +/// `kill` can possibly mean: only [`Self::LeaderPid`] aims at the +/// spawned child itself. The other two aim at a *group*, which for a +/// PTY is read from the terminal and can belong to something the +/// supervisor never spawned. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum TargetSource { + /// The tty's current foreground process group, read at signal + /// time. Diverges from the leader exactly when job control has + /// moved the terminal. + ForegroundGroup, + /// A `group = true` pipe child leading its own process group. + SpawnGroup, + /// The child's own pid. + LeaderPid, +} + +impl TargetSource { + fn as_str(self) -> &'static str { + match self { + Self::ForegroundGroup => "tcgetpgrp", + Self::SpawnGroup => "group", + Self::LeaderPid => "leader-pid", + } + } + + /// Whether the target is a process group rather than one process. + fn is_group(self) -> bool { + matches!(self, Self::ForegroundGroup | Self::SpawnGroup) + } +} + +/// The entity a signal was actually aimed at, plus the branch that +/// chose it. Carried so a failure can report the target as a fact +/// separate from the leader's state (Q#PD1). +#[derive(Debug, Clone, Copy)] +struct SignalTarget { + pid: Pid, + source: TargetSource, +} + +fn signal_target(proc: &ManagedProcess, pid: u32) -> Result { if let Some(runtime) = proc.runtime.as_ref() && let ChildHandle::Pty { _master: master, .. @@ -692,7 +742,10 @@ fn signal_target(proc: &ManagedProcess, pid: u32) -> Result { && let Some(pgrp) = master.process_group_leader() && pgrp > 0 { - return Ok(Pid::from_raw(-pgrp)); + return Ok(SignalTarget { + pid: Pid::from_raw(-pgrp), + source: TargetSource::ForegroundGroup, + }); } // `group = true` pipe children lead a fresh process group // (`process_group(0)` at spawn ⇒ pgid == pid), so fatal signals @@ -700,11 +753,80 @@ fn signal_target(proc: &ManagedProcess, pid: u32) -> Result { // (Q#CM3). if proc.spec.group { let pgid = i32::try_from(pid).map_err(|e| e.to_string())?; - return Ok(Pid::from_raw(-pgid)); + return Ok(SignalTarget { + pid: Pid::from_raw(-pgid), + source: TargetSource::SpawnGroup, + }); } - Ok(Pid::from_raw( - i32::try_from(pid).map_err(|e| e.to_string())?, - )) + Ok(SignalTarget { + pid: Pid::from_raw(i32::try_from(pid).map_err(|e| e.to_string())?), + source: TargetSource::LeaderPid, + }) +} + +/// The spawned leader's state at the moment a `kill` failed (Q#PD1). +/// +/// Deliberately reported *beside* the target rather than folded into a +/// verdict: for a PTY the two are different entities whenever job +/// control has moved the terminal, and three successive designs for +/// this code were unsound precisely because they collapsed them. +enum LeaderObservation { + Exited(TermStatus), + Live, + Unobservable(String), + NoRuntime, +} + +impl LeaderObservation { + fn render(&self) -> String { + match self { + Self::Exited(TermStatus::Exited(code)) => format!("exited(code {code})"), + Self::Exited(TermStatus::Signaled(sig)) => format!("exited(signal {sig})"), + Self::Live => "live".to_owned(), + Self::Unobservable(e) => format!("unobservable({e})"), + Self::NoRuntime => "no-runtime".to_owned(), + } + } +} + +/// Observe the spawned leader. Note this *reaps* an exited child and +/// caches its status; that is why Q#PD3 claims "no disposition change" +/// rather than "strictly additive", and why an event-count test pins +/// that `poll_one` still emits exactly one exit event afterwards. +fn observe_leader(proc: &mut ManagedProcess) -> LeaderObservation { + let Some(runtime) = proc.runtime.as_mut() else { + return LeaderObservation::NoRuntime; + }; + match runtime.child.try_wait() { + Ok(Some(status)) => LeaderObservation::Exited(status), + Ok(None) => LeaderObservation::Live, + Err(e) => LeaderObservation::Unobservable(e), + } +} + +/// Render a failing `kill` as the five facts of Q#PD1. The disposition +/// is unchanged (Q#PD2) — this only replaces a message that said +/// nothing but the errno. +fn signal_failure_report( + target: SignalTarget, + leader_pid: u32, + errno: nix::errno::Errno, + leader: &LeaderObservation, +) -> String { + let expected = if target.source.is_group() { + match i32::try_from(leader_pid) { + Ok(p) => format!(", expected_group=-{p}"), + Err(_) => String::new(), + } + } else { + String::new() + }; + format!( + "kill: {errno} (target={} via {}, leader_pid={leader_pid}{expected}, leader={})", + target.pid.as_raw(), + target.source.as_str(), + leader.render(), + ) } /// Termination status of one generation. Internal --- the supervisor @@ -807,9 +929,20 @@ impl ProcessSupervisor { shut_down: false, reap_ledger: HashMap::new(), group_term_grace: GROUP_TERM_GRACE, + forced_kill_errno: None, } } + /// Q#PD4 test seam: make the next `kill(2)` attempt in + /// [`Self::signal`] report `errno` instead of calling the kernel. + /// Consumed by that one attempt. Everything downstream — target + /// selection, the leader observation against the real child, and + /// the error construction — runs unmodified. + #[cfg(test)] + fn force_next_kill_errno(&mut self, errno: nix::errno::Errno) { + self.forced_kill_errno = Some(errno); + } + /// Override the SIGTERM-to-SIGKILL grace window. Test helper. pub fn set_grace_period(&mut self, d: Duration) { self.grace_period = d; @@ -928,7 +1061,21 @@ impl ProcessSupervisor { return Err(format!("process {id} is not running")); }; let target = signal_target(proc, pid)?; - nix::sys::signal::kill(target, Some(signal)).map_err(|e| format!("kill: {e}"))?; + // Q#PD4: the seam injects the KILL attempt's result only — + // never the observation below — so target selection, the real + // `ChildHandle::try_wait` against the real child, and the error + // construction all run for real. Consumed once. + let kill_result = match self.forced_kill_errno.take() { + Some(errno) => Err(errno), + None => nix::sys::signal::kill(target.pid, Some(signal)), + }; + if let Err(errno) = kill_result { + // Q#PD1/Q#PD2: the failure describes itself; the + // disposition is unchanged — this still returns `Err`, + // with no state transition and no ledger arming. + let leader = observe_leader(proc); + return Err(signal_failure_report(target, pid, errno, &leader)); + } if matches!(signal, Signal::SIGTERM | Signal::SIGKILL | Signal::SIGHUP) { proc.state = ProcessState::Exiting { pid, @@ -2131,6 +2278,290 @@ mod tests { ); } + /// Spawn a PTY child that leads its own session and stays alive + /// until terminated, returning its id and OS pid. + /// + /// `/bin/sleep` directly rather than through a shell: a shell may + /// place the command in a different foreground process group, and + /// these tests assert the exact target the tty reports. + fn spawn_live_pty(sup: &mut ProcessSupervisor, name: &str) -> (ProcessId, u32) { + let mut spec = ProcessSpec::new(name, "/bin/sleep"); + spec.args = vec!["30".into()]; + spec.mode = ProcessMode::Pty { + rows: 24, + cols: 80, + mode: TerminalMode::Canonical, + }; + let id = sup.spawn(spec).expect("spawn"); + (id, spawn_started_pid(sup, id)) + } + + /// The OS pid straight from the supervisor's own record, WITHOUT + /// ticking. + /// + /// `drain_until` ticks, and a tick can observe a fast child's exit + /// and transition the record out of `Running` — after which + /// `signal` returns "is not running" and never reaches the + /// diagnostic at all. Any test whose child exits promptly must read + /// the pid this way. (Found by the parallel workspace sweep: the + /// drain-based helper raced only under load.) + fn record_pid(sup: &ProcessSupervisor, id: ProcessId) -> u32 { + match sup.processes.get(&id).expect("record").state { + ProcessState::Running { pid, .. } | ProcessState::Exiting { pid, .. } => pid, + ProcessState::Starting => panic!("spawn has not reported a pid yet"), + ProcessState::Terminated(_) => { + panic!("the record already left Running; the pid is unavailable") + } + } + } + + /// Drain until `Started` and return the OS pid it carries. Safe + /// only for children that outlive the drain; see [`record_pid`]. + fn spawn_started_pid(sup: &mut ProcessSupervisor, id: ProcessId) -> u32 { + let evs = drain_until(sup, id, Duration::from_secs(5), |evs| { + evs.iter() + .any(|e| matches!(e.kind, ProcessEventKind::Started { .. })) + }); + evs.iter() + .find_map(|e| match e.kind { + ProcessEventKind::Started { pid } => Some(pid), + _ => None, + }) + .expect("Started carries a pid") + } + + /// Drive the production diagnostic until it observes the leader as + /// exited, bounded by `timeout`. + /// + /// A fixed sleep is NOT proof of exit — on a loaded runner the child + /// can still be live, which would turn these tests into false + /// failures. This synchronises on the very observation under test. + /// Each failing attempt leaves the record untouched, because the + /// failure path returns before any bookkeeping (Q#PD2), so looping + /// is side-effect free. + fn terminate_until_leader_exited( + sup: &mut ProcessSupervisor, + id: ProcessId, + timeout: Duration, + ) -> String { + let deadline = Instant::now() + timeout; + loop { + sup.force_next_kill_errno(nix::errno::Errno::EPERM); + let err = sup.terminate(id).expect_err("injected EPERM must fail"); + if err.contains("leader=exited(") { + return err; + } + assert!( + !err.contains("is not running"), + "the record left Running before the diagnostic could run, so \ + this test never exercised it: {err}" + ); + assert!( + Instant::now() < deadline, + "leader never observed as exited within {timeout:?}: {err}" + ); + std::thread::sleep(Duration::from_millis(10)); + } + } + + /// Q#PD1 acceptance 1 — a group-directed failure names the target, + /// the branch that chose it, the expected group, the errno, and the + /// leader's own state, as five separate facts. + /// + /// Asserted as an exact message against the pid the kernel actually + /// assigned, so a hardcoded target could not satisfy it. The leader + /// field is the one that matters: for a PTY the signal goes to the + /// terminal's foreground group, a different entity from the spawned + /// child whenever job control has moved the terminal. Three rejected + /// designs for this code collapsed the two; the report keeps them + /// apart, and here they are asserted to agree only because nothing + /// has moved the terminal. + #[test] + fn a_group_directed_kill_failure_reports_target_and_leader_separately() { + let mut sup = ProcessSupervisor::new(); + let (id, pid) = spawn_live_pty(&mut sup, "diag-group"); + + sup.force_next_kill_errno(nix::errno::Errno::EPERM); + let err = sup.terminate(id).expect_err("injected EPERM must fail"); + + let expected = format!( + "kill: {} (target=-{pid} via tcgetpgrp, leader_pid={pid}, expected_group=-{pid}, leader=live)", + nix::errno::Errno::EPERM + ); + assert_eq!( + err, expected, + "the report names the exact target the tty reported, the exact \ + leader pid, and observes the leader as live" + ); + + let _ = sup.signal(id, Signal::SIGKILL); + } + + /// Q#PD1 acceptance 2 — a leader-directed failure records the + /// fallback branch and a positive target, and omits the group field + /// that would be meaningless for it. Exact message again. + #[test] + fn a_leader_directed_kill_failure_reports_the_fallback_branch() { + let mut sup = ProcessSupervisor::new(); + let mut spec = ProcessSpec::new("diag-leader", "/bin/sleep"); + spec.args = vec!["30".into()]; + let id = sup.spawn(spec).expect("spawn"); + let pid = spawn_started_pid(&mut sup, id); + + sup.force_next_kill_errno(nix::errno::Errno::ESRCH); + let err = sup.terminate(id).expect_err("injected ESRCH must fail"); + + let expected = format!( + "kill: {} (target={pid} via leader-pid, leader_pid={pid}, leader=live)", + nix::errno::Errno::ESRCH + ); + assert_eq!( + err, expected, + "a non-group pipe child targets its own pid, and the group \ + field is omitted where it has no meaning" + ); + + let _ = sup.signal(id, Signal::SIGKILL); + } + + /// Q#PD1 acceptance 3 — every leader state renders distinctly. The + /// `Unobservable` and `NoRuntime` arms cannot be produced by a real + /// child on demand, so they are pinned directly; `live` and `exited` + /// are pinned through the real path by the tests around this one. + #[test] + fn every_leader_observation_renders_distinctly() { + assert_eq!( + LeaderObservation::Exited(TermStatus::Exited(0)).render(), + "exited(code 0)" + ); + assert_eq!( + LeaderObservation::Exited(TermStatus::Signaled("SIGTERM".into())).render(), + "exited(signal SIGTERM)" + ); + assert_eq!(LeaderObservation::Live.render(), "live"); + assert_eq!( + LeaderObservation::Unobservable("try_wait: boom".into()).render(), + "unobservable(try_wait: boom)" + ); + assert_eq!(LeaderObservation::NoRuntime.render(), "no-runtime"); + } + + /// Q#PD1 acceptance 3, exited arm through the REAL path — the leader + /// has genuinely exited and the report carries its exact code, not + /// merely "some exit". + #[test] + fn a_failure_after_the_child_exits_reports_the_leader_as_exited() { + let mut sup = ProcessSupervisor::new(); + let mut spec = ProcessSpec::new("diag-exited", "/bin/sh"); + spec.args = vec!["-c".into(), "exit 3".into()]; + let id = sup.spawn(spec).expect("spawn"); + // NOT `spawn_started_pid`: draining ticks, and this child exits + // immediately. + let pid = record_pid(&sup, id); + + let err = terminate_until_leader_exited(&mut sup, id, Duration::from_secs(10)); + + let expected = format!( + "kill: {} (target={pid} via leader-pid, leader_pid={pid}, leader=exited(code 3))", + nix::errno::Errno::EPERM + ); + assert_eq!( + err, expected, + "the exact exit code is observed from the real child, not \ + inferred from the errno" + ); + } + + /// Q#PD2 acceptance 4 — **the disposition is unchanged.** An + /// injected failure still fails, and neither the state transition + /// nor the reap-ledger arming runs. This is the assertion that + /// separates a diagnostic from the tolerance rules three review + /// rounds rejected; flipping any arm to `Ok` fails it. + #[test] + fn an_injected_failure_changes_no_state_and_arms_no_ledger() { + let mut sup = ProcessSupervisor::new(); + let mut spec = ProcessSpec::new("diag-disposition", "/bin/sh"); + spec.args = vec!["-c".into(), "sleep 30".into()]; + spec.group = true; + let id = sup.spawn(spec).expect("spawn"); + let pid = spawn_started_pid(&mut sup, id); + assert!( + sup.reap_ledger.is_empty(), + "precondition: nothing armed before the attempt" + ); + + sup.force_next_kill_errno(nix::errno::Errno::EPERM); + let err = sup.terminate(id).expect_err("injected EPERM must fail"); + + let expected = format!( + "kill: {} (target=-{pid} via group, leader_pid={pid}, expected_group=-{pid}, leader=live)", + nix::errno::Errno::EPERM + ); + assert_eq!(err, expected, "a group=true pipe child reports via group"); + + assert!( + matches!( + sup.processes.get(&id).expect("record").state, + ProcessState::Running { .. } + ), + "a failed kill must not transition the record to Exiting" + ); + assert!( + sup.reap_ledger.is_empty(), + "a failed kill must not arm the reap ledger" + ); + + let _ = sup.signal(id, Signal::SIGKILL); + } + + /// Q#PD3/Q#PD4 acceptance 5 — the diagnostic consults the REAL + /// `ChildHandle::try_wait` on the REAL child, which reaps it and + /// caches the status. `poll_one` must still emit exactly one exit + /// event, carrying the exact code. + /// + /// A stubbed observation would bypass the double-`try_wait` path + /// entirely and pin nothing, so the injection replaces the kill + /// result only. + #[test] + fn observing_the_leader_does_not_consume_the_exit_event() { + let mut sup = ProcessSupervisor::new(); + let mut spec = ProcessSpec::new("diag-one-event", "/bin/sh"); + spec.args = vec!["-c".into(), "exit 7".into()]; + spec.mode = ProcessMode::Pty { + rows: 24, + cols: 80, + mode: TerminalMode::Canonical, + }; + let id = sup.spawn(spec).expect("spawn"); + // NOT `spawn_started_pid`: draining ticks, and a tick can reap + // this immediately-exiting child before the diagnostic runs. + let _ = record_pid(&sup, id); + + // Drives `observe_leader`, which try_waits the real PTY child + // for the first time and reaps it. + let err = terminate_until_leader_exited(&mut sup, id, Duration::from_secs(10)); + assert!( + err.contains("leader=exited(code 7)"), + "the real handle was consulted and carries the exact code: {err}" + ); + + // The supervisor's own try_wait must still see that status. + let evs = drain_until(&mut sup, id, Duration::from_secs(5), has_exited); + let terminal: Vec = evs + .iter() + .filter_map(|e| match e.kind { + ProcessEventKind::Exited { code, .. } => Some(code), + ProcessEventKind::Signaled { .. } => Some(-1), + _ => None, + }) + .collect(); + assert_eq!( + terminal, + vec![7], + "exactly one terminal event survives the diagnostic's try_wait, \ + carrying the child's real exit code" + ); + } #[test] fn signal_terminates_a_running_child() { let mut sup = ProcessSupervisor::new();