rest: gzip FlushError + Unwrap — ResponseController works behind gzip (#167)

rest/cachecontrol.go

@@ -100,15 +100,22 @@ func (w *cacheControlWriter) Write(b []byte) (int, error) {
 // map. Delegating through a fresh ResponseController keeps the downstream
 // search semantics identical.
 //
-// If the delegated flush reports http.ErrNotSupported, no layer below could
-// flush — nothing reached the wire, so nothing committed — and the stamp
-// this call made is rolled back. Leaving it would poison the live header map
-// and lie committed=true: a handler reacting to the failed flush by writing
-// an error would commit a non-200 carrying max-age, the exact leak class the
-// type doc forbids. Worst on the gzip chain, where gzipResponseWriter
-// supports no flush at all, so the delegated flush ALWAYS fails this way. A
-// genuine I/O error keeps the state: by then net/http has already
-// snapshotted the headers onto the wire, so the commit really happened.
+// If the delegated flush reports http.ErrNotSupported, the stamp this call
+// made is rolled back. Leaving it would poison the live header map and lie
+// committed=true: a handler reacting to the failed flush by writing an
+// error would commit a non-200 carrying max-age, the exact leak class the
+// type doc forbids. The rollback's premise — nothing reached the wire, so
+// nothing committed — holds for wrappers that fail before writing anything:
+// an unflushable wrapper ABOVE gzip, or a chain with no flushable bottom and
+// no gzip in between. It is NOT universal: gzip's FlushError (rest/gzip.go)
+// pushes its header and sync block downstream BEFORE the delegated flush can
+// fail, so a future unflushable wrapper BELOW gzip would surface
+// ErrNotSupported here after bytes had already latched a Write-committing
+// base — a commit this rollback would wrongly undo. Unreachable today
+// (everything below gzip is flushable); noted so such a wrapper isn't added
+// casually. A genuine I/O error keeps the state: by then net/http has
+// already snapshotted the headers onto the wire, so the commit really
+// happened.
 func (w *cacheControlWriter) FlushError() error {
 	stamped := false
 	if !w.committed {

rest/cachecontrol_internal_test.go

@@ -80,9 +80,9 @@ func TestCacheControlWriter_CommitDecision(t *testing.T) {
 		assert.Empty(t, rr.Header().Get("Cache-Control"))
 	})
 
-	// Regression pin (#163 R1): on a gzip-shaped chain — a plain
-	// ResponseWriter with no FlushError/Flusher/Unwrap, exactly
-	// gzipResponseWriter's shape — the delegated flush ALWAYS returns
+	// Regression pin (#163 R1): on an unflushable chain — a plain
+	// ResponseWriter with no FlushError/Flusher/Unwrap, the shape
+	// gzipResponseWriter had before #167 — the delegated flush ALWAYS returns
 	// http.ErrNotSupported: nothing reached the wire. The stamp and
 	// committed=true must roll back, or a handler reacting to the failed
 	// flush by writing an error commits a non-200 carrying max-age (the leak
@@ -93,17 +93,17 @@ func TestCacheControlWriter_CommitDecision(t *testing.T) {
 
 		err := http.NewResponseController(w).Flush()
 		require.ErrorIs(t, err, http.ErrNotSupported,
-			"a gzip-shaped writer supports no flush — nothing was sent")
+			"an unflushable writer supports no flush — nothing was sent")
 
 		w.WriteHeader(http.StatusNotFound)
 		assert.Empty(t, rr.Result().Header.Get("Cache-Control"),
 			"a 404 after a failed flush must not carry the freshness signal")
 	})
 }
 
-// noFlushWriter hides the recorder's Flusher — gzipResponseWriter's shape
-// (no FlushError, no Flusher, no Unwrap), where a delegated flush always
-// fails with http.ErrNotSupported.
+// noFlushWriter hides the recorder's Flusher — the shape gzipResponseWriter
+// had before #167 (no FlushError, no Flusher, no Unwrap), where a delegated
+// flush always fails with http.ErrNotSupported.
 type noFlushWriter struct {
 	rr *httptest.ResponseRecorder
 }

rest/gzip.go

