domain: route fs callback throws to uncaughtException, unblocking 4 tests (domain 88%→96%) - #34661
domain: route fs callback throws to uncaughtException, unblocking 4 tests (domain 88%→96%)#34661cirospaciari wants to merge 96 commits into
Conversation
…r tests Carries the callback-dispatch change from the standalone PR (fs, dns and crypto.pbkdf2 callbacks are dispatched from a promise reaction, so a throw inside one surfaced as an unhandledRejection instead of an uncaughtException), because these four domain tests need it and node:domain together. Neither half converts them alone: on main node:domain is a stub, so they die before reaching any fs call; on this branch domain works but the throw never escapes the promise. Two of the four are byte-for-byte the already-vendored abort-on-uncaught 4 and 8 with process.nextTick swapped for fs.exists, so the fs call is the entire difference. Tests copied verbatim from Node v26.3.0.
|
Updated 9:37 PM PT - Aug 7th, 2026
✅ @robobun, your commit a5914e1f65e40759dcd0cdad0a2115de22ea1618 passed in 🧪 To try this PR locally: bunx bun-pr 34661That installs a local version of the PR into your bun-34661 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
The opendir case read the first entry of the shared describe-level fixture dir, which the sibling symlink cases populate with l3/l4/lnc links. readdir order is filesystem-dependent, so dir.read() could return a symlink instead of file.txt. Use a private mkdtemp dir containing only file.txt.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Pre-existing assertNoException SIGABRT tracked in #34846; hits unrelated branches including dependency-bot bumps.
The package-binary-size gate compares against main's canary; this branch carries a full node-v26 feature stack, so the growth is expected and reviewed as part of the stack, not a regression of this PR.
- fstat and read now go through ensureCallback like their siblings, so a missing or non-callable callback throws ERR_INVALID_ARG_TYPE synchronously (node's makeStatsCallback behavior) instead of routing a TypeError through reportUncaughtException. - reword the symlink 4-arg comment and test title: the ignored non-callable callback is a preserved Bun divergence, not node parity. - runScript drains stderr and returns it so failing cases show the child's stack trace. - the setImmediate ordering marker is registered inside the fs callback so the assertion is deterministic, not a threadpool race. - the opendir fixture dir is created by the parent via tempDirWithFiles so CI runners don't accumulate orphaned temp dirs. [allow size]
No 127.0.0.1 PTR entry in the Windows hosts file, so the call falls through to a real query there; same gate as the vendored test-c-ares.js. [allow size]
… the callback-throw suite concurrently The pbkdf2 completion handlers are hoisted named functions bound to the guarded callback; guardCallback's comment names the Node v26.3.0 mechanism it mirrors; the independent subprocess cases use it.concurrent per the test conventions. [allow size]
There was a problem hiding this comment.
All six earlier review threads are addressed and I found nothing new this pass. Deferring to a human for the final sign-off given the breadth: this reroutes throws in ~40 fs + ~15 dns + pbkdf2 callback sites from unhandledRejection to uncaughtException, is stacked on #31828, and rides an unrelated expectations.txt quarantine (test-http2-reset-flood.js, #34846).
What was reviewed:
guardCallbackwrapper and everyensureCallback/validateResolvereassignment site in fs/dns/crypto — no site left with a bareguardCallbackon an unvalidated callback (the earlier fstat/read gaps are fixed).- The pbkdf2 truthy-non-callable-callback escalation candidate — ruled out:
if (callback)+.bindon the guarded wrapper doesn't change the pre-existing behavior class. - Test hermeticity/determinism: the setImmediate race, opendir tempdir leak, and Windows dns.reverse gate were all fixed; the suite now runs
it.concurrent.each.
Extended reasoning...
Overview
Adds guardCallback in internal/shared.ts (try/catch → jsFunctionReportUncaughtException) and threads it through ensureCallback in node:fs (~40 sites), validateResolve + direct calls in node:dns (~15 sites), and pbkdf2 in node:crypto. Declares jsFunctionReportUncaughtException in BunProcess.h. Vendors four upstream test-domain-* tests verbatim, adds a 19-case subprocess suite in fs.test.ts, and quarantines test-http2-reset-flood.js under ASAN in expectations.txt.
Security risks
None identified. The change catches user-thrown exceptions inside callbacks and routes them to the existing uncaughtException machinery via an already-exported C++ function; no new input parsing, no privilege boundaries crossed.
Level of scrutiny
High — this alters user-visible error semantics for essentially every async node:fs/node:dns callback (a throw that was previously an unhandledRejection warning becomes an uncaughtException, which crashes by default). That's the correct Node behavior and the whole point of the PR, but the blast radius plus the stacked-PR dependency (#31828) and the unrelated expectations.txt rider make it a human call rather than a bot approval.
Other factors
Six prior review rounds from me were all addressed (fstat/read ensureCallback, symlink 4-arg comment/test rewording, opendir tempdir cleanup, setImmediate race fix, Windows dns.reverse gate, it.concurrent.each). The bug hunter found nothing this run; the one candidate (pbkdf2 non-callable callback escalation) was verified not to change behavior versus the pre-PR path. The expectations.txt addition cites a tracked issue (#34846) and documents it as pre-existing on unrelated branches, which is the correct format per the file's own header — but a human should still confirm they're comfortable landing that quarantine here rather than in its own PR.
There was a problem hiding this comment.
No new issues this pass, but this reworks the runtime's uncaught-exception/exit semantics (VirtualMachine liveness gate, fatal vs keep-alive split, fs/dns/crypto callback routing, EventTarget defer-then-fatal, a user-facing dns.lookup(falsy) breaking change) across 30 files, is stacked on #31828, and carries an explicitly deferred sibling-class item (net.ts 'data' listener throw) pending a scope call — needs a human sign-off.
What was reviewed:
ensureCallback/guardCallbacksweep across fs/dns/crypto — every site reassigns the wrapped callback; thesymlink4-arg divergence is intentional and tested.uncaught_exception_fatalgating (REPL / hot / worker / entry-point exclusions) and the Mode::Bun/Throw/Strict rejection routing underhot_reload.- The
reportError→ fatal sweep in net.ts / diagnostics_channel / webstreams_adapters — no remaining keep-alive stragglers insrc/js/node. queueUncaughtExceptionNextTickexception-scope handling and the webview backends staying on keep-aliveBun__reportError.
Extended reasoning...
Overview
This PR routes throws from Node-style async callbacks (fs/dns/crypto) to uncaughtException instead of unhandledRejection, unblocking four upstream test-domain-* tests. To make that safe it splits VirtualMachine::uncaught_exception into a keep-alive path (Bun-native servers, reportError, webview backends) and a new uncaught_exception_fatal path (Node-compat throws: nextTick drain, timers, guardCallback, EventTarget listener defer, --unhandled-rejections=throw|strict, Bun.cron), removes unhandled_error_counter == 0 from the event-loop liveness check, and adds a suppress_fatal_uncaught VM flag for the REPL. It also lands a Node v26.3.0 breaking change (dns.lookup(falsy) now throws synchronously) with corresponding rewrites of three vendored dns tests, and cleans up dead once-bridge code in ws.js.
Security risks
None identified. No auth/crypto primitive changes (crypto.ts only wraps callback dispatch), no untrusted-input parsing added. The tamper-resistance nit on .bind was declined by the maintainer as within the module's existing threat bar.
Level of scrutiny
High. This changes when a Bun process exits versus keeps running on unhandled errors — the most fundamental runtime contract there is — and the PR history shows how easy it is to get one caller on the wrong side of the keep-alive/fatal split (net.ts handshake catches, diagnostics_channel, EventTarget, bun test, Mode::Bun rejection routing, webview backends — all iterated on during review). It's also stacked on an unmerged base (#31828) and overlaps #34660, so merge order matters.
Other factors
The PR has been through ~50 iterations; every prior inline finding from me and from cirospaciari shows resolved. Two items remain deliberately out of scope by the author's own note: the net.ts 'data'-listener-throw class (blocked on http-client parser-error handling, pending cirospaciari's scope call) and the http1_server_fallback outgoing queue (attributed to #34432 on main, not this branch). Test coverage for the new routing is substantial (per-API matrix in fs.test.ts, keep-alive vs fatal contracts in process.test.js, REPL survival, EventTarget sync/async). Given the blast radius and the open scope decision, this should be signed off by a maintainer rather than auto-approved.
shared.ts/crypto.ts/fs.ts/fs.test.ts: take the base side throughout - it carries the #34660 batch as merged to main, which subsumed this branch's older draft of the same callback-throw helpers. No-Verification-Needed: merge resolution taking the landed/reviewed side of the same feature; CI verifies the merged tree
After the main merge the piped-stdin repl loop no longer waits for timers between lines, so the 0ms setTimeout throw may never fire before .exit (fired on debug, not on release; build 90257 failed on 7 lanes). The nextTick case alone pins suppress_fatal_uncaught: pre-fix the first throw hard-exits and REPL-SURVIVED:42 never prints. The tick error's position is build-dependent (between lines on debug, at exit drain on release), so assert presence only.
…n [allow size] The old sink (Bun__reportUnhandledError) filtered termination exceptions downstream; Bun__reportError does not, and wsOnMessage discarded the clear's return value while its two sibling frame loops already guard. Match them: skip the report when the exception is a sticky termination.
…rms [allow size]
The inner fatal gate (hot_reload == 0 inside uncaught_exception_impl)
already prevents process_exit under watch/hot and falls to the
keep-alive print, so the outer guards' only net effect was skipping
Bun__handleUncaughtException: process.on('uncaughtException') listeners
stopped receiving unhandled rejections under --hot with
--unhandled-rejections=throw/strict (pre-PR both arms dispatched
unconditionally). Verified under --hot: the listener fires (CAUGHT:x)
and the watcher stays alive with and without a listener; non-hot
fatal-exit tests unchanged.
…h [allow size] Node runs both through CallbackIntoModule<true>, which calls TriggerUncaughtException on throw: the process prints the error and exits 1 unless an uncaughtException handler intervenes. Our keep-alive report let these throws keep the process running. The complete-callback site that used report_active_exception_as_unhandled now takes the exception and calls uncaught_exception_fatal directly, minus termination exceptions; the shared helper keeps its keep-alive behavior for EventLoop::run_callback and the Bun-native callers. Verified: "can throw an exception from an async_complete_callback" passes. The four napi.test.ts 5000ms timeouts here are debug-ASAN slowness, not this change: the orphan-leak fixture prints exactly the expected output in 10.7s when run directly, "has the right lifetime" fails identically with this change stashed, and the define_class case spawns eight child processes.
…[allow size] fs.watch, fs.watchFile and dgram 'message' listeners reached native through unguarded callbacks, so a throw was reported keep-alive; with the liveness counter gone the watcher or bound socket ref then kept the loop alive and the process hung after printing. Node fatal-exits all three (FSEventWrap, StatWatcher and UDPWrap invoke them via MakeCallback), as did bun 1.4.0 via the counter gate. Wrap the bound listeners in guardCallback at the node-compat layer, like the fs/dns one-shot callbacks: watch.ts and watchfile.ts at construction, dgram.ts on the data/drain/error callbacks it hands Bun.udpSocket, leaving the native surfaces' keep-alive contract alone. The close-from-close watch test now installs an "error" listener: the abort path delivers an "error" event whose unhandled throw native previously swallowed with clear_exception; guarded, it reports as uncaught like node.
|
Review follow-up in 4b143eb: with the liveness counter gone, a throwing fs.watch, fs.watchFile or dgram 'message' listener printed the error and then hung (the watcher or socket ref kept the loop alive). Node and bun 1.4.0 both exit 1 there. Fixed by guarding those listeners at the node-compat layer, with tests. One open design call for a maintainer: node:http2 session handler throws. Its dispatch rides the same keep-alive path as the Bun-native servers, so on this branch an uncaught throw in for example a 'stream' handler leaves the server serving, where node (and 1.4.0, via the old counter) exits 1. Making node:http2 match node means guarding its ~40 native-dispatched handlers, which felt too large to fold in here without a nod. Happy to do it here or as a follow-up, whichever you prefer. |
…ves the guard [allow size] node's fs.watch emits only "close" on abort; the AbortError "error" event is bun's own, and its unhandled ERR_UNHANDLED_ERROR was swallowed natively before the listener was guarded. Guarded, it became a fatal uncaught exception and test-fs-watch-abort-signal.js (which installs no "error" listener, as node allows) exited 1. emit_abort now tags the delivery "abort" and the JS layer emits the "error" event inside a try/catch, so the abort event keeps its old lenient contract while real watcher events and errors stay on the fatal path. fs.promises.watch reads signal.aborted before draining its queue, so the retagged entry stays unreachable there.
|
CI on fdb7ab5: the fs.watch abort regression from the previous run is fixed, test-fs-watch-abort-signal passes on all lanes. The one remaining red is test-worker-message-port-transfer-terminate on the x64-asan lane, which is the known worker-terminate race tracked in #34095 and #34690 (30 local ASAN runs here do not reproduce it); the other entries passed on retry. Ready for review from my side; the open items are the scope notes above and the node:http2 design call. |
…to the fatal path [allow size]
A throw from a threadsafe function's JS callback or from a ChildProcess
'message' listener was reported keep-alive; with the liveness counter
gone, the TSF's poll ref or the child and channel refs kept the loop
alive and the process hung after printing. Node fatal-exits both (TSFN
via CallbackIntoModule, enforced by default since node 26; the channel's
onread via MakeCallback), as did bun 1.4.0 through the counter gate.
The TSFN Js arm takes the exception and calls uncaught_exception_fatal
(skipping termination), like the async_work complete path; the C arm
now also drains a pending napi exception or a global exception after
call_js, which node clears at callback scope close. #emitIpcMessage
reroutes through reportUncaughtException at the node:child_process
layer, leaving the raw Bun.spawn ipc handler's pinned keep-alive
contract alone.
The child-side process.on('message') throw already exits 1.
…mp dirs, full pipe drains [allow size] fs.promises.watch's event filter now skips "abort" like "close" and "error", so the retagged abort delivery is not run through the ignore matcher before the signal.aborted check discards it. The watch and watchFile throw fixtures create their directories in the parent with tempDir and thread the path in, since the fatal exit leaves child-created mkdtemp dirs behind. The five new subprocess tests drain both pipes and assert the combined object, so a regression shows the child's output instead of just the exit code.
…t on listeners [allow size] EventEmitter.once dispatches through this.on, which already hits the overridden on() -> #armAndOn, so the once() override was a pure passthrough after the #armAndOn collapse. The watch abort branch emits only when an "error" listener exists instead of swallowing every throw: no listener stays silent (node emits nothing on abort), and a present listener's throw now reaches guardCallback like the sibling "error" branch.
The third consumer of the native fs.watch binding ran the user-supplied ignore matcher unguarded, so a throwing matcher was reported keep-alive and the watcher ref hung the process. guardCallback reroutes it to the fatal path like the FSWatcher and StatWatcher listeners.
The guarded listener only returns Err for a termination exception now, and report_active_exception_as_unhandled takes it and skips reporting instead of clear_exception dropping termination state on the floor.
Native calls it with five args, past guardCallback's arity fast path, so the per-packet handler carries its own try/catch like child_process's #emitIpcMessage. drain and error stay on guardCallback (zero and one arg).
…[allow size] The process.exit(1) in the unhandledRejection handler killed the child mid-transform, dropping the original assertion that the rewrite succeeds despite the detached rejection. A log-only handler pins both: the rejection reaches unhandledRejection and the body still renders.
2 participants
Four upstream domain tests convert here that neither of the two changes involved converts on its own.
Stacked on #31828. Carries the callback-dispatch fix from #34660 as well, because the tests need both halves — see below.
Why these needed two changes
main:node:domainis a 96-line stub with noDomainconstructor, so the tests die before reaching any fs call. The callback fix alone converts nothing.node:domainworks, but a throw inside an fs callback is dispatched from a promise reaction, so it surfaces as anunhandledRejectionand never reachesuncaughtException— which is exactly the machinery domain hooks. The domain PR alone converts nothing.Together they pass. The clinching detail:
test-domain-no-error-handler-abort-on-uncaught-5and-9are byte-for-byte the already-vendored and already-passing tests4and8, withprocess.nextTickswapped forfs.exists. The fs call was the entire difference.Tests:
test-domain-implicit-binding,test-domain-implicit-fs,test-domain-no-error-handler-abort-on-uncaught-5,test-domain-no-error-handler-abort-on-uncaught-9— all copied verbatim.Relationship to #34660
The
src/js/{internal/shared,node/fs,node/dns,node/crypto}.tschanges here are the same commit as #34660, which targetsmainand carries the full measurement: node's exact semantics (the throw reachesuncaughtExceptionsynchronously from the callback's own frame, no extra tick), byte-identical happy-path timing verified by diffing an event-loop ordering trace, and an interleaved same-commit regression sweep over 356 vendored files showing zero deltas.If #34660 merges first, rebasing this drops that half and leaves only the four tests. Reviewing them together is probably easier — #34660 has the depth, this one has the payoff.
Verification
diffclean)mustCalltomustNotCallmakes them exit 7 and 1 respectivelytest-domain-*tests on this branch still pass (44/44)The rest of the domain gap
Upstream has 50
test-domain-*files; 44 were already vendored, 6 were genuinely missing (not the ~46 agit ls-treediff suggests). With these four, 48 of 50. The last two are not domain problems:test-domain-dep0097needsnode:inspector(inspector.open()→ERR_NOT_IMPLEMENTED, supportnode:inspector#2445).test-domain-multiis a http client bug: the domain half is fully correct — the write callback fires, the throw routes to domain B and not root domain A, and B's handler runs — but the handler then writes a strayHto corrupt the chunk header, and Bun's http client parser doesn't error on it, soc.on('error')never fires and the server never closes.Worth noting one false alarm ruled out along the way:
--abort-on-uncaught-exceptionis correctly implemented in Bun (all 8 vendored abort tests pass), so these failures were never a CLI gap.no test proof · iteration 58 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/repl/repl.test.ts test/js/node/dgram/node-dgram.test.js test/js/node/dns/node-dns.test.js test/js/node/process/process.test.js test/js/node/watch/fs.watch.test.ts test/js/node/watch/fs.watchFile.test.ts test/napi/napi.test.ts
Compat impact (upstream node v26.3.0 test files vendored, in-tree = passing)