module-loader: resume suspended TLA body when a cycle sibling throws - #33961
module-loader: resume suspended TLA body when a cycle sibling throws#33961robobun wants to merge 1 commit into
Conversation
WalkthroughChangesUpdates the default WebKit preview version and adds three regression tests for top-level-await import cycles involving sibling evaluation errors. WebKit version update
Top-level-await cycle regressions
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Re #30651: that repro does have the cycle ( |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find issues in the tests, but deferring to a human: the actual fix lives in oven-sh/WebKit#281 (JSC's JSModuleRecord::evaluate() — core module-evaluation semantics), and WEBKIT_VERSION is currently pinned to that PR's preview-build tag pending its merge.
What was reviewed:
- The three new tests follow the existing patterns in this file (tempDir, concurrent pipe drain, exit-code asserted last) and assert exact log ordering /
flagvalues. - Checked that
prebuiltUrl()/prebuiltDestDir()already handleautobuild--prefixed version strings, so the preview tag resolves correctly. - Confirmed the tests exercise distinct variants (plain resume, try/finally, multiple awaits) rather than duplicating one repro.
Extended reasoning...
Overview
This PR pairs with oven-sh/WebKit#281. In this repo it changes two files: scripts/build/deps/webkit.ts bumps WEBKIT_VERSION from a commit sha to the preview-build tag autobuild-preview-pr-281-0e491dad, and test/js/bun/resolve/dynamic-import-tla-cycle.test.ts gains three regression tests for the TLA-cycle-sibling-throw scenario. The behavioral fix itself — gating the evaluationError() early-return in JSModuleRecord::evaluate() on the generator still being in State::Init — lives entirely in the WebKit fork.
Security risks
None identified. The bun-repo diff is a version-string change plus test fixtures that spawn bunExe() in a temp dir. No auth, crypto, network, or untrusted-input handling is touched here.
Level of scrutiny
High, and outside what I can sign off on. The substantive change modifies JavaScriptCore's cyclic-module evaluation / async-module resume path, which is spec-defined (CyclicModuleRecord::Evaluate, AsyncModuleExecutionFulfilled/Rejected, AsyncBlockStart) and historically subtle — several existing tests in this same file guard adjacent edge cases. Whether skipping the evaluationError() re-throw for a suspended body is correct in all callers of JSModuleRecord::evaluate() (not just the resume microtask) needs a human who can read the WebKit-side diff in context.
Other factors
- The PR description explicitly states
WEBKIT_VERSIONis a temporary preview pin and will be switched to the merged commit hash once oven-sh/WebKit#281 lands — so this PR is not yet in its final form by the author's own account. - Per CLAUDE.md's "Dependencies & vendoring" guidance, WebKit bumps warrant explicit review.
- The tests themselves look solid: they use
tempDir/bunEnv/bunExe, drain stdout/stderr/exited concurrently, assert combined{out, stderr}objects before the exit code, and the author verified fail-before viaUSE_SYSTEM_BUN=1. I did note the twoexpect(stderr).toBe("")lines in the third test are stricter than the "never assert stderr is exactly empty" guidance, but the first two tests wrap stderr into the combined object which is the recommended shape — minor and consistent with the rest of this file.
A top-level-await module that is suspended at its await inside an import
cycle was never resumed if a sibling module threw during the same
Evaluate() pass: everything after the await (finally blocks, export
initialisers) was silently skipped while the process continued. Node and
the spec run the continuation.
The resume microtask re-enters JSModuleRecord::evaluate(), which took the
evaluationError() early-return once the cycle root had stamped the
sibling's error onto the suspended module. The WebKit change gates that
early-return on the generator State still being Init; a suspended body
proceeds to executeModuleProgram and runs to completion, and
asyncExecution{Fulfilled,Rejected} already no-op when status is Evaluated.
WebKit change: oven-sh/WebKit#281 (autobuild-preview-pr-281-0e491dad)
f72406c to
4a61e27
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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`:
- Line 10: Update WEBKIT_VERSION to a permanent merged-main commit SHA or stable
release tag instead of the temporary autobuild-preview-pr-281-3a40c5b6 value,
preserving it as the default prebuilt WebKit source.
🪄 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: 0a24c74c-cb06-4301-9438-2d6d6b727913
📒 Files selected for processing (2)
scripts/build/deps/webkit.tstest/js/bun/resolve/dynamic-import-tla-cycle.test.ts
There was a problem hiding this comment.
No issues found in the tests or the version-bump plumbing, but the actual fix here is a change to JSC's module evaluation semantics shipped via a WebKit bump — and WEBKIT_VERSION is currently pinned to an unmerged preview-build tag that the description says will be swapped for the merged hash. That's a two-repo coordination on engine-critical behavior, so leaving this for a human to sign off.
What was reviewed:
- The three new tests follow the existing patterns in this file (tempDir, concurrent pipe drain, exit-code-last) and assert exact log ordering.
prebuiltUrl()/prebuiltDestDir()already handleautobuild--prefixed versions, so the preview tag resolves correctly.- Checked that the new tests don't rely on
stderrbeing exactly empty in a way that would break under debug/ASAN — they assert it as part of a combined object, matching the file's existing convention.
Extended reasoning...
Overview
This PR adds three regression tests to test/js/bun/resolve/dynamic-import-tla-cycle.test.ts covering a TLA module suspended inside an import cycle whose sibling throws, and bumps WEBKIT_VERSION in scripts/build/deps/webkit.ts to a preview-build tag from oven-sh/WebKit#281. The behavioral fix lives entirely in the WebKit prebuilt — this repo's diff is version pin + test coverage only.
Security risks
None. No user-input parsing, auth, crypto, or filesystem-path handling is touched. The tests spawn hermetic subprocesses in temp dirs.
Level of scrutiny
High. Although the in-repo diff is small and mechanical, the change it pulls in modifies JSModuleRecord::evaluate() — the resume path for top-level-await module bodies — which is spec-sensitive and affects every ESM program with TLA + cycles + errors. The PR description's analysis is careful and the fix is narrowly gated on generator State::Init, but this is exactly the kind of engine-semantics change a maintainer should look at directly (and likely wants to review alongside the WebKit-side PR).
Other factors
- The PR description explicitly states
WEBKIT_VERSIONis temporarily pinned to a preview tag and will be updated once oven-sh/WebKit#281 merges — so the PR is not yet in its final mergeable state by the author's own account. - WebKit version bumps are a vendored-dependency change; per the repo's review guidance, dependency bumps warrant human review.
- The tests themselves look correct and hermetic; the ruled-out finding (preview tag being unmerged) is real as a process concern but not a code bug — the build machinery already handles
autobuild-tags.
1 participant
Repro
A top-level-await module that is suspended at its
awaitinside an import cycle is never resumed if a sibling module throws during the sameEvaluate()pass. Everything after theawait(finally blocks, export initialisers, resource registration) is silently skipped while the process continues; itstry { await } catch {}catches nothing. Node and the spec run the continuation.Cause
While
bis suspended atawait 0it is still on theInnerModuleEvaluationstack with statusEvaluating(its SCC has not been popped yet because it cycles througha). Whencthrows, step 9 ofCyclicModuleRecord::evaluate()stamps the error onto every stack member, includingb, asevaluationErrorand transitions them toEvaluated.When
b's resume microtask (AsyncModuleExecutionResume) later fires, it re-entersJSModuleRecord::evaluate(), which seesevaluationError()set and re-throws it instead of resuming the body. Per spec the body runs as an independent async context (AsyncBlockStart);AsyncModuleExecutionFulfilled/AsyncModuleExecutionRejectedalready handle the "status is alreadyEvaluated" case afterwards (step 1 of each).Fix
oven-sh/WebKit#281 gates the
evaluationError()early-return on the generatorStatestill beingInit(body not yet started). A suspended body (State > 0) proceeds toexecuteModuleProgramand runs to completion.Verification
USE_SYSTEM_BUN=1 bun test test/js/bun/resolve/dynamic-import-tla-cycle.test.ts→ 3 new tests fail (flag: "started", missingb:after-await/b:end,finallynot run)bun run build:local test test/js/bun/resolve/dynamic-import-tla-cycle.test.tswith JSModuleRecord::evaluate: resume suspended TLA body when a cycle sibling throws WebKit#281 applied → 9/9 pass, output matches Node exactlyBUN_JSC_validateExceptionChecks=1passestest/js/bun/resolve/(282 tests),test/regression/issue/{24387,30887,32178}.test.ts,require-esm-transitive-tla.test.tsall passWEBKIT_VERSIONis pinned toautobuild-preview-pr-281-3a40c5b6(oven-sh/WebKit#281 rebased onto4895f45dso it sits on top of #34009's allocator changes). Once that PR merges I will switch this to the merged commit hash.