From 088f417e70a94bb6e42a4c0cfdbe89061e5b4456 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 30 Jul 2026 16:13:02 -0400 Subject: [PATCH] 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.