Key the ES module registry on the host-defined import attribute type - #32999
Key the ES module registry on the host-defined import attribute type#32999robobun wants to merge 1 commit into
Conversation
|
Updated 9:33 PM PT - Aug 12th, 2026
❌ @robobun, your commit 29a818f has some failures in 🧪 To try this PR locally: bunx bun-pr 32999That installs a local version of the PR into your bun-32999 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
adb530c to
ff51a6c
Compare
|
Verified: #19834 is the same bug and this PR fixes it. Added Two details worth stating precisely, since the issue is from 1.2.13:
The new test covers both: the bare-vs-attributed pair from the issue as a regression guard, and the attributed-vs-attributed pair that this PR changes, in both import orders. |
|
CI status for
Every I am deliberately not pushing a |
|
The original fuzz report for this bug was a concurrent variant that this test suite does not cover: two This passes on a build of oven-sh/WebKit#258 alone (I verified it locally from source), so it belongs with these tests. Suggested addition, adapted to the // The original report: two concurrent import()s of one path where one
// loader fails. The failing loader's rejection must stay on its own
// registry entry: it must neither settle the other attribute's in-flight
// fetch nor poison a later, unrelated import of the path with the
// succeeding attribute.
test("a concurrently failing loader does not poison another attribute's entry", async () => {
const out = await run("import-attr-poison", {
// "=" alone is a TOML parse error but loads fine as text.
"data.toml": "=\n",
"index.ts": /* ts */ `
const p = import.meta.dir + "/data.toml";
const [toml, text] = await Promise.allSettled([
import(p, { with: { type: "toml" } }),
import(p, { with: { type: "text" } }),
]);
const later = await import(p, { with: { type: "text" } });
console.log(JSON.stringify({
toml: toml.status === "rejected" ? toml.reason?.name : toml.status,
text: text.status === "rejected" ? text.reason?.name : text.value.default,
later: later.default,
}));
`,
});
expect(out).toEqual({ toml: "BuildMessage", text: "=\n", later: "=\n" });
});On released Bun this fails with Unrelated to the key fix but found in the same rejected branch: |
test/CLAUDE.md prefers test.concurrent when multiple independent tests in one file spawn processes; both tests here use their own tempDir and share no state. sync-webkit-source did `git checkout main && git pull && git checkout WEBKIT_VERSION`, which cannot resolve an autobuild-preview-* release tag: the tagged commit lives on a PR branch, not main, so the plain pull never fetches it. Add a `git fetch --tags origin` so the checkout works while a preview tag is pinned (here and in #32999). No behavior change for the normal 40-hex SHA pin.
ff51a6c to
1eb210c
Compare
|
CI status for
Every As before I am not pushing a |
5e8ef39 to
16aae77
Compare
|
The two Rebased onto Still blocked on oven-sh/WebKit#258; once that lands I will retarget |
|
CI status for
Every This is the fourth consecutive CI result for this diff where every non-green job is unrelated flake or infra (66233: darwin artifact-download timeouts; 67542: alpine memleak flake + darwin artifact timeout; 72861: a |
Importing the same path with two different `with { type }` attributes
returned one shared module: JSC's ModuleMapKey was
(specifier, ScriptFetchParameters::Type), and every Bun-defined attribute
value ("text", "file", "toml", "html", ...) maps onto the single
Type::HostDefined enum value, so whichever import evaluated first decided
what both callers got.
The key now also carries the host-defined attribute string. The fix lives
in oven-sh/WebKit (oven-sh/WebKit#258); this commit
bumps WEBKIT_VERSION, updates Bun's three ModuleMapKey users to the widened
key (NodeVMSourceTextModule uses the new ModuleRequest::moduleMapKey()
helper; two diagnostic iterations switch from .first to std::get<0>), and
adds the regression test, including the HTML reproduction from issue 19834.
Fixes #19834
|
Status check against current main (f426a8e), since #19834 was closed as fixed in the meantime. The closing comment on #19834 verified the literal snippet from that issue (a bare I copied this PR's So this is not covered by main; leaving it open. It remains blocked on oven-sh/WebKit#258 and will need the |
16aae77 to
29a818f
Compare
|
CI status for
Every build step passed on every platform against the preview artifact, all x64-asan test shards are green, and binary size is +0.0 KB on every platform that has a Not pushing a retrigger: it would re-enter the same darwin queue, and this is still a draft blocked on oven-sh/WebKit#258 (retargeting |
1 participant
Importing the same path with two different
with { type }attributes returned one shared module, and whichever import evaluated first decided what every other attribute form got. The import-attributes proposal requires the module registry to be keyed on(specifier, attributes): different attributes are different modules.Fixes #19834
Important
Blocked on oven-sh/WebKit#258. The fix is in JavaScriptCore's module map key.
WEBKIT_VERSIONhere points at #258's preview build artifact (autobuild-preview-pr-258-a024b4c8, built on current oven-sh/WebKitmain) so CI runs the new tests against the fixed JSC. Once #258 lands I will update it to the merge commit; please do not merge while it still points at the preview tag.Reproduction
Deterministic on the released
bunand onmain. #19834 is the HTML form of the same bug (type: "html"vstype: "file"on one.htmlfile share a module onmain).Cause
JSC's
ModuleMapKeyis(UniquedStringImpl* specifier, ScriptFetchParameters::Type).ScriptFetchParameters::parseTypemaps every non-json/ non-webassemblyattribute value onto the singleType::HostDefinedenum value, sowith { type: "text" }andwith { type: "file" }hash to the same registry bucket. The string that distinguishes them (ScriptFetchParameters::m_hostDefinedImportType) never reached the key.There is no Bun-side seam:
moduleLoaderResolve(which produces the key's specifier component) does not receive the attributes, and the cache hit inJSModuleLoader::loadModulehappens beforemoduleLoaderFetchis ever called for the second import.On #19834 specifically: the exact snippet in its body (a bare
import("./index.html")vsimport("./index.html", { with: { type: "file" } })) already behaves correctly onmain, because an attribute-less import keys onType::JavaScriptwhile an attributed one keys onType::HostDefined, and that two-value key was added to the WebKit fork after the issue was filed on 1.2.13. What the issue's title reports, two differentwith { type }values sharing one cache entry, is exactly what was still broken and what this PR fixes. Both forms are covered by the new tests.Fix
oven-sh/WebKit#258 widens the key to
(specifier, Type, hostDefinedImportType)and routes every key construction through one helper, including the parse-time requested-modules dedup and the static-binding resolution that oven-sh/WebKit#403 moved onto the same key. On the Bun side this PR is:scripts/build/deps/webkit.ts: theWEBKIT_VERSIONbump.src/jsc/bindings/NodeVMSourceTextModule.cpp: the one Bun-sideModuleMapKey { a, b }construction moves to the newModuleRequest::moduleMapKey()helper. The key grew a third component, so the two-argument form no longer compiles; that is intentional, so no call site can silently drop the attribute.src/jsc/bindings/InspectorLifecycleAgent.cpp,src/jsc/bindings/ZigGlobalObject.cpp: two diagnostic iterations over the module map read the key's specifier via.first; they now usestd::get<0>. No behavior change.test/js/bun/resolve/import-attributes.test.ts: the regression tests. Six cover dynamic import (both orders, three distinct types on one path, same-attribute dedup, and the Importing HTML bundle with differentwith { type }are cached incorrectly #19834 HTML case in both orders); the seventh covers static imports and a re-export with host-defined attributes, which exercise the binding-resolution path Implement aError.captureStackTracepolyfill #258 had to extend after Support for completion in Bash #403 (it is a guard for that path, not a fail-before case).Verification
Released Bun (
1.3.14),bun test test/js/bun/resolve/import-attributes.test.ts: 1 pass, 6 fail. The six dynamic-import tests fail with exactly the bug (fileandbase64receive thetextcontents; the.htmlimportedwith { type: "file" }receives the HTMLBundle); the static-binding guard passes because a single attributed import per path never collided.bun bd test test/js/bun/resolve/import-attributes.test.tsagainst the #258 artifact: 7 pass, 0 fail.Also against the #258 artifact:
import-empty,import-defer,import-query,esModule,regression/issue/16476(48 tests), plustext-loader,jsonc,json5,yaml,toml, the attribute regressions 22656 / 26360 / 28042 / 30717 / 31575, and thenode:vmmodule tests (337 tests). The only failure istext-loader's "reloaded 10000 times" stress case, which fails identically on amaindebug+ASAN build in the same container (it times out at 5s; the fixture takes ~15s under ASAN); interleaved 5-run averages of that fixture are 15.48s onmainvs 15.46s on this branch, so it is not affected by this change.A note on fail-before: the load-bearing change lives in
vendor/WebKit(a separate repository) plusscripts/build/deps/webkit.ts; the onlysrc/edits are compile fixes for the new WebKit API. Stashingsrc/therefore does not reproduce the bug, it just breaks the build against the new WebKit. The released-Bun run above is the fail-before proof.What is still not covered
Two narrower siblings remain, each its own PR:
Two static imports of the same path with different host-defined types in one module still bind to the first one. After Support for completion in Bash #403 and Implement a
Error.captureStackTracepolyfill #258, the loader holds both modules, but import/export entries record only the attribute'sType, so Implement aError.captureStackTracepolyfill #258 resolves aHostDefinedbinding to the module's first request for that specifier and type. Finishing it means carrying the attribute string throughImportEntry/ExportEntry/ the star-export set and the sevenJSC_JSModuleRecord__add*FFI entry points insrc/jsc/bindings/BunAnalyzeTranspiledModule.cppplus their Rust callers. Dynamic imports, and static imports of different paths or of one path with one attribute, are fully correct now.The bundler's module graph.
PathToSourceIndexMapkeys onpath.textonly; the attribute loader is computed right next to each lookup (bundle_v2.rs:6390) and then discarded, sobun buildof the two-import file also produces one module, order dependent. A naive(path, loader)composite key is not safe: the map has 20+ consumers and four of them genuinely need a bare-path lookup.Bundler map consumer audit (why that is its own PR)
Bare-path dependents that a composite key would break:
src/runtime/bake/dev_server/incremental_graph.rs:1620:on_file_deleted(abs_path)callsmap.remove(abs_path)for every target map with the watcher's bare absolute path. A composite key would silently miss every(path, loader)entry and the dev server would serve stale source indices for deleted-then-recreated files.src/bundler/bundle_v2.rs:2494: the dev-server invalidation re-enqueue does a bare-pathget(path_slice).is_some()membership check (no import record, no attributes in scope). A miss duplicates parse work and source indices in watch mode.src/bundler/LinkerContext.rs:692: looks up an HTML import's own source path andpanic!s on a miss, so the key derivation must be bit-identical between its insert site (bundle_v2.rs:6509) and this read.src/bundler/bundle_v2.rs:2107: the dual-package-hazard rewrite looks up a resolversecondary_path, whose effective loader is not the current record's.On top of that, three sites overwrite an existing key in place (the CJS re-export shim at
bundle_v2.rs:6679, the HTML manifest alias at:6771, the "use client" proxy at:7149), so any key function has to be deterministic and identical between the original insert and the overwrite. The shape that works is to keep the barepath.textkey for every import whose effective loader matches the path's extension-derived one (every existing consumer is in that set) and only suffix the key when awith { type }attribute forces a different loader, plus a suffix sweep inon_file_deleted. That is roughly 18 touch points acrossbundle_v2.rs,PathToSourceIndexMap.rs,barrel_imports.rs,LinkerContext.rs, and the incremental graph, and deserves its own PR with watch-mode coverage rather than riding on a WebKit version bump. Line numbers are as of the original audit and may have drifted.[decide:webkit] gate passed · iteration 10 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 10
evidence per changed file