Fix dynamic import() resolving before TLA module finishes evaluating

src/bun.js/bindings/ZigGlobalObject.cpp

@@ -2711,6 +2711,19 @@ void GlobalObject::addBuiltinGlobals(JSC::VM& vm)
 
     putDirectBuiltinFunction(vm, this, builtinNames.overridableRequirePrivateName(), commonJSOverridableRequireCodeGenerator(vm), 0);
 
+    // Override the stock WebKit `requestImportModule` builtin on the
+    // JSModuleLoader with a version that correctly waits for an in-flight
+    // top-level-await evaluation before resolving. The stock builtin sets
+    // `entry.evaluated = true` synchronously at the start of async
+    // evaluation, which causes a concurrent dynamic `import()` of the same
+    // module to take a fast path and return the namespace before the TLA
+    // completes. See https://github.com/oven-sh/bun/issues/29221.
+    //
+    // The JSC C++ `JSModuleLoader::requestImportModule` looks this function
+    // up dynamically by its private-name property, so installing our version
+    // here replaces both C++-initiated and JS-initiated dynamic imports.
+    this->moduleLoader()->putDirectBuiltinFunction(vm, this, vm.propertyNames->builtinNames().requestImportModulePublicName(), moduleLoaderOverridesRequestImportModuleCodeGenerator(vm), PropertyAttribute::Builtin | PropertyAttribute::DontEnum);
+
     putDirectNativeFunction(vm, this, builtinNames.createUninitializedArrayBufferPrivateName(), 1, functionCreateUninitializedArrayBuffer, ImplementationVisibility::Public, NoIntrinsic, PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly);
     putDirectNativeFunction(vm, this, builtinNames.resolveSyncPrivateName(), 1, functionImportMeta__resolveSyncPrivate, ImplementationVisibility::Public, NoIntrinsic, PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly);
     putDirectNativeFunction(vm, this, builtinNames.createInternalModuleByIdPrivateName(), 1, InternalModuleRegistry::jsCreateInternalModuleById, ImplementationVisibility::Public, NoIntrinsic, PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly);

src/js/builtins/ModuleLoaderOverrides.ts

@@ -0,0 +1,83 @@
+// Override(s) installed on the JSC `JSModuleLoader` object at Zig::GlobalObject
+// construction. Each builtin in this file is installed by
+// ZigGlobalObject::finishCreation via `moduleLoader->putDirectBuiltinFunction`.
+//
+// Why this file exists: the stock WebKit builtin `ModuleLoader.js`'s
+// `requestImportModule` has a fast path that returns the module namespace
+// synchronously when `entry.evaluated` is set, even though for a top-level
+// await (TLA) module `entry.evaluated` is set *before* evaluation has
+// actually completed. As a result, a second dynamic `import()` of the same
+// module while the first is still mid-TLA resolves its promise before the
+// module finishes evaluating — breaking ECMA262 ContinueDynamicImport and
+// diverging from Node/Deno. See https://github.com/oven-sh/bun/issues/29221.
+//
+// Bun ships prebuilt WebKit, so patching the vendored `ModuleLoader.js`
+// isn't enough — we install this override on top of the existing builtin.
+// JSC's C++ `JSModuleLoader::requestImportModule` looks the function up
+// dynamically by its private-name property every time, so overriding the
+// property is sufficient for both C++-initiated and JS-initiated imports.
+//
+// The fix: cache the evaluation promise on the registry entry so concurrent
+// dynamic imports can `await` it instead of taking the early-return path.
+
+// `this` is the JSModuleLoader; `requestImportModule` is a builtin method.
+$visibility = "Private";
+export async function requestImportModule(
+  this: any,
+  moduleName: string,
+  referrer: unknown,
+  parameters: unknown,
+  fetcher: unknown,
+) {
+  "use strict";
+
+  const key = moduleName;
+  let entry = this.ensureRegistered(key);
+  let mod: unknown;
+
+  // Fast path 1: entry already present with a module record.
+  //
+  // If evaluation is still in flight (TLA), `entry.evaluatingPromise` holds
+  // the async evaluation promise — wait on it before handing the namespace
+  // back. This is the key fix for issue #29221.
+  if (entry.evaluated && (mod = entry.module)) {
+    if (entry.evaluatingPromise) {
+      await entry.evaluatingPromise;
+    }
+    return this.getModuleNamespaceObject(mod);
+  }
+
+  entry = await this.requestSatisfy(entry, parameters, fetcher, new Set());
+
+  // Fast path 2: another caller raced us through requestSatisfy and already
+  // finished (or is in the middle of) evaluating.
+  //
+  // `entry.evaluated` and `entry.module` are both set synchronously at the
+  // start of `linkAndEvaluateModule` below, so whenever `evaluatingPromise`
+  // is truthy `evaluated`/`module` are already set — this path is the only
+  // place concurrent TLA callers rendezvous.
+  if (entry.evaluated && (mod = entry.module)) {
+    if (entry.evaluatingPromise) {
+      await entry.evaluatingPromise;
+    }
+    return this.getModuleNamespaceObject(mod);
+  }
+
+  // First call to reach evaluation for this entry. `linkAndEvaluateModule`
+  // returns `moduleEvaluation(entry, fetcher)` directly, which for a TLA
+  // module is the promise returned by `asyncModuleEvaluation`. Cache it on
+  // the entry so any concurrent caller that slips through the fast paths
+  // can observe and await the same in-flight evaluation. Use the tamper-
+  // proof `$isPromise` intrinsic rather than a duck-typed `.then` check so
+  // `delete Promise.prototype.then` in user code can't defeat the fix.
+  const evalResult = this.linkAndEvaluateModule(entry.key, fetcher);
+  if (evalResult && $isPromise(evalResult)) {
+    entry.evaluatingPromise = evalResult;
+    try {
+      await evalResult;
+    } finally {
+      entry.evaluatingPromise = undefined;
+    }
+  }
+  return this.getModuleNamespaceObject(entry.module);
+}

