node: implement process.addUncaughtExceptionCaptureCallback - #33154
node: implement process.addUncaughtExceptionCaptureCallback#33154robobun wants to merge 3 commits into
Conversation
Adds the auxiliary uncaught-exception capture callback API introduced in Node.js v25.9.0 (nodejs/node#61227). Unlike setUncaughtExceptionCaptureCallback, multiple callbacks can be registered. They are dispatched most-recent-first when no primary capture callback is set; a callback that returns exactly true marks the exception as handled and skips the remaining callbacks and the 'uncaughtException' event.
|
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 (5)
Comment |
|
Updated 8:28 PM PT - Jun 30th, 2026
❌ @robobun, your commit 1052912 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 33154That installs a local version of the PR into your bun-33154 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
In a Worker, Bun__Process__exit requests termination and returns instead of exiting the process, so the auxiliary-callback loop kept iterating after a callback threw. Each subsequent call raised a TerminationException that was logged again, and the function fell through to return false, which fired a spurious null 'error' event on the parent Worker handle. Bail out of the loop after the first throwing callback, matching the primary-callback path.
There was a problem hiding this comment.
The Worker-thread fix in 1da4876 looks right and the added parity test pins it down nicely. I'm deferring to a maintainer since this adds new Node-compat API surface and rewires the shared Bun__handleUncaughtException dispatch path.
Extended reasoning...
Overview
Implements process.addUncaughtExceptionCaptureCallback (Node v25.9.0). Touches five files: a new host function and dispatch loop in src/jsc/bindings/BunProcess.cpp, a new WriteBarrierList<JSObject> member on Process in BunProcess.h (visited from visitChildrenImpl), a type declaration in overrides.d.ts, a type-fixture line, and ~200 lines of subprocess-isolated tests in process.test.js. The existing primary-callback exception path is refactored into a shared abortOnCaptureCallbackException helper that now returns bool.
Security risks
None identified. No parsing of untrusted input, no auth/crypto/permissions surface. The only user-controlled input is the callback function, validated via V::validateFunction. The dispatch loop snapshots into a MarkedArgumentBuffer before iterating, guarding against re-entrant list mutation, and checks hasOverflowed().
Level of scrutiny
Medium-high. Bun__handleUncaughtException is the single funnel every uncaught exception in the runtime goes through, and the change restructures its control flow (the else if / else chain becomes fall-through with early returns). It also adds GC-traced state and calls user JS in a loop from an error-handling path — the kind of code where subtle mistakes (GC rooting, exception-scope discipline, Worker-vs-main-thread process.exit semantics) have outsized blast radius. The one bug the earlier review found in exactly that area (Worker-thread loop not bailing after a throw) has been fixed and pinned with a test that asserts set and add produce identical parent-observable output.
Other factors
- New user-facing API on
process: even though the semantics are externally specified by Node, additions to the compat surface are a maintainer call. - The refactor of the pre-existing primary-callback path into the shared helper looks behavior-preserving (same log-then-
Bun__Process__exit-then-return truesequence), but it does change tested-in-production code and deserves a second pair of eyes. - Test coverage is thorough: arg validation,
set/hasnon-interaction, full dispatch-order matrix including the strict=== trueshort-circuit, GC survival, unhandled-still-fatal, throwing-callback abort, and the Worker parity case. All run in isolated subprocesses. - No outstanding review threads; my prior inline comment is resolved.
There was a problem hiding this comment.
Thanks for the fix in 1da4876 — the Worker bail-out now matches the primary path and the new test pins it. I didn't find anything else, but this adds a new user-facing API and reworks the Bun__handleUncaughtException control flow with a new GC-visited member, so it's worth a human pass.
Extended reasoning...
Overview
Implements process.addUncaughtExceptionCaptureCallback (Node v25.9.0). Adds a WriteBarrierList<JSObject> member on Process (visited in visitChildrenImpl), a new host function wired into the LUT, and restructures Bun__handleUncaughtException from an else-if chain into a sequential fallthrough (primary callback → auxiliary loop → 'uncaughtException' listener). The auxiliary loop snapshots into a MarkedArgumentBuffer to survive re-entrant registration and bails via the now-bool-returning abortOnCaptureCallbackException helper. Also adds the type declaration and six subprocess tests plus a Worker parity test.
Security risks
None identified. Argument validation goes through V::validateFunction; no untrusted parsing, filesystem, or network surface. The exception-handling path is process-local.
Level of scrutiny
Medium-high. This is production-critical native code: it sits on the uncaught-exception path (affects process/worker exit semantics), adds a GC-traced member, and changes the fallthrough behavior of the existing handler (the old else-if chain became sequential blocks — I verified the observable behavior is preserved for the pre-existing paths, but a maintainer should confirm the API design matches Node's intent, particularly around hasOverflowed() silently skipping all callbacks and the interaction with domain).
Other factors
My earlier inline concern (Worker-thread loop not bailing after a throw) was fixed in 1da4876 with a regression test that asserts add and set variants produce identical parent-side output. Test coverage is thorough (validation, dispatch order, strict-true short-circuit, GC survival, fatal fallthrough, throwing callback, Worker parity). The bug-hunting pass on the current revision found nothing. Still, new public API surface + JSC GC + exception-path control-flow changes in C++ warrant a human sign-off rather than a bot approval.
CI statusThe code is done and the new tests pass on every lane that ran them; the red on both builds is CI flake unrelated to this change.
Neither failing test is related to this PR:
|
1 participant
Implements
process.addUncaughtExceptionCaptureCallback, added in Node.js v25.9.0 by nodejs/node#61227 and documented at https://nodejs.org/api/process.html#processadduncaughtexceptioncapturecallbackfn. Node's v26 REPL depends on it, and #31827 currently has to shim it inside the REPL because the API is missing from Bun; this lets that shim be dropped.Note:
process.removeUncaughtExceptionCaptureCallbackis not a Node API (there is no way to unregister these callbacks), so it is intentionally not added.What it does
Unlike
setUncaughtExceptionCaptureCallback(one primary callback, conflicts withdomain),addUncaughtExceptionCaptureCallbackregisters any number of auxiliary callbacks. On an uncaught exception, afteruncaughtExceptionMonitor:truemarks the exception handled and short-circuits.'uncaughtException'is emitted as before.hasUncaughtExceptionCaptureCallback()still reports only the primary callback, and registering auxiliary callbacks does not make a latersetUncaughtExceptionCaptureCallback(fn)throwERR_UNCAUGHT_EXCEPTION_CAPTURE_ALREADY_SET.Implementation
BunProcess.h: newBun::WriteBarrierList<JSObject>member onProcessholding the auxiliary callbacks, visited fromvisitChildrenImpl.BunProcess.cpp:Process_addUncaughtExceptionCaptureCallbackhost function (validates withV::validateFunction, appends) wired into theprocessObjectTableLUT, and the auxiliary loop inBun__handleUncaughtException. The loop iterates aMarkedArgumentBuffersnapshot because a callback can re-enteraddUncaughtExceptionCaptureCallbackand reallocate the backing vector. When a callback throws, the loop bails out immediately, matching the primary-callback path. That matters in a Worker, whereBun__Process__exitrequests termination and returns instead of exiting the process.packages/bun-types/overrides.d.ts: declaration onNodeJS.Process(not in@types/node@25yet).Verification
The dispatch test scripts produce byte-identical output to Node v26.3.0, including the ordering, the strict
=== trueshort-circuit (a truthy1must not stop the chain), and the primary callback taking precedence:Tests in
test/js/node/process/process.test.js(each in its own subprocess since the callbacks cannot be unregistered): argument validation and theset/hasinteraction, the full dispatch-order matrix, a handled exception keeping the process alive acrossBun.gc(true)with nouncaughtExceptionlistener, an unhandled one still being fatal after the callbacks run, a throwing callback aborting the process, and a throwing callback inside a Worker terminating the worker identically to the primary callback. All six fail on the released Bun and pass with this change; the existingsetUncaughtExceptionCaptureCallbacktests andtest/js/node/test/parallel/test-process-exception-capture*.jsstill pass, as doestest/integration/bun-types/bun-types.test.ts.