fetch: report early rejections as unhandled rejections - #37434
Conversation
Every early exit in fetch() (missing or blank URL, unsupported scheme, unresolvable blob: URL, GET with a body, pre-aborted signal, unreadable Bun.file() body, s3 signing errors, exceptions thrown while converting the arguments, ...) built its rejected promise with JSPromise::dangerously_create_rejected_promise_value_without_notifying_vm, which never tells the promise rejection tracker about the rejection. When the caller attached no handler, nothing was printed, unhandledRejection listeners never ran, and the process exited 0. Build those promises with JSPromise::rejected_promise instead, the same helper Body.rs, streams.rs and the s3 client already use, so an unhandled early rejection is reported like any other rejected promise.
|
Warning Review limit reached
Next review available in: 26 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 (3)
Comment |
|
Status Reproduced on the current release and on main: Fix: the 20 early-exit sites in CI was green on 5a48030 (build 91997, 190/190); the two pushes since only touch tests. Ready for review. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — mechanical swap of a deprecated helper for its documented replacement, with a subprocess test per call site.
Checked that JSPromise::rejected_promise is the exact replacement the deprecation comment on both the Rust helper (src/jsc/JSPromise.rs:330) and the C++ binding (bindings.cpp:5237) points to, and that Body.rs/streams.rs/s3/client.rs/Image.rs already use it in the same .to_js() shape.
Confirmed GlobalObject::promiseRejectionTracker for Reject only appends the promise to m_aboutToBeNotifiedRejectedPromises (ZigGlobalObject.cpp:1101) — no user JS runs synchronously, so no new reentrancy at these mid-fetch_impl call sites.
Tests follow harness conventions (bunEnv/bunExe/tempDir, concurrent pipe drain, describe.concurrent); the 127.0.0.1:1 and /tmp/fetch-args.sock inputs are never opened because every case rejects before the network path. The "Bun.file() body that is a directory" test's Windows behavior was examined and ruled out — the read-file fallback surfaces EISDIR there too.
Extended reasoning...
Overview
Replaces 20 uses of JSPromise::dangerously_create_rejected_promise_value_without_notifying_vm in src/runtime/webcore/fetch.rs with JSPromise::rejected_promise(global, err).to_js(), so early fetch() rejections are registered with the promise rejection tracker and reported via unhandledRejection when nothing catches them. Adds a describe.concurrent block to test/js/web/fetch/fetch-args.test.ts with 21 subprocess tests covering 19 of the 20 call sites plus the unhandledRejection listener contract and the handled-rejection-is-not-reported case.
Security risks
None. No parsing, allocation, or trust boundary changes; error values and messages are unchanged. The rejection tracker path for Reject (ZigGlobalObject.cpp:1099-1102) only appends to a WriteBarrier vector, so switching to it introduces no synchronous re-entry into user JS while fetch_impl still holds RAII guards (SignalRef, FetchHeadersRef, the sendfile fd guard, etc.).
Level of scrutiny
Low-to-medium. The Rust change is a purely mechanical substitution at every early-exit site — same arguments, same return type via .to_js() — of a helper whose own doc comment says "DEPRECATED, use rejected_promise instead", and the pattern is already in production use in four other webcore/image files. It is a user-visible behavior change (scripts that let an early fetch rejection fall on the floor now exit 1), but that is the stated fix, matches Node, and matches what Bun already does for the same fetch() call once it reaches the network path.
Other factors
The tests are unusually thorough for this kind of change: one spawned process per changed exit, exact stderr substring + exit code, plus a positive check that process.on('unhandledRejection') receives the returned promise and a negative check that a synchronously handled rejection is not reported. They use bunEnv, bunExe, tempDir, drain stdout/stderr/exited concurrently, and JSON.stringify the temp-dir path so Windows backslashes survive the -e string. None of the http://127.0.0.1:1/ inputs reach the socket layer since every case rejects before FetchTasklet::queue. The one call site not covered ("Failed to start s3 stream") is noted in the description as untriggerable from JS. No prior reviews to reconcile; no CODEOWNERS on these files.
|
Updated 5:08 AM PT - Aug 11th, 2026
✅ @robobun, your commit 3643b50bb63c6e39b61c44b7cd51152c9e533111 passed in 🧪 To try this PR locally: bunx bun-pr 37434That installs a local version of the PR into your bun-37434 --bun |
expect(fn).toThrow() reports every rejection that is still pending when it is called. Now that fetch() registers its early rejections with the VM, the 1000 promises this test creates before asserting on them were reported as unhandled. expect(promise).rejects marks the promise handled when the assertion runs, which is what the test meant.
There was a problem hiding this comment.
LGTM — mechanical swap from the deprecated dangerously_create_rejected_promise_value_without_notifying_vm to JSPromise::rejected_promise(...).to_js(), which is exactly what the doc comments on both the Rust helper (src/jsc/JSPromise.rs:330) and the C++ binding (bindings.cpp:5237) prescribe.
What was reviewed
- All 20 call sites: same one-line substitution, no error values or control flow changed;
reject_on_exceptionclears the pending exception before building the promise, so calling into the tracker is safe. - Confirmed
rejected_promise().to_js()is the established pattern already used inBody.rs,streams.rs,s3/client.rs, andImage.rs. fetch-file-upload.test.ts:.rejects.toThrow()attaches a handler synchronously so the 1000-iteration GC loop doesn't emit unhandled-rejection reports; the un-awaited assertion is pre-existing andBun.peek.status(resp)remains the load-bearing check.- New tests spawn a subprocess per exit path and assert stderr + exit 1, plus a handled-rejection negative test — none contact the network.
Extended reasoning...
Overview
The PR replaces JSPromise::dangerously_create_rejected_promise_value_without_notifying_vm(global, err) with JSPromise::rejected_promise(global, err).to_js() at all 20 early-exit sites in src/runtime/webcore/fetch.rs (plus data_url_response and reject_on_exception). It adds a 21-test describe.concurrent block in test/js/web/fetch/fetch-args.test.ts that spawns bun -e for each exit path and asserts the rejection reaches stderr with exit 1, plus two tests confirming unhandledRejection listeners fire and that handled rejections stay silent. test/js/bun/http/fetch-file-upload.test.ts swaps expect(async () => await resp).toThrow(...) for expect(resp).rejects.toThrow(...) so the now-tracked rejections are handled synchronously inside the 1000-iteration GC loop.
Security risks
None. No input parsing, validation, or trust boundary changes — only which JSC helper constructs the already-rejected promise. JSC::JSPromise::rejectedPromise registers the promise with the rejection tracker (a bookkeeping list) and does not run user JS synchronously.
Level of scrutiny
Medium: fetch() is a high-traffic user-facing API, but the change is a mechanical one-line substitution repeated 20 times. Both the Rust helper's doc comment ("DEPRECATED use rejected_promise instead") and the C++ binding's comment explicitly direct callers to the replacement used here, and four sibling files in src/runtime/webcore/ already use it. The user-visible behavior change (silent exit 0 → unhandled-rejection report + exit 1) aligns fetch's argument-validation exits with its own network-path rejections and with Node.js.
Other factors
- Test coverage is thorough: 19 of 20 exit paths get a subprocess test (the one omission, "Failed to start s3 stream", is an internal-failure branch the author could not trigger from JS and says so), plus positive (
unhandledRejectionlistener receives the returned promise) and negative (.catch()suppresses the report) checks. Tests usebunEnv/bunExe/tempDir, drain stdout/stderr/exited concurrently, and hit no external network (127.0.0.1:1andgopher://reject before any socket is opened). - The
fetch-file-upload.test.tsadjustment is required by the fix (otherwise 1000 unhandled-rejection reports) and does not weaken the assertion —Bun.peek.status(resp) === "rejected"is still asserted synchronously each iteration, and.rejectsattaches a handler before the tick ends. - One candidate issue ("un-awaited
.rejects.toThrow()in the loop") was raised and refuted: the un-awaited state is pre-existing, and the primary synchronous assertion remains.
With the untracked promise, attaching a handler still reached the tracker's handled path, so process emitted rejectionHandled (and PromiseRejectionHandledWarning under --unhandled-rejections=warn) for a rejection that was never reported as unhandled.
The tracker holds the 1000 rejected promises until the end of the tick, so the trailing Bun.gc(true) was no longer collecting anything. Yield one turn of the event loop first; a microtask is not enough because the list is drained after the microtask queue.
There was a problem hiding this comment.
LGTM — the earlier note on fetch-file-upload.test.ts is addressed (3643b50 adds await Bun.sleep(0) before the final Bun.gc(true) so the rejection tracker drains first).
What was reviewed:
- All 20
fetch.rssites: identical swap from the deprecateddangerously_create_rejected_promise_value_without_notifying_vmtorejected_promise(...).to_js(), which is exactly whatJSPromise.rs:324documents. - New tests cover 19 of the 20 exits via subprocess spawns plus the
unhandledRejection/rejectionHandledcontract; the one uncovered exit ("Failed to start s3 stream") is noted in the description. - Checked that
rejected_promisereturns&mut JSPromise(not aJSValue), so the added.to_js()is the intended shape and no Strong ref is introduced.
Extended reasoning...
Overview
Swaps 20 early-rejection sites in src/runtime/webcore/fetch.rs from JSPromise::dangerously_create_rejected_promise_value_without_notifying_vm(global, err) to JSPromise::rejected_promise(global, err).to_js(), so those rejections reach promiseRejectionTracker and behave like every other rejected promise (reported when unhandled, no spurious rejectionHandled when handled). Adds a 21-test describe.concurrent block in test/js/web/fetch/fetch-args.test.ts covering each reachable exit via bun -e subprocesses, and updates one existing GC-stress test in test/js/bun/http/fetch-file-upload.test.ts to use .rejects.toThrow() plus a tick before the final forced GC.
Security risks
None. No parsing, no auth, no input validation changes — only the promise-construction helper is swapped. Error values and messages are unchanged.
Level of scrutiny
Low-to-medium. The Rust change is purely mechanical: the deprecated helper's own doc comment (src/jsc/JSPromise.rs:330) says "use rejected_promise instead", and rejected_promise's doc (line 324) says "use .to_js()" for a JSValue result — this PR does exactly that at every site. It's the same helper Body.rs, streams.rs, s3/client.rs and ErrorCode::reject() already use. The user-visible behavior change (unhandled early fetch() rejections now report and exit 1) is a bug fix that aligns with Node and with Bun's own network-path rejections from the same call.
Other factors
- My previous inline comment (rejection tracker pinning the 1000 promises past the forced GC) is addressed in commit 3643b50 with
await Bun.sleep(0)and an explanatory comment; the author correctly noted a microtask alone would not drainhandleRejectedPromises(). - CI was green on build 91997 (190/190) before the last two test-only commits.
- Tests follow harness conventions:
bunEnv/bunExe,tempDir, concurrent subprocess drain viaPromise.all([stdout, stderr, exited]),describe.concurrentfor the independent spawns, and specific error-message assertions. - No memory-safety surface:
rejected_promisereturns a GC-managed&mut JSPromiseand.to_js()is a plainJSValueconversion; no new refs or roots at the call sites.
1 participant
What does this PR do?
When
fetch()rejects before it ever touches the network and nothing handles that rejection, Bun prints nothing and exits 0:The error exists (
.catch()receivesTypeError: protocol must be http:, https: or s3:andFailed to resolve blob:...), it is just never reported.process.on("unhandledRejection")listeners do not run either. Node reports both as unhandled rejections and exits 1, and so does Bun for every other rejected promise, including the onesfetch()produces later on from the network path.The handled side is off too: because the tracker never saw the rejection, attaching a handler (
fetch("gopher://example.com/").catch(...)) makes it look like a late handler for an already reported rejection, soprocessemits a spuriousrejectionHandledevent, and--unhandled-rejections=warnprintsPromiseRejectionHandledWarningfor a rejection that was handled synchronously.Cause: every early exit in
fetch_impl(src/runtime/webcore/fetch.rs) builds its promise withJSPromise::dangerously_create_rejected_promise_value_without_notifying_vm. That helper (JSC__JSPromise__rejectedPromiseValue) sets the promise's rejected flag and result slot directly and never callspromiseRejectionTracker, which is exactly what its doc comment insrc/jsc/JSPromise.rswarns about. The affected exits are: no arguments, blank URL, unparsable URL,data:URLs that fail to parse or decode, invalidproxy(string and object form),signalthat is not anAbortSignal(init and input object form),proxycombined withunix, unresolvableblob:URL, unsupported scheme, GET/HEAD with a body, already aborted signal, aBun.file()body that fails to open or read, s3 signing errors, a stream body with a non-upload method on s3, andreject_on_exception, which turns any exception thrown while converting the arguments into a rejection.Fix: build them with
JSPromise::rejected_promise(global, err).to_js()instead. That goes throughJSC::JSPromise::rejectedPromise, so the rejection is registered with the tracker like aPromise.reject()from JS. It is the helperBody.rs,streams.rs,s3/client.rsandErrorCode::reject()already use for the same purpose. The change is the same one line at each of the 20 call sites infetch.rs; no messages or error types change.Why this is the right behavior
The promise is handed straight back to the caller, so the usual rules apply: attaching a handler synchronously (
.catch(),await,expect(...).rejects) removes it from the pending list before anything is reported, and only a rejection nobody handles by the end of the tick is reported. That is what the network-path rejections from the samefetch()call already do, so callers no longer get different reporting depending on how farfetch()got before failing. It also matches Node for these inputs.bun:test'sexpect(fn).toThrow()captures rejections produced duringfnthrough its own scope, so the existingfetch-args.test.tstests that assert on these errors viatoThrowstill pass unchanged.One existing test needed updating:
test/js/bun/http/fetch-file-upload.test.ts("missing file throws the expected error") creates 1000 rejectedfetch()promises and then asserts on each withexpect(async () => await resp).toThrow(...).toThrow()drains the rejections that are still pending when it is called, so with the promises now registered the first assertion reported the rest as unhandled (the same thing happens today ifrespis a plainPromise.reject()). The test now usesexpect(resp).rejects.toThrow(...), which marks the promise handled as part of the assertion. That was the only failure in the full CI run of the first commit. Review then pointed out that the tracker keeps the 1000 promises alive until the end of the tick, which would have turned the trailingBun.gc(true)into a no-op; verified withheapStats()(1000 promises and 1000 errors still live at that point, and still live after a bareawait, since the list is drained after the microtask queue), so the test now yields one event loop turn withawait Bun.sleep(0)before the forced collection, after which the counts are back to zero.The other users of the deprecated helper (
Bun.write,server.fetch(),Bun.resolve(), ...) have the same problem and are being handled separately; #37425 adds a newfile:rejection that already usesrejected_promise.How did you verify your code works?
New
describeblock intest/js/web/fetch/fetch-args.test.ts. It spawnsbun -efor 19 of the 20 exits above (the remaining one, "Failed to start s3 stream", needs an internal stream creation failure that I could not trigger from JS) and checks that the error reaches stderr and the process exits 1. Two more tests check that aprocess.on("unhandledRejection")listener receives the rejection with the returned promise, and that a rejection handled with.catch()is neither reported nor followed by arejectionHandledevent. All 21 fail on the current release (empty stderr and exit 0 for the first twenty, the spuriousrejectionHandledfor the last one); all pass with this change, together with the rest offetch-args.test.ts.Also ran
fetch.test.ts,fetch.stream.test.ts,fetch-redirect.test.ts,fetch.unix.test.ts,client-fetch.test.ts,blob.test.ts, thenode-fetchandundicishim tests with the debug build. The only failures are the same ones the unmodified build has in this environment (localhost connections refused,(with gc)tests and a few compression tests exceeding their timeout under the debug build when the whole file runs; they pass in isolation).no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/http/fetch-file-upload.test.ts