@@ -44,3 +44,45 @@ func (w *gzipResponseWriter) Write(b []byte) (int, error) {
 	w.Header().Del("Content-Length") // This is necessary as otherwise it will have the uncompressed length
 	return w.Writer.Write(b)
 }
+
+// FlushError keeps http.ResponseController.Flush working behind gzip (#167):
+// first flush the gzip.Writer — emitting a sync block so everything the
+// handler wrote so far reaches the underlying writer as a decodable gzip
+// prefix — THEN flush the underlying chain onto the wire. That order is the
+// invariant: the bytes on the wire are always a valid gzip stream prefix. A
+// bare Unwrap instead would let flushes bypass the gzip buffer entirely,
+// pushing a stream the client cannot yet decode while the compressed tail
+// sits buffered here. Delegating through a fresh ResponseController keeps the
+// downstream search semantics identical (same pattern as commitWriter and
+// cacheControlWriter). Any error from the delegated flush — including
+// ErrNotSupported from an unflushable chain — is NOT a no-op: the gzip
+// header and sync block are already downstream before it runs. (If gz.Flush
+// itself fails, the failure came from a downstream write — what landed is
+// an arbitrary prefix, possibly nothing.)
+func (w *gzipResponseWriter) FlushError() error {
+	// A flush before the first write commits the headers, so the stale
+	// uncompressed Content-Length must go here too — same staleness Write
+	// handles above; left in place it truncates the compressed stream. An
+	// explicit WriteHeader still commits it (net/http latches the length at
+	// WriteHeader) — pre-existing hole, reachable on develop without any
+	// flush, tracked separately.
+	w.Header().Del("Content-Length")
+	if err := w.Writer.Flush(); err != nil {
+		return err
+	}
+	return http.NewResponseController(w.ResponseWriter).Flush()
+}
+
+// Unwrap exposes the underlying writer to http.ResponseController for the
+// verbs that don't touch the compressed stream — deadline control is the
+// deliberately supported set (EnableFullDuplex rides along harmlessly).
+// Hijack is NOT like hijacking past other wrappers here: the middleware's
+// deferred gz.Close() still fires after the hijack and fails with
+// http.ErrHijacked, emitting the misleading "response likely truncated"
+// log, and Content-Encoding: gzip is already on the header map. Flush can
+// never take this route: the controller's method search prefers the
+// explicit FlushError above, which is what keeps flushes from bypassing the
+// gzip buffer and corrupting the stream.
+func (w *gzipResponseWriter) Unwrap() http.ResponseWriter {
+	return w.ResponseWriter
+}

rest/gzip_internal_test.go

