Emit unhandledRejection at the end of the tick that created it - #33354
Emit unhandledRejection at the end of the tick that created it#33354robobun wants to merge 5 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe PR changes rejected-promise handling to return a boolean across the C++/FFI/Rust boundary, adds post-tick event-loop draining for rejected promises and microtasks, wires that into timer execution paths, and adds process tests covering ordering with timers, immediates, and nextTick. ChangesUnhandled rejection bool return and post-tick draining
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 8:05 AM PT - Jul 5th, 2026
❌ @robobun, your commit 4c2a3b6 has some failures in 🧪 To try this PR locally: bunx bun-pr 33354That installs a local version of the PR into your bun-33354 --bun |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
This doesn't fix #22546. That issue is a different bug, and this PR leaves it unchanged. In that repro the rejection is already reported at the right moment, before the loop's next iteration: What's broken there is that Bun doesn't abort afterwards, and only when the entry point uses top-level await. Dropping the
Same on both sides of this diff, so the ordering change has no bearing on it. Node aborts in both cases because its default is |
|
Both reviews landed on the
then(onfulfilled, onrejected) {
this.#run(); // interp.run() -> finish() -> rej(potentialError)
return super.then(onfulfilled, onrejected); // ...handler attached only here
}
Review on the microtask re-drain (
Review on |
EventLoop::exit() is where a macrotask callback's microtasks drain, but the unhandled-rejection scan only ran at the tail of autoTick, after the poll and timer phases. A rejection created in a setImmediate callback was therefore reported after any macrotask that callback scheduled, after a sibling immediate queued behind it, and after an already-due timer. Run the scan from exit() once the enter/exit counter returns to 0, which is what Node does from processTicksAndRejections.
EventLoop::exit() is not the end-of-tick hook it looks like: native code also uses it to re-enter JS from inside a JS turn. Interpreter::finish() does exactly that, so scanning there reported a ShellPromise rejection that then() was about to handle three statements later, failing every Bun Shell test that asserts a non-zero exit code. Call the scan from the two batch-drain phases instead, where no JS frame sits above us. Report-then-redrain in a loop so a nextTick queued by the unhandledRejection handler also runs before the next macrotask, matching processTicksAndRejections; handleRejectedPromises() now returns whether a handler ran so the loop terminates.
62c8204 to
c938dc0
Compare
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 `@test/js/node/process/process.test.js`:
- Around line 964-1043: The subprocess cases in the current process tests are
independent and should run concurrently instead of sequentially. Update the
`it.each` block and the trailing `it("drains process.nextTick...")` test to use
the concurrent test style recommended by the suite conventions (for example,
`test.concurrent`/`describe.concurrent` or the equivalent in this file), since
each case uses its own `Bun.spawn` and has no shared state.
🪄 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: 88ec2feb-5173-4462-bd37-5ff5e071b937
📒 Files selected for processing (8)
src/jsc/JSGlobalObject.rssrc/jsc/bindings/ZigGlobalObject.cppsrc/jsc/bindings/ZigGlobalObject.hsrc/jsc/bindings/bindings.cppsrc/jsc/bindings/headers.hsrc/jsc/event_loop.rssrc/runtime/timer/timer_object_internals.rstest/js/node/process/process.test.js
|
The 🔴 on the throw path was right, and the bug was real. But the suggested fix (drain before the first scan) turns out to break a different node behaviour, so the resolution went a different way. Pushed as 404b0f4. ConfirmedsetImmediate(() => {
const p = Promise.reject(new Error("x"));
queueMicrotask(() => p.catch(() => {}));
throw new Error("boom");
});Why draining there is wrong
So node isn't draining-then-scanning at the throw site. It marks the failed callback's scope and skips the checkpoint entirely, then runs it once the immediate batch drains. Four probes against node v26 pin the contract:
Deferred to the end of the batch, not to the throw site (rows 1 and 4) and not to the next loop turn (row 2, where it still beats the timer phase). What landedNo scan at the throw site; one scan after On the other two commentsStale PR description (socket row / "five"): already fixed before that review rendered. The description now has six fail-on-main rows plus two regression guards, and calls out the poll-phase gap explicitly.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/jsc/event_loop.rs (1)
860-880: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winVerify batch ordering when a non-last immediate throws.
exception_thrownin theto_run_nowloop (line 865) is overwritten on every iteration, so it reflects only the last task's outcome. This PR relies on that value to decide whetherhandle_rejected_promises_after_tickfires per-task (insiderun_immediate_task, when the task didn't throw) or once at the end of the batch (here, when the last task threw).I traced the "throw-first, succeed-second" case (covered by the new "defers a rejection..." test) and it resolves correctly, because the succeeding second task's own per-task hook ends up scanning the still-global rejection table left by the first (throwing) task. However, the symmetric case — an earlier immediate in the same batch succeeds while a later one throws — isn't exercised by any test here, and depends on this same last-task-wins nuance to route through the post-loop branch correctly.
Since this mechanism is pre-existing (only the new
handle_rejected_promises_after_tickcall was added to theexception_thrownbranch), it's likely fine, but a Node-parity test with the throw at the end of a multi-immediate batch would pin down this coupling for future refactors.🤖 Prompt for 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. In `@src/jsc/event_loop.rs` around lines 860 - 880, Add a regression test around `run_immediate_task`/`handle_rejected_promises_after_tick` for a multi-immediate batch where an earlier immediate succeeds and a later one throws, so `exception_thrown` being overwritten by the last task is covered explicitly. Verify the batch still routes the rejection handling through the post-loop `maybe_drain_microtasks` + `handle_rejected_promises_after_tick` path in `src/jsc/event_loop.rs`, matching Node’s ordering for a throw-at-end case. Keep the new test symmetric with the existing throw-first/succeed-second coverage to pin down the last-task-wins behavior.
🤖 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.
Outside diff comments:
In `@src/jsc/event_loop.rs`:
- Around line 860-880: Add a regression test around
`run_immediate_task`/`handle_rejected_promises_after_tick` for a multi-immediate
batch where an earlier immediate succeeds and a later one throws, so
`exception_thrown` being overwritten by the last task is covered explicitly.
Verify the batch still routes the rejection handling through the post-loop
`maybe_drain_microtasks` + `handle_rejected_promises_after_tick` path in
`src/jsc/event_loop.rs`, matching Node’s ordering for a throw-at-end case. Keep
the new test symmetric with the existing throw-first/succeed-second coverage to
pin down the last-task-wins behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3596de60-9c56-444b-8910-d5d7c3d374fa
📒 Files selected for processing (3)
src/jsc/event_loop.rssrc/runtime/timer/timer_object_internals.rstest/js/node/process/process.test.js
exit_maybe_drain_microtasks(false) skips the drain when a setImmediate callback throws, so scanning at the throw site saw un-drained state: a promise whose .catch() was merely queued looked unhandled, producing a spurious unhandledRejection and rejectionHandled pair. Node marks the failed callback's scope and skips its checkpoint entirely, running it once the immediate batch drains, which is what keeps the timers-immediate-exception fixture's nextTicks deferred past the next immediate. Do the same: no scan at the throw site, one scan after tick_immediate_tasks' existing post-batch drain. That still reports ahead of the timer phase, matching Node.
404b0f4 to
1f6e5e3
Compare
|
Good catch on the You're right that
All three route correctly, for the reason you traced: when the last task didn't throw, its own per-task hook picks up whatever an earlier throwing task left in the rejection table; when it did throw, the post-batch branch does. The first row is the interesting one and it's a fix ( So rather than add a fourth near-identical block, I folded the throwing-immediate cases into one |
… one tick_immediate_tasks assigned exception_thrown rather than OR-ing it, so a task that returned early without running (cleared via clearImmediate, unref'd with nothing else alive, stale test-isolation generation) erased the fact that an earlier task threw. The batch tail then skipped both the microtask drain and the rejection scan, and the rejection landed after the timer phase.
|
The ConfirmedsetImmediate(() => {
Promise.reject(new Error("x"));
setTimeout(() => console.log("later timer"), 0);
clearImmediate(im2);
throw new Error("boom");
});
var im2 = setImmediate(() => console.log("never"));The cleared sibling takes
On the stale-description commentAlready fixed. Known gap has been singular (poll-phase only) since 404b0f4; the reject-and-throw bullet went away in the same commit that closed it. The description now lists eight fail-on-main rows and two regression guards. |
Status: diff is green, one lane is blocked on CI infraThe only red lane on https://buildkite.com/bun/bun/builds/68519 is That is the artifact transfer, not this diff. It is failing identically on every concurrent build in the pipeline, on unrelated branches:
Everything else that has reported is green, including every shard of the lanes that would catch this change:
Those are the same lanes that caught the two real regressions earlier in this PR (the Bun Shell exit-code suite, and the spurious Final: I re-rolled once ( That is the only re-roll I'll spend, so no further retriggers from me. Build 68550 also lost a second darwin shard, to a different network failure, which makes the diagnosis clearer: it isn't one flaky artifact step, the darwin agents can't reach the network.
282 of 284 lanes passed, and there are no error-level annotations on the build, i.e. not a single test failed anywhere. The diff itself is green and that is not an inference: the retrigger commit is empty, so build 68519 above ran the byte-identical tree and finished 283 passed / 1 failed, the one failure being this same artifact download, with no error-level annotations anywhere. Every Ready for a maintainer as-is. A re-run once the darwin artifact fetch is healthy should go green without any change to the code. |
|
Cross-reference: #33356 lands in the same area and will conflict with this on merge. Details and a suggested reconciliation are in #33356 (comment). Short version: that PR fixes a different bug (work a handler defers behind a microtask hop is dropped, rather than emitted in the wrong order), and I verified this branch does not fix its repro, so both are needed. But both add the same report-then-drain loop, plumbed through |
The darwin aarch64 test-bun shard failed on 'buildkite-agent artifact download timed out after 120s for step darwin-aarch64-build-bun' before running a test. Every other lane passed (283/284).
Repro
Without an
unhandledRejectionhandler, node prints the error and exits beforelater timerruns; Bun runs the timer first.Cause
Node reports unhandled rejections from
processTicksAndRejections, which its timer and immediate queues run after every callback. So a rejection is always delivered before anything that callback scheduled, and before anything already queued behind it.Bun only scanned at the tail of
autoTick, after the poll and timer phases, so everything the callback scheduled got to run first. The timer case looked correct only by accident: asetTimeout(…, 0)created inside a timer callback is not due within the same drain, so the tail scan beat it. Once a sibling timer is already due, the same reordering shows up.Fix
Run the scan at the end of each timer and immediate callback, which is where node's
runNextTicks()sits.handleRejectedPromises()now reports whether a handler ran, so the caller can re-drain the microtask queue the handler may have pushed onto, mirroring node'sdo { drain } while (processPromiseRejections()).Two placements that look right but aren't:
EventLoop::exit(), even though that is where the callback's microtasks already drain.exit()is also how native code re-enters JS from inside a JS turn:Interpreter::finish()calls it whileShellPromise.then()is mid-flight, betweenrej(potentialError)and thesuper.then()that handles it. Scanning there reports a rejection the caller is about to handle. The enter/exit counter cannot tell the two apart, so only the two batch-drain phases call the new hook.exit_maybe_drain_microtasks(false)deliberately skips the drain there, so the scan would see a promise whose.catch()is merely queued and report it. Node marks the failed callback's scope and skips its checkpoint entirely, running it once the immediate batch drains, which is what keeps thetimers-immediate-exceptionfixture's nextTicks deferred past the next immediate. This does the same.Verification
Differential against node v26, comparing emission order. All of these were wrong before this change:
setImmediatesetTimeout(…, 0)scheduled by itlater timer,unhandledRejectionunhandledRejection,later timersetImmediatesetImmediatequeued behind itsecond immediate,unhandledRejectionunhandledRejection,second immediatesetImmediatesetTimeout(…, 5)already scheduled5ms timer,unhandledRejectionunhandledRejection,5ms timersetTimeoutsecond timer,unhandledRejectionunhandledRejection,second timersetImmediateprocess.nextTickqueued by the handlerunhandledRejection,nextTick from handler,second immediatesetImmediatesetTimeout(…, 0)scheduled by itlater timer,unhandledRejectionuncaughtException,unhandledRejection,later timersetImmediateending a batch that started cleanfirst immediate,later timer,unhandledRejectionfirst immediate,uncaughtException,unhandledRejection,later timersetImmediatethatclearImmediates its siblinglater timer,unhandledRejectionuncaughtException,unhandledRejection,later timerPlus two cases that already matched node on
mainand are pinned so the checkpoint placement can't drift:| a throwing
setImmediatewhose rejection is.catch()ed by a queued microtask | reports nothing (uncaughtExceptiononly) || a throwing
setImmediatewith a sibling immediate behind it |uncaughtException,second immediate,unhandledRejection|All ten are cases in
test/js/node/process/process.test.js; the eight in the first table fail onmain.Also re-checked against node and unchanged by this PR: rejections from
process.nextTick,queueMicrotask, module top level,fs.readFilecallbacks, asetTimeoutscheduling anothersetTimeout, asetTimeoutscheduling asetImmediate, asetImmediatescheduling anothersetImmediate, andnextTickqueued from the rejecting callback (still delivered before the rejection).Known gap, not addressed here
A rejection created in the poll phase (e.g. a socket
datahandler) is still reported after an already-due timer. Pre-existing; the natural hook isexit(), which is unusable for the reason above.Suites run against the debug build (no new failures vs. main)
test/js/node/process/process.test.jstest/js/node/timers/(including node'stimers-immediate-exceptionfixture, which pins the deferred-checkpoint behaviour)test/js/bun/shell/commands/(the suite an earlier revision of this PR broke)test/js/web/timers/,test/js/node/timers.promises/test/js/web/streams/streams.test.js,test/js/bun/http/serve.test.tstest/js/bun/test/(bun's own test-runner suite)test/parallel/test-promise*-unhandled-*.js,test-promises-unhandled-rejections.js,test-microtask-queue-run-immediate.js,test-timers-immediate.jsThe pre-existing failures in those suites (
bunshell ls > permission denied directory— the container runs as root,server.requestIP > v6,expect.assertions DOES fail the test, …, …) reproduce identically withsrc/reverted tomain.