Report handler panics through the logger and answer 500 (closes #187)
All checks were successful
check / check (push) Successful in 2m56s

chi v1.5.5's middleware.Recoverer neither logged a handler panic nor
answered 500. Its pretty-printer scans the stack for a frame beginning
"panic(0x", which the runtime no longer emits, so the scan never
terminates early and every line reaches decorateFuncCallLine, which
slices pkg[strings.Index(pkg, "."):] without checking for -1. That
second panic escaped chi's own deferred function, so its
WriteHeader(500) never ran: net/http closed the connection and reported
its own crash, losing the original panic value entirely.

Middleware.Recoverer replaces it. It writes one ERROR record through
internal/logger carrying the panic value, the stack and the request id,
and answers 500. http.ErrAbortHandler is re-panicked rather than
swallowed, and a response the handler already committed is left alone
rather than overwritten.

It is registered inside every middleware that observes the response, so
the 500 is the status the access log records and the metrics count, and
outside the sentryhttp handler, whose Repanic option needs something
further out to catch what it re-raises.

Both fields are bounded in encoded bytes, through the same
internal/logfield budget the access log spends: 512 for the panic value,
since a handler may build one out of the request, and 8192 for the
stack, cut at its far end so the panic site survives.
MaxPanicLogLineBytes states the resulting ceiling at 10240; measured,
the widest line either handler produces is 8898, a figure that carries
no source paths and reproduces across checkouts. The real case through
the shipped chain measures roughly 3960 bytes; that one moves with the
checkout, because debug.Stack() embeds absolute source paths, so it is
stated as a measurement rather than as an invariant and no test asserts
it.

Because the panic record no longer reaches net/http's error log, the
carve-outs in README.md and in the MaxAccessLogLineBytes doc comment
that described that path are removed rather than reworded. What
replaces them states the ceiling the record is now written under, and
internal/server/recoverer_test.go asserts that "http: panic serving"
appears in neither of the process's streams.
This commit is contained in:
2026-08-18 01:57:55 +00:00
committed by sneak
parent 0c64c411cc
commit ea1733615d
10 changed files with 1218 additions and 46 deletions

View File

@@ -1144,10 +1144,10 @@ including cases built from the characters the handlers escape, and
against the widest access log line the service can be made to write: a
5xx that keeps its concrete path while all three header fields are also
at their budget. Every case runs through both handlers
`internal/logger` can select — the JSON one and the text one it
installs on a tty — since the
two do not escape alike and the ceiling is quoted unqualified. Measured
over a real connection, the widest line is 1,972 bytes.
`internal/logger` can select — the JSON one and the text one it installs
on a tty — since the two do not escape alike and the ceiling is quoted
unqualified. Measured over a real connection, the widest access log line
is 1,972 bytes.
Multiply that ceiling by the request rate to size log storage. Note
that the rate is not bounded by the limits above on every route:
@@ -1280,23 +1280,46 @@ read as more than it is:
`internal/logger` calls `slog.SetDefault`, which redirects that logger
into whichever handler it installed. Those lines therefore arrive on
standard output, shaped like every other line, at `INFO`. They are not
truncated and they are not bounded by the ceiling: a handler panic
arrives as one record carrying a whole goroutine stack, above the
ceiling's 2,560 bytes — measured at roughly 2,770 in one checkout. The
exact width is not an invariant, since it moves with the goroutine
number and with the source paths baked into the stack; that it exceeds
the ceiling does not move. The value is the runtime's, not a client's.
- **A handler panic reaches that path** rather than the one it looks
like it should. `internal/server/routes.go` installs chi's
`middleware.Recoverer` in front of every route, which is meant to
print the panic and its stack to standard error and answer 500. On the
Go version this service builds against it does neither: chi v1.5.5's
stack pretty-printer looks for a `panic(0x` frame that the runtime no
longer emits, walks past the end of its own slice, and panics before
writing a byte. That second panic escapes to `net/http`, which drops
the connection and reports it through the nil `ErrorLog` above.
Tracked separately in
<https://git.eeqj.de/sneak/webhooker/issues/187>.
truncated. A handler panic is no longer one of them: the recover
middleware below answers it and writes it as the bounded record
described there instead, and `internal/server/recoverer_test.go`
requires that `http: panic serving` appear in neither of the process's
two streams when a panic is driven through the production router. The
one panic still handed back to `net/http` is `http.ErrAbortHandler`,
which it special-cases and does not log at all. What is left on this
path is `net/http`'s own diagnostics, whose values are the runtime's,
not a client's.
Wider than that 2,560-byte ceiling, and stated separately rather than
carved out of it: the record a recovered panic produces. The recover
middleware in `internal/middleware` answers `500` and writes one `ERROR`
record through `internal/logger` carrying the panic value, the stack and
the request id — the same `request_id` the access log line for that
request carries, which is how the two are joined. It replaced chi's
`middleware.Recoverer`, which on a current Go release crashed inside its
own stack pretty-printer: the connection was dropped rather than
answered, and what reached the operator described that crash rather than
the fault behind it.
That record is bounded the same way, in the same encoded bytes and
through the same `internal/logfield` budget: 512 for the panic value,
because a handler is free to build one out of the request, and 8,192 for
the stack, cut at its far end so that the panic site survives a cut and
`net/http`'s accept frames are what is lost. Net: **at most 10,240
bytes, once per recovered panic** — 9,121 by the arithmetic (523 + 8,203
+ 139 + a 256-byte fixed portion), stated at 10,240 for headroom.
`internal/middleware/recoverer_test.go` measures 8,898 bytes with the
stack and the panic value both driven past their budgets, over both
handlers; that figure carries no source paths and reproduces across
checkouts. The real case is far below it: through the shipped middleware
chain the whole record measures roughly 3,960 bytes over a roughly
3,690-byte stack, taken by `internal/server/recoverer_test.go` from the
process's own file descriptors while driving a panic through the
production router over a real server in a subprocess. That pair is
**not** an invariant — `debug.Stack()` embeds absolute source paths, so
it moves with where the tree is checked out, and three checkouts have
reported 3,959, 3,961 and 4,026. What the tests assert is the ceiling
and that the stack arrived uncut, never the number.
Every limiter here — receiver, login, and password change — identifies
the client the same way, through one shared key function: the
@@ -1635,19 +1658,32 @@ to record results.
Applied to all routes in this order:
1. **Recoverer**Panic recovery (chi built-in)
2. **RequestID** — Generate unique request IDs (chi built-in)
3. **SecurityHeaders** — Production security headers on every response
1. **RequestID**Generate unique request IDs (chi built-in)
2. **SecurityHeaders** — Production security headers on every response
(HSTS, X-Content-Type-Options, X-Frame-Options, CSP, Referrer-Policy,
Permissions-Policy)
4. **Logging** — Structured request logging (method, URL, status,
3. **Logging** — Structured request logging (method, URL, status,
latency, remote IP, user agent, request ID)
5. **Metrics** — Prometheus HTTP metrics (if `METRICS_USERNAME` is set)
6. **CORS** — Cross-origin resource sharing headers
7. **Timeout** — 60-second request timeout
4. **Metrics** — Prometheus HTTP metrics (if `METRICS_USERNAME` is set)
5. **CORS** — Cross-origin resource sharing headers
6. **Timeout** — 60-second request timeout
7. **Recoverer** — Panic recovery: one `ERROR` record through
`internal/logger` and a `500`
8. **Sentry** — Error reporting to Sentry (if `SENTRY_DSN` is set;
configured with `Repanic: true` so panics still reach Recoverer)
Recoverer sits seventh rather than first, and both neighbours are the
reason. It runs **inside** everything that observes the response, so
the `500` it writes for a panicking handler is the status the access
log records and the metrics count; registered first, as chi's own
`middleware.Recoverer` was, the same request was logged as a `200` that
the client never received. It runs **outside** the Sentry handler, so
`Repanic: true` has something to re-raise into: an operator with
`SENTRY_DSN` set keeps the report, and one without it now gets the
local record instead of nothing. What that placement gives up is
recovery of a panic in the six entries above it, none of which does
more than set a header or start a timer.
Additionally, form endpoints (`/pages`, `/user/*`, `/sources`,
`/source/*`) apply a **MaxBodySize** middleware that limits
POST/PUT/PATCH request bodies to 1 MB. It is registered ahead of the