innerModuleEvaluation: only skip async-dep wait when cycle root has executed#202
Merged
Merged
Conversation
…xecuted The depWasAlreadyEvaluatingAsync skip (added for the dynamic-import-re- enters-its-own-TLA-awaiter deadlock) also fired for sibling static imports in the same Evaluate() pass: an earlier sibling pops an SCC to EvaluatingAsync, then a later sibling that imports an SCC member sees status==EvaluatingAsync, skips the wait, and runs with the SCC's bindings still in TDZ. Narrow the skip to require the cycle root's pendingAsyncDependencies == 0, i.e. its body has been entered (the Nitro case, where bindings before the await are initialised). For an SCC still queued behind an async dep the count is > 0, so the spec-mandated wait is preserved.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughUpdated TLA cycle handling in Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Jarred-Sumner
added a commit
to oven-sh/bun
that referenced
this pull request
Apr 27, 2026
Adds two cases to dynamic-import-tla-cycle.test.ts: - static sibling import waits for an async-pending SCC: entry imports A then B; A↔C cycle; C imports a HasTLA module; B reads a binding from C at top-level. Works in Node and 1.3.13, TDZ's on main since #29393 because the depWasAlreadyEvaluatingAsync skip in innerModuleEvaluation fires for sibling static imports in the same Evaluate() pass. - non-entry TLA self-import: same Nitro deadlock pattern as the existing test, but the awaiting module is a static dep of the entry rather than the entry itself. Locks in why the discriminator must be pendingAsyncDependencies==0 (cycle root's body has been entered), not topLevelCapability (only set on the Evaluate() entry). The static-sibling test fails until WEBKIT_VERSION includes oven-sh/WebKit#202.
Jarred-Sumner
added a commit
to oven-sh/bun
that referenced
this pull request
Apr 27, 2026
`fetch-cli.ts` is imported by `source.ts`/`zig.ts` as a library (for
`fetchCliPath`) and also runs as a CLI. The guarded `await main()` marks
the module `HasTLA`, which forces every importer — and the
`{config,webkit,flags,source}` cycle — onto the spec's async-evaluation
path for code that's dead on import. Replace with `main().catch(...)` so
the module stays sync when imported.
This is the immediate trigger for the `ReferenceError: Cannot access
'webkit' before initialization` crash a freshly-built bun hits running
`scripts/build.ts`, which several open `farm/*` branches (#29725,
#29731, #29733, #29749, #29756, #28512) each work around by relocating
`WEBKIT_VERSION`. Supersedes the `scripts/build/` portions of those.
The underlying loader regression (the `depWasAlreadyEvaluatingAsync`
skip in `innerModuleEvaluation` over-firing for sibling static imports)
is fixed in oven-sh/WebKit#202; tests + `WEBKIT_VERSION` bump are in
#29770. This change is independently correct and lands first so the farm
stops thrashing.
Verified: `build/debug/bun-debug scripts/build.ts --help` (was crashing,
now works), `fetch-cli.ts` CLI usage and BuildError/non-BuildError exit
codes unchanged.
Preview Builds
|
This was referenced Apr 27, 2026
Jarred-Sumner
added a commit
to oven-sh/bun
that referenced
this pull request
Apr 27, 2026
Narrows the depWasAlreadyEvaluatingAsync skip in innerModuleEvaluation to require the cycle root's pendingAsyncDependencies == 0, so sibling static imports in the same Evaluate() pass wait for an async-pending SCC instead of running with TDZ bindings.
xhjkl
pushed a commit
to xhjkl/bun
that referenced
this pull request
May 14, 2026
`fetch-cli.ts` is imported by `source.ts`/`zig.ts` as a library (for
`fetchCliPath`) and also runs as a CLI. The guarded `await main()` marks
the module `HasTLA`, which forces every importer — and the
`{config,webkit,flags,source}` cycle — onto the spec's async-evaluation
path for code that's dead on import. Replace with `main().catch(...)` so
the module stays sync when imported.
This is the immediate trigger for the `ReferenceError: Cannot access
'webkit' before initialization` crash a freshly-built bun hits running
`scripts/build.ts`, which several open `farm/*` branches (oven-sh#29725,
oven-sh#29731, oven-sh#29733, oven-sh#29749, oven-sh#29756, oven-sh#28512) each work around by relocating
`WEBKIT_VERSION`. Supersedes the `scripts/build/` portions of those.
The underlying loader regression (the `depWasAlreadyEvaluatingAsync`
skip in `innerModuleEvaluation` over-firing for sibling static imports)
is fixed in oven-sh/WebKit#202; tests + `WEBKIT_VERSION` bump are in
oven-sh#29770. This change is independently correct and lands first so the farm
stops thrashing.
Verified: `build/debug/bun-debug scripts/build.ts --help` (was crashing,
now works), `fetch-cli.ts` CLI usage and BuildError/non-BuildError exit
codes unchanged.
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
1 participant
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.
The
depWasAlreadyEvaluatingAsyncskip (added in 18d48e3 for the dynamic-import-re-enters-its-own-TLA-awaiter deadlock) also fires for sibling static imports in the sameEvaluate()pass: an earlier sibling pops an SCC toEvaluatingAsync, then a later sibling that imports an SCC member seesstatus == EvaluatingAsync, skips the wait, and runs with the SCC's bindings still in TDZ.Minimal repro (works in Node and pre-rewrite Bun, TDZ's after #199):
When entry processes A, the {A,C} SCC pops to
EvaluatingAsync. When entry then processes B→C, the patch sees C alreadyEvaluatingAsyncand makes B skip the wait — B executes synchronously while C's bindings are TDZ.Fix: narrow the skip to also require
cyclic->pendingAsyncDependencies() == 0— i.e. the cycle root'sExecuteModule/ExecuteAsyncModulehas been called and its bindings before the first await are initialised. For an SCC still queued behind an async dep the count is> 0, so the spec-mandated wait is preserved. For the Nitro deadlock case (entry or non-entry), the re-entered module's body is mid-await with count== 0, so the skip still fires.Bun-side test coverage (3 cases) is in the paired bun PR.