domain: implement node:domain on AsyncLocalStorage and port the upstream domain suite (+40 upstream tests) - #31828
cirospaciari wants to merge 76 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:
WalkthroughThis pull request adds Node-compatible domains with async context propagation, EventEmitter integration, uncaught-exception routing, and abort-on-uncaught-exception support. It also updates exception-origin plumbing and adds domain and subprocess regression tests. ChangesDomain runtime and EventEmitter integration
Uncaught-exception handling
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 1:05 AM PT - Sep 8th, 2026
❌ @robobun, your commit 32c7a46 has 1 failures in 🧪 To try this PR locally: bunx bun-pr 31828That installs a local version of the PR into your bun-31828 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
d620847 to
e190cb5
Compare
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/js/node/process/process.test.js (1)
817-825: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAssert stderr before exitCode for better test failure diagnostics.
The current assertion order makes test failures less informative. If the exit code is wrong, you won't see what stderr actually contained. As per coding guidelines, subprocess tests should assert output before exit code.
♻️ Recommended fix
it("aborts when the uncaughtExceptionCaptureCallback throws", async () => { - const proc = Bun.spawn([bunExe(), join(import.meta.dir, "process-uncaughtExceptionCaptureCallbackAbort.js")], { + await using proc = Bun.spawn([bunExe(), join(import.meta.dir, "process-uncaughtExceptionCaptureCallbackAbort.js")], { stderr: "pipe", }); + const [stderr, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]); + expect(stderr).toContain("bar"); // An exception thrown from the capture callback exits with code 7 like // node (internal exception handler run-time failure). - expect(await proc.exited).toBe(7); - expect(await proc.stderr.text()).toContain("bar"); + expect(exitCode).toBe(7); });Based on learnings: applies to **/*.test.{ts,tsx}: assert stdout/stderr BEFORE exitCode; subprocess tests must drain pipes concurrently with
Promise.all.🤖 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 `@test/js/node/process/process.test.js` around lines 817 - 825, The test currently checks proc.exited before reading proc.stderr, which hides stderr when the exit code assertion fails; change the assertions to drain pipes concurrently and assert output first by awaiting both with Promise.all (e.g., await Promise.all([proc.stderr.text(), proc.exited])) and then assert that the stderr string contains "bar" before asserting the exit code equals 7; update the test that uses Bun.spawn and variables proc, proc.stderr.text(), and proc.exited to use this pattern so stdout/stderr are read concurrently and checked prior to exit code assertion.
🤖 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/async_hooks.ts`:
- Line 283: Replace the explicit null and undefined check for the variable
`domain` with a combined loose equality check: locate the conditional `if
(domain !== null && domain !== undefined)` in src/js/node/async_hooks.ts and
change it to use `!= null` so it succinctly checks for both null and undefined
(keep the rest of the conditional block unchanged and ensure no other logic is
affected).
In `@src/js/node/domain.ts`:
- Around line 434-436: The code currently treats any falsy second argument as
missing when constructing the error payload (const er = args.length > 1 &&
args[1] ? args[1] : $ERR_UNHANDLED_ERROR()), which drops valid falsy values like
0/false/""/null/undefined; change the condition to only consider the absence of
a second argument (e.g. const er = args.length > 1 ? args[1] :
$ERR_UNHANDLED_ERROR()) so emit("error", <falsy>) is preserved; update the
assignment to variable er in src/js/node/domain.ts (the
EventEmitter.prototype.emit/error handling block) accordingly to preserve
provided values.
In `@src/jsc/bindings/BunProcess.cpp`:
- Around line 1252-1256: In the fatal branches inside
Bun__handleUncaughtException, after calling Bun__Process__exit(...) you must
return immediately so worker threads don't continue execution; add an immediate
return statement right after the Bun__Process__exit(lexicalGlobalObject, 7) call
in the branch following Bun__logUnhandledException and
Bun__Node__AbortOnUncaughtException, and do the same for the other identical
code path later (the branch around the second Bun__Process__exit call), so
neither path can fall through in worker contexts.
---
Outside diff comments:
In `@test/js/node/process/process.test.js`:
- Around line 817-825: The test currently checks proc.exited before reading
proc.stderr, which hides stderr when the exit code assertion fails; change the
assertions to drain pipes concurrently and assert output first by awaiting both
with Promise.all (e.g., await Promise.all([proc.stderr.text(), proc.exited]))
and then assert that the stderr string contains "bar" before asserting the exit
code equals 7; update the test that uses Bun.spawn and variables proc,
proc.stderr.text(), and proc.exited to use this pattern so stdout/stderr are
read concurrently and checked prior to exit code assertion.
🪄 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: c79a864b-3171-4931-903d-6fb8f19d82d8
📒 Files selected for processing (51)
src/js/node/async_hooks.tssrc/js/node/domain.tssrc/js/node/events.tssrc/jsc/VirtualMachine.rssrc/jsc/bindings/BunProcess.cppsrc/jsc/bindings/BunProcess.hsrc/jsc/bindings/ZigGlobalObject.cppsrc/runtime/cli/Arguments.rstest/js/node/process/process.test.jstest/js/node/test/parallel/test-domain-abort-on-uncaught.jstest/js/node/test/parallel/test-domain-add-remove.jstest/js/node/test/parallel/test-domain-async-id-map-leak.jstest/js/node/test/parallel/test-domain-bind-timeout.jstest/js/node/test/parallel/test-domain-ee-implicit.jstest/js/node/test/parallel/test-domain-ee.jstest/js/node/test/parallel/test-domain-emit-error-handler-stack.jstest/js/node/test/parallel/test-domain-enter-exit.jstest/js/node/test/parallel/test-domain-error-types.jstest/js/node/test/parallel/test-domain-from-timer.jstest/js/node/test/parallel/test-domain-fs-enoent-stream.jstest/js/node/test/parallel/test-domain-http-server.jstest/js/node/test/parallel/test-domain-intercept.jstest/js/node/test/parallel/test-domain-load-after-set-uncaught-exception-capture.jstest/js/node/test/parallel/test-domain-multiple-errors.jstest/js/node/test/parallel/test-domain-nested-throw.jstest/js/node/test/parallel/test-domain-nested.jstest/js/node/test/parallel/test-domain-nexttick.jstest/js/node/test/parallel/test-domain-no-error-handler-abort-on-uncaught-0.jstest/js/node/test/parallel/test-domain-no-error-handler-abort-on-uncaught-1.jstest/js/node/test/parallel/test-domain-no-error-handler-abort-on-uncaught-2.jstest/js/node/test/parallel/test-domain-no-error-handler-abort-on-uncaught-3.jstest/js/node/test/parallel/test-domain-no-error-handler-abort-on-uncaught-4.jstest/js/node/test/parallel/test-domain-no-error-handler-abort-on-uncaught-6.jstest/js/node/test/parallel/test-domain-no-error-handler-abort-on-uncaught-7.jstest/js/node/test/parallel/test-domain-no-error-handler-abort-on-uncaught-8.jstest/js/node/test/parallel/test-domain-promise.jstest/js/node/test/parallel/test-domain-run.jstest/js/node/test/parallel/test-domain-safe-exit.jstest/js/node/test/parallel/test-domain-set-uncaught-exception-capture-after-load.jstest/js/node/test/parallel/test-domain-stack-empty-in-process-uncaughtexception.jstest/js/node/test/parallel/test-domain-stack.jstest/js/node/test/parallel/test-domain-throw-error-then-throw-from-uncaught-exception-handler.jstest/js/node/test/parallel/test-domain-thrown-error-handler-stack.jstest/js/node/test/parallel/test-domain-timer.jstest/js/node/test/parallel/test-domain-timers-uncaught-exception.jstest/js/node/test/parallel/test-domain-timers.jstest/js/node/test/parallel/test-domain-top-level-error-handler-clears-stack.jstest/js/node/test/parallel/test-domain-top-level-error-handler-throw.jstest/js/node/test/parallel/test-domain-uncaught-exception.jstest/js/node/test/parallel/test-domain-vm-promise-isolation.jstest/js/node/test/parallel/test-domain-with-abort-on-uncaught-exception.js
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/js/node/test/parallel/test-domain-nested-throw.js (1)
46-48: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winExit code assertion could be stricter for robustness.
The assertion
assert(!c)accepts any falsy exit code, includingnull(signal termination) in addition to0(normal successful exit). For a domain error-handling test, the child should exit normally with code0after handling the errors, not be terminated by a signal.🔒 Stricter exit assertion
- child.on('exit', common.mustCall((c) => { - assert(!c); + child.on('exit', common.mustCall((code, signal) => { + assert.strictEqual(code, 0); + assert.strictEqual(signal, null); }));This ensures the child exited normally with success code
0, not via signal or other abnormal termination.🤖 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 `@test/js/node/test/parallel/test-domain-nested-throw.js` around lines 46 - 48, The exit assertion in the child process handler currently uses assert(!c) which allows any falsy value (including null for signal termination); update the assertion in the child.on('exit', common.mustCall(...)) callback to assert that the exit code is exactly 0 (e.g., use assert.strictEqual(c, 0)) so the test verifies the child exited normally with success rather than being terminated by a signal.
🤖 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/jsc/bindings/BunProcess.cpp`:
- Around line 1220-1223: Bun__Node__AbortOnUncaughtException is an AtomicBool in
Rust but is declared as extern "C" bool in C++, causing ABI/atomic mismatches;
add a C ABI accessor in Rust (e.g., a function named
Bun__Node__AbortOnUncaughtException_load that returns the result of
.load(Ordering::Relaxed)) and change shouldAbortOnUncaughtException() to call
that loader instead of reading Bun__Node__AbortOnUncaughtException directly so
the C++ side performs a proper atomic-safe load.
---
Outside diff comments:
In `@test/js/node/test/parallel/test-domain-nested-throw.js`:
- Around line 46-48: The exit assertion in the child process handler currently
uses assert(!c) which allows any falsy value (including null for signal
termination); update the assertion in the child.on('exit', common.mustCall(...))
callback to assert that the exit code is exactly 0 (e.g., use
assert.strictEqual(c, 0)) so the test verifies the child exited normally with
success rather than being terminated by a signal.
🪄 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: 0644b2fe-bfef-44d9-bcb4-11bf820acdd8
📒 Files selected for processing (6)
src/jsc/bindings/BunProcess.cpptest/js/node/test/parallel/test-domain-abort-on-uncaught.jstest/js/node/test/parallel/test-domain-nested-throw.jstest/js/node/test/parallel/test-domain-nested.jstest/js/node/test/parallel/test-domain-thrown-error-handler-stack.jstest/js/node/test/parallel/test-domain-top-level-error-handler-clears-stack.js
e8d4378 to
2b58181
Compare
297a234 to
0b243a8
Compare
…eam domain test suite
node:domain was a ~70-line stub (sync-only run/bind, no process.domain, no
uncaught-exception routing). This replaces it with a port of Node's
lib/domain.js and vendors the test-domain-* suite from the Node v26.3.0 tag.
- domain.ts: full port. Async pairing rides on AsyncLocalStorage (Bun has no
async_hooks.createHook): the active domain is carried in an ALS box that
AsyncContextFrame snapshots/restores around every callback. The box also
records a token identifying the synchronous execution that wrote it; when
a callback later observes domain state with a stale token, the paired
domain is entered on the module-global stack — the equivalent of Node's
before() hook. Synchronous throws that unwind to the native fatal path
lose the ALS box (the context frame pops with the unwind), so the
dispatcher falls back to the throw-surviving module-global stack/active.
- BunProcess.{h,cpp}: dedicated domain error-handler slot
(jsFunctionSetDomainErrorHandler) consulted by Bun__handleUncaughtException
before the capture callback and 'uncaughtException' listeners. Errors
thrown from the handler/capture callback now exit with code 7 (Node's
internal-exception-handler failure code). --abort-on-uncaught-exception
aborts before 'uncaughtException' listeners are consulted unless a capture
callback (or domain error handler) is installed, matching V8's throw-time
abort semantics.
- Arguments.rs: implement --abort-on-uncaught-exception, accepting both the
dashed and underscored spellings like V8.
- events.ts: extract the constructor body into a Node-compatible
EventEmitter.init static so domain can wrap it; remove the old
`init: EventEmitter` alias from the exports Object.assign — with the
constructor delegating through EventEmitter.init, the alias made the
constructor call itself.
- async_hooks.ts: AsyncResource instances created inside a domain get the
non-enumerable `domain` property like Node's init hook provides.
- ZigGlobalObject.cpp: fix a pre-existing bug where process.nextTick
callbacks queued alongside AsyncLocalStorage.enterWith() were dropped:
cleanupAsyncHooksData unhooked the microtask-tick callback without
draining the pending nextTick queue, so the process exited with ticks
still queued (reproduces on stock Bun with enterWith + nextTick at main
scope and no other event-loop work).
- process.test.js: the capture-callback-throw fixture now exits 7 (was 1).
Most of the suite is vendored verbatim; divergences are commented in-place:
- Tests throwing from fs callbacks (test-domain-implicit-binding/-fs, the
fs cases in the abort tests) are omitted: errors thrown from fs callbacks
surface through the unhandled rejection path in Bun, which does not yet
route rejections through the domain machinery (same reason the unhandled
rejection block of test-domain-promise is omitted).
- test-domain-dep0097 needs node:inspector; test-domain-multi needs raw
res.socket writes to corrupt the wire protocol mid-response.
- test-domain-with-abort-on-uncaught-exception's synchronous throw case is
omitted: Bun reports a main-module synchronous throw after the nextTick
queue has drained, so the nextTick error's domain cleanup runs first.
Follow-ups from CI on the node:domain port: - Workers: node only honors --abort-on-uncaught-exception on the main thread; an uncaught exception inside a Worker is forwarded to the parent's 'error' handler instead of aborting the process. Gate the abort paths in Bun__handleUncaughtException on Bun__isMainThreadVM(). Fixes test-worker-abort-on-uncaught-exception aborting the whole test process. - Windows: raising SIGABRT there terminates with an ambiguous exit code (observed as 9), which the node test harness does not recognize as an abort. Call _exit(134) in place of abort() like node does; common.nodeProcessAborted expects exactly that value. - Skip the four domain tests (and one case of test-domain-abort-on-uncaught) whose main module throws an uncaught exception that a domain then handles on Windows: that path leaves the process hanging there due to a pre-existing event loop bug, the same one tracked by the zeroExitWithUncaughtHandler windows-todo in test/js/node/process/process.test.js.
Three more entry points into the same pre-existing Windows hang: a handled uncaught exception thrown synchronously from the main module leaves the process hanging there (the bug tracked by the zeroExitWithUncaughtHandler windows-todo in test/js/node/process/process.test.js). - test-domain-abort-on-uncaught: the firstRunOnlyTopLevelErrorHandler and firstRunNestedWithErrorHandler cases throw synchronously from the main module like the already-skipped firstRun case; early-return them on Windows too. The async cases (nextTick/timer/immediate/netServer and the nested variants) are unaffected and keep running. - test-domain-stack-empty-in-process-uncaughtexception: the throw from d.run() is swallowed by the process 'uncaughtException' listener — the exact zeroExitWithUncaughtHandler scenario. - test-crypto-domain: d.run(cb) throws synchronously at module top level and the domain handles it. This passed on Windows before only because the old domain stub caught the error inside run() in JS instead of routing it through the native uncaught-exception path.
Review follow-ups on the node:domain port: - Bun's EventEmitter.init installs the capture-rejections emit variant as an own instance property, which shadows the domain-aware prototype emit, so emitters constructed with captureRejections (or while EventEmitter.captureRejections is enabled globally) bypassed domain error routing entirely. The domain emit override is now built by a factory and EventEmitter.init wraps an own emit with it too. - adopt() enters an async callback's paired domain on the module-global stack (node's before() hook equivalent), but nothing exited it when the callback returned, so the pairing leaked into unrelated callbacks (process.domain reported a stale domain) and repeated adoption grew the stack without bound. The next domain-state access from a different execution context now lazily undoes the previous adoption — the deferred equivalent of node's after() hook, which exits the paired domain along with anything entered above it that was never exited. - Bun__handleUncaughtException: return immediately after the two Bun__Process__exit(7) calls. Bun__Process__exit is only noreturn on the main thread; in a Worker it requests termination and returns, so these fatal branches could fall through into the capture-callback / 'uncaughtException' routing (and into toBoolean() on the call result, which is not meaningful when the call threw). - test-crypto-domain.js: document why this copy diverges from upstream's d.run(fn, cb) (errors thrown from crypto callbacks surface through the unhandled rejection path, which does not yet consult domains) instead of presenting the synchronous throw as upstream behavior. - Style: use `!= null` for the combined null/undefined checks.
When the main module's synchronous evaluation throws and a user 'uncaughtException' listener (or domain error handler) claims the error, the run command grouped that case with --hot/--watch and called tickPossiblyForever(). That arms a ref'd four-minute repeating "forever timer" whenever the loop looks inactive — which is exactly the watcher keep-alive semantics, not what a handled error needs. On Windows this hung the process: the libuv-backed tick is uv_run(UV_RUN_ONCE), which blocks until the next event — the four-minute timer — and the ref'd timer keeps uv_loop_alive() true, so the regular run-loop afterwards never sees the loop go idle and the process never exits. POSIX only escaped by accident: its tick (us_loop_run_bun_tick) happens to find the loop's wakeup eventfd already signaled and returns immediately, and the POSIX is_active() counter is Bun-managed and never incremented by the forever timer. Handled entry errors now just drain the event loop once and fall through to the regular run-loop, which finishes any work the handler scheduled and exits when the loop is empty — same observable behavior on POSIX, no hang on Windows. The --hot/--watch arm is unchanged. This removes the underlying reason for the Windows skips added earlier: - test-domain-nested, test-domain-nested-throw, test-domain-thrown-error-handler-stack, test-domain-top-level-error-handler-clears-stack, test-domain-stack-empty-in-process-uncaughtexception, test-crypto-domain: common.skip(isWindows) removed. - test-domain-abort-on-uncaught: the three firstRun* early-returns removed. - process.test.js: zeroExitWithUncaughtHandler and changeCodeInUncaughtHandler windows-todos flipped to regular tests; both exercise this exact path.
- event-emitter.test.ts: import the harness at module scope instead of
require() inside the describe block, and drain the subprocess stderr
pipes, asserting a combined { stdout, stderr, exitCode } object so
failure diffs show the child's diagnostics.
- test-domain-with-abort-on-uncaught-exception.js: drop the fs require
left behind when the fs.exists case was omitted.
--abort-on-uncaught-exception aborted before 'uncaughtException' listeners for every origin, but node only does that for synchronous throws (V8 aborts at throw time). Promise rejections that reach the uncaught-exception path (--unhandled-rejections=strict) go through process._fatalException first; node aborts only if it returns unhandled (TriggerUncaughtException in node_errors.cc). So a listener that swallowed a strict-mode rejection still SIGABRTed under the flag. Replace the boolean is_rejection with a three-valued origin (exception / rejection / entry-point rejection). True rejections now skip the pre-listener abort and instead abort in the nothing-handled branch at the bottom. The entry-point kind keeps abort-before-listeners semantics: a synchronous throw from the main module surfaces as the rejected entry promise and must abort like a throw (a rejected top-level await is indistinguishable at this layer and shares the behavior), while listeners still observe the 'unhandledRejection' origin string. Adds two regression tests: a strict-mode rejection swallowed by an 'uncaughtException' listener exits 0 under the flag, and one with no listeners still aborts.
The no-listener --abort-on-uncaught-exception regression test spawns a child that SIGABRTs by design. CI lanes that collect core files at teardown (alpine aarch64) found that child's core and flagged the test file as crashed even though every test passed. Wrap the child in `ulimit -c 0`, exactly like the upstream node abort tests (test-domain-abort-on-uncaught and friends) already do for their intentionally-aborting children.
…ters Two residual gaps in the domain-aware EventEmitter integration: - An emitter constructed with captureRejections before node:domain loads carries the un-wrapped capture emit as an own property; the wrapped EventEmitter.init only covers construction after load, so its 'error' events bypassed domain routing even after d.add(ee). add() — the only way such an emitter acquires a domain — now wraps an own emit too. - The process.domain / domain.active setters wrote the context box without entering the callback's scheduling-time pairing first, so a callback whose first domain operation is a write could observe a previous tick's adopted entry on domain._stack (the freshened token made the stale globals look current). Both setters now adopt() first. Adds a subprocess regression test for each.
9a76a62 to
2e035df
Compare
There was a problem hiding this comment.
All prior feedback has been addressed and this run found no new issues, but given the scope — a novel ALS-based domain implementation with the adopt/unadopt reconciliation mechanism, changes to the native uncaught-exception/abort path and exit-code semantics, and the EventEmitter.init/emit restructuring — this warrants a human pass before merging.
Extended reasoning...
Overview
This PR replaces the ~70-line node:domain stub with a full implementation (585 lines) built on AsyncLocalStorage rather than async_hooks.createHook, and wires it into the native uncaught-exception path. It touches 67 files: the core implementation in src/js/node/domain.ts, EventEmitter restructuring (events.ts now exposes a wrappable EventEmitter.init static and drops the init: EventEmitter alias), a new domain-error-handler slot on Process (BunProcess.{h,cpp}), --abort-on-uncaught-exception support, the bool isRejection → three-valued UncaughtExceptionOrigin enum threaded through ~15 Rust call sites, exit-code 1→7 for capture-callback failures, a nextTick-drain fix in ZigGlobalObject.cpp, and a run_command.rs change splitting the handled-entry-error path from the hot-reload path. Plus 48 vendored upstream tests and several Bun-specific regression tests.
Security risks
None identified. The new jsFunctionSetDomainErrorHandler is reachable only via $newCppFunction from the builtin module (not exposed on process), the abort path is gated on an init-time CLI flag, and the EventEmitter prototype mutation is the standard Node behavior being ported. No auth/crypto/permission surfaces touched.
Level of scrutiny
High. This is production-critical exception-handling code: Bun__handleUncaughtException ordering, abort semantics, process exit codes, and the entry-point error path in run_command.rs all affect how every Bun process behaves on error. The adopt()/unadopt() deferred-after-hook mechanism in domain.ts is a novel design (not a port — Node uses createHook) that went through two rounds of correctness fixes during review (the stack-leak and write-first-setter gaps), and the PR description itself notes 18 upstream tests still failing across four buckets. The EventEmitter.init restructuring and removal of the init export alias is a user-visible API surface change.
Other factors
The author has been highly responsive — every inline comment I and CodeRabbit left across five review rounds was addressed with targeted commits and regression tests, and the reasoning in each reply was sound (e.g., correctly pushing back on the AtomicBool ABI and ERR_UNHANDLED_ERROR truthiness suggestions as matching established patterns / upstream). Test coverage is substantial. But the combination of architectural novelty, critical-path changes, known-incomplete coverage, and behavior changes (exit code 7, the events.ts init export removal) puts this well outside auto-approval territory.
2e035df to
9658a11
Compare
…e stack After a setter-only process.domain = d, exit() is a no-op, so the loop spun forever; node returns because its setter does not touch exports.active.
There was a problem hiding this comment.
I reviewed this PR and the bug-hunting pass found no issues on the current revision. Given the scope — a full node:domain implementation that rewires Bun__handleUncaughtException, unifies EventEmitter.prototype.emit, adds --abort-on-uncaught-exception with real abort(), and changes exit codes (1→7) on capture-callback/domain-handler throws — a maintainer sign-off is still worthwhile.
Checked: the UncaughtExceptionOrigin bool→enum migration hits every uncaught_exception call site with the correct variant; the new m_domainErrorHandler/m_domainWouldClaim WriteBarriers are visited in visitChildrenImpl; addCatch now guards on kCapture so the collapsed single-emit preserves the no-capture fast path; the cleanupAsyncHooksData drain is gated on !queue->isEmpty(). All prior inline findings on this PR are resolved.
Extended reasoning...
Overview
This PR replaces the ~70-line node:domain stub with a full port of Node v26's lib/domain.js built on AsyncLocalStorage, and vendors 40+ upstream test-domain-* tests. It touches: src/js/node/domain.ts (rewrite), src/js/node/events.ts (constructor delegates through EventEmitter.init; two emit variants collapsed to one prototype method with addCatch gated on kCapture), src/js/node/async_hooks.ts (domain-active getter hook + .domain tag on AsyncResource), BunProcess.cpp/.h (domain error-handler dispatch slot, --abort-on-uncaught-exception with real abort(), exit-code-7 on handler throw, Worker substituteError out-param), VirtualMachine.rs (UncaughtExceptionOrigin enum replacing the is_rejection bool across ~15 call sites), ZigGlobalObject.cpp (drain pending nextTicks in cleanupAsyncHooksData), and Arguments.rs (new CLI flag + underscore alias).
Security risks
None identified. The new abort path is opt-in via CLI flag and gated to the main-thread VM. Domain error routing runs user JS but only on the pre-existing uncaught-exception path where user callbacks already run. The process.domain accessor and ALS-carried box are per-VM state (not process-global), and non-Domain values written to process.domain are filtered at every dispatch site by the _errorHandler-typeof guard.
Level of scrutiny
High. This is not a mechanical change: it rewires the process-fatal exception path (ordering of domain handler vs. capture callback vs. uncaughtException listeners vs. abort), changes the shape of EventEmitter (removes the own-property emit stamping and the init: EventEmitter self-alias), changes an observable exit code (1→7), and adds a real abort() on Windows/POSIX. The EventEmitter.init change in particular is load-bearing for every emitter subclass in the runtime. These are exactly the kinds of cross-cutting semantic changes REVIEW.md flags for maintainer review.
Other factors
Test coverage is extensive — 44 upstream domain tests plus targeted Bun-specific tests for every edge case raised in the ~30 prior review rounds (non-Domain process.domain, Worker throw-from-handler, unbalanced enter(), monitor-removes-listener, etc.). Every prior claude[bot] inline finding on this PR is marked resolved with a fix commit and a test. The maintainer (cirospaciari) has been actively shepherding the PR and just requested a final conflict-resolution merge, which robobun confirmed passing locally. The remaining known gap (unhandled-rejection → domain routing) is documented in the PR body and pinned by .todo tests. Given the breadth of runtime surface touched, deferring to a human for the final sign-off.
…in-tests # Conflicts: # src/jsc/JSGlobalObject.rs # src/runtime/api/cron.rs # src/runtime/napi/napi_body.rs # src/runtime/server/WebSocketServerContext.rs # src/runtime/socket/Handlers.rs
…failure site The Worker teardown merge added a call still using the old bool signature, which failed to compile on every lane.
robobun
changed the title
…in-tests # Conflicts: # src/jsc/VirtualMachine.rs
There was a problem hiding this comment.
I reviewed the latest revision (post-b5a1024bdc merge) and the bug-hunting pass found nothing new. Given the scope — a full node:domain rewrite, the unified EventEmitter.prototype.emit, and the reworked Bun__handleUncaughtException dispatch (domain slot, --abort-on-uncaught-exception, exit-7, worker substitute-error) — a maintainer sign-off is still warranted.
What was reviewed:
Bun__handleUncaughtExceptionordering vs Node (domainWouldClaimsnapshot, monitor→capture re-read, rejection-vs-exception abort timing) — matches the differential tests in process.test.js.- The
emitWithoutRejectionCapture/emitWithRejectionCapturemerge —addCatchnow guards onkCapture, so non-capture emitters are unchanged; own-emitshadowing onEventEmitterAsyncResource/Http2Servercorrectly removed. cleanupAsyncHooksDatanextTick drain — guarded on!queue->isEmpty()so it doesn't re-drain microtasks.- All prior inline findings (non-Domain
process.domainguards,__proto__: nullon the AsyncResource descriptor,configurable: trueon_stack/active) confirmed applied.
Extended reasoning...
Overview
This PR replaces the ~70-line node:domain stub with a full port of Node v26's lib/domain.js, backed by Bun's AsyncLocalStorage instead of Node's createHook init/before/after hooks. It touches 71 files: the new src/js/node/domain.ts (~440 lines), a refactor of src/js/node/events.ts (unifying two emit implementations into one prototype method and introducing EventEmitter.init), a new native domain-error dispatch slot on Process (BunProcess.{cpp,h}), a new UncaughtExceptionOrigin enum threaded through ~12 Rust call sites, the --abort-on-uncaught-exception CLI flag with Node-matching abort ordering, exit-code-7 semantics for capture-callback / domain-handler throws, a nextTick-drain fix in cleanupAsyncHooksData, and 40+ vendored upstream domain tests plus Bun-specific coverage in domain.test.ts, event-emitter.test.ts, and process.test.js.
Security risks
None identified. The domain module is opt-in (nothing changes until require('node:domain') runs), the new WriteBarrier slots are visited in visitChildrenImpl, and the abort path is main-thread-gated via Bun__isMainThreadVM(). The Bun__Node__AbortOnUncaughtException global is a process-wide atomic flag consistent with the sibling --throw-deprecation flags.
Level of scrutiny
High. This is not a mechanical change: it rewrites process-global uncaught-exception dispatch ordering, changes the shape of every EventEmitter instance (no more own-emit under captureRejections), changes an exit code (1→7), and introduces an ALS-based design that intentionally diverges from Node's implementation strategy. The EventEmitter.init static replacing the init: EventEmitter self-alias is a subtle Node-compat surface change. The abort-ordering logic in Bun__handleUncaughtException (throw-time snapshot of captureAtThrow/domainClaimsAtThrow, then re-read after monitor) went through several correctness iterations in earlier review rounds. All of this warrants a maintainer's eyes even though the automated pass is now clean.
Other factors
The PR has been through ~40 iterations with extensive prior review (both mine and the comment-cop bot's), and every prior inline finding is marked resolved with a fix commit and a Node-differential verification. Test coverage is strong: 44 upstream tests pass, plus targeted Bun tests for each edge case raised in review (non-Domain process.domain, unbalanced enter(), worker throwing-handler, monitor-removes-listener, etc.). cirospaciari has been actively shepherding merges; the last activity was a conflict-fix on 2026-08-10. This clean pass is the signal that the automated side is done; final approval should come from the maintainer already engaged.
|
Checked this branch (df87d41, local debug build) against the other open Passes on this branch: the 13 tests in Gap: unhandled rejections do not reach the domain. Node delivers a rejection to the domain that was active when the promise rejected ( const d = require("domain").create();
d.on("error", e => console.log("domain:" + e.message));
process.on("unhandledRejection", e => console.log("unhandledRejection:" + e.message));
d.run(() => { Promise.reject(new Error("boom")); });
// node v26.3.0: domain:boom
// this branch: unhandledRejection:boomThe same holds for
#40666 implements this in two parts:
Also: #40666 stays open as the source for the gap above until this PR absorbs it. |
|
Heads-up for the rebase: #40665 fixes the dropped-tick case this PR patches in |
|
Dedupe result: #40666 is closed in favor of this PR. Both implement What #40666 has that this PR lacks, with the files to take:
Do not take #40666's edits to If you want, I can open a stacked PR on this branch with item 1. |
…in-tests # Conflicts: # src/js/node/async_hooks.ts # src/jsc/bindings/ZigGlobalObject.cpp
… throws After #41190 the async-context box set during the entry module's own execution legitimately survives to the uncaught-exception dispatch, while Bun drains nextTicks before that dispatch. The token retired by that tick made the still-running entry execution look like a restored pairing, so process.domain = d followed by a synchronous throw was routed to d (node: abort / exit 1, because the setter never puts d on the stack). - New UncaughtExceptionOrigin::EntryPointException for a CJS entry throw (listeners still see 'uncaughtException'); together with EntryPointRejection it tells node:domain that no callback boundary preceded the dispatch, so the pairing view is skipped. - fatalErrorDispatch now follows node's rule exactly: route to process.domain._errorHandler only while a domain on the stack has an 'error' listener, and never push the active domain at dispatch time. This also fixes the silent no-flag variant (bun printed "handled", node exits 1). - async_hooks: the debug assertions in enterWith()/run() read the store through a private #peek() instead of the patchable getStore(). - web_worker: the node bootstrap failure site passes the origin enum.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…cess.abort() The raw abort() went through Bun's crash handler in release builds, printing the crash banner and offering a bun.report upload for a user-requested abort. Resetting the disposition first makes the four test-side BUN_CRASH_REPORT_URL / BUN_ENABLE_CRASH_REPORTING workarounds unnecessary; test-domain-throw-error-then-throw-from-uncaught-exception-handler.js and test/common/index.js are byte-identical to upstream / main again.
|
Two more
Verified on a local debug build of 32c7a46 merged with No test in the PR asserts either case. The vendored test.concurrent("bind() and intercept() return the callback's value, add() accepts a timer", async () => {
const r = await run(`
const domain = require("domain");
const d = domain.create();
const twice = d.bind(x => x * 2);
const sum = d.intercept((a, b) => a + b);
const t = setTimeout(() => {}, 1);
d.add(t);
const added = [t.domain === d, d.members.length];
d.remove(t);
clearTimeout(t);
console.log(JSON.stringify([twice(21), twice.name, sum(null, 1, 2), sum.name, added, t.domain, d.members.length]));
`);
expect(r.stdout).toBe('[42,"runBound",3,"runIntercepted",[true,1],null,0]\n');
expect(r.exitCode).toBe(0);
});One note for the next merge of |
This branch has not been deployed
4 participants
Implements
node:domain(previously a ~70-line stub) as a port of Node'slib/domain.js, and vendors the upstream domain test suite verbatim from Node v26.44 of the 50 upstream
test-domain-*tests now pass (40 newly vendored, plus 3 existing files re-synced to upstream). The 6 that do not pass are not in the tree — they are listed as gaps below rather than marked failing.test/expectations.txtis untouched by this PR. On linux-x64-musl,test-gc-http-client-connaborted.jsnow joinstest-net-connect-memleak.jsas a manifestation of open issue #33044 (one object not finalized in a gc+setImmediate loop; no JS-level retention found, 10/10 on glibc); it is left un-quarantined alongside its sibling pending that issue's fix.What this does
node:domainwas a stub, so any package depending on it silently did nothing. This implements it and proves it against Node's own test suite.node:domainimplemented:create()/Domain,run/bind/intercept/add/remove/enter/exit,domain.active/domain._stack, and Node's error decoration (err.domain,domainThrown,domainEmitter,domainBound).createHook: Node pairs each async resource with the active domain via the async-hooksinithook, then enters it inbefore(). Bun has nocreateHook, so the active domain rides Bun's AsyncLocalStorage/AsyncContextFrame machinery instead — snapshotted at schedule time, restored around every callback, which is the same pairing. The synchronous domain stack stays a module-global like Node's; the uncaught-exception dispatcher reconciles the two at async boundaries.Bun__handleUncaughtExceptionbefore the capture callback and'uncaughtException'listeners — where Node's domain hooks intoprocess._fatalException.process.domainbecomes an accessor reading the async-local active domain.EventEmitterintegration: the constructor now delegates through a Node-compatibleEventEmitter.initstatic (matching Node, and fixing userland that callsEventEmitter.init.call(this)). Emitters created inside a domain get.domainand route'error'into it with Node's stack pruning/restore semantics. The oldinit: EventEmitterself-alias is removed — with the constructor delegating throughinit, that alias would recurse infinitely.emiton the prototype: Bun had two emit implementations and assignedemitWithRejectionCaptureas an own instance property whencaptureRejectionswas set. An own property shadows any prototype override, so those emitters bypassed domains entirely. Both are now the single prototypeemitNode has, with the rejection-capture check guarded bykCapture(addCatchearly-returns when it is off). This also removes the own-emitshadowing thatHttp2Serverhad a comment working around.AsyncResourcecreated inside a domain gets the non-enumerable.domainproperty Node's init hook provides.async_hooksstays domain-agnostic: the getter is null untilnode:domainloads, so nothing touchesprocess.domainotherwise.--abort-on-uncaught-exception: implemented, including the--abort_on_uncaught_exceptionspelling V8 also accepts. Aborts after printing instead ofexit(1), and a throwing top-level domain'error'handler aborts rather than being swallowed. Ordering matches Node: exceptions abort before'uncaughtException'listeners are consulted (V8 aborts at throw time), while true promise rejections abort only after listeners decline.(An earlier revision also carried a drive-by fix for
enterWith()+process.nextTickdropping the tick at main-module scope, patched incleanupAsyncHooksData. #41190 removed that function and fixes the same case at the event-loop boundary, so the hunk is gone from this diff; the repro still passes on the merged branch.)How we know it works
Every vendored test is byte-identical to upstream v26 except three, each carrying an inline
Note for Buncomment explaining the omitted case (see gaps). All 42 fail on released Bun and pass on this branch. That is the 40 added files plus the two re-syncedtest-domain-*files; the third re-synced file,test-crypto-domain.js, is skipped (see gaps), andtest-domain-crypto.jsandtest-domain-ee-error-listener.js, the other two of the 44, were already on main and are untouched here. Also re-ran theevents,async-hooks, andasynclocalstorageupstream suites (29/29) and Bun's ownasync_hooks(111) andevent-emitter/process(184) tests, sinceEventEmitter.initand the nextTick drain are cross-cutting.Merge note (d38da28): main refactored several uncaught-exception reporting sites into shared helpers (
task::report_error_or_terminate,NapiEnv::surface_exception,run_callback_with_result). Those conflicts were resolved by taking main's structure and passingUncaughtExceptionOrigin::Exceptionat the three remaining direct calls (Task.rs, WebSocketServerContext.rs, socket Handlers.rs); the branch's now-redundantreport_active_exception_as_unhandledhelper was dropped with main. Verified by probing each path (timer throw, socketopenthrow without anerrorhandler, websocketopenthrow) for theuncaughtExceptionorigin, plus the cron, domain, events, and process suites.Merge note (7836e08 + 12ff462): #41190 keeps an
enterWith()frame alive through the uncaught-exception dispatch of the same synchronous execution (as node does), while Bun drains nextTicks before dispatching an entry-point throw. node:domain's tick-retired pairing token then misread the still-running entry execution as a restored pairing, soprocess.domain = d; throw ewas routed tod(node aborts under the flag, exits 1 without it, because the setter never putsdon the stack). Fix: a newUncaughtExceptionOrigin::EntryPointException(CJS entry throw; listeners still see'uncaughtException') joinsEntryPointRejectionin telling node:domain that no callback boundary preceded the dispatch, andfatalErrorDispatchnow follows node'supdateExceptionCapturerule exactly: route toprocess.domain._errorHandleronly while a domain on the stack has an'error'listener, never pushing the active domain at dispatch time. All four setter cases (sync throw with and without the flag, timer, nextTick) match node v26.3.0.Gaps
Six upstream tests are deliberately not vendored, all blocked on the same two missing pieces:
fscallbacks are promise reactions, so a throw inside one surfaces via the rejection path with the context already restored. Blockstest-domain-implicit-fs.js,test-domain-implicit-binding.js,test-domain-multi.js,test-domain-no-error-handler-abort-on-uncaught-{5,9}.js. For the same reason, three vendored tests omit one case each (test-domain-abort-on-uncaught,test-domain-promise,test-domain-with-abort-on-uncaught-exception), andtest-crypto-domain.jsis vendored byte-identical behind a documentedcommon.skip(its callback throw takes the same rejection path).MakeCallbackequivalent, so the DEP0097 warning has no source. Blockstest-domain-dep0097.js.no test proof · iteration 41 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/process/process.test.js