fetch: resolve empty compressed responses as empty bodies#31736
Conversation
A response that declares a Content-Encoding but sends zero body bytes (for example an empty chunked gzip response) failed with ZlibError: the decompressor ran on zero input at stream end and reported a truncated stream. Node resolves these as an empty body. Skip decompression entirely when the response delivered no body bytes. An empty final flush after real data still flushes (total_body_received is non-zero on that path). Fixes #23149
|
Warning Review limit reached
More reviews will be available in 46 minutes and 57 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 11:05 PM PT - Jun 2nd, 2026
❌ @robobun, your commit b8ffbf3 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 31736That installs a local version of the PR into your bun-31736 --bun |
|
@robobun adopt |
|
✅ Adopted, validated, all review feedback addressed — ready for maintainer sign-off.
CI (build 60080): 282 jobs passed; the 3 red tests are all timeout/measurement heuristics with no connection to this diff:
All fetch/http-decompression lanes are green everywhere they ran. Needs a maintainer to merge or re-run the two red shards. |
There was a problem hiding this comment.
LGTM — tight, well-guarded early-out that matches Node's behavior; the inline note is just a test-style nit.
Extended reasoning...
Overview
This PR adds a 3-line guard at the top of InternalState::decompress_bytes in src/http/InternalState.rs: when the incoming buffer is empty and total_body_received == 0, skip decompression entirely and return Ok(()). This fixes #23149, where a response declaring Content-Encoding: gzip but delivering zero body bytes (e.g. an empty chunked body) would surface a ZlibError from the decompressor's truncated-stream check. Two regression tests are added in test/js/web/fetch/fetch-gzip.test.ts — one for the chunked case (the actual bug) and one pinning the already-working Content-Length: 0 case.
I traced the call sites: total_body_received is incremented before every path that reaches decompress_bytes (lib.rs:4082, :4147, :4234, :4319), so the guard correctly distinguishes "never received any body" from "empty final flush after real data" — the latter still runs the decompressor as the PR description notes. The guard also calls compressed_body.reset() to preserve the existing reset-on-every-exit invariant documented in the PORT NOTE just below it.
Security risks
None. This relaxes an error into an empty body only in the degenerate zero-byte case, matching Node/undici. It cannot cause partially-decompressed data to be silently accepted, since any non-zero total_body_received bypasses the guard.
Level of scrutiny
This is core fetch() decompression, so it's production-critical, but the change is minimal (one conservative conditional), well-commented, and covered by new tests that exercise the exact wire format from the issue. The blast radius is narrow: the new branch is unreachable unless the server sent literally zero body bytes.
Other factors
The only finding from the bug-hunting pass is a style nit — the new test uses await import("node:net") inside the test body rather than a module-scope aliased import, contrary to test/CLAUDE.md. This doesn't affect correctness and isn't worth blocking on. No CODEOWNERS cover these paths and there are no outstanding human review comments.
There was a problem hiding this comment.
LGTM — tight early-out guarded by total_body_received == 0, nit addressed.
Extended reasoning...
Overview
Fixes #23149: a response declaring Content-Encoding but delivering zero body bytes (e.g. an empty chunked gzip response) threw ZlibError because the decompressor was finalized on an empty stream. The fix is a 5-line early-return at the top of InternalState::decompress_bytes in src/http/InternalState.rs, plus two regression tests in test/js/web/fetch/fetch-gzip.test.ts and an incidental flake fix (localhost → 127.0.0.1) in a pre-existing test in the same file.
Security risks
None. The early-return is gated on buffer.is_empty() && self.total_body_received == 0, so it only fires when no body bytes were ever delivered — it cannot cause compressed data to be skipped or partially decoded. It does not touch auth, TLS, header parsing, or memory ownership; the compressed_body.reset() mirrors the deferred reset that already runs on every other exit path of this function.
Level of scrutiny
Low-to-medium. The change sits in the HTTP client decompression path, but it is a narrow, additive guard whose precondition I verified against every total_body_received += ... site in src/http/lib.rs (4082, 4147, 4234, 4319) — all increment before decompression is invoked, so the guard's semantics ("never received any body bytes") hold. The PR description's note that an empty final flush after real data still goes through the decompressor (because total_body_received > 0 by then) checks out.
Other factors
- robobun adopted and validated fail-before/pass-after under debug+ASAN and release, with all CI lanes green.
- My only prior feedback (hoist the dynamic
node:netimport to module scope) was addressed in d0c383c and the thread is resolved. - No CODEOWNERS entries cover
src/http/or this test file. - The bug-hunting system found nothing this round.
Newer UCRT builds on the CI Windows agents contain cldemote (NP 0f 1c /0) encodings in strpbrk's vectorized path, tripping the static baseline scan outside the existing [AVX, AVX2] ceiling. CLDEMOTE is hint-space and architecturally a NOP on CPUs without it, so it is baseline-safe; widen the ceiling.
There was a problem hiding this comment.
All earlier feedback is addressed and I didn't find anything further — deferring to a human for sign-off since this is a behavior change in the fetch decompression path.
Extended reasoning...
Overview
The PR adds a 4-line early-return guard in InternalState::decompress_bytes (src/http/InternalState.rs): when the incoming buffer is empty and total_body_received == 0, skip decompression and return Ok(()). This makes fetch() resolve a response that declares Content-Encoding but delivers zero body bytes (e.g. an empty chunked gzip response) as an empty body, matching Node, instead of surfacing a ZlibError from the decompressor's truncated-stream check. Two regression tests are added in test/js/web/fetch/fetch-gzip.test.ts (chunked and Content-Length: 0 variants), plus an unrelated hardening of an existing test in the same file (localhost → 127.0.0.1 to avoid IPv6/IPv4 bind/resolve mismatch).
I traced the callers in src/http/lib.rs: every path that reaches decompress_bytes updates total_body_received before the call, so the guard genuinely fires only when the entire response carried zero body bytes — an empty intermediate streaming flush with later real data won't be affected (later calls have total_body_received > 0). The compressed_body.reset() in the new branch mirrors what every other exit from this function already does.
Security risks
None identified. The change relaxes an error into an empty-body success for the zero-input case only; it doesn't expand what input reaches the decompressor or introduce any new parsing.
Level of scrutiny
Moderate. The diff is tiny and well-commented, with fail-before/pass-after test coverage verified by robobun. But it's a behavior change in the HTTP client's response-decompression path — one of Bun's most-exercised code paths — so a maintainer glance is appropriate. I also note the parallel src/http/InternalState.zig does not carry the equivalent guard; someone familiar with the Zig→Rust port status should confirm that's expected (the passing tests indicate the Rust path is the live one).
Other factors
All three of my earlier inline nits (dynamic import → module-scope, CLDEMOTE allowlist → superseded by main, socket.write → socket.end) were addressed and the corresponding threads are resolved; the interim CI-unblock rider was dropped after merging main, so the diff is now purely the fetch fix + tests. No CODEOWNERS entry covers these files. The bug-hunting pass found nothing.
What does this PR do?
Fixes #23149: a response that declares a
Content-Encodingbut sends zero body bytes — e.g. an empty chunked gzip response — failed withZlibError. The decompressor ran on zero input at stream end and reported a truncated stream; Node resolves these as an empty body.The fix is an early-out in
decompress_bytes: when the response delivered no body bytes at all (bufferempty andtotal_body_received == 0), there is nothing to decompress. An empty final flush after real data still flushes normally (total_body_receivedis non-zero on that path).Tests
Two cases added to
test/js/web/fetch/fetch-gzip.test.tsusing a raw socket server: empty gzip body via chunked encoding (fails on released Bun withZlibError, passes with this PR) and viaContent-Length: 0(already worked; pinned).fetch-gzip.test.ts+fetch.test.tssuites pass locally (the only failures are pre-existing external-network tests that fail identically on released Bun in this environment).CI notes
127.0.0.1: bindinglocalhostcan pick::1whilefetch()resolveslocalhostto127.0.0.1, which made it failConnectionRefusedon some hosts.windows-x64-baseline-verify-baselinefailed withstrpbrk [CLDEMOTE]— newer UCRT on the CI Windows agents emitscldemote(NP 0f 1c /0, hint-space, NOP where unsupported) instrpbrk. An interim per-symbol allowlist widening unblocked this branch; main then landed the proper global ignore (Hardening: input validation and compatibility fixes across 9 subsystems (round 9) #31606), so this branch merged main and dropped the interim entry — no verifier changes remain in the diff.