rest: gzip FlushError + Unwrap — ResponseController works behind gzip (#167)#171
Merged
Conversation
…#167) 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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
…, harden tests (#167) 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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
This was referenced Jun 7, 2026
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #167.
gzipResponseWritersupported noResponseControllerverb — flush dead-ended at the gzip layer withErrNotSupported, blocking any future streaming handler. AddsFlushError(gzip buffer first — sync block keeps the wire a valid gzip prefix — then the underlying chain) andUnwrap(deadline-class verbs pass through; the controller's method search prefers the explicitFlushError, so flush can never bypass the gzip buffer — verified against the installed stdlib). Explicitly not a bareUnwrap, which would corrupt the stream. Supported-verb decisions documented on the type.Review-driven hardening in-branch:
FlushErrornow strips stale uncompressedContent-Length(review found flush-before-first-write committed it → silently truncated stream, flush and Write both returning nil; verified by live repro, pinned RED-first, Del-placement mutants killed)-raceno flakescacheControlWriterrollback comment re-scoped — its "nothing reached the wire" premise no longer holds below gzipTwo review rounds; remaining pre-existing findings (WriteHeader stale-CL hole, gateway statusRecorder) deferred to tracked issues.
🤖 Generated with Claude Code