Skip to content

moduleLoaderImportModule: thread referrer asyncEvaluationOrder for TLA self-deadlock skip #32437

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Jarred-Sumner merged 4 commits into from
Jun 17, 2026
Merged

moduleLoaderImportModule: thread referrer asyncEvaluationOrder for TLA self-deadlock skip #32437

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9e7e662
moduleLoaderImportModule: pass referrer asyncEvaluationOrder to JSC::…
Jarred-Sumner Jun 16, 2026
0e9e62c
moduleLoaderImportModule: include query string in referrer registry-k…
Jarred-Sumner Jun 16, 2026
cdd26eb
Upgrade WebKit to cd821fecca0d (oven-sh/WebKit#254)
Jarred-Sumner Jun 17, 2026
566cb13
Merge branch 'main' into claude/tla-referrer-async-order
Jarred-Sumner Jun 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion scripts/build/deps/webkit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
// -lto variants built with ThinLTO (per-module summaries for cross-language
// importing), and the Windows ICU data table filtered + per-item zstd
// compressed (lazily decompressed via bun_icu_decompress.cpp).
export const WEBKIT_VERSION = "09f04cd5a489b7c0b44aed255bfafce2a316eada";
export const WEBKIT_VERSION = "cd821fecca0d39c8bac874c283d956868c7f0de0";

/**
* WebKit (JavaScriptCore) — the JS engine.
Expand Down
40 changes: 23 additions & 17 deletions src/jsc/bindings/ZigGlobalObject.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3481,11 +3481,31 @@ JSC::JSPromise* GlobalObject::moduleLoaderImportModule(JSGlobalObject* jsGlobalO

auto moduleName = moduleNameValue->value(globalObject);
RETURN_IF_EXCEPTION(scope, nullptr);

auto sourceURL = sourceOrigin.url();
String sourceOriginStringHolder;
int64_t referrerAsyncOrder = -1;
if (sourceURL.isEmpty()) {
sourceOriginStringHolder = String("."_s);
} else if (sourceURL.protocolIsFile()) {
sourceOriginStringHolder = sourceURL.fileSystemPath();
auto query = sourceURL.queryWithLeadingQuestionMark();
auto referrerKey = query.isEmpty()
? JSC::Identifier::fromString(vm, sourceOriginStringHolder)
: JSC::Identifier::fromString(vm, makeString(sourceOriginStringHolder, query));
referrerAsyncOrder = globalObject->moduleLoader()->asyncEvaluationOrderForKey(referrerKey);
Comment on lines +3492 to +3496

@claude claude Bot Jun 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 The !query.isEmpty() branch added in bfdabd0 is unreachable: Bun's SourceOrigin for file modules is built from ResolvedSource.source_url, which is always set to path.text (the query-stripped filesystem path) and then run through WTF::URL::fileURLWithFileSystemPath(), so sourceURL.queryWithLeadingQuestionMark() is always empty. A referrer registered as /abs/wrapper.mjs?v=1 is therefore still looked up as /abs/wrapper.mjs, asyncEvaluationOrderForKey returns -1, and the #30634 fix doesn't apply to query-keyed TLA referrers — the CodeRabbit comment marked '✅ Addressed' isn't actually addressed. Not a regression (the no-query #30634 case is fixed), but consider either threading the query into ResolvedSource.source_url / the SourceOrigin, or dropping the dead branch.

Extended reasoning...

What the bug is

Commit bfdabd0 was added in response to the CodeRabbit inline comment: when the referrer module's registry key includes a query string (e.g. /abs/wrapper.mjs?v=1), the asyncEvaluationOrderForKey() lookup should include that query so it matches the registry entry. The fix reads sourceURL.queryWithLeadingQuestionMark() and, if non-empty, appends it to the filesystem path before the lookup.

The problem is that for file-protocol referrers in Bun, sourceURL never has a query component, so query.isEmpty() is always true and the makeString(...) branch is dead code. The CodeRabbit comment is marked "✅ Addressed in commit bfdabd0", but the query-keyed-referrer case it describes remains unfixed.

The code path

sourceURL here is sourceOrigin.url(). For file modules the SourceOrigin is constructed in ZigSourceProvider.cpp:

// ZigSourceProvider.cpp:89
auto sourceURLString = resolvedSource.source_url.toWTFString(BunString::ZeroCopy);
// ZigSourceProvider.cpp:48 (via toSourceOrigin)
return SourceOrigin(WTF::URL::fileURLWithFileSystemPath(sourceURL));

Every assignment of ResolvedSource.source_url in the loaders — ModuleLoader.zig:109,348,362,371,380,392,410,445,588,..., RuntimeTranspilerStore.rs:556, AsyncModule.zig:731, VirtualMachine.zig:1596,1610 — sets it to the content of path.text via input_specifier.createIfDifferent(path.text) (or String.init(path.text)). createIfDifferent (string.zig:117-125) returns other.dupeRef() when other equals utf8_slice, else cloneUTF8(utf8_slice) — i.e. its result is always semantically equal to the second argument, path.text.

path.text is the resolved on-disk path. The query was already split off by normalizeSpecifierForResolution (VirtualMachine.zig:1712-1721) / normalizeSpecifier (options.zig:935-966) before Fs.Path.init, and is never re-joined into path.text. So source_url is always the query-less filesystem path. (And even if a ? survived, fileURLWithFileSystemPath() percent-encodes it into the path component, so the resulting WTF::URL would still have an empty query.)

Why existing code doesn't prevent it

Registry keys do include the query: the same function builds resolvedIdentifier = makeString(resolved.result.value, queryString) at line 3547, so a module imported as ./wrapper.mjs?v=1 lives in the loader registry under /abs/wrapper.mjs?v=1. But the referrer lookup key is derived from sourceOrigin.url(), which — as shown above — never carries the query. CodeRabbit's premise ("that query is present in sourceURL") is wrong for Bun's file modules; bfdabd0 implemented exactly what it suggested, so it inherits the wrong premise.

Step-by-step proof

Given:

entry.mjs:     await Promise.all([import('./consumer1.mjs?v=1'), import('./consumer2.mjs?v=1')])
consumerN.mjs: import { X } from './wrapper.mjs?v=1'
wrapper.mjs:   const m = await import('./inner.mjs'); export const X = m.X;
  1. consumer1.mjs?v=1 resolves ./wrapper.mjs?v=1 → registry key /abs/wrapper.mjs?v=1 (line 3547 path).
  2. The fetch for that key transpiles /abs/wrapper.mjs; ResolvedSource.source_url = path.text = "/abs/wrapper.mjs" (no query).
  3. ZigSourceProvider builds SourceOrigin(fileURLWithFileSystemPath("/abs/wrapper.mjs")) → file:///abs/wrapper.mjs.
  4. wrapper.mjs runs await import('./inner.mjs'); moduleLoaderImportModule receives sourceOrigin.url() = file:///abs/wrapper.mjs.
  5. Line 3484: queryWithLeadingQuestionMark() → ""; referrerKey = "/abs/wrapper.mjs".
  6. asyncEvaluationOrderForKey("/abs/wrapper.mjs") misses (registry has /abs/wrapper.mjs?v=1) → returns -1.
  7. referrerAsyncOrder = -1 is forwarded to JSC::importModule, so the TLA self-deadlock skip never fires for this referrer — the [1.3.14] ESM TDZ error when importing Lexical React modules that re-export through top-level await #30634 TDZ behaviour persists for query-keyed wrappers.

Impact

Not a regression: pre-PR there was no referrerAsyncOrder at all, so query-keyed referrers were equally broken. The no-query case — which is what #30634 actually reports and what the new test covers — is fixed. The impact is (a) misleading dead code, and (b) the "✅ Addressed" mark on the review thread is inaccurate: the query-stringed-referrer edge case remains unfixed.

How to fix

Either:

  • Thread the query into the SourceOrigin so sourceURL actually carries it — e.g. set ResolvedSource.source_url to the full registry key (path + query) instead of bare path.text, or append the query before calling fileURLWithFileSystemPath and re-set it on the resulting URL; or
  • Drop the dead !query.isEmpty() branch and leave a comment that query-keyed referrers aren't yet handled, so the code doesn't imply otherwise.

If you keep the fix, a variant of the new test that imports ./wrapper.mjs?v=1 would exercise it.

} else if (sourceURL.protocol() == "builtin"_s) {
ASSERT(sourceURL.string().startsWith("builtin://"_s));
sourceOriginStringHolder = sourceURL.string().substringSharingImpl(10 /* builtin:// */);
} else {
sourceOriginStringHolder = sourceURL.path().toString();
}

