Repository navigation
Conversation
…frame "handle stack overflow" created a Response and called text() in every one of the ~26k frames it took to exhaust the stack. On a debug build that is 5 to 8 seconds, past the default 5 second per-test timeout, so the file fails on main under `bun bd test`. Recurse with empty frames instead and only create Responses from the deepest frames once the stack has run out (about 40ms on a debug build), and assert on what those calls return. "print size" snapshotted the byte size of this test file, so the snapshot had to change with every edit to the file. Inspect a fixture with a fixed size instead.
|
Warning Review limit reached
Next review available in: 10 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
|
Status: reproduced and fixed, waiting on CI.
|
There was a problem hiding this comment.
LGTM — test-only refactor that speeds up a debug-build timeout and stops a snapshot from churning on every edit.
What was reviewed:
handle stack overflow: confirmed the deepest surviving frame still calls.text()with the JS stack at its limit (host calls skip the prologue check), so the #23961 condition is preserved; exactly-one-overflow invariant holds because every parent'srecurse()returns normally after the deepest catch.- New assertions are strictly stronger — the old test never awaited the promises; a pending/rejected
.text()now fails viaPromise.all. print size:tempDir+normalizeBunSnapshot(_, dir)matches the harness convention and the<dir>substitution path intest/harness.ts:2131; also fixesimport.meta.dirbeing passed toexpect()instead of the normalizer.
Extended reasoning...
Overview
This PR touches a single test file, test/js/web/fetch/response.test.ts, with two independent improvements and no production code changes:
handle stack overflowis rewritten to reach stack exhaustion via cheap empty-frame recursion, catch theRangeErrorin the deepest frame, and callnew Response().text()only from the 32 deepest frames while unwinding — instead of allocating a Response and awaiting.text()in every one of ~26k frames. This drops debug-build runtime from 5–8s (which trips the default 5s timeout underbun bd test) to ~40ms, and adds an assertion the old test lacked: that all 32.text()promises actually resolve to"".print sizenow inspects a fixed-size 18-byte fixture in atempDirrather thanimport.meta.filename, so the inline snapshot no longer encodes this file's own byte count and won't need bumping on every edit. It also fixes a latent bug whereimport.meta.dirwas the second argument toexpect()(a no-op custom message) rather than tonormalizeBunSnapshot.
Security risks
None. Test-only change; no runtime, network, filesystem-permission, or auth surface is affected. The tempDir helper is the harness-standard, self-cleaning temp directory (using), so no leftover state.
Level of scrutiny
Low-to-moderate. The only real question is whether the rewritten stack-overflow test still exercises the #23961 regression path. I traced the mechanism: JSC's stack check fires at the JS function prologue, so the frame whose recursive call just failed is by construction at the JS stack limit; new Response() and .text() are host calls that skip that check and run in JSC's reserve zone, so they execute in exactly the near-exhausted state #23961 cared about. Because every frame wraps its recursive call in try/catch, only the deepest frame's catch fires (all shallower recurse() calls return normally), making overflows.length === 1 deterministic. The author validated this across JIT on/off, ulimit -s 1024/unlimited, and 40 runs — well beyond what REVIEW.md asks for de-flaking work.
The new test is strictly stronger than the old one: previously it only asserted that something threw the overflow message (the recursion itself would, regardless of what .text() did) and never inspected the returned promises. Now a pending promise (the pre-#23961 symptom) hangs Promise.all and times out; a rejected one fails the equality; a leaked pending exception would show up as a second entry in overflows.
Other factors
- Follows harness conventions verbatim (
tempDirwithusing,normalizeBunSnapshot(str, dir)→<dir>pertest/harness.ts:2131-2135), matching the CLAUDE.md example pattern. - No CODEOWNERS entry covers this path.
- The PR description is unusually thorough — it explains the JSC stack-check mechanism, cites the exact
JSPromise.wrappath, provides a shape-vs-runtime table, and flags the two conflicting PRs (#32044, #37893). The claim that the oldprint sizesnapshot had to be bumped on 6/6 prior edits is consistent with it encoding the file's own byte size. - No prior reviews or unaddressed comments; only a CodeRabbit rate-limit notice in the timeline.
|
Updated 3:06 PM PT - Aug 12th, 2026
❌ @robobun, your commit e3e5fff has 2 failures in
🧪 To try this PR locally: bunx bun-pr 37910That installs a local version of the PR into your bun-37910 --bun |
Problem
bun bd test test/js/web/fetch/response.test.tsfails on main:(fail) handle stack overflow/this test timed out after 5000ms. The test takes 5 to 8 seconds on a debug build (~365ms on the release binary), so the file is not a usable pass/fail signal for changes to Response when run the way CLAUDE.md says to.Responseand calls.text()in every frame of a recursion that only overflows after ~26k frames (test/js/web/fetch/response.test.ts:152on main). A debug build spends ~0.2ms per frame in the constructor andtext(), which is where the time goes.scripts/runner.node.mjspasses a much larger per-test timeout.print sizeinspectsBun.file(import.meta.filename), so its inline snapshot encodes the byte size of the test file itself and has had to be bumped by every change to this file so far (6 of 6, and again by this one).Fix
handle stack overflownow recurses with empty frames, catches the overflow in the deepest frame, and callsnew Response().text()only from the 32 deepest frames on the way back out. It asserts that exactly oneRangeError: Maximum call stack size exceeded.was thrown and that all 32 promises resolve to"".text()path runs with no JS stack left in each of those frames. The old shape only reached that state in the last few of its ~26k frames; the rest were padding.ulimit -s 1024andunlimitedas well.text()returned in the frames at the limit. The new one does.JSPromise::resolvedPromiseis now plain C++ for a non-object result (vendor/WebKit/Source/JavaScriptCore/runtime/JSPromise.cpp,promiseResolve->resolvePromise->fulfillPromise) and cannot throw there, so on maintext()in the deepest frames returns fulfilled promises too (checked withBun.peek.statuson both builds), and the only overflow the old test saw came from its own JS recursion. What both shapes verify is that the native path survives being called with the JS stack exhausted; the new one does it at the deepest frame deterministically and checks the promises it gets back.print sizeinspects a fixture of fixed size in atempDirinstead of the test file, so the snapshot stops depending on this file's length. (It also passes the directory tonormalizeBunSnapshot; before,import.meta.dirwas being passed toexpect()by mistake, and the<cwd>replacement happened to cover for it.)bun bd test test/js/web/fetch/response.test.tspasses 23/23.handle stack overflowtakes 36 to 51ms on the debug build and 4 to 7ms on release. 10 debug runs and 30 release runs of the file, all green.print sizesnapshot. Both hunks conflict trivially with this.Background
RangeError: Maximum call stack size exceeded.into the caller, which is left intact and can catch it. Calls from JS into host (native) functions such as theResponseconstructor andResponse.prototype.textdo not perform that check; they run in the stack space JSC keeps in reserve below the JS limit. That is why the deepest frame can still create a Response after catching the overflow, and why doing it there exercises "native promise-returning method called with the JS stack exhausted", the condition Fix unhandled exception in JSC__JSPromise__wrap when resolving promise #23961 was about.JSPromise.wrap(src/jsc/JSPromise.rs, backed byJSC__JSPromise__wrapinsrc/jsc/bindings/bindings.cpp) is the pathtext()takes on an empty body: it runs the native body conversion and wraps the result in a promise. Fix unhandled exception in JSC__JSPromise__wrap when resolving promise #23961 made it convert an exception raised while creating that promise into a rejected promise instead of leaving it pending.Measurements (debug build in this container)
bun bd test test/js/web/fetch/response.test.ts -t "handle stack overflow"on main: 7560ms, reported asthis test timed out after 5000ms. The report that prompted this saw 8029ms, 7655ms and 5081ms on another machine. The recursion alone, run as a script with the debug binary: 7288ms, 5254ms, 4936ms.Cost of different test shapes, same debug binary: