Merge pull request #202 from levineuwirth/reap-ledger-silent-failures
Make the reap ledger's silent kill failures testable
This commit is contained in:
commit
fbcf235cec
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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<GroupDrainCtx>` — 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.
|
||||
544
src/process.rs
544
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<Result<i32, PtyLookupFailure>>,
|
||||
/// 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<Mutex<ReapKillFaults>>,
|
||||
}
|
||||
|
||||
/// 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<ReapKillSite, VecDeque<nix::errno::Errno>>,
|
||||
/// The errno every probe inside the one claiming `GroupDrainCtx`
|
||||
/// reports, for that drain's whole lifetime.
|
||||
in_drain: Option<nix::errno::Errno>,
|
||||
/// 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<nix::errno::Errno> {
|
||||
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<nix::errno::Errno> {
|
||||
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<ReapKillFaults>,
|
||||
site: ReapKillSite,
|
||||
pgid: i32,
|
||||
signal: Option<Signal>,
|
||||
) -> 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<bool> {
|
||||
self.reap_ledger.get(&pgid).map(|e| e.killed)
|
||||
}
|
||||
}
|
||||
|
||||
impl Drop for ProcessSupervisor {
|
||||
|
|
@ -2322,13 +2508,40 @@ fn drain_runtime_output(rt: &RuntimeHandles) -> Vec<ProcessEventKind> {
|
|||
/// 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<Mutex<ReapKillFaults>>,
|
||||
/// 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<GroupDrainCtx>) -> Vec<ProcessEventKind> {
|
||||
/// 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<ProcessEventKind> {
|
||||
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<GroupDrainCtx>) -> 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();
|
||||
|
|
|
|||
Loading…
Reference in New Issue