Repository navigation
Run the ticks and microtasks that 'unhandledRejection' listeners queue - #45597
dylan-conway wants to merge 1 commit into
Conversation
… and match Node's captureRejections
|
Updated 7:07 PM PT - Oct 16th, 2026
✅ @dylan-conway, your commit b70b4421089180d353b2517edcbd78201b4f07c2 passed in 🧪 To try this PR locally: bunx bun-pr 45597That installs a local version of the PR into your bun-45597 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe changes update EventEmitter rejection capture for thenables and adjust unhandled rejection reporting to signal and perform required microtask checkpoints. Tests cover thenable capture, capture-setting behavior, and the ordering of unhandled rejection listeners and follow-up work. ChangesRejection Handling
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly and concisely describes the main change: executing ticks and microtasks queued by unhandledRejection listeners. |
| Description check | |
The description includes both required sections. It explains the behavior change, related captureRejections changes, known difference from Node, and detailed verification results. |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
✨ Finishing Touches: 💡 1
-
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked the FFI signature change for Bun__handleRejectedPromise: the C shim is generated from the Rust pub fn under the HOST_EXPORT marker (src/codegen/generate-host-exports.ts), so the new -> bool is the single source of truth and matches the extern "C" bool declaration in ZigGlobalObject.cpp. Also looked at whether addCatch reading emitter[kCapture] lets a later captureRejections = false leak capture onto emitters that already reported — it does not, the check happens per call.
Extended reasoning...
The change touches the unhandled-rejection checkpoint ordering across Rust (VirtualMachine.rs, virtual_machine_exports.rs) and C++ (ZigGlobalObject.cpp) plus node:events captureRejections in src/js/node/events.ts, with new tests in both suites. No security-sensitive surface. The inline findings already signal that a human look is needed; this note only records what else was examined and ruled out.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/js/node/events.ts— Subclasses that overrideemitand are constructed while capture is on never have their override called, unlike Node. The constructor stamps an ownemitat src/js/node/events.ts:120 and events.ts:126, which shadows a prototype-chainemitdefined by the subclass. The setter change at events.ts:1015-1018 guards a userland prototypeemitfor the prototype swap, but the instance stamping it leaves in place has the same flaw fornew Sub({captureRejections:true})and for every emitter built whileEventEmitter.captureRejectionsis true. Fix: stop installing an ownemiton instances and have oneemitbranch onthis[kCapture], or at least skip the stamp whenthis.emit !== EventEmitterPrototype.emit.Why this was flagged
Trigger:
class Sub extends EventEmitter { emit(t, ...a) { log(t); return super.emit(t, ...a); } }thennew Sub({ captureRejections: true }), orEventEmitter.captureRejections = truebeforenew Sub(); any node:events consumer that wraps emit (loggers, tracing, stream subclasses) is in the population. src/js/node/events.ts:120 and events.ts:126 writethis.emit = emitWithRejectionCaptureas an own property, sosub.emit('x')resolves to the own property andSub.prototype.emitnever runs. Node's constructor only setsthis[kCapture]and keeps a single prototypeemit, so the override always runs. Base behaves the same, but the PR adds a 'a userland emit stays' guarantee at events.ts:1015 for the prototype swap and leaves the per-instance twin unguarded, so the class is fixed at one site only. Remedy: remove the instance stamp and branch on kCapture inside one emit, or skip the stamp when an inherited emit is not the built-in.Verification: pre-existing. Trigger: any subclass that overrides
emitand is constructed while capture is on. src/js/node/events.ts:120 and :126 write an OWNemitproperty on the instance, soSub.prototype.emitnever runs. The setter guard at events.ts:1015-1018 does nothing about the instance stamping. Lines 120 and 126 are byte-identical in the base, so the base fails the same way.
| if handle_unhandled() { | ||
| return; | ||
| return true; |
There was a problem hiding this comment.
🟡 (optional) Scripts under plain bun run that import expect from bun:test and call expect(fn).toThrow() while an 'unhandledRejection' listener is installed now run unrelated nextTicks and microtasks synchronously inside that call. The Mode::Bun arm at src/jsc/VirtualMachine.rs:4424 returns true after a listener claims the rejection, and handleRejectedPromises (src/jsc/bindings/ZigGlobalObject.cpp:3425) drains the whole microtask queue, which expect.rs:865 reaches with user test code still on the stack. Fix: let the expect.rs:853/865 reporting sites opt out of the deferred checkpoint, or only drain when handleRejectedPromises is entered from the event loop turn, so the drain never runs under a synchronous user frame. [also at: src/jsc/bindings/ZigGlobalObject.cpp:3425 - Scripts run outside bun test that call expect(fn).toThrow() from bun:test can now have unrelated microtasks and nextTicks run synchronously inside that call.]
Why this was flagged
Trigger: a script run with bun file.ts (isBunTest false, so the early return at src/jsc/VirtualMachine.rs:4396 does not fire) that has process.on('unhandledRejection', ...) and calls expect(() => { Promise.reject(e); throw x }).toThrow(); src/runtime/test_runner/expect.rs:865 calls handle_rejected_promises synchronously inside the matcher. The listener claims the rejection, Mode::Bun returns true at VirtualMachine.rs:4424, and the new drain at src/jsc/bindings/ZigGlobalObject.cpp:3425 runs every pending microtask and nextTick, including ones the caller queued before the expect call, with the caller's frame still active. On base the Mode::Bun claimed arm returned without draining, so those microtasks ran at the next turn as the caller expects. Population: scripts and tools that use bun:test matchers outside the runner (e.g. ad-hoc assertion scripts); frequency is every such toThrow call that observes a claimed rejection. Remedy: suppress the deferred drain when the report is made from expect.rs rather than from the loop.
Verification: /home/claude/bun/src/runtime/test_runner/expect.rs:865 calls vm.global().handle_rejected_promises() synchronously inside the matcher. In Mode::Bun, handle_unhandled() makes the arm return true at VirtualMachine.rs:4424, so ZigGlobalObject.cpp:3425 runs the full checkpoint. On the base, the same arm was if handle_unhandled() { return; } with no drain.
| emit_warning(self); | ||
| } | ||
| drain(self); | ||
| return; | ||
| return false; | ||
| } | ||
| Mode::Throw => { | ||
| if handle_unhandled() { | ||
| drain(self); | ||
| return; | ||
| return false; | ||
| } | ||
| let wrapped = unhandled_rejection_as_uncaught_error(global_object, reason); | ||
| if self.uncaught_exception(global_object, wrapped, true) { | ||
| drain(self); | ||
| return; | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: pre-existing: users running with --unhandled-rejections=throw|warn|none|strict still get Node's old-Bun ordering, not the one this PR adds for the default mode. Only the Mode::Bun arm defers the checkpoint to the batch; every other arm still calls drain(self) per rejection at src/jsc/VirtualMachine.rs:4430, 4436, 4445, 4458, 4463, 4468, so with two rejections it prints listener a, tick a, listener b, tick b instead of listener a, listener b, tick a, tick b. Fix: make every arm that ran a JS listener return true and drop its local drain so handleRejectedPromises does the one checkpoint, which covers the 6 sites listed, while the unhandled_rejection callers that discard the bool (cron, macros, hot reload) still get a checkpoint from their own turn. [also at: src/jsc/VirtualMachine.rs:4465 - Pre-existing: users running with a non-default --unhandled-rejections mode still get listener a, tick a, listener b ordering, not Node's listener a, listener b, ticks.]
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
A script run with bun --unhandled-rejections=warn (or throw, none, strict) registers an 'unhandledRejection' listener that queues process.nextTick and queueMicrotask, then rejects two promises. handleRejectedPromises at src/jsc/bindings/ZigGlobalObject.cpp:3409 calls Bun__handleRejectedPromise per promise; for these modes unhandled_rejection_owned runs drain(self) right after handle_unhandled() (src/jsc/VirtualMachine.rs:4430, 4436, 4445, 4458, 4463) and returns false, so ticks and microtasks of listener a run before listener b is invoked. Node's processTicksAndRejections runs all pending rejection listeners in processPromiseRejections regardless of mode and only then runMicrotasks(), so the user sees a different order than Node. The base branch behaves the same way in these arms; this PR's batch checkpoint (ZigGlobalObject.cpp:3425) only fires when owesCheckpoint is true, which only Mode::Bun returns at VirtualMachine.rs:4424.
Verification: pre-existing — the base already orders these modes the same way by the same route; this PR touches every arm of the match but only changes Mode::Bun. In src/jsc/VirtualMachine.rs every other arm still calls drain(self) immediately after handle_unhandled() and returns false: Mode::None line 4430, Warn 4436, WarnWithErrorCode 4445, Strict 4458, Throw 4463 and 4468.
| } | ||
| } | ||
| // The ticks and the microtasks of the listeners run now (node: processTicksAndRejections). | ||
| if (owesCheckpoint && drainMicrotasks()) |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: Pre-existing: a database query or redis command that an 'unhandledRejection' listener issues after an await still is not sent until unrelated I/O or a timer wakes the loop. The checkpoint added at ZigGlobalObject.cpp:3425 runs ticks and promise jobs but not deferred_tasks.run() (event_loop.rs:440), where the Postgres, MySQL, Valkey and HTTP auto-flushers write their buffers. When handleRejectedPromises runs at the end of tick_turn (event_loop.rs:835) the next step is auto_tick_active's blocking poll. Fix: make the owed checkpoint EventLoop::drain_microtasks, as drain(self) at VirtualMachine.rs:4402 does for every other mode, e.g. by returning owesCheckpoint to the Rust callers.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
Trigger: the loop is kept alive by something idle, an I/O or timer callback rejects a promise nobody handles, and process.on('unhandledRejection', async e => { await sqlinsert ... }) or await redis.set(...) is registered. In Mode::Bun handleRejectedPromises calls GlobalObject::drainMicrotasks() at src/jsc/bindings/ZigGlobalObject.cpp:3425. The listener's continuation runs there and enqueues its command: PostgresSQLConnection.rs:2514 and valkey.rs:1398 call register_auto_flusher, which posts to event_loop.deferred_tasks; the bytes are written to the socket only in deferred_tasks.run(), which EventLoop::drain_microtasks_with_global runs at src/jsc/event_loop.rs:440 and the C++ drain does not. tick_turn returns; auto_tick_active then blocks in tick_with_timeout at jsc_hooks.rs:1201 with no immediate, timer or sweep due, so the command is not sent and the await never resolves until some other event wakes the loop. On the base branch the Mode::Bun arm returned without any drain, so the whole continuation waited for the same wake-up: the PR runs the JS but leaves its I/O in the same stall.
Verification: GlobalObject::handleRejectedPromises() now calls drainMicrotasks() (src/jsc/bindings/ZigGlobalObject.cpp:3425). That function never touches deferred_tasks. The Rust checkpoint drain_microtasks_with_global runs self.deferred_tasks.run() (src/jsc/event_loop.rs:440). After tick_turn's final handle_rejected_promises() nothing drains before tick_with_timeout blocks (jsc_hooks.rs:1199-1202).
| if (owesCheckpoint && drainMicrotasks()) | ||
| return; |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: pre-existing: an app whose last timer or I/O callback rejects a promise still loses the async work its 'unhandledRejection' listener starts, unlike Node. Such a rejection is first reported by the handle_rejected_promises call at src/runtime/cli/run_command.rs:1629, after 'beforeExit'. The new checkpoint at src/jsc/bindings/ZigGlobalObject.cpp:3425 now runs the listener's ticks and microtasks there, but nothing re-enters the event loop before vm.on_exit(), so a setTimeout or fetch they queue is dropped. Fix: report pending rejections before 'beforeExit' is emitted and re-run the is_event_loop_alive loop when the listeners or their checkpoint made the loop alive, so a listener's queued timer or I/O completes as it does for the top-level rejection case the PR tests.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
A script registers process.on('unhandledRejection', async e => { await null; setTimeout(() => console.log('scheduled')) }) and its last setTimeout callback does Promise.reject(new Error('a')). The timer fires inside auto_tick_active (src/runtime/jsc_hooks.rs:1110-1218), which never calls handle_rejected_promises; the main loop at src/runtime/cli/run_command.rs:1568 then sees is_event_loop_alive() false and stops. on_before_exit (src/jsc/VirtualMachine.rs:2173) emits 'beforeExit' and breaks because the loop is not alive. The rejection is first handled at src/runtime/cli/run_command.rs:1629; the checkpoint at src/jsc/bindings/ZigGlobalObject.cpp:3425 runs the listener's continuation, which queues the timer. The next statement is vm.on_exit() at run_command.rs:1634, so the timer never runs. On the base branch the listener's continuation did not run at all, so this path is pre-existing; in Node the listener runs before 'beforeExit' and the loop stays alive for what it queues. The PR's tests only cover a rejection reported during the entry-point wait, where wait_for_promise re-checks the loop.
Verification: pre-existing — triggers when the rejection comes from the last timer callback and the 'unhandledRejection' listener schedules loop work. The rejection is first reported at run_command.rs:1629; the drain at ZigGlobalObject.cpp:3425 runs the listener's continuation, but the next statement is vm.on_exit(), so a queued setTimeout is abandoned. The PR's test only covers a top-level rejection.
| // The callers are in a turn of the loop, whose checkpoint is still to come. | ||
| let _ = self.unhandled_rejection_owned(global_object, reason, promise, owner); |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: Users running bun --hot whose reloaded entry module rejects still lose or delay the async work their 'unhandledRejection' listener starts, unlike Node and unlike the plain-run path this PR fixes. The comment at src/jsc/VirtualMachine.rs:4365 says every caller is mid-turn, but report_exception_in_hot_reloaded_module_if_needed (VirtualMachine.rs:4525) runs between ticks from run_command.rs:1553/1557/1561, so the discarded true means nothing drains until the loop next wakes. Fix: make unhandled_rejection drain (or hand the bool to) callers that are not inside a turn, so a claiming listener's ticks and microtasks run at every reporting site, not only the tracker path in handleRejectedPromises.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
A user runs bun --hot app.ts; app.ts registers process.on('unhandledRejection', async e => { await null; console.log('after') }) and its top-level await rejects after a reload. The entry promise is a JSInternalPromise reported by report_exception_in_hot_reloaded_module_if_needed at src/jsc/VirtualMachine.rs:4525, which calls unhandled_rejection, not the tracker. In Mode::Bun the listener claims it and unhandled_rejection_owned returns true (VirtualMachine.rs:4424), but unhandled_rejection discards it with let _ = at VirtualMachine.rs:4366 and drains nothing. That caller runs from src/runtime/cli/run_command.rs:1553, 1557 and 1561, outside tick_turn, so the drain at src/jsc/event_loop.rs:808 does not follow; the loop then blocks in tick_possibly_forever until a file change. The continuation runs only on the next wake (--hot) or is lost when the process restarts (--watch). Base behaved the same, but the comment the PR adds asserting all callers are mid-turn is false for this site.
Verification: The base's Mode::Bun arm was if handle_unhandled() { return; } with no drain, so the same route already failed before this PR. src/jsc/VirtualMachine.rs:4422-4425 returns without draining; VirtualMachine.rs:4366 let _ = self.unhandled_rejection_owned(...) discards it. The caller at VirtualMachine.rs:4525 is invoked from src/runtime/cli/run_command.rs:1553, 1557 and 1561, all outside vm.tick().
| // An emitter whose constructor never ran has no own `emit`. A userland `emit` stays. | ||
| if (EventEmitterPrototype.emit === (value ? emitWithoutRejectionCapture : emitWithRejectionCapture)) { |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: pre-existing: after a userland patch of EventEmitter.prototype.emit, turning on EventEmitter.captureRejections still does not capture rejections on emitters whose constructor never ran, unlike Node. The setter at src/js/node/events.ts:1016 only swaps the prototype emit when it is still the built-in variant, so a wrapper installed while capture was off keeps delegating to emitWithoutRejectionCapture, which never calls addCatch for non-'error' events even though this[kCapture] is true. Fix: make capture follow kCapture at call time regardless of which built-in emit is installed, e.g. have emitWithoutRejectionCapture fall through to the capturing loop when this[kCapture] is true.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
An app loads an instrumentation library that wraps EventEmitter.prototype.emit (saving the current built-in, which is emitWithoutRejectionCapture) before it sets EventEmitter.captureRejections = true, then emits on an object created with Object.setPrototypeOf(..., EventEmitter.prototype) or old util.inherits code that never calls EventEmitter.call(this). The setter at src/js/node/events.ts:1014-1018 sets EventEmitterPrototype[kCapture] = true but skips the swap because EventEmitterPrototype.emit is the userland wrapper, not emitWithoutRejectionCapture. The wrapper calls emitWithoutRejectionCapture (src/js/node/events.ts:237-292), whose non-'error' branches discard listener return values and never reach addCatch, so an async listener that throws produces an unhandled rejection instead of an 'error' event. Node's lib/events.js emit always runs addCatch, which checks that[kCapture], so Node captures here. The base branch fails the same way (it never swapped emit at all); this PR's setter fixes only the no-wrapper case and the description notes a userland emit is left in place, but the consequence for wrapped prototypes is not bounded by that note.
Verification: The setter at src/js/node/events.ts:1014-1018 sets EventEmitterPrototype[kCapture] = value and swaps emit only if (EventEmitterPrototype.emit === (value ? emitWithoutRejectionCapture : emitWithRejectionCapture)). With a wrapper installed, the comparison is false, so the wrapper keeps delegating to emitWithoutRejectionCapture (lines 238-292), which discards every listener result and never calls addCatch.
| function addCatch(emitter, promise, type, args) { | ||
| promise.then(undefined, function (err) { | ||
| // The callback is called with nextTick to avoid a follow-up rejection from this promise. | ||
| process.nextTick(emitUnhandledRejectionOrErr, emitter, err, type, args); | ||
| }); | ||
| if (!emitter[kCapture]) return; | ||
|
|
||
| // Handle Promises/A+ spec, then could be a getter that throws on second use. | ||
| try { | ||
| const then = promise.then; | ||
|
|
||
| if (typeof then === "function") { | ||
| then.$call(promise, undefined, function (err) { | ||
| // The callback is called with nextTick to avoid a follow-up rejection from this promise. | ||
| process.nextTick(emitUnhandledRejectionOrErr, emitter, err, type, args); | ||
| }); | ||
| } | ||
| } catch (err) { | ||
| emitter.emit("error", err); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: nit, pre-existing: the browser polyfill copy of this module in src/node-fallbacks/events.js keeps the old addCatch, captureRejections setter and promise-only check, so bun build --target=browser output still diverges from Node on the four captureRejections cases this PR fixes. Fix: port the same kCapture gate, thenable handling, then-getter try/catch and setter swap to the fallback, or state in the PR that the browser copy is intentionally excluded.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
src/node-fallbacks/events.js:68-72 calls promise.then(...) with no emitter[kCapture] check and no try/catch; its emit at line 159 only captures results whose then === Promise.prototype.then; its setter at line 484-491 only writes EventEmitterPrototype[kCapture] and never swaps emit. A bundle built for the browser that sets EventEmitter.captureRejections = true on a no-constructor emitter, or returns a non-promise thenable from a listener, gets no capture, exactly as the base runtime did. This copy is unchanged by the diff, so it is pre-existing and the runtime module is now out of step with it.
Verification: pre-existing (nit): triggered when a bun build --target=browser bundle uses the events polyfill with captureRejections. src/node-fallbacks/events.js still has the old shapes the PR fixes: addCatch at lines 68-72 has no emitter[kCapture] gate; the captureRejections setter at lines 484-492 never swaps EventEmitterPrototype.emit. The base fails the same way; merging makes nothing worse.
What does this PR do?
What an
unhandledRejectionlistener queues withawait,process.nextTickorqueueMicrotasknow runs even when nothing else is pending. Bun ran it at the next turn of the event loop, so with no next turn it was dropped and the process exited.As in Node's
processTicksAndRejections, the listeners of every pending rejection run first, then one microtask checkpoint, then the list is read again.handleRejectedPromisesdoes the checkpoint at the end of a batch, and only if a listener took a rejection:expect(fn).toThrow()reports rejections with test code still on the stack.asynclistenerawaitcaptureRejectionstest-event-capture-rejections.jspassed only because it stopped early: its third stage continues from aprocess.nextTickin anunhandledRejectionlistener, so it exited 0 after 3 of its 13 stages. It now runs all 13, which needed these innode:events:EventEmitter.captureRejections = true, emitter whose constructor never ranthenthrows'error'is emittedasync'error'listener rejectsThe setter swaps
EventEmitter.prototype.emitbetween the two built-in variants, soemitwithout capture still pays nothing. It leaves a userlandprototype.emitin place.One difference from Node is left as on main: when a listener throws, Bun still calls the listeners of the other pending rejections.
How did you verify your code works?
New tests in
process.test.jsandevent-emitter.test.ts, with every expectation checked against Node 26.7. They pass withbun bd test; withUSE_SYSTEM_BUN=1eight fail and the rest are controls that were already right. Undoing the capture check inaddCatchor the userlandemitguard each fails a test. 15 morecaptureRejectionsscenarios print the same as Node, and the 17 vendored Node tests that mention either feature pass.