Stop a cancelled context from leaving corestreamd alive forever - #4
Merged
Conversation
…ter it A corestreamd from the Aug 20 A/B was still running two days later: 21 threads in futex wait, HTTP listener already shut, a zombie sh child, and — from a SIGQUIT dump — goroutine 55 blocked in xdr.ReadFrameLength at pipesource.go:117 for 3,106 minutes, with runServices' WaitGroup waiting on it forever. sh forked rather than exec'd, and the stellar-core grandchild escaped the process group, so the shutdown SIGTERM never reached it. It kept the pipe's write end open, so the read never saw EOF. Nothing could break that cycle: the defer that closes the read end is downstream of the read returning, and WaitDelay escalates only to the direct child. SIGTERM therefore closed the listener and left the daemon alive indefinitely. The grandchild was still spinning at 100% of a core when I found it. Cancellation now interrupts the read directly — os.Pipe is poller-backed, so context.AfterFunc setting a deadline in the past unblocks it at once — and the defer sweeps the process group with SIGKILL after Wait, so an escaped grandchild cannot outlive the daemon that started it. frameTail reports the cancellation rather than the read deadline that implemented it, matching how Run already tells its own shutdown from a source failure. The test reproduces the escape exactly, with setsid putting the writer beyond the reach of the group signal; it fails at the 10s bound without the fix. The runbook also now says what P2 does not cover: apply-load produces a ledger every ~2s, so the real-core cell runs at a third of the 600ms target cadence. Only the synthetic source runs at the target shape, and the two have never been measured together. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013zyTXU8wkocJgN6mBafnou
There was a problem hiding this comment.
Pull request overview
This PR hardens the pipe ledger source so corestreamd cannot hang indefinitely during shutdown when a downstream pipe read blocks due to a lingering writer, and it updates operational documentation accordingly.
Changes:
- Unblocks
os.Pipereads on context cancellation via a forced read deadline, ensuringServer.Runcan return even when EOF never arrives. - Sweeps the command’s process group with SIGKILL after
Wait()to prevent non-direct descendants from surviving a shutdown path. - Adds a regression test for the cancel-unblocks-read contract and updates the two-box runbook with guidance on what P2 does/doesn’t validate plus target-shape sizing notes.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/server/pipesource.go | Ensures pipe reads are interruptible by context cancel; improves shutdown cleanup behavior; reports cancellation consistently from frameTail. |
| internal/server/pipesource_test.go | Adds a regression test covering shutdown behavior when a writer keeps the pipe open. |
| docs/two-box-runbook.md | Clarifies runbook interpretation for P2 and adds target-shape sizing/measurement notes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+93
to
+95
| stopUnblock := context.AfterFunc(ctx, func() { | ||
| _ = r.SetReadDeadline(time.Now()) | ||
| }) |
Comment on lines
+238
to
+250
| // TestPipeSource_CancelUnblocksAnEscapedWriter pins the shutdown contract | ||
| // against the case that actually happened: a grandchild that escapes the | ||
| // process group keeps the pipe's write end open, so the read never EOFs. The | ||
| // source must still return when the context is cancelled — before this was | ||
| // fixed it did not, and the daemon stayed alive with its listener already | ||
| // shut down and a stellar-core spinning behind it for two days. | ||
| func TestPipeSource_CancelUnblocksAnEscapedWriter(t *testing.T) { | ||
| // setsid puts the sleeper in its own session, so the source's group | ||
| // SIGTERM cannot reach it; it inherits fd 3 and holds the pipe open. The | ||
| // parent shell exits immediately, so the child is gone while the writer | ||
| // is not — exactly the observed shape. | ||
| src := PipeSource("setsid sleep 60 & exit 0") | ||
|
|
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
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.
A
corestreamdfrom the Aug 20 A/B was still running two days later. Its HTTP listener was already shut, it had a zombieshchild, and all 21 threads sat in futex wait. A SIGQUIT dump put the blame precisely:What happened
shforked rather than exec'd, and thestellar-coregrandchild escaped the process group, so the shutdown SIGTERM never reached it. It kept the pipe's write end open, so the read never saw EOF.Nothing could break that cycle. The defer that closes the read end is downstream of the read returning, and
WaitDelayescalates only to the direct child. So SIGTERM closed the listener and left the daemon alive indefinitely — and the escaped grandchild was still spinning at 100% of a core when I found it two days on.The fix
os.Pipeis poller-backed, socontext.AfterFuncsetting a read deadline in the past unblocks it at once.Wait, so an escaped grandchild cannot outlive the daemon that started it. On the ordinary path the group is already empty and this is an ESRCH no-op.frameTailreports the cancellation, not the read deadline that implemented it — matching howRunalready distinguishes its own shutdown from a source failure.The test reproduces the escape exactly, using
setsidto put the writer beyond the reach of the group signal. It fails at its 10s bound without the fix and passes in 0.20s with it.Also
The runbook now says what cell P2 does not cover:
apply-loadproduces a ledger every ~2s, so the real-core cell runs at a third of the 600ms target cadence. Only the synthetic source runs at the target shape, and the two have never been measured together. A P2 pass means "the tap is honest", not "the target shape is proven".🤖 Generated with Claude Code
https://claude.ai/code/session_013zyTXU8wkocJgN6mBafnou