diff --git a/docs/active-work.md b/docs/active-work.md index 09c5c9d..fc8c695 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -333,6 +333,94 @@ compatible. githubsucks/test-ambient-config-isolation ``` +## Reap-ledger silent failures — IMPLEMENTED, PR OPEN + +- **Branch `reap-ledger-silent-failures`**, worktree + `../pmacs-reap-ledger`, based on `githubsucks/main` @ `22df6ab`. + `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. +- **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, 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. +- **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. **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. **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 + 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. 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 + 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. +- 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/agent-handoff.md b/docs/agent-handoff.md index b4629f5..d7e4d2b 100644 --- a/docs/agent-handoff.md +++ b/docs/agent-handoff.md @@ -257,6 +257,69 @@ 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 **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 + 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 new file mode 100644 index 0000000..6eec255 --- /dev/null +++ b/docs/reap-ledger-silent-failures-framing.md @@ -0,0 +1,500 @@ +# Framing — group cleanup fails silently at four sites + +**Revision 4.** Status: **APPROVED at revision 3; implemented.** Lane +`reap-ledger-silent-failures`, worktree `../pmacs-reap-ledger`, based on +`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 +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. + +- **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. + +- **§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 +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 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. +- **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 in the persistent ledger + +```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, 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: + +```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.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. 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 +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. 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 + +`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 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 +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? **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.* +- **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 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 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 | 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.** 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 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` 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 + 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. + - **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 + 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. 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 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 + 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 persistent-ledger paths and + the in-drain probe twin, 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: **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. diff --git a/src/process.rs b/src/process.rs index 33fdf8f..1d8af2f 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,319 @@ 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: **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)); + 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 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 + // 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. + // + // **`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; ( 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); + 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();