fix(server): recover from panics in RPC and UI HTTP handlers - #1877
fix(server): recover from panics in RPC and UI HTTP handlers#1877whoAbhishekSah wants to merge 4 commits into
Conversation
A panicking handler previously fell through to net/http's per-connection recover: the client's connection was dropped with no response (HTTP/2 stream reset), and the stack trace bypassed structured logging. Add connect.WithRecover on the Frontier and Admin service handlers so a panic anywhere in the handler chain returns CodeInternal with a generic message, and wrap the UI server mux with an equivalent recovery handler that returns a plain 500. Both log the panic value and stack through slog at error level; no panic details reach the caller. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe server adds centralized panic recovery for Connect RPC and UI HTTP handlers. Recovery logs panic context, hides panic details from clients, returns controlled errors, and preserves ChangesPanic recovery
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: 🟡 Moderate · up to The PR adds panic recovery for RPC and UI requests, but a UI panic after response data or status has already been sent can still leave clients with a successful status and partial response instead of a controlled 500; this response-integrity issue should be fixed or explicitly accepted before merge. Suggested reviewers: 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: bda1a8bf-e75d-4b74-815a-5f79464ef6d5
📒 Files selected for processing (3)
pkg/server/recovery.gopkg/server/recovery_test.gopkg/server/server.go
…covery Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Coverage Report for CI Build 31690670686Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage increased (+0.1%) to 48.313%Details
Uncovered Changes
Coverage Regressions126 previously-covered lines in 4 files lost coverage.
Coverage Stats
💛 - Coveralls |
…body When a UI handler panics after writing part of a response, the status line is already on the wire: http.Error cannot change it and would only append the error text to the partial body, so the client would read a corrupt 200. Track response commitment; once committed, log the panic and re-panic with http.ErrAbortHandler so net/http drops the connection and the client sees a truncated response instead of a fake success. The wrapper forwards Flush for the reverse proxy's streaming. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
|
||
| // Flush commits the response too. The connect reverse proxy on the UI mux | ||
| // needs the Flusher interface for streaming. | ||
| func (c *committedWriter) Flush() { |
There was a problem hiding this comment.
committedWriter only re-exposes Flush. It does not implement http.Hijacker and has no Unwrap method, so http.ResponseController on this writer cannot reach the underlying connection.
The UI mux hands committedWriter to the /frontier-connect/ ReverseProxy. If a request ever needs a protocol upgrade, ReverseProxy calls http.NewResponseController(rw).Hijack(). Since committedWriter is not a Hijacker and has no Unwrap, that returns ErrNotSupported and the proxy fails with can't switch protocols using non-Hijacker ResponseWriter type. The same missing Unwrap also blocks SetReadDeadline, SetWriteDeadline, and EnableFullDuplex, and turns FlushError into the error-less Flush, so flush and write errors on a broken client are dropped without notice.
One method fixes all of it, because http.ResponseController walks Unwrap to find Hijacker, the deadline setters, and FlushError:
func (c *committedWriter) Unwrap() http.ResponseWriter { return c.ResponseWriter }
There was a problem hiding this comment.
Agree on Unwrap — worth adding. One correction: Unwrap restores Hijack, the deadline setters, and EnableFullDuplex, but not FlushError. ResponseController.Flush checks FlushError, then Flusher, then unwraps; committedWriter implements Flush, so the lookup stops at the wrapper and returns nil either way. Getting flush errors back needs a FlushError method on the wrapper itself. The proxy discards flush errors anyway, so Unwrap alone seems enough.
There was a problem hiding this comment.
Added Unwrap in 838970f. ResponseController can now reach the underlying writer for Hijack, the deadline setters, and full duplex. Left FlushError alone per the thread below — the proxy discards flush errors anyway.
| c.ResponseWriter.WriteHeader(statusCode) | ||
| } | ||
|
|
||
| func (c *committedWriter) Write(b []byte) (int, error) { |
There was a problem hiding this comment.
committedWriter does not forward io.ReaderFrom, so the SPA static file server behind the UI mux loses its sendfile fast path.
Before this change the SPA handler got the raw *http.response, which implements io.ReaderFrom, so ServeContent used sendfile for static assets. Now it gets committedWriter, which has no ReadFrom, so every admin UI asset is copied through a 32KB userspace buffer loop instead. Small but real cost on the busiest UI path.
Unwrap does not fix this one. io.Copy checks the destination for io.ReaderFrom directly and does not walk Unwrap, so it needs its own method:
func (c *committedWriter) ReadFrom(r io.Reader) (int64, error) {
c.committed = true
if rf, ok := c.ResponseWriter.(io.ReaderFrom); ok {
return rf.ReadFrom(r)
}
return io.Copy(c.ResponseWriter, r)
}There was a problem hiding this comment.
The sendfile premise doesn't hold here. The admin SPA is served from embed.FS (web/apps/admin/embed.go, go:embed all:dist), and sendfile only applies when the copy source is a real OS file. Embedded assets were already copied through a userspace buffer before this change. The only delta is io.Copy allocating its 32KB buffer instead of using net/http's pooled one. The ReadFrom passthrough is still fine to add, but it's tidiness, not a lost fast path.
There was a problem hiding this comment.
Added ReadFrom in 838970f. As noted below, the admin assets come from embed.FS so there was no sendfile path to lose, but the passthrough keeps the underlying writer's copy path and it costs one small method. It also marks the response as committed, since bytes reached the wire. Added a test that io.Copy into the wrapper dispatches to it.
Address review feedback on the recovery wrapper and its coverage: - Add Unwrap on committedWriter so http.ResponseController can reach the underlying writer's Hijacker, deadline, and full-duplex methods; without it a protocol upgrade through the connect reverse proxy on the UI server would fail. - Add ReadFrom passthrough so copies into the wrapper keep the underlying writer's optimized path, which io.Copy looks up on the destination directly without walking Unwrap. - Wrap the connect server mux with the same recovery handler. The webhook bridge does its own parsing before the protected handler, and ping, health, reflection, and CORS had no recovery at all. RPC panics are still converted to connect error codes by WithRecover first; the outer wrapper only sees panics that escape it. - Rename uiPanicRecovery to httpPanicRecovery since it now fronts both servers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 03e7e87a-e04d-437e-84ce-32e176594906
📒 Files selected for processing (3)
pkg/server/recovery.gopkg/server/recovery_test.gopkg/server/server.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/server/recovery.go
| rec := httptest.NewRecorder() | ||
| cw := &committedWriter{ResponseWriter: rec} | ||
|
|
||
| n, err := io.Copy(cw, strings.NewReader("data")) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the declared Go version and the test dispatch path.
if [[ -f go.mod ]]; then
rg -n '^go [0-9.]+' go.mod
fi
sed -n '173,187p' pkg/server/recovery_test.go
rg -n -A12 -B2 'func \(c \*committedWriter\) ReadFrom' pkg/server/recovery.goRepository: raystack/frontier
Length of output: 1370
Exercise committedWriter.ReadFrom directly.
io.Copy selects strings.Reader.WriteTo before committedWriter.ReadFrom. The test can therefore pass without executing ReadFrom. Replace io.Copy(cw, strings.NewReader("data")) with cw.ReadFrom(strings.NewReader("data")).
4 participants
Problem
Frontier has no panic recovery in the request path. When a handler panics, the only net is Go's built-in per-connection recover in
net/http:INTERNAL_ERRORstream reset), instead of an RPC error.msg— invisible to anything watching for error-level logs, and with no request context.Reproduced by planting a deliberate
panic()in a handler:Fix
connect.WithRecover(...)when building the Frontier and Admin service handlers. The recover callback logs the procedure, panic value, and stack through slog at error level, and returnsCodeInternalwith a generic message. It is placed before the interceptor chain so it sits outermost and also catches panics thrown from interceptors, not just handlers./configs,/frontier-connect/proxy, SPA routes) with an equivalent recovery handler that returns a plain 500, sinceWithRecoveronly covers connect handlers. It re-panics onhttp.ErrAbortHandler, matching net/http's own contract.Out of scope: panics on internal background goroutines (audit log listener, shutdown watchers) — those are not HTTP calls and need their own recover treatment separately.
After the fix
Same deliberately panicking handler:
and one structured log entry:
Tests
"code":"internal", no panic detail in the body, and an error-level log entry carrying the procedure and stack.http.ErrAbortHandler.🤖 Generated with Claude Code