Conversation
|
Updated 2:09 AM PT - Aug 25th, 2026
❌ @robobun, your commit 6da9bca has 1 failures in
🧪 To try this PR locally: bunx bun-pr 33356That installs a local version of the PR into your bun-33356 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
WalkthroughThis PR adds a pending-rejected-promises query and event-loop draining helper, wires that draining into shutdown and auto-tick paths, and expands ChangesRejected promise draining
EventEmitter captureRejections thenables
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the problem, cause, implementation, scope, follow-up captureRejections fixes, and verification results. It does not use the exact template headings, but it provides the required information in equivalent sections. Comment |
|
Checked the #22546 repro against this branch: it behaves exactly like import { setTimeout as sleep } from "node:timers/promises";
let x = 1;
async function main() {
global.setTimeout(() => Promise.reject("oops"), 300);
while (true) { x++; await sleep(100); console.log(x); }
}
await main();
The rejection is already reported on The one visible difference this PR makes on a variant of that repro without top-level await is that the process stops one loop iteration sooner, because the rejection is now notified in the timer phase rather than on the following tick. That is a side effect of the ordering fix, not a fix for the issue. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Pushed Doc comment: fixed. The sibling comment on the CI: That test is a chain of twelve stages, each handing off to the next with With the event-loop fix the chain keeps going, the remaining nine stages run for the first time, and they land on three pre-existing
Causes, in
That second fix alone would over-capture: an emitter constructed while the flag was off has its own Four tests added to Sweeps, no new failures: all 39 |
There was a problem hiding this comment.
Thanks for the guard on the prototype .emit swap (and the test) — that closes out my last note. No further findings from me, but the event-loop / on_before_exit control-flow changes are the kind of shutdown-semantics work a human should sign off on.
Extended reasoning...
Overview
This PR does two things: (1) adds EventLoop::drain_rejected_promises() and wires it into tick, auto_tick, auto_tick_active, on_before_exit, and Run::start so microtasks/nextTicks scheduled inside an unhandledRejection handler are drained before the loop re-checks liveness; and (2) fixes three captureRejections gaps in src/js/node/events.ts (thenables, throwing then getter, inherits()-without-super) that the first fix exposed by making test-event-capture-rejections.js run all twelve stages. A new hasPendingRejectedPromises C++/FFI binding backs the fast-path check.
Since my last review, commit 070f31c addressed the .emit-clobbering concern exactly as suggested (guard the prototype swap on the current value being one of Bun's two internal functions) and added a dedicated test. Both earlier findings are now resolved.
Security risks
None identified. No auth/crypto/network-boundary code; the changes are event-loop scheduling and a Node-compat builtin. The drain_rejected_promises loop terminates on TerminationException, and has_pending_rejected_promises is a plain isEmpty() read.
Level of scrutiny
High. The on_before_exit restructuring (moving drain_rejected_promises() + is_event_loop_alive() above the dispatch branch, and unconditionally continue-ing after dispatch_on_before_exit) changes shutdown control flow. auto_tick_active gaining a rejection tail and tick's tail switching from a single handle_rejected_promises to a looping drain both touch the hottest runtime path. These are exactly the areas where subtle ordering has bitten before, and the PR description itself flags an interaction with #33354 that whichever lands second must reconcile.
Other factors
The PR is unusually well-tested (differential vs Node across seven scenarios, three new process-on tests, five new event-emitter tests including the negative guard, and broad suite sweeps), and the author has been responsive to bot feedback. That raises confidence, but the scope — core event-loop tick/shutdown semantics plus Node-compat builtin behavior — still puts it firmly in "human maintainer should review" territory rather than bot-approvable.
StatusRebased onto current main ( Build 105519 on the current head ( The named darwin agents are the only thing that has been red on this PR for three consecutive builds, each time failing to fetch bits over the network before any test runs: a This fourth rebase was trivial: one context-only conflict in The previous head's build, 105308, finished 180 passed, 1 failed with no test-failure annotation: the one failed job never checked out the repository (three This third rebase was trivial: the only conflict was in The previous head's build, 103223, finished 180 passed, 1 failed. The one failed job was a single The previous head's build, 100373, finished 178 passed, 1 failed. The failure was The previous build on the rebased branch, 100358, finished 178 passed, 1 failed. The one failure was the Every lane that was flaky on the old base (the two darwin agents, Review: all threads resolved. The comment-bot pass did turn up one real thing, now in Verified on the current head: Earlier CI history on the pre-rebase base (superseded)Builds 68503 and 68530 on the original base were green on every lane except: two |
![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 `@src/js/node/events.ts`:
- Around line 842-849: The comment block in EventEmitterPrototype.emit handling
is too long and exceeds the 3-line limit; trim the explanatory text while
keeping the same meaning. Update the nearby comment in the emit swap logic so it
stays concise, and preserve the actual assignment logic that toggles between
emitWithRejectionCapture and emitWithoutRejectionCapture.
🪄 Autofix (Beta)
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: d4e61d41-1022-4b9e-a639-4289a5ac5311
📒 Files selected for processing (3)
src/js/node/events.tssrc/jsc/VirtualMachine.rstest/js/node/events/event-emitter.test.ts
|
Heads up: this overlaps with #33354 (opened a few hours earlier). Both are real and neither subsumes the other, but they add the same report-then-drain loop two different ways and touch the same five files, so they will conflict on merge. They fix different bugs
I checked this PR's repro against #33354's build, and it is not fixed there: A top-level rejection is still scanned at Where they collideBoth add
Same five files either way: SuggestionWhichever lands first, the other rebases and reuses the loop that's already there rather than adding a second one. The two mechanisms are interchangeable; the signal just needs to exist once. #33354 is the older and currently the further-along of the two (green except for the darwin artifact-download infra failure that's hitting every build right now), but I have no stake in which way it goes. |
|
Another report of the same bug, in the let res; const p = new Promise(r => res = r);
process.on("unhandledRejection", () => { console.log("handler"); res(); });
(async () => { await p; console.log("continuation"); })();
Promise.reject(new Error("x"));bun 1.4.0 prints Two data points that may help whoever picks this up:
Not opening a separate PR for this since the fix is already here. |
ec1737f to
6a99346
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
6a99346 to
7f1ac49
Compare
7578c54 to
792f4a7
Compare
There was a problem hiding this comment.
I reviewed this PR and the current bug-hunting pass found no new issues; both earlier findings (the stale auto_tick_active doc comment and the captureRejections setter clobbering a userland emit patch) are resolved with tests. Because this restructures the on_before_exit drain loop and adds a rejection drain at the tail of every auto_tick/auto_tick_active exit — core shutdown-ordering paths — a human look is still worthwhile, and there is the noted #33354 overlap to reconcile on merge.
What was reviewed:
drain_rejected_promisesloop and its four call sites;Stoppedpropagation matches the surroundinglet _ =/ early-return pattern on main.on_before_exitreflow: the unconditionalcontinueafterdispatch_on_before_exitre-enters the loop so rejections from abeforeExitlistener drain before the next liveness check;dispatch = falsestill terminates on the following pass.addCatchthenable duck-typing:thenis read once inside a try, primitives are filtered by the!== null/!== undefinedguards, and thekCapturere-check prevents over-capture when the prototype holds the capturing variant.
Extended reasoning...
Overview
Two related fixes: (1) a new EventLoop::drain_rejected_promises() that runs the nextTick + microtask checkpoint after unhandledRejection handlers, wired into tick_turn, both auto_tick exits, both auto_tick_active exits, Run::start's post-loop scan, and the on_before_exit drain loop (which is also restructured so rejections from a beforeExit listener are notified before the next liveness check); and (2) three captureRejections gaps in src/js/node/events.ts — thenable duck-typing, a throwing then getter, and emitters whose constructor never ran — plus a guard so the prototype-emit swap does not clobber a userland monkey-patch. New C++/Rust binding hasPendingRejectedPromises keeps the common path to one isEmpty() read. Eight new tests across two files; all subprocess-based with piped stdout/stderr and combined-object assertions.
Security risks
None identified. No auth/crypto/permissions surface. The addCatch change now reads .then off arbitrary listener return values, but only under captureRejections (opt-in), inside a try/catch, and matches Node's reference implementation.
Level of scrutiny
High. This touches the event-loop tick tail, the beforeExit re-dispatch loop, and EventEmitter.prototype.emit — all load-bearing for shutdown ordering and process-exit semantics. The on_before_exit reflow moves the is_event_loop_alive() check from after dispatch_on_before_exit to before it and makes the post-dispatch continue unconditional; I traced it and believe it terminates correctly (dispatch = false on the second pass), but this is exactly the kind of subtle control-flow change a maintainer should confirm.
Other factors
The PR is thoroughly tested (differential against node v26, fail-before verified, upstream test suites swept) and both prior review findings are addressed with negative tests. The description notes a semantic overlap with #33354 that whoever lands second must reconcile (handle_rejected_promises_after_tick should call drain_rejected_promises). CI on the current head is 180/1 with the one failure triaged as an unrelated darwin agent issue. Given the scope and the merge-order coordination needed, deferring to a human.
An unhandledRejection handler is user JS that can queue microtasks and nextTicks. Node loops back into processTicksAndRejections after processPromiseRejections(), so anything the handler schedules behind an await still runs. Bun returned straight to the liveness check, dropping that work and exiting 0. Add EventLoop::drain_rejected_promises(): notify pending rejections, then re-enter the nextTick + microtask checkpoint they filled, repeating while the drain produces new rejections. Use it at the tail of EventLoop::tick, auto_tick and auto_tick_active, and in the before-exit drain loop. auto_tick_active had no rejection handling at all, so a rejection raised in the timer or poll phase was only notified after beforeExit, with no loop iteration left to run what the handler scheduled.
…nstructor never ran Unmasked by the unhandledRejection fix: test-event-capture-rejections.js stopped after its third stage on main because the nextTick chain inside the rejection handler was dropped, so the later stages never ran. Three gaps in captureRejections, all reproducible on released bun: - addCatch only attached to real promises, so a thenable returned by a handler was never captured. Duck-type `then` like node, read it once, and route a throwing getter to an 'error' event. - emit is swapped per instance in the constructor, so an emitter built by inherits() without a super call kept the non-capturing emit. Swap the prototype's emit from the static captureRejections setter too. - addCatch now re-checks kCapture, so an emitter that opted out does not start capturing when the global flag flips after it was constructed. Also drop handleRejectedPromises from the auto_tick_active hook doc, which no longer describes the body.
The captureRejections setter swapped the prototype's emit unconditionally, so toggling the flag discarded a monkey-patched emit (what APM and tracing libraries install). Node's setter only flips the flag. Swap only when the current value is one of the two internal emit functions.
…eeds Use global_ref() and drain_microtasks() directly instead of hoisting the global and VM into locals, which removes the borrowck note explaining the hoist. Drop two comments that restate what the adjacent code already says and shorten the rest.
792f4a7 to
6da9bca
Compare
There was a problem hiding this comment.
I reviewed this PR and the bug-hunting pass found no issues. Both concerns I raised earlier (the stale auto_tick_active doc comment and the emit prototype-patch clobbering) have been addressed. Given the scope — event-loop tick/shutdown semantics, the on_before_exit control-flow restructuring, and the noted coordination with #33354 — a human look is still warranted.
Checked: the new drain_rejected_promises loop and its four call sites propagate Stopped consistently with neighbouring loop-level code; the on_before_exit restructuring terminates (unconditional continue after re-dispatch falls through to break on the next pass with dispatch = false); both emitWithRejectionCapture dispatch sites got the thenable change; the addCatch kCapture guard prevents over-capture when the prototype holds the capturing variant.
Extended reasoning...
Overview
Two related fixes: (1) drain the nextTick + microtask checkpoint after unhandledRejection handlers run so work they schedule behind an await isn't dropped, wired into tick_turn, auto_tick, auto_tick_active, on_before_exit, and Run::start; (2) three captureRejections Node-compat gaps in events.ts (thenable duck-typing, throwing then getter, inherits() without super()) uncovered when fix (1) let test-event-capture-rejections.js run past its third stage. Adds one C++ accessor + Rust FFI wrapper for hasPendingRejectedPromises. Eight new tests across two files, all differentially verified against Node.
Security risks
None identified. No untrusted-input parsing, no auth/crypto, no path handling. The events.ts change reads user-controlled .then under a try/catch as Node does.
Level of scrutiny
High. This touches the core event-loop tick and shutdown/drain paths (EventLoop::tick_turn, VirtualMachine::on_before_exit, auto_tick/auto_tick_active), where ordering mistakes surface as hangs, dropped work, or wrong beforeExit semantics rather than clean failures. The on_before_exit loop was restructured (the post-dispatch liveness check moved and the continue became unconditional). events.ts is a hot-path built-in, and the setter now writes EventEmitter.prototype.emit (guarded).
Other factors
The PR has been through 8 iterations with extensive verification (CI green on 180 lanes, node-differential repros byte-identical, rust:check-all on all targets, fail-before checks against released Bun). Both issues I raised previously are resolved with tests. There is a stated interaction with #33354 (same five files, overlapping report-then-drain loop) that whoever merges will need to reconcile — the PR description documents exactly what the second-to-land should do. That coordination and the event-loop-semantics scope are why this warrants a human reviewer rather than auto-approval.
… await pending; fold in the cases from #37981, #34193 and #33356 A worker whose 'beforeExit' listener calls process.exit(0) while the entry module's top-level await is still pending exited 13. The exit-13 stamp now also checks that no stop was requested, as the enclosing 'beforeExit' condition does. Node exits 0 here. Tests carried over from the PRs this one supersedes: - #37981: repl -e last-timer rejection, two main-thread 'beforeExit' orderings, the top-level-await worker cases, and the reported-before-queued-task ordering. - #34193: worker-error-exit-code.test.ts (message-listener and timer sites, the worker's own 'exit' argument, listener suppression). - #33356: a main-thread listener that recovers from a 'beforeExit' rejection after a microtask hop.
|
Closing in favor of #42031 and #42032, which are the same fixes on current main.
I ran this PR's 8 new tests on #42032's branch (it includes #42031): the 5 |
… await pending; fold in the cases from #37981, #34193 and #33356 A worker whose 'beforeExit' listener calls process.exit(0) while the entry module's top-level await is still pending exited 13. The exit-13 stamp now also checks that no stop was requested, as the enclosing 'beforeExit' condition does. Node exits 0 here. Tests carried over from the PRs this one supersedes: - #37981: repl -e last-timer rejection, two main-thread 'beforeExit' orderings, the top-level-await worker cases, and the reported-before-queued-task ordering. - #34193: worker-error-exit-code.test.ts (message-listener and timer sites, the worker's own 'exit' argument, listener suppression). - #33356: a main-thread listener that recovers from a 'beforeExit' rejection after a microtask hop.
… await pending; fold in the cases from #37981, #34193 and #33356 A worker whose 'beforeExit' listener calls process.exit(0) while the entry module's top-level await is still pending exited 13. The exit-13 stamp now also checks that no stop was requested, as the enclosing 'beforeExit' condition does. Node exits 0 here. Tests carried over from the PRs this one supersedes: - #37981: repl -e last-timer rejection, two main-thread 'beforeExit' orderings, the top-level-await worker cases, and the reported-before-queued-task ordering. - #34193: worker-error-exit-code.test.ts (message-listener and timer sites, the worker's own 'exit' argument, listener suppression). - #33356: a main-thread listener that recovers from a 'beforeExit' rejection after a microtask hop.
… await pending; fold in the cases from #37981, #34193 and #33356 A worker whose 'beforeExit' listener calls process.exit(0) while the entry module's top-level await is still pending exited 13. The exit-13 stamp now also checks that no stop was requested, as the enclosing 'beforeExit' condition does. Node exits 0 here. Tests carried over from the PRs this one supersedes: - #37981: repl -e last-timer rejection, two main-thread 'beforeExit' orderings, the top-level-await worker cases, and the reported-before-queued-task ordering. - #34193: worker-error-exit-code.test.ts (message-listener and timer sites, the worker's own 'exit' argument, listener suppression). - #33356: a main-thread listener that recovers from a 'beforeExit' rejection after a microtask hop.
… await pending; fold in the cases from #37981, #34193 and #33356 A worker whose 'beforeExit' listener calls process.exit(0) while the entry module's top-level await is still pending exited 13. The exit-13 stamp now also checks that no stop was requested, as the enclosing 'beforeExit' condition does. Node exits 0 here. Tests carried over from the PRs this one supersedes: - #37981: repl -e last-timer rejection, two main-thread 'beforeExit' orderings, the top-level-await worker cases, and the reported-before-queued-task ordering. - #34193: worker-error-exit-code.test.ts (message-listener and timer sites, the worker's own 'exit' argument, listener suppression). - #33356: a main-thread listener that recovers from a 'beforeExit' rejection after a microtask hop.
1 participant
Repro
process.nextTickandqueueMicrotaskare dropped the same way. Scheduling the timer directly in the handler works, so only the hop is affected.It matters because
unhandledRejectionhandlers are where apps flush a logger, write a crash report, or start a graceful shutdown, and almost all of that code awaits something before scheduling a continuation.Cause
An
unhandledRejectionhandler is user JS that can queue microtasks and nextTicks. Node loops back into its checkpoint afterwards:Bun's
handleRejectedPromises()emits the event and returns straight to theis_event_loop_alive()check. The handler's microtask is still sitting in the queue, whichis_event_loop_alive()does not count, so the loop exits and the work never runs.A second gap in the same family:
auto_tick_active(the timer + poll phase thatbun run's main loop andon_before_exitdrive) had no rejection handling at its tail at all. A rejection raised by a timer callback was therefore notified only afterbeforeExit, from the exit path, with no loop iteration left. That dropped even a directly scheduledsetTimeout, and emittedunhandledRejectionafterbeforeExitinstead of before it.Fix
EventLoop::drain_rejected_promises(): notify pending rejections, then drain the nextTick + microtask checkpoint they filled, repeating while the drain produces new rejections. It returnsResult<(), Stopped>like the rest of the loop-level code on main, so a VM termination met inside a handler propagates instead of being swallowed. Used at the tail ofEventLoop::tick_turn,auto_tick,auto_tick_active, and inon_before_exit's drain loop, so whatever a handler schedules is visible before liveness is re-checked.The new
hasPendingRejectedPromises()binding keeps the common path to a singleisEmpty()read: when nothing rejected, no drain runs.Emission order within a batch is unchanged (all handlers, then one checkpoint), matching node.
Verification
Differential against node v26. All of these produce byte-identical output with the fix and were wrong before it:
setTimeoutbehind.then()recoveredsetTimeoutbehindqueueMicrotaskqueueMicrotasksetTimeoutbehindnextTicknextTicksetTimeoutcallbacksetTimeoutbehind.then()handler,recoveredsetTimeoutcallbacksetTimeoutdirectlybeforeExitonlyhandler,timer-from-handler,beforeExitsetTimeoutcallbackbeforeExit,handlerhandler,beforeExitbeforeExitlistenersetTimeoutbehind.then()handler,recovered,beforeExit#2Three of those are the new cases in
test/js/node/process/process-on.test.ts; they fail onmainand pass with the fix.Suites run against the debug build
No new failures in:
test/js/node/process/process-on.test.tstest/js/node/process/process.test.js(includingdelivers many unhandledRejections in order, which pins batch ordering)test/js/node/process/process-nexttick.test.jstest/js/node/test/parallel/test-promises-unhandled-rejections.jsand the seven othertest-promise-unhandled-*filestest/js/web/workers/worker.test.tstest/js/web/timers/{setTimeout,setInterval,setImmediate}.test.jstest/js/node/timers/node-timers.test.tstest/js/bun/cron/in-process-cron.test.tstest/js/bun/http/serve.test.ts,test/js/web/streams/streams.test.js,test/js/bun/spawn/spawn-stdin-readable-stream.test.tsbun run rust:check-allpasses on all 10 targets.Pre-existing failures in this container, unrelated and reproducible without the change: the timer RSS-leak fixtures gate on
process.execPath.includes("bun-asan"), which is false forbun-debug, so the 192 MB ASAN threshold never applies (a release build measures a 2.0 MB delta); a fewserve.test.tscases need IPv6 / a non-root uid.Second fix:
captureRejectionsFixing the event loop made
test/js/node/test/parallel/test-event-capture-rejections.jsfail. That test is a chain of twelve stages, each handing off withprocess.nextTickfrom inside anunhandledRejectionhandler, so onmainit silently stopped after its third stage and still exited 0. With the chain repaired the remaining nine stages run for the first time and hit three pre-existingcaptureRejectionsgaps, all reproducible on released bun:Promise)erroreventthengetter throwserroreventinherits(X, EventEmitter)with nosuper(), globalcaptureRejections = trueunhandledRejectionerroreventIn
src/js/node/events.ts:addCatchonly attached to real promises ($isPromise(result)). Node duck-typesthen. Readingthenis observable (Promises/A+ allows a getter), so it is read once and a throwing getter becomes anerrorevent.emitis swapped per instance in the constructor, so an emitter built byinherits()without asuper()call keeps the non-capturingemitregardless of the global flag. The staticcaptureRejectionssetter now swaps the prototype'semittoo.kCapture = falseand has no ownemit, so flipping the flag later would start capturing it, which node does not do.addCatchnow re-checkskCapture, as node does.That prototype swap only ever replaces one of the two internal emit functions. Node's setter leaves
emitalone, so a userland patch ofEventEmitter.prototype.emit(what APM and tracing libraries install) has to survive a toggle of the flag.Five tests in
test/js/node/events/event-emitter.test.ts; three fail onmain, and two are negative tests for the guards above (each fails if its guard is deleted).test-event-capture-rejections.jsnow runs all twelve stages and exits 0.Rebase notes
Rebased across a large stretch of main. Two resolutions changed code rather than just context:
Stoppedmodel. Main'stick()becametick_turn() -> Result<(), Stopped>, andhandle_rejected_promises()/drain_microtasks()now returnResults.drain_rejected_promises()adopts the same signature and uses?throughout; theauto_tick/auto_tick_active/Run::startcall sites discard it withlet _ =exactly as main does for the call it replaces, andon_before_exitreturns onErr, matching thescript_allowed()returns main added around it (process: skip 'beforeExit' after a fatal uncaught exception #34639).on_before_exitguards. Main now skipsbeforeExitafter a fatal throw and stops the drain on a termination request. The rejection drain is inserted at the point the loop goes idle, ahead of those guards, so they still gate the re-dispatch.events.ts. node:events: single listeners stored bare like node; copy-on-write arrays #34519 splitemitWithRejectionCaptureinto a single-listener fast path and an array path, each with its own$isPromisecheck. Auto-merge only updated one; the other is updated by hand so a thenable is captured regardless of listener count.test-event-capture-rejections.jscovers both shapes.Everything else merged cleanly. Verified on the new base:
process-on.test.ts+event-emitter.test.ts86/86,test-event-capture-rejections.jsall 12 stages, 56 upstreamtest-event*/test-promise*files, the 7 node-differential repros byte-identical,rust:check-all12/12 targets, and the 6 expected fail-before failures against released bun.Note on #33354
#33354 changes when a rejection is emitted (moving the scan into
EventLoop::exit()so it lands before anything the rejecting callback scheduled). This PR changes what happens after the handler runs. They are independent and do not conflict textually, but whichever lands second should havehandle_rejected_promises_after_tick()calldrain_rejected_promises()rather thanglobal_ref().handle_rejected_promises(), otherwise the checkpoint is skipped for rejections reported fromexit().[review] gate passed · iteration 9 · 11 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 0 rejected · iteration 9
evidence per changed file