From dd581cd90ce1cc72c5d9a5f09caa3ca852b853b9 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sat, 25 Jul 2026 21:12:51 -0400 Subject: [PATCH 1/7] docs: frame the PTY terminate diagnostic (revision 4) A docs-only PR (#172) failed Test (macos-latest / luajit) on acc28_child_input_and_the_c_c_escape_work_unchanged_in_a_panel with "kill: EPERM: Operation not permitted" raised out of terminate. A docs diff cannot cause that, main was green at the PR's exact base, and three other PRs passed the same job. This framing reaches revision 4 after three review rounds, and what it proposes is much smaller than what it started with. Revisions 1 to 3 each proposed a tolerance rule -- treat some errno as success -- and each was unsound in the same way: they concluded something about a process from something that was not about that process. Revision 1 concluded from an errno alone, which says only that a syscall failed. Revision 2 concluded from the spawned leader while a PTY signal targets the tty's foreground process group, which diverges from the leader exactly when job control is in use. Revision 3 corrected EPERM but kept group-directed ESRCH, which proves only that the selected foreground group vanished, not that the leader exited. So no tolerance rule lands. The disposition is preserved exactly: every failing call still fails, with no state transition and no ledger arming. What lands is that the failure explains itself, recording the target source and value, the spawn-time pgid or leader pid, the errno, and the leader's real try_wait state as five separate facts. Every candidate fix is decidable from those together and none is decidable from the errno alone. Two claims are stated more narrowly than earlier revisions had them. Consulting try_wait reaps an exited child and caches its status, so this is not "strictly additive" -- it is "no disposition change", with an event-count test pinning that poll_one still emits exactly one exit event. And the test seam injects the kill attempt's result only, never the observation, so the real ChildHandle::try_wait runs against the real child; a stubbed observation would bypass the path under test. Parked with their reasons: all tolerance rules, terminate becoming idempotent for an already-reaped process (an independent fix answering a different failure), and signal_target's read-then-kill of tcgetpgrp, which is the most likely real fix site. The lane closes when this lands rather than waiting for the flake to recur; the next occurrence carries its own evidence under whoever's PR. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HZjWMjwPXhPbt9upku9mCk --- docs/process-signal-tolerance-framing.md | 258 +++++++++++++++++++++++ 1 file changed, 258 insertions(+) create mode 100644 docs/process-signal-tolerance-framing.md 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. From 62316a9ced3e8e36d9771fb3bd365640cc757052 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sat, 25 Jul 2026 21:25:58 -0400 Subject: [PATCH 2/7] fix(process): make a failing kill describe itself (Q#PD1) A failing kill in ProcessSupervisor::signal reported an errno and nothing else, which is not enough to diagnose the macOS CI failure that prompted this lane: three different hypotheses about that EPERM produce the same message, and the fix each one implies is different. The error now carries five facts as separate fields: the target source (which branch of signal_target ran), the target kind and value, the spawn-time group for a group-directed signal, the errno, and the spawned leader's real try_wait state. Keeping the target and the leader apart is the whole point. For a PTY the signal goes to the terminal's foreground process group, read from the tty at signal time, while the leader is the child that was spawned. Those are different entities whenever job control has moved the terminal, and three rejected designs for this code were unsound precisely because they concluded something about one from the other. The report states both and concludes nothing. The disposition is unchanged. Every call that failed before still fails, with no state transition and no reap-ledger arming. That is asserted directly rather than assumed, because it is what separates this from the tolerance rules review rejected. Q#PD3, stated narrowly: this is not a pure message change. Consulting try_wait reaps an exited child and caches its status, so the child may be reaped earlier than it otherwise would be. That is observably safe because portable-pty 0.9.0 returns a std::process::Child on Unix and delegates try_wait straight to it, so the status is cached and poll_one still sees it -- but safe by argument is not safe by assertion, so a test forces a kill failure against the real PTY child and then checks that exactly one terminal event survives. Q#PD4: the test seam injects the kill attempt's result only, never the observation. Target selection, the real ChildHandle::try_wait against the real child, and the error construction all run unmodified; a stubbed observation would bypass the code path under test. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HZjWMjwPXhPbt9upku9mCk --- src/process.rs | 385 ++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 378 insertions(+), 7 deletions(-) diff --git a/src/process.rs b/src/process.rs index 2b53bf3..88fb42e 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,230 @@ mod tests { ); } + /// Spawn a PTY child that stays alive until terminated, and wait + /// for its `Started` event so a pid and a foreground group exist. + fn spawn_live_pty(sup: &mut ProcessSupervisor, name: &str) -> ProcessId { + let mut spec = ProcessSpec::new(name, "/bin/sh"); + spec.args = vec!["-c".into(), "sleep 30".into()]; + spec.mode = ProcessMode::Pty { + rows: 24, + cols: 80, + mode: TerminalMode::Canonical, + }; + let id = sup.spawn(spec).expect("spawn"); + let _ = drain_until(sup, id, Duration::from_secs(5), |evs| { + evs.iter() + .any(|e| matches!(e.kind, ProcessEventKind::Started { .. })) + }); + id + } + + /// 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. + /// + /// The leader field is the one that matters: for a PTY the signal + /// goes to the terminal's foreground group, which is 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. + #[test] + fn a_group_directed_kill_failure_reports_target_and_leader_separately() { + let mut sup = ProcessSupervisor::new(); + let id = 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"); + + assert!(err.contains("EPERM"), "errno is reported: {err}"); + assert!( + err.contains("via tcgetpgrp"), + "the target SOURCE distinguishes a tty-read group from a spawn group: {err}" + ); + assert!( + err.contains("target=-"), + "a group target renders negative: {err}" + ); + assert!( + err.contains("expected_group=-"), + "the spawn-time group is shown so a divergence is visible: {err}" + ); + assert!( + err.contains("leader=live"), + "the leader is observed independently of the group: {err}" + ); + // Non-vacuity: the two numbers are actually rendered, not empty. + assert!( + err.contains("leader_pid=") && !err.contains("leader_pid=0,"), + "a real leader pid is reported: {err}" + ); + } + + /// 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. + #[test] + fn a_leader_directed_kill_failure_reports_the_fallback_branch() { + let mut sup = ProcessSupervisor::new(); + let mut spec = ProcessSpec::new("diag-leader", "/bin/sh"); + spec.args = vec!["-c".into(), "sleep 30".into()]; + let id = sup.spawn(spec).expect("spawn"); + let _ = drain_until(&mut sup, id, Duration::from_secs(5), |evs| { + evs.iter() + .any(|e| matches!(e.kind, ProcessEventKind::Started { .. })) + }); + + sup.force_next_kill_errno(nix::errno::Errno::ESRCH); + let err = sup.terminate(id).expect_err("injected ESRCH must fail"); + + assert!(err.contains("ESRCH"), "errno is reported: {err}"); + assert!( + err.contains("via leader-pid"), + "a non-group pipe child targets its own pid: {err}" + ); + assert!( + !err.contains("target=-"), + "a leader target renders positive: {err}" + ); + assert!( + !err.contains("expected_group="), + "the group field is omitted where it has no meaning: {err}" + ); + 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 says so. + #[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"); + // Wait for the child to actually be gone, but do NOT tick past + // the point where the record leaves Running — `signal` needs a + // live record to reach the kill at all. + std::thread::sleep(Duration::from_millis(300)); + + sup.force_next_kill_errno(nix::errno::Errno::EPERM); + let err = sup.terminate(id).expect_err("injected EPERM must fail"); + + assert!( + err.contains("leader=exited("), + "an exited leader is observed as exited, not guessed from the errno: {err}" + ); + } + + /// 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 _ = drain_until(&mut sup, id, Duration::from_secs(5), |evs| { + evs.iter() + .any(|e| matches!(e.kind, ProcessEventKind::Started { .. })) + }); + 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"); + assert!(err.contains("via group"), "a group=true pipe child: {err}"); + + 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 afterwards. + /// + /// 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"); + std::thread::sleep(Duration::from_millis(300)); + + // The forced failure drives `observe_leader`, which try_waits + // the real PTY child for the first time. + sup.force_next_kill_errno(nix::errno::Errno::EPERM); + let err = sup.terminate(id).expect_err("injected EPERM must fail"); + assert!( + err.contains("leader=exited("), + "the real handle was consulted: {err}" + ); + + // Now the supervisor's own try_wait must still see the status. + let evs = drain_until(&mut sup, id, Duration::from_secs(5), has_exited); + let terminal = evs + .iter() + .filter(|e| { + matches!( + e.kind, + ProcessEventKind::Exited { .. } | ProcessEventKind::Signaled { .. } + ) + }) + .count(); + assert_eq!( + terminal, 1, + "exactly one terminal event survives the diagnostic's try_wait" + ); + } + #[test] fn signal_terminates_a_running_child() { let mut sup = ProcessSupervisor::new(); From 52731ba1216055ab1c1380ee4f3cc63b42bfae26 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sat, 25 Jul 2026 21:30:23 -0400 Subject: [PATCH 3/7] style: pass Errno by value (clippy pedantic) Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HZjWMjwPXhPbt9upku9mCk --- src/process.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/process.rs b/src/process.rs index 88fb42e..1453e10 100644 --- a/src/process.rs +++ b/src/process.rs @@ -810,7 +810,7 @@ fn observe_leader(proc: &mut ManagedProcess) -> LeaderObservation { fn signal_failure_report( target: SignalTarget, leader_pid: u32, - errno: &nix::errno::Errno, + errno: nix::errno::Errno, leader: &LeaderObservation, ) -> String { let expected = if target.source.is_group() { @@ -1074,7 +1074,7 @@ impl ProcessSupervisor { // 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)); + return Err(signal_failure_report(target, pid, errno, &leader)); } if matches!(signal, Signal::SIGTERM | Signal::SIGKILL | Signal::SIGHUP) { proc.state = ProcessState::Exiting { From 40f7f8169019204ec2df2a85366f2673326d604b Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sat, 25 Jul 2026 22:16:56 -0400 Subject: [PATCH 4/7] test(process): pin exact diagnostic values and drop the timing-dependent sleeps Round-1 review found both test weaknesses. The exited-child tests used a fixed 300 ms sleep as proof the child had exited, which on a loaded runner can be false and would turn them into spurious failures. nix's waitid is unavailable on macOS and libc::waitid would need unsafe, which the crate forbids, so the tests now synchronise on the observation under test: a bounded loop that drives the production diagnostic until it reports the leader as exited. Each failing attempt leaves the record untouched because the failure path returns before any bookkeeping, so the loop is side-effect free, and it is strictly stronger than a sleep because it observes the actual state rather than assuming it. The assertions were substring checks -- target=-, expected_group=-, leader=exited( -- which a hardcoded target or a wrong exit code would satisfy. They are now exact message equality built from the pid the kernel actually assigned and the errno's own Display, and the one-event test asserts the surviving event carries exit code 7 rather than any terminal event. The group test also spawns /bin/sleep directly rather than through a shell, since a shell may place the command in a different foreground process group than the one being asserted. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HZjWMjwPXhPbt9upku9mCk --- src/process.rs | 213 ++++++++++++++++++++++++++++--------------------- 1 file changed, 122 insertions(+), 91 deletions(-) diff --git a/src/process.rs b/src/process.rs index 1453e10..a944de4 100644 --- a/src/process.rs +++ b/src/process.rs @@ -2278,103 +2278,131 @@ mod tests { ); } - /// Spawn a PTY child that stays alive until terminated, and wait - /// for its `Started` event so a pid and a foreground group exist. - fn spawn_live_pty(sup: &mut ProcessSupervisor, name: &str) -> ProcessId { - let mut spec = ProcessSpec::new(name, "/bin/sh"); - spec.args = vec!["-c".into(), "sleep 30".into()]; + /// 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"); - let _ = drain_until(sup, id, Duration::from_secs(5), |evs| { + (id, spawn_started_pid(sup, id)) + } + + /// Drain until `Started` and return the OS pid it carries. + 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 { .. })) }); - id + 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!( + 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. /// - /// The leader field is the one that matters: for a PTY the signal - /// goes to the terminal's foreground group, which is 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. + /// 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 = spawn_live_pty(&mut sup, "diag-group"); + 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"); - assert!(err.contains("EPERM"), "errno is reported: {err}"); - assert!( - err.contains("via tcgetpgrp"), - "the target SOURCE distinguishes a tty-read group from a spawn group: {err}" + let expected = format!( + "kill: {} (target=-{pid} via tcgetpgrp, leader_pid={pid}, expected_group=-{pid}, leader=live)", + nix::errno::Errno::EPERM ); - assert!( - err.contains("target=-"), - "a group target renders negative: {err}" - ); - assert!( - err.contains("expected_group=-"), - "the spawn-time group is shown so a divergence is visible: {err}" - ); - assert!( - err.contains("leader=live"), - "the leader is observed independently of the group: {err}" - ); - // Non-vacuity: the two numbers are actually rendered, not empty. - assert!( - err.contains("leader_pid=") && !err.contains("leader_pid=0,"), - "a real leader pid is reported: {err}" + 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. + /// 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/sh"); - spec.args = vec!["-c".into(), "sleep 30".into()]; + let mut spec = ProcessSpec::new("diag-leader", "/bin/sleep"); + spec.args = vec!["30".into()]; let id = sup.spawn(spec).expect("spawn"); - let _ = drain_until(&mut sup, id, Duration::from_secs(5), |evs| { - evs.iter() - .any(|e| matches!(e.kind, ProcessEventKind::Started { .. })) - }); + 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"); - assert!(err.contains("ESRCH"), "errno is reported: {err}"); - assert!( - err.contains("via leader-pid"), - "a non-group pipe child targets its own pid: {err}" + let expected = format!( + "kill: {} (target={pid} via leader-pid, leader_pid={pid}, leader=live)", + nix::errno::Errno::ESRCH ); - assert!( - !err.contains("target=-"), - "a leader target renders positive: {err}" - ); - assert!( - !err.contains("expected_group="), - "the group field is omitted where it has no meaning: {err}" + 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. + /// `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!( @@ -2393,25 +2421,27 @@ mod tests { 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 says so. + /// 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"); - // Wait for the child to actually be gone, but do NOT tick past - // the point where the record leaves Running — `signal` needs a - // live record to reach the kill at all. - std::thread::sleep(Duration::from_millis(300)); + let pid = spawn_started_pid(&mut sup, id); - sup.force_next_kill_errno(nix::errno::Errno::EPERM); - let err = sup.terminate(id).expect_err("injected EPERM must fail"); + let err = terminate_until_leader_exited(&mut sup, id, Duration::from_secs(10)); - assert!( - err.contains("leader=exited("), - "an exited leader is observed as exited, not guessed from the errno: {err}" + 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" ); } @@ -2427,10 +2457,7 @@ mod tests { spec.args = vec!["-c".into(), "sleep 30".into()]; spec.group = true; let id = sup.spawn(spec).expect("spawn"); - let _ = drain_until(&mut sup, id, Duration::from_secs(5), |evs| { - evs.iter() - .any(|e| matches!(e.kind, ProcessEventKind::Started { .. })) - }); + let pid = spawn_started_pid(&mut sup, id); assert!( sup.reap_ledger.is_empty(), "precondition: nothing armed before the attempt" @@ -2438,7 +2465,12 @@ mod tests { sup.force_next_kill_errno(nix::errno::Errno::EPERM); let err = sup.terminate(id).expect_err("injected EPERM must fail"); - assert!(err.contains("via group"), "a group=true pipe child: {err}"); + + 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!( @@ -2458,7 +2490,7 @@ mod tests { /// 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 afterwards. + /// 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 @@ -2474,34 +2506,33 @@ mod tests { mode: TerminalMode::Canonical, }; let id = sup.spawn(spec).expect("spawn"); - std::thread::sleep(Duration::from_millis(300)); + let _ = spawn_started_pid(&mut sup, id); - // The forced failure drives `observe_leader`, which try_waits - // the real PTY child for the first time. - sup.force_next_kill_errno(nix::errno::Errno::EPERM); - let err = sup.terminate(id).expect_err("injected EPERM must fail"); + // 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("), - "the real handle was consulted: {err}" + err.contains("leader=exited(code 7)"), + "the real handle was consulted and carries the exact code: {err}" ); - // Now the supervisor's own try_wait must still see the status. + // 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 = evs + let terminal: Vec = evs .iter() - .filter(|e| { - matches!( - e.kind, - ProcessEventKind::Exited { .. } | ProcessEventKind::Signaled { .. } - ) + .filter_map(|e| match e.kind { + ProcessEventKind::Exited { code, .. } => Some(code), + ProcessEventKind::Signaled { .. } => Some(-1), + _ => None, }) - .count(); + .collect(); assert_eq!( - terminal, 1, - "exactly one terminal event survives the diagnostic's try_wait" + 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(); From 18d481b046a5237a5b955dea45e9b13043cf20df Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sat, 25 Jul 2026 22:19:19 -0400 Subject: [PATCH 5/7] docs: record the PTY terminate diagnostic lane in the ledger The ledger's own update protocol requires a lane for volatile work, and PR #176 had none: branch, worktree, review state, and verification were all missing. Records why the lane ships a diagnostic rather than a fix -- three rejected tolerance designs, the two facts that killed the original argument (group=true is rejected for PTY mode so the reap ledger never applies to that path, and the ledger comment asserts EPERM cannot happen rather than ruling that it means dead), and that the CI evidence never established the child had exited. Also records the round-1 test fixes and the four verified bites, so a reader can tell which assertions are load-bearing. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HZjWMjwPXhPbt9upku9mCk --- docs/active-work.md | 69 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) diff --git a/docs/active-work.md b/docs/active-work.md index b60d62c..ffd9b1e 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -55,6 +55,75 @@ 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. +- 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; sweep 3,256 across + 93 suites with two load-contention flakes that pass 3/3 isolated + (`read_dir_supersede_cancels_in_flight_predecessor`, known + pre-existing, and + `headless_snapshot_round_trip_summary_restores_the_minimap`). The + second is structurally unreachable from this diff: `pmacs-gpu` depends + on `pmacs-protocol`, 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) — Stage 1 MERGED; Stage 2 IN REVIEW (PR #161) - Stage 1 **merged as #160** (`main` @ `0827dd1`, 2026-07-25, one review From 00cc615db557d8f8cf67a574df3e4f0e5a65aef8 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sat, 25 Jul 2026 22:25:05 -0400 Subject: [PATCH 6/7] test(process): read the pid without ticking in the fast-exit tests The parallel workspace sweep failed observing_the_leader_does_not_consume_the_exit_event with "process ProcessId(26) is not running". A real defect in the test, not a flake. The helper that fetched the pid drained for the Started event, and draining ticks. A tick can observe an immediately-exiting child and transition the record out of Running, after which signal returns "is not running" and never reaches the diagnostic -- so the loop spun to its 10 s bound and panicked. It passed standalone because the drain returned on Started before poll_one saw the exit; only the sweep's load shifted the timing enough to lose that race. Fast-exiting children now read the pid straight from the supervisor record, which does not tick. The bounded loop also fails fast when the record has left Running, so a future recurrence is diagnosed in one line rather than surfacing as a timeout. Verified under matched load: 15/15 green with all 16 cores saturated. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HZjWMjwPXhPbt9upku9mCk --- src/process.rs | 35 ++++++++++++++++++++++++++++++++--- 1 file changed, 32 insertions(+), 3 deletions(-) diff --git a/src/process.rs b/src/process.rs index a944de4..9e02629 100644 --- a/src/process.rs +++ b/src/process.rs @@ -2296,7 +2296,27 @@ mod tests { (id, spawn_started_pid(sup, id)) } - /// Drain until `Started` and return the OS pid it carries. + /// 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() @@ -2331,6 +2351,11 @@ mod tests { 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}" @@ -2430,7 +2455,9 @@ mod tests { let mut spec = ProcessSpec::new("diag-exited", "/bin/sh"); spec.args = vec!["-c".into(), "exit 3".into()]; let id = sup.spawn(spec).expect("spawn"); - let pid = spawn_started_pid(&mut sup, id); + // 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)); @@ -2506,7 +2533,9 @@ mod tests { mode: TerminalMode::Canonical, }; let id = sup.spawn(spec).expect("spawn"); - let _ = spawn_started_pid(&mut sup, id); + // 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. From 3b7cc67197924474544d9d73e6539c9446f1ac75 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sat, 25 Jul 2026 22:33:22 -0400 Subject: [PATCH 7/7] docs: record the sweep-found test race in the lane The parallel sweep failed one of the new tests for a real reason, not a flake: drain_until ticks, and a tick can reap an immediately-exiting child before the diagnostic runs. Recorded with the matched-load measurement that shows the fix is load-bearing (0/15 fixed vs 1/10 unfixed under full saturation), and the final sweep numbers. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HZjWMjwPXhPbt9upku9mCk --- docs/active-work.md | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index ffd9b1e..497dd63 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -106,15 +106,32 @@ If it does not, stop and repair the remote/fetch configuration. 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; sweep 3,256 across - 93 suites with two load-contention flakes that pass 3/3 isolated - (`read_dir_supersede_cancels_in_flight_predecessor`, known - pre-existing, and - `headless_snapshot_round_trip_summary_restores_the_minimap`). The - second is structurally unreachable from this diff: `pmacs-gpu` depends - on `pmacs-protocol`, never on `pmacs`. + 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