fix(process): scope the escalation claim to ticks, and pin the boundary
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Lv428Fth9LRtffwJSsqH7T
This commit is contained in:
parent
2a1cf9be2e
commit
7df13f2257
|
|
@ -363,10 +363,15 @@ compatible.
|
||||||
justified the ledger's leniency and deliberately changed no
|
justified the ledger's leniency and deliberately changed no
|
||||||
disposition; this lane owns what it refused.
|
disposition; this lane owns what it refused.
|
||||||
- **Four sites, not the two #200 named.** In the persistent ledger: a
|
- **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
|
probe error of any errno drops the entry and cancels escalation; a
|
||||||
`SIGKILL` is marked as succeeded and never retried; and `shutdown()`
|
failed escalating `SIGKILL` is marked as succeeded, so **no later tick
|
||||||
discards its own force-kill result the same way — on the path that
|
retries it**; and `shutdown()` discards its own force-kill result the
|
||||||
exists specifically to stop a leak at editor exit. **Plus the in-drain
|
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"
|
twin** `final_drain_runtime`, which collapses every errno to "dead"
|
||||||
while no tick runs; a false "dead" there cancels the readers, so its
|
while no tick runs; a false "dead" there cancels the readers, so its
|
||||||
failure mode is truncated output rather than a leaked process.
|
failure mode is truncated output rather than a leaked process.
|
||||||
|
|
|
||||||
|
|
@ -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
|
correct) from any other errno (we could not ask — not correct), and
|
||||||
`retain` deletes the entry either way, cancelling escalation.
|
`retain` deletes the entry either way, cancelling escalation.
|
||||||
- The deadline escalation sets `killed = true` whether or not the
|
- The deadline escalation sets `killed = true` whether or not the
|
||||||
`SIGKILL` landed, so a failed one is **never retried by anything**.
|
`SIGKILL` landed, so **no later tick retries it** — that arm is
|
||||||
- `shutdown()`'s force-kill does the same, on the path written
|
guarded by `!entry.killed` and never fires again for the group.
|
||||||
specifically to stop a leak at editor exit.
|
- `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",
|
- `final_drain_runtime`'s twin collapses every errno into "dead",
|
||||||
which quiesces the drain and **cancels the readers** — truncated
|
which quiesces the drain and **cancels the readers** — truncated
|
||||||
output rather than a leaked process, and terminal for that drain
|
output rather than a leaked process, and terminal for that drain
|
||||||
|
|
|
||||||
|
|
@ -169,7 +169,23 @@ behaviour is unchanged: an `EPERM` probe is indistinguishable from
|
||||||
`ESRCH`.
|
`ESRCH`.
|
||||||
|
|
||||||
**(b)** A failed `SIGKILL` is recorded as a successful one. The entry
|
**(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
|
**(c) `shutdown()` has the same discard** (`:1763-1766`), on the path
|
||||||
that exists specifically to stop a leak at editor exit:
|
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.**
|
- **Bet 2 — each silent consequence is demonstrable once injectable.**
|
||||||
With the seam: an `EPERM` probe drops an entry whose group is still
|
With the seam: an `EPERM` probe drops an entry whose group is still
|
||||||
alive; a failed `SIGKILL` leaves `killed = true` and is never retried;
|
alive; a failed `SIGKILL` leaves `killed = true` so no later **tick**
|
||||||
`shutdown()`'s discard does the same at exit; and a continuously false
|
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
|
in-drain probe cancels readers before a live descendant's deliberately
|
||||||
late output can arrive.
|
late output can arrive.
|
||||||
- *Falsified if* any of the three turns out to be unreachable in
|
- *Falsified if* any of the three turns out to be unreachable in
|
||||||
|
|
|
||||||
|
|
@ -4649,8 +4649,16 @@ mod tests {
|
||||||
fn a_failed_escalation_is_recorded_as_a_successful_one() {
|
fn a_failed_escalation_is_recorded_as_a_successful_one() {
|
||||||
// §1.2 (b). `entry.killed = true` runs unconditionally, so a
|
// §1.2 (b). `entry.killed = true` runs unconditionally, so a
|
||||||
// SIGKILL that never landed satisfies `!entry.killed == false`
|
// SIGKILL that never landed satisfies `!entry.killed == false`
|
||||||
// forever. The consequence is not bookkeeping: the survivor is
|
// forever. The consequence is not bookkeeping: **no later tick
|
||||||
// never killed again, by anything.
|
// 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 dir = tempfile::tempdir().expect("tempdir");
|
||||||
let mut sup = ProcessSupervisor::new();
|
let mut sup = ProcessSupervisor::new();
|
||||||
sup.set_group_term_grace(Duration::from_millis(150));
|
sup.set_group_term_grace(Duration::from_millis(150));
|
||||||
|
|
@ -4683,11 +4691,61 @@ mod tests {
|
||||||
pid_alive(survivor),
|
pid_alive(survivor),
|
||||||
"no tick ever retries the failed SIGKILL, so the survivor outlives the ledger's only escalation"
|
"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();
|
sup.assert_reap_faults_consumed();
|
||||||
reap_fixture_survivor(survivor);
|
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]
|
#[test]
|
||||||
fn a_failed_shutdown_force_kill_leaks_the_group_and_burns_the_bound() {
|
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
|
// §1.2 (c). The path that exists specifically to stop a leak at
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue