From c0bd6b2d82e2cd632f159fc372be25374bbc213c Mon Sep 17 00:00:00 2001 From: Radoslav Dimitrov Date: Tue, 17 Feb 2026 16:59:23 +0200 Subject: [PATCH] Add logging support to recovery middleware Signed-off-by: Radoslav Dimitrov --- recovery/doc.go | 8 +++++ recovery/recovery.go | 44 +++++++++++++++++++++---- recovery/recovery_test.go | 69 ++++++++++++++++++++++++++++++++++++++- 3 files changed, 114 insertions(+), 7 deletions(-) diff --git a/recovery/doc.go b/recovery/doc.go index dac9a68..d8c1208 100644 --- a/recovery/doc.go +++ b/recovery/doc.go @@ -14,6 +14,14 @@ // wrappedMux := recovery.Middleware(mux) // http.ListenAndServe(":8080", wrappedMux) // +// # Logging +// +// By default panics are recovered silently. Use [WithLogger] to log +// panic details (value, stack trace, request method and path) at ERROR level: +// +// logger := logging.New() +// wrappedMux := recovery.Middleware(mux, recovery.WithLogger(logger)) +// // # Stability // // This package is Beta stability. The API may have minor changes before diff --git a/recovery/recovery.go b/recovery/recovery.go index 5caaf45..7ec15be 100644 --- a/recovery/recovery.go +++ b/recovery/recovery.go @@ -4,21 +4,53 @@ package recovery import ( + "fmt" + "log/slog" "net/http" + "runtime/debug" ) +// config holds the resolved configuration for the recovery middleware. +type config struct { + logger *slog.Logger +} + +// Option configures the recovery middleware. +type Option func(*config) + +// WithLogger sets the logger used to report recovered panics. +// When a panic is recovered and a logger is configured, the middleware +// logs the panic value, stack trace, and request context at ERROR level. +func WithLogger(l *slog.Logger) Option { + return func(c *config) { + c.logger = l + } +} + // Middleware is an HTTP middleware that recovers from panics. // When a panic occurs, it returns a 500 Internal Server Error response // to the client, preventing the panic from crashing the server. // -// TODO(#7): Add configurable logging support once common logging is -// established across ToolHive. Currently panics are silently recovered. -func Middleware(next http.Handler) http.Handler { +// Options can be provided to configure logging behavior. By default, +// panics are recovered silently. Use [WithLogger] to enable logging. +func Middleware(next http.Handler, opts ...Option) http.Handler { + cfg := &config{} + for _, opt := range opts { + opt(cfg) + } + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { defer func() { - if recover() != nil { - // TODO(#7): Log panic value and stack trace - // stack := debug.Stack() + if v := recover(); v != nil { + if cfg.logger != nil { + stack := debug.Stack() + cfg.logger.ErrorContext(r.Context(), "panic recovered", + slog.String("panic", fmt.Sprintf("%v", v)), + slog.String("method", r.Method), + slog.String("path", r.URL.Path), + slog.String("stack", string(stack)), + ) + } http.Error(w, "Internal Server Error", http.StatusInternalServerError) } }() diff --git a/recovery/recovery_test.go b/recovery/recovery_test.go index e08c952..692a546 100644 --- a/recovery/recovery_test.go +++ b/recovery/recovery_test.go @@ -4,12 +4,15 @@ package recovery import ( + "bytes" "context" "net/http" "net/http/httptest" "testing" "github.com/stretchr/testify/assert" + + "github.com/stacklok/toolhive-core/logging" ) func TestMiddleware_NoPanic(t *testing.T) { @@ -44,7 +47,7 @@ func TestMiddleware_RecoverFromPanic(t *testing.T) { panic("test panic") }) - // Wrap with recovery middleware + // Wrap with recovery middleware (no logger — silent recovery) wrappedHandler := Middleware(testHandler) // Create test request @@ -59,6 +62,70 @@ func TestMiddleware_RecoverFromPanic(t *testing.T) { assert.Contains(t, rec.Body.String(), "Internal Server Error") } +func TestMiddleware_RecoverFromPanicWithLogger(t *testing.T) { + t.Parallel() + + var buf bytes.Buffer + logger := logging.New(logging.WithOutput(&buf)) + + // Create a test handler that panics + testHandler := http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) { + panic("test panic") + }) + + // Wrap with recovery middleware with logger + wrappedHandler := Middleware(testHandler, WithLogger(logger)) + + // Create test request + req := httptest.NewRequest(http.MethodPost, "/api/resource", nil) + rec := httptest.NewRecorder() + + // Execute request - should not panic + wrappedHandler.ServeHTTP(rec, req) + + // Verify 500 Internal Server Error response + assert.Equal(t, http.StatusInternalServerError, rec.Code) + assert.Contains(t, rec.Body.String(), "Internal Server Error") + + // Verify log output contains expected fields + logOutput := buf.String() + assert.Contains(t, logOutput, "panic recovered") + assert.Contains(t, logOutput, "test panic") + assert.Contains(t, logOutput, "POST") + assert.Contains(t, logOutput, "/api/resource") + assert.Contains(t, logOutput, "stack") +} + +func TestMiddleware_NoPanicWithLogger(t *testing.T) { + t.Parallel() + + var buf bytes.Buffer + logger := logging.New(logging.WithOutput(&buf)) + + // Create a test handler that does not panic + testHandler := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("success")) + }) + + // Wrap with recovery middleware with logger + wrappedHandler := Middleware(testHandler, WithLogger(logger)) + + // Create test request + req := httptest.NewRequest(http.MethodGet, "/test", nil) + rec := httptest.NewRecorder() + + // Execute request + wrappedHandler.ServeHTTP(rec, req) + + // Verify response + assert.Equal(t, http.StatusOK, rec.Code) + assert.Equal(t, "success", rec.Body.String()) + + // Verify no log output when no panic occurs + assert.Empty(t, buf.String()) +} + func TestMiddleware_PreservesRequestContext(t *testing.T) { t.Parallel()