fix: a failing pprof listener must not kill the media server (#64 item 1) - #69
Merged
jason-shen merged 1 commit intoAug 25, 2026
Conversation
Item 1 of streamcoreai#64. The goroutine in startDebugServer ended in log.Fatalf, which is os.Exit(1). That path skips sm.CloseAll() and both graceful shutdowns in main, so an accept error on an optional profiling socket dropped every live WebRTC call with no cleanup. The main HTTP server keeps log.Fatalf because the process genuinely cannot serve without that listener; pprof is opt-in and it can. Now log.Printf, and the server keeps running without pprof. The goroutine literal is lifted into a named serveDebug so the return is observable. Two tests: - TestServeDebugSurvivesAFailedListener drives serveDebug with a listener whose Accept returns a permanent error — the shape that actually terminates Serve, since net/http retries temporary ones — and requires the call to return. - TestPublicMuxStillServesAfterTheDebugListenerDies goes through the real startDebugServer, stops the debug listener under its own Serve loop, and requires the public mux to still answer /health. On its own the first test would be satisfied by a serveDebug nobody calls; this one asserts the operator-visible claim. Red proof: with only the log.Printf reverted to log.Fatalf, the first test does not report a failure — it kills the test binary. `=== RUN` prints, no result line follows, and the package reports FAIL. That is the defect itself, reproduced inside the suite. gofmt clean, go build ./... and go vet ./... clean, and `go test -race ./...` (the CI command) passes across all 15 packages. Items 3 through 10 of streamcoreai#64 are deliberately untouched; this is one independent item as the issue invites.
Member
|
looks good to me, thanks @vsolano9 |
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.
Item 1 of #64. Independent of the rest of that list; items 3–10 are untouched.
The defect
startDebugServer's goroutine ended inlog.Fatalf, which isos.Exit(1):That skips
sm.CloseAll()and both graceful shutdowns inmain, so an accept error on an optional profiling socket drops every live WebRTC call with no cleanup. As the issue puts it, this mirrors the main HTTP server's pattern in a place where the reasoning does not carry: the process genuinely cannot do its job without the media listener. It can without pprof.Now
log.Printf, and the server keeps running without profiling.Making it testable
The goroutine literal is lifted into a named
serveDebug(srv, listener)so its return is observable. Nothing else moved.Two tests, because the obvious one is not sufficient on its own:
TestServeDebugSurvivesAFailedListenerdrivesserveDebugwith a listener whoseAcceptreturns a permanent error, and requires the call to return. The error is deliberately not anet.ErrorwithTemporary() == true—net/httpretries those with a backoff, so a temporary error would hang rather than exercise the exit path.TestPublicMuxStillServesAfterTheDebugListenerDiesgoes through the realstartDebugServer, stops the debug listener under its ownServeloop, then requires the public mux to still answer/health. On its own the first test would be satisfied by aserveDebugthat nothing calls; this one asserts the thing an operator actually cares about.Red proof
Reverting only the
log.Printfback tolog.Fatalf, with both tests in place:Worth reading closely: there is no
--- FAIL:line. The test does not report a failure — the test binary is killed mid-run, which is the defect reproduced inside the suite. A reader who greps for--- FAILwould conclude nothing ran.Validation
The exact CI commands from
.github/workflows/ci.yml:gofmt -l .— cleango build ./...— cleango vet ./...— cleango test -race ./...— ok across all 15 packagesNot claimed
I have not reproduced a real accept failure on a live socket; the listener is a stub, which is what makes the test hermetic. And this changes only the crash behaviour — a failed pprof listener now leaves the server running without profiling, silently apart from the log line. If you would rather it also surface in
/healthor a metric, that is a design call I did not want to presume, and it belongs with item 7 (warning when profiling settings cannot take effect) rather than here.