server: enforce a terminal's deadline after the exit scan, not before - #215
Conversation
`supervise` opened each pass with `enforce_deadlines`, which signals any terminal whose deadline or stop grace has come due. That is the wrong end of the pass, because `reap_zombies` — which runs at the tail of the same pass — waits a child without marking its terminal exited. The two together leave a window. A child that dies between the exit scan and `reap_zombies` gets waited there, freeing its pid, but its `Pty` stays `exited: false` because nothing in `reap_zombies` can reach the session. The next pass then ran `enforce_deadlines` first, saw a live terminal, and, if the stop grace was due, sent `kill(-pid, SIGKILL)` at a pid the kernel had already released. A zombie pins its process group, so signalling a not-yet-reaped child is safe; signalling a reaped one aims at a pgid that is free to be reused. Move the call after the exit scan and its cleanup, still ahead of `reap_zombies`. Anything the previous pass waited is now marked exited by `poll_child_exited` — which reports a parked status — before deadlines are considered, and `enforce_deadlines` skips exited entries. That closes the window in both directions: a pid freed by this pass's `reap_zombies` is marked exited by the next pass's scan before this call can see it. No behaviour change for a terminal that is actually alive: the deadline still fires in the pass its instant falls in, only later within it, and `earliest_armed_deadline` computes the wakeup independently. Not covered by a test. `supervise` needs an `AppState`, which nothing in the suite constructs — the deadline and retention policies are unit tested through the pure `armed_deadline` and `slots_to_evict` for that reason, and neither is where this ordering lives. Verified by reading the pass against `reap_zombies`, `poll_child_exited` and `enforce_deadlines`; workspace clippy and `cargo fmt` clean, 287 blit-server tests passing. Co-Authored-By: Claude <noreply@anthropic.com>
|
Coverage
|
|
Correct, and a genuine miss on my part — thanks for chasing it. What stings is that #204's own description named this hazard and then failed to look for it: "group kill widens a pid-reuse race — The framing in the PR body is the part I'd missed entirely: a zombie pins its process group, so signalling a not-yet-reaped child is harmless — the danger only starts at the wait. That is what makes I re-derived the invariant against
So the reordering closes it completely, not just narrowly. Nothing else in the tree frees a pid while leaving a live One consequence worth writing down somewhere, since it is now load-bearing and invisible: |
1 participant
Follow-up to #204, from reviewing the merged PTY lifecycle work.
The bug
superviseopened each pass withenforce_deadlines, which signals any terminal whose deadline or stop grace has come due. That is the wrong end of the pass, becausereap_zombies— at the tail of the same pass — waits a child without marking its terminal exited; it has no way to reach the session.So a child that dies between the exit scan and
reap_zombiesgets waited there, freeing its pid, while itsPtystaysexited: false. The next pass then ranenforce_deadlinesfirst, saw a live terminal, and — if the stop grace was due — sentkill(-pid, SIGKILL)at a pid the kernel had already released.The distinction that matters: a zombie pins its process group, so signalling a not-yet-reaped child is safe. Signalling a reaped one aims at a pgid that is free to be reused. That is the only window where the
!pty.exitedguard does not hold, andreap_zombiesis the only thing that opens it.The fix
Move the call after the exit scan and its cleanup, still ahead of
reap_zombies. Anything the previous pass waited is now marked exited bypoll_child_exited— which reports a parked status — before deadlines are considered, andenforce_deadlinesskips exited entries. That closes the window in both directions: a pid freed by this pass'sreap_zombiesis marked exited by the next pass's scan before this call can see it.No behaviour change for a terminal that is actually alive. The deadline still fires in the pass its instant falls in, only later within it, and
earliest_armed_deadlinecomputes the wakeup independently.Verification
Not covered by a test, deliberately stated rather than glossed:
superviseneeds anAppState, which nothing in the suite constructs — the deadline and retention policies are unit tested through the purearmed_deadlineandslots_to_evictfor exactly that reason, and neither is where this ordering lives.Verified by reading the pass against
reap_zombies,poll_child_exitedandenforce_deadlines. Workspace clippy andcargo fmtclean, 287 blit-server tests passing.