test/regression/issue/29221.test.ts

@@ -0,0 +1,98 @@
+// https://github.com/oven-sh/bun/issues/29221
+// Also covers https://github.com/oven-sh/bun/issues/20489
+// and         https://github.com/oven-sh/bun/issues/22367
+//
+// Dynamic `import()` of a module with top-level await must not resolve its
+// promise before the module finishes evaluating. Repeated `import()` calls
+// for the same module share one evaluation — every `.then()` handler fires
+// AFTER the module's TLA settles, matching Node.js / Deno.
+//
+// Bug was in JSC's ModuleLoader.js builtin (`requestImportModule` /
+// `moduleEvaluation`): `entry.evaluated` was set synchronously at the start
+// of async evaluation, so a second `import()` in the same tick took a fast
+// path that returned the namespace without awaiting the pending TLA. The
+// visible symptoms were (a) `.then()` handlers firing in reversed order
+// (#29221) and (b) concurrent importers observing uninitialized bindings
+// ("Cannot access 'x' before initialization" — #20489, #22367).
+
+import { expect, test } from "bun:test";
+import { bunEnv, bunExe, tempDir } from "harness";
+
+test("dynamic import waits for top-level await to settle (#29221)", async () => {
+  using dir = tempDir("issue-29221", {
+    "entry.mjs": `
+globalThis.order = [];
+const a = import("./tla.mjs").then(() => globalThis.order.push("then-a"));
+const b = import("./tla.mjs").then(() => globalThis.order.push("then-b"));
+await Promise.all([a, b]);
+console.log(JSON.stringify(globalThis.order));
+`,
+    "tla.mjs": `
+globalThis.order.push("tla-start");
+await new Promise((r) => setTimeout(r, 50));
+globalThis.order.push("tla-end");
+`,
+  });
+
+  await using proc = Bun.spawn({
+    cmd: [bunExe(), "entry.mjs"],
+    cwd: String(dir),
+    env: bunEnv,
+    stdout: "pipe",
+    stderr: "pipe",
+  });
+  const [stdout, , exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+
+  // Expected ordering: the TLA module runs to completion first (tla-start
+  // then tla-end), then BOTH .then() handlers fire in import order.
+  //
+  // Pre-fix, Bun produced ["tla-start","then-b","then-a","tla-end"] — the
+  // second import's `.then()` fired before the TLA even resumed, because
+  // the JSC builtin's fast path returned the namespace without awaiting
+  // the pending evaluation promise.
+  expect(stdout.trim()).toBe(`["tla-start","tla-end","then-a","then-b"]`);
+  expect(exitCode).toBe(0);
+});
+
+test("concurrent dynamic imports of a TLA module all see initialized bindings (#20489, #22367)", async () => {
+  // Mirrors the reproduction from #20489: five concurrent `import()` calls
+  // for the same TLA module. Pre-fix, imports 2..5 took the fast path,
+  // resolved early, and saw the module's named exports still in the TDZ
+  // ("Cannot access 'x' before initialization"). After the fix, every
+  // import waits for the same in-flight evaluation and observes fully
+  // initialized bindings.
+  using dir = tempDir("issue-29221-concurrent", {
+    "entry.mjs": `
+const results = [];
+async function load(i) {
+  const mod = await import("./tla-exports.mjs");
+  // Touching both exports would throw TDZ pre-fix. Read them eagerly.
+  results.push([i, mod.arr.length, typeof mod.fn]);
+}
+await Promise.all([load(1), load(2), load(3), load(4), load(5)]);
+// Sort by import index so the assertion doesn't depend on resolution order.
+results.sort((a, b) => a[0] - b[0]);
+console.log(JSON.stringify(results));
+`,
+    "tla-exports.mjs": `
+// Yield across a microtask boundary so all five imports start before
+// this module's bindings are initialized.
+await new Promise((r) => setTimeout(r, 20));
+export const arr = [1, 2, 3];
+export function fn() { return "ok"; }
+`,
+  });
+
+  await using proc = Bun.spawn({
+    cmd: [bunExe(), "entry.mjs"],
+    cwd: String(dir),
+    env: bunEnv,
+    stdout: "pipe",
+    stderr: "pipe",
+  });
+  const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+
+  expect(stdout.trim()).toBe(`[[1,3,"function"],[2,3,"function"],[3,3,"function"],[4,3,"function"],[5,3,"function"]]`);
+  expect(stderr).not.toContain("before initialization");
+  expect(exitCode).toBe(0);
+});