From 7df13f225770eae1987c1e2ae31200cc4c93a0c7 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 30 Jul 2026 20:41:03 -0400 Subject: [PATCH] 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