moduleLoaderImportModule: thread referrer asyncEvaluationOrder for TLA self-deadlock skip - #32437
Conversation
|
Updated 8:53 PM PT - Jun 16th, 2026
❌ @Jarred-Sumner, your commit 566cb13 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 32437That installs a local version of the PR into your bun-32437 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Walkthrough
ChangesTLA sibling dynamic-import fix and regression test
WebKit prebuilt version update
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![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 `@src/jsc/bindings/ZigGlobalObject.cpp`:
- Around line 3482-3484: The issue is that the asyncEvaluationOrderForKey()
lookup uses sourceOriginStringHolder which is derived from
sourceURL.fileSystemPath(), but fileSystemPath() strips query strings. However,
modules are registered in the loader with their full identifiers including query
strings, so the lookup fails to find the cached entry. Fix this by extracting
the search component from sourceURL (the query string portion) and appending it
to sourceOriginStringHolder before passing it to asyncEvaluationOrderForKey(),
matching the pattern used for resolvedIdentifier on lines 3540-3544 where
queryString is appended. Additionally, add a test case that imports a module
with a query parameter to verify the async evaluation ordering optimization
works correctly for referrers with query strings.
🪄 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: 0b6543a0-daff-4d3a-9ec5-f5be26ea2983
📒 Files selected for processing (4)
scripts/build/deps/webkit.tssrc/jsc/bindings/NodeVM.cppsrc/jsc/bindings/ZigGlobalObject.cpptest/js/bun/resolve/dynamic-import-tla-cycle.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| 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); |
There was a problem hiding this comment.
🟡 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;
consumer1.mjs?v=1resolves./wrapper.mjs?v=1→ registry key/abs/wrapper.mjs?v=1(line 3547 path).- The fetch for that key transpiles
/abs/wrapper.mjs;ResolvedSource.source_url = path.text = "/abs/wrapper.mjs"(no query). ZigSourceProviderbuildsSourceOrigin(fileURLWithFileSystemPath("/abs/wrapper.mjs"))→file:///abs/wrapper.mjs.wrapper.mjsrunsawait import('./inner.mjs');moduleLoaderImportModulereceivessourceOrigin.url() = file:///abs/wrapper.mjs.- Line 3484:
queryWithLeadingQuestionMark()→"";referrerKey = "/abs/wrapper.mjs". asyncEvaluationOrderForKey("/abs/wrapper.mjs")misses (registry has/abs/wrapper.mjs?v=1) → returns-1.referrerAsyncOrder = -1is forwarded toJSC::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
sourceURLactually carries it — e.g. setResolvedSource.source_urlto the full registry key (path + query) instead of barepath.text, or append the query before callingfileURLWithFileSystemPathand 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.
…importModule For the TLA self-deadlock skip at innerModuleEvaluation 12.b.v (see oven-sh/WebKit claude/tla-referrer-async-order). Look up the referrer's asyncEvaluationOrder via the new JSModuleLoader::asyncEvaluationOrderForKey() and pass it through; the resolve() referrer stays empty so plugin onResolve and the second resolve pass are unchanged. Hoists the sourceOrigin->path computation so the existing virtual-module branch and the resolve block share it. Adds test for #30634 (sibling dynamic imports sharing a TLA wrapper).
…ey lookup A referrer registered as /abs/index.mjs?v=1 has that as its module-map key; sourceURL.fileSystemPath() drops the query, so the asyncEvaluationOrder lookup missed and the deadlock skip never fired for query-keyed TLA modules.
758bc34 to
cdd26eb
Compare
2 participants
Bun-side half of oven-sh/WebKit
claude/tla-referrer-async-order(supersedes oven-sh/WebKit#228). Fixes #30634.What
moduleLoaderImportModulelooks up the referrer'sasyncEvaluationOrdervia the newJSModuleLoader::asyncEvaluationOrderForKey()and passes it toJSC::importModuleas the newreferrerAsyncOrderparam. Theresolve()referrer stays empty (JSC::Identifier()), so pluginonResolveand the second resolve pass are unchanged.The
sourceOrigin → pathcomputation is hoisted so the virtual-module branch and the resolve block share it.Tests
test/js/bun/resolve/dynamic-import-tla-cycle.test.ts— adds the #30634 sibling-dynamic-import test (fails on system Bun withReferenceError: Cannot access 'wrapped' before initialization, passes on this build). The 5 existing tests (2 deadlock, 3 #30259 sibling) still pass.Depends on the WebKit PR landing + bumping
scripts/build/deps/webkit.ts. This commit compiles against current WebKit (the new param is defaulted) but the deadlock tests will hang until the WebKit side lands.