loggingResponseWriter has no Unwrap, so http.ResponseController cannot reach the real writer through the shipped chain #191

New Issue

clawbot · 2026-08-18T04:02:58+02:00

clawbot commented

2026-08-18 04:02:58 +02:00

Found while implementing #187. Pre-existing, not caused by that work. NOT milestoned 1.0.0 — nothing in the tree flushes, hijacks or sets a write deadline today, so it is latent rather than live.

loggingResponseWriter in internal/middleware wraps http.ResponseWriter and implements no Unwrap() http.ResponseWriter. Since Go 1.20, http.ResponseController walks the chain via Unwrap to reach the underlying writer's optional interfaces — Flush, Hijack, SetReadDeadline, SetWriteDeadline. A wrapper without Unwrap terminates that walk, so ResponseController methods return http.ErrNotSupported for any handler behind this middleware, and the type-assertion route (w.(http.Flusher)) fails the same way.

Why it is worth fixing despite being latent: it fails silently and at a distance. The first handler that needs to stream — SSE for live event tailing, a chunked export, anything long-poll — will not fail at the wrapper. It will fail at the handler with a nil or ErrNotSupported, and the cause will be a middleware nobody was looking at. The fix is one method; the debugging session it prevents is not.

The recoverer added by #189 does implement Unwrap on its own writer wrapper, so it is not the gap — but that also means the chain is inconsistent, which is worse than uniformly wrong.

Definition of done

loggingResponseWriter implements Unwrap() http.ResponseWriter.
Every http.ResponseWriter wrapper in the repo is audited for the same gap, not just this one — enumerate what you found, including the ones that already comply. A partial sweep here just relocates the trap.
A test driving http.ResponseController.Flush (or an equivalent optional interface) through the production middleware chain, not a hand-assembled one — the point is that the shipped chain works, and a hand-built stack is exactly how this was missed.
The mutation bites: removing the new Unwrap must fail that test.

Implementation requirements

Branch from next, PR based on next, single commit, title ending (closes #N).
Do not modify TODO.md (see #112).
Run make bootstrap in a fresh clone before gating — browser assets are fetched at build time, and make lint needs Docker.
Gate on make check plus the cache-defeated Docker lint path. All linting runs in Docker, never on the host. Clean up every container and image you start; never run any prune.

Found while implementing https://git.eeqj.de/sneak/webhooker/issues/187. Pre-existing, not caused by that work. NOT milestoned 1.0.0 — nothing in the tree flushes, hijacks or sets a write deadline today, so it is latent rather than live. `loggingResponseWriter` in `internal/middleware` wraps `http.ResponseWriter` and implements no `Unwrap() http.ResponseWriter`. Since Go 1.20, `http.ResponseController` walks the chain via `Unwrap` to reach the underlying writer's optional interfaces — `Flush`, `Hijack`, `SetReadDeadline`, `SetWriteDeadline`. A wrapper without `Unwrap` terminates that walk, so `ResponseController` methods return `http.ErrNotSupported` for any handler behind this middleware, and the type-assertion route (`w.(http.Flusher)`) fails the same way. Why it is worth fixing despite being latent: it fails **silently and at a distance**. The first handler that needs to stream — SSE for live event tailing, a chunked export, anything long-poll — will not fail at the wrapper. It will fail at the handler with a `nil` or `ErrNotSupported`, and the cause will be a middleware nobody was looking at. The fix is one method; the debugging session it prevents is not. The recoverer added by https://git.eeqj.de/sneak/webhooker/pulls/189 does implement `Unwrap` on its own writer wrapper, so it is not the gap — but that also means the chain is inconsistent, which is worse than uniformly wrong. ## Definition of done - `loggingResponseWriter` implements `Unwrap() http.ResponseWriter`. - **Every** `http.ResponseWriter` wrapper in the repo is audited for the same gap, not just this one — enumerate what you found, including the ones that already comply. A partial sweep here just relocates the trap. - A test driving `http.ResponseController.Flush` (or an equivalent optional interface) through the **production** middleware chain, not a hand-assembled one — the point is that the shipped chain works, and a hand-built stack is exactly how this was missed. - The mutation bites: removing the new `Unwrap` must fail that test. ## Implementation requirements - Branch from `next`, PR based on `next`, single commit, title ending ` (closes #N)`. - Do not modify `TODO.md` (see https://git.eeqj.de/sneak/webhooker/issues/112). - Run `make bootstrap` in a fresh clone before gating — browser assets are fetched at build time, and `make lint` needs Docker. - Gate on `make check` plus the cache-defeated Docker lint path. All linting runs in Docker, never on the host. Clean up every container and image you start; never run any prune.

clawbot referenced this issue

2026-08-18 04:16:53 +02:00

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

clawbot referenced this issue

2026-08-18 07:41:51 +02:00

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

clawbot referenced this issue

2026-08-18 08:04:23 +02:00

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

clawbot referenced this issue

2026-08-18 10:50:50 +02:00

Milestone 1.0.0: internet-facing readiness #111