@@ -0,0 +1,122 @@
+package rest
+
+// Direct pins on gzipResponseWriter's FlushError ordering (#167), observed
+// against a recording fake writer: the e2e streaming test (gzip_test.go) can
+// only turn a wrong flush order into a watchdog timeout, while the snapshot
+// taken here at the moment the underlying Flush fires fails crisply. The
+// snapshot is wire-faithful — bytes the underlying writer had received when
+// the flush reached it, not the buffer's state after the handler finished.
+
+import (
+	"bytes"
+	"compress/gzip"
+	"io"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// flushSnapshotWriter records what the gzip layer pushed down to it, and
+// snapshots that buffer the moment Flush is called — the bytes that would be
+// on the wire after the flush.
+type flushSnapshotWriter struct {
+	header   http.Header
+	buf      bytes.Buffer
+	flushed  int
+	snapshot []byte // buf contents at the first Flush
+}
+
+func (w *flushSnapshotWriter) Header() http.Header         { return w.header }
+func (w *flushSnapshotWriter) Write(b []byte) (int, error) { return w.buf.Write(b) }
+func (w *flushSnapshotWriter) WriteHeader(int)             {}
+func (w *flushSnapshotWriter) Flush() {
+	if w.flushed == 0 {
+		w.snapshot = append([]byte(nil), w.buf.Bytes()...)
+	}
+	w.flushed++
+}
+
+// FlushError must flush the gzip.Writer's buffered output into the underlying
+// writer BEFORE flushing the underlying chain — in that order the bytes on
+// the wire at flush time are a valid gzip stream prefix decoding to exactly
+// the pre-flush writes. A mutant that skips the gzip flush (the corruption a
+// bare Unwrap would institutionalize) leaves the snapshot holding only the
+// 10-byte gzip header from the handler's first Write — the decode fails at
+// io.ReadFull, not gzip.NewReader; one that reorders leaves the sync block
+// out of the snapshot. Both fail the decode.
+func TestGzipResponseWriter_FlushOrdersGzipBeforeUnderlying(t *testing.T) {
+	const part1 = "written before the flush"
+
+	fake := &flushSnapshotWriter{header: make(http.Header)}
+	flushErr := make(chan error, 1)
+	h := GzipMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		_, err := io.WriteString(w, part1)
+		require.NoError(t, err)
+		flushErr <- http.NewResponseController(w).Flush()
+		_, err = io.WriteString(w, "after the flush, never flushed explicitly")
+		require.NoError(t, err)
+	}))
+
+	req := httptest.NewRequest(http.MethodGet, "/", nil)
+	req.Header.Set("Accept-Encoding", "gzip")
+	h.ServeHTTP(fake, req)
+
+	require.NoError(t, <-flushErr)
+	require.Equal(t, 1, fake.flushed, "exactly one delegated flush must reach the underlying writer")
+
+	zr, err := gzip.NewReader(bytes.NewReader(fake.snapshot))
+	require.NoError(t, err, "snapshot at flush time must open as a gzip stream")
+	prefix := make([]byte, len(part1))
+	_, err = io.ReadFull(zr, prefix)
+	require.NoError(t, err, "snapshot must contain the complete sync block for the pre-flush writes")
+	assert.Equal(t, part1, string(prefix))
+}
+
+// noFlushUnderlying is a writer the controller cannot flush — no FlushError,
+// no Flusher, no Unwrap.
+type noFlushUnderlying struct {
+	header http.Header
+	buf    bytes.Buffer
+}
+
+func (w *noFlushUnderlying) Header() http.Header         { return w.header }
+func (w *noFlushUnderlying) Write(b []byte) (int, error) { return w.buf.Write(b) }
+func (w *noFlushUnderlying) WriteHeader(int)             {}
+
+// When nothing below the gzip layer can flush, the handler must still hear
+// about it loudly: FlushError reports the chain's http.ErrNotSupported
+// instead of swallowing it and pretending the bytes reached the wire. The
+// failure is NOT a no-op, though — the gzip header and sync block reach the
+// underlying writer before the delegated flush can fail, and this pins that
+// half-state (it is the fact cacheControlWriter's rollback comment scopes
+// itself around). The underlying buffer is snapshotted INSIDE the handler,
+// the moment the flush returns: once ServeHTTP returns, the middleware's
+// deferred gz.Close() writes the gzip header and trailer into the buffer
+// regardless of what FlushError did, so any post-return assertion on the
+// buffer is vacuous — a probe-first FlushError that pushes nothing before
+// failing would pass it.
+func TestGzipResponseWriter_FlushReportsUnsupportedChain(t *testing.T) {
+	out := &noFlushUnderlying{header: make(http.Header)}
+	flushErr := make(chan error, 1)
+	atFlush := make(chan []byte, 1) // out.buf contents the moment FlushError returns
+	h := GzipMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		err := http.NewResponseController(w).Flush()
+		atFlush <- append([]byte(nil), out.buf.Bytes()...)
+		flushErr <- err
+	}))
+
+	req := httptest.NewRequest(http.MethodGet, "/", nil)
+	req.Header.Set("Accept-Encoding", "gzip")
+	h.ServeHTTP(out, req)
+
+	require.ErrorIs(t, <-flushErr, http.ErrNotSupported,
+		"an unflushable chain below gzip must surface, not vanish")
+	downstream := <-atFlush
+	require.GreaterOrEqual(t, len(downstream), 15,
+		"the failed flush is not a no-op: the 10-byte gzip header and 5-byte sync block must already be downstream when FlushError returns")
+	assert.Equal(t, []byte{0x1f, 0x8b}, downstream[:2],
+		"bytes downstream at flush-failure time must start with the gzip magic")
+}