neuwirth
/
pmacs
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix(process): sample the group before the kill; make corroboration real 

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.
This commit is contained in:
Levi Neuwirth 2026-07-30 14:27:46 -04:00
parent 66f73013fc
commit 95897f7563
3 changed files with 128 additions and 39 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

29
docs/active-work.md
View File

@ -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

53
docs/process-signal-diagnostic-completeness-framing.md
View File

@ -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.

85
src/process.rs
View File

@ -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<i32> {
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);