jsc: let worker.terminate() preempt pure-Wasm loops - #36356
Conversation
|
Warning Review limit reached
Next review available in: 8 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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 8:22 AM PT - Aug 9th, 2026
✅ @robobun, your commit b16dd4cdbfdd3abf47346bfe850af19873fe08d7 passed in 🧪 To try this PR locally: bunx bun-pr 36356That installs a local version of the PR into your bun-36356 --bun |
There was a problem hiding this comment.
Beyond the two test-file nits, I checked that prebuiltUrl() / prebuiltDestDir() in scripts/build/deps/webkit.ts already handle the autobuild-preview-pr-* tag shape (they do — release-tag prefix and cache-dir keying both branch on startsWith("autobuild-")), and that download.ts has a dedicated 404 message for when the preview release disappears. The test itself is well-constructed: subprocess isolation, concurrent pipe drain, exact-value assertions on {terminated, code, exited}, and the wasm case demonstrably hangs on the pre-bump build.
The substantive change here is the WebKit bump — the actual fix lives in oven-sh/WebKit#372, which adds a trap-aware stack-limit poll at every Wasm loop back-edge across IPInt/BBQ/OMG. That's JIT-codegen and hot-path territory in the JS engine, and WEBKIT_VERSION now points at an unmerged preview tag, so this needs a maintainer's eyes on the WebKit PR and the bump sequencing rather than auto-approval.
Extended reasoning...
This PR's Bun-side diff is minimal (a version-string bump plus a new test), and the test file follows repo conventions well. But the effective change is a JavaScriptCore engine bump that pulls in a JIT codegen modification touching every Wasm loop back-edge across three tiers — that is core-runtime, performance-sensitive, and lives in an external PR (oven-sh/WebKit#372) that this review cannot inspect. Pinning WEBKIT_VERSION to an autobuild-preview-pr-* tag is a supported pattern in the build scripts, but it also means main will 404 on WebKit fetch the moment that PR merges or closes, so merge sequencing is a maintainer decision. The two inline findings are style-level nits on the test file and would not on their own block approval; the reason to defer is the engine bump, not the nits.
|
WebKit preview build 972b37c3 failed to assemble on ARM64: the IPInt loop VMTraps poll used Bumped |
There was a problem hiding this comment.
Beyond the inline nit: both earlier review nits (wire error → reject, comment the warm sleep) are addressed in 5093966. Also checked that scripts/build/deps/webkit.ts handles the autobuild-preview-pr-* tag shape — prebuiltUrl skips the double-prefix and prebuiltDestDir keys on the full tag, so the mechanical bump is fine. The substantive change (Wasm loop back-edge trap poll across IPInt/BBQ/OMG) lives in oven-sh/WebKit#372 and pins main to a preview autobuild — leaving that merge-order call to a maintainer.
Extended reasoning...
The Bun-side diff is a one-line WEBKIT_VERSION bump plus a well-structured regression test; the test now incorporates both fixes from my previous review pass. The only new finding this run is a cosmetic redundant assertion. What actually needs human eyes is not in this diff: the JSC change adds a per-back-edge poll to every Wasm tier's codegen, and WEBKIT_VERSION is being pointed at an unmerged WebKit PR's preview tag — both are maintainer decisions I can't make.
|
Pushed review refinements to oven-sh/WebKit@bff0b81e and bumped
WebKit CI for the previous revision (3cb341c2) was 39/39 green; Bun CI will 404 on the prebuilt until the bff0b81e autobuild finishes. |
| * From https://github.com/oven-sh/WebKit releases. | ||
| */ | ||
| export const WEBKIT_VERSION = "549170099226f816a4b204ea1d8fa102fb79eefa"; | ||
| export const WEBKIT_VERSION = "autobuild-preview-pr-372-bff0b81e"; |
There was a problem hiding this comment.
🔴 WEBKIT_VERSION is pinned to autobuild-preview-pr-372-bff0b81e, an ephemeral preview release for the still-open oven-sh/WebKit#372. Per the repo's own docs (scripts/build/download.ts:278, .claude/commands/upgrade-webkit.md:34, .claude/docs/landing-prs.md:47), preview releases are deleted when the WebKit PR merges/closes, so landing this as-is will 404 every fresh build's prebuilt fetch. Before merging this PR, oven-sh/WebKit#372 must land and WEBKIT_VERSION must be repinned to the merged commit's stable 40-hex sha.
Extended reasoning...
What the pin points at
scripts/build/deps/webkit.ts:6 changes WEBKIT_VERSION from a stable 40-hex commit sha (549170099226f816…) to autobuild-preview-pr-372-bff0b81e. That is a preview release tag keyed to an unmerged oven-sh/WebKit pull request (#372). prebuiltUrl() at scripts/build/deps/webkit.ts:73-79 will resolve this to https://github.com/oven-sh/WebKit/releases/download/autobuild-preview-pr-372-bff0b81e/bun-webkit-<os>-<arch><suffix>.tar.gz for every fresh build.
Why this is a merge-blocker per the repo's own rules
Three separate places in this repo document that preview tags must never reach main:
.claude/docs/landing-prs.md:47(Dependencies & vendoring): "Never merge a pin to an ephemeral artifact (preview tags, unmerged-PR builds) — swap to the merged upstream SHA and verify prebuilt artifacts exist for every platform × flavor before merge."scripts/build/download.ts:278-279: "autobuild-preview-pr-*WebKit tags are the sharp edge: GitHub deletes the preview release when the PR merges or closes, so every build 404s at once.".claude/commands/upgrade-webkit.md:34: "Preview mode: also note in the description that WEBKIT_VERSION points at a preview build and must be bumped to the merge-commit'sautobuild-<sha>after the oven-sh/WebKit PR merges — do that bump before merging the bun PR."
The build system explicitly supports autobuild-preview-* tags (prebuiltUrl and prebuiltDestDir both branch on startsWith('autobuild-')), but that support exists so an in-flight Bun PR can run CI against an unreleased WebKit — not so main can depend on it. git log -S 'autobuild-preview' shows no preview tag has ever landed on main; only this branch uses one.
Why nothing else guards this
test/internal/source-lints/webkit-prebuilt-url.test.ts:123-125 accepts autobuild-* as a valid WEBKIT_VERSION form (it has to, so the branch itself passes CI), so no automated check catches this — human review is the only gate. The PR timeline confirms the author knows the state is temporary ("Bun CI will 404 on the prebuilt until the bff0b81e autobuild finishes"), but the PR description does not carry the "must be repinned before merge" note the workflow doc requires, and no existing review comment flags it as merge-blocking.
Step-by-step: what happens if this merges as-is
- This PR merges to
mainwithWEBKIT_VERSION = "autobuild-preview-pr-372-bff0b81e". - wasm: poll VMTraps at loop back-edges so pure-Wasm loops can be terminated WebKit#372 is subsequently merged (or closed, or force-pushed and re-tagged).
- GitHub deletes the
autobuild-preview-pr-372-bff0b81erelease — this is documented behavior atdownload.ts:278-279. - Every fresh Bun build (CI, contributor clones, release pipeline) hits
prebuiltUrl(cfg)→https://github.com/oven-sh/WebKit/releases/download/autobuild-preview-pr-372-bff0b81e/…→ HTTP 404. prebuiltDownloadError()atdownload.ts:282-294fires with "WebKit preview release is gone" and the build fails. Every branch based onmainis broken until someone pushes a repin.
Additionally, while the preview exists, main depends on code from an unmerged, force-pushable branch — the exact contents behind the fix are not yet on oven-sh/WebKit main.
Fix
Do not merge until:
- wasm: poll VMTraps at loop back-edges so pure-Wasm loops can be terminated WebKit#372 has merged, and
WEBKIT_VERSIONis updated to the merged commit's 40-hex sha (the stableautobuild-<sha>release), andgh release view autobuild-<sha> --repo oven-sh/WebKitconfirms artifacts exist for every platform × flavor.
Then this PR is a one-line change back to the shape the doc-comment above the constant describes: "WebKit commit — determines prebuilt download URL."
There was a problem hiding this comment.
Correct, and intentional for CI verification against the unmerged JSC change. Merge sequencing is: (1) oven-sh/WebKit#372 merges, (2) this PR repins WEBKIT_VERSION to the merged 40-hex sha, (3) this PR merges. Leaving this thread open as the merge-blocker reminder.
Bump WEBKIT_VERSION to the oven-sh/WebKit#372 preview build (rebased onto fork main 78d45d3184 as a50d1e5072): JSC now polls the trap-aware stack limit at every Wasm loop back-edge (IPInt/BBQ/OMG), so a Worker spinning in a pure-Wasm loop (no JS re-entry, e.g. loop { br 0 }) observes VM::notifyNeedTermination() instead of never reaching a trap check. worker-terminate-wasm-loop.test.ts covers the three loop shapes (JS loop, Wasm loop calling a JS import, pure Wasm loop) and asserts terminate() resolves with exit code 1 for each. Before merging, oven-sh/WebKit#372 must land and WEBKIT_VERSION must be repinned to the merged 40-hex sha; the preview tag is CI-only.
b193049 to
b16dd4c
Compare
|
Refreshed this PR:
Still blocked on oven-sh/WebKit#372 review/merge, then the repin to a stable sha (tracked in the open thread on |
Problem
A Worker running a WebAssembly function whose body is a tight loop with no JS re-entry (e.g.
(loop (br 0))) cannot be preempted byworker.terminate(): the promise never settles, noexitevent fires, and the thread spins at 100% CPU until the process dies. JSfor(;;){}and Wasm loops that call an imported JS function per iteration are already preemptible because they reach a JS-side trap check; Node terminates all three shapes in a few ms.Any host that isolates untrusted or third-party Wasm in a Worker cannot reclaim a spinning module: the worker is unkillable and burns a core forever. Since the worker-lifetime rework (#37075) joined worker teardown, the blast radius is larger: a parent worker calling
process.exit()while a descendant spins in Wasm now wedges the whole process instead of exiting.Cause
Root cause is in JavaScriptCore, not Bun's worker plumbing.
VM::notifyNeedTermination()firesVMTraps::NeedTermination->StackManager::requestStop()setsm_trapAwareSoftStackLimiton every registeredMirror(including eachJSWebAssemblyInstance'sm_stackMirror) toUINTPTR_MAX. The only Wasm-side consumer of that field was the IPInt function prologue; BBQ/OMG prologues read the non-trap-awarem_softStackLimit, and no tier reads it at loop back-edges. TheVMTraps::SignalSenderpath only patches JS DFG/FTL CodeBlocks. A pure-Wasm loop therefore never observes the termination request.Fix
The JSC change is in oven-sh/WebKit#372: add a trap-aware stack-limit poll at each Wasm loop head (IPInt / BBQ / OMG). One load + one predicted-not-taken branch per back-edge; the slow path services the trap under a probe so non-termination async traps (NeedStopTheWorld / NeedWatchdogCheck / NeedDebuggerBreak) resume the loop with all live state preserved, and termination unwinds via
ExceptionType::Termination. That PR is now rebased onto fork main78d45d3184(one commit ahead of the171babe26cthis repo currently pins), so the bump stays a single commit.This PR bumps
WEBKIT_VERSIONto the oven-sh/WebKit#372 preview build (autobuild-preview-pr-372-a50d1e50, full 38-asset matrix published) and addstest/js/web/workers/worker-terminate-wasm-loop.test.ts, which spawns three workers (JS loop, Wasm loop calling a JS import, pure Wasm loop) and assertsterminate()resolves with{terminated: true, code: 1, exited: 1}for each.Verification
Built bun main (
9008ae7ab) against the rebased WebKit branch as a local WebKit (linux x64, debug+ASAN):worker.terminate()on the pure-Wasm loop resolves (exit event fires, process exits); unpatched hangs foreverjs/wasmcall/wasm)process.exit(5): exits cleanly 3/3; on current main this never exitswasm-streaming.test.ts(33) andbun-build-compile-wasm.test.tspass;worker.test.tsshows the same 3 timing-sensitive failures as an unpatched debug build of the same tree (pre-existing)Merge sequencing: (1) oven-sh/WebKit#372 merges, (2) repin
WEBKIT_VERSIONhere to the merged 40-hex sha, (3) merge this PR. The preview pin is CI-only.Note: the fix is entirely in the vendored WebKit (via
WEBKIT_VERSION), so the src-stash fail-before check is not mechanically applicable here;USE_SYSTEM_BUN=1 bun teston the new test file demonstrates the fail-before (wasm -> HUNG).[decide:webkit] gate passed · iteration 2 · 2 files touched
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 2
evidence per changed file