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:
WalkthroughAdds a domain-aware implementation of Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun.js/bindings/BunProcess.cpp`:
- Around line 3493-3510: The code calls countValue.toNumber(globalObject) and
uses the result without checking for exceptions; change it to call toNumber into
a local numeric result (e.g., double or JSValue numeric form) and immediately
invoke the existing exception check (RETURN_IF_EXCEPTION(scope, {})) before
using the value in the comparison; update the comparison that uses
countValue.toNumber(globalObject) to use this checked local result so any
exception from countValue.toNumber (Symbol/getter throws) is handled before
entering the domain emit logic.
In `@src/js/node/domain.ts`:
- Around line 91-100: The d.run implementation never calls d.exit() when fn()
completes successfully; update d.run (the function named "d.run") so that
d.exit() is invoked after the synchronous call to fn() in all cases (use a
finally-style flow), keeping the existing error handling that calls
emitError(err) on exceptions; ensure the order is: d.enter(), try/catch to call
emitError(err) on thrown errors, then always call d.exit() before returning this
to match Node.js domain behavior.
In `@test/regression/issue/28664.test.ts`:
- Around line 1-3: Add a single-line GitHub issue URL comment at the very top of
the test file (above the existing imports of "test, expect" and "bunEnv,
bunExe") for traceability; the comment should be a single line like //
https://github.com/<org>/<repo>/issues/28664 (replace with the correct org/repo
if needed).
🪄 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: 5a3bfd77-edeb-4f1f-bc20-c88d18b60238
📥 Commits
Reviewing files that changed from the base of the PR and between 17616ae and 8dbe5f3339301535382d5dbde1da74ec5958e8c0.
📒 Files selected for processing (3)
src/bun.js/bindings/BunProcess.cppsrc/js/node/domain.tstest/regression/issue/28664.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/28664.test.ts`:
- Around line 132-157: Update the test in the existing "domain.enter/exit
properly sets process.domain" block to exercise nested domains: create two
domain objects (e.g., d1 and d2) in addition to the existing d, call d1.enter();
d2.enter(); then d2.exit(); and assert that process.domain is restored to d1
(not undefined) before finally exiting d1 and asserting undefined; modify the
inline script passed to Bun.spawn (the string under cmd "-e") to include these
extra enter/exit calls and corresponding console.log checks so the test
validates nested-domain stack restoration by referencing process.domain, d1, and
d2.
- Around line 85-130: Add a new test that spawns Bun and calls
process._fatalException without any domain or uncaughtException handlers to
cover the unhandled path; run the same Bun.spawn pattern as the existing tests
but with no listeners, capture stdout/stderr/exit code, assert stdout contains
"result: false", assert the process reported the unhandled path (e.g., stderr
contains "Bun__reportUnhandledError" or other unhandled-error text), and check
the exit code indicates failure (exitCode !== 0) to satisfy the guideline to
verify error scenarios for process._fatalException and
Bun__reportUnhandledError.
🪄 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: b2b3db8f-5156-41fe-9cd1-161eb1b19f31
📥 Commits
Reviewing files that changed from the base of the PR and between 8dbe5f3339301535382d5dbde1da74ec5958e8c0 and 15a02441cdbca43f57b9551c2128e0f289a46e5b.
📒 Files selected for processing (1)
test/regression/issue/28664.test.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/js/node/domain.ts`:
- Around line 81-95: The add/remove logic can leave duplicate listeners or
detach the wrong domain; fix d.add and d.remove so ownership is checked and old
domains are cleaned up: in d.add(emitter) return immediately if emitter.domain
=== d to avoid duplicate listeners; if emitter.domain exists and !== d, call the
old domain's remove(emitter) (or otherwise remove the emitter from the old
domain's members and listener) before attaching emitError, then set
emitter.domain = d and push to d.members; in d.remove(emitter) only perform
removal when emitter.domain === d (removeListener("error", emitError), set
emitter.domain = null, and splice from d.members) so you never clear another
domain's ownership. Ensure you reference the existing symbols: d.add, d.remove,
emitError, emitter.domain, and d.members.
- Around line 9-52: The current patch only wraps
setImmediate/setTimeout/setInterval so async callbacks scheduled via
process.nextTick, queueMicrotask, and promise continuations still lose
process.domain; update the module to also wrap process.nextTick,
globalThis.queueMicrotask, and monkey-patch Promise.prototype.then/catch/finally
(using saved originals like _origNextTick, _origQueueMicrotask, and
_origPromiseThen) to call _wrapWithDomain for function callbacks when
process.domain is present; while doing this also fix the invocation bugs in
_wrapWithDomain/_patched* by using fn.apply/Function.prototype.call on the
original functions (not .$apply/.$call) and call the saved originals (e.g.,
_origSetImmediate.call(this, callback, ...args)) so domain enter/exit is
reliably propagated for microtasks and promise continuations as well as timers.
- Around line 147-150: The dispose implementation currently clears d.members but
leaves each emitter still wired to the domain; update d.dispose (the function
paired with d.add and the emitError handler) to iterate over d.members and for
each member call member.removeListener('error', emitError') and delete
member.domain (or set to undefined) to unregister the domain from the emitter,
then call this.removeAllListeners(), clear the members array, and finally call
d.exit(); this ensures emitters no longer reference the disposed domain and the
emitError listener is removed.
🪄 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: fd0bcbf7-fd5a-48b9-91e5-07f8e58ebbfa
📥 Commits
Reviewing files that changed from the base of the PR and between 15a02441cdbca43f57b9551c2128e0f289a46e5b and 1d991d5f6320e19ee9f9d35c1764da87e0cb948e.
📒 Files selected for processing (3)
src/bun.js/bindings/BunProcess.cppsrc/js/node/domain.tstest/regression/issue/28664.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/js/node/domain.ts (1)
11-13:⚠️ Potential issue | 🟠 MajorMonkey-patching the globals is still import-order dependent.
src/js/node/timers.promises.ts:7-9snapshotsglobalThis.setImmediate,setTimeout, andsetIntervalat module load. If that module is loaded beforenode:domain, it keeps the unwrapped originals and bypasses the new domain propagation entirely. This needs to live at a shared timer entry point, ortimers.promises.tsneeds the same wrapping.Also applies to: 50-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/js/node/domain.ts` around lines 11 - 13, The global timer snapshots (_origSetImmediate, _origSetTimeout, _origSetInterval) in domain.ts make propagation order-dependent; fix by centralizing the timer wrappers so all modules use the same wrapped originals: either move the snapshot/wrapping logic into a shared timer entry point and import that from src/js/node/timers.promises.ts, or export the wrapped functions (or a setup function) from domain.ts and have timers.promises.ts call/import them so it does not capture unwrapped globals; update references to the original globals in both modules to use the shared wrappers to ensure domain propagation regardless of import order.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/js/node/domain.ts`:
- Around line 65-78: The emitError function must not coerce falsy error payloads
into a synthetic error; remove the e ||= $ERR_UNHANDLED_ERROR() line and stop
replacing 0/false/""/undefined before emitting. Instead only attach metadata
when the payload is an object or function (adjust the typeof check to include
"function" if needed), keep ObjectDefineProperty(e, "domain", ...) and
e.domainThrown = thrown === true, then call d.emit("error", e) unchanged so the
original exception value is forwarded to d.emit.
- Around line 15-23: The wrapper _wrapWithDomain currently exits the domain in a
finally block which clears process.domain before native uncaught-exception
handling sees it; change the implementation so exceptions are routed through the
domain before exit by replacing the plain finally-only approach with
try/catch/finally: inside catch, invoke the domain's error routing (e.g., call
activeDomain.emit('error', err) or the domain's error handler) then rethrow the
error, and only call activeDomain.exit() in the finally block after routing;
reference _wrapWithDomain, activeDomain.enter(), activeDomain.exit(), and the
wrapped function call (fn.$apply) when making the change.
---
Duplicate comments:
In `@src/js/node/domain.ts`:
- Around line 11-13: The global timer snapshots (_origSetImmediate,
_origSetTimeout, _origSetInterval) in domain.ts make propagation
order-dependent; fix by centralizing the timer wrappers so all modules use the
same wrapped originals: either move the snapshot/wrapping logic into a shared
timer entry point and import that from src/js/node/timers.promises.ts, or export
the wrapped functions (or a setup function) from domain.ts and have
timers.promises.ts call/import them so it does not capture unwrapped globals;
update references to the original globals in both modules to use the shared
wrappers to ensure domain propagation regardless of import order.
🪄 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: 57b0e4e8-ddbf-4c48-9210-521b9b3f1962
📥 Commits
Reviewing files that changed from the base of the PR and between 1d991d5f6320e19ee9f9d35c1764da87e0cb948e and f6b96d05770b5e8910e1fdd47baed9b1f7f5692b.
📒 Files selected for processing (1)
src/js/node/domain.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/28664.test.ts`:
- Around line 196-198: The assertion fails because the test checks
process.domain with strict equality to undefined after calling d1.exit(); update
the check to use loose null comparison so it accepts both null and undefined
(change the console.log/expect that uses "=== undefined" to use "== null" or
equivalent). Locate the lines around the call to d1.exit() and the subsequent
console.log/expect that reference process.domain and replace the strict ===
undefined check with == null to match the other tests and the domain
implementation.
🪄 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: 4801a22e-4577-44d0-b006-cb4e9b883e0a
📥 Commits
Reviewing files that changed from the base of the PR and between f6b96d05770b5e8910e1fdd47baed9b1f7f5692b and e0133482215ecfec8c5f0121222b36b039c41ad6.
📒 Files selected for processing (2)
src/js/node/domain.tstest/regression/issue/28664.test.ts
a1a1d25 to
4f25883
Compare
480c95b to
2d31b97
Compare
| // https://github.com/oven-sh/bun/issues/28664 | ||
| import { expect, test } from "bun:test"; | ||
| import { bunEnv, bunExe } from "harness"; | ||
|
|
||
| test("process._fatalException routes errors to active domain", async () => { | ||
| await using proc = Bun.spawn({ | ||
| cmd: [ | ||
| bunExe(), | ||
| "-e", | ||
| ` | ||
| require('domain') | ||
| .create() | ||
| .on('error', (e) => console.log('domain error: ' + e.message)) | ||
| .run(() => { | ||
| setImmediate(() => { | ||
| process._fatalException(new Error('CRASH!!!')); | ||
| }); | ||
| }); | ||
| `, | ||
| ], | ||
| env: bunEnv, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
|
|
||
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
|
|
||
| expect(stdout).toContain("domain error: CRASH!!!"); | ||
| expect(exitCode).toBe(0); | ||
| }); |
There was a problem hiding this comment.
🔴 All 8 tests in test/regression/issue/28664.test.ts call process._fatalException() directly rather than exercising the natural throw-inside-timer path, leaving the _wrapWithDomain catch block in domain.ts with zero test coverage. Add a test that does d.run(() => { setImmediate(() => { throw new Error('real throw'); }); }) to cover the primary integration path that was added during review.
Extended reasoning...
Missing test coverage for _wrapWithDomain catch block
The _wrapWithDomain function in src/js/node/domain.ts contains a catch block that is the primary integration path for domain error routing — it catches errors thrown inside domain-wrapped timer callbacks (setImmediate/setTimeout/setInterval) and routes them through process._fatalException(err) while the domain is still active. This block was added iteratively during PR review (commit ee4adc0) because reviewers noted that without it, process.domain would be cleared by the finally block before the native error handler could inspect it.
All 8 tests in test/regression/issue/28664.test.ts bypass this catch block entirely. Tests 1 and 2 do call setImmediate inside a d.run() context (so _wrapWithDomain IS invoked and wraps the callback), but the callback explicitly calls process._fatalException(new Error(...)) — the try block returns normally without throwing, so the catch block is never entered. Tests 3–6 call process._fatalException() without any timer, and tests 7–8 test enter()/exit() without exceptions.
The untested code path is:
function _wrapWithDomain(fn: any, activeDomain: any) {
return function (this: any) {
activeDomain.enter();
try {
return fn.$apply(this, arguments);
} catch (err) { // ← NEVER REACHED by any test
if (\!process._fatalException(err)) {
throw err;
}
} finally {
activeDomain.exit();
}
};
}Step-by-step proof of the gap:
- Test 1 calls
d.run(() => { setImmediate(() => { process._fatalException(new Error('CRASH\!\!\!')); }); }) d.runwraps the outer callback; on scheduling,setImmediate's callback is also wrapped by_wrapWithDomain- When the setImmediate fires,
_wrapWithDomain's wrapper callsactiveDomain.enter(), then invokes the original callback - Inside the callback,
process._fatalException()is called explicitly — this routes to the domain handler and returnstrue - The try block returns normally (the call to
_fatalExceptionreturns a boolean; no exception is thrown) - The catch block is never evaluated; the finally block runs and calls
activeDomain.exit()
A minimal test covering the untested path would be:
const d = require('domain').create();
d.on('error', (e) => console.log('caught: ' + e.message));
d.run(() => {
setImmediate(() => {
throw new Error('real throw'); // caught by _wrapWithDomain's catch block
});
});Without this test, any regression in the catch block (wrong condition on _fatalException's return value, wrong variable name, missing err argument, accidentally swapped then/else) would pass the entire test suite undetected. The catch block was added specifically to handle the case reviewers flagged — not testing it means the core improvement from the PR review has no verification.
| d.bind = function (fn) { | ||
| return function () { | ||
| var args = Array.prototype.slice.$call(arguments); | ||
| try { | ||
| fn.$apply(null, args); | ||
| return fn.$apply(this, args); | ||
| } catch (err) { | ||
| emitError(err); | ||
| emitError(err, true); | ||
| } | ||
| }; | ||
| }; |
There was a problem hiding this comment.
🔴 d.bind() and d.intercept() do not call d.enter()/d.exit() around the wrapped function's execution, so process.domain is wrong inside bound/intercepted callbacks; additionally, the wrapper returned by d.bind() is missing a .domain property, breaking any code that inspects bound.domain. Both issues cause incorrect domain association for errors thrown inside callbacks created with these methods.
Extended reasoning...
Missing d.enter()/d.exit() in d.bind() and d.intercept()
The Node.js domain module contract is that any callback wrapped by d.bind() or d.intercept() should execute with that domain active (i.e., process.domain === d) for the duration of the call. The current implementation at lines 107–130 of src/js/node/domain.ts calls fn directly without ever invoking d.enter() before execution or d.exit() in a finally block after. This means process.domain retains whatever value it had before the bound callback was called — which could be null, a different domain, or a stale reference.
Comparison to correct patterns in the same file
Both d.run() (lines 147–157) and _wrapWithDomain() (lines 15–29) implement the correct enter/finally-exit pattern. d.run() calls d.enter() before fn(), catches errors with emitError(), then always calls d.exit() in a finally block. _wrapWithDomain() does the same for timer-patched callbacks. d.bind() and d.intercept() conspicuously omit this pattern despite being the primary public API for domain-aware callbacks.
Missing wrapper.domain = d on the d.bind() return value
Node.js's Domain.prototype.bind explicitly stamps the returned wrapper function with bound.domain = this (set via ObjectDefineProperty with enumerable: false). Code that calls d.bind(fn) and then inspects the returned function's .domain property — or that later removes the binding by checking emitter.domain — will get undefined instead of d. This is a compatibility gap versus Node.js's documented behavior. Note: d.intercept() does not set .domain on its wrapper in Node.js either, so this specific sub-issue is scoped to d.bind() only.
Concrete proof of the enter/exit bug
Step 1: create domain d1, enter it (process.domain === d1). Step 2: create domain d2, call var cb = d2.bind(fn). Step 3: exit d1 (process.domain === null). Step 4: call cb(). Inside fn, process.domain is still null — not d2 as expected. Any throw inside fn calls emitError(err, true), which emits on d2 correctly, but process.domain is wrong throughout fn's execution, so any nested async scheduling (setTimeout, etc.) inside fn would capture null as the active domain rather than d2.
How to fix
In d.bind(), wrap the fn. call with d.enter() before and d.exit() in a finally block (matching the d.run() pattern), and add ObjectDefineProperty(bound, 'domain', { configurable: true, enumerable: false, value: d, writable: true }) on the returned wrapper. In d.intercept(), add the same d.enter()/d.exit() wrapping around the fn. call in the non-error branch.
| if (count > 0) { | ||
| // Domain has error listeners - emit the error on the domain | ||
| JSValue emitFn = domainObj->get(globalObject, Identifier::fromString(vm, "emit"_s)); | ||
| RETURN_IF_EXCEPTION(scope, {}); | ||
|
|
||
| if (emitFn && emitFn.isCallable()) { | ||
| auto emitCallData = JSC::getCallData(emitFn); | ||
| MarkedArgumentBuffer emitArgs; | ||
| emitArgs.append(jsString(vm, String("error"_s))); | ||
| emitArgs.append(exception); | ||
| JSC::call(globalObject, emitFn, emitCallData, domainObj, emitArgs); | ||
| RETURN_IF_EXCEPTION(scope, {}); | ||
| return JSValue::encode(jsBoolean(true)); | ||
| } |
There was a problem hiding this comment.
🔴 The async domain error path in Process_fatalException passes null directly to d.emit("error", null) without a null guard, while the synchronous path through JS emitError() has an explicit if (\!e) e = $ERR_UNHANDLED_ERROR() guard. This creates a behavioral split: throw null inside a d.run() callback works correctly in sync code but causes handlers to receive null in async callbacks (e.g., setImmediate), leading to TypeError: Cannot read properties of null when the handler accesses err.message.
Extended reasoning...
What the bug is and how it manifests
PR #28665 adds domain error routing via a new C++ Process_fatalException implementation. However, the async and sync code paths have an asymmetry: the synchronous path (via emitError() in domain.ts) has a null guard that converts falsy errors into proper ERR_UNHANDLED_ERROR objects, but the async path (via C++ Process_fatalException) has no such guard and passes the raw value straight to d.emit('error', exception).
The specific code path that triggers it
When code throws a falsy value (e.g., null) inside an async callback that was scheduled while a domain was active, the path is:
_wrapWithDomainwraps the async callback (e.g., thesetImmediatehandler)- The callback throws
null _wrapWithDomain's catch block callsprocess._fatalException(null)- C++
Process_fatalExceptionreceivesnullasexception - The
if (exception.isObject())guard is skipped (null is not an object in JSC) emitArgs.append(exception)appends the rawnulld.emit("error", null)fires withnullas the error argument- The domain error handler receives
nulland any access likeerr.messagethrows aTypeError
Why existing code doesn't prevent it
The if (exception.isObject()) check in Process_fatalException only guards property-stamping (adding domainThrown and domain properties). It does NOT check whether the exception is falsy before emitting. The null guard that Node.js relies on — if (\!e) e = $ERR_UNHANDLED_ERROR() — exists only in the JS emitError() function, which is only called on the synchronous path.
What the impact would be
Any code that (intentionally or by accident) does throw null, throw undefined, throw 0, or throw "" inside an async callback (timer, I/O, promise, etc.) while a domain is active will cause domain error handlers to receive that falsy value instead of a proper Error. This silently breaks the Node.js contract for domain error handlers and can cause cascading TypeErrors in handlers that expect an Error-like object.
How to fix it
Add a null/falsy guard at the top of Process_fatalException, mirroring the JS guard:
// At the start of Process_fatalException, after getting exception:
if (\!exception || exception.isNull() || exception.isUndefined() || exception.isFalse() ||
(exception.isNumber() && exception.asNumber() == 0) ||
(exception.isString() && exception.getString(lexicalGlobalObject)->length() == 0)) {
// Replace with ERR_UNHANDLED_ERROR equivalent
exception = ...;
}Or more simply, mirror the JS: convert the exception to a proper Error if it is falsy before emitting.
Step-by-step proof
Consider this program:
const domain = require('domain');
const d = domain.create();
d.on('error', (err) => {
console.log('got error:', err); // prints 'null' instead of an Error
console.log(err.message); // throws TypeError: Cannot read properties of null
});
d.run(() => {
setImmediate(() => { throw null; }); // async throw of null
});- In Node.js,
d.on('error')would receive anERR_UNHANDLED_ERRORwrapping the null (sync path also does this viaemitError) - In Bun with this PR:
setImmediateis wrapped by_wrapWithDomain, its catch block callsprocess._fatalException(null), C++ emitsnulldirectly, and the handler crashes onerr.message - The same
throw nullinside a synchronousd.run(() => { throw null; })would correctly produceERR_UNHANDLED_ERRORbecause the sync path goes through JSemitError()which has the guard
process._fatalException was a no-op stub that returned undefined. Now it properly: - Checks process.domain for an active domain with error listeners - Sets domainThrown and domain (non-enumerable) properties on the error - Emits the error on the domain if one is active - Falls back to uncaughtException/captureCallback handling - Returns a boolean indicating whether the error was handled Also fixes the domain module: - enter()/exit() properly set process.domain with stack tracking - run() uses try/catch/finally with proper domain lifecycle - Async timer wrapping propagates domain context through callbacks - add()/remove()/dispose() manage emitter ownership correctly - bind()/intercept() forward this, return values, and thrown status - splice(index, 1) fix for exit() stack management Fixes #28664
2d31b97 to
ad79007
Compare
| if (count > 0) { | ||
| // Domain has error listeners - emit the error on the domain | ||
| JSValue emitFn = domainObj->get(globalObject, Identifier::fromString(vm, "emit"_s)); | ||
| RETURN_IF_EXCEPTION(scope, {}); | ||
|
|
||
| if (emitFn && emitFn.isCallable()) { | ||
| auto emitCallData = JSC::getCallData(emitFn); | ||
| MarkedArgumentBuffer emitArgs; | ||
| emitArgs.append(jsString(vm, String("error"_s))); | ||
| emitArgs.append(exception); | ||
| JSC::call(globalObject, emitFn, emitCallData, domainObj, emitArgs); | ||
| RETURN_IF_EXCEPTION(scope, {}); | ||
| return JSValue::encode(jsBoolean(true)); |
There was a problem hiding this comment.
🔴 Process_fatalException returns early after routing to a domain error listener without ever emitting the 'uncaughtExceptionMonitor' event, violating Node.js compatibility. In Node.js, 'uncaughtExceptionMonitor' is an unconditional passive observer that must fire before any domain or uncaughtException routing — it cannot be suppressed by a domain handler. Any monitoring tool using process.on('uncaughtExceptionMonitor', ...) alongside a domain error handler will silently fail to receive notifications in Bun.
Extended reasoning...
What the bug is and how it manifests
In Process_fatalException (BunProcess.cpp), when a domain has error listeners and handles the exception, the function calls domain.emit('error', exception) and immediately returns jsBoolean(true) at line 3510. The 'uncaughtExceptionMonitor' event is only emitted inside Bun__handleUncaughtException (lines 1184-1187), which is only reached on the fallthrough path (no domain handled the error, line 3517). This means for any exception handled by a domain, uncaughtExceptionMonitor is never fired.
The specific code path that triggers it
- process._fatalException(err) is called while a domain is active
- Process_fatalException finds process.domain set to an object domain
- domain.listenerCount('error') > 0, so the error is emitted on the domain
- The function returns jsBoolean(true) immediately
- Bun__handleUncaughtException is never called — uncaughtExceptionMonitor never fires
Why existing code doesn't prevent it
The uncaughtExceptionMonitor emission is entirely encapsulated inside Bun__handleUncaughtException. There is no unconditional pre-routing emission. The domain-handling path at lines 3498-3510 is a pure early-return that bypasses Bun__handleUncaughtException entirely.
What the impact would be
This is a Node.js compatibility regression. APM tools (Sentry, Datadog, New Relic) and monitoring frameworks rely on uncaughtExceptionMonitor to track ALL uncaught exceptions regardless of how they are ultimately handled. A program that registers both a domain error handler and an uncaughtExceptionMonitor listener will never receive monitor events in Bun when exceptions are caught by domains. The monitor is specifically designed to be unsuppressible — it fires even when uncaughtException or domain handlers are present.
Step-by-step proof
In Node.js, uncaughtExceptionMonitor fires first in the fatalException function before any domain routing takes place. Bun's implementation never reaches the emission for domain-handled errors.
How to fix it
Before the domain check in Process_fatalException, explicitly fire the uncaughtExceptionMonitor event if there are listeners, mirroring how Bun__handleUncaughtException does it but unconditionally — before any domain routing logic.
|
Closing: this PR predates the Rust rewrite and modifies source files that no longer exist on If the underlying issue is still present, it will need a fresh fix against the current tree. |
1 participant
Fixes #28664
Problem
process._fatalExceptionwas mapped to a no-op stub (Process_stubEmptyFunction) that returnedundefined. This meant errors passed to it were silently ignored, and thenode:domainmodule could not intercept them.Reproduction:
Expected:
domain error: Error: CRASH!!!Actual: No output, exit code 0.
Root Cause
process._fatalExceptionwas a stub returningundefineddomain.enter()/exit()were no-ops that never setprocess.domaindomain.run()only caught synchronous errors via try/catchFix
BunProcess.cpp: Replaced the stub withProcess_fatalExceptionthat:process.domainfor an active domain witherrorlistenersdomainThrown: trueanddomainproperties on the error objectuncaughtException/captureCallbackhandlingdomain.ts: Fixed domain tracking:enter()properly setsprocess.domainand pushes to domain stackexit()restores the previous domain from the stackrun()keeps the domain active for async callbacks scheduled during executionVerification