Session exit delivers its tail before closing subscribers (CI flake root-caused) - #3
Merged
Merged
Conversation
markExitedLocked dropped every subscriber the moment the leader was reaped, and a child that writes and exits in the same breath can be reaped on Linux while its last bytes still sit unread in the pty buffer — the supervisor's 5ms poll beating the pump to them. Subscribers then died over an empty ring, which is the CI flake recorded in FOLLOW-UPS (two sightings, same signature to the byte). Darwin cannot lose this race at all: a session leader's exit blocks until its pty is drained, which is why 130 loaded local runs never caught it. The exit now drains before it drops. markExitedLocked defers the drop to whoever can prove the drain finished: the pump, after the chunk that leaves the master with nothing readable — its verdict is authoritative, being the only reader — or the supervisor, after two of its polls agree the master is quiet and the ring has not grown between them, which covers a pump parked in a read that will never return (a background job holding the slave in silence). A stream that ends with the exit already recorded closes the subscribers itself, and an exit recorded after the stream ended drops them on the spot, so every ordering has exactly one closer. Reproduced deterministically in a Linux container by starving the pump: the old code fails with the flake's exact signature, the new code delivers the tail even with the reap winning by 250ms. Pinned by TestExitDeliversTheTailBeforeClosingSubscribers and TestMasterReadable; session package clean under -race on Darwin and in a Linux container. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…retires Three findings from the adversarial review, plus one measurement that reshaped the story. The supervisor could return with the drain still pending: a job-control background job lives in a process group of its own, so the leader's group can empty at the very reap that armed the drain, and a supervisor gated only on the group being empty retired after a single look. Its exit is now gated on drainSettled. The two-look quiet rule was a schedule, not a proof: signal requests and the master hint wake the select, compressing consecutive looks to microseconds, and a pump descheduled between its read returning and taking the lock holds a tail no poll can see. Looks now count only when reapPollMin apart on the wall clock, and the pump bumps an atomic read counter before it can take the lock — a tail in flight at one look has moved the counter by the next. The session field exitDrain collided with the daemon's exitDrain duration; renamed drainPending. And the measurement: Linux hangs the tty up when the session leader exits — a setsid'd grandchild's open slave descriptors do not keep the master alive, buffered output survives the hangup and is delivered before the error. So the pump-end path, not the supervisor fallback, is the working closer on both platforms; noteMasterEnded's platform notes now say so, and the new out-of-group-holder test pins the prompt close while its comment is honest about which mechanism it can and cannot exercise. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The re-review caught the counter comment claiming more than the counter delivers: a tail lifted off the master before the first look leaves the counter equal at both. The pump now flags itself parked only while it is inside Read — holding nothing — and a look believes quiet only with the pump parked and the counter still, which leaves the pump entering Read empty-handed as the one unobservable instant, and that is the very state quiet asserts. Comment stragglers from the rename fixed, the platform note's coincide claim narrowed to the one the supervisor actually rests on, and the out-of-group test's leader now lingers a beat so its subscriber provably beats the drop, with a holder short enough not to outlive the run it leaks from. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PR #2 recorded the second sighting and asked for instrumentation; this branch is the diagnosis and the fix, so the merged note keeps the second sighting's forensics and retires the instrumentation ask. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
internal/session imports unix for the master's poll; the marker just catches up with the import. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The flake, diagnosed
TestCloseTerminatesBackgroundChildrenfailed twice on CI (runs 31212710318, 31259207719) with the same signature: subscriber closed over an empty ring in the test's first millisecond. Root cause:markExitedLockeddropped every subscriber the moment the leader was reaped, and a child that writes-then-exits can be reaped on Linux while its final bytes still sit unread in the pty buffer — the supervisor's 5ms poll beating the pump's read. Reproduced deterministically in a Linux container by starving the pump: the old code fails with the flake's exact message.Why only Linux: Darwin blocks a session leader's exit until its pty is drained (measured —
psstateEthrough the whole window), so the reap structurally cannot win there. This is also a real product bug, not just a test one: a session whose process exits with output in flight lost that tail's delivery — exit pill before final lines.The fix: drain-then-drop
markExitedLockedrecords the exit and defers the subscriber drop to whoever can prove the drain finished:The supervisor's own retirement is gated on the drain having settled, so it can never exit leaving subscribers with no closer.
Measured along the way and now in the docs: Linux hangs the pty up when the session leader exits — a setsid'd grandchild's open slave fds do not keep the master alive, and buffered output survives the hangup.
noteMasterEnded's platform notes were corrected accordingly.Verification
TestExitDeliversTheTailBeforeClosingSubscribers(invariant tripwire),TestMasterReadable(the poll probe),TestExitWithOutOfGroupSlaveHolderClosesSubscribers(prompt close with an out-of-group slave holder; comment honest about which mechanism it exercises).internal/sessionclean under-race -count=2on Darwin and-count=2in a Linux container; fullgo test ./...clean.🤖 Generated with Claude Code