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

@@ -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)

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.

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()
 

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: <nil>, flush: <nil>", 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,
+				)
+			}
+		})
+	}
+}