From f5b2cae93e6c707fa1bfe8b002412ae94d911332 Mon Sep 17 00:00:00 2001 From: Abhishek Sah Date: Thu, 13 Aug 2026 11:27:30 +0530 Subject: [PATCH 1/5] fix(server): recover from panics in RPC and UI HTTP handlers 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 --- pkg/server/recovery.go | 47 ++++++++++++++ pkg/server/recovery_test.go | 120 ++++++++++++++++++++++++++++++++++++ pkg/server/server.go | 8 ++- 3 files changed, 172 insertions(+), 3 deletions(-) create mode 100644 pkg/server/recovery.go create mode 100644 pkg/server/recovery_test.go diff --git a/pkg/server/recovery.go b/pkg/server/recovery.go new file mode 100644 index 000000000..ed2af5ce6 --- /dev/null +++ b/pkg/server/recovery.go @@ -0,0 +1,47 @@ +package server + +import ( + "context" + "errors" + "log/slog" + "net/http" + "runtime/debug" + + "connectrpc.com/connect" +) + +var errInternalServer = errors.New("internal server error") + +// connectPanicRecovery converts a panic anywhere in the RPC handler chain +// into a CodeInternal error response. The panic value and stack go to the +// server log only, never to the caller. +func connectPanicRecovery(logger *slog.Logger) connect.HandlerOption { + return connect.WithRecover(func(ctx context.Context, spec connect.Spec, _ http.Header, panicValue any) error { + logger.ErrorContext(ctx, "rpc handler panic", + "procedure", spec.Procedure, + "panic", panicValue, + "stack", string(debug.Stack())) + return connect.NewError(connect.CodeInternal, errInternalServer) + }) +} + +// uiPanicRecovery responds with a plain 500 instead of dropping the +// connection when a handler behind the UI mux panics. +func uiPanicRecovery(logger *slog.Logger, next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + defer func() { + if panicValue := recover(); panicValue != nil { + // net/http checks for ErrAbortHandler with ==, so we should too. + if panicValue == http.ErrAbortHandler { + panic(panicValue) + } + logger.ErrorContext(r.Context(), "http handler panic", + "path", r.URL.Path, + "panic", panicValue, + "stack", string(debug.Stack())) + http.Error(w, errInternalServer.Error(), http.StatusInternalServerError) + } + }() + next.ServeHTTP(w, r) + }) +} diff --git a/pkg/server/recovery_test.go b/pkg/server/recovery_test.go new file mode 100644 index 000000000..309357c36 --- /dev/null +++ b/pkg/server/recovery_test.go @@ -0,0 +1,120 @@ +package server + +import ( + "bytes" + "context" + "io" + "log/slog" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "connectrpc.com/connect" + "google.golang.org/protobuf/types/known/emptypb" +) + +func TestConnectPanicRecoveryReturnsInternalError(t *testing.T) { + var logBuf bytes.Buffer + logger := slog.New(slog.NewJSONHandler(&logBuf, nil)) + + handler := connect.NewUnaryHandler( + "/test.v1.TestService/Panic", + func(ctx context.Context, req *connect.Request[emptypb.Empty]) (*connect.Response[emptypb.Empty], error) { + panic("boom") + }, + connectPanicRecovery(logger), + ) + mux := http.NewServeMux() + mux.Handle("/test.v1.TestService/Panic", handler) + srv := httptest.NewServer(mux) + defer srv.Close() + + resp, err := http.Post(srv.URL+"/test.v1.TestService/Panic", "application/json", strings.NewReader("{}")) + if err != nil { + t.Fatalf("expected an error response, got a dropped connection: %v", err) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusInternalServerError { + t.Errorf("status = %d, want %d", resp.StatusCode, http.StatusInternalServerError) + } + body, err := io.ReadAll(resp.Body) + if err != nil { + t.Fatalf("read body: %v", err) + } + if !strings.Contains(string(body), `"internal"`) { + t.Errorf("body %q does not carry the internal error code", body) + } + if strings.Contains(string(body), "boom") { + t.Errorf("panic detail leaked to the client: %q", body) + } + + logged := logBuf.String() + if !strings.Contains(logged, `"level":"ERROR"`) { + t.Errorf("panic was not logged at error level: %q", logged) + } + if !strings.Contains(logged, "boom") || !strings.Contains(logged, "/test.v1.TestService/Panic") { + t.Errorf("log entry is missing the panic value or procedure: %q", logged) + } + if !strings.Contains(logged, "recovery_test.go") { + t.Errorf("log entry is missing the stack trace: %q", logged) + } +} + +func TestUIPanicRecoveryReturnsInternalError(t *testing.T) { + var logBuf bytes.Buffer + logger := slog.New(slog.NewJSONHandler(&logBuf, nil)) + + mux := http.NewServeMux() + mux.HandleFunc("/panic", func(w http.ResponseWriter, r *http.Request) { + panic("boom") + }) + + rec := httptest.NewRecorder() + uiPanicRecovery(logger, mux).ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/panic", nil)) + + if rec.Code != http.StatusInternalServerError { + t.Errorf("status = %d, want %d", rec.Code, http.StatusInternalServerError) + } + if strings.Contains(rec.Body.String(), "boom") { + t.Errorf("panic detail leaked to the client: %q", rec.Body.String()) + } + + logged := logBuf.String() + if !strings.Contains(logged, `"level":"ERROR"`) || !strings.Contains(logged, "boom") || !strings.Contains(logged, "/panic") { + t.Errorf("log entry is missing the error level, panic value, or path: %q", logged) + } +} + +func TestUIPanicRecoveryPassesThroughNormalRequests(t *testing.T) { + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + + mux := http.NewServeMux() + mux.HandleFunc("/ok", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusTeapot) + }) + + rec := httptest.NewRecorder() + uiPanicRecovery(logger, mux).ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/ok", nil)) + + if rec.Code != http.StatusTeapot { + t.Errorf("status = %d, want %d", rec.Code, http.StatusTeapot) + } +} + +func TestUIPanicRecoveryRepanicsOnErrAbortHandler(t *testing.T) { + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + + mux := http.NewServeMux() + mux.HandleFunc("/abort", func(w http.ResponseWriter, r *http.Request) { + panic(http.ErrAbortHandler) + }) + + defer func() { + if recovered := recover(); recovered != http.ErrAbortHandler { + t.Errorf("recovered %v, want http.ErrAbortHandler", recovered) + } + }() + uiPanicRecovery(logger, mux).ServeHTTP(httptest.NewRecorder(), httptest.NewRequest(http.MethodGet, "/abort", nil)) +} diff --git a/pkg/server/server.go b/pkg/server/server.go index c93d02b00..fbd0a1a89 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -100,7 +100,7 @@ func ServeUI(ctx context.Context, logger *slog.Logger, uiConfig UIConfig, apiSer server := &http.Server{ Addr: fmt.Sprintf(":%d", uiConfig.Port), - Handler: mux, + Handler: uiPanicRecovery(logger, mux), ReadHeaderTimeout: apiServerConfig.ReadHeaderTimeout, IdleTimeout: apiServerConfig.IdleTimeout, } @@ -178,8 +178,10 @@ func ServeConnect(ctx context.Context, logger *slog.Logger, cfg Config, deps api auditInterceptor, sessionInterceptor.UnaryConnectResponseInterceptor()) - frontierPath, frontierHandler := frontierv1beta1connect.NewFrontierServiceHandler(frontierService, interceptors, connect.WithCodec(connectCodec{})) - adminPath, adminHandler := frontierv1beta1connect.NewAdminServiceHandler(frontierService, interceptors, connect.WithCodec(connectCodec{})) + // Panic recovery goes first so it sits outermost and catches panics from + // the other interceptors as well, not just the handlers. + frontierPath, frontierHandler := frontierv1beta1connect.NewFrontierServiceHandler(frontierService, connectPanicRecovery(logger), interceptors, connect.WithCodec(connectCodec{})) + adminPath, adminHandler := frontierv1beta1connect.NewAdminServiceHandler(frontierService, connectPanicRecovery(logger), interceptors, connect.WithCodec(connectCodec{})) // Create mux and register handlers mux := http.NewServeMux() From eecc659633e2c2b49329ab8a4e483339bea44e00 Mon Sep 17 00:00:00 2001 From: Abhishek Sah Date: Thu, 13 Aug 2026 11:34:53 +0530 Subject: [PATCH 2/5] refactor(server): reuse pkg/errors ErrInternalServerError in panic recovery Co-Authored-By: Claude Fable 5 --- pkg/server/recovery.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/pkg/server/recovery.go b/pkg/server/recovery.go index ed2af5ce6..4dca9e372 100644 --- a/pkg/server/recovery.go +++ b/pkg/server/recovery.go @@ -2,16 +2,14 @@ package server import ( "context" - "errors" "log/slog" "net/http" "runtime/debug" "connectrpc.com/connect" + "github.com/raystack/frontier/pkg/errors" ) -var errInternalServer = errors.New("internal server error") - // connectPanicRecovery converts a panic anywhere in the RPC handler chain // into a CodeInternal error response. The panic value and stack go to the // server log only, never to the caller. @@ -21,7 +19,7 @@ func connectPanicRecovery(logger *slog.Logger) connect.HandlerOption { "procedure", spec.Procedure, "panic", panicValue, "stack", string(debug.Stack())) - return connect.NewError(connect.CodeInternal, errInternalServer) + return connect.NewError(connect.CodeInternal, errors.ErrInternalServerError) }) } @@ -39,7 +37,7 @@ func uiPanicRecovery(logger *slog.Logger, next http.Handler) http.Handler { "path", r.URL.Path, "panic", panicValue, "stack", string(debug.Stack())) - http.Error(w, errInternalServer.Error(), http.StatusInternalServerError) + http.Error(w, errors.ErrInternalServerError.Error(), http.StatusInternalServerError) } }() next.ServeHTTP(w, r) From 094eb1f947e4fca442f079b792ddd408db3bdef1 Mon Sep 17 00:00:00 2001 From: Abhishek Sah Date: Thu, 13 Aug 2026 11:46:32 +0530 Subject: [PATCH 3/5] fix(server): abort committed responses instead of appending an error 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 --- pkg/server/recovery.go | 38 ++++++++++++++++++++++-- pkg/server/recovery_test.go | 58 +++++++++++++++++++++++++++++++++++++ 2 files changed, 93 insertions(+), 3 deletions(-) diff --git a/pkg/server/recovery.go b/pkg/server/recovery.go index 4dca9e372..f24989941 100644 --- a/pkg/server/recovery.go +++ b/pkg/server/recovery.go @@ -23,10 +23,39 @@ func connectPanicRecovery(logger *slog.Logger) connect.HandlerOption { }) } +// committedWriter tracks whether the response status line has gone out, so +// the recovery handler knows whether a 500 can still be sent. +type committedWriter struct { + http.ResponseWriter + committed bool +} + +func (c *committedWriter) WriteHeader(statusCode int) { + c.committed = true + c.ResponseWriter.WriteHeader(statusCode) +} + +func (c *committedWriter) Write(b []byte) (int, error) { + c.committed = true + return c.ResponseWriter.Write(b) +} + +// Flush commits the response too. The connect reverse proxy on the UI mux +// needs the Flusher interface for streaming. +func (c *committedWriter) Flush() { + c.committed = true + if f, ok := c.ResponseWriter.(http.Flusher); ok { + f.Flush() + } +} + // uiPanicRecovery responds with a plain 500 instead of dropping the -// connection when a handler behind the UI mux panics. +// connection when a handler behind the UI mux panics. If the handler already +// wrote part of a response, the status can no longer be changed, so it aborts +// the connection instead of appending an error to the partial body. func uiPanicRecovery(logger *slog.Logger, next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + cw := &committedWriter{ResponseWriter: w} defer func() { if panicValue := recover(); panicValue != nil { // net/http checks for ErrAbortHandler with ==, so we should too. @@ -37,9 +66,12 @@ func uiPanicRecovery(logger *slog.Logger, next http.Handler) http.Handler { "path", r.URL.Path, "panic", panicValue, "stack", string(debug.Stack())) - http.Error(w, errors.ErrInternalServerError.Error(), http.StatusInternalServerError) + if cw.committed { + panic(http.ErrAbortHandler) + } + http.Error(cw, errors.ErrInternalServerError.Error(), http.StatusInternalServerError) } }() - next.ServeHTTP(w, r) + next.ServeHTTP(cw, r) }) } diff --git a/pkg/server/recovery_test.go b/pkg/server/recovery_test.go index 309357c36..c73bc4fc4 100644 --- a/pkg/server/recovery_test.go +++ b/pkg/server/recovery_test.go @@ -103,6 +103,64 @@ func TestUIPanicRecoveryPassesThroughNormalRequests(t *testing.T) { } } +func TestUIPanicRecoveryAbortsCommittedResponses(t *testing.T) { + var logBuf bytes.Buffer + logger := slog.New(slog.NewJSONHandler(&logBuf, nil)) + + mux := http.NewServeMux() + mux.HandleFunc("/partial", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + if _, err := w.Write([]byte("partial")); err != nil { + t.Fatalf("write: %v", err) + } + panic("boom") + }) + + rec := httptest.NewRecorder() + func() { + defer func() { + if recovered := recover(); recovered != http.ErrAbortHandler { + t.Errorf("recovered %v, want http.ErrAbortHandler", recovered) + } + }() + uiPanicRecovery(logger, mux).ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/partial", nil)) + }() + + if rec.Code != http.StatusOK { + t.Errorf("status = %d, want the already-committed %d", rec.Code, http.StatusOK) + } + if rec.Body.String() != "partial" { + t.Errorf("body = %q, want the partial body with nothing appended", rec.Body.String()) + } + if !strings.Contains(logBuf.String(), "boom") { + t.Errorf("panic was not logged: %q", logBuf.String()) + } +} + +func TestUIPanicRecoveryAbortsAfterFlush(t *testing.T) { + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + + mux := http.NewServeMux() + mux.HandleFunc("/flush", func(w http.ResponseWriter, r *http.Request) { + w.(http.Flusher).Flush() + panic("boom") + }) + + rec := httptest.NewRecorder() + func() { + defer func() { + if recovered := recover(); recovered != http.ErrAbortHandler { + t.Errorf("recovered %v, want http.ErrAbortHandler", recovered) + } + }() + uiPanicRecovery(logger, mux).ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/flush", nil)) + }() + + if strings.Contains(rec.Body.String(), "internal server error") { + t.Errorf("error body appended to a flushed response: %q", rec.Body.String()) + } +} + func TestUIPanicRecoveryRepanicsOnErrAbortHandler(t *testing.T) { logger := slog.New(slog.NewTextHandler(io.Discard, nil)) From 838970f10a7369582dcf2ef4729f638db6342a8c Mon Sep 17 00:00:00 2001 From: Abhishek Sah Date: Thu, 13 Aug 2026 15:50:39 +0530 Subject: [PATCH 4/5] fix(server): extend panic recovery wrapper and cover the connect mux 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 --- pkg/server/recovery.go | 30 ++++++++++++++++++++----- pkg/server/recovery_test.go | 45 ++++++++++++++++++++++++++++--------- pkg/server/server.go | 9 +++++--- 3 files changed, 65 insertions(+), 19 deletions(-) diff --git a/pkg/server/recovery.go b/pkg/server/recovery.go index f24989941..2d3b61e45 100644 --- a/pkg/server/recovery.go +++ b/pkg/server/recovery.go @@ -2,6 +2,7 @@ package server import ( "context" + "io" "log/slog" "net/http" "runtime/debug" @@ -40,7 +41,7 @@ func (c *committedWriter) Write(b []byte) (int, error) { return c.ResponseWriter.Write(b) } -// Flush commits the response too. The connect reverse proxy on the UI mux +// Flush commits the response too. The connect reverse proxy on the UI server // needs the Flusher interface for streaming. func (c *committedWriter) Flush() { c.committed = true @@ -49,11 +50,28 @@ func (c *committedWriter) Flush() { } } -// uiPanicRecovery responds with a plain 500 instead of dropping the -// connection when a handler behind the UI mux panics. If the handler already -// wrote part of a response, the status can no longer be changed, so it aborts -// the connection instead of appending an error to the partial body. -func uiPanicRecovery(logger *slog.Logger, next http.Handler) http.Handler { +// Unwrap lets http.ResponseController reach the underlying writer's +// Hijacker, deadline, and full-duplex methods. +func (c *committedWriter) Unwrap() http.ResponseWriter { + return c.ResponseWriter +} + +// ReadFrom keeps the underlying writer's optimized copy path (pooled +// buffers, sendfile for OS files), which io.Copy looks for on the +// destination directly without walking Unwrap. +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) +} + +// httpPanicRecovery responds with a plain 500 instead of dropping the +// connection when a handler behind the wrapped mux panics. If the handler +// already wrote part of a response, the status can no longer be changed, so +// it aborts the connection instead of appending an error to the partial body. +func httpPanicRecovery(logger *slog.Logger, next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { cw := &committedWriter{ResponseWriter: w} defer func() { diff --git a/pkg/server/recovery_test.go b/pkg/server/recovery_test.go index c73bc4fc4..509c1b8fa 100644 --- a/pkg/server/recovery_test.go +++ b/pkg/server/recovery_test.go @@ -62,7 +62,7 @@ func TestConnectPanicRecoveryReturnsInternalError(t *testing.T) { } } -func TestUIPanicRecoveryReturnsInternalError(t *testing.T) { +func TestHTTPPanicRecoveryReturnsInternalError(t *testing.T) { var logBuf bytes.Buffer logger := slog.New(slog.NewJSONHandler(&logBuf, nil)) @@ -72,7 +72,7 @@ func TestUIPanicRecoveryReturnsInternalError(t *testing.T) { }) rec := httptest.NewRecorder() - uiPanicRecovery(logger, mux).ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/panic", nil)) + httpPanicRecovery(logger, mux).ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/panic", nil)) if rec.Code != http.StatusInternalServerError { t.Errorf("status = %d, want %d", rec.Code, http.StatusInternalServerError) @@ -87,7 +87,7 @@ func TestUIPanicRecoveryReturnsInternalError(t *testing.T) { } } -func TestUIPanicRecoveryPassesThroughNormalRequests(t *testing.T) { +func TestHTTPPanicRecoveryPassesThroughNormalRequests(t *testing.T) { logger := slog.New(slog.NewTextHandler(io.Discard, nil)) mux := http.NewServeMux() @@ -96,14 +96,14 @@ func TestUIPanicRecoveryPassesThroughNormalRequests(t *testing.T) { }) rec := httptest.NewRecorder() - uiPanicRecovery(logger, mux).ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/ok", nil)) + httpPanicRecovery(logger, mux).ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/ok", nil)) if rec.Code != http.StatusTeapot { t.Errorf("status = %d, want %d", rec.Code, http.StatusTeapot) } } -func TestUIPanicRecoveryAbortsCommittedResponses(t *testing.T) { +func TestHTTPPanicRecoveryAbortsCommittedResponses(t *testing.T) { var logBuf bytes.Buffer logger := slog.New(slog.NewJSONHandler(&logBuf, nil)) @@ -123,7 +123,7 @@ func TestUIPanicRecoveryAbortsCommittedResponses(t *testing.T) { t.Errorf("recovered %v, want http.ErrAbortHandler", recovered) } }() - uiPanicRecovery(logger, mux).ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/partial", nil)) + httpPanicRecovery(logger, mux).ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/partial", nil)) }() if rec.Code != http.StatusOK { @@ -137,7 +137,7 @@ func TestUIPanicRecoveryAbortsCommittedResponses(t *testing.T) { } } -func TestUIPanicRecoveryAbortsAfterFlush(t *testing.T) { +func TestHTTPPanicRecoveryAbortsAfterFlush(t *testing.T) { logger := slog.New(slog.NewTextHandler(io.Discard, nil)) mux := http.NewServeMux() @@ -153,7 +153,7 @@ func TestUIPanicRecoveryAbortsAfterFlush(t *testing.T) { t.Errorf("recovered %v, want http.ErrAbortHandler", recovered) } }() - uiPanicRecovery(logger, mux).ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/flush", nil)) + httpPanicRecovery(logger, mux).ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/flush", nil)) }() if strings.Contains(rec.Body.String(), "internal server error") { @@ -161,7 +161,32 @@ func TestUIPanicRecoveryAbortsAfterFlush(t *testing.T) { } } -func TestUIPanicRecoveryRepanicsOnErrAbortHandler(t *testing.T) { +func TestCommittedWriterUnwrapReturnsUnderlyingWriter(t *testing.T) { + rec := httptest.NewRecorder() + cw := &committedWriter{ResponseWriter: rec} + + if cw.Unwrap() != http.ResponseWriter(rec) { + t.Errorf("Unwrap() = %v, want the underlying writer", cw.Unwrap()) + } +} + +func TestCommittedWriterReadFromCommitsAndCopies(t *testing.T) { + rec := httptest.NewRecorder() + cw := &committedWriter{ResponseWriter: rec} + + n, err := io.Copy(cw, strings.NewReader("data")) + if err != nil || n != 4 { + t.Fatalf("copy = (%d, %v), want (4, nil)", n, err) + } + if !cw.committed { + t.Error("ReadFrom did not mark the response as committed") + } + if rec.Body.String() != "data" { + t.Errorf("body = %q, want %q", rec.Body.String(), "data") + } +} + +func TestHTTPPanicRecoveryRepanicsOnErrAbortHandler(t *testing.T) { logger := slog.New(slog.NewTextHandler(io.Discard, nil)) mux := http.NewServeMux() @@ -174,5 +199,5 @@ func TestUIPanicRecoveryRepanicsOnErrAbortHandler(t *testing.T) { t.Errorf("recovered %v, want http.ErrAbortHandler", recovered) } }() - uiPanicRecovery(logger, mux).ServeHTTP(httptest.NewRecorder(), httptest.NewRequest(http.MethodGet, "/abort", nil)) + httpPanicRecovery(logger, mux).ServeHTTP(httptest.NewRecorder(), httptest.NewRequest(http.MethodGet, "/abort", nil)) } diff --git a/pkg/server/server.go b/pkg/server/server.go index fbd0a1a89..144cd2166 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -100,7 +100,7 @@ func ServeUI(ctx context.Context, logger *slog.Logger, uiConfig UIConfig, apiSer server := &http.Server{ Addr: fmt.Sprintf(":%d", uiConfig.Port), - Handler: uiPanicRecovery(logger, mux), + Handler: httpPanicRecovery(logger, mux), ReadHeaderTimeout: apiServerConfig.ReadHeaderTimeout, IdleTimeout: apiServerConfig.IdleTimeout, } @@ -220,8 +220,11 @@ func ServeConnect(ctx context.Context, logger *slog.Logger, cfg Config, deps api _ = json.NewEncoder(w).Encode(map[string]string{"status": "SERVING"}) }) - // Configure and create the server - handler := connectinterceptors.WithConnectCORS(mux, cfg.ConnectCors) + // Configure and create the server. Panic recovery wraps the whole mux so + // routes outside the two service handlers (webhook bridge, ping, health, + // reflection, CORS) are covered too; RPC panics are still converted to + // connect error codes by WithRecover before they can reach this net. + handler := httpPanicRecovery(logger, connectinterceptors.WithConnectCORS(mux, cfg.ConnectCors)) // Serve HTTP/1.1 and unencrypted HTTP/2. Unlike the x/net h2c wrapper // this replaces, the server tracks these HTTP/2 connections itself, so From f89137210b554bbbd777c85466fb92d8115b824a Mon Sep 17 00:00:00 2001 From: Abhishek Sah Date: Fri, 14 Aug 2026 13:34:11 +0530 Subject: [PATCH 5/5] test(server): call committedWriter.ReadFrom directly io.Copy prefers the source's WriteTo over the destination's ReadFrom, and strings.Reader implements WriteTo, so the test never reached ReadFrom. Call it directly so the test exercises the method it checks. Co-Authored-By: Claude Fable 5 --- pkg/server/recovery_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/server/recovery_test.go b/pkg/server/recovery_test.go index 509c1b8fa..a7b5851d8 100644 --- a/pkg/server/recovery_test.go +++ b/pkg/server/recovery_test.go @@ -174,9 +174,9 @@ func TestCommittedWriterReadFromCommitsAndCopies(t *testing.T) { rec := httptest.NewRecorder() cw := &committedWriter{ResponseWriter: rec} - n, err := io.Copy(cw, strings.NewReader("data")) + n, err := cw.ReadFrom(strings.NewReader("data")) if err != nil || n != 4 { - t.Fatalf("copy = (%d, %v), want (4, nil)", n, err) + t.Fatalf("ReadFrom = (%d, %v), want (4, nil)", n, err) } if !cw.committed { t.Error("ReadFrom did not mark the response as committed")