From c32a7b1b5ef94b50579a12b82f27bd6c30c6e371 Mon Sep 17 00:00:00 2001 From: Victor Solano Date: Tue, 25 Aug 2026 15:00:00 +0200 Subject: [PATCH] fix: a failing pprof listener must not kill the media server MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Item 1 of #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 #64 are deliberately untouched; this is one independent item as the issue invites. --- main.go | 24 ++++++++++++---- main_test.go | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 97 insertions(+), 6 deletions(-) diff --git a/main.go b/main.go index d2eab1c..5d0bb58 100644 --- a/main.go +++ b/main.go @@ -199,15 +199,27 @@ func startDebugServer(cfg config.DebugConfig) (*http.Server, error) { Handler: newDebugMux(), ReadHeaderTimeout: 5 * time.Second, } - go func() { - log.Printf("Debug pprof server listening on %s", srv.Addr) - if err := srv.Serve(listener); err != nil && err != http.ErrServerClosed { - log.Fatalf("debug server error: %v", err) - } - }() + go serveDebug(srv, listener) return srv, nil } +// serveDebug runs the pprof listener until it stops. +// +// A failure here must not end the process. pprof is opt-in and optional; the +// media pipeline is neither. log.Fatalf would be os.Exit(1), skipping +// sm.CloseAll() and both graceful shutdowns in main, so an accept error on a +// profiling socket would drop every live WebRTC call with no cleanup. The main +// HTTP server keeps log.Fatalf because the process genuinely cannot do its job +// without that listener; this one it can. +// +// Separate from the goroutine literal so a test can prove it returns. +func serveDebug(srv *http.Server, listener net.Listener) { + log.Printf("Debug pprof server listening on %s", srv.Addr) + if err := srv.Serve(listener); err != nil && err != http.ErrServerClosed { + log.Printf("debug server error: %v; continuing without pprof", err) + } +} + func corsMiddleware(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Access-Control-Allow-Origin", "*") diff --git a/main_test.go b/main_test.go index 65dc1c5..36a8af8 100644 --- a/main_test.go +++ b/main_test.go @@ -3,6 +3,8 @@ package main import ( "context" "encoding/json" + "errors" + "net" "net/http" "net/http/httptest" "strings" @@ -226,3 +228,80 @@ func TestDebugServerRejectsPublicBindWithoutAcknowledgement(t *testing.T) { t.Fatalf("error = %q, want allow_public guidance", err) } } + +// failingListener returns a permanent Accept error, the shape of a listener +// that has died under a running server. net/http retries temporary errors, so +// a plain error is what actually terminates Serve. +type failingListener struct { + addr net.Addr + err error +} + +func (l *failingListener) Accept() (net.Conn, error) { return nil, l.err } +func (l *failingListener) Close() error { return nil } +func (l *failingListener) Addr() net.Addr { return l.addr } + +// A dead pprof listener must not take the process down with it. +// +// This is the whole point of the change: log.Fatalf is os.Exit(1), which skips +// sm.CloseAll() and both graceful shutdowns, so an accept error on an optional +// profiling socket would drop every live WebRTC call. The assertion is that +// serveDebug *returns* — under the old code this test does not fail, it kills +// the test binary, which is exactly the failure mode being fixed. +func TestServeDebugSurvivesAFailedListener(t *testing.T) { + addr, err := net.ResolveTCPAddr("tcp", "127.0.0.1:6060") + if err != nil { + t.Fatalf("resolve addr: %v", err) + } + srv := &http.Server{Addr: addr.String(), Handler: newDebugMux()} + listener := &failingListener{addr: addr, err: errors.New("accept: bad file descriptor")} + + done := make(chan struct{}) + go func() { + defer close(done) + serveDebug(srv, listener) + }() + + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatal("serveDebug did not return after a permanent Accept error") + } +} + +// The main pipeline keeps serving after pprof dies. +// +// The previous test proves serveDebug returns; on its own that is satisfied by +// a serveDebug nobody calls. This one drives the real startDebugServer path, +// kills the pprof listener underneath it, and then requires the public mux to +// still answer /health — the observable claim an operator cares about. +func TestPublicMuxStillServesAfterTheDebugListenerDies(t *testing.T) { + debugSrv, err := startDebugServer(config.DebugConfig{Bind: "127.0.0.1:0"}) + if err != nil { + t.Fatalf("startDebugServer: %v", err) + } + if debugSrv == nil { + t.Fatal("startDebugServer returned no server for a non-empty bind") + } + + public := httptest.NewServer(newPublicMux(func(http.ResponseWriter, *http.Request) {}, nil)) + t.Cleanup(public.Close) + + // Close the debug server out from under its own Serve loop. Serve then + // returns ErrServerClosed, the same return path a fatal accept error takes. + ctx, cancel := context.WithTimeout(context.Background(), time.Second) + defer cancel() + if err := debugSrv.Shutdown(ctx); err != nil { + t.Fatalf("shutdown debug server: %v", err) + } + + client := &http.Client{Timeout: 5 * time.Second} + resp, err := client.Get(public.URL + "/health") + if err != nil { + t.Fatalf("GET /health after the debug listener stopped: %v", err) + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + t.Fatalf("status = %d, want 200", resp.StatusCode) + } +}