From 7557e06f907badd90a2d59dec3c2761da733c300 Mon Sep 17 00:00:00 2001 From: SyniRon <66834451+SyniRon@users.noreply.github.com> Date: Sun, 7 Jun 2026 10:44:31 -0400 Subject: [PATCH 1/4] =?UTF-8?q?rest:=20gzip=20FlushError=20+=20Unwrap=20?= =?UTF-8?q?=E2=80=94=20ResponseController=20works=20behind=20gzip=20(#167)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit gzipResponseWriter implemented neither Flush/FlushError nor Unwrap, so http.NewResponseController(w).Flush() on any gzip-negotiated request died with ErrNotSupported and every other controller verb was equally unreachable — a future streaming handler could never flush behind gzip. FlushError flushes the gzip.Writer first (sync block: everything written so far becomes a decodable gzip prefix on the wire) THEN the underlying chain — never the reverse, and never a bare Unwrap for flush, which would bypass the gzip buffer and corrupt the stream. Unwrap passes the non-stream verbs (deadline control) through; the controller's method search prefers the explicit FlushError, so flush can't take that route. Pinned behaviorally over real server connections (the recorder is a Flusher and would mask a dead tunnel): flush succeeds when negotiated, mid-body flushed prefix decodes incrementally while the handler still holds the tail, full body stays byte-identical through the trailer, non-negotiated requests untouched. Internal pins take a wire-faithful snapshot at the moment the delegated flush fires (crisp ordering witness) and keep an unflushable chain failing loud (ErrNotSupported propagates). Stale "gzip chain can never flush" notes in cachecontrol truth-ed up; its rollback stays — any future unflushable wrapper reopens that hole. Co-Authored-By: Claude Opus 4.8 (1M context) --- rest/cachecontrol.go | 10 +- rest/cachecontrol_internal_test.go | 14 +-- rest/gzip.go | 29 +++++ rest/gzip_internal_test.go | 102 ++++++++++++++++ rest/gzip_test.go | 189 +++++++++++++++++++++++++++++ 5 files changed, 333 insertions(+), 11 deletions(-) create mode 100644 rest/gzip_internal_test.go create mode 100644 rest/gzip_test.go diff --git a/rest/cachecontrol.go b/rest/cachecontrol.go index 9ddd466..32de5e3 100644 --- a/rest/cachecontrol.go +++ b/rest/cachecontrol.go @@ -105,10 +105,12 @@ func (w *cacheControlWriter) Write(b []byte) (int, error) { // 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. +// type doc forbids. (Before #167 gzipResponseWriter supported no flush at +// all, so on the gzip chain the delegated flush ALWAYS failed this way; its +// FlushError closed that hole, but any future unflushable wrapper reopens +// it, so the rollback stays.) 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 { diff --git a/rest/cachecontrol_internal_test.go b/rest/cachecontrol_internal_test.go index e728201..8300d66 100644 --- a/rest/cachecontrol_internal_test.go +++ b/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,7 +93,7 @@ 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"), @@ -101,9 +101,9 @@ func TestCacheControlWriter_CommitDecision(t *testing.T) { }) } -// 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 } diff --git a/rest/gzip.go b/rest/gzip.go index 71cb004..dbbb804 100644 --- a/rest/gzip.go +++ b/rest/gzip.go @@ -44,3 +44,32 @@ 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). +func (w *gzipResponseWriter) FlushError() error { + 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; a +// Hijack caller takes the raw connection and owns the consequences, same as +// hijacking past any wrapper). 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 +} diff --git a/rest/gzip_internal_test.go b/rest/gzip_internal_test.go new file mode 100644 index 0000000..d5ea511 --- /dev/null +++ b/rest/gzip_internal_test.go @@ -0,0 +1,102 @@ +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 empty; 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. +func TestGzipResponseWriter_FlushReportsUnsupportedChain(t *testing.T) { + flushErr := make(chan error, 1) + h := GzipMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + flushErr <- http.NewResponseController(w).Flush() + })) + + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.Header.Set("Accept-Encoding", "gzip") + h.ServeHTTP(&noFlushUnderlying{header: make(http.Header)}, req) + + require.ErrorIs(t, <-flushErr, http.ErrNotSupported, + "an unflushable chain below gzip must surface, not vanish") +} diff --git a/rest/gzip_test.go b/rest/gzip_test.go new file mode 100644 index 0000000..c5a76e6 --- /dev/null +++ b/rest/gzip_test.go @@ -0,0 +1,189 @@ +package rest_test + +// Behavioral tests for the gzip layer's ResponseController support (#167): +// a handler behind GzipMiddleware must be able to flush mid-body without +// corrupting the compressed stream. Driven over real server connections +// (httptest.NewServer) — the recorder implements Flusher directly and would +// mask a dead tunnel — with Accept-Encoding set explicitly so the transport +// neither injects the header nor transparently decompresses: the tests read +// the raw gzip bytes exactly as a streaming consumer would. + +import ( + "compress/gzip" + "io" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/7cav/api/rest" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// ResponseController.Flush from a handler on a gzip-negotiated request must +// succeed — before #167 gzipResponseWriter implemented neither FlushError nor +// Unwrap, so the controller dead-ended with http.ErrNotSupported and no +// streaming handler could ever flush behind gzip. +func TestGzip_ResponseControllerFlushSucceeds(t *testing.T) { + flushErr := make(chan error, 1) // handler runs on the server goroutine + h := rest.GzipMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + flushErr <- http.NewResponseController(w).Flush() + })) + + srv := httptest.NewServer(h) + defer srv.Close() + + req, err := http.NewRequest(http.MethodGet, srv.URL+"/", nil) + require.NoError(t, err) + req.Header.Set("Accept-Encoding", "gzip") + res, err := srv.Client().Do(req) + require.NoError(t, err) + defer res.Body.Close() + + require.Equal(t, "gzip", res.Header.Get("Content-Encoding"), + "the request negotiated gzip — the wrapper under test must be in play") + require.NoError(t, <-flushErr, + "ResponseController.Flush must succeed through the gzip wrapper") +} + +// Non-gzip-negotiated requests are unaffected (#167 acceptance): without +// Accept-Encoding: gzip the middleware passes the writer straight through, so +// a mid-body flush succeeds and the flushed bytes arrive uncompressed. +func TestGzip_NonNegotiatedRequestFlushesPlain(t *testing.T) { + const body = "plain, never compressed" + + flushErr := make(chan error, 1) // handler runs on the server goroutine + h := rest.GzipMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, err := io.WriteString(w, body) + assert.NoError(t, err) + flushErr <- http.NewResponseController(w).Flush() + })) + + srv := httptest.NewServer(h) + defer srv.Close() + + req, err := http.NewRequest(http.MethodGet, srv.URL+"/", nil) + require.NoError(t, err) + req.Header.Set("Accept-Encoding", "identity") // explicit: suppress the transport's auto-gzip + res, err := srv.Client().Do(req) + require.NoError(t, err) + defer res.Body.Close() + + require.NoError(t, <-flushErr, "flush must keep working off the gzip path") + require.Empty(t, res.Header.Get("Content-Encoding"), + "no negotiation, no compression") + got, err := io.ReadAll(res.Body) + require.NoError(t, err) + assert.Equal(t, body, string(got), "body must arrive as written, uncompressed") +} + +// The other ResponseController verbs (deadline control here, as the witness) +// must tunnel through the gzip wrapper via Unwrap — they don't touch the +// compressed stream, so passing them straight down is safe. Flush is the one +// verb that must NOT take that route; the explicit FlushError above wins the +// controller's method search, so Unwrap never opens the corruption path. +func TestGzip_ResponseControllerDeadlinesTunnelThrough(t *testing.T) { + deadlineErr := make(chan error, 1) // handler runs on the server goroutine + h := rest.GzipMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + deadlineErr <- http.NewResponseController(w).SetWriteDeadline(time.Now().Add(time.Minute)) + })) + + srv := httptest.NewServer(h) + defer srv.Close() + + req, err := http.NewRequest(http.MethodGet, srv.URL+"/", nil) + require.NoError(t, err) + req.Header.Set("Accept-Encoding", "gzip") + res, err := srv.Client().Do(req) + require.NoError(t, err) + defer res.Body.Close() + + require.NoError(t, <-deadlineErr, + "deadline control must reach the connection through the gzip wrapper") +} + +// The streaming contract (#167): bytes received after a mid-body flush must +// be incrementally decodable as a gzip stream — the flushed prefix +// decompresses to exactly what the handler wrote before flushing, while the +// handler is still alive and holding the rest. The handler blocks after the +// flush until the client has decoded the prefix, so a flush that left the +// sync block buffered anywhere (gzip.Writer or the chain below) deadlocks the +// decode — the watchdog turns that into a crisp failure. Then the handler +// finishes and the full body must still decompress byte-identically to the +// uncompressed handler output, through an intact trailer. +func TestGzip_MidBodyFlushStreamsDecodablePrefix(t *testing.T) { + const part1 = "first chunk, flushed mid-body" + const part2 = "; second chunk, after the flush" + + flushErr := make(chan error, 1) // handler runs on the server goroutine + release := make(chan struct{}) // client → handler: prefix decoded, finish + handlerErr := make(chan error, 2) + h := rest.GzipMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, err := io.WriteString(w, part1) + handlerErr <- err + flushErr <- http.NewResponseController(w).Flush() + <-release + _, err = io.WriteString(w, part2) + handlerErr <- err + })) + + srv := httptest.NewServer(h) + defer srv.Close() + + req, err := http.NewRequest(http.MethodGet, srv.URL+"/", nil) + require.NoError(t, err) + req.Header.Set("Accept-Encoding", "gzip") + res, err := srv.Client().Do(req) + require.NoError(t, err) + defer res.Body.Close() + require.Equal(t, "gzip", res.Header.Get("Content-Encoding")) + + require.NoError(t, <-handlerErr, "pre-flush write must succeed") + require.NoError(t, <-flushErr, "mid-body flush must succeed") + + // Decode the flushed prefix while the handler still blocks on release — + // possible only if the flush pushed a complete sync block to the wire. + zr, prefix := decodePrefixWithin(t, res.Body, len(part1), 5*time.Second) + assert.Equal(t, part1, prefix, + "flushed prefix must decompress to exactly the pre-flush writes") + + close(release) + rest_, err := io.ReadAll(zr) // EOF verifies the gzip CRC/size trailer + require.NoError(t, err, "tail must decode through an intact trailer") + require.NoError(t, <-handlerErr, "post-flush write must succeed") + assert.Equal(t, part1+part2, prefix+string(rest_), + "full body must decompress byte-identically to the handler output") +} + +// decodePrefixWithin opens a gzip reader over body and decodes exactly n +// bytes, failing the test if that takes longer than timeout — the deadlock +// shape a buffered (non-streamed) flush produces. Returns the open reader so +// the caller can decode the rest of the stream. +func decodePrefixWithin(t *testing.T, body io.Reader, n int, timeout time.Duration) (*gzip.Reader, string) { + t.Helper() + type result struct { + zr *gzip.Reader + buf []byte + err error + } + done := make(chan result, 1) + go func() { + zr, err := gzip.NewReader(body) + if err != nil { + done <- result{err: err} + return + } + buf := make([]byte, n) + _, err = io.ReadFull(zr, buf) + done <- result{zr: zr, buf: buf, err: err} + }() + select { + case res := <-done: + require.NoError(t, res.err, "flushed prefix must be decodable as a gzip stream") + return res.zr, string(res.buf) + case <-time.After(timeout): + t.Fatal("flushed prefix never became decodable — the flush left compressed bytes buffered instead of streaming a sync block to the wire") + return nil, "" + } +} From 41fd6448373df47742262543f3ccd31ff844bd7b Mon Sep 17 00:00:00 2001 From: SyniRon <66834451+SyniRon@users.noreply.github.com> Date: Sun, 7 Jun 2026 10:48:43 -0400 Subject: [PATCH 2/4] rest: unblock streaming-test handler on failure paths (gzip #167) Mutation-validating the flush pins exposed a harness hang: when the prefix-decode watchdog fired, the handler stayed blocked on release and the deferred srv.Close waited on it forever. A deferred sync.OnceFunc release (LIFO, before Close) turns that regression shape into a crisp failure instead of a suite hang. Co-Authored-By: Claude Opus 4.8 (1M context) --- rest/gzip_test.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/rest/gzip_test.go b/rest/gzip_test.go index c5a76e6..a4f3811 100644 --- a/rest/gzip_test.go +++ b/rest/gzip_test.go @@ -13,6 +13,7 @@ import ( "io" "net/http" "net/http/httptest" + "sync" "testing" "time" @@ -118,6 +119,7 @@ func TestGzip_MidBodyFlushStreamsDecodablePrefix(t *testing.T) { flushErr := make(chan error, 1) // handler runs on the server goroutine release := make(chan struct{}) // client → handler: prefix decoded, finish + releaseOnce := sync.OnceFunc(func() { close(release) }) handlerErr := make(chan error, 2) h := rest.GzipMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { _, err := io.WriteString(w, part1) @@ -130,6 +132,9 @@ func TestGzip_MidBodyFlushStreamsDecodablePrefix(t *testing.T) { srv := httptest.NewServer(h) defer srv.Close() + // LIFO: unblock the handler before srv.Close waits on it, so a failure + // before the deliberate release fails the test instead of hanging it. + defer releaseOnce() req, err := http.NewRequest(http.MethodGet, srv.URL+"/", nil) require.NoError(t, err) @@ -148,7 +153,7 @@ func TestGzip_MidBodyFlushStreamsDecodablePrefix(t *testing.T) { assert.Equal(t, part1, prefix, "flushed prefix must decompress to exactly the pre-flush writes") - close(release) + releaseOnce() rest_, err := io.ReadAll(zr) // EOF verifies the gzip CRC/size trailer require.NoError(t, err, "tail must decode through an intact trailer") require.NoError(t, <-handlerErr, "post-flush write must succeed") From 062e631094e40bb92212c137bdd10a84f6f25ef3 Mon Sep 17 00:00:00 2001 From: SyniRon <66834451+SyniRon@users.noreply.github.com> Date: Sun, 7 Jun 2026 12:01:04 -0400 Subject: [PATCH 3/4] rest: FlushError strips stale Content-Length; scope flush/hijack docs, harden tests (#167) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Post-review fixes: - FlushError now dels the uncompressed Content-Length before emitting the sync block — flush is a header-committing event, and the stale length truncated the compressed stream silently (flush AND Write returned nil). Pinned RED-first with a real-server flush-before-first-write test. - cacheControlWriter rollback comment scoped: 'nothing reached the wire' holds above gzip, not below — gzip's FlushError writes header+sync block downstream before the delegated flush can fail. Logic unchanged. - FlushError doc + internal test now pin the partial-write-on-failure property (ErrNotSupported is not a no-op). - Streaming test gets a request-context deadline so a flush mutant fails in seconds instead of hanging Do to the global go-test timeout. - Comment corrections: real reasons for real connections in the package doc, honest Hijack consequences (deferred gz.Close fires ErrHijacked + misleading truncation log), rest_ -> tail. Co-Authored-By: Claude Opus 4.8 (1M context) --- rest/cachecontrol.go | 27 +++++++------ rest/gzip.go | 22 +++++++---- rest/gzip_internal_test.go | 11 +++++- rest/gzip_test.go | 78 +++++++++++++++++++++++++++++++++----- 4 files changed, 109 insertions(+), 29 deletions(-) diff --git a/rest/cachecontrol.go b/rest/cachecontrol.go index 32de5e3..b5468e4 100644 --- a/rest/cachecontrol.go +++ b/rest/cachecontrol.go @@ -100,17 +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. (Before #167 gzipResponseWriter supported no flush at -// all, so on the gzip chain the delegated flush ALWAYS failed this way; its -// FlushError closed that hole, but any future unflushable wrapper reopens -// it, so the rollback stays.) 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 { diff --git a/rest/gzip.go b/rest/gzip.go index dbbb804..8d1a734 100644 --- a/rest/gzip.go +++ b/rest/gzip.go @@ -54,8 +54,14 @@ func (w *gzipResponseWriter) Write(b []byte) (int, error) { // 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). +// cacheControlWriter). An error return — including ErrNotSupported from an +// unflushable chain — is NOT a no-op: the gzip header and sync block are +// already downstream by the time the delegated flush can fail. 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. + w.Header().Del("Content-Length") if err := w.Writer.Flush(); err != nil { return err } @@ -64,12 +70,14 @@ func (w *gzipResponseWriter) FlushError() error { // 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; a -// Hijack caller takes the raw connection and owns the consequences, same as -// hijacking past any wrapper). 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. +// 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 } diff --git a/rest/gzip_internal_test.go b/rest/gzip_internal_test.go index d5ea511..ed4a174 100644 --- a/rest/gzip_internal_test.go +++ b/rest/gzip_internal_test.go @@ -86,7 +86,11 @@ 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. +// 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). func TestGzipResponseWriter_FlushReportsUnsupportedChain(t *testing.T) { flushErr := make(chan error, 1) h := GzipMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -95,8 +99,11 @@ func TestGzipResponseWriter_FlushReportsUnsupportedChain(t *testing.T) { req := httptest.NewRequest(http.MethodGet, "/", nil) req.Header.Set("Accept-Encoding", "gzip") - h.ServeHTTP(&noFlushUnderlying{header: make(http.Header)}, req) + out := &noFlushUnderlying{header: make(http.Header)} + h.ServeHTTP(out, req) require.ErrorIs(t, <-flushErr, http.ErrNotSupported, "an unflushable chain below gzip must surface, not vanish") + assert.Positive(t, out.buf.Len(), + "the failed flush is not a no-op: the gzip header and sync block already reached the underlying writer") } diff --git a/rest/gzip_test.go b/rest/gzip_test.go index a4f3811..ff4a661 100644 --- a/rest/gzip_test.go +++ b/rest/gzip_test.go @@ -3,16 +3,23 @@ package rest_test // Behavioral tests for the gzip layer's ResponseController support (#167): // a handler behind GzipMiddleware must be able to flush mid-body without // corrupting the compressed stream. Driven over real server connections -// (httptest.NewServer) — the recorder implements Flusher directly and would -// mask a dead tunnel — with Accept-Encoding set explicitly so the transport -// neither injects the header nor transparently decompresses: the tests read -// the raw gzip bytes exactly as a streaming consumer would. +// (httptest.NewServer) because a recorder cannot carry these tests: +// incremental mid-body delivery is unobservable on a recorder (one buffer, +// no wire timing), the deadline test needs a real connection to set a +// deadline on, and the dangerous mutant — Unwrap without FlushError — +// reports flush success on recorder and real connection alike, so only the +// streaming decode of real wire bytes catches it. Accept-Encoding is set +// explicitly so the transport neither injects the header nor transparently +// decompresses: the tests read the raw gzip bytes exactly as a streaming +// consumer would. import ( "compress/gzip" + "context" "io" "net/http" "net/http/httptest" + "strconv" "sync" "testing" "time" @@ -79,11 +86,58 @@ func TestGzip_NonNegotiatedRequestFlushesPlain(t *testing.T) { assert.Equal(t, body, string(got), "body must arrive as written, uncompressed") } +// A flush before the first write is a header-committing event, so FlushError +// must strip a stale uncompressed Content-Length exactly as Write does +// (rest/gzip.go). If the stale length commits alongside Content-Encoding: +// gzip, net/http truncates the longer compressed stream at that length and +// the client hits unexpected EOF mid-stream — while the flush AND the +// handler's writes all returned nil, so the corruption is invisible to the +// handler. Regression pin vs pre-#167, where the same handler got a loud +// ErrNotSupported with zero bytes moved. +func TestGzip_FlushBeforeFirstWriteStripsStaleContentLength(t *testing.T) { + const payload = `{"roster":"live","unit":"7th Cavalry","status":"active"}` + + flushErr := make(chan error, 1) // handler runs on the server goroutine + writeErr := make(chan error, 1) + h := rest.GzipMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // A length-aware handler sets the UNCOMPRESSED length, then flushes + // before its first write. + w.Header().Set("Content-Length", strconv.Itoa(len(payload))) + flushErr <- http.NewResponseController(w).Flush() + _, err := io.WriteString(w, payload) + writeErr <- err + })) + + srv := httptest.NewServer(h) + defer srv.Close() + + req, err := http.NewRequest(http.MethodGet, srv.URL+"/", nil) + require.NoError(t, err) + req.Header.Set("Accept-Encoding", "gzip") + res, err := srv.Client().Do(req) + require.NoError(t, err) + defer res.Body.Close() + + require.Equal(t, "gzip", res.Header.Get("Content-Encoding")) + require.NoError(t, <-flushErr, "the flush reports success either way — corruption would be silent") + require.NoError(t, <-writeErr, "and so does the handler's write") + + zr, err := gzip.NewReader(res.Body) + require.NoError(t, err, "body must open as a gzip stream") + decoded, err := io.ReadAll(zr) + require.NoError(t, err, + "compressed stream must arrive whole, not truncated at the stale uncompressed Content-Length") + require.NoError(t, zr.Close(), "gzip trailer (CRC + size) must be intact") + assert.Equal(t, payload, string(decoded), + "body must decompress byte-identically to the handler output") +} + // The other ResponseController verbs (deadline control here, as the witness) // must tunnel through the gzip wrapper via Unwrap — they don't touch the // compressed stream, so passing them straight down is safe. Flush is the one -// verb that must NOT take that route; the explicit FlushError above wins the -// controller's method search, so Unwrap never opens the corruption path. +// verb that must NOT take that route; gzipResponseWriter's explicit +// FlushError (rest/gzip.go) wins the controller's method search, so Unwrap +// never opens the corruption path. func TestGzip_ResponseControllerDeadlinesTunnelThrough(t *testing.T) { deadlineErr := make(chan error, 1) // handler runs on the server goroutine h := rest.GzipMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -136,7 +190,13 @@ func TestGzip_MidBodyFlushStreamsDecodablePrefix(t *testing.T) { // before the deliberate release fails the test instead of hanging it. defer releaseOnce() - req, err := http.NewRequest(http.MethodGet, srv.URL+"/", nil) + // The watchdog and the deferred release only arm once Do returns — a + // mutant whose flush delivers nothing before the headers would leave Do + // blocked forever. The request deadline turns that hang into a crisp + // failure, which in turn lets the deferred release unblock the handler. + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + req, err := http.NewRequestWithContext(ctx, http.MethodGet, srv.URL+"/", nil) require.NoError(t, err) req.Header.Set("Accept-Encoding", "gzip") res, err := srv.Client().Do(req) @@ -154,10 +214,10 @@ func TestGzip_MidBodyFlushStreamsDecodablePrefix(t *testing.T) { "flushed prefix must decompress to exactly the pre-flush writes") releaseOnce() - rest_, err := io.ReadAll(zr) // EOF verifies the gzip CRC/size trailer + tail, err := io.ReadAll(zr) // EOF verifies the gzip CRC/size trailer require.NoError(t, err, "tail must decode through an intact trailer") require.NoError(t, <-handlerErr, "post-flush write must succeed") - assert.Equal(t, part1+part2, prefix+string(rest_), + assert.Equal(t, part1+part2, prefix+string(tail), "full body must decompress byte-identically to the handler output") } From 82c91d25d87e3cc23f5da9e15edf7dff7216f333 Mon Sep 17 00:00:00 2001 From: SyniRon <66834451+SyniRon@users.noreply.github.com> Date: Sun, 7 Jun 2026 12:26:36 -0400 Subject: [PATCH 4/4] rest: de-vacuous flush half-state pin; correct gzip doc claims (#167) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixup round 2 from review: - gzip_internal_test.go: snapshot the underlying buffer INSIDE the handler the moment FlushError returns — after ServeHTTP the deferred gz.Close() writes header+trailer regardless, so the old post-return assert.Positive passed even for a probe-first mutant that pushes nothing (mutation-proven vacuous). Now pins >=15 bytes (10-byte gzip header + 5-byte sync block) and the 1f 8b magic at flush-failure time. Also corrected the skip-flush-mutant comment: its snapshot holds the 10-byte header, and the decode fails at io.ReadFull, not gzip.NewReader. - gzip_test.go: dropped the false "only the streaming decode … catches it" exclusivity (the internal snapshot test kills the same mutant); the true claim is no error assertion can catch it — the catch must decode what was delivered at flush time. Added the missing fourth recorder limitation (only net/http enforces a declared Content-Length), and fixed the watchdog/deferred-release arming description (registered before Do, fires only after). - gzip.go: scoped the "not a no-op" sentence to the delegated-flush failure path (a gz.Flush failure leaves an arbitrary prefix), and noted the pre-existing explicit-WriteHeader stale-CL hole the Del cannot cover, tracked separately. Co-Authored-By: Claude Opus 4.8 (1M context) --- rest/gzip.go | 13 +++++++++---- rest/gzip_internal_test.go | 27 ++++++++++++++++++++------- rest/gzip_test.go | 25 +++++++++++++++---------- 3 files changed, 44 insertions(+), 21 deletions(-) diff --git a/rest/gzip.go b/rest/gzip.go index 8d1a734..7e94d70 100644 --- a/rest/gzip.go +++ b/rest/gzip.go @@ -54,13 +54,18 @@ func (w *gzipResponseWriter) Write(b []byte) (int, error) { // 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). An error return — including ErrNotSupported from an -// unflushable chain — is NOT a no-op: the gzip header and sync block are -// already downstream by the time the delegated flush can fail. +// 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. + // 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 diff --git a/rest/gzip_internal_test.go b/rest/gzip_internal_test.go index ed4a174..e88fa97 100644 --- a/rest/gzip_internal_test.go +++ b/rest/gzip_internal_test.go @@ -43,8 +43,10 @@ func (w *flushSnapshotWriter) Flush() { // 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 empty; one that -// reorders leaves the sync block out of the snapshot. Both fail the decode. +// 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" @@ -90,20 +92,31 @@ func (w *noFlushUnderlying) WriteHeader(int) {} // 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). +// 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) { - flushErr <- http.NewResponseController(w).Flush() + 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") - out := &noFlushUnderlying{header: make(http.Header)} h.ServeHTTP(out, req) require.ErrorIs(t, <-flushErr, http.ErrNotSupported, "an unflushable chain below gzip must surface, not vanish") - assert.Positive(t, out.buf.Len(), - "the failed flush is not a no-op: the gzip header and sync block already reached the underlying writer") + 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") } diff --git a/rest/gzip_test.go b/rest/gzip_test.go index ff4a661..d188d6c 100644 --- a/rest/gzip_test.go +++ b/rest/gzip_test.go @@ -6,12 +6,16 @@ package rest_test // (httptest.NewServer) because a recorder cannot carry these tests: // incremental mid-body delivery is unobservable on a recorder (one buffer, // no wire timing), the deadline test needs a real connection to set a -// deadline on, and the dangerous mutant — Unwrap without FlushError — -// reports flush success on recorder and real connection alike, so only the -// streaming decode of real wire bytes catches it. Accept-Encoding is set -// explicitly so the transport neither injects the header nor transparently -// decompresses: the tests read the raw gzip bytes exactly as a streaming -// consumer would. +// deadline on, only net/http enforces a declared Content-Length (a recorder +// neither truncates nor errors, so the stale-CL corruption is unobservable +// on it), and the dangerous mutant — Unwrap without FlushError — reports +// flush success on recorder and real connection alike, so no error +// assertion can catch it: the catch must come from decoding what was +// actually delivered at flush time. These tests do that on real wire bytes; +// gzip_internal_test.go pins the same property crisply against a snapshot +// fake. Accept-Encoding is set explicitly so the transport neither injects +// the header nor transparently decompresses: the tests read the raw gzip +// bytes exactly as a streaming consumer would. import ( "compress/gzip" @@ -190,10 +194,11 @@ func TestGzip_MidBodyFlushStreamsDecodablePrefix(t *testing.T) { // before the deliberate release fails the test instead of hanging it. defer releaseOnce() - // The watchdog and the deferred release only arm once Do returns — a - // mutant whose flush delivers nothing before the headers would leave Do - // blocked forever. The request deadline turns that hang into a crisp - // failure, which in turn lets the deferred release unblock the handler. + // The watchdog only arms once Do returns, and the deferred release — + // registered above, before Do — can only fire once Do returns. A mutant + // whose flush delivers nothing before the headers would leave Do blocked + // forever; the request deadline turns that hang into a crisp failure, + // which in turn lets the deferred release unblock the handler. ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() req, err := http.NewRequestWithContext(ctx, http.MethodGet, srv.URL+"/", nil)