Report handler panics through the logger and answer 500 (closes #187)

internal/server/export_test.go

@@ -54,3 +54,40 @@ func NewRouterForTest(
 
 	return s.router
 }
+
+// ProbePattern is the route NewRouterWithProbeForTest adds to the
+// production route tree.
+const ProbePattern = "/probe"
+
+// NewRouterWithProbeForTest builds the production route tree exactly
+// as NewRouterForTest does and then registers probe at ProbePattern,
+// so a test can drive a handler that panics through the shipped
+// global middleware chain rather than a hand-assembled one. Nothing
+// about the chain is rebuilt here: the probe is an extra leaf under
+// the same Use() registrations every other route gets.
+//
+// sentryEnabled selects whether the sentryhttp handler is registered,
+// which in production a configured SENTRY_DSN decides. It is a
+// parameter because the relationship between that handler's Repanic
+// option and the recoverer registered outside it is the thing a test
+// has to be able to pin.
+func NewRouterWithProbeForTest(
+	log *slog.Logger,
+	cfg *config.Config,
+	mw *middleware.Middleware,
+	h *handlers.Handlers,
+	sentryEnabled bool,
+	probe http.HandlerFunc,
+) http.Handler {
+	s := &Server{
+		log:           log,
+		mw:            mw,
+		h:             h,
+		params:        ServerParams{Config: cfg},
+		sentryEnabled: sentryEnabled,
+	}
+	s.SetupRoutes()
+	s.router.Handle(ProbePattern, probe)
+
+	return s.router
+}

internal/server/recoverer_test.go

@@ -0,0 +1,285 @@
+package server_test
+
+import (
+	"bytes"
+	"context"
+	"encoding/json"
+	"fmt"
+	"net/http"
+	"net/http/httptest"
+	"os"
+	"os/exec"
+	"strings"
+	"testing"
+
+	"github.com/getsentry/sentry-go"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/webhooker/internal/middleware"
+	"sneak.berlin/go/webhooker/internal/server"
+)
+
+// panicProbeMarker is the value the probe handler panics with. The
+// defect this pins lost it entirely: what reached the operator was the
+// recoverer's own secondary panic, naming chi's decorateFuncCallLine
+// and nothing about the fault that caused it.
+const panicProbeMarker = "QQPRODUCTIONPANICVALUEQQ"
+
+// panicChildEnv, when set, tells the re-executed test binary to run
+// the child half of the fd-level probe below.
+const panicChildEnv = "WEBHOOKER_PANIC_PROBE_CHILD"
+
+// panicChildResultPrefix labels the child's own one-line report of
+// what the HTTP client saw, so the parent can find it among whatever
+// else lands on the child's standard output.
+const panicChildResultPrefix = "PANIC-PROBE-RESULT "
+
+// stackTruncationMarker mirrors what internal/middleware appends to a
+// field it cut. It is duplicated rather than exported, as the access
+// log's budgets are, so that changing it has to be restated here
+// deliberately.
+const stackTruncationMarker = "[truncated]"
+
+// TestPanicThroughProductionRouter drives a handler panic through the
+// shipped router, over a real server, in a subprocess whose actual
+// file descriptors are captured.
+//
+// Every part of that is load-bearing.
+//
+// A subprocess, because the question is what reaches fd 1 and fd 2 of
+// the process an operator runs. The defect's signature was 0 bytes on
+// standard error and a 2,772-byte record on standard output describing
+// chi's own crash, and neither is visible to a test that swaps the
+// logger for a buffer.
+//
+// A real server, because a panicking handler under chi's Recoverer
+// dropped the connection: the client got EOF, not a status. An
+// httptest.ResponseRecorder has no connection to drop and would have
+// recorded the same unwritten response either way, which is why this
+// defect survived the existing suite.
+//
+// The production router, because the placement of the recoverer among
+// the other global middleware is part of the fix.
+func TestPanicThroughProductionRouter(t *testing.T) {
+	t.Parallel()
+
+	if os.Getenv(panicChildEnv) != "" {
+		t.Skip("child half; run by the parent below")
+	}
+
+	//nolint:gosec // Re-executing this test binary, with a fixed arg.
+	cmd := exec.CommandContext(
+		t.Context(), os.Args[0],
+		"-test.run", "^TestPanicProbeChild$",
+	)
+
+	cmd.Env = append(os.Environ(), panicChildEnv+"=1")
+
+	var stdout, stderr bytes.Buffer
+
+	cmd.Stdout = &stdout
+	cmd.Stderr = &stderr
+
+	require.NoError(
+		t, cmd.Run(),
+		"child failed\nstdout:\n%s\nstderr:\n%s",
+		stdout.String(), stderr.String(),
+	)
+
+	assertPanicProbeOutput(t, stdout.String(), stderr.String())
+}
+
+// assertPanicProbeOutput holds the child's descriptors to what a
+// working recoverer produces.
+func assertPanicProbeOutput(t *testing.T, stdout, stderr string) {
+	t.Helper()
+
+	result := ""
+
+	var record map[string]any
+
+	for line := range strings.SplitSeq(stdout, "\n") {
+		if after, found := strings.CutPrefix(
+			line, panicChildResultPrefix,
+		); found {
+			result = after
+
+			continue
+		}
+
+		if !strings.HasPrefix(line, `{"time"`) {
+			continue
+		}
+
+		decoded := map[string]any{}
+		if json.Unmarshal([]byte(line), &decoded) != nil {
+			continue
+		}
+
+		if decoded["msg"] == "handler panic" {
+			require.Nil(
+				t, record, "one panic record expected, got two",
+			)
+
+			record = decoded
+
+			assert.LessOrEqual(
+				t, len(line), middleware.MaxPanicLogLineBytes,
+				"the panic record must hold its stated ceiling",
+			)
+
+			t.Logf(
+				"panic record through the shipped chain: %d bytes "+
+					"(ceiling %d)",
+				len(line), middleware.MaxPanicLogLineBytes,
+			)
+		}
+	}
+
+	// What the client got. Under the defect this read
+	// `status=0 err=... EOF`.
+	require.Equal(
+		t, "status=500 err=<nil>", result,
+		"the client must receive a 500, not a dropped connection",
+	)
+
+	// What the operator got. Under the defect there was no such
+	// record: standard output carried net/http reporting chi's own
+	// crash, at INFO, with the original panic value nowhere in it.
+	require.NotNil(
+		t, record,
+		"no structured panic record reached standard output",
+	)
+	assert.Equal(t, "ERROR", record["level"])
+	assert.Equal(t, panicProbeMarker, record["panic"])
+	assert.Equal(t, false, record["response_committed"])
+
+	stack, ok := record["stack"].(string)
+	require.True(t, ok)
+	assert.Contains(t, stack, "panicProbeHandler")
+	assert.NotContains(
+		t, stack, stackTruncationMarker,
+		"the shipped middleware chain's own stack must fit the "+
+			"stack budget without being cut",
+	)
+	t.Logf("stack through the shipped chain: %d bytes", len(stack))
+
+	// The secondary panic, in every form it took. net/http's report
+	// is the tell: it only logs a request when something escaped the
+	// handler chain.
+	assert.NotContains(t, stdout, "http: panic serving")
+	assert.NotContains(t, stdout, "slice bounds out of range")
+	assert.NotContains(t, stdout, "decorateFuncCallLine")
+	assert.Empty(
+		t, strings.TrimSpace(stderr),
+		"nothing may reach standard error",
+	)
+}
+
+// panicProbeHandler is the panicking route the child installs. It is a
+// named function so the stack assertion has something to look for.
+func panicProbeHandler(http.ResponseWriter, *http.Request) {
+	panic(panicProbeMarker)
+}
+
+// TestPanicProbeChild is the child half of the probe above. It runs
+// only when re-executed with panicChildEnv set; in an ordinary run it
+// returns immediately.
+//
+// It writes its result to standard output with a prefix rather than
+// asserting, because the assertions belong to the parent, which is the
+// only side that can see both descriptors.
+func TestPanicProbeChild(t *testing.T) {
+	t.Parallel()
+
+	if os.Getenv(panicChildEnv) == "" {
+		return
+	}
+
+	env := newTestEnv(t)
+
+	router := server.NewRouterWithProbeForTest(
+		env.log.Get(), env.cfg, env.mw, env.hnd,
+		false, panicProbeHandler,
+	)
+
+	srv := httptest.NewServer(router)
+	defer srv.Close()
+
+	req, err := http.NewRequestWithContext(
+		context.Background(), http.MethodGet,
+		srv.URL+server.ProbePattern, nil,
+	)
+	require.NoError(t, err)
+
+	status := 0
+
+	resp, err := srv.Client().Do(req)
+	if err == nil {
+		status = resp.StatusCode
+
+		_ = resp.Body.Close()
+	}
+
+	// Written to the descriptor rather than through the testing
+	// package's own output, because fd 1 is exactly what the parent
+	// is measuring.
+	_, writeErr := fmt.Fprintf(
+		os.Stdout, "%sstatus=%d err=%v\n",
+		panicChildResultPrefix, status, err,
+	)
+	require.NoError(t, writeErr)
+}
+
+// TestSentryStillSeesAPanic pins the relationship the recoverer's
+// placement has to preserve. sentryhttp is registered with
+// Repanic: true, inside the recoverer, so an operator with SENTRY_DSN
+// set keeps the report and the client still gets a 500. Registered the
+// other way round, the SDK would swallow the panic and the recoverer
+// would never see it.
+func TestSentryStillSeesAPanic(t *testing.T) {
+	t.Parallel()
+
+	env := newTestEnv(t)
+
+	transport := &captureTransport{}
+
+	opts := server.SentryClientOptionsForTest(
+		"https://public@sentry.invalid/1", "webhooker-test",
+	)
+	opts.Transport = transport
+
+	client, err := sentry.NewClient(opts)
+	require.NoError(t, err)
+
+	router := server.NewRouterWithProbeForTest(
+		env.log.Get(), env.cfg, env.mw, env.hnd,
+		true, panicProbeHandler,
+	)
+
+	req := httptest.NewRequestWithContext(
+		sentry.SetHubOnContext(
+			context.Background(),
+			sentry.NewHub(client, sentry.NewScope()),
+		),
+		http.MethodGet, server.ProbePattern, nil,
+	)
+
+	w := httptest.NewRecorder()
+	router.ServeHTTP(w, req)
+
+	assert.Equal(
+		t, http.StatusInternalServerError, w.Code,
+		"the recoverer must still answer what sentryhttp re-raised",
+	)
+
+	events := transport.events
+	require.Len(t, events, 1, "Sentry must still see the panic")
+	assert.Equal(t, sentry.LevelFatal, events[0].Level)
+	// The SDK renders a string panic value as the event message
+	// rather than an exception, so the whole payload is checked for
+	// the value rather than one field of it.
+	assert.Contains(
+		t, marshalEvent(t, events[0]), panicProbeMarker,
+	)
+}

internal/server/routes.go

@@ -51,7 +51,6 @@ func (s *Server) SetupRoutes() {
 }
 
 func (s *Server) setupGlobalMiddleware() {
-	s.router.Use(middleware.Recoverer)
 	s.router.Use(middleware.RequestID)
 	s.router.Use(s.mw.SecurityHeaders())
 	s.router.Use(s.mw.Logging())
@@ -64,8 +63,21 @@ func (s *Server) setupGlobalMiddleware() {
 	s.router.Use(s.mw.CORS())
 	s.router.Use(middleware.Timeout(requestTimeout))
 
+	// Panic recovery, deliberately here rather than first. It has to
+	// run inside every middleware that observes the response, so the
+	// 500 it writes is the status the access log records and the
+	// metrics count, and outside the sentryhttp handler below, whose
+	// Repanic option needs something further out to catch what it
+	// re-raises. chi's own middleware.Recoverer held the first slot
+	// until it was measured: on a current Go release it crashes
+	// inside its stack pretty-printer instead of recovering, so the
+	// connection dropped and the original panic was never reported.
+	// See https://git.eeqj.de/sneak/webhooker/issues/187.
+	s.router.Use(s.mw.Recoverer())
+
 	// Sentry error reporting (if SENTRY_DSN is set). Repanic is
-	// true so panics still bubble up to the Recoverer middleware.
+	// true so panics still bubble up to the Recoverer middleware
+	// registered immediately above.
 	if s.sentryEnabled {
 		sentryHandler := sentryhttp.New(sentryhttp.Options{
 			Repanic: true,

internal/server/routes_test.go

@@ -52,6 +52,15 @@ type testEnv struct {
 	sess   *session.Session
 	db     *database.Database
 	dbMgr  *database.WebhookDBManager
+
+	// The collaborators the router was built from, kept so a test
+	// that needs a second router over the same graph — one carrying
+	// a panicking probe route, or one with Sentry registered — can
+	// build it without wiring the graph again.
+	log *logger.Logger
+	cfg *config.Config
+	mw  *middleware.Middleware
+	hnd *handlers.Handlers
 }
 
 // newTestEnv wires the dependency graph with fx and builds the
@@ -100,6 +109,10 @@ func newTestEnv(t *testing.T) *testEnv {
 		sess:   sess,
 		db:     db,
 		dbMgr:  dbMgr,
+		log:    log,
+		cfg:    cfg,
+		mw:     mw,
+		hnd:    hnd,
 	}
 }
 

internal/server/sentry_test.go

@@ -127,12 +127,14 @@ func (c sentryCase) capture(t *testing.T) []*sentry.Event {
 	return transport.events
 }
 
-// router mirrors setupGlobalMiddleware's ordering over the two route
-// patterns these tests need: a recovering middleware first, then the
+// router mirrors the one ordering these tests depend on, over the two
+// route patterns they need: a recovering middleware outside, then the
 // sentryhttp handler registered with Use and Repanic set, exactly as
-// routes.go registers it. The local recover stands in for chi's
-// middleware.Recoverer, which holds that slot in production; it is
-// here only to keep panic stacks out of the test output.
+// routes.go orders the two. The bare recover stands in for
+// Middleware.Recoverer, which holds that outer slot in production; it
+// is here only to keep panic stacks out of the test output. That the
+// production one really does catch what sentryhttp re-raises is
+// pinned separately, by TestSentryStillSeesAPanic.
 func (c sentryCase) router() http.Handler {
 	handler := func(_ http.ResponseWriter, r *http.Request) {
 		// This call is what drains the body tee and fills the