if (globalObject->onLoadPlugins.hasVirtualModules()) {
if (auto resolution = globalObject->onLoadPlugins.resolveVirtualModule(moduleName, sourceOrigin.url().protocolIsFile() ? sourceOrigin.url().fileSystemPath() : String())) {
if (auto resolution = globalObject->onLoadPlugins.resolveVirtualModule(moduleName, sourceURL.protocolIsFile() ? sourceOriginStringHolder : String())) {
resolvedIdentifier = JSC::Identifier::fromString(vm, resolution.value());

auto result = JSC::importModule(globalObject, resolvedIdentifier, JSC::Identifier(), parameters, nullptr);
auto result = JSC::importModule(globalObject, resolvedIdentifier, JSC::Identifier(), parameters, nullptr, /* deferred */ false, referrerAsyncOrder);
if (scope.exception()) [[unlikely]] {
return JSC::JSPromise::rejectedPromiseWithCaughtException(globalObject, scope);
}
Expand All @@ -3497,7 +3517,6 @@ JSC::JSPromise* GlobalObject::moduleLoaderImportModule(JSGlobalObject* jsGlobalO
ErrorableString resolved;
memset(&resolved, 0, sizeof(resolved));

auto sourceURL = sourceOrigin.url();
BunString moduleNameZ;
String moduleStringHolder;
if (moduleName->startsWith("file://"_s)) {
Expand All @@ -3513,19 +3532,6 @@ JSC::JSPromise* GlobalObject::moduleLoaderImportModule(JSGlobalObject* jsGlobalO
}

BunString queryString = { BunStringTag::Empty, nullptr };
String sourceOriginStringHolder;

if (sourceURL.isEmpty()) {
sourceOriginStringHolder = String("."_s);
} else if (sourceURL.protocolIsFile()) {
sourceOriginStringHolder = sourceURL.fileSystemPath();
} else if (sourceURL.protocol() == "builtin"_s) {
ASSERT(sourceURL.string().startsWith("builtin://"_s));
sourceOriginStringHolder = sourceURL.string().substringSharingImpl(10 /* builtin:// */);
} else {
sourceOriginStringHolder = sourceURL.path().toString();
}

auto sourceOriginZ = Bun::toStringRef(sourceOriginStringHolder);

Zig__GlobalObject__resolve(&resolved, globalObject, &moduleNameZ, &sourceOriginZ, &queryString);
Expand Down Expand Up @@ -3559,7 +3565,7 @@ JSC::JSPromise* GlobalObject::moduleLoaderImportModule(JSGlobalObject* jsGlobalO
// ScriptFetchParameters before calling this hook, so `parameters` is
// already the parsed RefPtr (or null). Just forward it.
auto result = JSC::importModule(globalObject, resolvedIdentifier,
JSC::Identifier(), WTF::move(parameters), nullptr);
JSC::Identifier(), WTF::move(parameters), nullptr, /* deferred */ false, referrerAsyncOrder);
if (scope.exception()) [[unlikely]] {
return JSC::JSPromise::rejectedPromiseWithCaughtException(globalObject, scope);
}
Expand Down
41 changes: 41 additions & 0 deletions test/js/bun/resolve/dynamic-import-tla-cycle.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,3 +199,44 @@ test("static sibling import waits for an indirectly-shared TLA dep in the same E
expect(stdout.trim()).toBe("456");
expect(exitCode).toBe(0);
});

// https://github.com/oven-sh/bun/issues/30634
test("sibling dynamic imports sharing a TLA wrapper wait for its post-await exports", async () => {
using dir = tempDir("dyn-tla-shared-wrapper", {
"entry.mjs": `
const [c1, c2] = await Promise.all([import("./consumer1.mjs"), import("./consumer2.mjs")]);
console.log(c1.FOO, c2.BAR);
`,
"wrapper.mjs": `
const mod = await import("./inner.mjs");
export const FOO = mod.FOO;
export const BAR = mod.BAR;
`,
"inner.mjs": `
export const FOO = "foo";
export const BAR = "bar";
`,
"consumer1.mjs": `
import { FOO as wrapped } from "./wrapper.mjs";
export const FOO = wrapped;
`,
"consumer2.mjs": `
import { BAR as wrapped } from "./wrapper.mjs";
export const BAR = wrapped;
`,
});

await using proc = Bun.spawn({
cmd: [bunExe(), "entry.mjs"],
env: bunEnv,
cwd: String(dir),
stdout: "pipe",
stderr: "pipe",
});

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

expect(stderr).toBe("");
expect(stdout.trim()).toBe("foo bar");
expect(exitCode).toBe(0);
});
Loading