From e88192aa9a0287dd58215b56289b1346fb474e7c Mon Sep 17 00:00:00 2001 From: sneak Date: Fri, 2 Oct 2026 08:46:08 +0000 Subject: [PATCH] Let a handler's flush reach the client through the access log (closes #191) The access log's response writer had no Unwrap method, so http.ResponseController stopped at it and a handler's Flush returned http.ErrNotSupported. It now has one. With metrics on, adding Unwrap alone is not enough: go-http-metrics' writer only passes a flush on when the writer inside it has a Flush method, and it sat inside Logging, so the flush silently did nothing. Metrics is now registered outside Logging, and the README's middleware list follows. A new test flushes through the production router, with the defaults and with metrics and Sentry on, on a global route and inside an admin page route group. Model: opus-5-5 --- README.md | 10 ++- internal/middleware/middleware.go | 7 ++ internal/middleware/recoverer_test.go | 8 +-- internal/server/flush_test.go | 96 +++++++++++++++++++++++++++ internal/server/routes.go | 7 +- 5 files changed, 119 insertions(+), 9 deletions(-) create mode 100644 internal/server/flush_test.go diff --git a/README.md b/README.md index 7f44357..7c385dc 100644 --- a/README.md +++ b/README.md @@ -2979,10 +2979,10 @@ Applied to all routes in this order: 2. **SecurityHeaders** — Production security headers on every response (HSTS, X-Content-Type-Options, X-Frame-Options, CSP, Referrer-Policy, Permissions-Policy) -3. **Logging** — Structured request logging (method, URL, status, - latency, remote IP, user agent, request ID) -4. **Metrics** — Prometheus HTTP metrics (if `METRICS_USERNAME` and +3. **Metrics** — Prometheus HTTP metrics (if `METRICS_USERNAME` and `METRICS_PASSWORD` are both set) +4. **Logging** — Structured request logging (method, URL, status, + latency, remote IP, user agent, request ID) 5. **CORS** — Cross-origin resource sharing headers 6. **Timeout** — 60-second request timeout 7. **Recoverer** — Panic recovery: one `ERROR` record through @@ -3002,6 +3002,10 @@ 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. +Metrics sits outside Logging so that a handler's flush reaches the +client: go-http-metrics' writer passes a flush on only when the writer +inside it has a `Flush` method, and the access log's writer has none. + Each admin page route group (`/pages`, `/user/*`, `/hooks`, `/hook/*`) starts with its own **Recoverer** and, if `SENTRY_DSN` is set, its own **Sentry** error reporting. That Recoverer answers a panic 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..4614499 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; +// TestFlushThroughProductionRouter in internal/server covers the +// shipped chain. func TestRecovererKeepsResponseControllerWorking(t *testing.T) { t.Parallel() diff --git a/internal/server/flush_test.go b/internal/server/flush_test.go new file mode 100644 index 0000000..2b6a674 --- /dev/null +++ b/internal/server/flush_test.go @@ -0,0 +1,96 @@ +package server_test + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sneak.berlin/go/webhooker/internal/config" + "sneak.berlin/go/webhooker/internal/server" +) + +// TestFlushThroughProductionRouter drives http.ResponseController.Flush +// through the shipped router and checks that the flush reaches the +// writer the server handed in. +// +// Every middleware that wraps the writer has to pass a flush through, +// with an Unwrap method or a Flush of its own. One that does neither +// makes Flush return http.ErrNotSupported, or do nothing at all when +// the wrapper outside it only looks for a Flush method, and 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 TestFlushThroughProductionRouter(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + cfg func(t *testing.T) *config.Config + sentryEnabled bool + }{ + { + name: "defaults", + cfg: func(t *testing.T) *config.Config { + t.Helper() + + return &config.Config{ + DataDir: t.TempDir(), + Environment: config.EnvironmentDev, + } + }, + }, + { + name: "metrics and Sentry on", + cfg: func(t *testing.T) *config.Config { + t.Helper() + + return metricsConfig(t, metricsUser, metricsAuthValue) + }, + sentryEnabled: true, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + env := newTestEnvWithConfig(t, tc.cfg(t)) + + var flushErr error + + probe := func(w http.ResponseWriter, _ *http.Request) { + flushErr = http.NewResponseController(w).Flush() + } + + 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 { + flushErr = nil + + w := httptest.NewRecorder() + router.ServeHTTP(w, httptest.NewRequestWithContext( + t.Context(), http.MethodGet, path, nil, + )) + + require.NoError(t, flushErr, path) + assert.True( + t, w.Flushed, + "%s: the flush must reach the server's writer", path, + ) + } + }) + } +} diff --git a/internal/server/routes.go b/internal/server/routes.go index 85dbb59..9660faf 100644 --- a/internal/server/routes.go +++ b/internal/server/routes.go @@ -61,16 +61,21 @@ func (s *Server) SetupRoutes() { func (s *Server) setupGlobalMiddleware() { s.router.Use(middleware.RequestID) s.router.Use(s.mw.SecurityHeaders()) - s.router.Use(s.mw.Logging()) // Metrics recording middleware, registered only when the // endpoint that exposes what it records is served. The // condition is the same MetricsAuthEnabled the /metrics mount // in setupRoutes reads. + // + // It goes outside Logging. go-http-metrics' writer passes a flush + // on only when the writer inside it has a Flush method, and the + // access log's writer has none, so inside Logging a handler's + // flush would silently do nothing. if s.params.Config.MetricsAuthEnabled() { s.router.Use(s.mw.Metrics()) } + s.router.Use(s.mw.Logging()) s.router.Use(s.mw.CORS()) s.router.Use(middleware.Timeout(requestTimeout))