fetch: hand off buffered response bodies instead of copying them - #31676
Open
alii wants to merge 11 commits into
Open
fetch: hand off buffered response bodies instead of copying them#31676alii wants to merge 11 commits into
alii wants to merge 11 commits into
Claude / Claude Code Review
completed
Jun 5, 2026 in 32m 50s
Code review found 1 important issue
Found 5 candidates, confirmed 2. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 1 |
| 🟡 Nit | 1 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | src/http/InternalState.rs:315-334 |
shrink_to_fit gated on read==buffer.len(), so lying-ISIZE-via-trailing-data reservation is never shrunk |
| 🟡 Nit | test/js/web/fetch/fetch-gzip.test.ts:425-432 |
Lying-ISIZE subprocess test pipes stderr but never reads/asserts it |
Annotations
Check failure on line 334 in src/http/InternalState.rs
claude / Claude Code Review
shrink_to_fit gated on read==buffer.len(), so lying-ISIZE-via-trailing-data reservation is never shrunk
The `shrink_to_fit` is gated on `result.read == buffer.len()`, but the only way `cap >> len` can happen here is exactly when that guard is **false** (Success + trailing data, as in the new "lying huge ISIZE" RSS test) — so the over-reserved Vec falls through to the slow path unshrunk and is then swapped into the JS body with ~30 MB capacity behind a 17-byte view. Conversely, when `read == buffer.len()` the buffer was a single valid gzip member, libdeflate has verified ISIZE == output length, so
Check warning on line 432 in test/js/web/fetch/fetch-gzip.test.ts
claude / Claude Code Review
Lying-ISIZE subprocess test pipes stderr but never reads/asserts it
nit: `stderr: "pipe"` is set but `proc.stderr` is never read. If the child throws (e.g. the `held[i].length !== 17` check, or `fetch()` rejecting), stdout is empty and `JSON.parse("")` fails with an opaque `Unexpected end of JSON input` before the exitCode assertion runs — the real diagnostic in stderr is lost. Per the repo convention for `bunEnv` subprocess tests, include `proc.stderr.text()` in the `Promise.all` and assert `expect(stderr).toBe("")` before parsing stdout / asserting exitCode.
Loading