JSModuleLoader: do not abort when terminate() lands during a worker’s module loads (WebKit pin bump) - #34655
JSModuleLoader: do not abort when terminate() lands during a worker’s module loads (WebKit pin bump)#34655robobun wants to merge 6 commits into
Conversation
…preload resolve() Picks up oven-sh/WebKit#309 and adds a stress test that terminates node:worker_threads Workers while they are still in their startup preload.
|
Updated 4:11 PM PT - Aug 7th, 2026
✅ @robobun, your commit bd9359dd77c7b4add1b3c1a7e5a12b4ed522e101 passed in 🧪 To try this PR locally: bunx bun-pr 34655That installs a local version of the PR into your bun-34655 --bun |
WalkthroughThe WebKit revision changes to include a termination-exception fix. A Node ChangesWorker preload termination coverage
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@scripts/build/deps/webkit.ts`:
- Around line 10-13: Replace the ephemeral preview value in WEBKIT_VERSION with
the merged stable autobuild-<sha> tag for WebKit PR `#309` before merging. Do not
leave autobuild-preview-pr-309-9cb28173 in the mergeable branch, and preserve
the existing version constant interface.
In `@test/js/node/worker_threads/worker-terminate-during-preload.test.ts`:
- Around line 61-63: Update the structured result assertions in the worker
termination test to treat stderr as diagnostic-only: preserve validation of
stdout containing “ok” and exitCode equal to 0, but remove the requirement that
stderr be empty. Ensure the subprocess continues draining stdout, stderr, and
exit concurrently.
🪄 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: 45bd6182-2816-4881-80ef-1988ef940755
📒 Files selected for processing (2)
scripts/build/deps/webkit.tstest/js/node/worker_threads/worker-terminate-during-preload.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Per review: keep stderr in the failure diff but do not require it to be empty (debug/ASAN lanes can emit benign warnings), and wire the calibration worker's error event to reject so a startup failure surfaces through the child's own catch.
The previous preview was based on WebKit main, which pulled in the JSMicrotask/JSPromise refactors (oven-sh/WebKit#295/#301) and nudged the node-net mimalloc-page-count leak test past its threshold on aarch64. The minimal branch carries only the termination fixes the new stress test needs (plus Jarred's oven-sh/WebKit#286 for the property-lookup assertions the same sweep surfaces).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@scripts/build/deps/webkit.ts`:
- Around line 11-15: Update the WEBKIT_VERSION constant to use the merged stable
autobuild-<sha> WebKit tag instead of the ephemeral
autobuild-preview-pr-309-5113fb40 preview tag; do not leave the branch mergeable
with the PR preview version.
🪄 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: 170f6dae-56c2-4653-b430-cac6a2c81431
📒 Files selected for processing (2)
scripts/build/deps/webkit.tstest/js/node/worker_threads/worker-terminate-during-preload.test.ts
|
Verified this also fixes #34690, the I captured a symbolized backtrace of the aborting worker thread by keeping the process alive after the report (ASAN Repro and verification on linux x64 debug ASAN, running 4 concurrent instances of
This also identifies the residual ~1-in-4000 |
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.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
robobun
changed the title
|
Revived this: oven-sh/WebKit#391 (the merged successor of #309) landed today and its autobuild is published, so the preview pin is replaced with the real one, 171babe26c3b, one commit past main's current pin. Merged main into the branch. Verified locally on the debug ASAN build: this PR's test, worker-transfer-terminate-stress.test.ts, test-worker-message-port-transfer-terminate.js, and 18/18 concurrent runs of the worker_destruction fixture all pass; the same fixture loop aborts 3/18 on the old pin. Details and the lldb backtrace are in the updated PR body. This assert is currently the only red on several otherwise green node-compat PRs (#34658, #34719, #34515, and worker_destruction on #34654), so it unblocks those once merged. |
robobun
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/js/node/worker_threads/worker-terminate-during-preload.test.ts`:
- Around line 28-35: Update timeToOnline() to track whether the worker has
reached the online event and register an exit handler that rejects if the worker
exits beforehand. Keep the existing online resolution and error rejection
behavior, ensuring every pre-online failure settles the promise with the worker
exit diagnostic.
🪄 Autofix
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: c14c0bd7-aad5-4a5a-9bf4-0542f713d70c
📒 Files selected for processing (2)
scripts/build/deps/webkit.tstest/js/node/worker_threads/worker-terminate-during-preload.test.ts
There was a problem hiding this comment.
I re-reviewed after the latest push and didn't find any bugs — the earlier feedback (stderr exact-match, wiring error/exit in the calibration wait, replacing the preview tag with the merged 171babe26c3b pin) is all addressed. Deferring to a maintainer only because this is a WebKit pin bump: even a one-commit engine bump should get a human sign-off that the autobuild artifacts are published and CI is green across all lanes on the new pin.
What was reviewed:
WEBKIT_VERSIONnow points at a stable merged sha (not a preview tag); the comment above it accurately names the upstream PR and delta.- Test drains stdout/stderr/exit concurrently, asserts a combined object with
stderr: expect.any(String), and the calibration promise rejects onerrorand pre-onlineexit. - Sweep loop's busy-wait and
terminate()sequencing look sound; no unbounded hangs (child.catch→process.exit(1), outer per-test timeout branches onisDebug).
Extended reasoning...
Overview
This PR does two things: (1) bumps WEBKIT_VERSION in scripts/build/deps/webkit.ts from ddea71318fec to 171babe26c3b — a single upstream commit (oven-sh/WebKit#391) that makes JSModuleLoader::hostLoadImportedModule propagate a TerminationException from resolve() instead of caching it as a resolution failure and tripping scope.assertNoException() in continueDynamicImport; and (2) adds test/js/node/worker_threads/worker-terminate-during-preload.test.ts, a stress test that calibrates a warm worker's time-to-online and sweeps terminate() through the preload window to hit the module loader mid-resolve().
Security risks
None identified. The WebKit-side change is a narrow control-flow adjustment in the module loader's termination-exception path (already reviewed and merged in oven-sh/WebKit). The Bun-side diff is a version constant + comment and a test file; no auth, crypto, parsing of untrusted input, or new user-facing surface.
Level of scrutiny
High, purely because of what is being bumped. WebKit is Bun's JS engine and its ABI is tightly coupled to every native bun build (per the ASAN/layout notes in webkit.ts itself). Even though the delta is one targeted commit and the actual code change was reviewed upstream, a maintainer should confirm (a) the autobuild-171babe26c3b… release artifacts exist for every os/arch/abi combination the build downloads, and (b) CI is green on the current HEAD — the only CI status visible in the timeline is build #75506 on an earlier preview-tag commit, which failed build-cpp everywhere. The test file on its own would be trivially approvable.
Other factors
All prior review feedback is resolved: my two earlier nits (exact-empty stderr assertion; unrejected once('online') wait) and CodeRabbit's three (preview tag, stderr assertion, pre-online exit) are each reflected in the current diff — the calibration promise now wires online/error/exit, the assertion is a combined {stdout, exitCode, stderr} object with stderr: expect.any(String), and the pin is a stable merged sha. The bug-hunting pass on this revision found nothing. The PR description includes an 8/8-fail-before / 23/23-pass-after probe on debug+ASAN and robobun's follow-up confirms it also fixes #34690 with a symbolized backtrace matching the described path. Given all that, the code itself looks ready; I'm deferring solely so a human signs off on the engine bump and confirms green CI on the final pin.
…sert is open ASAN builds compile WebKit assertions in, and terminate() landing during the worker's module loads trips ExceptionScope::assertNoException, tracked in #34655. Non-ASAN lanes keep running all three methods.
|
Additional evidence that this fix covers another CI abort seen in the wild: That test looks like a message-dispatch race (it transfers a MessagePort and terminates), but measured on a debug+ASAN build the Verified on current main (392726b, pin
Note on the base: main's pin moved to |
The fixture terminated 1ms after online. With online now firing before entry evaluation, that raced module loading, and on ASAN builds terminate() landed mid-load and tripped the WebKit assert tracked in #34655 rather than the mid-I/O window the test is for. The worker now posts a message once its action is in flight and the parent terminates on it, making the timing deterministic, so the ASAN skip from 7d70027 comes back out. The pre-existing fetch skip stays: terminate landing mid-fetch still hits its ASAN failure (about 1 in 25 locally).
1 participant
What does this PR do?
Bumps
WEBKIT_VERSIONfromddea71318fecto171babe26c3b(one commit: oven-sh/WebKit#391,JSModuleLoader: propagate a TerminationException from resolve() instead of treating it as a resolution failure) and adds a stress test for terminatingnode:worker_threadsWorkers mid-preload.test/js/node/worker_threads/worker-transfer-list.test.tsintermittently SIGABRTs on the debug+ASAN lane (seen in build 75365; test-side workaround in #34644):Cause
Since #31216 every
node:worker_threadsWorkerpreloadsnode:worker_threadson startup, so evennew Worker("", { eval: true })runs module-loader microtasks during startup.terminate()fires theNeedTerminationtrap on the worker VM; when that lands insideJSModuleLoader::hostLoadImportedModule'sresolve()call, theTerminationExceptionwas caught as a resolution error: cached inm_resolutionFailures, left pending byrejectWithCaughtException(TRY_CLEAR_EXCEPTIONrefuses to clear a termination), and carried intofinishLoadingImportedModule->continueDynamicImport, whosescope.assertNoException()aborts underENABLE(EXCEPTION_SCOPE_VERIFICATION). Frame-pointer trace from the worker thread at the abort:Release builds are unaffected: without
ENABLE(EXCEPTION_SCOPE_VERIFICATION)the assertion compiles to an emptyASSERTand the nextRETURN_IF_EXCEPTIONin the caller unwinds normally.Fix
oven-sh/WebKit#391 (merged):
hostLoadImportedModulereturns early when the exception caught fromresolve()is aTerminationException, so the caller'sRETURN_IF_EXCEPTIONunwinds instead of enteringFinishLoadingImportedModulewith a termination pending. This PR pins that commit.Testing
test/js/node/worker_threads/worker-terminate-during-preload.test.tsspawns a child that calibrates a warm worker's time-to-online, then sweepsterminate()through a band just below it against workers started with fourteen explicitnode:preloads (widening the cumulativeresolve()window so the sweep lands in it on any host speed). On the unfixed WebKit the child aborts; with the fix it exits 0.Multi-run probe on debug+ASAN linux-x64 (the assertion only exists under exception-scope verification):
The fix-side change is the
WEBKIT_VERSIONbump inscripts/build/deps/webkit.ts; there is nosrc/diff for the fail-before gate to stash, so the probe above is the fail-before evidence.Supersedes the test-side workaround in #34644.
[decide:webkit] gate passed · iteration 4 · 2 files touched
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 1 rejected · iteration 4
evidence per changed file