From 95897f75636a0bd3ffa10b4a1cadbac8bce04e42 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 30 Jul 2026 14:27:46 -0400 Subject: [PATCH] fix(process): sample the group before the kill; make corroboration real MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 4: three blocking, one major. All accepted. **`measured_group` was sampled after the failure.** It ran inside `signal_failure_report`, after the `kill` and after `observe_leader`, while the framing and the function's own doc both said before. A concurrent group change would have made the diagnostic report post-failure state as evidence about the attempted target. It is now sampled in `signal` before the kill and passed into the report, so the field describes the target that was attempted rather than the state the failure left behind. **The Linux corroboration did not exercise the production lookup.** Its helper read `portable_pty::process_group_leader` — the accessor this lane stopped using on the signal path — so `pty_foreground_group` could have fallen back on every call with every test still green. Forcing it to always fall back demonstrates the hole exactly: the corroboration fails, and the injected pin PASSES, because the injected tests supply the group themselves and structurally cannot detect a broken lookup. The helper now calls the production lookup, and the corroboration forces only the kill so the report is built from a real terminal read. The residual limitation is recorded rather than left to the green: on macOS `pty_foreground_group` has no end-to-end coverage, because the platform cannot produce the precondition. **The framing did not update its own acceptance contract.** Revision 5 recorded Bet 1's falsification in the revision history and in the bet, and left the normative criterion demanding the real-shell rewrite — the "implementation quietly diverges from the contract" shape this project already recorded as a lesson on #191/#188. Acceptance 1 now describes the injected pin, 1a adds the corroboration and its macOS limitation, and 4 states the sampling order. The ledger is synchronized: revision 6, four commits, 4,471 tests, bash armed on Linux only. **`TargetSource`'s doc had the wrong classification.** Two of the four variants target the leader pid, not one, and the pid-versus-group split does not line up with PTY-versus-pipe — which is why the fallback needed its own variant rather than reusing `LeaderPid`. Gates: 11 gates, 4,471 tests, zero failures. --- docs/active-work.md | 29 ++++--- ...-signal-diagnostic-completeness-framing.md | 53 ++++++++++-- src/process.rs | 85 ++++++++++++++----- 3 files changed, 128 insertions(+), 39 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index 33ad59d..28be8ca 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -122,11 +122,13 @@ If it does not, stop and repair the remote/fetch configuration. - **Portable branch:** `githubsucks/process-signal-diagnostic-completeness`; worktree `../pmacs-signal-identity`. Governing document: - `docs/process-signal-diagnostic-completeness-framing.md`, **revision 4, - approved** after three review rounds. -- **State:** implemented and gated; canonical `githubsucks/main` @ - `b8e18f6` (the ledger absorption #199) is integrated. Two commits — - Bet 1 alone, then Bets 2–4 — per the framing's branch plan. + `docs/process-signal-diagnostic-completeness-framing.md`, **revision 6**; + the contract was approved at revision 4 and has been revised twice + since to match what shipped. +- **State:** **PR #200 open**, four review rounds closed, held for + review. Canonical `githubsucks/main` @ `b8e18f6` (the ledger + absorption #199) is integrated. Four commits: Bet 1 alone, Bets 2–4, + Bet 1's fallback after CI falsified it, then round 4's corrections. - **Boundary, unchanged:** evidence collection only. No signal retargeting, tolerance, disposition change, or reap-ledger repair. **Group identity remains unprovable** (framing §1.5): the measured @@ -143,7 +145,7 @@ If it does not, stop and repair the remote/fetch configuration. claiming "EPERM cannot happen for our own children" is corrected **without** claiming the child itself received EPERM. - **Verification at the merged tree** (not inherited from the pre-merge - head): 11 gates, **4469 tests, zero failures** — fmt, diff-check, + head): 11 gates, **4471 tests, zero failures** — fmt, diff-check, clippy, `--lib`, `--lib --features crdt`, compile-mode, copy-mode in both feature configurations, bottom-panel Stage 1, M4 with the basedpyright skip, and required GPU. The job-control divergence @@ -157,11 +159,16 @@ If it does not, stop and repair the remote/fetch configuration. Separately, the **pre-Stage-B test was restored verbatim under the substitution mutation and PASSED**, which is the finding that justified rewriting it rather than adding to it. -- **`/bin/bash` is a declared optional test dependency**, armed by - `PMACS_REQUIRE_BASH` on **both** CI platforms rather than Linux only: - the failures this diagnostic exists to explain have so far occurred - only on macOS, so arming it Linux-only would leave it dark exactly - where it matters. +- **Bet 1 was falsified by CI and the framing's fallback shipped.** + `bash -m` diverges on Linux and **never on macOS**, where both legs + observed the terminal stay with the leader for a full 10s wait. The + divergent case is now pinned by **injecting** the foreground group + (runs everywhere, weaker); a Linux-only corroboration drives a real + shell and is the **only** test exercising `pty_foreground_group` + end-to-end. `/bin/bash` is a declared optional test dependency armed + by `PMACS_REQUIRE_BASH` on **Linux only** — macOS ships bash but + cannot produce the precondition, so arming it there would make a + missing binary fatal for a test that can never run. - **Recovery from a clean checkout:** ```sh diff --git a/docs/process-signal-diagnostic-completeness-framing.md b/docs/process-signal-diagnostic-completeness-framing.md index 7dad7fe..5c41617 100644 --- a/docs/process-signal-diagnostic-completeness-framing.md +++ b/docs/process-signal-diagnostic-completeness-framing.md @@ -1,6 +1,6 @@ # Framing — make the signal diagnostic discriminating (evidence collection) -**Revision 5.** Status: implemented; PR open. Lane: +**Revision 6.** Status: implemented; PR open, review round 4 closed. Lane: `process-signal-diagnostic-completeness`, worktree `../pmacs-signal-identity`, based on `githubsucks/main` @ `4cd4a7b` (re-measure at branch time; this is a reading, not a constant). @@ -17,6 +17,32 @@ gets signalled, no disposition change.** Everything behavioural is in §5. ## Revision history +**Revision 5 → 6**, after review round 4 (three blocking, one major). +All four accepted. + +- **`measured_group` was sampled AFTER the failed `kill`** and after + `observe_leader`, while both the framing and the function's own doc + said before. A concurrent group change would have made the diagnostic + report post-failure state as evidence about the attempted target. It + is now sampled in `signal` before the kill and passed into the report. +- **The Linux corroboration did not exercise the production lookup.** + Its helper read `portable_pty::process_group_leader` — the accessor + this lane stopped using — so `pty_foreground_group` could have fallen + back on every call with every test still green. Demonstrated: forcing + it to always fall back leaves the injected pin **passing** and only + the corroboration failing. The helper now calls the production lookup, + and the corroboration forces *only* the kill so the report is built + from a real terminal read. +- **This document did not update its own acceptance contract** (§4.1). + Revision 5 recorded the falsification in the revision history and Bet + 1 but left the normative criterion demanding the real-shell rewrite — + the exact "implementation quietly diverges from the contract" shape + this project recorded as a lesson on #191/#188. +- **`TargetSource`'s doc had the wrong classification.** Two of four + variants now target the leader pid, not one, and the pid-versus-group + split does not line up with PTY-versus-pipe — which is *why* + `PtyForegroundFallback` needed its own variant. + **Revision 4 → 5**, after implementation. **Bet 1 was falsified by CI**, and the framing's own fallback is what shipped. @@ -431,10 +457,24 @@ assertion (`:2517`) is one of the four sites acceptance 5 must update. ## 4. Acceptance -1. A PTY job-control fixture where `tcgetpgrp` != leader pid, both exact - values asserted and asserted to differ. The test at `:2400` is - **rewritten**, not supplemented — it currently pins a substitution as +1. The divergent case is pinned on **every** platform: a group-directed + failure whose target differs from the leader pid, with both exact + values asserted and asserted to differ. The pre-Stage-B test is + **rewritten**, not supplemented — it pinned a substitution as acceptable. + + **The divergence is injected at the `signal_target` seam**, per Bet + 1's falsification: a real `bash -m` fixture diverges on Linux and + never on macOS. The injected form is weaker and §3 Bet 1 says how. + +1a. **A Linux-only corroboration** drives a real job-control shell, + forces *only* the kill failure, and asserts the production report + names the real foreground group. This is the sole test that exercises + `pty_foreground_group` end-to-end — every other test supplies the + group itself and therefore cannot detect a lookup that always falls + back. **Residual limitation, stated rather than buried: on macOS the + production lookup has no end-to-end coverage**, because the platform + cannot produce the precondition. 2. The PTY foreground-lookup fallback reports a source distinct from a pipe child's. Separate tests drive the duplicate-error and `tcgetpgrp`-error arms and assert the exact stage and errno; a third @@ -450,7 +490,10 @@ assertion (`:2517`) is one of the four sites acceptance 5 must update. while a successful `SIGTERM` does. 4. For `spec.group` children the report carries the measured pgid as a field distinct from the assumed one, renderable as unobservable, with - a test asserting a case where they **differ** (§3 Bet 4). + a test asserting a case where they **differ** (§3 Bet 4). **It is + sampled before the `kill`**, not during report construction, so it + describes the target that was attempted rather than the state left + behind by the failure. 5. All four exact-string sites — `:2408`, `:2435`, `:2485`, `:2517` — updated **individually**, each listed in the PR body with before and after. No blanket rewrite: that is how a format regression hides. diff --git a/src/process.rs b/src/process.rs index 70c1c80..ea9b377 100644 --- a/src/process.rs +++ b/src/process.rs @@ -717,10 +717,18 @@ impl ChildHandle { /// 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. +/// `kill` can possibly mean. Two of the four aim at the spawned child +/// itself — [`Self::LeaderPid`] for a pipe child with no group, and +/// [`Self::PtyForegroundFallback`] for a PTY whose terminal named no +/// group. The other two aim at a *group*: +/// [`Self::SpawnGroup`] at one computed from the spawn-time `pgid == +/// pid` assumption, and [`Self::ForegroundGroup`] at one read from the +/// terminal, which can belong to something the supervisor never spawned. +/// +/// The pid-versus-group split is the classification that matters here, +/// and it does **not** line up with the PTY-versus-pipe split — which is +/// exactly why the fallback needed its own variant instead of reusing +/// `LeaderPid`. #[derive(Debug, Clone, Copy, PartialEq, Eq)] enum TargetSource { /// The tty's current foreground process group, read at signal @@ -976,7 +984,7 @@ fn observe_leader(proc: &mut ManagedProcess) -> LeaderObservation { /// makes it worth printing. /// /// **It does not establish identity** (framing §1.5). It is read before -/// the `kill` in the same read-then-act window, and a number cannot +/// the `kill`, in the same read-then-act window, and a number cannot /// distinguish the original group from a recycled one. No portable /// mechanism can: `pidfd` closes pid reuse for a process, not a group, /// and macOS has none at all. This records an observation; it settles @@ -1007,6 +1015,7 @@ fn signal_failure_report( signal: Signal, errno: nix::errno::Errno, leader: &LeaderObservation, + measured: Option<&str>, ) -> String { let expected = if target.source.is_group() { match i32::try_from(leader_pid) { @@ -1016,15 +1025,11 @@ fn signal_failure_report( } else { String::new() }; - // Only the spawn-group path computes its target from the assumption, - // so it is the only one where a measurement can contradict anything. - // A PTY target came from the terminal and a leader-directed target is - // not a group at all. - let measured = if matches!(target.source, TargetSource::SpawnGroup) { - measured_group_of(leader_pid) - } else { - String::new() - }; + // Supplied by the caller, which samples it BEFORE the `kill`. Doing + // it here would describe the group as it stands *after* the failure + // and after `observe_leader`, which is post-hoc state presented as + // evidence about the attempted target. + let measured = measured.unwrap_or(""); format!( "kill: {errno} (signal={signal:?}, target={} via {}, leader_pid={leader_pid}{expected}{measured}, leader={})", target.pid.as_raw(), @@ -1292,6 +1297,13 @@ impl ProcessSupervisor { }; let forced_lookup = self.forced_pty_lookup.take(); let target = signal_target(proc, pid, forced_lookup)?; + // Sample the real group BEFORE signalling. Only the spawn-group + // path computes its target from the `pgid == pid` assumption, so + // it is the only one a measurement can contradict; a PTY target + // came from the terminal and a leader-directed target is not a + // group at all. + let measured = + matches!(target.source, TargetSource::SpawnGroup).then(|| measured_group_of(pid)); // 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 @@ -1305,7 +1317,14 @@ 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, signal, errno, &leader)); + return Err(signal_failure_report( + target, + pid, + signal, + errno, + &leader, + measured.as_deref(), + )); } if matches!(signal, Signal::SIGTERM | Signal::SIGKILL | Signal::SIGHUP) { proc.state = ProcessState::Exiting { @@ -2628,15 +2647,19 @@ mod tests { (id, spawn_started_pid(sup, id)) } - /// The tty's current foreground process group, read through the same - /// `MasterPty` accessor production uses. `None` for a pipe + /// The tty's current foreground process group, read through the + /// **production** lookup — not `portable_pty`'s + /// `process_group_leader`, which this crate no longer uses on the + /// signal path. Reading it any other way would let + /// `pty_foreground_group` fall back on every call while every test + /// that depends on it stayed green. `None` for a pipe /// generation, or when the terminal reports no foreground group. fn foreground_pgid(sup: &ProcessSupervisor, id: ProcessId) -> Option { let runtime = sup.processes.get(&id)?.runtime.as_ref()?; match &runtime.child { ChildHandle::Pty { _master: master, .. - } => master.process_group_leader(), + } => pty_foreground_group(master.as_ref()).ok(), ChildHandle::Pipes(_) => None, } } @@ -2824,12 +2847,28 @@ mod tests { "a real job-control shell must move the terminal off the leader" ); - // And the production lookup — not the fixture's own polling — - // reports that same group. - let observed = foreground_pgid(&sup, id).expect("PTY reports a foreground group"); + // Force ONLY the kill failure. The lookup is left alone, so + // `pty_foreground_group` runs for real against a real terminal + // and the report below is built from what it returned. + // + // This is the assertion that makes the injected pin meaningful: + // without it, `pty_foreground_group` could fall back on every + // call and every other test here would still pass, because they + // all supply the group themselves. + sup.force_next_kill_errno(nix::errno::Errno::EPERM); + let err = sup.terminate(id).expect_err("injected EPERM must fail"); + + let expected = format!( + "kill: {} (signal=SIGTERM, target=-{fg} via tcgetpgrp, leader_pid={pid}, expected_group=-{pid}, leader=live)", + nix::errno::Errno::EPERM + ); assert_eq!( - observed, fg, - "the production accessor must see what the fixture waited for" + err, expected, + "the production lookup must report the real foreground group" + ); + assert!( + !err.contains("pty-leader-fallback"), + "a healthy terminal must not take the fallback branch: {err}" ); let _ = sup.signal(id, Signal::SIGKILL);