Report unhandled early rejections from Bun.write, server.fetch and Bun.resolve - #37474
Report unhandled early rejections from Bun.write, server.fetch and Bun.resolve#37474robobun wants to merge 3 commits into
Conversation
…friends The promises these APIs return when they fail before doing any work were built with JSPromise::dangerously_create_rejected_promise_value_without_notifying_vm, which never registers the rejection with the promise rejection tracker. An unhandled one was therefore silently dropped and the process exited 0. Build them with JSPromise::rejected_promise instead, like the rest of the runtime does. Sites that rejected with a caught exception used to hand out the JSC::Exception cell itself; they now reject with the thrown value via JSPromise::rejected_promise_with_caught_exception. server.fetch() also takes the exception thrown while converting the body off the VM instead of returning a promise with it still pending. Bun__resolve, another user of the helper, had no callers and is removed.
|
Warning Review limit reached
Next review available in: 22 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 (11)
Comment |
|
Status: reproduced on the 1.4.0 release and on main ( |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Neither of the two is a duplicate of this PR:
|
|
Updated 8:18 AM PT - Aug 11th, 2026
✅ @robobun, your commit eeedfe4373c3d0be10677d396ecbdcc71f707544 passed in 🧪 To try this PR locally: bunx bun-pr 37474That installs a local version of the PR into your bun-37474 --bun |
There was a problem hiding this comment.
I re-reviewed after bf55edb/eeedfe4 and didn't find any bugs. My earlier Windows EBADF concern was checked on a real Windows canary and a comment was added; the doc-comment length flags were addressed. Because this changes user-visible behavior (silent → reported unhandled rejections, exit 0 → 1) across ~30 rejection sites in JSC/runtime code and includes a real semantic fix in server.fetch()'s body-conversion path, a maintainer sign-off would still be worthwhile.
What was reviewed:
- Confirmed
Bun__resolve/bun_resolvehave no remaining callers (C++ extern, HOST_EXPORT marker, and Rust fn all removed together). - The new
rejected_promise_with_caught_exceptioncomposes existingcreate()+reject(Err(err)), so termination handling and Exception-cell unwrapping reuse the auditedrejectpath. pipe_readable_stream_to_blob's switch tounwrap(MarkHandled)matches the pattern inblob/write_file.rsand prevents double-reporting the internal stream promise.- Remaining users of the deprecated helper are only
fetch.rs(covered by #37434) and the definition itself.
Extended reasoning...
Overview
The PR converts every remaining call site of JSPromise::dangerously_create_rejected_promise_value_without_notifying_vm outside fetch.rs (~30 sites across Blob.rs, server_body.rs, BunObject.rs, ByteStream.rs, node_fs_binding.rs, subprocess.rs, and JSPromise.rs::wrap_value) to JSPromise::rejected_promise(...).to_js(), so early rejections from Bun.write, server.fetch, Bun.resolve, etc. are registered with the rejection tracker like every other rejection in Bun. Six sites that previously rejected with a raw JSC::Exception cell now go through a new 5-line helper rejected_promise_with_caught_exception that takes the pending exception off the VM and rejects with the thrown value (propagating termination). One site in server_body.rs additionally fixes a pre-existing bug where the body-conversion Err was discarded without clearing the pending exception. The dead Bun__resolve FFI export is removed. The large hunk in write_file_internal is rustfmt re-indentation of a closure whose only over-long line was one of the converted calls.
Security risks
None identified. No new input parsing, no auth/crypto/permission changes; the change routes existing error values through the tracked rejection path instead of the untracked one.
Level of scrutiny
Medium-high. Each individual site is a mechanical one-line swap to the helper the deprecated function's own doc comment says to use, and the new helper is a trivial composition of existing primitives. But collectively this is a user-visible behavioral change (processes that previously exited 0 with a silent rejection now print the error and exit 1) across many entry points, plus a real semantic fix in server.fetch() (pending-exception leak → proper rejection) and a MarkHandled adjustment in pipe_readable_stream_to_blob. That's the kind of intent change a maintainer should confirm, even though the PR description makes a thorough case that it aligns these paths with Node and with Bun's own async paths for the same calls.
Other factors
- My previous inline finding (Windows
EBADFvsEPERMin the read-only-fd test rows) was resolved: the author ran the exact test rows on a Windows canary, observedUV_EBADF(errno -4083), and added a comment explaining the substring match. The comment-cop long-comment flags were also addressed in eeedfe4. - I verified
Bun__resolvehas no remaining references anywhere (C++ header,HOST_EXPORTcodegen marker, Rust body all removed), and that the only remaining users of the deprecated helper arefetch.rs(#37434) and the definition itself. - Test coverage is strong: subprocess-spawned checks of stderr + exit code for each converted early-exit, in-process
rejects.toBe(error)identity checks for the Exception-cell fix, anunhandledRejectionlistener identity check, and negative tests that handled rejections stay silent. robobun confirmed fails-before/passes-after on the release build and on main. - No outstanding human reviewer comments.
What does this PR do?
When
Bun.write(),server.fetch()orBun.resolve()fail before doing any work, the rejected promise they return is invisible to the unhandled rejection machinery. Nothing is printed and the process exits 0:Adding
.catch()to any of them shows the error is there (ENOTDIR,ERR_MODULE_NOT_FOUND,fetch() expects a string but received no arguments.), andprocess.on("unhandledRejection")listeners never see it either. Every other rejection in Bun is reported and exits 1, including the sameBun.write()failure once it takes a different internal route:Bun.write("plainfile/x", "x".repeat(300000))andBun.write("plainfile/x", new Blob(["x"]))both print theENOTDIRerror and exit 1 today, because only payloads under 256 KiB go through the synchronous fast path. On Windows, which has no fast path, all of them are reported. Whether a failed write is reported currently depends on the payload size and type.Cause: these early exits build their promise with
JSPromise::dangerously_create_rejected_promise_value_without_notifying_vm(JSC__JSPromise__rejectedPromiseValuein bindings.cpp), which sets the rejected flag and the result slot directly and never callspromiseRejectionTracker. Its doc comment has said this since it was renamed in #18549.fetch()has the same problem and is fixed separately in #37434; this PR converts every other user:src/runtime/webcore/Blob.rsBun.write(),BunFile.write(),S3File.write(),S3Client.write(): sync fast paths for strings and typed arrays, empty Blob sources, S3 option validation, S3 stream setup failures, erroredResponse/Requestbody sources,pipe_readable_stream_to_blobsrc/runtime/server/server_body.rsserver.fetch(): no arguments, blank URL, wrong argument type, no fetch handler, body conversion failure, handler throwing, returning an Error, returning undefinedsrc/runtime/api/BunObject.rsBun.resolve()src/runtime/webcore/ByteStream.rstext()/json()/... on a native stream that already failed (the C++ buffered fast path chains a reaction onto this promise, so the change is not observable there)src/runtime/node/node_fs_binding.rsfs.promises.readFile/writeFile/appendFilewith an already aborted signal (the JS wrappers adopt the promise, not observable either)src/runtime/api/bun/subprocess.rssubprocess.exitedread after awaitpidfailure; the promise created while the process was still running is already rejected through the tracked path, so this makes both reads behave the samesrc/jsc/JSPromise.rswrap_value(only used byformData(), whose value is never an error)Fix: build them with
JSPromise::rejected_promise(global, err).to_js(), the helperBody.rs,streams.rs,s3/client.rsandErrorCode::reject()already use. That goes throughJSC::JSPromise::rejectedPromise, so the promise is registered with the tracker exactly like aPromise.reject()from JS. Two kinds of sites needed slightly more than the one-line swap:Bun.writepaths,Bun.resolve, theserver.fetch()handler call and its body conversion). They usedtake_exception(), which returns theJSC::Exceptioncell, and rejected with that.awaithappens to unwrap it, but.catch()callbacks received the bare cell: nomessage, nocode,instanceof TypeErrorfalse, andString(e)throws.JSC::JSPromise::rejectedPromisealso asserts it is not given anException. These now go through a small newJSPromise::rejected_promise_with_caught_exception(global, err), which iscreate()+ the existingreject(Err(err)): it takes the exception off the VM, rejects with the thrown value, and propagates a termination instead of turning it into a rejection reason. The body conversion site inserver.fetch()additionally had a real bug: it discarded theErrwithout clearing the exception, so the generic "fetch() received invalid body" promise it built was never observed. Release builds threw the conversion error synchronously out ofserver.fetch()and debug builds hitExceptionScope::releaseAssertNoException. It now rejects with that error, likefetch()does for its argument conversion errors.pipe_readable_stream_to_blobforwards the rejection of the stream's own promise. It now reads it withunwrap(MarkHandled)(the patternblob/write_file.rsandRequestContext.rsuse) so that only the promise handed to the caller is reported, not the internal one as well.Bun__resolveinBunObject.rswas another user of the helper. Nothing calls it on either side (the only reference was the extern declaration inImportMetaObject.h), so it is removed instead of converted. The deprecated Rust helper andJSC__JSPromise__rejectedPromiseValuethemselves stay until #37434 removes thefetch.rsuses; whichever of the two lands second can delete them.Note on the diff: the large hunk in
write_file_internal(Blob.rs) iscargo fmtre-indenting thebody_dispatchclosure. Its only over-long line was one of the converted calls, which had made rustfmt skip the whole closure until now; apart from the one converted call,git diff -wshows only re-indentation and re-wrapping in it.Why this is the right behavior
The promise is returned straight to the caller, so the normal rules apply unchanged: a handler attached before the microtask checkpoint (
.catch(),await,expect(p).rejects, adopting it from an async function) clears it from the pending list and nothing is reported; only a rejection nobody handles is. That is already how the async paths of the very same calls behave, so this removes the dependence on which internal route a call happened to take, and it matches Node, where every unhandled rejection is reported. I audited the native consumers of these promises: every one either returns the value to JS, resolves another promise with it (Image.rs), or reads it withunwrap(MarkHandled)(blob/write_file.rs), so none of them produces a new spurious report. I also checked the test suite for code that creates one of these rejections and handles it late; there is none (the one such case forfetch()is handled in #37434).How did you verify your code works?
New tests, all spawning
bun -eand asserting on stderr and the exit code, plus in-process checks of the rejection reason:test/js/bun/io/bun-write.test.js: string and typed array to a directory (both fast-path open errors) and to a read-only fd (both fast-path write errors), empty Blob to a directory,BunFile.write(), S3 writes with an invalidstorageClassfor a string and an empty Blob source (the two S3 validation sites); that the S3 rejection reason is theTypeErroritself; that the promise passed tounhandledRejectionis the one returned; and that a handled rejection is still silent.test/js/bun/http/bun-serve-fetch-invalid-args.test.ts: all eightserver.fetch()exits as unhandled rejections,rejects.toBe(error)for a throwing handler (Error and non-Error values) and for a handler returning an Error, a rejection instead of a synchronous throw when the body cannot be converted, theunhandledRejectionidentity check and the handled case.test/js/bun/resolve/resolve-error.test.ts:Bun.resolve()reported, delivered tounhandledRejection, and silent when handled.On the current release 11 of 12 tests in the new
bun-writeblock, 10 of 14 in theserver.fetchfile and 2 of 3 in the resolve block fail (empty stderr and exit 0, or the bare exception cell as the reason); all pass with this change. The remaining converted sites (S3 stream setup failures, errored body sources,pipe_readable_stream_to_blob,ByteStream, the aborted-signal fs path,subprocess.exitedafter awaitpidfailure,wrap_value) are the same one-line change; the first group needs S3 or internal stream failures to reach and the rest are not observable from JS, as described in the table above.Also ran with the debug build: the rest of
bun-write.test.js,resolve.test.ts,bun-object/write.spec.ts,util/fuzzy-wuzzy.test.ts(calls everyBun.*function with no arguments),bun-serve-routes.test.ts,image.test.ts(resolves with aBun.writepromise),spawn.test.ts,node/fs/promises.test.js,web/fetch/body*.test.ts,body-stream,fetch-abort-stream-body,stream-fast-path,FormData.test.tsandutil/bun-file.test.ts. All pass exceptbun-write.test.js"copyFileRange is not available > on large files", which fills a 256 MiB buffer in a JS loop and exceeds its 5 s timeout under the debug build in this environment regardless of this change (0.9 s on the release build).cargo clippyonbun_jscandbun_runtimeis clean.