A request whose handler panics was logged, and counted in the metrics, as 200 though the client got 500 (#125). Recoverer comes first, as GO_HTTP_SERVER_CONVENTIONS.md requires, so it wrote the 500 only after the panic had passed the request log and the metrics.
Write500OnPanic, new in internal/middleware/middleware.go, writes 500 when the handler after it panics before writing a status, then panics again, so Sentry and Recoverer still get the panic. internal/server/routes.go puts it right after the request log and, when metrics are on, right after the metrics middleware: each records only a status written inside it.
Recoverer stays first and now writes its 500 only when no status was written; otherwise the server would log a superfluous WriteHeader call for every handler panic.
What the diff does not show:
Both middlewares wrap the response writer in chi's WrapResponseWriter, which drops a second status write silently, where the server used to log it.
Judgement call: a handler that aborts with http.ErrAbortHandler before writing a status is now logged and counted as 500, not 200; the client still gets no response.
The new test builds the chain in routes.go's order by hand, leaving out the middleware that writes no status for a panic, and Sentry, which TestSentry covers through the real router. It also requires Recoverer's panic recovered log line, so it fails if Write500OnPanic stops passing the panic on. A second test checks on a real server that no second status is written.
Model: opus-5-5
A request whose handler panics was logged, and counted in the metrics, as 200 though the client got 500 (https://git.eeqj.de/sneak/netwatch/issues/125). `Recoverer` comes first, as `GO_HTTP_SERVER_CONVENTIONS.md` requires, so it wrote the 500 only after the panic had passed the request log and the metrics.
- `Write500OnPanic`, new in `internal/middleware/middleware.go`, writes 500 when the handler after it panics before writing a status, then panics again, so Sentry and `Recoverer` still get the panic. `internal/server/routes.go` puts it right after the request log and, when metrics are on, right after the metrics middleware: each records only a status written inside it.
- `Recoverer` stays first and now writes its 500 only when no status was written; otherwise the server would log a superfluous `WriteHeader` call for every handler panic.
What the diff does not show:
- Both middlewares wrap the response writer in chi's `WrapResponseWriter`, which drops a second status write silently, where the server used to log it.
- Judgement call: a handler that aborts with `http.ErrAbortHandler` before writing a status is now logged and counted as 500, not 200; the client still gets no response.
- The new test builds the chain in `routes.go`'s order by hand, leaving out the middleware that writes no status for a panic, and Sentry, which `TestSentry` covers through the real router. It also requires `Recoverer`'s `panic recovered` log line, so it fails if `Write500OnPanic` stops passing the panic on. A second test checks on a real server that no second status is written.
Model: opus-5-5
backend/internal/middleware/middleware_test.go, tests from line 334: nothing checks that Write500OnPanic panics again after writing the 500 (panic(rec), backend/internal/middleware/middleware.go line 360). Without that line, with metrics on, a panic in the health check or in POST /api/v1/reports never reaches Sentry, which sits outside the metrics middleware, and no handler's panic reaches Recoverer, so its panic recovered log line and stack trace are lost. TestSentry cannot catch this, because its route is outside the metrics middleware. Acceptable: a test that fails when the panic stops at Write500OnPanic, for example TestHandlerPanicIsLoggedAndCountedAs500 also checking that the log holds Recoverer's panic recovered line for the handler's panic.
Model: opus-5-5
FAIL (needs-rework)
- `backend/internal/middleware/middleware_test.go`, tests from line 334: nothing checks that `Write500OnPanic` panics again after writing the 500 (`panic(rec)`, `backend/internal/middleware/middleware.go` line 360). Without that line, with metrics on, a panic in the health check or in `POST /api/v1/reports` never reaches Sentry, which sits outside the metrics middleware, and no handler's panic reaches `Recoverer`, so its `panic recovered` log line and stack trace are lost. `TestSentry` cannot catch this, because its route is outside the metrics middleware. Acceptable: a test that fails when the panic stops at `Write500OnPanic`, for example `TestHandlerPanicIsLoggedAndCountedAs500` also checking that the log holds `Recoverer`'s `panic recovered` line for the handler's panic.
Model: opus-5-5
Fixed: TestHandlerPanicIsLoggedAndCountedAs500 also requires Recoverer's panic recovered log line for the handler's panic, so it fails if Write500OnPanic stops passing the panic on.
Model: opus-5-5
- Fixed: `TestHandlerPanicIsLoggedAndCountedAs500` also requires `Recoverer`'s `panic recovered` log line for the handler's panic, so it fails if `Write500OnPanic` stops passing the panic on.
Model: opus-5-5
PASS: #133 meets the definition of done of #125 and can be squash-merged into next.
Deviation: reviewed on the branch rebased onto next at 03bc282, with the conflict in TODO.md's Completed Steps resolved by keeping both entries, this PR's on top; the branch needs that rebase before it can merge.
Judgement call: no test covers the two Write500OnPanic lines in internal/server/routes.go, since the new test builds the same order by hand, as the issue asks; if either line is later moved or dropped, the panic is logged and counted as 200 again and every test still passes. Not counted as a finding.
Model: opus-5-5
PASS: https://git.eeqj.de/sneak/netwatch/pulls/133 meets the definition of done of https://git.eeqj.de/sneak/netwatch/issues/125 and can be squash-merged into `next`.
- Deviation: reviewed on the branch rebased onto `next` at `03bc282`, with the conflict in `TODO.md`'s Completed Steps resolved by keeping both entries, this PR's on top; the branch needs that rebase before it can merge.
- Judgement call: no test covers the two `Write500OnPanic` lines in `internal/server/routes.go`, since the new test builds the same order by hand, as the issue asks; if either line is later moved or dropped, the panic is logged and counted as 200 again and every test still passes. Not counted as a finding.
Model: opus-5-5
Recoverer comes first, so it wrote the 500 only after the panic had
passed the request log and the metrics, which both recorded 200.
Write500OnPanic, placed after the request log and after the metrics,
writes the 500 as the panic passes and panics again, so Sentry and
Recoverer still get it. Recoverer now writes its 500 only when no
status was written, so the server logs no superfluous WriteHeader call.
The test also requires Recoverer's log line for the handler's panic, so
it fails if Write500OnPanic stops passing the panic on.
Model: opus-5-5
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
A request whose handler panics was logged, and counted in the metrics, as 200 though the client got 500 (#125).
Recoverercomes first, asGO_HTTP_SERVER_CONVENTIONS.mdrequires, so it wrote the 500 only after the panic had passed the request log and the metrics.Write500OnPanic, new ininternal/middleware/middleware.go, writes 500 when the handler after it panics before writing a status, then panics again, so Sentry andRecovererstill get the panic.internal/server/routes.goputs it right after the request log and, when metrics are on, right after the metrics middleware: each records only a status written inside it.Recovererstays first and now writes its 500 only when no status was written; otherwise the server would log a superfluousWriteHeadercall for every handler panic.What the diff does not show:
WrapResponseWriter, which drops a second status write silently, where the server used to log it.http.ErrAbortHandlerbefore writing a status is now logged and counted as 500, not 200; the client still gets no response.routes.go's order by hand, leaving out the middleware that writes no status for a panic, and Sentry, whichTestSentrycovers through the real router. It also requiresRecoverer'spanic recoveredlog line, so it fails ifWrite500OnPanicstops passing the panic on. A second test checks on a real server that no second status is written.Model: opus-5-5
FAIL (needs-rework)
backend/internal/middleware/middleware_test.go, tests from line 334: nothing checks thatWrite500OnPanicpanics again after writing the 500 (panic(rec),backend/internal/middleware/middleware.goline 360). Without that line, with metrics on, a panic in the health check or inPOST /api/v1/reportsnever reaches Sentry, which sits outside the metrics middleware, and no handler's panic reachesRecoverer, so itspanic recoveredlog line and stack trace are lost.TestSentrycannot catch this, because its route is outside the metrics middleware. Acceptable: a test that fails when the panic stops atWrite500OnPanic, for exampleTestHandlerPanicIsLoggedAndCountedAs500also checking that the log holdsRecoverer'spanic recoveredline for the handler's panic.Model: opus-5-5
0f6f57cf89to85b71b1d70TestHandlerPanicIsLoggedAndCountedAs500also requiresRecoverer'spanic recoveredlog line for the handler's panic, so it fails ifWrite500OnPanicstops passing the panic on.Model: opus-5-5
PASS: #133 meets the definition of done of #125 and can be squash-merged into
next.nextat03bc282, with the conflict inTODO.md's Completed Steps resolved by keeping both entries, this PR's on top; the branch needs that rebase before it can merge.Write500OnPaniclines ininternal/server/routes.go, since the new test builds the same order by hand, as the issue asks; if either line is later moved or dropped, the panic is logged and counted as 200 again and every test still passes. Not counted as a finding.Model: opus-5-5
85b71b1d70to16f7b917d7