Repository navigation
fix: close the debug server instead of spending the shutdown budget on it (#64 item 6) - #87
Merged
Conversation
…n it debugSrv.Shutdown reused the 5s context main had just given the public server. An in-flight pprof CPU profile or trace holds its request for as many seconds as asked, so shutdown ran that context out and ended in a race with the 5s force-exit watchdog (exit 0 or 1). Close the debug server first instead: a profile cut short by a stopping server is worth nothing, and closing cancels the request so pprof releases the profiler. Addresses item 6 of streamcoreai#64. Co-Authored-By: Claude Opus 5.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
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.
Addresses item 6 of #64: the debug server no longer shares main's shutdown context.
Cause
debugSrv.Shutdown(shutdownCtx)reused the 5s context thatsrv.Shutdownhad just been given.Shutdownwaits for in-flight requests, and a pprof CPU profile or trace holds its request open for as many seconds as the caller asked. So one?seconds=30profile made every shutdown run that context out. The force-exit watchdog is armed for the same five seconds, so the end of shutdown is a race betweenmainreturning andos.Exit(1).Change
The debug server is stopped with
Close(), first, and outside the graceful budget (stopDebugServerinmain.go). The issue suggested a context of its own or shutting the debug server down first. Neither stopsShutdownfrom waiting for the profile to finish, so both would still cost seconds the sessions and plugins need. A profile cut short by a server that is going away is worth nothing. Closing the connection cancels the request context, which ends pprof's wait and releases the CPU profiler.Item 6 only. Items 5 and 8–10 are untouched.
Tests
TestStopDebugServerDoesNotWaitForAnInFlightProfilestarts the real debug server and opens a 30s CPU profile. It waits until the handler actually holds the CPU profiler (StartCPUProfilefails), with no sleeps. Then it requiresstopDebugServerto return in under 1s, the profile request to end, and the profiler to be released.TestStopDebugServerAcceptsNilcovers thebind = ""case, whichmainnow passes straight through.Red proof: with
stopDebugServerchanged back toShutdownwith a 5s context, the first test fails withstopDebugServer took 5.000370844s with a profile in flight; want under 1s.Stability:
go test -race -count=40 -run '^TestStopDebugServer' .and the same withGOMAXPROCS=1 -count=20: all pass.End to end
I built the server from
mainand from this branch. Each ran with only[server] portand[debug] bind = "127.0.0.1:16060", with a?seconds=30profile in flight, and then got SIGINT. Two runs, shutdown log lines only:So on
mainthe outcome is a coin flip at the 5s mark. In run 2 the watchdog won, so the process exited 1 before it logged anything about the debug server.Validation
The four CI commands (Linux, Go 1.27.3):
gofmt -l .: cleango build ./...: cleango vet ./...: cleango test -race ./...: ok in every package with tests (internal/turnhas none)Prepared with AI assistance (Claude Code). I ran the checks above myself and the outputs are quoted as observed.