JSModuleLoader: propagate a TerminationException from resolve() instead of treating it as a resolution failure - #391
Conversation
…ion from resolve() instead of treating it as a resolution failure Host resolve() hooks can surface a TerminationException (RETURN_IF_EXCEPTION services VM traps, so a terminate request lands at any exception check). The resolution-error path attached error info to it, cached it as the specifier's resolution failure, called rejectWithCaughtException() — which deliberately leaves a TerminationException pending — and then went on into finishLoadingImportedModule() with the exception still set, tripping scope.assertNoException() in continueDynamicImport(). Every other rejectWithCaughtException() in this file is wrapped in RETURN_IF_EXCEPTION for exactly this reason; this path was missing the equivalent. Both callers already handle a null promise with an exception pending.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
There was a problem hiding this comment.
LGTM — narrow, well-justified guard for a termination race.
What was reviewed:
- Confirmed
vm.isTerminationException(Exception*)exists andRELEASE_AND_RETURNleaves the exception pending. - Verified both callers (
loadModuleandinnerModuleLoading) already checkscope.exception()right after the call and handle the null-promise + pending-exception shape. - Checked that the removed side effects (caching in
m_resolutionFailures,attachErrorInfo) are indeed inappropriate for a termination exception.
Extended reasoning...
Overview
Adds a 2-line early return (plus comment) in JSModuleLoader::hostLoadImportedModule: when resolve() surfaces a TerminationException, return nullptr with the exception still pending instead of treating it as a resolution failure. This prevents caching the termination as a resolution error and prevents continuing into finishLoadingImportedModule → continueDynamicImport → scope.assertNoException() with the un-clearable termination still set (the observed ASAN abort).
Security risks
None. This is exception-propagation plumbing on the VM-termination path; no user-controlled input handling, no auth/crypto/permissions.
Level of scrutiny
Moderate. It's core JSC runtime, but the change is a single guarded early return on an [[unlikely]] path that only fires during VM termination. It mirrors the pattern already applied to every other rejectWithCaughtException() site in this file (which are wrapped in RETURN_IF_EXCEPTION), so this is closing a gap rather than introducing new behavior. rejectWithCaughtException uses TRY_CLEAR_EXCEPTION, which by design does not clear termination exceptions — the PR description's analysis of why the old code aborts is correct.
Other factors
- Both call sites of
hostLoadImportedModulewere checked:loadModule(…, ModuleReferrer, …)hasRETURN_IF_EXCEPTION(scope, nullptr)immediately after;innerModuleLoadinghas an explicitif (scope.exception())that resetsdrainingInnerLoadand returns. So a null return with a pending exception is already the expected contract. - Skipping
addResolutionFailurefor termination is correct — caching it would poison future imports of that specifier with a bogus error after a new VM/worker starts, and the termination exception'svalue()isn't a meaningful resolution error anyway. RELEASE_AND_RETURNis the right macro: it releases the scope's exception-check obligation (satisfied by thescope.exception()read that producedresolutionError) and returns without touching the pending exception.- PR includes a concrete repro stack trace from CI. No prior reviewer comments to address.
Take main's WEBKIT_VERSION shape and bump it to 171babe26c3b (oven-sh/WebKit#391), the merged successor of the #309 preview this branch was pinned to.
…String Bump WebKit to 78d45d318434 (oven-sh/WebKit#393), which exposes TemporalZonedDateTime::toString(JSGlobalObject*) and a JSC::TemporalType / temporalType(JSValue) classifier. Drop Bun's copies: the ZonedDateTime toString recipe, Bun::TemporalType, and Bun::temporalObjectType. cppbind maps JSC::TemporalType to bun_jsc::TemporalType. Also picks up oven-sh/WebKit#391 (module loader: propagate TerminationException from resolve()).
…e ordered VM teardown (#37075) Makes `Worker` / `node:worker_threads` stable rather than experimental: every crash, use-after-free, assertion, leak and hang class around a VM's lifetime and teardown, on all platforms. Missing `worker_threads` API surface (`resourceLimits`, `trackUnmanagedFds`, `moveMessagePortToContext`, …) is out of scope. ### Lifetime model - WebCore's `ActiveDOMObject` / `ScriptExecutionContext` registry is restored, so `Worker`, `MessagePort`, `BroadcastChannel` and `WebSocket` are stopped in a real stop phase before the JSC VM is destroyed instead of from inside `~VM`. `Worker` is split, as upstream, into the script object and a `WorkerMessagingProxy` that owns the parent↔thread relationship. - Worker threads are refcounted and **joined** by their parent (Node's model). A parent tracks its children, stops them in its own stop phase and joins them before its VM goes away, so `terminate()` propagates through nested workers and resolves only once the thread is gone. No `pthread_exit`. - `VirtualMachine::teardown()` is the one ordered sequence for a finished worker and for main-thread exit: exit handlers run, then script is forbidden and everything the VM owns is stopped natively (WebCore objects, servers, listeners, watchers, sockets, dns, sqlite, in-flight `fetch`/S3 requests, `Bun.build` passes waiting on this VM's plugins — as in Node, no `'close'`/`'error'` handler runs after `'exit'`) → timers cancelled, children joined, in-flight off-thread work waited for or released, VM handle closed, queued work released → JSC VM destroyed → loops freed (uSockets; libuv on Windows) → destroy. - Every off-thread completion — thread pool (fs, crypto, zlib, transpiler, `dns.lookup`, shell builtins, `Bun.Archive`, password hashing), the HTTP thread (`fetch`, S3), the bundle thread, child-process waiter, fs watcher threads, napi async work / threadsafe functions, JSC helper threads — reaches a VM only through a per-VM `VmHandle` that teardown closes. Pool work is one typed carrier (`bun_jsc::Job`) whose JS-affine half only the owning thread can touch and teardown releases; work whose storage lives in JS objects or on another thread is counted and waited for; work that can block on an external party is registered so the stop phase aborts it. A late completion is refused and released by its producer instead of touching a dead VM. `EventLoop` has no cross-thread entry points any more. - A worker's "may run script" gate closes the moment its stop is requested — a parent's `terminate()`, its own `process.exit()`, or an uncaught error — not when its thread gets around to tearing down (Node's `can_call_into_js` / `is_stopping`). Every native→JS entry consults it (timer and immediate callbacks, event listeners, socket/server callbacks, pool-job completions, JSC deferred work, N-API), so nothing dispatches into a worker that is being stopped, whichever event source it came from. Promise settlement is the one native→promise boundary and never accepts an empty value: a JS conversion that a termination interrupted becomes "reject with the pending exception", which itself yields to the termination. - The event loop stays fair under producers that outpace it: one turn refills from the concurrent queue a bounded number of times; message drains take a fixed budget per task (a bounded batch per lock acquisition, never a whole-queue hand-back) and continue after the loop has polled; a UDP socket is read a bounded number of batches per readiness event. A worker posting faster than its parent deserializes, or a datagram socket that never runs dry, no longer holds that loop's timers, I/O — or its own pending stop. - Cross-thread costs of the handle are kept off hot paths: its read-mostly state sits on its own cache line away from the counters other threads update, and C++ tests the "may run script" byte inline rather than calling out per callback. ### Behaviour changes (Node parity) - `parentPort` is a real `MessagePort`: `parentPort.close()` ends the worker, `.ref()`/`.unref()` work, `receiveMessageOnPort` returns falsy messages, and parent messages are delivered only after the worker's entry module has run (a preload's un-awaited `import()` does not count as the entry running). - A worker with a pending top-level await starts and receives messages; it exits 13 if the await never settles, and a top-level await rejecting later fails the worker at that moment. - `await worker.terminate()` resolves the exit code (`1` for a running worker); `threadId` stays valid until exit; everything a worker posted before it exited is delivered before `'exit'`/`'close'`; `postMessage()` to a terminated worker is a no-op rather than an error; a rejection that is only a consequence of `terminate()` (a lookup or request cancelled by the stop) is not reported as the worker's `'error'`. - `process.exit()` / worker exit no longer runs microtasks or `nextTick`s queued before it. A worker's own `process.exit()` or uncaught error runs its `'exit'` handlers; a parent `terminate()` does not. `process.exit()` from inside (nested) `node:vm` contexts in a worker unwinds like any exception, and a `node:vm` `timeout` inside a worker no longer leaves the worker unable to run script afterwards. - Workers inside a process that has an IPC channel do not get a `process.send()` of their own over the process's channel fd. - N-API's pure constructors/accessors are callable while an exception is pending (as in Node), so node-addon-api can build the `Error` for a call a termination interrupted instead of aborting the process. - Assigning a non-function to `port.onmessage` releases the keep-alive a handler took. - Servers, listeners, sockets, UDP sockets, watchers and `dns.Resolver`s are closed by the exiting VM rather than left to GC finalizers; sqlite connections a VM opened are checkpointed and closed by that VM's exit; a `Bun.build` whose VM goes away mid-build is cancelled (its plugin requests failed, the pass finished) rather than abandoned or waited on, and one still queued behind other builds is released without waiting for them. - A connect-path DNS lookup (`Bun.connect`, `net`, `WebSocket` to a hostname) is process-wide and outlives the thread that happened to issue it: on macOS a worker exiting mid-lookup no longer answers every other thread's coalesced waiters with an error (and caches it for the TTL). - An addon's external-buffer finalizers run when the Worker that loaded it exits (`napi_create_external_{arraybuffer,buffer}`), as Node's environment teardown finalizes every remaining reference. - Releasing the last keep-alive from an immediate or a late promise reaction (e.g. `port.close()` inside `setImmediate`) is noticed before the loop parks. - Windows: a worker thread closes its loops. Open pipe/tty/process handles and readers mid file-read are closed through their owners in the stop phase, and requests still in flight are drained there — against a live VM that still accepts (and then awaits) the follow-on work a completion may start — before anything is released; sockets over named pipes and TLS-over-duplex sockets join the stop phase; a reader dropped mid-read keeps the buffer its pending read lands in. Two of the crashes were in JavaScriptCore rather than Bun: a `TerminationException` raised while the module loader resolves an import continued into `finishLoadingImportedModule` (fixed in oven-sh/WebKit#391, picked up by the WebKit version bump here). A worker parked in `Atomics.wait` with no timeout still cannot be terminated (#32802); that needs a JSC change and is tracked separately. ### Testing New tests accompany each behaviour fix (worker_threads, Web Worker lifecycle edges — `terminate()` at every phase of dns/fs/build/vm/napi/http work, message ordering and flooding, process exit ordering, sqlite). Seven more upstream `test-worker-*` files are vendored (one of them, the message-port infinite-message-loop test, passes only with these changes) and previously todo/skipped worker-related napi and regression cases run again. LeakSanitizer validation is turned back on for the ~70 worker / MessagePort / BroadcastChannel test files that were exempt. A source lint rejects laundering a `JsResult<JSValue>` into an empty `JSValue`. Main-thread `process.exit()` keeps its current fast path by default; the full main-thread teardown stays behind `BUN_DESTRUCT_VM_ON_EXIT=1`. Workers always tear down fully. ### Known / not in this PR - A worker parked in `Atomics.wait` with no timeout still cannot be terminated (#32802; needs a JSC change). - `worker_threads` message throughput through the real `MessagePort` is ~0.8× the previous ad-hoc path in a flood microbenchmark (round-trip latency and Web `Worker` messaging are unchanged); a follow-up, not a behaviour regression. - Windows: several concurrent connects to `localhost` can leave one connect stuck (pre-existing; reproduces on current releases; DNS-coalescing on the connect path). - A UDP socket whose receive buffer never drains (e.g. one echoing datagrams to itself on a fast machine) keeps its event loop from running anything else, including a worker's own exit; pre-existing loop-fairness issue, most visible on Windows, follow-up. - Memory a burst of concurrent workers used stays resident after they exit (sequential worker churn plateaus; it is the concurrent peak that is not returned to the OS) — allocator thread-exit policy, follow-up. - `Worker` start semantics for a never-settling top-level await, file-stream fairness on a saturated loop, and a few diagnostics-only items found while fuzzing are tracked separately. Fixes #31281 Fixes #30421 Fixes #15964 Fixes #29173 Fixes #34690 Fixes #31880 Fixes #33936 Fixes #32073 Fixes #33313 Fixes #32828 Fixes #11760 Fixes #26501 Fixes #18661 Fixes #15408 Fixes #23102 Fixes #21101 Fixes #13570 Fixes #31224 Fixes #28643 Fixes #37163 Fixes #25860 Likely also addressed (mechanism matches, not verified end-to-end): #34095; #22376 and the other emscripten-pthread reports (#25454, #19453, #29211, #29635) whose glue installs both a `parentPort` listener and `self.onmessage` — the double delivery behind them (#25860) is fixed, the packages themselves were not run; and the `parentPort.on('message').unref()` hang half of #32609 (its `Worker.performance` half is API surface, not addressed here). --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
…e ordered VM teardown (oven-sh#37075) Makes `Worker` / `node:worker_threads` stable rather than experimental: every crash, use-after-free, assertion, leak and hang class around a VM's lifetime and teardown, on all platforms. Missing `worker_threads` API surface (`resourceLimits`, `trackUnmanagedFds`, `moveMessagePortToContext`, …) is out of scope. - WebCore's `ActiveDOMObject` / `ScriptExecutionContext` registry is restored, so `Worker`, `MessagePort`, `BroadcastChannel` and `WebSocket` are stopped in a real stop phase before the JSC VM is destroyed instead of from inside `~VM`. `Worker` is split, as upstream, into the script object and a `WorkerMessagingProxy` that owns the parent↔thread relationship. - Worker threads are refcounted and **joined** by their parent (Node's model). A parent tracks its children, stops them in its own stop phase and joins them before its VM goes away, so `terminate()` propagates through nested workers and resolves only once the thread is gone. No `pthread_exit`. - `VirtualMachine::teardown()` is the one ordered sequence for a finished worker and for main-thread exit: exit handlers run, then script is forbidden and everything the VM owns is stopped natively (WebCore objects, servers, listeners, watchers, sockets, dns, sqlite, in-flight `fetch`/S3 requests, `Bun.build` passes waiting on this VM's plugins — as in Node, no `'close'`/`'error'` handler runs after `'exit'`) → timers cancelled, children joined, in-flight off-thread work waited for or released, VM handle closed, queued work released → JSC VM destroyed → loops freed (uSockets; libuv on Windows) → destroy. - Every off-thread completion — thread pool (fs, crypto, zlib, transpiler, `dns.lookup`, shell builtins, `Bun.Archive`, password hashing), the HTTP thread (`fetch`, S3), the bundle thread, child-process waiter, fs watcher threads, napi async work / threadsafe functions, JSC helper threads — reaches a VM only through a per-VM `VmHandle` that teardown closes. Pool work is one typed carrier (`bun_jsc::Job`) whose JS-affine half only the owning thread can touch and teardown releases; work whose storage lives in JS objects or on another thread is counted and waited for; work that can block on an external party is registered so the stop phase aborts it. A late completion is refused and released by its producer instead of touching a dead VM. `EventLoop` has no cross-thread entry points any more. - A worker's "may run script" gate closes the moment its stop is requested — a parent's `terminate()`, its own `process.exit()`, or an uncaught error — not when its thread gets around to tearing down (Node's `can_call_into_js` / `is_stopping`). Every native→JS entry consults it (timer and immediate callbacks, event listeners, socket/server callbacks, pool-job completions, JSC deferred work, N-API), so nothing dispatches into a worker that is being stopped, whichever event source it came from. Promise settlement is the one native→promise boundary and never accepts an empty value: a JS conversion that a termination interrupted becomes "reject with the pending exception", which itself yields to the termination. - The event loop stays fair under producers that outpace it: one turn refills from the concurrent queue a bounded number of times; message drains take a fixed budget per task (a bounded batch per lock acquisition, never a whole-queue hand-back) and continue after the loop has polled; a UDP socket is read a bounded number of batches per readiness event. A worker posting faster than its parent deserializes, or a datagram socket that never runs dry, no longer holds that loop's timers, I/O — or its own pending stop. - Cross-thread costs of the handle are kept off hot paths: its read-mostly state sits on its own cache line away from the counters other threads update, and C++ tests the "may run script" byte inline rather than calling out per callback. - `parentPort` is a real `MessagePort`: `parentPort.close()` ends the worker, `.ref()`/`.unref()` work, `receiveMessageOnPort` returns falsy messages, and parent messages are delivered only after the worker's entry module has run (a preload's un-awaited `import()` does not count as the entry running). - A worker with a pending top-level await starts and receives messages; it exits 13 if the await never settles, and a top-level await rejecting later fails the worker at that moment. - `await worker.terminate()` resolves the exit code (`1` for a running worker); `threadId` stays valid until exit; everything a worker posted before it exited is delivered before `'exit'`/`'close'`; `postMessage()` to a terminated worker is a no-op rather than an error; a rejection that is only a consequence of `terminate()` (a lookup or request cancelled by the stop) is not reported as the worker's `'error'`. - `process.exit()` / worker exit no longer runs microtasks or `nextTick`s queued before it. A worker's own `process.exit()` or uncaught error runs its `'exit'` handlers; a parent `terminate()` does not. `process.exit()` from inside (nested) `node:vm` contexts in a worker unwinds like any exception, and a `node:vm` `timeout` inside a worker no longer leaves the worker unable to run script afterwards. - Workers inside a process that has an IPC channel do not get a `process.send()` of their own over the process's channel fd. - N-API's pure constructors/accessors are callable while an exception is pending (as in Node), so node-addon-api can build the `Error` for a call a termination interrupted instead of aborting the process. - Assigning a non-function to `port.onmessage` releases the keep-alive a handler took. - Servers, listeners, sockets, UDP sockets, watchers and `dns.Resolver`s are closed by the exiting VM rather than left to GC finalizers; sqlite connections a VM opened are checkpointed and closed by that VM's exit; a `Bun.build` whose VM goes away mid-build is cancelled (its plugin requests failed, the pass finished) rather than abandoned or waited on, and one still queued behind other builds is released without waiting for them. - A connect-path DNS lookup (`Bun.connect`, `net`, `WebSocket` to a hostname) is process-wide and outlives the thread that happened to issue it: on macOS a worker exiting mid-lookup no longer answers every other thread's coalesced waiters with an error (and caches it for the TTL). - An addon's external-buffer finalizers run when the Worker that loaded it exits (`napi_create_external_{arraybuffer,buffer}`), as Node's environment teardown finalizes every remaining reference. - Releasing the last keep-alive from an immediate or a late promise reaction (e.g. `port.close()` inside `setImmediate`) is noticed before the loop parks. - Windows: a worker thread closes its loops. Open pipe/tty/process handles and readers mid file-read are closed through their owners in the stop phase, and requests still in flight are drained there — against a live VM that still accepts (and then awaits) the follow-on work a completion may start — before anything is released; sockets over named pipes and TLS-over-duplex sockets join the stop phase; a reader dropped mid-read keeps the buffer its pending read lands in. Two of the crashes were in JavaScriptCore rather than Bun: a `TerminationException` raised while the module loader resolves an import continued into `finishLoadingImportedModule` (fixed in oven-sh/WebKit#391, picked up by the WebKit version bump here). A worker parked in `Atomics.wait` with no timeout still cannot be terminated (oven-sh#32802); that needs a JSC change and is tracked separately. New tests accompany each behaviour fix (worker_threads, Web Worker lifecycle edges — `terminate()` at every phase of dns/fs/build/vm/napi/http work, message ordering and flooding, process exit ordering, sqlite). Seven more upstream `test-worker-*` files are vendored (one of them, the message-port infinite-message-loop test, passes only with these changes) and previously todo/skipped worker-related napi and regression cases run again. LeakSanitizer validation is turned back on for the ~70 worker / MessagePort / BroadcastChannel test files that were exempt. A source lint rejects laundering a `JsResult<JSValue>` into an empty `JSValue`. Main-thread `process.exit()` keeps its current fast path by default; the full main-thread teardown stays behind `BUN_DESTRUCT_VM_ON_EXIT=1`. Workers always tear down fully. - A worker parked in `Atomics.wait` with no timeout still cannot be terminated (oven-sh#32802; needs a JSC change). - `worker_threads` message throughput through the real `MessagePort` is ~0.8× the previous ad-hoc path in a flood microbenchmark (round-trip latency and Web `Worker` messaging are unchanged); a follow-up, not a behaviour regression. - Windows: several concurrent connects to `localhost` can leave one connect stuck (pre-existing; reproduces on current releases; DNS-coalescing on the connect path). - A UDP socket whose receive buffer never drains (e.g. one echoing datagrams to itself on a fast machine) keeps its event loop from running anything else, including a worker's own exit; pre-existing loop-fairness issue, most visible on Windows, follow-up. - Memory a burst of concurrent workers used stays resident after they exit (sequential worker churn plateaus; it is the concurrent peak that is not returned to the OS) — allocator thread-exit policy, follow-up. - `Worker` start semantics for a never-settling top-level await, file-stream fairness on a saturated loop, and a few diagnostics-only items found while fuzzing are tracked separately. Fixes oven-sh#31281 Fixes oven-sh#30421 Fixes oven-sh#15964 Fixes oven-sh#29173 Fixes oven-sh#34690 Fixes oven-sh#31880 Fixes oven-sh#33936 Fixes oven-sh#32073 Fixes oven-sh#33313 Fixes oven-sh#32828 Fixes oven-sh#11760 Fixes oven-sh#26501 Fixes oven-sh#18661 Fixes oven-sh#15408 Fixes oven-sh#23102 Fixes oven-sh#21101 Fixes oven-sh#13570 Fixes oven-sh#31224 Fixes oven-sh#28643 Fixes oven-sh#37163 Fixes oven-sh#25860 Likely also addressed (mechanism matches, not verified end-to-end): `self.onmessage` — the double delivery behind them (oven-sh#25860) is fixed, the packages themselves were not run; and the `parentPort.on('message').unref()` hang half of oven-sh#32609 (its `Worker.performance` half is API surface, not addressed here). --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Host
resolve()— and JSC's ownJSModuleLoader::resolve(const Identifier&, …)wrapper around it, whoseRETURN_IF_EXCEPTIONs after thejsStringconversions run before the hook is even called — can surface aTerminationException:RETURN_IF_EXCEPTIONservices VM traps, so aterminate()request from another thread materializes at whatever exception check runs next. No embedder code needs to be involved.hostLoadImportedModule's resolution-error path thenpromise->rejectWithCaughtException(vm, scope), which by design refuses to clear aTerminationExceptionand leaves it pending,finishLoadingImportedModule(...)with the exception still set — for a dynamic import that reachescontinueDynamicImport, whosepromise->reject(...); scope.assertNoException();aborts.Every other
rejectWithCaughtException()in this file is wrapped inRETURN_IF_EXCEPTION(scope, ...)so a termination propagates; this path was missing the equivalent. The fix returns early (null promise, exception pending) when the caught exception is the termination exception; both callers (loadModule,innerModuleLoading) already handle that shape. Upstreammainhas the same code: https://github.com/WebKit/WebKit/blob/9ef04dabf52d7432a3e7ab13f44f9d8b4bd933a0/Source/JavaScriptCore/runtime/JSModuleLoader.cpp#L646 . An audit of the other catch-and-convert sites in the module machinery (JSMicrotask.cppmodule-loading tasks,CyclicModuleRecord.cpp,Completion.cpp,JSWebAssembly.cpp,globalFuncImportModule) found they all return immediately after the catch; this is the only one that continues.Observed in Bun as an intermittent
ASSERTION FAILED: !exception()(ExceptionScope.h:61) on the ASAN lane whenworker.terminate()lands while the worker thread is resolving its preload module: