manager: skip gzip for bodiless, ranged, HEAD and compressed responses (closes #28) #29

Merged

linus merged 1 commit from ash/gzip-skip-28 into develop

2026-10-23 19:01:44 +00:00

ash commented

2026-10-23 18:57:26 +00:00

Collaborator

Closes #28. Follow-up to #26, folding in Blake's review 852.

The middleware now decides per response, in WriteHeader, whether to compress:

Left alone: 204, 304, 206 or any Content-Range response, responses that already set Content-Encoding, and already-compressed types (images other than SVG and BMP, which covers png, jpeg, webp, gif and exr; video; audio; zip, gzip and other archives; woff).
Skipped up front: HEAD and Range requests, WebSocket upgrades as before.
Accept-Encoding is parsed with q values, so gzip;q=0, *;q=0 and *, gzip;q=0 all mean no gzip.
The writer implements Flush (flushes gzip, then the connection) and Unwrap, for streaming handlers and http.ResponseController.
Vary: Accept-Encoding is added rather than overwritten.

One side effect: the EXR preview from #25 is image/png, so it is no longer gzipped at all and keeps its real Content-Length. The #26 Content-Length test now uses a tar body.

Tests (internal/manager/gzip_test.go, real HTTP via httptest): each skip case above comes back unencoded and byte-identical, text still compresses and decompresses, a table for acceptsGzip, and a flush test that reads the first gzipped chunk while the handler is still blocked.

go test -race ./internal/manager passes on the jumpbox. Across the whole tree, only runner/encoding's ffmpeg integration test fails there, because the jumpbox has no ffmpeg. CI has it.

Closes #28. Follow-up to #26, folding in Blake's review 852. The middleware now decides per response, in `WriteHeader`, whether to compress: - Left alone: 204, 304, 206 or any `Content-Range` response, responses that already set `Content-Encoding`, and already-compressed types (images other than SVG and BMP, which covers png, jpeg, webp, gif and exr; video; audio; zip, gzip and other archives; woff). - Skipped up front: HEAD and `Range` requests, WebSocket upgrades as before. - `Accept-Encoding` is parsed with q values, so `gzip;q=0`, `*;q=0` and `*, gzip;q=0` all mean no gzip. - The writer implements `Flush` (flushes gzip, then the connection) and `Unwrap`, for streaming handlers and `http.ResponseController`. - `Vary: Accept-Encoding` is added rather than overwritten. One side effect: the EXR preview from #25 is `image/png`, so it is no longer gzipped at all and keeps its real `Content-Length`. The #26 Content-Length test now uses a tar body. **Tests** (`internal/manager/gzip_test.go`, real HTTP via httptest): each skip case above comes back unencoded and byte-identical, text still compresses and decompresses, a table for `acceptsGzip`, and a flush test that reads the first gzipped chunk while the handler is still blocked. `go test -race ./internal/manager` passes on the jumpbox. Across the whole tree, only `runner/encoding`'s ffmpeg integration test fails there, because the jumpbox has no ffmpeg. CI has it.

ash added 1 commit

2026-10-23 18:57:26 +00:00

manager: skip gzip for bodiless, ranged, HEAD and compressed responses (closes #28 )

CI / check-and-test (pull_request) Successful in 39s

Details

71cbc7be7c

Decide per response in WriteHeader: leave 204, 304, 206 and Content-Range
responses, ones that already set Content-Encoding, and already-compressed
types (images other than SVG/BMP, video, audio, zip/gzip/archives, woff)
uncompressed. Skip HEAD and Range requests. Parse Accept-Encoding q values
so gzip;q=0 and *;q=0 mean no gzip. Add Flush and Unwrap so streaming
handlers and http.ResponseController still work. Vary: Accept-Encoding is
added rather than set.

Follow-up to #26 per Blake review 852.

ash requested reviews from eva, blake

2026-10-23 18:57:26 +00:00

eva approved these changes

2026-10-23 18:59:16 +00:00

eva left a comment

GO on 71cbc7be7c.

This covers everything in #28 and Blake's notes from 852. The skip decision lives in one place (WriteHeader), Content-Length is dropped only when we actually compress, and q-value parsing handles the * vs explicit gzip cases correctly, including gzip;q=0, *. The tests go through real HTTP, and the flush test proves the first chunk reaches the client while the handler is still blocked. Head is on the current develop tip and CI run 50 is green.

One small thing, not blocking: HEAD and Range requests return before the wrapper, so they don't get Vary: Accept-Encoding the way the matching GET does. If you're in here again, it's worth adding there too.

Blake is already on as reviewer. Once he's clear, this goes to Linus.

GO on 71cbc7be7c. This covers everything in #28 and Blake's notes from 852. The skip decision lives in one place (`WriteHeader`), Content-Length is dropped only when we actually compress, and q-value parsing handles the `*` vs explicit `gzip` cases correctly, including `gzip;q=0, *`. The tests go through real HTTP, and the flush test proves the first chunk reaches the client while the handler is still blocked. Head is on the current develop tip and CI run 50 is green. One small thing, not blocking: HEAD and Range requests return before the wrapper, so they don't get `Vary: Accept-Encoding` the way the matching GET does. If you're in here again, it's worth adding there too. Blake is already on as reviewer. Once he's clear, this goes to Linus.

blake approved these changes

2026-10-23 19:00:51 +00:00

blake left a comment

Security GO on 71cbc7be7c (base develop bae029e7b1, which is the current tip).

I read the whole diff myself and checked each item from #28 and my 852 notes:

Skip decision happens in WriteHeader. 204, 304, 206, an existing Content-Range and an existing Content-Encoding all pass through untouched. Content-Encoding: gzip is only set on the compress path, and Content-Length and Content-Range are only touched there too, so skipped responses keep them. HEAD and any request with a Range header skip before the wrapper is created, so the range branch at jobs.go:3080-3094 is never gzipped. Already-compressed types (image/video/audio except svg/bmp, zip/gz/7z/rar/bz2/xz/zstd, woff) skip too.
No writer leak or empty gzip body on skips. The gzip writer is only created on the compress path, and close() does nothing when it's nil.
Flush writes out the gzip buffer first, then flushes the underlying writer. If headers haven't gone out yet, it writes them first. TestGzipMiddlewareFlush proves the first chunk reaches the client while the handler is still running.
WebSocket/Hijack is unchanged. Upgrade: websocket (now matched without regard to case) still bypasses the wrapper before it's built, so the four wsUpgrader.Upgrade call sites get the raw writer. Unwrap only lets http.ResponseController reach the underlying writer, and ResponseController uses our Flush first.
Accept-Encoding parsing. It handles case, whitespace, x-gzip, *, q=0 / 0.0 (an explicit gzip entry beats *) and bad q values (treated as not acceptable). I found no way to make it panic: it only uses Cut, Split and ParseFloat with no indexing. TestAcceptsGzip covers all of these.
Nothing unexpected in the diff: just manager.go and gzip_test.go.

Non-blocking nits:

Responses that skip before the wrapper (HEAD, Range, no gzip offered) don't get Vary: Accept-Encoding, but wrapped responses always do. A shared cache could keep an identity response and serve it to everyone, which is harmless. Setting Vary in the outer function would make it consistent.
The skip tests compare bodies but don't check that Content-Length and Content-Range survive on skipped responses. A check on the 206 case would lock that in.
Sniffing treats a Content-Type key that is present but empty the same as a missing one. net/http skips sniffing in that case. No route here relies on it.

Security GO on 71cbc7be7c (base develop bae029e7b1, which is the current tip). I read the whole diff myself and checked each item from #28 and my 852 notes: - **Skip decision happens in WriteHeader.** 204, 304, 206, an existing Content-Range and an existing Content-Encoding all pass through untouched. Content-Encoding: gzip is only set on the compress path, and Content-Length and Content-Range are only touched there too, so skipped responses keep them. HEAD and any request with a Range header skip before the wrapper is created, so the range branch at jobs.go:3080-3094 is never gzipped. Already-compressed types (image/video/audio except svg/bmp, zip/gz/7z/rar/bz2/xz/zstd, woff) skip too. - **No writer leak or empty gzip body on skips.** The gzip writer is only created on the compress path, and close() does nothing when it's nil. - **Flush** writes out the gzip buffer first, then flushes the underlying writer. If headers haven't gone out yet, it writes them first. TestGzipMiddlewareFlush proves the first chunk reaches the client while the handler is still running. - **WebSocket/Hijack is unchanged.** Upgrade: websocket (now matched without regard to case) still bypasses the wrapper before it's built, so the four wsUpgrader.Upgrade call sites get the raw writer. Unwrap only lets http.ResponseController reach the underlying writer, and ResponseController uses our Flush first. - **Accept-Encoding parsing.** It handles case, whitespace, x-gzip, `*`, q=0 / 0.0 (an explicit gzip entry beats `*`) and bad q values (treated as not acceptable). I found no way to make it panic: it only uses Cut, Split and ParseFloat with no indexing. TestAcceptsGzip covers all of these. - Nothing unexpected in the diff: just manager.go and gzip_test.go. Non-blocking nits: 1. Responses that skip before the wrapper (HEAD, Range, no gzip offered) don't get `Vary: Accept-Encoding`, but wrapped responses always do. A shared cache could keep an identity response and serve it to everyone, which is harmless. Setting Vary in the outer function would make it consistent. 2. The skip tests compare bodies but don't check that Content-Length and Content-Range survive on skipped responses. A check on the 206 case would lock that in. 3. Sniffing treats a Content-Type key that is present but empty the same as a missing one. net/http skips sniffing in that case. No route here relies on it.