Fix dynamic import() resolving before TLA module finishes evaluating#29224
Fix dynamic import() resolving before TLA module finishes evaluating#29224
Conversation
When a module with top-level await is dynamically import()-ed a second time while the first import's evaluation is still pending, Bun resolved the second promise before the module finished evaluating, causing .then handlers to run in the wrong order. The stock WebKit `requestImportModule` builtin sets `entry.evaluated` synchronously at the start of `asyncModuleEvaluation`, then has a fast path that returns the namespace as soon as `entry.evaluated && entry.module`. A second import reaching that fast path skipped awaiting the in-flight TLA and fulfilled its promise early. Fix: install an override `requestImportModule` builtin on the JSModuleLoader at global-object init time. The override caches the evaluation promise on the registry entry and awaits it on concurrent imports, matching the ECMA-262 ContinueDynamicImport semantics and Node/Deno behaviour. Closes #29221
|
Updated 1:05 PM PT - Apr 12th, 2026
❌ @robobun, your commit 7f50881 has some failures in 🧪 To try this PR locally: bunx bun-pr 29224That installs a local version of the PR into your bun-29224 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a builtin override for Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR touches JSC's module loading critical path (async TLA evaluation) with non-trivial concurrency logic — deferring to a human reviewer.
Extended reasoning...
Overview
The PR installs a custom requestImportModule builtin on JSModuleLoader via ZigGlobalObject::addBuiltinGlobals to fix a spec-compliance bug where concurrent dynamic import() calls for the same TLA module could resolve before evaluation completed. It introduces ModuleLoaderOverrides.ts (new builtin), modifies ZigGlobalObject.cpp, and adds a regression test.
Security Risks
No direct security risks. The change is limited to module evaluation ordering semantics — it cannot expose private data, bypass auth, or allow injection. The entry.evaluatingPromise field is a locally-scoped cache on the registry entry.
Level of Scrutiny
High. This is a subtle concurrency fix in the module loading hot path. Correctness depends on precise knowledge of JSC's async module evaluation lifecycle (when entry.evaluated and entry.module are set, how asyncModuleEvaluation interacts with the promise cache, etc.). The logic is well-commented and the test reproduces the exact regression, but the interaction with the C++ JSModuleLoader::requestImportModule dispatch mechanism deserves a review by someone familiar with JSC internals.
Other Factors
A nit-level dead-code block (lines 61–65 of ModuleLoaderOverrides.ts) was flagged by the bug hunter — the third evaluatingPromise check is unreachable because Fast Path 2 already covers that scenario. This has no runtime impact but warrants cleanup. The overall fix direction looks correct, and a regression test is included. The PR should be reviewed by someone with JSC/Bun module loader expertise before merge.
- ModuleLoaderOverrides: swap duck-typed `.then` check for $isPromise so `delete Promise.prototype.then` can't defeat the fix, matching the pattern in CommonJS.ts loadEsmIntoCjs - ModuleLoaderOverrides: drop the unreachable mid-path `evaluatingPromise` check — Fast Path 2 already handles that scenario since `entry.evaluated`/`entry.module` are set synchronously inside `linkAndEvaluateModule` - test: use `cwd` + relative path instead of concatenated `/entry.mjs`, split combined toEqual into separate stdout-first + exitCode assertions - test: add second test case covering the TDZ scenario from #20489 and #22367 (concurrent dynamic imports seeing uninitialized bindings)
- Correct 'private-name' → 'public-name' in the comments describing how JSC's C++ `JSModuleLoader::requestImportModule` looks up the builtin. The code already uses `requestImportModulePublicName()`; the old wording was misleading. - Assert test 1's stderr is empty (filtering the ASAN warning WebKit unconditionally prints from Options.cpp in ASAN builds) instead of silently discarding it. Use the same canonical helper for test 2.
![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 the current code and only fix it if needed.
Inline comments:
In `@src/js/builtins/ModuleLoaderOverrides.ts`:
- Line 51: The code in ModuleLoaderOverrides constructs a public Set for
internal tracking (new Set()) when calling requestSatisfy; replace that with the
intrinsic private global $Set to avoid user-tampering in builtin paths. Update
the call site in requestSatisfy invocation (entry = await
this.requestSatisfy(entry, parameters, fetcher, new Set());) to pass a new
$Set() instead, ensuring you reference the built-in private global $Set (not a
local variable) so internal tracking uses the private intrinsic.
In `@test/regression/issue/29221.test.ts`:
- Around line 21-30: Replace brittle assertions that a spawned subprocess
produced empty stderr (e.g. checks comparing cleanStderr(stderr) === "" or
stderr === "") with assertions on the subprocess exitCode to detect
crashes/failures; keep the cleanStderr function and only assert on stderr
contents when the test is explicitly validating an error message. Update the
test cases that call cleanStderr (and any assertions near where cleanStderr is
used) to assert exitCode === 0 (or the expected non-zero code) instead of empty
stderr, and only use cleanStderr(...) comparisons when validating specific error
text.
🪄 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: 1a9b6ee1-1abe-4a93-b1ef-09f8a8619775
📒 Files selected for processing (3)
src/bun.js/bindings/ZigGlobalObject.cppsrc/js/builtins/ModuleLoaderOverrides.tstest/regression/issue/29221.test.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/regression/issue/29221.test.ts (1)
65-66:⚠️ Potential issue | 🟡 MinorRemove brittle empty-
stderrsuccess assertions.At Line 65 and Line 108, asserting
cleanStderr(stderr) === ""is still flaky for spawned subprocess tests. KeepexitCodeas the success signal, and only assert stderr when validating specific expected error output.Proposed change
- expect(cleanStderr(stderr)).toBe(""); expect(exitCode).toBe(0); @@ - expect(cleanStderr(stderr)).toBe(""); expect(exitCode).toBe(0);Based on learnings: in
test/regression/issue/*.test.ts, avoid empty-stderrassertions for spawned Bun subprocesses (ASAN/CI can emit benign stderr); useexitCodeas the primary crash/failure signal.Also applies to: 108-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/regression/issue/29221.test.ts` around lines 65 - 66, Remove the brittle assertions that require cleanStderr(stderr) to be exactly "" in the test (instances calling expect(cleanStderr(stderr)).toBe("") in test/regression/issue/29221.test.ts), and rely on expect(exitCode).toBe(0) as the primary success signal; leave or add stderr assertions only where a specific error message is being validated (i.e., replace or delete the expect(cleanStderr(stderr)).toBe("") lines around the exits at the two locations and keep the existing expect(exitCode).toBe(0) checks).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/regression/issue/29221.test.ts`:
- Around line 65-66: Remove the brittle assertions that require
cleanStderr(stderr) to be exactly "" in the test (instances calling
expect(cleanStderr(stderr)).toBe("") in test/regression/issue/29221.test.ts),
and rely on expect(exitCode).toBe(0) as the primary success signal; leave or add
stderr assertions only where a specific error message is being validated (i.e.,
replace or delete the expect(cleanStderr(stderr)).toBe("") lines around the
exits at the two locations and keep the existing expect(exitCode).toBe(0)
checks).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ca284a1e-e3d1-4fb0-a47d-d7bb216e08ce
📒 Files selected for processing (1)
test/regression/issue/29221.test.ts
…t tests - ModuleLoaderOverrides: replace `new Set()` with `new $Set()` so the visited-set used by `requestSatisfy` is tamper-proof to user-modified `globalThis.Set`. - test: drop the brittle `cleanStderr(stderr) === ""` assertions and the helper entirely. Rely on `exitCode === 0` as the crash/failure signal; test 2 still asserts the specific TDZ error string (#20489/#22367's symptom) doesn't appear on stderr. - test: switch both cases to `test.concurrent` — they spawn independent subprocesses against separate tempDirs with no shared state, so there's no reason to serialize them.
The override was installed with only `Builtin | DontEnum`, which meant a bug in a Bun builtin (the module loader is reachable from any builtin via the `$Loader` private name) could silently overwrite or delete the property and revert to the stock WebKit `requestImportModule`, reintroducing the TLA race. Match the defensive pattern used for other safety-critical builtins in `addBuiltinGlobals` by adding `DontDelete` and `ReadOnly`. JSC's C++ `JSModuleLoader::requestImportModule` uses `get()` to look up the function every call, which works unchanged on ReadOnly properties.
There was a problem hiding this comment.
All previously flagged issues have been addressed — for tamper-proofing, $Set for builtin safety, DontDelete+ReadOnly lock-down, dead-code removal, accurate public-name comments, test.concurrent, and the cwd pattern. LGTM.
Extended reasoning...
Overview
This PR adds a requestImportModule override on the JSModuleLoader to fix a TLA (top-level await) race condition. The override caches the evaluation promise on entry.evaluatingPromise so concurrent dynamic import() calls await it rather than taking an early-return fast path. Three files are changed: the C++ install site (ZigGlobalObject.cpp), the new builtin (ModuleLoaderOverrides.ts), and regression tests (29221.test.ts).
Security risks
No direct security risks. The module loader override is installed with DontDelete | ReadOnly so internal builtins cannot accidentally overwrite it. The tamper-proof $isPromise intrinsic is used instead of duck-typing, preventing delete Promise.prototype.then from bypassing the fix.
Level of scrutiny
This touches a core module-loading code path; high scrutiny is warranted. Multiple rounds of review have gone through the code, and all flagged issues (🔴 $isPromise, 🟡 $Set, 🟡 DontDelete+ReadOnly, 🟡 dead code, 🟡 comment accuracy, 🟡 test conventions) have been addressed in subsequent commits.
Other factors
The fix is spec-aligned (mirrors [[TopLevelCapability]] semantics). Test coverage is solid with two end-to-end subprocess tests exercising both the ordering and TDZ invariants. The remaining nit (Test 1 silently discarding stderr) does not affect correctness or CI signal.
No-op comment tweak in ModuleLoaderOverrides.ts — `finishCreation` was imprecise; the `putDirectBuiltinFunction` call lives in `GlobalObject::addBuiltinGlobals`.
There was a problem hiding this comment.
LGTM — all prior feedback addressed.
Extended reasoning...
Overview
This PR installs a requestImportModule builtin override on the JSModuleLoader at GlobalObject init time to fix a TLA race condition: the stock WebKit builtin sets entry.evaluated = true synchronously at the start of async evaluation, so a concurrent import()\ in the same tick took a fast-path that returned the namespace before TLA settled. The fix caches the in-flight evaluation promise on entry.evaluatingPromise` and awaits it before handing the namespace to any caller.
Security risks
No security risks. The override is installed before any user code runs and is locked down with DontDelete | ReadOnly so it cannot be removed by builtins or user code. $isPromise (tamper-proof intrinsic) is used to detect the evaluation promise.
Level of scrutiny
Moderate. Module loading is a core runtime path, but the change is well-scoped — a single new builtin file plus a one-line registration in addBuiltinGlobals. The two-test regression suite directly reproduces both failure modes (#29221 ordering bug and #20489/#22367 TDZ bug). All feedback from prior rounds has been incorporated.
Other factors
- Dead branch removed (commit 8106ce2)
typeof .thenduck-type replaced with$isPromiseintrinsicnew Set()replaced withnew $Set()DontDelete | ReadOnlyattributes added to the property installation- Comments corrected from private-name to public-name
- Tests use
test.concurrent,cwdspawn pattern, and separate stdout/exitCode assertions - No bugs found by the automated bug hunting system
|
Build #45387: darwin-{13,14}-aarch64-test-bun both expired (no runner available); the same two jobs expired on #29233, #29231, #29229, #29228 — fleet-wide macOS aarch64 runner starvation. The darwin x64 test-bun jobs (macOS 13 + 14) passed in 19–25 min, every build job on every platform passed, and every other test-bun job (linux, windows, alpine, debian, ubuntu) passed. Code is fine; just waiting on macOS aarch64 runner capacity. |
1 participant
Repro
Node/Deno print
1 2 3 4. Bun printed1 4 2 3— both.then()handlers fired beforetemp2.mjs's top-level await settled, and the second handler ran before the first. Users of #20489 and #22367 see the same root cause surface asCannot access 'x' before initializationwhen a concurrentimport()reads a named export before the TLA completes.Cause
The stock WebKit
ModuleLoader.jsbuiltin'srequestImportModule:For an async (TLA) module,
moduleEvaluationsetsentry.evaluated = truesynchronously at the start ofasyncModuleEvaluationand then awaits the TLA. A second dynamicimport()of the same module in the same tick hits that fast path, seesevaluated === true, and returns the namespace without awaiting the in-flight evaluation promise.The stock builtin never caches the evaluation promise anywhere on the entry, so there's nothing for a concurrent caller to await — this diverges from the spec's
[[TopLevelCapability]]behaviour.Fix
Since Bun ships prebuilt WebKit, patching the vendored
ModuleLoader.jsisn't enough — any release using the cached tarball would miss the fix. Instead, install an overriderequestImportModulebuiltin on theJSModuleLoaderatZig::GlobalObjectinit time.JSC's C++
JSModuleLoader::requestImportModulelooks the function up dynamically by its private name on the loader object every call, somoduleLoader->putDirectBuiltinFunction(..., requestImportModulePublicName(), ...)cleanly replaces both C++-initiated and JS-initiated dynamic imports.The override caches the evaluation promise on
entry.evaluatingPromisewhen it first kicks offlinkAndEvaluateModule, and on every fast path checks for and awaits that promise before handing back the namespace. Once evaluation settles the field is cleared. Uses$isPromise(the tamper-proof intrinsic) rather than a duck-typed.thencheck.Verification
test/regression/issue/29221.test.tshas two cases:.then()ordering — two concurrentimport()calls for a TLA module. Pre-fix:["tla-start","then-b","then-a","tla-end"]. Post-fix:["tla-start","tla-end","then-a","then-b"], matching Node and Deno.import()calls that readmod.arr.lengthandtypeof mod.fn. Pre-fix: imports 2..5 fail withCannot access 'arr' before initialization(reproducing Concurrent dynamic import with top level await trigger temporal dead zone #20489 / Dynamic Import fails when multiple Vite plugins import same file #22367). Post-fix: all five see fully initialized bindings.Fixes #29221
Fixes #20489
Fixes #22367