From c87b469dcdce5a19783840deb6ce0db32bab8a61 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Fri, 2 Oct 2026 13:08:54 +0200 Subject: [PATCH] Let a handler flush or set a write deadline through the access log and metrics (closes #191) The access log's response writer and the metrics middleware's writer hid the writer beneath them, so a handler's flush, hijack or write deadline set through http.ResponseController failed with "not supported" behind them. The access log's writer now has Unwrap. The metrics middleware calls the library's public Measure with a writer of our own that has Unwrap, in place of std.Handler's writer, so the middleware order and what metrics record are unchanged. A test over a real connection sets a write deadline and flushes, metrics on and off, on a global route and in an admin page group, and fails without either Unwrap. Model: opus-5-5 --- internal/middleware/metrics.go | 76 ++++++++++++-- internal/middleware/middleware.go | 7 ++ internal/middleware/recoverer_test.go | 8 +- internal/server/response_controller_test.go | 109 ++++++++++++++++++++ 4 files changed, 186 insertions(+), 14 deletions(-) create mode 100644 internal/server/response_controller_test.go diff --git a/internal/middleware/metrics.go b/internal/middleware/metrics.go index 9b8268b..af092e0 100644 --- a/internal/middleware/metrics.go +++ b/internal/middleware/metrics.go @@ -8,7 +8,6 @@ import ( "github.com/go-chi/chi" httpmetrics "github.com/slok/go-http-metrics/metrics" ghmm "github.com/slok/go-http-metrics/middleware" - "github.com/slok/go-http-metrics/middleware/std" ) // inflightHandler is the fixed `handler` label on @@ -169,13 +168,72 @@ func metricsMiddleware( }) return func(next http.Handler) http.Handler { - // The handler id is unmatchedRoute rather than "" so that - // the client-chosen URL path never enters the metrics - // pipeline at all: an empty id is the library's signal to - // substitute it. boundedLabelRecorder overwrites this value - // on every observation, so it is reachable only if that - // decorator is removed — in which case the metrics collapse - // to one series instead of leaking again. - return std.Handler(unmatchedRoute, mdlw, next) + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mw := &metricsResponseWriter{ + ResponseWriter: w, + request: r, + statusCode: http.StatusOK, + } + + // The handler id is unmatchedRoute rather than "" so + // that the client-chosen URL path never enters the + // metrics pipeline at all: an empty id is the library's + // signal to substitute it. boundedLabelRecorder + // overwrites this value on every observation, so it is + // reachable only if that decorator is removed — in which + // case the metrics collapse to one series instead of + // leaking again. + mdlw.Measure(unmatchedRoute, mw, func() { + next.ServeHTTP(mw, r) + }) + }) } } + +// metricsResponseWriter records the status code and body size of a +// response, and hands them with the request to go-http-metrics' +// Measure as its Reporter. +// +// It stands in for the library's std.Handler, whose writer has no +// Unwrap: behind it, http.ResponseController cannot reach net/http's +// own writer, so a handler's write deadline fails with metrics on. +type metricsResponseWriter struct { + http.ResponseWriter + + request *http.Request + statusCode int + bytesWritten int64 +} + +func (w *metricsResponseWriter) WriteHeader(code int) { + w.statusCode = code + + w.ResponseWriter.WriteHeader(code) +} + +func (w *metricsResponseWriter) Write(b []byte) (int, error) { + w.bytesWritten += int64(len(b)) + + //nolint:wrapcheck // Pass the writer's own error through unchanged. + return w.ResponseWriter.Write(b) +} + +// Unwrap lets http.ResponseController reach the writer underneath, so +// a handler can still flush or set a write deadline with metrics on. +func (w *metricsResponseWriter) Unwrap() http.ResponseWriter { + return w.ResponseWriter +} + +func (w *metricsResponseWriter) Method() string { return w.request.Method } + +func (w *metricsResponseWriter) Context() context.Context { + return w.request.Context() +} + +func (w *metricsResponseWriter) URLPath() string { return w.request.URL.Path } + +func (w *metricsResponseWriter) StatusCode() int { return w.statusCode } + +func (w *metricsResponseWriter) BytesWritten() int64 { return w.bytesWritten } + +var _ ghmm.Reporter = (*metricsResponseWriter)(nil) diff --git a/internal/middleware/middleware.go b/internal/middleware/middleware.go index cc9cbe6..7552a4a 100644 --- a/internal/middleware/middleware.go +++ b/internal/middleware/middleware.go @@ -233,6 +233,13 @@ func (lrw *loggingResponseWriter) WriteHeader(code int) { lrw.ResponseWriter.WriteHeader(code) } +// Unwrap lets http.ResponseController reach the writer underneath, so +// a handler can still flush or set a write deadline through the access +// log. +func (lrw *loggingResponseWriter) Unwrap() http.ResponseWriter { + return lrw.ResponseWriter +} + // concreteLogURL renders the request's own URL for the access log // branches that keep it, with the query string replaced by a fixed // marker. diff --git a/internal/middleware/recoverer_test.go b/internal/middleware/recoverer_test.go index 485d6e1..157c1fb 100644 --- a/internal/middleware/recoverer_test.go +++ b/internal/middleware/recoverer_test.go @@ -627,11 +627,9 @@ func TestRecovererIgnoresANonPanickingHandler(t *testing.T) { // net/http's own writer from http.ResponseController, so a handler // that flushes or sets a deadline starts failing. // -// The recoverer is the only middleware in the chain here. The access -// logger's own wrapper does not implement Unwrap, so a chain -// containing it fails this regardless of what the recoverer does; -// what is being pinned is that the recoverer adds no such opacity of -// its own. +// The recoverer is the only middleware in the chain here; +// TestResponseControllerThroughProductionRouter in internal/server +// covers the shipped chain. func TestRecovererKeepsResponseControllerWorking(t *testing.T) { t.Parallel() diff --git a/internal/server/response_controller_test.go b/internal/server/response_controller_test.go new file mode 100644 index 0000000..ee08889 --- /dev/null +++ b/internal/server/response_controller_test.go @@ -0,0 +1,109 @@ +package server_test + +import ( + "fmt" + "io" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sneak.berlin/go/webhooker/internal/server" +) + +// TestResponseControllerThroughProductionRouter sets a write deadline +// and flushes through http.ResponseController, behind the shipped +// router and over a real connection, and checks that both reach +// net/http's own writer. +// +// Every middleware that wraps the writer has to let them through with +// an Unwrap method. One that does not makes the call return +// http.ErrNotSupported, or, if it has a Flush of its own that cannot +// reach further in, makes the flush silently do nothing; either way +// the handler that trips over it is far from the cause. +// +// It runs once with the defaults and once with metrics and Sentry on, +// because those two add middleware to the chain, and through both the +// global middleware and an admin page route group, which adds its own. +func TestResponseControllerThroughProductionRouter(t *testing.T) { + t.Parallel() + + // Without /metrics credentials, metricsConfig is the default + // Config. + cases := []struct { + name string + username, password string + sentryEnabled bool + }{ + {name: "defaults"}, + { + name: "metrics and Sentry on", + username: metricsUser, password: metricsAuthValue, + sentryEnabled: true, + }, + } + + // The probe answers with what each call returned. + probe := func(w http.ResponseWriter, _ *http.Request) { + rc := http.NewResponseController(w) + + deadlineErr := rc.SetWriteDeadline(time.Now().Add(time.Minute)) + flushErr := rc.Flush() + + _, _ = fmt.Fprintf( + w, "deadline: %v, flush: %v", deadlineErr, flushErr, + ) + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + env := newTestEnvWithConfig( + t, metricsConfig(t, tc.username, tc.password), + ) + + routers := map[string]http.Handler{ + server.ProbePattern: server.NewRouterWithProbeForTest( + env.log.Get(), env.cfg, env.mw, env.hnd, + tc.sentryEnabled, probe, + ), + server.PageProbePattern: server.NewRouterWithPageProbeForTest( + env.log.Get(), env.cfg, env.mw, env.hnd, + tc.sentryEnabled, probe, + ), + } + + for path, router := range routers { + srv := httptest.NewServer(router) + t.Cleanup(srv.Close) + + req, err := http.NewRequestWithContext( + t.Context(), http.MethodGet, srv.URL+path, nil, + ) + require.NoError(t, err) + + resp, err := srv.Client().Do(req) + require.NoError(t, err) + + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + + assert.Equal( + t, "deadline: , flush: ", string(body), path, + ) + + // A response the server holds until the handler returns + // goes out with a Content-Length; one flushed while the + // handler is still running goes out in chunks. + assert.Equal( + t, []string{"chunked"}, resp.TransferEncoding, + "%s: the flush must reach the client", path, + ) + } + }) + } +}