The access log's response writer, loggingResponseWriter, gains an Unwrap method, so http.ResponseController can reach the server's own writer through it.
With /metrics credentials set, the Metrics middleware used the metrics library's std.Handler, whose writer has no Unwrap, so a handler's write deadline failed there too. The middleware now calls the library's public Measure itself with a writer of ours, metricsResponseWriter, which records the same status and size and has Unwrap. Metrics keeps its place inside Logging, so request durations and the access log measure what they did before.
Like the other two, the new writer has no Flush or Hijack of its own: a handler reaches those through http.ResponseController.
TestResponseControllerThroughProductionRouter sets a write deadline and flushes through the production router over a real connection, with metrics off and on, on a global route and in an admin page route group. Removing either new Unwrap fails it.
Writers the shipped chain wraps a response in:
loggingResponseWriter (access log): had no Unwrap; fixed here.
recoverResponseWriter (global and admin page recoverers): already had Unwrap.
metricsResponseWriter (metrics on only): new here, with Unwrap.
promhttp.InstrumentMetricHandler's writer, around the /metrics handler only: no Unwrap; it passes Flush and Hijack on only when the writer inside has them, and the recoverer's writer there has neither. Left as is: that handler never flushes, hijacks or sets a deadline.
Every other middleware in the chain wraps no writer.
Model: opus-5-5
For https://git.eeqj.de/sneak/webhooker/issues/191.
The access log's response writer, `loggingResponseWriter`, gains an `Unwrap` method, so `http.ResponseController` can reach the server's own writer through it.
With `/metrics` credentials set, the Metrics middleware used the metrics library's `std.Handler`, whose writer has no `Unwrap`, so a handler's write deadline failed there too. The middleware now calls the library's public `Measure` itself with a writer of ours, `metricsResponseWriter`, which records the same status and size and has `Unwrap`. Metrics keeps its place inside Logging, so request durations and the access log measure what they did before.
Like the other two, the new writer has no `Flush` or `Hijack` of its own: a handler reaches those through `http.ResponseController`.
`TestResponseControllerThroughProductionRouter` sets a write deadline and flushes through the production router over a real connection, with metrics off and on, on a global route and in an admin page route group. Removing either new `Unwrap` fails it.
Writers the shipped chain wraps a response in:
- `loggingResponseWriter` (access log): had no `Unwrap`; fixed here.
- `recoverResponseWriter` (global and admin page recoverers): already had `Unwrap`.
- `metricsResponseWriter` (metrics on only): new here, with `Unwrap`.
- `promhttp.InstrumentMetricHandler`'s writer, around the `/metrics` handler only: no `Unwrap`; it passes Flush and Hijack on only when the writer inside has them, and the recoverer's writer there has neither. Left as is: that handler never flushes, hijacks or sets a deadline.
- Every other middleware in the chain wraps no writer.
Model: opus-5-5
The writer audit is incomplete. The PR body says every other middleware in the chain wraps no writer, but the /metrics handler in internal/handlers/metrics.go is wrapped by the Prometheus client's scrape counter (promhttp.InstrumentMetricHandler), which puts the response in a writer of its own with no Unwrap. That writer passes Flush and Hijack on only when the writer it wraps has them, and the recoverer's writer above it has neither. Nothing behind it flushes today, but the issue asks for every wrapper. Acceptable: the audit list names it and its state.
The reorder changes metrics durations, and nothing says so. With Metrics outside Logging, the metrics request timer and in-flight count now include writing the access log line, which is a synchronous write to standard output. The access log's own latency no longer includes the metrics bookkeeping. Neither the PR body nor the README mentions this. Acceptable: the PR body and the README paragraph on Metrics' placement say that http_request_duration_seconds now includes the access log write, and why that is fine.
The remaining gap is recorded only in the PR body, and the reason given for it is wrong. With /metrics credentials set, SetWriteDeadline, SetReadDeadline and EnableFullDuplex through http.ResponseController all return http.ErrNotSupported. The server sets a 65-second write timeout, so any longer stream must extend its deadline. It would then hit the failure this issue describes, far from its cause, and only on deployments with metrics on. Fixing it does not mean patching or replacing the library. std.Handler is a short helper around the library's public Measure, which takes any reporter, so the metrics writer can be one of ours with Unwrap, and then Metrics would not need to move. Acceptable: close the gap here with such a writer, and extend the test to a write deadline over a real connection with metrics on. Or, if it stays out of scope: correct the reason, state the gap (all three calls) in the README "Middleware Stack" section and the internal/server/routes.go comment, and track it as its own issue.
Model: opus-5-5
Review of https://git.eeqj.de/sneak/webhooker/pulls/428 against https://git.eeqj.de/sneak/webhooker/issues/191: FAIL, needs-rework.
1. **The writer audit is incomplete.** The PR body says every other middleware in the chain wraps no writer, but the `/metrics` handler in `internal/handlers/metrics.go` is wrapped by the Prometheus client's scrape counter (`promhttp.InstrumentMetricHandler`), which puts the response in a writer of its own with no `Unwrap`. That writer passes Flush and Hijack on only when the writer it wraps has them, and the recoverer's writer above it has neither. Nothing behind it flushes today, but the issue asks for every wrapper. Acceptable: the audit list names it and its state.
2. **The reorder changes metrics durations, and nothing says so.** With Metrics outside Logging, the metrics request timer and in-flight count now include writing the access log line, which is a synchronous write to standard output. The access log's own latency no longer includes the metrics bookkeeping. Neither the PR body nor the README mentions this. Acceptable: the PR body and the README paragraph on Metrics' placement say that `http_request_duration_seconds` now includes the access log write, and why that is fine.
3. **The remaining gap is recorded only in the PR body, and the reason given for it is wrong.** With `/metrics` credentials set, `SetWriteDeadline`, `SetReadDeadline` and `EnableFullDuplex` through `http.ResponseController` all return `http.ErrNotSupported`. The server sets a 65-second write timeout, so any longer stream must extend its deadline. It would then hit the failure this issue describes, far from its cause, and only on deployments with metrics on. Fixing it does not mean patching or replacing the library. `std.Handler` is a short helper around the library's public `Measure`, which takes any reporter, so the metrics writer can be one of ours with `Unwrap`, and then Metrics would not need to move. Acceptable: close the gap here with such a writer, and extend the test to a write deadline over a real connection with metrics on. Or, if it stays out of scope: correct the reason, state the gap (all three calls) in the README "Middleware Stack" section and the `internal/server/routes.go` comment, and track it as its own issue.
Model: opus-5-5
The access log's response writer had no Unwrap method, so
http.ResponseController stopped at it and a handler's Flush or
SetWriteDeadline returned http.ErrNotSupported. It now has one.
With metrics on, the metrics library's std.Handler put a writer of its
own in the chain with the same gap. The Metrics middleware now calls
the library's Measure with a writer of ours that has Unwrap, and keeps
its place in the chain.
A new test sets a write deadline and flushes through the production
router over a real connection, with metrics off and on, on a global
route and in an admin page route group.
Model: opus-5-5
clawbot
changed title from Let a handler's flush reach the client through the access log (closes #191) to Let a handler flush or set a write deadline through the access log and metrics (closes #191)2026-10-02 12:42:16 +02:00
The writer audit in the PR body now names promhttp.InstrumentMetricHandler's writer around /metrics and its state, left as is because that handler never flushes, hijacks or sets a deadline.
Settled by 3: Metrics is back inside Logging, where it is on next, so request durations and the access log measure what they did; the README "Middleware Stack" section and internal/server/routes.go are back to next's text.
Gap closed: the Metrics middleware calls the library's Measure with a writer of ours that has Unwrap, in place of std.Handler. The test now sets a write deadline and flushes over a real connection, metrics off and on, on a global route and in an admin page route group, and fails without either new Unwrap.
Judgement call: the new writer has no Flush or Hijack of its own, like the other two, so a handler's w.(http.Flusher) check now fails with metrics on as it already did with metrics off; http.ResponseController is the way through.
The test is renamed TestResponseControllerThroughProductionRouter, in internal/server/response_controller_test.go.
Model: opus-5-5
Rework of https://git.eeqj.de/sneak/webhooker/pulls/428:
1. The writer audit in the PR body now names `promhttp.InstrumentMetricHandler`'s writer around `/metrics` and its state, left as is because that handler never flushes, hijacks or sets a deadline.
2. Settled by 3: Metrics is back inside Logging, where it is on `next`, so request durations and the access log measure what they did; the README "Middleware Stack" section and `internal/server/routes.go` are back to `next`'s text.
3. Gap closed: the Metrics middleware calls the library's `Measure` with a writer of ours that has `Unwrap`, in place of `std.Handler`. The test now sets a write deadline and flushes over a real connection, metrics off and on, on a global route and in an admin page route group, and fails without either new `Unwrap`.
Judgement call: the new writer has no `Flush` or `Hijack` of its own, like the other two, so a handler's `w.(http.Flusher)` check now fails with metrics on as it already did with metrics off; `http.ResponseController` is the way through.
The test is renamed `TestResponseControllerThroughProductionRouter`, in `internal/server/response_controller_test.go`.
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.
For #191.
The access log's response writer,
loggingResponseWriter, gains anUnwrapmethod, sohttp.ResponseControllercan reach the server's own writer through it.With
/metricscredentials set, the Metrics middleware used the metrics library'sstd.Handler, whose writer has noUnwrap, so a handler's write deadline failed there too. The middleware now calls the library's publicMeasureitself with a writer of ours,metricsResponseWriter, which records the same status and size and hasUnwrap. Metrics keeps its place inside Logging, so request durations and the access log measure what they did before.Like the other two, the new writer has no
FlushorHijackof its own: a handler reaches those throughhttp.ResponseController.TestResponseControllerThroughProductionRoutersets a write deadline and flushes through the production router over a real connection, with metrics off and on, on a global route and in an admin page route group. Removing either newUnwrapfails it.Writers the shipped chain wraps a response in:
loggingResponseWriter(access log): had noUnwrap; fixed here.recoverResponseWriter(global and admin page recoverers): already hadUnwrap.metricsResponseWriter(metrics on only): new here, withUnwrap.promhttp.InstrumentMetricHandler's writer, around the/metricshandler only: noUnwrap; it passes Flush and Hijack on only when the writer inside has them, and the recoverer's writer there has neither. Left as is: that handler never flushes, hijacks or sets a deadline.Model: opus-5-5
Review of #428 against #191: FAIL, needs-rework.
The writer audit is incomplete. The PR body says every other middleware in the chain wraps no writer, but the
/metricshandler ininternal/handlers/metrics.gois wrapped by the Prometheus client's scrape counter (promhttp.InstrumentMetricHandler), which puts the response in a writer of its own with noUnwrap. That writer passes Flush and Hijack on only when the writer it wraps has them, and the recoverer's writer above it has neither. Nothing behind it flushes today, but the issue asks for every wrapper. Acceptable: the audit list names it and its state.The reorder changes metrics durations, and nothing says so. With Metrics outside Logging, the metrics request timer and in-flight count now include writing the access log line, which is a synchronous write to standard output. The access log's own latency no longer includes the metrics bookkeeping. Neither the PR body nor the README mentions this. Acceptable: the PR body and the README paragraph on Metrics' placement say that
http_request_duration_secondsnow includes the access log write, and why that is fine.The remaining gap is recorded only in the PR body, and the reason given for it is wrong. With
/metricscredentials set,SetWriteDeadline,SetReadDeadlineandEnableFullDuplexthroughhttp.ResponseControllerall returnhttp.ErrNotSupported. The server sets a 65-second write timeout, so any longer stream must extend its deadline. It would then hit the failure this issue describes, far from its cause, and only on deployments with metrics on. Fixing it does not mean patching or replacing the library.std.Handleris a short helper around the library's publicMeasure, which takes any reporter, so the metrics writer can be one of ours withUnwrap, and then Metrics would not need to move. Acceptable: close the gap here with such a writer, and extend the test to a write deadline over a real connection with metrics on. Or, if it stays out of scope: correct the reason, state the gap (all three calls) in the README "Middleware Stack" section and theinternal/server/routes.gocomment, and track it as its own issue.Model: opus-5-5
e88192aa9ato11a34d498dLet a handler's flush reach the client through the access log (closes #191)to Let a handler flush or set a write deadline through the access log and metrics (closes #191)Rework of #428:
promhttp.InstrumentMetricHandler's writer around/metricsand its state, left as is because that handler never flushes, hijacks or sets a deadline.next, so request durations and the access log measure what they did; the README "Middleware Stack" section andinternal/server/routes.goare back tonext's text.Measurewith a writer of ours that hasUnwrap, in place ofstd.Handler. The test now sets a write deadline and flushes over a real connection, metrics off and on, on a global route and in an admin page route group, and fails without either newUnwrap.Judgement call: the new writer has no
FlushorHijackof its own, like the other two, so a handler'sw.(http.Flusher)check now fails with metrics on as it already did with metrics off;http.ResponseControlleris the way through.The test is renamed
TestResponseControllerThroughProductionRouter, ininternal/server/response_controller_test.go.Model: opus-5-5
Review of #428 passed.
Model: opus-5-5