From 088f417e70a94bb6e42a4c0cfdbe89061e5b4456 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 30 Jul 2026 16:13:02 -0400 Subject: [PATCH 1/8] docs(process): frame the reap ledger's silent failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Unparked from PR #200's §5, which retired the premise justifying the ledger's leniency and deliberately changed no disposition. Scouting found three silent failures where #200 named two. A probe error of any errno drops the entry and cancels escalation; a failed escalating SIGKILL is recorded as a success and never retried; and `shutdown()` discards its own force-kill result identically — on the path written specifically to stop a leak at editor exit. Two facts shape the lane. `shutdown()`'s final loop terminates when the ledger empties, which happens through the same silent drop, so the probe error that hides a leak can also end the cleanup loop early — the probe cannot be made strict in isolation. And all three paths are untestable today: `tick_reap_ledger` and `shutdown()` call nix directly and consult no injection seam, unlike `signal()`'s `forced_kill_errno`, so every one of the five existing ledger tests exercises only the success path. Diagnosis first; no disposition change is proposed. Stage A of the signal lane had three tolerance rules rejected across three revisions, each for concluding something about one entity from something about another, on this same data structure. The framing states plainly that none of the three has been observed: #200 saw an explicit SIGTERM fail in `signal()`, not a ledger call. Framing only. No code. --- docs/active-work.md | 44 ++++ docs/reap-ledger-silent-failures-framing.md | 273 ++++++++++++++++++++ 2 files changed, 317 insertions(+) create mode 100644 docs/reap-ledger-silent-failures-framing.md diff --git a/docs/active-work.md b/docs/active-work.md index 09c5c9d..0399359 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -333,6 +333,50 @@ compatible. githubsucks/test-ambient-config-isolation ``` +## Reap-ledger silent failures — FRAMING OPEN, revision 1 + +- **Branch `reap-ledger-silent-failures`**, worktree + `../pmacs-reap-ledger`, based on `githubsucks/main` @ `22df6ab`. + **Framing only; no code, no PR yet.** + `docs/reap-ledger-silent-failures-framing.md`, revision 1. +- **Unparked from PR #200's §5.** #200 retired the premise that + justified the ledger's leniency and deliberately changed no + disposition; this lane owns what it refused. +- **Three silent failures, not the two #200 named.** A probe error of + any errno drops the entry and cancels escalation; a failed escalating + `SIGKILL` is marked as succeeded and never retried; and `shutdown()` + discards its own force-kill result the same way — on the path that + exists specifically to stop a leak at editor exit. +- **The blast radius is exactly what the ledger exists for:** a + TERM-ignoring descendant that outlived its leader with output + redirected. Neither leader state nor reader state can see it; only + group liveness can. A silent drop leaks the one process nothing else + is watching. +- **`shutdown()`'s final loop terminates when the ledger empties**, + which happens via the same silent drop — so the probe error that hides + a leak can also end the cleanup loop early. That coupling is why the + probe cannot be made strict on its own. +- **None of the three has been observed.** #200 saw an explicit + `SIGTERM` fail in `signal()`, not a ledger call. The premise is + falsified and the path exposed; the occurrence is not evidence these + fire. +- **They are also untestable today**: `tick_reap_ledger` and + `shutdown()` call `nix` directly and consult no injection seam, unlike + `signal()`'s `forced_kill_errno`. All five existing ledger tests + exercise the success path only. **Bet 1 ships the seam alone and + first** and is worth landing even if the rest is abandoned. +- **Diagnosis first, no disposition change proposed.** Stage A of the + signal lane had three tolerance rules rejected across three revisions + for the same shape of error on the same data structure. +- Recovery from a clean checkout: + + ```sh + git fetch githubsucks + git worktree add ../pmacs-reap-ledger \ + -b reap-ledger-silent-failures \ + githubsucks/reap-ledger-silent-failures + ``` + ## Folding lane (Arc 6) — Stages 1 and 2 MERGED; Stage 3 (GPU) is next Both shipped stages are on `main`; nothing in this arc is in flight. Stage 3 diff --git a/docs/reap-ledger-silent-failures-framing.md b/docs/reap-ledger-silent-failures-framing.md new file mode 100644 index 0000000..e92920e --- /dev/null +++ b/docs/reap-ledger-silent-failures-framing.md @@ -0,0 +1,273 @@ +# Framing — the reap ledger fails silently in three places + +**Revision 1.** Status: awaiting review round 1. Proposed lane: +`reap-ledger-silent-failures`, worktree `../pmacs-reap-ledger`, based on +`githubsucks/main` @ `22df6ab` (a reading; re-measure at branch time). + +**Parked by PR #200's framing §5 and unparked by its evidence.** #200 +retired the premise that justified the ledger's leniency; it deliberately +changed no disposition, and said so. This lane owns what it refused. + +**Diagnosis first. No disposition change is proposed in this revision.** +§2 asks whether one is warranted; §5 parks every candidate until the +lane's own evidence exists. That ordering is not caution for its own +sake — Stage A of the signal lane had three tolerance rules rejected +across three revisions, each because it concluded something about one +entity from something that was not about that entity, and this is the +same shape of problem on the same data structure. + +## 0. Coherence impact (COHERENCE §20) + +- **Journey step 8, "Open a terminal"**, teardown half, and every + compile/grep run through `spec.group`. **No grade change proposed.** +- **Serves §9 (worker model), failure attribution.** The ledger is the + one mechanism that can see a survivor nothing else can, and today it + cannot report that it failed to. +- **Interaction islands: none. Config registry: not adopted. + Background-work attribution: unchanged.** +- **No audited claim in COHERENCE.md changes**; under §25 no COHERENCE + edit rides this PR. + + +## 1. Ground truth (verified at `22df6ab`) + +### 1.1 What the ledger is for + +`tick_reap_ledger` (`src/process.rs:1472`) exists for one case its own +doc names: **a TERM-ignoring descendant that survived its leader's clean +exit with its output redirected.** Neither leader state nor reader state +can see that survivor — `try_wait` reports the leader, and the readers +see a closed pipe. Only group liveness can (Q#CM3, round-3 finding 1). + +That is the blast radius. A silent failure here leaks *precisely* the +process the mechanism exists to catch, and nothing else in the +supervisor is looking. + +### 1.2 Three silent failures, not two + +```rust +// (a) any probe error drops the entry +if nix::sys::signal::kill(Pid::from_raw(-*pgid), None).is_err() { + return false; +} +// (b) the escalating SIGKILL's result is discarded, and the entry is +// marked killed regardless +if now >= entry.deadline && !entry.killed { + let _ = nix::sys::signal::kill(Pid::from_raw(-*pgid), Some(Signal::SIGKILL)); + entry.killed = true; +} +``` + +**(a)** `retain` returning `false` deletes the entry, so escalation is +cancelled. The comment is honest that this is a *bounded-growth policy* +rather than a claim the group is gone — #200 corrected it — but the +behaviour is unchanged: an `EPERM` probe is indistinguishable from +`ESRCH`. + +**(b)** A failed `SIGKILL` is recorded as a successful one. The entry +then satisfies `!entry.killed == false` forever and is never retried. + +**(c) `shutdown()` has the same discard** (`:1763-1766`), on the path +that exists specifically to stop a leak at editor exit: + +```rust +for (pgid, entry) in &mut self.reap_ledger { + let _ = nix::sys::signal::kill(Pid::from_raw(-*pgid), Some(Signal::SIGKILL)); + entry.killed = true; +} +``` + +Its own comment says it is there because "a pre-deadline ledger ... would +be silently discarded at Drop and leak the member". The fix for one +silent leak was written with a discarded result of its own. + +### 1.3 The shutdown loop's exit condition depends on the silent drop + +`shutdown()`'s final loop (`:1773-1775`) runs while +`self.any_running() || !self.reap_ledger.is_empty()`, bounded at 2s, and +calls `tick()` — which calls `tick_reap_ledger`. + +So the loop terminates when the ledger empties, and **the ledger empties +via (a)**. On `ESRCH` that is correct: the group is gone. On any other +errno the loop exits *early*, having concluded cleanup finished because +the probe failed. The mechanism added to prevent a leak at exit can be +ended by the same error that hides one. + +This coupling is the reason (a) cannot be changed casually: making the +probe strict without touching the loop converts a silent early exit into +a guaranteed 2-second stall at every editor exit that hits it. + +### 1.4 None of the three has been observed + +**#200 observed an explicit `SIGTERM` failing in `signal()`, not any +ledger call.** What it established is narrower and still sufficient to +open this lane: a group-directed `kill` computed from the spawn-time +`pgid == pid` assumption returned `EPERM` while the leader was alive, on +macOS, intermittently +([run 30553376486](https://github.com/levineuwirth/pmacs/actions/runs/30553376486/job/90907461258)). + +That retires "EPERM cannot happen for our own children" as a *reason* to +discard an arbitrary group error. It does **not** show that any of (a), +(b) or (c) has fired. This lane must not claim otherwise, and §3's bets +are built to find out rather than to assume. + +### 1.5 The ledger's kills have no injection seam + +`signal()` consults `forced_kill_errno` (Q#PD4), which is how #176 and +#200 tested failure paths without provoking real errnos. +**`tick_reap_ledger` calls `nix` directly and consults nothing.** Same +for `shutdown()`'s loop. + +So all three failure paths are, today, **untestable**. That is the +first thing this lane has to fix, and it is a prerequisite for any +disposition change rather than a nicety: a disposition change whose +failure path cannot be exercised is a rule nobody can falsify. + +### 1.6 What is tested today + +`liveness_probe_reaps_term_ignoring_survivor_after_leader_exit` +(`:4274`), `repeated_terminate_does_not_extend_ledger_deadline` +(`:4314`), `shutdown_force_kills_outstanding_ledger_groups` (`:4350`), +`leader_exit_reap_bounds_drain_with_pipe_holding_descendant` (`:4415`), +and `setsid_escapee_is_not_reaped_and_teardown_reclaims_readers` +(`:4450`). + +Every one exercises the **success** path. None injects a probe or +`SIGKILL` failure, because §1.5 makes that impossible. + +*`repeated_terminate_does_not_extend_ledger_deadline` is also the test +that flaked on macOS in #191 — the occurrence that produced §1.4's +evidence. It is load-sensitive and unrelated to what this lane changes; +noted so a red run on it is not mistaken for this lane's doing.* + +### 1.7 Limits of the evidence + +- **Not reproduced.** No ledger probe or `SIGKILL` has been seen to + fail, on any platform. +- **The mechanism is not established.** Why a group-directed `kill` can + return `EPERM` against a live owned child is still unknown — #200 + narrowed it and explicitly did not solve it. +- **Group identity remains unprovable** (#200 §1.5). Nothing this lane + measures can distinguish the original group from a recycled one; a + `pidfd` covers a process, not a group, and macOS has neither. + + +## 2. Questions + +- **Q#RL1** — Should the probe distinguish `ESRCH` from other errnos? + *Unknown, and deliberately not proposed yet. It is the obvious change + and it directly trades bounded growth (the stated original reason) for + correctness, while §1.3 shows it also changes editor-exit timing. It + needs evidence and its own review.* +- **Q#RL2** — Should a failed escalation be retried, and how many times? + *Unknown. `killed = true` on a failed `SIGKILL` is clearly wrong as + bookkeeping; what should replace it is a policy question, not an + obvious fix.* +- **Q#RL3** — How does a background tick report anything? *The ledger + emits no events and has no status channel. `pmacs.error` is defined + only by a test stub and is dead at 15 call sites, so it is not the + answer. This is the lane's real design question.* +- **Q#RL4** — Does the `shutdown()` loop need its own termination + condition if (a) becomes strict? *Almost certainly (§1.3), and that + coupling is why the two cannot be changed independently.* +- **Q#RL5** — Is the ledger reachable for PTY children? **No** — + `spec.group` is rejected at spawn for PTY mode, so the ledger is a + pipe-path mechanism only. Stated because #200 revision 2 got this + exact relationship wrong in the opposite direction. + + +## 3. Bets + +- **Bet 1 — the three failure paths can be made injectable without + changing behaviour.** A seam on the same terms as `forced_kill_errno`: + it injects the *result* only, leaving the probe target, the deadline + arithmetic, the `retain` decision, and the real ledger state as + production code. + - *Falsified if* injecting cannot reach all three sites — (a) and (b) + are in one closure, (c) is in `shutdown()` — without restructuring + the code under test, which would make the test a test of the + restructuring. + - **This bet ships alone and first.** It is worth landing even if + every later bet is abandoned, because §1.6's coverage is + success-only and will stay that way otherwise. + +- **Bet 2 — each silent failure is demonstrable once injectable.** With + the seam: an `EPERM` probe drops an entry whose group is still alive; + a failed `SIGKILL` leaves `killed = true` and is never retried; and + `shutdown()`'s discard does the same at exit. + - *Falsified if* any of the three turns out to be unreachable in + practice — for instance if some earlier guard makes the entry + already absent. **That would be a genuinely good outcome** and would + shrink this lane rather than embarrass it. + +- **Bet 3 — the shutdown coupling is real and measurable.** A test shows + the final loop exiting early when the probe errors, rather than + running to its 2s bound. + - *Falsified if* the loop's other condition (`any_running()`) holds it + anyway, in which case §1.3 overstates the coupling and the two + changes can be separated after all. + +- **Bet 4 — a failure here is reportable at all.** Q#RL3 has no answer + yet. This bet is a scouting obligation, not a design: find every + channel a background tick could use, and say plainly if none exists. + - *Falsified if* the only available channel is one already known dead + (`pmacs.error`) — in which case reporting becomes its own lane and + this one ships instrumentation plus tests without it. + + +## 4. Acceptance + +1. An injection seam covering all three sites, on Q#PD4's terms: result + only, everything else production code. +2. A test per silent failure, each asserting the observable consequence + (entry dropped while the group lives; `killed` set after a failed + kill; the same at shutdown) rather than that a function was called. +3. Each new test falsified by an actual revert, both directions + recorded in the PR body. +4. The `shutdown()` coupling of §1.3 pinned by a test, whichever way + Bet 3 resolves. +5. **No disposition change.** `retain` still drops on any error; + `killed` is still set unconditionally. This lane makes the failures + visible and testable; changing them is §5's. +6. Q#RL3 answered in the PR body with the channels actually found, or + an explicit statement that none exists. +7. `docs/agent-handoff.md` records the three silent paths and that none + has been observed — the distinction #200 had to make twice. + +## 5. Parked + +- **Q#RL1's strict-`ESRCH` probe**, **Q#RL2's retry policy**, and any + other disposition change. Each needs this lane's evidence. +- **Reporting**, if Bet 4 finds no channel. +- **Retargeting to the measured pgid**, and any EPERM/ESRCH tolerance + rule in `signal()` — still parked from #200, unchanged. +- **`signal_target`'s read-then-kill of `tcgetpgrp`** on the PTY path — + Stage A's "most likely real fix site", still unframed, still not this. +- **Why a group-directed `kill` can return `EPERM` against a live owned + child.** The mechanism is unknown; this lane instruments the + consequences, not the cause. + +## 6. Gates + +Standard suite, each its own step with a real exit status and nothing +after the command that could mask it: `cargo fmt --check`; `cargo clippy +--workspace --all-targets -- -D warnings`; `cargo test --lib`; `cargo +test --lib --features crdt`; `compile_mode_acceptance`; +`terminal_copy_mode_acceptance` (both feature configurations); `cargo +test --test m4_acceptance -- --skip basedpyright`; +`PMACS_REQUIRE_GPU=1 cargo test -p pmacs-gpu`; `git diff --check`. + +**All five bootstrap-storage variables controlled locally** — +`XDG_CONFIG_HOME`, `XDG_DATA_HOME`, `XDG_STATE_HOME`, `XDG_CACHE_HOME`, +`PMACS_STATE_HOME` — per the ambient-root framing merged as #201. +Isolating only the config root leaves the data-root write path open. + +**Process-supervisor tests are load-sensitive**; the PR body records +repetition counts rather than a single green. + +## 7. Branch plan + +One branch, one PR. **Bet 1 first and alone**: the seam is the +prerequisite for everything else and is worth landing on its own. If +Bet 2 then finds a path unreachable, the lane shrinks and says so rather +than manufacturing a failure to justify itself. From b881bd4c4e7ae90ab39cd6a8e48c8c80e805269d Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 30 Jul 2026 16:20:09 -0400 Subject: [PATCH 2/8] =?UTF-8?q?docs(process):=20framing=20revision=202=20?= =?UTF-8?q?=E2=80=94=20close=20review=20round=201?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three blocking and two major findings, all accepted, all verified in the code first. - **§0 named the wrong journey step**, and contradicted Q#RL5 in the same document. The ledger arms only for `spec.group`, which spawn rejects for PTY mode, so no terminal reaches it. The only production `group = true` caller is compile mode, making this journey step 9 (build/test) plus general pipe-process cleanup. - **A fourth site was missed.** `final_drain_runtime` collapses every probe errno to "dead" while no tick runs, discards its SIGKILL result, and sets its own flag. It is not identical to the persistent ledger — a later tick can retry that, this decision is terminal for the drain — and its consequence differs: a false "dead" cancels the readers, so the failure mode is truncated output rather than a leaked process. It also constrains the seam, being a free function with no `&mut self`. - **The staging contradicted itself**: "Bet 1 ships alone" against an acceptance requiring failure-path tests, under one-branch/one-PR. It was also wrong on its own terms — a seam with no tests does not show it reaches the intended production calls. The first PR is now seam plus behaviour-preserving tests, still diagnosis-only. - **A generic seam is the wrong shape.** `shutdown()` calls `self.signal(*id, SIGKILL)` before its ledger force-kill, so one shared "next kill errno" would be consumed by the wrong call and the test would pass while proving nothing. The seam is now site-directed with four independently addressable outcomes, expressing more than one pending result because the coupling test needs two at once, and with fixture cleanup stated rather than assumed. - **§1.3 overstated the loop coupling.** Early exit needs the ledger empty AND `any_running()` already false. The precondition is now in the ground truth and in Bet 3's fixture requirement. Framing only. No code. --- docs/reap-ledger-silent-failures-framing.md | 162 ++++++++++++++++---- 1 file changed, 132 insertions(+), 30 deletions(-) diff --git a/docs/reap-ledger-silent-failures-framing.md b/docs/reap-ledger-silent-failures-framing.md index e92920e..ff85f5b 100644 --- a/docs/reap-ledger-silent-failures-framing.md +++ b/docs/reap-ledger-silent-failures-framing.md @@ -1,6 +1,6 @@ -# Framing — the reap ledger fails silently in three places +# Framing — group cleanup fails silently at four sites -**Revision 1.** Status: awaiting review round 1. Proposed lane: +**Revision 2.** Status: awaiting review round 2. Proposed lane: `reap-ledger-silent-failures`, worktree `../pmacs-reap-ledger`, based on `githubsucks/main` @ `22df6ab` (a reading; re-measure at branch time). @@ -8,6 +8,34 @@ retired the premise that justified the ledger's leniency; it deliberately changed no disposition, and said so. This lane owns what it refused. +## Revision history + +**Revision 1 → 2**, after review round 1 (three blocking, two major). +All five accepted; all five verified in the code first. + +- **§0 named the wrong journey step.** It claimed step 8, "Open a + terminal", while Q#RL5 in the same document says the ledger is + unreachable for PTY children. Both cannot be true. The only production + `group = true` caller is compile mode + (`builtin/runtime/compile.lua:820`), so this is **step 9, build/test**, + plus general pipe-process cleanup. +- **A fourth site was missed.** `final_drain_runtime` (`:2331`) probes + `kill(-pgid, None)`, treats every error as dead, discards its `SIGKILL` + result and sets its own `group_killed` flag — and it enforces the + ledger deadline *while no tick runs*. It is now in scope (§1.2a). +- **The staging contradicted itself.** "Bet 1 ships alone" against an + acceptance requiring three failure-path tests, under one-branch / + one-PR. A seam with no tests does not even prove it reaches the + production calls. §7 now scopes the first PR as seam **plus** + behaviour-preserving tests, which is still diagnosis-only. +- **A generic seam is the wrong shape.** `shutdown()` calls + `self.signal(*id, SIGKILL)` *before* its ledger force-kill, so a single + "next kill errno" would be eaten by the wrong call. The seam is now + site-directed (§3 Bet 1). +- **§1.3 overstated the loop coupling.** Early exit needs the ledger + empty **and** `any_running()` already false; a live managed record + keeps the loop going regardless. The precondition is now stated. + **Diagnosis first. No disposition change is proposed in this revision.** §2 asks whether one is warranted; §5 parks every candidate until the lane's own evidence exists. That ordering is not caution for its own @@ -18,8 +46,12 @@ same shape of problem on the same data structure. ## 0. Coherence impact (COHERENCE §20) -- **Journey step 8, "Open a terminal"**, teardown half, and every - compile/grep run through `spec.group`. **No grade change proposed.** +- **Journey step 9, "Build and test"** — every compile and grep run, + plus general pipe-process cleanup. **Not step 8:** the ledger arms + only for `proc.spec.group`, which spawn *rejects* for PTY mode, so no + terminal ever reaches it (Q#RL5). The only production `group = true` + caller is compile mode (`builtin/runtime/compile.lua:820`). + **No grade change proposed.** - **Serves §9 (worker model), failure attribution.** The ledger is the one mechanism that can see a survivor nothing else can, and today it cannot report that it failed to. @@ -43,7 +75,7 @@ That is the blast radius. A silent failure here leaks *precisely* the process the mechanism exists to catch, and nothing else in the supervisor is looking. -### 1.2 Three silent failures, not two +### 1.2 Three silent failures in the persistent ledger ```rust // (a) any probe error drops the entry @@ -81,6 +113,36 @@ Its own comment says it is there because "a pre-deadline ledger ... would be silently discarded at Drop and leak the member". The fix for one silent leak was written with a discarded result of its own. +### 1.2a A fourth site: the in-drain twin + +`final_drain_runtime` (`:2331`, called once from `:1573`) runs the same +pattern on the same pgid, while **no tick is running**: + +```rust +let group_alive = nix::sys::signal::kill(Pid::from_raw(-ctx.pgid), None).is_ok(); +if group_alive && now >= ctx.deadline && !group_killed { + let _ = nix::sys::signal::kill(Pid::from_raw(-ctx.pgid), Some(Signal::SIGKILL)); + group_killed = true; +} +``` + +`is_ok()` collapses every errno into "dead", exactly as the persistent +ledger's `is_err()` does — and the consequence differs. A false "dead" +here makes `quiesced` true, which sets `rt.cancel` and **cancels the +readers**, so the failure mode is truncated output rather than a leaked +process. + +**It is not identical to the persistent ledger and the framing does not +claim it is.** A later `tick` can retry the ledger entry; this decision +is terminal for that drain. It is in scope because it is the same +collapse on the same data with its own consequence, not because it is +the same bug. + +**It constrains the seam.** `final_drain_runtime` is a free function +taking `&RuntimeHandles` and `Option` — there is no +`&mut self` to hang a supervisor field on, so the injected outcome has +to arrive through `GroupDrainCtx`, populated at the `:1573` call site. + ### 1.3 The shutdown loop's exit condition depends on the silent drop `shutdown()`'s final loop (`:1773-1775`) runs while @@ -90,8 +152,15 @@ calls `tick()` — which calls `tick_reap_ledger`. So the loop terminates when the ledger empties, and **the ledger empties via (a)**. On `ESRCH` that is correct: the group is gone. On any other errno the loop exits *early*, having concluded cleanup finished because -the probe failed. The mechanism added to prevent a leak at exit can be -ended by the same error that hides one. +the probe failed. + +**The precondition matters and revision 1 omitted it.** The condition is +a disjunction: `any_running() || !reap_ledger.is_empty()`. An early exit +therefore needs the ledger empty **and** `any_running()` already false — +a live managed record keeps the loop running whatever the ledger does. +The coupling is real for the case the ledger exists to serve, the +leader-exited survivor, and Bet 3 must build exactly that fixture rather +than any group. This coupling is the reason (a) cannot be changed casually: making the probe strict without touching the loop converts a silent early exit into @@ -178,18 +247,35 @@ noted so a red run on it is not mistaken for this lane's doing.* ## 3. Bets -- **Bet 1 — the three failure paths can be made injectable without - changing behaviour.** A seam on the same terms as `forced_kill_errno`: - it injects the *result* only, leaving the probe target, the deadline - arithmetic, the `retain` decision, and the real ledger state as - production code. - - *Falsified if* injecting cannot reach all three sites — (a) and (b) - are in one closure, (c) is in `shutdown()` — without restructuring - the code under test, which would make the test a test of the - restructuring. - - **This bet ships alone and first.** It is worth landing even if - every later bet is abandoned, because §1.6's coverage is - success-only and will stay that way otherwise. +- **Bet 1 — the four sites can be made injectable, site by site, + without changing behaviour.** Not a generic "next kill errno": + `shutdown()` calls `self.signal(*id, SIGKILL)` *before* its ledger + force-kill, so a single shared one-shot would be consumed by the wrong + call and the test would pass while proving nothing. + + The seam is **directed and one-shot per site**, with four independently + addressable outcomes: + + | Site | What is injected | + |---|---| + | `tick_reap_ledger` probe | the `kill(-pgid, None)` result | + | `tick_reap_ledger` escalation | the `SIGKILL` result | + | `shutdown()` force-kill | the `SIGKILL` result | + | `final_drain_runtime` probe / kill | both, via `GroupDrainCtx` (§1.2a) | + + Bet 3's coupling test needs **two at once** — a failed shutdown + force-kill *and* a failed subsequent probe — so the seam must express + more than one pending outcome. A typed queue per site, or a per-site + slot, either is acceptable; a single global slot is not. + + **Fixture cleanup is part of the seam, not an afterthought.** An + uninjected outcome left armed leaks into the next test in the same + binary, and these tests run single-threaded in CI. Each seam is + consumed on use and asserted empty at fixture teardown. + - *Falsified if* a site cannot take a directed outcome without + restructuring the code under test — which would make the test a test + of the restructuring. `final_drain_runtime` is the one at risk, + being a free function. - **Bet 2 — each silent failure is demonstrable once injectable.** With the seam: an `EPERM` probe drops an entry whose group is still alive; @@ -203,9 +289,13 @@ noted so a red run on it is not mistaken for this lane's doing.* - **Bet 3 — the shutdown coupling is real and measurable.** A test shows the final loop exiting early when the probe errors, rather than running to its 2s bound. - - *Falsified if* the loop's other condition (`any_running()`) holds it - anyway, in which case §1.3 overstates the coupling and the two - changes can be separated after all. + - **The fixture must have `any_running()` already false** (§1.3): a + leader that has exited leaving a group survivor. Any other shape + tests the disjunction's other arm and proves nothing about the + coupling. + - *Falsified if* the loop still runs to its bound with the ledger + emptied and no managed record live — in which case §1.3 overstates + the coupling and the two changes can be separated after all. - **Bet 4 — a failure here is reportable at all.** Q#RL3 has no answer yet. This bet is a scouting obligation, not a design: find every @@ -217,11 +307,13 @@ noted so a red run on it is not mistaken for this lane's doing.* ## 4. Acceptance -1. An injection seam covering all three sites, on Q#PD4's terms: result - only, everything else production code. +1. A **directed, multi-outcome** injection seam covering all four sites + of §3 Bet 1, on Q#PD4's terms: result only, everything else + production code. Consumed on use, asserted empty at teardown. 2. A test per silent failure, each asserting the observable consequence - (entry dropped while the group lives; `killed` set after a failed - kill; the same at shutdown) rather than that a function was called. + — entry dropped while the group lives; `killed` set after a failed + kill; the same at shutdown; readers cancelled after a false "dead" in + the drain — rather than that a function was called. 3. Each new test falsified by an actual revert, both directions recorded in the PR body. 4. The `shutdown()` coupling of §1.3 pinned by a test, whichever way @@ -267,7 +359,17 @@ repetition counts rather than a single green. ## 7. Branch plan -One branch, one PR. **Bet 1 first and alone**: the seam is the -prerequisite for everything else and is worth landing on its own. If -Bet 2 then finds a path unreachable, the lane shrinks and says so rather -than manufacturing a failure to justify itself. +One branch, one PR: **the seam together with the tests that exercise +it.** Revision 1 said "Bet 1 ships alone", which contradicted an +acceptance requiring failure-path tests under one-branch/one-PR — and +was wrong on its own terms, because a seam with no tests does not even +demonstrate that it reaches the intended production calls. + +This is still diagnosis-only: every test pins **current** behaviour, +including the behaviour that is wrong. Nothing in this PR changes what +the supervisor does. + +If Bet 2 finds a path unreachable, the lane shrinks and says so rather +than manufacturing a failure to justify itself. If Q#RL3 finds no +reporting channel, reporting becomes its own lane and this PR ships +instrumentation plus tests without it. From b27b377f67521909c3a01186e5ea2794036b57bc Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 30 Jul 2026 16:22:43 -0400 Subject: [PATCH 3/8] docs(active-work): synchronize the reap-ledger lane with revision 2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The revision-2 commit shipped the framing alone. Its ledger edit was generated by a script that aborted on a line-wrap mismatch before writing, so `b881bd4` left the lane entry describing revision 1 — three silent failures instead of four, no in-drain twin, no site-directed seam, and the withdrawn "Bet 1 ships alone" staging. Synchronized now: four sites including `final_drain_runtime` and its distinct reader-cancellation consequence; journey step 9 rather than 8; the `any_running()` precondition on the shutdown coupling; and the first PR scoped as seam plus tests. The failure mode is worth naming, because a partial write is exactly the shape that leaves a document confidently wrong: the commit succeeded and reported one file changed, which is easy to read as success when two were intended. --- docs/active-work.md | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index 0399359..52cb239 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -333,29 +333,38 @@ compatible. githubsucks/test-ambient-config-isolation ``` -## Reap-ledger silent failures — FRAMING OPEN, revision 1 +## Reap-ledger silent failures — FRAMING OPEN, revision 2 - **Branch `reap-ledger-silent-failures`**, worktree `../pmacs-reap-ledger`, based on `githubsucks/main` @ `22df6ab`. **Framing only; no code, no PR yet.** - `docs/reap-ledger-silent-failures-framing.md`, revision 1. + `docs/reap-ledger-silent-failures-framing.md`, revision 2; one review + round closed (three blocking, two major, all accepted). - **Unparked from PR #200's §5.** #200 retired the premise that justified the ledger's leniency and deliberately changed no disposition; this lane owns what it refused. -- **Three silent failures, not the two #200 named.** A probe error of - any errno drops the entry and cancels escalation; a failed escalating +- **Four sites, not the two #200 named.** In the persistent ledger: a + probe error of any errno drops the entry and cancels escalation; a failed escalating `SIGKILL` is marked as succeeded and never retried; and `shutdown()` discards its own force-kill result the same way — on the path that - exists specifically to stop a leak at editor exit. + exists specifically to stop a leak at editor exit. **Plus the in-drain + twin** `final_drain_runtime`, which collapses every errno to "dead" + while no tick runs; a false "dead" there cancels the readers, so its + failure mode is truncated output rather than a leaked process. - **The blast radius is exactly what the ledger exists for:** a TERM-ignoring descendant that outlived its leader with output redirected. Neither leader state nor reader state can see it; only group liveness can. A silent drop leaks the one process nothing else - is watching. + is watching. **Journey step 9 (build/test), not step 8** — the ledger + arms only for `spec.group`, which spawn rejects for PTY mode, so no + terminal reaches it; compile mode is the only production caller. - **`shutdown()`'s final loop terminates when the ledger empties**, which happens via the same silent drop — so the probe error that hides a leak can also end the cleanup loop early. That coupling is why the - probe cannot be made strict on its own. + probe cannot be made strict on its own. **Its precondition is + `any_running()` already false**, so the fixture must be a leader that + exited leaving a survivor; any other shape tests the other arm of the + disjunction. - **None of the three has been observed.** #200 saw an explicit `SIGTERM` fail in `signal()`, not a ledger call. The premise is falsified and the path exposed; the occurrence is not evidence these @@ -363,8 +372,12 @@ compatible. - **They are also untestable today**: `tick_reap_ledger` and `shutdown()` call `nix` directly and consult no injection seam, unlike `signal()`'s `forced_kill_errno`. All five existing ledger tests - exercise the success path only. **Bet 1 ships the seam alone and - first** and is worth landing even if the rest is abandoned. + exercise the success path only. The seam must be **site-directed and + multi-outcome**: `shutdown()` calls `self.signal()` before its ledger + force-kill, so a single global slot would be consumed by the wrong + call, and the coupling test needs two pending outcomes at once. The + first PR is the seam **plus** the tests that exercise it — a seam + without tests does not show it reaches the intended calls. - **Diagnosis first, no disposition change proposed.** Stage A of the signal lane had three tolerance rules rejected across three revisions for the same shape of error on the same data structure. From a9a1484ff7544b4d3d0c70fe29bd5b754039def0 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 30 Jul 2026 16:41:10 -0400 Subject: [PATCH 4/8] docs(process): make reap-ledger seam falsifiable Refine the framing's in-drain test seam: a one-shot probe error cannot survive the 50 ms quiescence interval, so the test-only override lasts for one drain context and remains per-supervisor. Name the in-drain SIGKILL as a non-observable local fact rather than adding an untestable seam, and synchronize the revision-three ledger record. --- docs/active-work.md | 18 +++- docs/reap-ledger-silent-failures-framing.md | 103 +++++++++++++++----- 2 files changed, 91 insertions(+), 30 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index 52cb239..0f8b195 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -333,13 +333,14 @@ compatible. githubsucks/test-ambient-config-isolation ``` -## Reap-ledger silent failures — FRAMING OPEN, revision 2 +## Reap-ledger silent failures — FRAMING OPEN, revision 3 - **Branch `reap-ledger-silent-failures`**, worktree `../pmacs-reap-ledger`, based on `githubsucks/main` @ `22df6ab`. **Framing only; no code, no PR yet.** - `docs/reap-ledger-silent-failures-framing.md`, revision 2; one review - round closed (three blocking, two major, all accepted). + `docs/reap-ledger-silent-failures-framing.md`, revision 3; two review + rounds closed (round 1: three blocking, two major; round 2: two + blocking, two major; all accepted). - **Unparked from PR #200's §5.** #200 retired the premise that justified the ledger's leniency and deliberately changed no disposition; this lane owns what it refused. @@ -376,8 +377,15 @@ compatible. multi-outcome**: `shutdown()` calls `self.signal()` before its ledger force-kill, so a single global slot would be consumed by the wrong call, and the coupling test needs two pending outcomes at once. The - first PR is the seam **plus** the tests that exercise it — a seam - without tests does not show it reaches the intended calls. + in-drain probe repeats every 1 ms but only cancels readers after 50 ms + of false "dead", so it needs a directed full-drain override rather + than a one-shot error. Test state is per-supervisor and shared into + the drain context, never global; teardown proves its intended site was + reached. The in-drain SIGKILL's local flag has no independently + observable outer-path consequence, so it is named but not given a + dead injection seam. The first PR is the seam **plus** the tests that + exercise it — a seam without tests does not show it reaches the + intended calls. - **Diagnosis first, no disposition change proposed.** Stage A of the signal lane had three tolerance rules rejected across three revisions for the same shape of error on the same data structure. diff --git a/docs/reap-ledger-silent-failures-framing.md b/docs/reap-ledger-silent-failures-framing.md index ff85f5b..299c356 100644 --- a/docs/reap-ledger-silent-failures-framing.md +++ b/docs/reap-ledger-silent-failures-framing.md @@ -1,6 +1,6 @@ # Framing — group cleanup fails silently at four sites -**Revision 2.** Status: awaiting review round 2. Proposed lane: +**Revision 3.** Status: awaiting review round 3. Proposed lane: `reap-ledger-silent-failures`, worktree `../pmacs-reap-ledger`, based on `githubsucks/main` @ `22df6ab` (a reading; re-measure at branch time). @@ -10,6 +10,30 @@ changed no disposition, and said so. This lane owns what it refused. ## Revision history +**Revision 2 → 3**, after review round 2 (two blocking, two major). +All four accepted; all four verified in the code first. + +- **The new in-drain test could not fire under the specified seam.** The + drain initializes `last_data` on entry and probes again after each 1 ms + sleep; `quiesced` needs a false result for the full 50 ms + `READER_SEND_POLL_INTERVAL`. A one-shot error is therefore gone before + reader cancellation. The in-drain override now returns its selected + result for that one `GroupDrainCtx`, and the acceptance requires named + late output to prove the cancellation through `poll_one`. +- **The in-drain SIGKILL had no outer-path discriminator.** Its local + `group_killed` flag is followed by the persistent ledger's retry in the + same outer tick; testing it would require a call-count assertion or a + direct free-function test. It remains an explicit code fact but is + outside this diagnostic PR's injection seam. +- **The seam needed a lifetime owner.** A context-local queue cannot be + asserted at fixture teardown and a global one crosses test fixtures. + The test state is now specified as per-supervisor and shared into the + production `GroupDrainCtx`; an unconsumed outcome proves the intended + site was never reached. +- **The four-site scope had stale "three" language.** Acceptance and + handoff obligations now distinguish the three persistent-ledger paths + from the in-drain probe twin. + **Revision 1 → 2**, after review round 1 (three blocking, two major). All five accepted; all five verified in the code first. @@ -127,10 +151,12 @@ if group_alive && now >= ctx.deadline && !group_killed { ``` `is_ok()` collapses every errno into "dead", exactly as the persistent -ledger's `is_err()` does — and the consequence differs. A false "dead" -here makes `quiesced` true, which sets `rt.cancel` and **cancels the -readers**, so the failure mode is truncated output rather than a leaked -process. +ledger's `is_err()` does — and the consequence differs. Once that false +"dead" persists for one quiescent `READER_SEND_POLL_INTERVAL`, it makes +`quiesced` true, which sets `rt.cancel` and **cancels the readers**, so +the failure mode is truncated output rather than a leaked process. One +false probe is not enough: the loop probes again every millisecond, and +`last_data` starts at the drain's entry. **It is not identical to the persistent ledger and the framing does not claim it is.** A later `tick` can retry the ledger entry; this decision @@ -138,10 +164,23 @@ is terminal for that drain. It is in scope because it is the same collapse on the same data with its own consequence, not because it is the same bug. +The ignored in-drain `SIGKILL` is a real code fact, but not an +independently observable acceptance in this diagnostic PR. Its +`group_killed` flag is local; when the drain returns, the same outer +`tick` reaches the persistent ledger and can retry the group. Proving +the local non-retry without a call-count assertion would mean testing +the free function directly rather than its production caller. This lane +therefore tests the probe-collapse consequence above and does **not** +promise an injection seam for the in-drain `SIGKILL`. Any later retry +policy must re-scout that local flag with the persistent ledger. + **It constrains the seam.** `final_drain_runtime` is a free function taking `&RuntimeHandles` and `Option` — there is no -`&mut self` to hang a supervisor field on, so the injected outcome has -to arrive through `GroupDrainCtx`, populated at the `:1573` call site. +`&mut self` to hang a supervisor field on. Test-only injection state +therefore remains owned by the `ProcessSupervisor` and is shared into +`GroupDrainCtx` at the `:1573` production call site. It is never global: +fixture teardown can then assert the planned outcome was consumed, even +when unit tests run in parallel. ### 1.3 The shutdown loop's exit condition depends on the silent drop @@ -253,34 +292,44 @@ noted so a red run on it is not mistaken for this lane's doing.* force-kill, so a single shared one-shot would be consumed by the wrong call and the test would pass while proving nothing. - The seam is **directed and one-shot per site**, with four independently - addressable outcomes: + The persistent-ledger and shutdown outcomes are **directed and + one-shot**. The in-drain probe instead needs a directed result for one + complete drain: a one-shot error is consumed by its next 1 ms probe and + cannot reach `quiesced`'s 50 ms interval. The test mode must therefore + return the selected error for every in-drain probe until that + `GroupDrainCtx` ends, while leaving the later persistent-ledger probe + real. + + The independently addressable outcomes are: | Site | What is injected | |---|---| | `tick_reap_ledger` probe | the `kill(-pgid, None)` result | | `tick_reap_ledger` escalation | the `SIGKILL` result | | `shutdown()` force-kill | the `SIGKILL` result | - | `final_drain_runtime` probe / kill | both, via `GroupDrainCtx` (§1.2a) | + | `final_drain_runtime` probe | one selected result for the complete drain, via `GroupDrainCtx` (§1.2a) | Bet 3's coupling test needs **two at once** — a failed shutdown force-kill *and* a failed subsequent probe — so the seam must express more than one pending outcome. A typed queue per site, or a per-site slot, either is acceptable; a single global slot is not. - **Fixture cleanup is part of the seam, not an afterthought.** An - uninjected outcome left armed leaks into the next test in the same - binary, and these tests run single-threaded in CI. Each seam is - consumed on use and asserted empty at fixture teardown. + **Fixture cleanup is part of the seam, not an afterthought.** Test + state is per-supervisor, not global. An unconsumed planned outcome + proves the fixture missed its intended production site; each finite + outcome is consumed on use and asserted empty at fixture teardown, and + the in-drain override records that it was used before its context ends. - *Falsified if* a site cannot take a directed outcome without restructuring the code under test — which would make the test a test of the restructuring. `final_drain_runtime` is the one at risk, being a free function. -- **Bet 2 — each silent failure is demonstrable once injectable.** With - the seam: an `EPERM` probe drops an entry whose group is still alive; - a failed `SIGKILL` leaves `killed = true` and is never retried; and - `shutdown()`'s discard does the same at exit. +- **Bet 2 — each silent consequence is demonstrable once injectable.** + With the seam: an `EPERM` probe drops an entry whose group is still + alive; a failed `SIGKILL` leaves `killed = true` and is never retried; + `shutdown()`'s discard does the same at exit; and a continuously false + in-drain probe cancels readers before a live descendant's deliberately + late output can arrive. - *Falsified if* any of the three turns out to be unreachable in practice — for instance if some earlier guard makes the entry already absent. **That would be a genuinely good outcome** and would @@ -307,13 +356,16 @@ noted so a red run on it is not mistaken for this lane's doing.* ## 4. Acceptance -1. A **directed, multi-outcome** injection seam covering all four sites - of §3 Bet 1, on Q#PD4's terms: result only, everything else - production code. Consumed on use, asserted empty at teardown. -2. A test per silent failure, each asserting the observable consequence +1. A **directed, multi-outcome** injection seam covering the three + persistent-ledger paths plus the in-drain probe of §3 Bet 1, on + Q#PD4's terms: result only, everything else production code. The + in-drain result lasts only for its one production `GroupDrainCtx`. + Finite outcomes are consumed on use and asserted empty at teardown. +2. A test per silent consequence, each asserting the observable consequence — entry dropped while the group lives; `killed` set after a failed kill; the same at shutdown; readers cancelled after a false "dead" in - the drain — rather than that a function was called. + the drain and the live descendant's named late output absent — rather + than that a function was called. 3. Each new test falsified by an actual revert, both directions recorded in the PR body. 4. The `shutdown()` coupling of §1.3 pinned by a test, whichever way @@ -323,8 +375,9 @@ noted so a red run on it is not mistaken for this lane's doing.* visible and testable; changing them is §5's. 6. Q#RL3 answered in the PR body with the channels actually found, or an explicit statement that none exists. -7. `docs/agent-handoff.md` records the three silent paths and that none - has been observed — the distinction #200 had to make twice. +7. `docs/agent-handoff.md` records the three persistent-ledger paths and + the in-drain probe twin, and that none has been observed — the + distinction #200 had to make twice. ## 5. Parked From c61a60f3430a9e73e23723990d647979ae7fa567 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 30 Jul 2026 18:46:05 -0400 Subject: [PATCH 5/8] feat(process): make the reap ledger's silent kill failures testable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements `docs/reap-ledger-silent-failures-framing.md` (revision 3). Diagnosis only: every pin asserts CURRENT behaviour, including the behaviour that is wrong. No disposition changes. The ledger discards the result of four `kill(2)` calls, and each discard has its own consequence: - the liveness probe cannot tell ESRCH from any other errno, so an unreachable group is deleted from the ledger exactly as a dead one is, cancelling its escalation; - the deadline escalation sets `killed = true` whether or not the SIGKILL landed, so a failed one is never retried by anything; - `shutdown()`'s force-kill does the same on the path written specifically to stop a leak at editor exit; - `final_drain_runtime`'s twin collapses every errno into "dead", which quiesces the drain and cancels the readers. None had an injection seam, so none was testable. This adds one, on Q#PD4's terms: the injection replaces the *result only*, and every branch, syscall and bookkeeping step downstream is production code. The seam is directed per site, because `shutdown()` signals every managed process before it reaches its ledger force-kill — a single undirected slot would be eaten by the wrong call and the test would pass while proving nothing. The persistent sites take a FIFO each, since the shutdown-coupling pin needs a failed force-kill and a failed subsequent probe pending at once. The in-drain site instead takes one outcome that repeats for one whole drain: a one-shot is consumed by the next 1ms probe and can never survive the 50ms window `quiesced` requires. Fixture state is per-supervisor, never global, and teardown asserts every planned outcome was consumed — an unconsumed plan means the fixture never reached the site, which would leave an absence assertion vacuous. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Lv428Fth9LRtffwJSsqH7T --- src/process.rs | 470 ++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 461 insertions(+), 9 deletions(-) diff --git a/src/process.rs b/src/process.rs index 33fdf8f..78d2e4b 100644 --- a/src/process.rs +++ b/src/process.rs @@ -48,7 +48,7 @@ //! `unsafe` block — see [`build_pty_command`] for the full //! rationale. -use std::collections::HashMap; +use std::collections::{HashMap, VecDeque}; use std::io::{Read, Write}; use std::path::PathBuf; use std::sync::atomic::{AtomicBool, AtomicU64, AtomicUsize, Ordering}; @@ -480,6 +480,17 @@ pub struct ProcessSupervisor { /// Test seam for the PTY foreground-group lookup (see /// `force_next_pty_lookup`). Always `None` outside tests. forced_pty_lookup: Option>, + /// Directed test seam for the reap ledger's **own** `kill(2)` calls + /// (see [`ReapKillFaults`]). Always empty in production: the only + /// ways to add an outcome are `#[cfg(test)]`. + /// + /// Shared behind an `Arc` because `final_drain_runtime` is a free + /// function taking `&RuntimeHandles` — there is no `&mut self` to + /// reach — so the plan travels into it through [`GroupDrainCtx`]. + /// It is owned by the supervisor rather than living in a global, so + /// fixture teardown can assert its plan was consumed even when unit + /// tests run in parallel. + reap_kill_faults: Arc>, } /// One armed group in the reap ledger. @@ -491,6 +502,104 @@ struct GroupReap { killed: bool, } +/// Which of the reap ledger's `kill(2)` calls a planned test outcome +/// belongs to. +/// +/// **The seam is directed, and that is the whole point.** `shutdown()` +/// signals every managed process through [`ProcessSupervisor::signal`] +/// *before* it reaches its ledger force-kill, so a single undirected +/// "next kill fails" slot would be eaten by the wrong call and the test +/// would report a pass while proving nothing. +#[derive(Copy, Clone, Debug, Eq, PartialEq, Hash)] +enum ReapKillSite { + /// `tick_reap_ledger`'s liveness probe, `kill(-pgid, None)`. + LedgerProbe, + /// `tick_reap_ledger`'s deadline escalation, `kill(-pgid, SIGKILL)`. + LedgerEscalation, + /// `shutdown()`'s pre-loop force-kill, `kill(-pgid, SIGKILL)`. + ShutdownForceKill, +} + +/// Planned failures for the reap ledger's own `kill(2)` calls. Empty in +/// production; populated only by `#[cfg(test)]` helpers. +/// +/// Two shapes, because the sites need different lifetimes: +/// +/// * The three persistent-ledger paths take a **FIFO of one-shot** +/// outcomes each, since one tick makes one call per site. A queue +/// rather than a slot because the shutdown-coupling pin needs a failed +/// force-kill *and* a failed subsequent probe pending at once. +/// * The in-drain probe takes **one outcome that repeats for one whole +/// drain**. A one-shot cannot work there: `final_drain_runtime` probes +/// again every millisecond and `quiesced` needs a false answer to +/// persist across a full [`READER_SEND_POLL_INTERVAL`], so a one-shot +/// error is long gone before reader cancellation can be reached. +#[derive(Default)] +struct ReapKillFaults { + /// Per-site queue; each entry is consumed by one call at that site. + queued: HashMap>, + /// The errno every probe inside the one claiming `GroupDrainCtx` + /// reports, for that drain's whole lifetime. + in_drain: Option, + /// Set once a drain has claimed `in_drain`, so a second drain in the + /// same test probes for real instead of inheriting the fault. + in_drain_claimed: bool, + /// Set the first time the claiming drain actually reported the + /// fault. Teardown asserts it: an armed-but-never-reported plan + /// proves the fixture never reached the site it aimed at. + #[cfg(test)] + in_drain_used: bool, +} + +impl ReapKillFaults { + /// Consume one planned failure for `site`, if any. + fn take(&mut self, site: ReapKillSite) -> Option { + self.queued.get_mut(&site)?.pop_front() + } + + /// Claim the armed in-drain fault for one `GroupDrainCtx`. Returns + /// `false` in production, where `in_drain` is always `None`. + fn claim_in_drain(&mut self) -> bool { + if self.in_drain.is_none() || self.in_drain_claimed { + return false; + } + self.in_drain_claimed = true; + true + } + + /// The errno the claiming drain's probe should report. + fn in_drain_report(&mut self) -> Option { + let errno = self.in_drain?; + #[cfg(test)] + { + self.in_drain_used = true; + } + Some(errno) + } +} + +/// The reap ledger's own `kill(2)`, with the directed seam of +/// [`ReapKillSite`] applied first. +/// +/// On Q#PD4's terms: the injection replaces the **result only**. The +/// branch it feeds, the ledger bookkeeping, and every other syscall run +/// as production code. +fn reap_kill( + faults: &Mutex, + site: ReapKillSite, + pgid: i32, + signal: Option, +) -> nix::Result<()> { + if let Some(errno) = faults + .lock() + .expect("reap fault plan is never held across a panic") + .take(site) + { + return Err(errno); + } + nix::sys::signal::kill(Pid::from_raw(-pgid), signal) +} + struct ManagedProcess { spec: ProcessSpec, state: ProcessState, @@ -1140,6 +1249,55 @@ impl ProcessSupervisor { group_term_grace: GROUP_TERM_GRACE, forced_kill_errno: None, forced_pty_lookup: None, + reap_kill_faults: Arc::new(Mutex::new(ReapKillFaults::default())), + } + } + + /// Plan a one-shot failure for the reap ledger's `kill(2)` at + /// `site`. Queued per site (see [`ReapKillFaults`]), so a plan for + /// one site can never be consumed by another's call. + #[cfg(test)] + fn plan_reap_kill_failure(&mut self, site: ReapKillSite, errno: nix::errno::Errno) { + self.reap_kill_faults + .lock() + .expect("reap fault plan") + .queued + .entry(site) + .or_default() + .push_back(errno); + } + + /// Plan the in-drain probe failure: the *next* group drain to start + /// claims it and reports `errno` from every probe for that drain's + /// whole lifetime. See [`ReapKillFaults`] for why this one is not a + /// one-shot. + #[cfg(test)] + fn plan_in_drain_probe_failure(&mut self, errno: nix::errno::Errno) { + self.reap_kill_faults + .lock() + .expect("reap fault plan") + .in_drain = Some(errno); + } + + /// Fixture teardown: every planned outcome must have been consumed + /// by the production site it was aimed at. An unconsumed plan means + /// the fixture never reached that site, which would otherwise leave + /// a test asserting the *absence* of an effect it never provoked. + #[cfg(test)] + fn assert_reap_faults_consumed(&self) { + let faults = self.reap_kill_faults.lock().expect("reap fault plan"); + for (site, queue) in &faults.queued { + assert!( + queue.is_empty(), + "{} planned {site:?} failure(s) were never consumed — the fixture did not reach that production site", + queue.len() + ); + } + if faults.in_drain.is_some() { + assert!( + faults.in_drain_used, + "the in-drain probe fault was armed but never reported — the fixture did not reach final_drain_runtime's probe" + ); } } @@ -1471,6 +1629,8 @@ impl ProcessSupervisor { /// see that survivor; only group liveness can). fn tick_reap_ledger(&mut self) { let now = Instant::now(); + // Cloned out before `retain` takes `&mut self.reap_ledger`. + let faults = Arc::clone(&self.reap_kill_faults); self.reap_ledger.retain(|pgid, entry| { // ESRCH: no such group — done. Any other probe error is // also treated as "nothing left we can reach", so the @@ -1491,11 +1651,16 @@ impl ProcessSupervisor { // Both are known and deliberately unchanged here: the // diagnostic lane that found them does not alter // disposition. Fixing it is its own lane. - if nix::sys::signal::kill(Pid::from_raw(-*pgid), None).is_err() { + if reap_kill(&faults, ReapKillSite::LedgerProbe, *pgid, None).is_err() { return false; } if now >= entry.deadline && !entry.killed { - let _ = nix::sys::signal::kill(Pid::from_raw(-*pgid), Some(Signal::SIGKILL)); + let _ = reap_kill( + &faults, + ReapKillSite::LedgerEscalation, + *pgid, + Some(Signal::SIGKILL), + ); entry.killed = true; } true @@ -1564,13 +1729,19 @@ impl ProcessSupervisor { GroupDrainCtx { pgid, deadline: entry.deadline, + faults: Arc::clone(&self.reap_kill_faults), + in_drain_fault_claimed: self + .reap_kill_faults + .lock() + .expect("reap fault plan is never held across a panic") + .claim_in_drain(), } }) } else { None }; let now = Instant::now(); - let final_output = final_drain_runtime(runtime, group_ctx); + let final_output = final_drain_runtime(runtime, group_ctx.as_ref()); let (termination, event) = match status { Ok(Some(TermStatus::Exited(code))) => ( Termination::Exited { @@ -1760,8 +1931,14 @@ impl ProcessSupervisor { // (leader exited promptly, TERM-ignoring group member alive) // would be silently discarded at Drop and leak the member // (Q#CM3, round-4 finding 1). + let faults = Arc::clone(&self.reap_kill_faults); for (pgid, entry) in &mut self.reap_ledger { - let _ = nix::sys::signal::kill(Pid::from_raw(-*pgid), Some(Signal::SIGKILL)); + let _ = reap_kill( + &faults, + ReapKillSite::ShutdownForceKill, + *pgid, + Some(Signal::SIGKILL), + ); entry.killed = true; } // Final reap loop. SIGKILL is delivered immediately by the @@ -1805,6 +1982,15 @@ impl ProcessSupervisor { fn reap_ledger_len(&self) -> usize { self.reap_ledger.len() } + + /// Whether the ledger has recorded a SIGKILL as sent for `pgid`. + /// `None` if no entry is armed. The whole point of the + /// failed-escalation pin is that this reads `true` after a kill + /// that never happened. + #[cfg(test)] + fn reap_ledger_killed(&self, pgid: i32) -> Option { + self.reap_ledger.get(&pgid).map(|e| e.killed) + } } impl Drop for ProcessSupervisor { @@ -2322,13 +2508,40 @@ fn drain_runtime_output(rt: &RuntimeHandles) -> Vec { /// ledger's deadline for this group: the drain enforces it from /// inside its loop because no other tick runs while the drain /// blocks the frame. -#[derive(Clone, Copy)] struct GroupDrainCtx { pgid: i32, deadline: Instant, + /// The supervisor's fault plan (empty in production), carried here + /// because this drain runs in a free function with no `&mut self`. + faults: Arc>, + /// True when this drain claimed the armed in-drain probe fault at + /// construction. Exactly one drain can claim it. + in_drain_fault_claimed: bool, } -fn final_drain_runtime(rt: &RuntimeHandles, group: Option) -> Vec { +/// The in-drain liveness probe, with the seam of §1.2a applied first. +/// +/// The injected errno is *returned*, and the caller's `.is_ok()` then +/// discards it — which is the collapse under test, left exactly as it +/// is. This function changes what the probe reports, never what the +/// drain does with the report. +fn in_drain_probe(ctx: &GroupDrainCtx) -> nix::Result<()> { + if ctx.in_drain_fault_claimed + && let Some(errno) = ctx + .faults + .lock() + .expect("reap fault plan is never held across a panic") + .in_drain_report() + { + return Err(errno); + } + nix::sys::signal::kill(Pid::from_raw(-ctx.pgid), None) +} + +fn final_drain_runtime( + rt: &RuntimeHandles, + group: Option<&GroupDrainCtx>, +) -> Vec { let deadline = Instant::now() + EXIT_OUTPUT_DRAIN_TIMEOUT; let mut out = Vec::new(); // Group drains get tighter bounds than the plain byte-flush @@ -2358,9 +2571,9 @@ fn final_drain_runtime(rt: &RuntimeHandles, group: Option) -> Vec if rt.readers.iter().all(std::thread::JoinHandle::is_finished) && !drained_any { return out; } - if let Some(ctx) = &group { + if let Some(ctx) = group { let now = Instant::now(); - let group_alive = nix::sys::signal::kill(Pid::from_raw(-ctx.pgid), None).is_ok(); + let group_alive = in_drain_probe(ctx).is_ok(); if group_alive && now >= ctx.deadline && !group_killed { let _ = nix::sys::signal::kill(Pid::from_raw(-ctx.pgid), Some(Signal::SIGKILL)); group_killed = true; @@ -4374,6 +4587,245 @@ mod tests { ); } + // ----------------------------------------------------------------- + // Reap-ledger silent failures (framing §4). + // + // Every pin below asserts the *consequence* of a discarded kill(2) + // result — a live group the ledger stopped tracking, a survivor the + // ledger records as killed, output cancelled before it arrived — + // and never that a function was called. All five pin CURRENT + // behaviour, including the behaviour that is wrong: this lane + // changes no disposition (§7). + // + // Each ends with `assert_reap_faults_consumed`, which is not + // ceremony: a planned failure that was never consumed means the + // fixture never reached the production site, and an + // absence-assertion under those conditions is vacuous. + // ----------------------------------------------------------------- + + /// Kill a leaked fixture descendant that a pin deliberately let + /// survive. Called after the assertions, never before them. + fn reap_fixture_survivor(pid: i32) { + let _ = nix::sys::signal::kill(Pid::from_raw(pid), Some(Signal::SIGKILL)); + } + + #[test] + fn an_unreachable_probe_drops_an_entry_whose_group_is_still_alive() { + // §1.2 (a). `retain` returning false deletes the entry, so + // escalation is cancelled — and the probe cannot tell ESRCH + // ("the group is gone", correct) from any other errno ("we + // could not ask", not correct). + let dir = tempfile::tempdir().expect("tempdir"); + let mut sup = ProcessSupervisor::new(); + // Long enough that the escalation cannot fire and confuse the + // reading: the only thing that empties the ledger here is (a). + sup.set_group_term_grace(Duration::from_secs(30)); + let (script, pidfile) = survivor_script(dir.path(), true); + let id = sup + .spawn(sh_group_spec("probe-eperm", &script)) + .expect("spawn"); + let _ = drain_until(&mut sup, id, Duration::from_secs(5), has_exited); + let survivor = wait_pidfile(&pidfile); + assert!(pid_alive(survivor), "survivor alive before the probe"); + assert_eq!(sup.reap_ledger_len(), 1, "ledger armed before the probe"); + + sup.plan_reap_kill_failure(ReapKillSite::LedgerProbe, nix::errno::Errno::EPERM); + sup.tick(); + + assert_eq!( + sup.reap_ledger_len(), + 0, + "an EPERM probe drops the entry exactly as ESRCH would" + ); + assert!( + pid_alive(survivor), + "and the group it stopped tracking is still alive — this is the leak" + ); + sup.assert_reap_faults_consumed(); + reap_fixture_survivor(survivor); + } + + #[test] + fn a_failed_escalation_is_recorded_as_a_successful_one() { + // §1.2 (b). `entry.killed = true` runs unconditionally, so a + // SIGKILL that never landed satisfies `!entry.killed == false` + // forever. The consequence is not bookkeeping: the survivor is + // never killed again, by anything. + let dir = tempfile::tempdir().expect("tempdir"); + let mut sup = ProcessSupervisor::new(); + sup.set_group_term_grace(Duration::from_millis(150)); + let (script, pidfile) = survivor_script(dir.path(), true); + let id = sup + .spawn(sh_group_spec("kill-eperm", &script)) + .expect("spawn"); + let events = drain_until(&mut sup, id, Duration::from_secs(5), has_exited); + let pgid = i32::try_from(started_pid(&events).expect("leader pid")).expect("pgid fits"); + let survivor = wait_pidfile(&pidfile); + + sup.plan_reap_kill_failure(ReapKillSite::LedgerEscalation, nix::errno::Errno::EPERM); + // Past the grace, so the escalation arm is the one that runs. + std::thread::sleep(Duration::from_millis(250)); + sup.tick(); + + assert_eq!( + sup.reap_ledger_killed(pgid), + Some(true), + "the entry records a SIGKILL that returned EPERM as sent" + ); + // The retry that `killed = true` forecloses: keep ticking well + // past the grace and the survivor is still there. + let stop = Instant::now() + Duration::from_millis(600); + while Instant::now() < stop { + sup.tick(); + std::thread::sleep(Duration::from_millis(20)); + } + assert!( + pid_alive(survivor), + "no tick ever retries the failed SIGKILL, so the survivor outlives the ledger's only escalation" + ); + assert_eq!(sup.reap_ledger_len(), 1, "the entry is retained, and inert"); + sup.assert_reap_faults_consumed(); + reap_fixture_survivor(survivor); + } + + #[test] + fn a_failed_shutdown_force_kill_leaks_the_group_and_burns_the_bound() { + // §1.2 (c). The path that exists specifically to stop a leak at + // editor exit discards its own kill result — and because it + // still sets `killed`, the final loop can never escalate again. + // It therefore runs to its full 2s bound and exits with the + // member alive. + let dir = tempfile::tempdir().expect("tempdir"); + let mut sup = ProcessSupervisor::new(); + // The ledger must not be able to reap on its own; only + // shutdown's force-kill could have. + sup.set_group_term_grace(Duration::from_secs(30)); + let (script, pidfile) = survivor_script(dir.path(), true); + let id = sup + .spawn(sh_group_spec("shutdown-eperm", &script)) + .expect("spawn"); + let _ = drain_until(&mut sup, id, Duration::from_secs(5), has_exited); + let survivor = wait_pidfile(&pidfile); + assert!(pid_alive(survivor), "survivor alive pre-shutdown"); + + sup.plan_reap_kill_failure(ReapKillSite::ShutdownForceKill, nix::errno::Errno::EPERM); + let t0 = Instant::now(); + sup.shutdown(); + let elapsed = t0.elapsed(); + + assert!( + pid_alive(survivor), + "the group survives editor exit — the leak this path was written to prevent" + ); + assert_eq!( + sup.reap_ledger_len(), + 1, + "the entry never reaches ESRCH, so the loop holds it to the bound" + ); + assert!( + elapsed >= Duration::from_millis(1900), + "the final loop runs to its 2s bound rather than converging; took {elapsed:?}" + ); + sup.assert_reap_faults_consumed(); + reap_fixture_survivor(survivor); + } + + #[test] + fn a_probe_error_after_a_failed_force_kill_exits_the_shutdown_loop_early() { + // Bet 3, and §1.3's coupling. The loop runs while + // `any_running() || !reap_ledger.is_empty()`, so an early exit + // needs BOTH: the fixture is a leader that has already exited + // (any_running() false) leaving a group survivor, which is + // exactly the case the ledger exists to serve. + // + // This is also the pin that justifies a multi-outcome seam: + // it needs a failed force-kill AND a failed subsequent probe + // pending at the same time. + let dir = tempfile::tempdir().expect("tempdir"); + let mut sup = ProcessSupervisor::new(); + sup.set_group_term_grace(Duration::from_secs(30)); + let (script, pidfile) = survivor_script(dir.path(), true); + let id = sup + .spawn(sh_group_spec("coupling", &script)) + .expect("spawn"); + let _ = drain_until(&mut sup, id, Duration::from_secs(5), has_exited); + let survivor = wait_pidfile(&pidfile); + assert!( + !sup.any_running(), + "the leader has exited: the other arm of the disjunction is already false" + ); + + sup.plan_reap_kill_failure(ReapKillSite::ShutdownForceKill, nix::errno::Errno::EPERM); + sup.plan_reap_kill_failure(ReapKillSite::LedgerProbe, nix::errno::Errno::EPERM); + let t0 = Instant::now(); + sup.shutdown(); + let elapsed = t0.elapsed(); + + assert_eq!( + sup.reap_ledger_len(), + 0, + "the errored probe empties the ledger" + ); + assert!( + elapsed < Duration::from_millis(500), + "and the loop then exits early, concluding cleanup finished because the probe failed; took {elapsed:?}" + ); + assert!( + pid_alive(survivor), + "while the survivor it concluded about is alive" + ); + sup.assert_reap_faults_consumed(); + reap_fixture_survivor(survivor); + } + + #[test] + fn a_collapsed_in_drain_probe_cancels_readers_before_late_output() { + // §1.2a, the fourth site. `is_ok()` collapses every errno into + // "the group is dead", which makes `quiesced` true and cancels + // the readers. Unlike the persistent ledger, no later tick can + // revisit this: the decision is terminal for that drain, and + // the failure mode is truncated output rather than a leak. + // + // The fault repeats for the whole drain because a one-shot + // cannot reach `quiesced`: the loop probes again every 1ms and + // the quiescent window is a full READER_SEND_POLL_INTERVAL. + let dir = tempfile::tempdir().expect("tempdir"); + let pidfile = dir.path().join("pid"); + let mut sup = ProcessSupervisor::new(); + // Far enough out that the real path would wait, and the drain's + // own 2s EXIT_OUTPUT_DRAIN_TIMEOUT would still collect the late + // marker at ~500ms. That is what makes the absence meaningful. + sup.set_group_term_grace(Duration::from_secs(3)); + // The descendant keeps fd1, so the readers stay open after the + // leader exits. EARLY is written before the leader exits, so it + // is in the pipe before the drain begins. + let script = format!( + "echo EARLY; ( sleep 0.5; echo LATE-MARKER; sleep 5 ) & echo $! > {pid}", + pid = pidfile.display(), + ); + sup.plan_in_drain_probe_failure(nix::errno::Errno::EPERM); + let id = sup + .spawn(sh_group_spec("in-drain", &script)) + .expect("spawn"); + let events = drain_until(&mut sup, id, Duration::from_secs(5), has_exited); + let survivor = wait_pidfile(&pidfile); + + // Positive control on the fixture itself. Without it, "LATE is + // absent" would also hold if the pipe never carried anything. + assert!( + stdout_contains(&events, b"EARLY"), + "the fixture's pipe must actually deliver output; events: {events:?}" + ); + assert!( + !stdout_contains(&events, b"LATE-MARKER"), + "a probe that reports EPERM as 'dead' quiesces the drain and cancels the readers \ + before the live descendant's later output can arrive; events: {events:?}" + ); + sup.assert_reap_faults_consumed(); + reap_fixture_survivor(survivor); + sup.shutdown(); + } + #[test] fn maybe_restart_inert_once_shut_down() { let mut sup = ProcessSupervisor::new(); From 2f8a39463d2f8981218c0a686a18b0a8d424697b Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 30 Jul 2026 18:50:18 -0400 Subject: [PATCH 6/8] test(process): give the in-drain pin a fixture that can actually fail MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by bite-testing, not by review. With the in-drain seam reverted, the pin still passed both content assertions and failed only the consumed-plan check — meaning "LATE-MARKER is absent" was holding for a reason unrelated to the probe. `poll_one` sends SIGTERM to the whole group on leader exit, so the untrapped descendant died before its 0.5s sleep finished. The late marker never arrived on *either* path, which makes the absence assertion vacuous: it would have stayed green with the collapse fixed. The descendant now installs `trap '' TERM` behind `survivor_script`'s readiness gate, so it survives the group TERM and writes its marker at 0.5s — well inside the 2s drain timeout the real path would run to. Re-bitten: the reverted seam now fails on the LATE-MARKER assertion itself. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Lv428Fth9LRtffwJSsqH7T --- src/process.rs | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/src/process.rs b/src/process.rs index 78d2e4b..219cce9 100644 --- a/src/process.rs +++ b/src/process.rs @@ -4799,8 +4799,24 @@ mod tests { // The descendant keeps fd1, so the readers stay open after the // leader exits. EARLY is written before the leader exits, so it // is in the pipe before the drain begins. + // + // **`trap '' TERM` is load-bearing, and its absence made the + // first draft of this pin vacuous.** `poll_one` TERMs the whole + // group on leader exit, so an untrapped descendant dies before + // its 0.5s sleep ends — the late marker then never arrives on + // *either* path, and "LATE-MARKER is absent" holds for a reason + // that has nothing to do with the probe. The bite caught it: + // with the seam reverted the pin still passed both content + // assertions and failed only the consumed-plan check. + // + // The readiness gate is `survivor_script`'s, for its reason: a + // slow scheduler can otherwise deliver the group TERM before + // the subshell's `trap` runs. + let ready = dir.path().join("ready"); let script = format!( - "echo EARLY; ( sleep 0.5; echo LATE-MARKER; sleep 5 ) & echo $! > {pid}", + "echo EARLY; ( trap '' TERM; : > {ready}; sleep 0.5; echo LATE-MARKER; sleep 5 ) & \ + echo $! > {pid}; while [ ! -e {ready} ]; do sleep 0.01; done", + ready = ready.display(), pid = pidfile.display(), ); sup.plan_in_drain_probe_failure(nix::errno::Errno::EPERM); From 2a1cf9be2eed1860ffb1a435084907128387d3ce Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 30 Jul 2026 18:58:17 -0400 Subject: [PATCH 7/8] =?UTF-8?q?docs(process):=20framing=20revision=204=20?= =?UTF-8?q?=E2=80=94=20record=20what=20implementing=20found?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Not a new design round. Every bet resolved, and one acceptance turned out to be satisfiable vacuously. Acceptance 2's in-drain clause said "the live descendant's named late output absent" without saying how the descendant stays live. It does not, by default: `poll_one` TERMs the whole group on leader exit. The first fixture's marker was therefore absent on both paths, and the pin would have stayed green with the collapse fixed. Bets 1 and 2 hold — every site took a directed outcome without restructuring, and every consequence is reachable, so the lane does not shrink. Bet 3 resolves the shutdown coupling as real and measured. Bet 4 is falsified exactly as its own clause anticipated: `ProcessEvent` is keyed by `ProcessId` while the ledger is keyed by pgid and is deliberately independent of managed records, every production consumer polls per known id, and `take_all_events` has no production consumer at all. Q#RL3 is answered accordingly and reporting is parked as its own lane. Also records the four sites and the two reusable lessons in `docs/agent-handoff.md` — that a seam for a background loop has to be directed, and that an absence assertion is only as good as its fixture's ability to produce the thing. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Lv428Fth9LRtffwJSsqH7T --- docs/active-work.md | 28 +++++++-- docs/agent-handoff.md | 55 +++++++++++++++++ docs/reap-ledger-silent-failures-framing.md | 67 +++++++++++++++++++-- 3 files changed, 139 insertions(+), 11 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index 0f8b195..fc17960 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -333,14 +333,32 @@ compatible. githubsucks/test-ambient-config-isolation ``` -## Reap-ledger silent failures — FRAMING OPEN, revision 3 +## Reap-ledger silent failures — IMPLEMENTED, PR OPEN - **Branch `reap-ledger-silent-failures`**, worktree `../pmacs-reap-ledger`, based on `githubsucks/main` @ `22df6ab`. - **Framing only; no code, no PR yet.** - `docs/reap-ledger-silent-failures-framing.md`, revision 3; two review - rounds closed (round 1: three blocking, two major; round 2: two - blocking, two major; all accepted). + `docs/reap-ledger-silent-failures-framing.md`, **revision 4**; + approved at revision 3 after two review rounds (round 1: three + blocking, two major; round 2: two blocking, two major; all accepted). + Revision 4 records implementation findings, not a new design round. +- **All four bets resolved.** Bet 1 (every site takes a directed + outcome) and Bet 2 (every consequence is reachable) hold. **Bet 3 + resolves the shutdown coupling as real and measured** — under 500ms + with a failed force-kill plus an errored probe, versus the full 2s + bound with only the force-kill failing. **Bet 4 is falsified: no + reporting channel exists**, so reporting becomes its own lane. +- **The in-drain pin's first fixture was vacuous, and the bite caught + it.** `poll_one` TERMs the group on leader exit, so an untrapped + descendant died before writing its late marker — absent on *both* + paths. With the seam reverted the pin failed only the consumed-plan + check, never the content assertion. Fixed with `trap '' TERM` behind + the readiness gate. +- **Gates: 10/10 green** on the pushed tree, all five bootstrap-storage + variables controlled — fmt, diff-check, clippy, `--lib` (1888), + `--lib --features crdt` (2073), compile-mode (67), copy-mode in both + feature configurations (18/19), M4 with the basedpyright skip (149), + required GPU (221). The five new process pins ran **15/15** as a + repetition set, since supervisor tests are load-sensitive. - **Unparked from PR #200's §5.** #200 retired the premise that justified the ledger's leniency and deliberately changed no disposition; this lane owns what it refused. diff --git a/docs/agent-handoff.md b/docs/agent-handoff.md index b4629f5..f2af131 100644 --- a/docs/agent-handoff.md +++ b/docs/agent-handoff.md @@ -257,6 +257,61 @@ commands, read `docs/active-work.md` immediately after this file. disagree — and it still establishes no identity, because it is read inside the same read-then-act window and no portable mechanism closes that for a *group* (`pidfd` covers a process; macOS has neither). +- **Reap-ledger silent failures — DIAGNOSTIC, in flight** + (`docs/reap-ledger-silent-failures-framing.md`). The lane #200's + framing §5 parked and its evidence unparked. **Four `kill(2)` results + are discarded in the group reap ledger**, and each discard has its own + consequence — three in the persistent ledger, one in the in-drain + twin: + - `tick_reap_ledger`'s probe cannot tell `ESRCH` (the group is gone — + correct) from any other errno (we could not ask — not correct), and + `retain` deletes the entry either way, cancelling escalation. + - The deadline escalation sets `killed = true` whether or not the + `SIGKILL` landed, so a failed one is **never retried by anything**. + - `shutdown()`'s force-kill does the same, on the path written + specifically to stop a leak at editor exit. + - `final_drain_runtime`'s twin collapses every errno into "dead", + which quiesces the drain and **cancels the readers** — truncated + output rather than a leaked process, and terminal for that drain + where a later tick could revisit the ledger. + - **None of the four has been observed to fire.** #200's evidence is an + explicit `SIGTERM` failing in `signal()`, not any ledger call. What + it retires is the *reason* ("EPERM cannot happen for our own + children"), not the behaviour. + - **`shutdown()`'s loop exit depends on the silent drop, and this is + now measured.** It runs while `any_running() || !reap_ledger.is_empty()`, + so an early exit needs the ledger empty **and** no live managed + record — the leader-exited-survivor case the ledger exists to serve. + With a failed force-kill followed by an errored probe it exits in + under 500ms instead of holding its 2s bound, having concluded + cleanup finished because the probe failed. Making the probe strict + without touching the loop converts that silent early exit into a + guaranteed 2s stall at every editor exit that hits it: **the two + cannot be changed independently.** + - **There is no channel for a background tick to report on.** + `ProcessEvent` is keyed by `ProcessId`, while the ledger is keyed by + pgid and is deliberately independent of managed records, so in the + case that matters there is no id to attribute to. Every production + consumer polls `take_events(id)` per known id; `take_all_events` + would sidestep the keying but **has no production consumer at all** + (two test call sites only). `pmacs.error` is dead. Reporting is + therefore its own lane, as the framing's Bet 4 anticipated. + - **A test seam for a background loop has to be directed.** + `shutdown()` signals every managed process before it reaches its + ledger force-kill, so one undirected "next kill fails" slot is eaten + by the wrong call and the test passes while proving nothing. The + persistent sites take a FIFO each (the coupling pin needs two + outcomes pending at once); the in-drain site needs one outcome that + **repeats for a whole drain**, because a one-shot is consumed by the + next 1ms probe and can never survive the 50ms window `quiesced` + requires. + - **An absence assertion needs a fixture that could have produced the + thing.** The in-drain pin's first fixture had no `trap '' TERM`, so + `poll_one`'s leader-exit group TERM killed the descendant before it + wrote its late marker: the marker was absent on *both* paths and the + pin would have stayed green with the collapse fixed. The bite caught + it — the reverted seam failed only the consumed-plan check, not the + content assertion. That is what the consumed-plan check is for. - **Lean 4 arc (Arc 8) — stages 1, 2, 3a, 3b, 4a, 4b ALL LANDED** (`docs/lean4-mode-framing.md`; #160, #161, #167, #170, #179, #181). pmacs edits Lean 4: `arborium-lean` highlighting, a `lean4` major mode, `⟨⟩ ⦃⦄ ⟮⟯` pairs, and a `lake serve` language diff --git a/docs/reap-ledger-silent-failures-framing.md b/docs/reap-ledger-silent-failures-framing.md index 299c356..8d0dbdb 100644 --- a/docs/reap-ledger-silent-failures-framing.md +++ b/docs/reap-ledger-silent-failures-framing.md @@ -1,8 +1,9 @@ # Framing — group cleanup fails silently at four sites -**Revision 3.** Status: awaiting review round 3. Proposed lane: +**Revision 4.** Status: **APPROVED at revision 3; implemented.** Lane `reap-ledger-silent-failures`, worktree `../pmacs-reap-ledger`, based on -`githubsucks/main` @ `22df6ab` (a reading; re-measure at branch time). +`githubsucks/main` @ `22df6ab`. Revision 4 records what implementation +found; it is not a new design round. **Parked by PR #200's framing §5 and unparked by its evidence.** #200 retired the premise that justified the ledger's leniency; it deliberately @@ -10,6 +11,53 @@ changed no disposition, and said so. This lane owns what it refused. ## Revision history +**Revision 3 → 4**, found **while implementing**, not a new design +round. Every bet resolved; one acceptance turned out to be satisfiable +vacuously. + +- **Acceptance 2's in-drain clause could be met by a vacuous fixture, + and was.** "The live descendant's named late output absent" says + nothing about how the descendant stays live — and `poll_one` sends + `SIGTERM` to the whole group on leader exit, so an untrapped + descendant dies before it can write. The marker was then absent on + *both* paths and the pin would have stayed green with the collapse + fixed. The bite is what caught it: with the seam reverted, the pin + failed only the consumed-plan check, never the content assertion. The + fixture now uses `trap '' TERM` behind `survivor_script`'s readiness + gate. **The lesson generalises past this pin: an absence assertion is + only as good as the fixture's ability to produce the thing.** +- **Bet 1 holds.** All four sites took a directed outcome with no + restructuring. `final_drain_runtime` — the one §3 named as at risk, + being a free function — needed only a shared handle on the context it + already receives. +- **Bet 2 holds, in the direction that keeps the lane.** All four + consequences are reachable; none was already foreclosed by an earlier + guard. The lane does not shrink. +- **Bet 3 resolves: the coupling is real and measured.** With a failed + force-kill and an errored probe, `shutdown()` returns in **under + 500ms** instead of holding its 2s bound, with the survivor alive. With + only the failed force-kill it burns the full bound. §1.3's warning + stands: making the probe strict without touching the loop converts a + silent early exit into a guaranteed 2s stall. +- **Bet 4 is falsified, exactly as its own clause anticipated: no + channel exists.** `ProcessEvent` is keyed by `ProcessId` while the + ledger is keyed by pgid and is deliberately independent of managed + records, so in the leader-exited-survivor case there is no id to + attribute to. Every production consumer polls `take_events(id)` per + known id (`lua_bindings/mod.rs:8933`, `:10731`, `mcp.rs:361`). + `take_all_events` would sidestep the keying but **has no production + consumer at all** — its only two call sites are tests, despite a doc + comment naming a `*processes*` buffer. `pmacs.error` was already + known dead. **Reporting becomes its own lane** (§5), and this PR ships + instrumentation plus tests without it, which is what §7 said it would + do in this case. +- **The in-drain `SIGKILL` is still unpinned**, as §1.2a promised. Its + `group_killed` flag stays local and the persistent ledger retries in + the same outer tick, so pinning the local non-retry still needs a + call-count assertion or a direct free-function test. Unchanged, and + restated here so a later reader does not mistake the shipped seam for + covering it. + **Revision 2 → 3**, after review round 2 (two blocking, two major). All four accepted; all four verified in the code first. @@ -271,10 +319,17 @@ noted so a red run on it is not mistaken for this lane's doing.* *Unknown. `killed = true` on a failed `SIGKILL` is clearly wrong as bookkeeping; what should replace it is a policy question, not an obvious fix.* -- **Q#RL3** — How does a background tick report anything? *The ledger - emits no events and has no status channel. `pmacs.error` is defined - only by a test stub and is dead at 15 call sites, so it is not the - answer. This is the lane's real design question.* +- **Q#RL3** — How does a background tick report anything? **ANSWERED at + revision 4: it cannot, today.** `ProcessEvent` is keyed by + `ProcessId`; the ledger is keyed by pgid and is deliberately + independent of managed records, so in the leader-exited-survivor case + — the one the mechanism exists for — there is no id to attribute to. + All three production consumers poll `take_events(id)` per known id + (`lua_bindings/mod.rs:8933`, `:10731`, `mcp.rs:361`). + `take_all_events` would sidestep the keying but has **no production + consumer**; its only call sites are two tests, notwithstanding a doc + comment naming a `*processes*` buffer. `pmacs.error` was already dead + at 15 sites. Reporting is therefore parked as its own lane (§5). - **Q#RL4** — Does the `shutdown()` loop need its own termination condition if (a) becomes strict? *Almost certainly (§1.3), and that coupling is why the two cannot be changed independently.* From 7df13f225770eae1987c1e2ae31200cc4c93a0c7 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 30 Jul 2026 20:41:03 -0400 Subject: [PATCH 8/8] fix(process): scope the escalation claim to ticks, and pin the boundary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 1, one finding, accepted. "A failed escalation is never retried by anything" was false. `shutdown()`'s force-kill loop iterates the reap ledger with **no** `!entry.killed` guard, so it does re-kill an entry the escalation arm gave up on. The accurate claim is that no later *tick* retries it — `tick_reap_ledger`'s escalation is guarded by `!entry.killed` and never fires again for that group. The overclaim collapsed two failure modes that this lane exists to keep distinct: a failed escalation leaks the group until editor exit, where one more attempt is made, while a failed `shutdown()` force-kill leaks it past exit with nothing left to try. Narrowed in the framing, the handoff, the active-work ledger and the test commentary. The corrected claim was asserted in three documents and pinned by nothing, so it gets a pin: a failed escalation marks the entry, the survivor stays alive across ticks, and `shutdown()` — with no fault planned, so its force-kill really lands — still reaps it. Bitten by adding the missing `!entry.killed` guard to that loop: the new pin fails and the other five stay green. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Lv428Fth9LRtffwJSsqH7T --- docs/active-work.md | 13 +++-- docs/agent-handoff.md | 14 ++++- docs/reap-ledger-silent-failures-framing.md | 23 +++++++- src/process.rs | 64 ++++++++++++++++++++- 4 files changed, 101 insertions(+), 13 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index fc17960..fc8c695 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -363,10 +363,15 @@ compatible. justified the ledger's leniency and deliberately changed no disposition; this lane owns what it refused. - **Four sites, not the two #200 named.** In the persistent ledger: a - probe error of any errno drops the entry and cancels escalation; a failed escalating - `SIGKILL` is marked as succeeded and never retried; and `shutdown()` - discards its own force-kill result the same way — on the path that - exists specifically to stop a leak at editor exit. **Plus the in-drain + probe error of any errno drops the entry and cancels escalation; a + failed escalating `SIGKILL` is marked as succeeded, so **no later tick + retries it**; and `shutdown()` discards its own force-kill result the + same way — on the path that exists specifically to stop a leak at + editor exit. Those last two are **distinct, not cumulative**: + `shutdown()` force-kills every entry with no `!entry.killed` guard, so + a failed escalation still gets one attempt at exit, while a failed + force-kill leaks the group past exit with nothing left to try. + **Plus the in-drain twin** `final_drain_runtime`, which collapses every errno to "dead" while no tick runs; a false "dead" there cancels the readers, so its failure mode is truncated output rather than a leaked process. diff --git a/docs/agent-handoff.md b/docs/agent-handoff.md index f2af131..d7e4d2b 100644 --- a/docs/agent-handoff.md +++ b/docs/agent-handoff.md @@ -267,9 +267,17 @@ commands, read `docs/active-work.md` immediately after this file. correct) from any other errno (we could not ask — not correct), and `retain` deletes the entry either way, cancelling escalation. - The deadline escalation sets `killed = true` whether or not the - `SIGKILL` landed, so a failed one is **never retried by anything**. - - `shutdown()`'s force-kill does the same, on the path written - specifically to stop a leak at editor exit. + `SIGKILL` landed, so **no later tick retries it** — that arm is + guarded by `!entry.killed` and never fires again for the group. + - `shutdown()`'s force-kill discards its result the same way, on the + path written specifically to stop a leak at editor exit. **It is not + the same failure**, and the difference is the retry: `shutdown()` + iterates the ledger with *no* `!entry.killed` guard, so it does + re-kill an entry the escalation arm marked. A failed escalation + leaks the group until editor exit, where one more attempt is made; a + failed force-kill leaks it *past* editor exit, with nothing left to + try. Saying the escalation is "never retried by anything" collapses + the two. - `final_drain_runtime`'s twin collapses every errno into "dead", which quiesces the drain and **cancels the readers** — truncated output rather than a leaked process, and terminal for that drain diff --git a/docs/reap-ledger-silent-failures-framing.md b/docs/reap-ledger-silent-failures-framing.md index 8d0dbdb..6eec255 100644 --- a/docs/reap-ledger-silent-failures-framing.md +++ b/docs/reap-ledger-silent-failures-framing.md @@ -169,7 +169,23 @@ behaviour is unchanged: an `EPERM` probe is indistinguishable from `ESRCH`. **(b)** A failed `SIGKILL` is recorded as a successful one. The entry -then satisfies `!entry.killed == false` forever and is never retried. +then satisfies `!entry.killed == false` forever, so **no later tick +retries it** — the escalation arm is guarded by `!entry.killed` and +never fires again for that group. + +**Scoped to ticks, and the scope matters.** `shutdown()`'s force-kill +loop (c) iterates the ledger with **no `!entry.killed` guard**, so it +*does* re-kill an entry this arm marked. The two failure modes are +therefore distinct rather than cumulative: + +| Failure | Survivor lives until | +|---|---| +| escalation `SIGKILL` fails | editor exit, where `shutdown()` gets one more attempt | +| `shutdown()` force-kill fails | past editor exit — nothing else tries | + +Saying (b) is "never retried by anything" would collapse that +distinction and overstate it: the one remaining attempt is exactly what +(c) is, and (c)'s own failure is a different and worse outcome. **(c) `shutdown()` has the same discard** (`:1763-1766`), on the path that exists specifically to stop a leak at editor exit: @@ -381,8 +397,9 @@ noted so a red run on it is not mistaken for this lane's doing.* - **Bet 2 — each silent consequence is demonstrable once injectable.** With the seam: an `EPERM` probe drops an entry whose group is still - alive; a failed `SIGKILL` leaves `killed = true` and is never retried; - `shutdown()`'s discard does the same at exit; and a continuously false + alive; a failed `SIGKILL` leaves `killed = true` so no later **tick** + retries it; `shutdown()`'s discard leaks the group past editor exit, + which is a different and worse outcome; and a continuously false in-drain probe cancels readers before a live descendant's deliberately late output can arrive. - *Falsified if* any of the three turns out to be unreachable in diff --git a/src/process.rs b/src/process.rs index 219cce9..1d8af2f 100644 --- a/src/process.rs +++ b/src/process.rs @@ -4649,8 +4649,16 @@ mod tests { fn a_failed_escalation_is_recorded_as_a_successful_one() { // §1.2 (b). `entry.killed = true` runs unconditionally, so a // SIGKILL that never landed satisfies `!entry.killed == false` - // forever. The consequence is not bookkeeping: the survivor is - // never killed again, by anything. + // forever. The consequence is not bookkeeping: **no later tick + // retries it**, so the survivor outlives every escalation the + // ledger will ever attempt during the session. + // + // Scoped to ticks deliberately. `shutdown()`'s force-kill loop + // iterates the ledger with **no `!entry.killed` guard**, so it + // does re-kill an entry this arm marked — which is why that + // failure mode is distinct and has its own pin. This one ticks + // and never calls `shutdown`, so what it asserts is exactly + // what it says. let dir = tempfile::tempdir().expect("tempdir"); let mut sup = ProcessSupervisor::new(); sup.set_group_term_grace(Duration::from_millis(150)); @@ -4683,11 +4691,61 @@ mod tests { pid_alive(survivor), "no tick ever retries the failed SIGKILL, so the survivor outlives the ledger's only escalation" ); - assert_eq!(sup.reap_ledger_len(), 1, "the entry is retained, and inert"); + assert_eq!( + sup.reap_ledger_len(), + 1, + "the entry is retained, and inert to every subsequent tick" + ); sup.assert_reap_faults_consumed(); reap_fixture_survivor(survivor); } + #[test] + fn shutdown_still_force_kills_a_group_a_failed_escalation_marked_killed() { + // The boundary of the pin above, and the reason its claim is + // "no later TICK retries it" rather than "nothing retries it". + // + // `shutdown()`'s force-kill loop iterates the ledger with no + // `!entry.killed` guard, so the one thing that still acts on an + // entry the escalation arm gave up on is editor exit. That + // keeps the two failure modes distinct: a failed escalation + // leaks the group until exit; a failed force-kill leaks it + // past exit. + // + // No fault is planned for the shutdown site here — the whole + // point is that this force-kill really lands. + let dir = tempfile::tempdir().expect("tempdir"); + let mut sup = ProcessSupervisor::new(); + sup.set_group_term_grace(Duration::from_millis(150)); + let (script, pidfile) = survivor_script(dir.path(), true); + let id = sup + .spawn(sh_group_spec("escalation-then-shutdown", &script)) + .expect("spawn"); + let events = drain_until(&mut sup, id, Duration::from_secs(5), has_exited); + let pgid = i32::try_from(started_pid(&events).expect("leader pid")).expect("pgid fits"); + let survivor = wait_pidfile(&pidfile); + + sup.plan_reap_kill_failure(ReapKillSite::LedgerEscalation, nix::errno::Errno::EPERM); + std::thread::sleep(Duration::from_millis(250)); + sup.tick(); + assert_eq!( + sup.reap_ledger_killed(pgid), + Some(true), + "precondition: the entry is marked killed by a SIGKILL that failed" + ); + assert!(pid_alive(survivor), "precondition: the survivor is alive"); + + sup.shutdown(); + + assert!( + !pid_alive(survivor), + "shutdown force-kills every armed entry, marked or not — so an escalation \ + failure is not the survivor's last reprieve" + ); + assert_eq!(sup.reap_ledger_len(), 0, "and the entry probes to ESRCH"); + sup.assert_reap_faults_consumed(); + } + #[test] fn a_failed_shutdown_force_kill_leaks_the_group_and_burns_the_bound() { // §1.2 (c). The path that exists specifically to stop a leak at