git.iko.soy/crush
on main help sign in
fix(server): recover from handler panics + return 500
9c35ee0Christian Rocha
internal/server/recover.gonew file+59
compare with main
unifiedsplit
@@ -0,0 +1,59 @@
1+package server
2+
3+import (
4+ "log/slog"
5+ "net/http"
6+ "runtime/debug"
7+)
8+
9+// recoverHandler wraps the next handler in a panic-recovery middleware.
10+// If a handler panics, the panic is logged with a stack trace and a 500
11+// JSON error is written to the client (when no response has been started
12+// yet). Without this, a panicking handler closes the connection silently
13+// and surfaces as an opaque EOF on the client side.
14+func (s *Server) recoverHandler(next http.Handler) http.Handler {
15+ return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
16+ rrw := &recoverResponseWriter{ResponseWriter: w}
17+ defer func() {
18+ rec := recover()
19+ if rec == nil {
20+ return
21+ }
22+ // http.ErrAbortHandler is the documented way to abort a
23+ // handler without logging; preserve that contract.
24+ if rec == http.ErrAbortHandler {
25+ panic(rec)
26+ }
27+ s.logError(
28+ r, "Panic in handler",
29+ slog.Any("panic", rec),
30+ slog.String("stack", string(debug.Stack())),
31+ )
32+ if !rrw.wroteHeader {
33+ jsonError(rrw, http.StatusInternalServerError, "internal server error")
34+ }
35+ }()
36+ next.ServeHTTP(rrw, r)
37+ })
38+}
39+
40+// recoverResponseWriter tracks whether the response has been started so
41+// the recovery middleware knows if it can still write a 500 error.
42+type recoverResponseWriter struct {
43+ http.ResponseWriter
44+ wroteHeader bool
45+}
46+
47+func (rrw *recoverResponseWriter) WriteHeader(code int) {
48+ rrw.wroteHeader = true
49+ rrw.ResponseWriter.WriteHeader(code)
50+}
51+
52+func (rrw *recoverResponseWriter) Write(b []byte) (int, error) {
53+ rrw.wroteHeader = true
54+ return rrw.ResponseWriter.Write(b)
55+}
56+
57+func (rrw *recoverResponseWriter) Unwrap() http.ResponseWriter {
58+ return rrw.ResponseWriter
59+}
internal/server/recover_test.gonew file+94
@@ -0,0 +1,94 @@
1+package server
2+
3+import (
4+ "encoding/json"
5+ "io"
6+ "net/http"
7+ "net/http/httptest"
8+ "testing"
9+
10+ "github.com/charmbracelet/crush/internal/proto"
11+ "github.com/stretchr/testify/require"
12+)
13+
14+// TestRecoverHandler_PanicReturns500 verifies that a panicking handler
15+// surfaces as a structured 500 to the client, rather than closing the
16+// connection silently and producing an opaque EOF.
17+func TestRecoverHandler_PanicReturns500(t *testing.T) {
18+ t.Parallel()
19+
20+ s := &Server{}
21+ h := s.recoverHandler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
22+ panic("kaboom")
23+ }))
24+
25+ rec := httptest.NewRecorder()
26+ req := httptest.NewRequest(http.MethodGet, "/test", nil)
27+ h.ServeHTTP(rec, req)
28+
29+ require.Equal(t, http.StatusInternalServerError, rec.Code)
30+ body, err := io.ReadAll(rec.Body)
31+ require.NoError(t, err)
32+ require.NotEmpty(t, body)
33+
34+ var perr proto.Error
35+ require.NoError(t, json.Unmarshal(body, &perr))
36+ require.NotEmpty(t, perr.Message)
37+}
38+
39+// TestRecoverHandler_NoPanicPassthrough verifies that the middleware
40+// does not interfere with successful responses.
41+func TestRecoverHandler_NoPanicPassthrough(t *testing.T) {
42+ t.Parallel()
43+
44+ s := &Server{}
45+ h := s.recoverHandler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
46+ w.WriteHeader(http.StatusTeapot)
47+ _, _ = w.Write([]byte("ok"))
48+ }))
49+
50+ rec := httptest.NewRecorder()
51+ req := httptest.NewRequest(http.MethodGet, "/test", nil)
52+ h.ServeHTTP(rec, req)
53+
54+ require.Equal(t, http.StatusTeapot, rec.Code)
55+ require.Equal(t, "ok", rec.Body.String())
56+}
57+
58+// TestRecoverHandler_PanicAfterWriteHeader verifies that if a handler
59+// panics after it has already started writing the response, the
60+// middleware does not attempt to overwrite the status (which would
61+// trigger a superfluous WriteHeader warning) but still logs and
62+// recovers.
63+func TestRecoverHandler_PanicAfterWriteHeader(t *testing.T) {
64+ t.Parallel()
65+
66+ s := &Server{}
67+ h := s.recoverHandler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
68+ w.WriteHeader(http.StatusOK)
69+ _, _ = w.Write([]byte("partial"))
70+ panic("late panic")
71+ }))
72+
73+ rec := httptest.NewRecorder()
74+ req := httptest.NewRequest(http.MethodGet, "/test", nil)
75+ require.NotPanics(t, func() { h.ServeHTTP(rec, req) })
76+ require.Equal(t, http.StatusOK, rec.Code)
77+ require.Equal(t, "partial", rec.Body.String())
78+}
79+
80+// TestRecoverHandler_AbortHandlerPropagates verifies that the documented
81+// http.ErrAbortHandler sentinel is re-panicked so the net/http server
82+// can handle it normally (suppress logging, close connection).
83+func TestRecoverHandler_AbortHandlerPropagates(t *testing.T) {
84+ t.Parallel()
85+
86+ s := &Server{}
87+ h := s.recoverHandler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
88+ panic(http.ErrAbortHandler)
89+ }))
90+
91+ rec := httptest.NewRecorder()
92+ req := httptest.NewRequest(http.MethodGet, "/test", nil)
93+ require.PanicsWithValue(t, http.ErrAbortHandler, func() { h.ServeHTTP(rec, req) })
94+}
internal/server/server.go+1−1
@@ -168,7 +168,7 @@ func NewServer(cfg *config.ConfigStore, network, address string) *Server {
168 mux.Handle("/v1/docs/", httpswagger.WrapHandler)
168 mux.Handle("/v1/docs/", httpswagger.WrapHandler)
169 s.h = &http.Server{
169 s.h = &http.Server{
170 Protocols: &p,
170 Protocols: &p,
171− Handler: s.loggingHandler(mux),
171+ Handler: s.recoverHandler(s.loggingHandler(mux)),
172 }
172 }
173 if network == "tcp" {
173 if network == "tcp" {
174 s.h.Addr = address
174 s.h.Addr = address