Harden the pprof debug listener: crash isolation, explicit mux, and the docs it still needs

[jason-shen]

Follow-up to #57 / #63. The internal pprof listener landed and is safe by default — debug.bind = "" means no listener, no runtime profile changes, and the public mux never sees /debug/pprof/. Everything below only matters once an operator actually opts in, which is why it was worth merging first and tightening after.

Items are independent. Grab one, grab several, no need to claim the whole list.

1. A failing debug listener must not kill the server

main.go, in the goroutine started by startDebugServer:

if err := srv.Serve(listener); err != nil && err != http.ErrServerClosed {
    log.Fatalf("debug server error: %v", err)
}

log.Fatalf is os.Exit(1): no sm.CloseAll(), no graceful HTTP shutdown, every live WebRTC call drops on the spot. An accept error on an optional profiling listener should never take production media down with it. This mirrors the pattern used for the main HTTP server, where the process genuinely cannot do its job without a listener — here it can. Make it log.Printf and let the server keep running without pprof.

2. Register pprof on an explicit mux, not DefaultServeMux

The debug server currently uses _ "net/http/pprof" plus Handler: http.DefaultServeMux. Nothing else in the tree registers on the default mux today, so this is correct right now — but it means any future http.Handle call, in this repo or in a dependency, silently starts serving on the profiling port. Make the guarantee structural:

mux := http.NewServeMux()
mux.HandleFunc("/debug/pprof/", pprof.Index)
mux.HandleFunc("/debug/pprof/cmdline", pprof.Cmdline)
mux.HandleFunc("/debug/pprof/profile", pprof.Profile)
mux.HandleFunc("/debug/pprof/symbol", pprof.Symbol)
mux.HandleFunc("/debug/pprof/trace", pprof.Trace)

with "net/http/pprof" as a named import. TestPublicMuxDoesNotServePprof then asserts something that is true by construction rather than by coincidence.

3. Say plainly what allow_public costs

There is no authentication on the listener. With debug.allow_public = true on a reachable interface:

• /debug/pprof/profile?seconds=3600 is an anonymous CPU-pinning DoS.
• A heap dump from this server can contain provider API keys, caller audio buffers and transcripts.

docs/configuration.md currently says profiles "expose process data", which undersells it for a voice server. Either state that allow_public is only for network-isolated deployments, or gate the listener behind a shared token.

4. Document the container case

Dockerfile exposes 8080, 3478 and the media range. Inside a container, 127.0.0.1:6060 is reachable only from inside that container, so the loopback advice means docker exec / nsenter. Anyone who wants -p 127.0.0.1:6060:6060 on the host has to bind 0.0.0.0 inside the container and therefore set allow_public = true — which is fine there, but the docs currently frame that flag as the dangerous option, so people will reach for it without understanding when it is and isn't safe. A short paragraph covering both routes prevents that from being cargo-culted onto a genuinely public bind.

5. Check the bind address before claiming the port

The loopback check runs after net.Listen, so 0.0.0.0:6060 is really bound (and accepting into the backlog) for the moment before the check closes it. Not an exposure — Serve never starts — but parsing the host portion first fails earlier, without grabbing a public port, and gives a clearer error for ":6060".

6. Give the debug server its own shutdown context

debugSrv.Shutdown(shutdownCtx) reuses the 5s context the main server has already spent. An in-flight 30s CPU profile holds the listener open past the deadline, so shutdown logs a debug HTTP shutdown error right as the force-exit watchdog fires. Shut the debug server down first, or hand it a context of its own.

7. Warn when profiling settings can't take effect

block_profile_rate and mutex_profile_fraction are only applied when bind is set. Setting a rate and forgetting bind silently does nothing. One log.Printf at startup saves someone a confused hour.

8. Tests

• No coverage for bind = "" returning (nil, nil) — the exact invariant main.go's if debugSrv != nil shutdown path depends on.
• No coverage for allow_public = true accepting a non-loopback bind, or for the negative-rate validation.
• TestDebugServerServesPprofOnLoopback uses http.Get with the default client, which has no timeout: a regression hangs until the go test timeout instead of failing.

9. Translate the new config section

docs/configuration.zh-CN.md is missing the whole [debug] block that #63 added to docs/configuration.md — the TOML example, the two bullets under Notes, and the go tool pprof snippet. Independently claimable; needs Chinese, not Go.

10. Nit

config.toml.example: the bind = "" comment starts one column left of the other three in the [debug] block.

[vsolano9]

I’ll take item 2 only: replace the blank-import/http.DefaultServeMux coupling with a dedicated pprof mux, then add focused tests proving all standard pprof routes are served by that private mux while unrelated default-mux registrations cannot leak onto the debug listener. I’ll keep the crash-isolation, bind validation, shutdown, docs, and translation items separate and run the repository’s exact build/vet/race gates.

[vsolano9]

Opened #69 for item 1 — the log.Fatalf in startDebugServer. Taking just that one; items 3–10 are untouched and still free.

Two things worth surfacing here rather than only in the PR:

The test for this cannot be an ordinary failing test. With the fix reverted, TestServeDebugSurvivesAFailedListener does not report a failure — it kills the test binary. === RUN prints, no --- FAIL: line follows, and the package reports FAIL. That is the defect itself appearing inside the suite, and it is why the assertion is "serveDebug returns" rather than anything about the response.

The listener stub has to return a permanent error, not a temporary one. net/http retries net.Errors with Temporary() == true under a backoff, so a temporary error makes the test hang instead of exercising the exit path. Noting it because it is an easy way to write a test here that looks right and proves nothing.

gofmt, go build ./..., go vet ./... and go test -race ./... (your CI command) are all clean, 15/15 packages.

On item 2 from my earlier comment: that landed as #66, so it is done and I have not touched it again.