Upgrade WebKit to 5488984d: fix require(ESM) diamond-dep deadlock#30527
Merged
Conversation
…0493, #30281) Bumps WebKit to oven-sh/WebKit#225 (rebased #217 — adds the modulePromise->status() != Pending guard to moduleRegistryModuleSettled). require() of an ESM module whose graph contained a diamond dependency through a barrel deadlocked (release) / aborted on `m_status == Fetching` (debug). Regressed by the require(esm) sync-replay path; root cause was moduleRegistryModuleSettled firing twice for the same entry when hostLoadImportedModule's synchronous-replay branch had already driven fetchComplete inline while a stale normal-queue reaction was pending. WEBKIT_VERSION temporarily points at the preview-PR autobuild tag so CI can validate against the patched WebKit before #225 merges; will switch to the real sha before this lands. Fixes #30493 Fixes #30281
Collaborator
|
Updated 8:15 PM PT - May 11th, 2026
❌ @robobun, your commit 95258bf has 1 failures in
🧪 To try this PR locally: bunx bun-pr 30527That installs a local version of the PR into your bun-30527 --bun |
Contributor
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
Contributor
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
alii
changed the title
Contributor
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughBumps the WebKit dependency commit hash used by the build script and adds a regression test that spawns Bun to require() an ESM diamond dependency graph, asserting the process completes successfully and produces the expected output without assertion failures. ChangesFix require() deadlock in diamond dependency graph
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 `@test/regression/issue/30493.test.ts`:
- Around line 44-48: Remove the in-test watchdog: delete the
KILL_SIGNAL/KILL_AFTER_MS constants, the setTimeout that assigns to killer, the
proc.kill(...) call, and the clearTimeout(killer); instead rely on awaiting the
process condition with the existing Promise.all([proc.stdout.text(),
proc.stderr.text(), proc.exited]) and the test runner's timeout; specifically
remove the lines that create/clear killer and the call to proc.kill so the test
no longer uses a timing-based watchdog.
- Line 51: Remove the panic-style negative assertion
expect(stderr).not.toContain("ASSERTION FAILED") from the test; locate the
assertion in test/regression/issue/30493.test.ts (the exact statement
expect(stderr).not.toContain("ASSERTION FAILED")) and delete it so the test
relies on existing exit/signal or explicit failure checks instead of checking
for absence of a panic string.
🪄 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: c10f1f76-18b1-4427-a58a-e37d87126539
📒 Files selected for processing (2)
scripts/build/deps/webkit.tstest/regression/issue/30493.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Member
Author
|
@robobun adopt |
Collaborator
|
✅ Adopted — diff is green, ready to merge modulo a pre-existing Windows flake. What changed since
CI (#53572 → retrigger → #53577):
Needs a maintainer to merge past the Windows flake. |
- Replace manual setTimeout/clearTimeout with Bun.spawn's native timeout option (matches spawn-pipe-read-error-leak.test.ts pattern). - Drop the negative 'ASSERTION FAILED' stderr check; stdout snapshot + signalCode + exitCode cover both the abort and deadlock failure modes. - Fix stale comment: 30281.test.ts was never landed (this test subsumes the react+MUI repro from #30283), and the fix is WebKit#225 not #217.
Contributor
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/regression/issue/30493.test.ts`:
- Around line 18-51: The test uses a 10_000ms subprocess timeout (see Bun.spawn
timeout: 10_000) but relies on the test runner's default (5_000ms), so add an
explicit test timeout greater than the subprocess timeout so the test process
can finish and assertions on proc.signalCode/exitCode run; update the test
declaration (the test("require() of ESM with diamond dependency through barrel
does not deadlock", ...)) to specify a larger timeout (e.g. 20_000) or call the
global runner timeout helper (e.g. jest.setTimeout(20000)) before spawning the
subprocess.
🪄 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: c074803a-cc06-4ac9-a5ed-9f1ba0651f4a
📒 Files selected for processing (1)
test/regression/issue/30493.test.ts
Local `bun test` defaults to 5s per-test, which would mask the 10s subprocess timeout in the (future-regression) deadlock case. CI already passes --timeout=90000 so this only affects local runs.
alii
added a commit
that referenced
this pull request
May 12, 2026
… module no longer hangs (#30537) A second `await import()` of a path whose first import threw used to hang forever instead of re-throwing. Two reported variants: - **#23139**: same path twice - **#22743**: path X, then a *different* error path Y, then X again (true regression, 1.2.20 → 1.2.21) While verifying #30527 I checked whether oven-sh/WebKit#225 covered these — turns out both were **already fixed** before that bump (main + WebKit `88b2f7a2` doesn't hang either). The fix landed somewhere in the module-loader rewrite window (#29393 → #30262); the issues just never got re-tested. | build | #23139 | #22743 | |---|---|---| | 1.3.13 | hangs | hangs | | main + WebKit `88b2f7a2` (pre-#30527) | ✅ | ✅ | | main + WebKit `5488984d` | ✅ | ✅ | Both tests fail on `USE_SYSTEM_BUN=1` (stdout missing the post-hang lines, killed by spawn timeout) and pass on `bun bd test`. Fixes #23139 Fixes #22743
This was referenced May 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
WebKit changes (88b2f7a2 → 5488984d)
Single commit on top of the previous pin:
module-loader: don't double-fire moduleRegistryModuleSettled after inline sync replay(oven-sh/WebKit#225, rebased #217)File:
Source/JavaScriptCore/runtime/JSMicrotask.cppWhat: Adds a
modulePromise->status() != Pendingearly-return guard tomoduleRegistryModuleSettled, symmetric with the guard already present inmoduleRegistryFetchSettled. Gated under#if USE(BUN_JSC_ADDITIONS).Why:
require()of an ESM whose graph contained a diamond dependency through a barrel deadlocked (release) / aborted onASSERTION FAILED: m_status == Status::Fetching(debug).hostLoadImportedModule's synchronous-replay branch (taken whenrequire(esm)is draining the synchronous module queue) callsfetchComplete+ fulfillsmodulePromiseinline. If aModuleRegistryFetchSettledreaction had already run on the normal microtask queue for the same entry before sync mode was entered, it left a staleModuleRegistryModuleSettledreaction queued there. When the normal queue later drained, that reaction re-enteredfetchCompleteon an already-Fetchedentry.No changes to
JSType.h. No WebCore code-generator changes.Verification:
test/regression/issue/30493.test.tsfails on currentmain(assertion crash, empty stdout)bun run build:localwith the patched WebKitbun bdwith the prebuilt preview tarballautobuild-preview-pr-225-2b6b1c39(build #53556 — 67 pass, 3 pre-existing main flakes also red on publish: add --provenance (npm provenance via Sigstore keyless signing) #30522)Fixes #30493
Fixes #30281
Closes #30283 (the dependency-free 6-file repro in this PR covers the same root cause without needing a react+MUI install)