Add 2 GiB string-limit test for typed-array Response bodies - #37232
Add 2 GiB string-limit test for typed-array Response bodies#37232robobun wants to merge 2 commits into
Conversation
A typed-array body is stored as an InternalBlob, which converts to a string through Internal::to_string_owned, a different guard call site than the Blob store path the existing boundary tests cover. Before ab5b3f2 this path aborted the process with a StringImpl length assertion. Lock in the catchable ERR_STRING_TOO_LONG behavior.
WalkthroughChangesOversized Response test
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/web/fetch/blob-oom.test.ts`:
- Around line 211-215: Update the oversized-allocation subprocess setup to use
stderr: "inherit" instead of piping stderr, and remove proc.stderr.text() from
the Promise.all collection and related parsing fallback. Preserve stdout
consumption and exitCode handling so native or ASAN diagnostics remain visible
in the runner log.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0ccecc61-af15-4eea-ac0d-7d830d802886
📒 Files selected for processing (1)
test/js/web/fetch/blob-oom.test.ts
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
| expect(JSON.parse(stdout.trim() || JSON.stringify({ stdout, stderr, exitCode }))).toEqual([ |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Inherit stderr for this oversized-allocation child.
stderr is piped and consumed, but it is not preserved when stdout contains valid JSON. If the child emits the expected result and then aborts or writes a native/ASAN diagnostic, the test loses that diagnostic and reports only the exit-code mismatch. Use stderr: "inherit" and remove proc.stderr.text() from the Promise.all call.
Proposed fix
- stderr: "pipe",
+ stderr: "inherit",
...
- const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
- expect(JSON.parse(stdout.trim() || JSON.stringify({ stdout, stderr, exitCode }))).toEqual([
+ const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]);
+ expect(JSON.parse(stdout.trim() || JSON.stringify({ stdout, exitCode }))).toEqual([Based on learnings, oversized-allocation subprocesses should inherit stderr so native diagnostics remain visible in the runner log.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| stdout: "pipe", | |
| stderr: "pipe", | |
| }); | |
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | |
| expect(JSON.parse(stdout.trim() || JSON.stringify({ stdout, stderr, exitCode }))).toEqual([ | |
| stdout: "pipe", | |
| stderr: "inherit", | |
| }); | |
| const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]); | |
| expect(JSON.parse(stdout.trim() || JSON.stringify({ stdout, exitCode }))).toEqual([ |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/js/web/fetch/blob-oom.test.ts` around lines 211 - 215, Update the
oversized-allocation subprocess setup to use stderr: "inherit" instead of piping
stderr, and remove proc.stderr.text() from the Promise.all collection and
related parsing fallback. Preserve stdout consumption and exitCode handling so
native or ASAN diagnostics remain visible in the runner log.
Source: Learnings
|
On the stderr suggestion: keeping The case where the child prints valid JSON and then dies during teardown would fail on the exit-code assertion with less detail, but that tradeoff is shared by all five existing tests, and changing just this one would leave the block inconsistent. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only addition that mirrors the existing sibling tests in the same gated block.
What was reviewed:
- New test follows the identical subprocess/assertion pattern as the neighboring
Blob.text()/.json()case (concurrent pipe drain, structured error assertions, exit code last). - Confirmed it sits under the same
describe.skipIf(os.totalmem() < 10 * 1024 ** 3)gate, so no new risk on small CI machines. - PR description documents it fails on a pre-#37215 build and passes on current main.
Extended reasoning...
Overview
This PR adds a single test case to test/js/web/fetch/blob-oom.test.ts inside the existing byte sources at the 2 GiB string limit describe block. The new test spawns a subprocess that constructs new Response(new Uint8Array(2 ** 31)) and asserts that .text() and .json() reject with ERR_STRING_TOO_LONG rather than aborting via the WTF StringImpl::MaxLength RELEASE_ASSERT. No production code is touched.
Security risks
None. This is a test-only change that runs in an isolated subprocess with no network, no filesystem writes beyond the harness temp machinery, and no user-controlled input.
Level of scrutiny
Low. The new test is a near-verbatim copy of the adjacent Blob.text() and Blob.json() at 2^31 bytes test — same spawn structure, same Promise.all pipe drain, same JSON.parse(stdout || fallback) diagnostic pattern, same expected error objects. The only substantive difference is the body source (new Response(typedArray) vs new Blob([typedArray])), which is precisely the point: it exercises the InternalBlob → Internal::to_string_owned path that the existing tests do not cover.
Other factors
- The test lives under the existing
describe.skipIf(os.totalmem() < 10 * 1024 ** 3)gate, so it inherits the same skip behavior on memory-constrained runners as its siblings. - The PR description records verification both ways: fails under
USE_SYSTEM_BUN=1on a pre-fix canary and passes underbun bd teston main (19 pass / 0 fail), satisfying the repo's "prove the test fails for the right reason" requirement. - Assertions are strong (exact
name/code/message), pipes are drained concurrently, andexitCodeis asserted after stdout — all matching harness conventions. - The comment above the test explains why this path is distinct from the Blob store path, which is useful non-obvious context rather than narration.
There was a problem hiding this comment.
LGTM — test-only addition that mirrors the two sibling tests in the same describe.skipIf block.
What was reviewed:
- New test is structurally identical to the adjacent
Blob.text()/.json()case (same spawn/drain/fallback/assert shape), just swapping innew Response(Uint8Array)to cover the InternalBlob →to_string_ownedpath. - Subprocess pipes drained concurrently via
Promise.all,await usingfor cleanup, exact error code/message asserted, exit code checked last. - CodeRabbit's
stderr: "inherit"nit was reasonably declined — the pipe+fallback pattern matches all five existing sibling tests and surfaces abort output in the assertion diff.
Extended reasoning...
Overview
Adds one test case to test/js/web/fetch/blob-oom.test.ts inside the existing byte sources at the 2 GiB string limit block (already gated on os.totalmem() < 10 * 1024 ** 3). It spawns a subprocess that constructs new Response(new Uint8Array(2 ** 31)) and asserts .text() / .json() reject with ERR_STRING_TOO_LONG rather than aborting. No production code is touched.
Security risks
None. Test-only change; the subprocess runs a fixed inline script with bunExe() and bunEnv, no network, no filesystem writes, no user-controlled input.
Level of scrutiny
Low. The new test is a near-verbatim copy of the immediately preceding Blob.text() and Blob.json() test — same spawn options, same Promise.all pipe drain, same JSON.parse(stdout || fallback) assertion shape, same expected error objects, same exitCode assertion order. The only substantive difference is new Response(bytes) vs new Blob([bytes]), which the PR description justifies (InternalBlob path via Internal::to_string_owned vs the Blob store path). It sits under the same memory gate and runs in ~3.4s per the evidence log, comparable to its siblings.
Other factors
- REVIEW.md test criteria are met: pipes drained concurrently,
await usingfor the subprocess, exact error name/code/message asserted (not baretoThrow), exit code asserted last, hermetic (no network), gatedskipIffor small machines. - PR description documents that the test fails on a pre-#37215 build and passes on current main (19/19), satisfying the fails-for-the-right-reason requirement.
- The one outstanding CodeRabbit comment is a self-labelled "Trivial" nitpick about
stderr: "inherit"; robobun's reply is sound (consistency with the five existing tests using the same pipe+fallback pattern, and the fallback is what surfaces the abort text in the failing-case diff). Changing only this test would make the block inconsistent, which REVIEW.md discourages ("match the exact file's local conventions"). Not a blocker.
|
CI state: the change is test-only and every lane that actually ran tests is green across two full builds (194/196 then 195/196 jobs). The remaining red in each build is one macOS lane dying in CI infrastructure before any test runs: the tart guest VM rejects SSH with "Permission denied (publickey)" during checkout sync. It hit darwin 14 aarch64 in build 90804 and darwin 26 aarch64 in build 90814, and the annotation marks it pre-existing. The darwin 14 lane that failed in the first build passed in the second, so this is a roaming host issue, not something in the diff. I already re-ran the build once for this, so I will leave it here rather than retrigger again. The new test itself passed on every platform that executed it. Needs a maintainer to retry the one mac job or land once the lane is healthy. |
1 participant
What
Adds one test to the gated `byte sources at the 2 GiB string limit` block in `test/js/web/fetch/blob-oom.test.ts`: `new Response(new Uint8Array(2 ** 31)).text()` and `.json()` must reject with `ERR_STRING_TOO_LONG` instead of aborting the process.
Why
Fuzzing hit this abort on a build that predates #37215:
```
ASSERTION FAILED: data.size() <= MaxLength
wtf/text/StringImpl.h(891) : WTF::StringImplShape::StringImplShape(uint32_t, std::span, unsigned int)
```
repro: `new Response(new Uint8Array(3221225471)).text()` (release builds die with a silent SIGABRT via the RELEASE_ASSERT).
Current main already handles this correctly, the guards added in #37215 cover it. But the tests added there only exercise `new Blob([...])` and `Bun.file()`. A typed-array body takes a different path: it is stored as an InternalBlob (`Body.rs` copies the bytes into a Vec) and converts through `Internal::to_string_owned`, whose ASCII fast path goes through the `to_external_value` guard rather than the `external` guard the Blob store path uses. A regression in that path would abort the process again without any test catching it.
Verification
[stamp-90s] gate passed · iteration 1 · 1 files touched
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file