From dec51b7d0416ded1d19efaf7aa2259c2a7427d81 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sun, 23 Aug 2026 23:57:00 +0000 Subject: [PATCH 01/11] Re-fetch a module whose previous load failed before it evaluated The JSC module loader keeps every failed ModuleRegistryEntry and replays its error on the next lookup of the key. A syntax error, a static import that did not resolve, a link error, or a plugin onLoad that returned unparseable code therefore stayed failed for the whole process, even after the file on disk was fixed. Node never caches a module that failed to load, so the next import() re-reads the file. moduleLoaderResolve and the require(esm) path now drop an entry whose status is FetchFailed, InstantiationFailed, or EvaluationFailed with a record that never reached Evaluating. The loader then fetches the module again. removeEntry also clears the cached resolution failures of the module, so a dependency created later is found. A module whose body threw stays cached, as in Node and the spec. The eviction is skipped while an import() of the same key has not settled. The loader records a top-level failure in ModuleLoadTopSettled and reports it one microtask later in ModuleLoadTopRejected, which looks the key up by name again. A fetch registered in that window would inherit the stale error, and the debug build asserts in ModuleRegistryEntry::fetchComplete. The global object counts the pending import() calls per resolved key and removes the key when the import promise settles. --- src/jsc/bindings/ZigGlobalObject.cpp | 160 ++++++++++++++++-- src/jsc/bindings/ZigGlobalObject.h | 10 ++ .../import-retry-after-failure.test.ts | 146 ++++++++++++++++ 3 files changed, 298 insertions(+), 18 deletions(-) create mode 100644 test/js/bun/resolve/import-retry-after-failure.test.ts diff --git a/src/jsc/bindings/ZigGlobalObject.cpp b/src/jsc/bindings/ZigGlobalObject.cpp index 1bcd27a1a078..0710fd71f4b3 100644 --- a/src/jsc/bindings/ZigGlobalObject.cpp +++ b/src/jsc/bindings/ZigGlobalObject.cpp @@ -738,6 +738,122 @@ static bool isModuleEvaluating(JSC::AbstractModuleRecord* record) return cyclic && cyclic->status() == JSC::CyclicModuleRecord::Status::Evaluating; } +// The JSC loader caches every failure, including a transpile error, an +// unresolved static import, or a link error. That is the browser behavior +// (a module script that fails to fetch or parse is cached as null). Node never +// caches those: its ModuleJob is only added to the load cache once the source +// compiled and every dependency resolved, so the next import() re-reads the +// file. A module that never started evaluating has no side effects that a +// second load could duplicate, so drop the stale record and let the loader +// fetch again. A module whose body threw stays cached, as in Node and the spec. +static bool isFailedEntryThatNeverEvaluated(JSC::ModuleRegistryEntry* entry) +{ + switch (entry->status()) { + case JSC::ModuleRegistryEntry::Status::FetchFailed: + case JSC::ModuleRegistryEntry::Status::InstantiationFailed: + return true; + case JSC::ModuleRegistryEntry::Status::EvaluationFailed: { + // A dependency that failed to load is stored as an evaluation error on + // the importer even though the importer never linked. + auto* record = entry->record(); + if (!record) + return true; + auto* cyclic = dynamicDowncast(record); + return cyclic && cyclic->status() < JSC::CyclicModuleRecord::Status::Evaluating; + } + default: + return false; + } +} + +static void dropFailedEntryThatNeverEvaluated(Zig::GlobalObject* globalObject, const JSC::Identifier& key) +{ + // The map is keyed by (specifier, type). Probe each type directly: + // registryEntry() falls back to a scan of the whole map for a key that has + // no JavaScript entry, and this runs on every resolve. + static constexpr JSC::ScriptFetchParameters::Type moduleTypes[] = { + JSC::ScriptFetchParameters::Type::None, + JSC::ScriptFetchParameters::Type::JavaScript, + JSC::ScriptFetchParameters::Type::WebAssembly, + JSC::ScriptFetchParameters::Type::JSON, + JSC::ScriptFetchParameters::Type::Text, + JSC::ScriptFetchParameters::Type::HostDefined, + }; + auto* impl = key.impl(); + if (!impl) + return; + + // An import() of this key is still settling. The loader records a top-level + // failure in one microtask and reports it in the next, and the second one + // looks the key up by name: it would attach the stale error to any entry a + // new fetch registered in between. Keep the failed entry until the import() + // promise has settled. + if (globalObject->pendingDynamicImports.contains(impl)) + return; + + auto* loader = globalObject->moduleLoader(); + bool found = false; + const auto& moduleMap = loader->moduleMap(); + for (auto type : moduleTypes) { + auto* entry = moduleMap.get({ impl, type }).get(); + if (!entry) + continue; + // removeEntry drops every type variant of the key, so keep them all + // while any variant holds a module that may have run. + if (!isFailedEntryThatNeverEvaluated(entry)) + return; + found = true; + } + if (!found) + return; + + // JSModuleLoader::visitChildrenImpl iterates these maps on the GC thread + // under cellLock(); take the same lock so the removal can't race it. + WTF::Locker locker { loader->cellLock() }; + loader->removeEntry(key); +} + +// Reaction on the promise of an import(): argument 1 is the resolved key that +// importResolvedModule registered in pendingDynamicImports. +BUN_DEFINE_HOST_FUNCTION(Bun__onDynamicImportSettled, (JSC::JSGlobalObject * globalObject, JSC::CallFrame* callFrame)) +{ + auto& vm = JSC::getVM(globalObject); + auto scope = DECLARE_THROW_SCOPE(vm); + auto* keyString = dynamicDowncast(callFrame->argument(1)); + if (!keyString) [[unlikely]] + return JSValue::encode(jsUndefined()); + auto key = JSC::Identifier::fromString(vm, keyString->value(globalObject)); + RETURN_IF_EXCEPTION(scope, {}); + static_cast(globalObject)->pendingDynamicImports.remove(key.impl()); + return JSValue::encode(jsUndefined()); +} + +// import() of an already resolved key. The key stays in pendingDynamicImports +// until the returned promise settles, which is after the loader has finished +// touching the registry for this load. See dropFailedEntryThatNeverEvaluated. +static JSC::JSPromise* importResolvedModule(Zig::GlobalObject* globalObject, const JSC::Identifier& key, RefPtr&& parameters, int64_t referrerAsyncOrder) +{ + auto& vm = JSC::getVM(globalObject); + auto scope = DECLARE_THROW_SCOPE(vm); + + // Before this import() counts as pending, or the resolve hook would see it + // and keep the failed entry. + dropFailedEntryThatNeverEvaluated(globalObject, key); + globalObject->pendingDynamicImports.add(key.impl()); + auto* result = JSC::importModule(globalObject, key, JSC::Identifier(), WTF::move(parameters), nullptr, /* deferred */ false, referrerAsyncOrder); + if (scope.exception()) [[unlikely]] { + globalObject->pendingDynamicImports.remove(key.impl()); + return JSC::JSPromise::rejectedPromiseWithCaughtException(globalObject, scope); + } + ASSERT(result); + + // The key string shares the Identifier's atom, so the handler gets the same + // UniquedStringImpl back from Identifier::fromString. + JSFunction* onSettled = globalObject->thenable(Bun__onDynamicImportSettled); + result->performPromiseThenWithContext(vm, globalObject, onSettled, onSettled, jsUndefined(), jsString(vm, key.string())); + return result; +} + JSC_DEFINE_HOST_FUNCTION(functionEsmNamespaceForCjs, (JSC::JSGlobalObject * globalObject, JSC::CallFrame* callFrame)) { auto& vm = JSC::getVM(globalObject); @@ -801,6 +917,8 @@ JSC_DEFINE_HOST_FUNCTION(functionEsmLoadSync, (JSC::JSGlobalObject * lexicalGlob auto key = JSC::Identifier::fromString(vm, keyString); auto* loader = globalObject->moduleLoader(); + // loadModuleSync looks the key up directly, without the resolve hook. + dropFailedEntryThatNeverEvaluated(globalObject, key); bool entryExistedBefore = false; if (auto* entry = loader->registryEntry(key)) { entryExistedBefore = true; @@ -3409,11 +3527,8 @@ extern "C" bool Bun__standaloneModuleHasModuleInfo(const Latin1Character*, size_ extern "C" bool Bun__hasStandaloneModuleGraph(); extern "C" int ModuleLoader__builtinAliasIndex(const Latin1Character*, size_t); extern "C" bool Bun__hasPluginRunner(void*); -JSC::Identifier GlobalObject::moduleLoaderResolve(JSGlobalObject* jsGlobalObject, - JSModuleLoader* loader, JSValue key, - JSValue referrer, RefPtr, bool) +static JSC::Identifier resolveModuleKey(Zig::GlobalObject* globalObject, JSValue key, JSValue referrer) { - Zig::GlobalObject* globalObject = static_cast(jsGlobalObject); auto& vm = globalObject->vm(); auto scope = DECLARE_THROW_SCOPE(vm); @@ -3505,6 +3620,25 @@ JSC::Identifier GlobalObject::moduleLoaderResolve(JSGlobalObject* jsGlobalObject return Identifier::fromString(vm, resolved); } +JSC::Identifier GlobalObject::moduleLoaderResolve(JSGlobalObject* jsGlobalObject, + JSModuleLoader*, JSValue key, + JSValue referrer, RefPtr, bool) +{ + Zig::GlobalObject* globalObject = static_cast(jsGlobalObject); + auto& vm = globalObject->vm(); + auto scope = DECLARE_THROW_SCOPE(vm); + + JSC::Identifier resolved = resolveModuleKey(globalObject, key, referrer); + RETURN_IF_EXCEPTION(scope, {}); + + // Every lookup of a module in the registry, whether from import(), a + // static import of a module being linked, or the entry point, goes through + // resolve() first. Evicting here makes the loader fetch the module again + // when its last load never produced a module. + dropFailedEntryThatNeverEvaluated(globalObject, resolved); + return resolved; +} + JSC::Identifier StandaloneGlobalObject::moduleLoaderResolve(JSGlobalObject* globalObject, JSModuleLoader* loader, JSValue key, JSValue referrer, RefPtr fetcher, bool b) { // Embedded modules import each other by their final `/$bunfs/` key; hand it straight back (unless a plugin could claim it). @@ -3577,12 +3711,7 @@ JSC::JSPromise* GlobalObject::moduleLoaderImportModule(JSGlobalObject* jsGlobalO if (globalObject->onLoadPlugins.hasVirtualModules()) { 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, /* deferred */ false, referrerAsyncOrder); - if (scope.exception()) [[unlikely]] { - return JSC::JSPromise::rejectedPromiseWithCaughtException(globalObject, scope); - } - return result; + RELEASE_AND_RETURN(scope, importResolvedModule(globalObject, resolvedIdentifier, WTF::move(parameters), referrerAsyncOrder)); } } @@ -3617,14 +3746,7 @@ JSC::JSPromise* GlobalObject::moduleLoaderImportModule(JSGlobalObject* jsGlobalO // The C++ module loader now extracts `with.type` into a // 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, /* deferred */ false, referrerAsyncOrder); - if (scope.exception()) [[unlikely]] { - return JSC::JSPromise::rejectedPromiseWithCaughtException(globalObject, scope); - } - - ASSERT(result); - return result; + RELEASE_AND_RETURN(scope, importResolvedModule(globalObject, resolvedIdentifier, WTF::move(parameters), referrerAsyncOrder)); } static JSC::JSPromise* rejectedInternalPromise(JSC::JSGlobalObject* globalObject, JSC::JSValue value) @@ -4208,6 +4330,8 @@ GlobalObject::PromiseFunctions GlobalObject::promiseHandlerID(Zig::FFIFunction h return GlobalObject::PromiseFunctions::Bun__HTMLRewriter__onResolveInputStream; } else if (handler == Bun__HTMLRewriter__onRejectInputStream) { return GlobalObject::PromiseFunctions::Bun__HTMLRewriter__onRejectInputStream; + } else if (handler == Bun__onDynamicImportSettled) { + return GlobalObject::PromiseFunctions::Bun__onDynamicImportSettled; } else { RELEASE_ASSERT_NOT_REACHED(); } diff --git a/src/jsc/bindings/ZigGlobalObject.h b/src/jsc/bindings/ZigGlobalObject.h index df905583255e..7818291706cf 100644 --- a/src/jsc/bindings/ZigGlobalObject.h +++ b/src/jsc/bindings/ZigGlobalObject.h @@ -55,6 +55,8 @@ struct node_module; #include "headers-handwritten.h" #include #include +#include +#include #include #include #include "DOMConstructors.h" @@ -410,6 +412,7 @@ class GlobalObject : public Bun::GlobalScope { Bun__S3UploadStream__onRejectStream, Bun__HTMLRewriter__onResolveInputStream, Bun__HTMLRewriter__onRejectInputStream, + Bun__onDynamicImportSettled, Count_, }; static constexpr size_t promiseFunctionsSize = static_cast(PromiseFunctions::Count_); @@ -725,6 +728,13 @@ class GlobalObject : public Bun::GlobalScope { BunPlugin::OnLoad onLoadPlugins {}; BunPlugin::OnResolve onResolvePlugins {}; + // Resolved keys of the import() calls whose promise has not settled yet. + // moduleLoaderResolve must not drop a failed registry entry for such a key: + // the loader's ModuleLoadTopRejected microtask still looks the key up by + // name after the failure is recorded, and would store the stale error on a + // fresh entry. + WTF::HashCountedSet, JSC::IdentifierRepHash> pendingDynamicImports; + // This increases the cache hit rate for JSC::VM's SourceProvider cache // It also avoids an extra allocation for the SourceProvider // The key is a pointer to the source code diff --git a/test/js/bun/resolve/import-retry-after-failure.test.ts b/test/js/bun/resolve/import-retry-after-failure.test.ts new file mode 100644 index 000000000000..49fbc01f6711 --- /dev/null +++ b/test/js/bun/resolve/import-retry-after-failure.test.ts @@ -0,0 +1,146 @@ +import { expect, test } from "bun:test"; +import { bunEnv, bunExe, normalizeBunSnapshot, tempDir } from "harness"; + +// The module loader used to keep the first failure of a module for the whole +// process: a syntax error, a static import that did not resolve, or a plugin +// onLoad that returned unparseable code. Once the file on disk was fixed, +// import() still rejected with the stale error. Node re-reads the file on the +// next import() because it never caches a module that failed to load. Only a +// module whose body threw stays cached (that is the spec, and Node does it too). +test("import() of a module that failed to load retries after the file changes", async () => { + using dir = tempDir("import-retry", { + "main.mjs": ` + import fs from "node:fs"; + import { createRequire } from "node:module"; + import { basename } from "node:path"; + const require = createRequire(import.meta.url); + + async function t(label, specifier) { + try { + const ns = await import(specifier); + console.log(label, "OK", ns.v); + } catch (e) { + console.log(label, "ERR", e.message.split("\\n")[0]); + } + } + + // Own syntax error, then fixed. + fs.writeFileSync("a.mjs", "export const v == 42;"); + await t("A1", "./a.mjs"); + fs.writeFileSync("a.mjs", "export const v = 42;"); + await t("A2", "./a.mjs"); + + // Static dependency missing, then created. The dependency itself was + // imported in between, so its own entry is a success. + fs.writeFileSync("b.mjs", 'export { v } from "./c.mjs";'); + await t("B1", "./b.mjs"); + fs.writeFileSync("c.mjs", "export const v = 42;"); + await t("B2c", "./c.mjs"); + await t("B2", "./b.mjs"); + + // Static dependency with a syntax error, then fixed. Both the parent and + // the dependency hold a failed entry. + fs.writeFileSync("f.mjs", 'export { v } from "./g.mjs";'); + fs.writeFileSync("g.mjs", "export const v == 42;"); + await t("S1", "./f.mjs"); + fs.writeFileSync("g.mjs", "export const v = 42;"); + await t("S2", "./f.mjs"); + + // Dependency imported directly first and fails, fixed, then a new parent + // imports it statically. + fs.writeFileSync("i.mjs", "export const v == 42;"); + await t("N1", "./i.mjs"); + fs.writeFileSync("i.mjs", "export const v = 42;"); + fs.writeFileSync("h.mjs", 'export { v } from "./i.mjs";'); + await t("N2", "./h.mjs"); + + // import() failed, then require() of the fixed file. + fs.writeFileSync("r.mjs", "export const v == 42;"); + await t("R1", "./r.mjs"); + fs.writeFileSync("r.mjs", "export const v = 42;"); + try { + console.log("R2", "OK", require("./r.mjs").v); + } catch (e) { + console.log("R2", "ERR", e.message.split("\\n")[0]); + } + + // A plugin onLoad hook that returned unparseable code runs again. The + // first load of each .virt file fails, every later one succeeds. + const loads = new Map(); + Bun.plugin({ + name: "retry", + setup(build) { + build.onLoad({ filter: /\\.virt$/ }, ({ path }) => { + const n = (loads.get(basename(path)) ?? 0) + 1; + loads.set(basename(path), n); + return { contents: n === 1 ? "export const v == 1;" : "export const v = 42;", loader: "js" }; + }); + }, + }); + fs.writeFileSync("p.virt", ""); + await t("P1", "./p.virt"); + await t("P2", "./p.virt"); + console.log("P loads", loads.get("p.virt")); + + // With a plugin registered every load runs on this thread, so the two + // loads below settle in the same microtask checkpoint. The failed entry + // must stay in the registry until the first import() has settled: the + // loader reports a top-level failure one microtask after it records it, + // by key, so a fetch registered in between would inherit the stale error. + // Two import() calls in the same tick, then a retry after both settled. + fs.writeFileSync("h1.virt", ""); + const h1 = await Promise.allSettled([import("./h1.virt"), import("./h1.virt")]); + console.log("H1", h1.map(r => (r.status === "fulfilled" ? "OK " + r.value.v : "ERR")).join(" ")); + await t("H1b", "./h1.virt"); + console.log("H1 loads", loads.get("h1.virt")); + // An import() issued from a microtask queued right after the failing one. + fs.writeFileSync("h2.virt", ""); + const h2 = await Promise.allSettled([import("./h2.virt"), Promise.resolve().then(() => import("./h2.virt"))]); + console.log("H2", h2.map(r => (r.status === "fulfilled" ? "OK " + r.value.v : "ERR")).join(" ")); + await t("H2b", "./h2.virt"); + console.log("H2 loads", loads.get("h2.virt")); + + // A module whose body threw stays cached even after the file changes. + fs.writeFileSync("e.mjs", 'throw new Error("boom"); export const v = 1;'); + await t("E1", "./e.mjs"); + fs.writeFileSync("e.mjs", "export const v = 42;"); + await t("E2", "./e.mjs"); + `, + }); + + await using proc = Bun.spawn({ + cmd: [bunExe(), "main.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(normalizeBunSnapshot(stdout, String(dir))).toMatchInlineSnapshot(` + "A1 ERR 2 errors building "/a.mjs" + A2 OK 42 + B1 ERR Cannot find module './c.mjs' imported from /b.mjs + B2c OK 42 + B2 OK 42 + S1 ERR 2 errors building "/g.mjs" + S2 OK 42 + N1 ERR 2 errors building "/i.mjs" + N2 OK 42 + R1 ERR 2 errors building "/r.mjs" + R2 OK 42 + P1 ERR 2 errors building "/p.virt" + P2 OK 42 + P loads 2 + H1 ERR ERR + H1b OK 42 + H1 loads 3 + H2 ERR ERR + H2b OK 42 + H2 loads 2 + E1 ERR boom + E2 ERR boom" + `); + expect(stderr).toBe(""); + expect(exitCode).toBe(0); +}); From f1841594a056c7493f60f4e03dd423867dca3795 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Mon, 24 Aug 2026 00:46:38 +0000 Subject: [PATCH 02/11] Track Module.runMain loads and reset the pending set on reload Module.runMain starts a top-level load like import() does, so a failed entry of that key must stay put until its promise settles. Resolve the specifier first and register the load under the same key. clearModuleRegistry drops every entry, so a load that was in flight may never settle. Clear the pending set with the registry, or that key would keep its next failed entry forever. --- src/jsc/bindings/ZigGlobalObject.cpp | 52 +++++++++++++++------------- src/jsc/bindings/ZigGlobalObject.h | 17 +++++---- src/jsc/modules/NodeModuleModule.cpp | 13 +++++-- 3 files changed, 48 insertions(+), 34 deletions(-) diff --git a/src/jsc/bindings/ZigGlobalObject.cpp b/src/jsc/bindings/ZigGlobalObject.cpp index 0710fd71f4b3..feffb2695fdf 100644 --- a/src/jsc/bindings/ZigGlobalObject.cpp +++ b/src/jsc/bindings/ZigGlobalObject.cpp @@ -783,12 +783,12 @@ static void dropFailedEntryThatNeverEvaluated(Zig::GlobalObject* globalObject, c if (!impl) return; - // An import() of this key is still settling. The loader records a top-level - // failure in one microtask and reports it in the next, and the second one - // looks the key up by name: it would attach the stale error to any entry a - // new fetch registered in between. Keep the failed entry until the import() - // promise has settled. - if (globalObject->pendingDynamicImports.contains(impl)) + // A top-level load of this key is still settling. The loader records a + // top-level failure in one microtask and reports it in the next, and the + // second one looks the key up by name: it would attach the stale error to + // any entry a new fetch registered in between. Keep the failed entry until + // the load's promise has settled. + if (globalObject->pendingModuleLoads.contains(impl)) return; auto* loader = globalObject->moduleLoader(); @@ -813,9 +813,9 @@ static void dropFailedEntryThatNeverEvaluated(Zig::GlobalObject* globalObject, c loader->removeEntry(key); } -// Reaction on the promise of an import(): argument 1 is the resolved key that -// importResolvedModule registered in pendingDynamicImports. -BUN_DEFINE_HOST_FUNCTION(Bun__onDynamicImportSettled, (JSC::JSGlobalObject * globalObject, JSC::CallFrame* callFrame)) +// Reaction on the promise of a top-level module load: argument 1 is the +// resolved key that trackPendingModuleLoad registered in pendingModuleLoads. +BUN_DEFINE_HOST_FUNCTION(Bun__onModuleLoadSettled, (JSC::JSGlobalObject * globalObject, JSC::CallFrame* callFrame)) { auto& vm = JSC::getVM(globalObject); auto scope = DECLARE_THROW_SCOPE(vm); @@ -824,11 +824,21 @@ BUN_DEFINE_HOST_FUNCTION(Bun__onDynamicImportSettled, (JSC::JSGlobalObject * glo return JSValue::encode(jsUndefined()); auto key = JSC::Identifier::fromString(vm, keyString->value(globalObject)); RETURN_IF_EXCEPTION(scope, {}); - static_cast(globalObject)->pendingDynamicImports.remove(key.impl()); + static_cast(globalObject)->pendingModuleLoads.remove(key.impl()); return JSValue::encode(jsUndefined()); } -// import() of an already resolved key. The key stays in pendingDynamicImports +void GlobalObject::trackPendingModuleLoad(const JSC::Identifier& key, JSC::JSPromise* promise) +{ + auto& vm = this->vm(); + pendingModuleLoads.add(key.impl()); + // The key string shares the Identifier's atom, so the handler gets the same + // UniquedStringImpl back from Identifier::fromString. + JSFunction* onSettled = thenable(Bun__onModuleLoadSettled); + promise->performPromiseThenWithContext(vm, this, onSettled, onSettled, jsUndefined(), jsString(vm, key.string())); +} + +// import() of an already resolved key. The key stays in pendingModuleLoads // until the returned promise settles, which is after the loader has finished // touching the registry for this load. See dropFailedEntryThatNeverEvaluated. static JSC::JSPromise* importResolvedModule(Zig::GlobalObject* globalObject, const JSC::Identifier& key, RefPtr&& parameters, int64_t referrerAsyncOrder) @@ -836,21 +846,12 @@ static JSC::JSPromise* importResolvedModule(Zig::GlobalObject* globalObject, con auto& vm = JSC::getVM(globalObject); auto scope = DECLARE_THROW_SCOPE(vm); - // Before this import() counts as pending, or the resolve hook would see it - // and keep the failed entry. dropFailedEntryThatNeverEvaluated(globalObject, key); - globalObject->pendingDynamicImports.add(key.impl()); auto* result = JSC::importModule(globalObject, key, JSC::Identifier(), WTF::move(parameters), nullptr, /* deferred */ false, referrerAsyncOrder); - if (scope.exception()) [[unlikely]] { - globalObject->pendingDynamicImports.remove(key.impl()); + if (scope.exception()) [[unlikely]] return JSC::JSPromise::rejectedPromiseWithCaughtException(globalObject, scope); - } ASSERT(result); - - // The key string shares the Identifier's atom, so the handler gets the same - // UniquedStringImpl back from Identifier::fromString. - JSFunction* onSettled = globalObject->thenable(Bun__onDynamicImportSettled); - result->performPromiseThenWithContext(vm, globalObject, onSettled, onSettled, jsUndefined(), jsString(vm, key.string())); + globalObject->trackPendingModuleLoad(key, result); return result; } @@ -3483,6 +3484,9 @@ void GlobalObject::clearModuleRegistry() WTF::Locker locker { moduleLoader->cellLock() }; moduleLoader->clearAll(); } + // A load that was in flight has lost its entry, so its promise may never + // settle. Do not let it pin a failed entry of the next registry. + this->pendingModuleLoads.clear(); this->requireMap()->clear(this); } @@ -4330,8 +4334,8 @@ GlobalObject::PromiseFunctions GlobalObject::promiseHandlerID(Zig::FFIFunction h return GlobalObject::PromiseFunctions::Bun__HTMLRewriter__onResolveInputStream; } else if (handler == Bun__HTMLRewriter__onRejectInputStream) { return GlobalObject::PromiseFunctions::Bun__HTMLRewriter__onRejectInputStream; - } else if (handler == Bun__onDynamicImportSettled) { - return GlobalObject::PromiseFunctions::Bun__onDynamicImportSettled; + } else if (handler == Bun__onModuleLoadSettled) { + return GlobalObject::PromiseFunctions::Bun__onModuleLoadSettled; } else { RELEASE_ASSERT_NOT_REACHED(); } diff --git a/src/jsc/bindings/ZigGlobalObject.h b/src/jsc/bindings/ZigGlobalObject.h index 7818291706cf..aef9c3264cff 100644 --- a/src/jsc/bindings/ZigGlobalObject.h +++ b/src/jsc/bindings/ZigGlobalObject.h @@ -412,7 +412,7 @@ class GlobalObject : public Bun::GlobalScope { Bun__S3UploadStream__onRejectStream, Bun__HTMLRewriter__onResolveInputStream, Bun__HTMLRewriter__onRejectInputStream, - Bun__onDynamicImportSettled, + Bun__onModuleLoadSettled, Count_, }; static constexpr size_t promiseFunctionsSize = static_cast(PromiseFunctions::Count_); @@ -728,12 +728,15 @@ class GlobalObject : public Bun::GlobalScope { BunPlugin::OnLoad onLoadPlugins {}; BunPlugin::OnResolve onResolvePlugins {}; - // Resolved keys of the import() calls whose promise has not settled yet. - // moduleLoaderResolve must not drop a failed registry entry for such a key: - // the loader's ModuleLoadTopRejected microtask still looks the key up by - // name after the failure is recorded, and would store the stale error on a - // fresh entry. - WTF::HashCountedSet, JSC::IdentifierRepHash> pendingDynamicImports; + // Resolved keys of the top-level module loads (import(), Module.runMain) + // whose promise has not settled yet. moduleLoaderResolve must not drop a + // failed registry entry for such a key: the loader's ModuleLoadTopRejected + // microtask still looks the key up by name after the failure is recorded, + // and would store the stale error on a fresh entry. + WTF::HashCountedSet, JSC::IdentifierRepHash> pendingModuleLoads; + // Keeps `key` in pendingModuleLoads until `promise`, the result of a + // top-level load of that key, settles. + void trackPendingModuleLoad(const JSC::Identifier& key, JSC::JSPromise* promise); // This increases the cache hit rate for JSC::VM's SourceProvider cache // It also avoids an extra allocation for the SourceProvider diff --git a/src/jsc/modules/NodeModuleModule.cpp b/src/jsc/modules/NodeModuleModule.cpp index f4ba6173af35..beaf2f8e8b11 100644 --- a/src/jsc/modules/NodeModuleModule.cpp +++ b/src/jsc/modules/NodeModuleModule.cpp @@ -777,17 +777,24 @@ JSC_DEFINE_HOST_FUNCTION(jsFunctionLoad, (JSGlobalObject * globalObject, JSC::Ca } extern "C" void Bun__VirtualMachine__setOverrideModuleRunMainPromise(void* bunVM, JSPromise* promise); -JSC_DEFINE_HOST_FUNCTION(jsFunctionRunMain, (JSGlobalObject * globalObject, JSC::CallFrame* callFrame)) +JSC_DEFINE_HOST_FUNCTION(jsFunctionRunMain, (JSGlobalObject * lexicalGlobalObject, JSC::CallFrame* callFrame)) { + auto* globalObject = defaultGlobalObject(lexicalGlobalObject); auto& vm = JSC::getVM(globalObject); auto scope = DECLARE_THROW_SCOPE(vm); auto arg1 = callFrame->argument(0); auto name = arg1.toWTFString(globalObject); RETURN_IF_EXCEPTION(scope, {}); - auto* promise = JSC::loadAndEvaluateModule(globalObject, name, nullptr, nullptr); + // Resolve first: the load is a top-level load like import() and is tracked + // under the key the loader uses while its promise is pending. + auto key = Zig::GlobalObject::moduleLoaderResolve(globalObject, globalObject->moduleLoader(), JSC::jsString(vm, name), JSC::jsUndefined(), nullptr, false); + RETURN_IF_EXCEPTION(scope, {}); + + auto* promise = JSC::loadAndEvaluateModule(globalObject, key.string(), nullptr, nullptr); RETURN_IF_EXCEPTION(scope, {}); - Bun__VirtualMachine__setOverrideModuleRunMainPromise(defaultGlobalObject(globalObject)->bunVM(), promise); + globalObject->trackPendingModuleLoad(key, promise); + Bun__VirtualMachine__setOverrideModuleRunMainPromise(globalObject->bunVM(), promise); return JSC::JSValue::encode(JSC::jsUndefined()); } From 1de09c14959af8588a472c1a558f11ef1b319e26 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Mon, 24 Aug 2026 00:50:08 +0000 Subject: [PATCH 03/11] Shorten the module loader comments --- src/jsc/bindings/ZigGlobalObject.cpp | 47 +++++++++------------------- src/jsc/bindings/ZigGlobalObject.h | 8 ++--- src/jsc/modules/NodeModuleModule.cpp | 3 +- 3 files changed, 17 insertions(+), 41 deletions(-) diff --git a/src/jsc/bindings/ZigGlobalObject.cpp b/src/jsc/bindings/ZigGlobalObject.cpp index feffb2695fdf..8fbea544e988 100644 --- a/src/jsc/bindings/ZigGlobalObject.cpp +++ b/src/jsc/bindings/ZigGlobalObject.cpp @@ -738,14 +738,8 @@ static bool isModuleEvaluating(JSC::AbstractModuleRecord* record) return cyclic && cyclic->status() == JSC::CyclicModuleRecord::Status::Evaluating; } -// The JSC loader caches every failure, including a transpile error, an -// unresolved static import, or a link error. That is the browser behavior -// (a module script that fails to fetch or parse is cached as null). Node never -// caches those: its ModuleJob is only added to the load cache once the source -// compiled and every dependency resolved, so the next import() re-reads the -// file. A module that never started evaluating has no side effects that a -// second load could duplicate, so drop the stale record and let the loader -// fetch again. A module whose body threw stays cached, as in Node and the spec. +// Like Node, re-fetch a module whose load failed before it ran; only a module +// whose body threw keeps its error (the spec's [[EvaluationError]]). static bool isFailedEntryThatNeverEvaluated(JSC::ModuleRegistryEntry* entry) { switch (entry->status()) { @@ -753,8 +747,7 @@ static bool isFailedEntryThatNeverEvaluated(JSC::ModuleRegistryEntry* entry) case JSC::ModuleRegistryEntry::Status::InstantiationFailed: return true; case JSC::ModuleRegistryEntry::Status::EvaluationFailed: { - // A dependency that failed to load is stored as an evaluation error on - // the importer even though the importer never linked. + // A dependency that failed to load is stored here on the importer too. auto* record = entry->record(); if (!record) return true; @@ -768,9 +761,8 @@ static bool isFailedEntryThatNeverEvaluated(JSC::ModuleRegistryEntry* entry) static void dropFailedEntryThatNeverEvaluated(Zig::GlobalObject* globalObject, const JSC::Identifier& key) { - // The map is keyed by (specifier, type). Probe each type directly: - // registryEntry() falls back to a scan of the whole map for a key that has - // no JavaScript entry, and this runs on every resolve. + // Probe each (specifier, type) bucket: registryEntry() scans the whole map + // for a key without a JavaScript entry, and this runs on every resolve. static constexpr JSC::ScriptFetchParameters::Type moduleTypes[] = { JSC::ScriptFetchParameters::Type::None, JSC::ScriptFetchParameters::Type::JavaScript, @@ -783,11 +775,8 @@ static void dropFailedEntryThatNeverEvaluated(Zig::GlobalObject* globalObject, c if (!impl) return; - // A top-level load of this key is still settling. The loader records a - // top-level failure in one microtask and reports it in the next, and the - // second one looks the key up by name: it would attach the stale error to - // any entry a new fetch registered in between. Keep the failed entry until - // the load's promise has settled. + // ModuleLoadTopRejected looks the key up by name one microtask after the + // failure is recorded; a fresh entry there would inherit the stale error. if (globalObject->pendingModuleLoads.contains(impl)) return; @@ -798,8 +787,7 @@ static void dropFailedEntryThatNeverEvaluated(Zig::GlobalObject* globalObject, c auto* entry = moduleMap.get({ impl, type }).get(); if (!entry) continue; - // removeEntry drops every type variant of the key, so keep them all - // while any variant holds a module that may have run. + // removeEntry drops every type variant of the key. if (!isFailedEntryThatNeverEvaluated(entry)) return; found = true; @@ -813,8 +801,7 @@ static void dropFailedEntryThatNeverEvaluated(Zig::GlobalObject* globalObject, c loader->removeEntry(key); } -// Reaction on the promise of a top-level module load: argument 1 is the -// resolved key that trackPendingModuleLoad registered in pendingModuleLoads. +// Argument 1 is the resolved key passed by trackPendingModuleLoad. BUN_DEFINE_HOST_FUNCTION(Bun__onModuleLoadSettled, (JSC::JSGlobalObject * globalObject, JSC::CallFrame* callFrame)) { auto& vm = JSC::getVM(globalObject); @@ -832,15 +819,12 @@ void GlobalObject::trackPendingModuleLoad(const JSC::Identifier& key, JSC::JSPro { auto& vm = this->vm(); pendingModuleLoads.add(key.impl()); - // The key string shares the Identifier's atom, so the handler gets the same - // UniquedStringImpl back from Identifier::fromString. + // The string shares the Identifier's atom, so the handler gets the same impl back. JSFunction* onSettled = thenable(Bun__onModuleLoadSettled); promise->performPromiseThenWithContext(vm, this, onSettled, onSettled, jsUndefined(), jsString(vm, key.string())); } -// import() of an already resolved key. The key stays in pendingModuleLoads -// until the returned promise settles, which is after the loader has finished -// touching the registry for this load. See dropFailedEntryThatNeverEvaluated. +// import() of an already resolved key. static JSC::JSPromise* importResolvedModule(Zig::GlobalObject* globalObject, const JSC::Identifier& key, RefPtr&& parameters, int64_t referrerAsyncOrder) { auto& vm = JSC::getVM(globalObject); @@ -3484,8 +3468,7 @@ void GlobalObject::clearModuleRegistry() WTF::Locker locker { moduleLoader->cellLock() }; moduleLoader->clearAll(); } - // A load that was in flight has lost its entry, so its promise may never - // settle. Do not let it pin a failed entry of the next registry. + // An in-flight load lost its entry and may never settle. this->pendingModuleLoads.clear(); this->requireMap()->clear(this); } @@ -3635,10 +3618,8 @@ JSC::Identifier GlobalObject::moduleLoaderResolve(JSGlobalObject* jsGlobalObject JSC::Identifier resolved = resolveModuleKey(globalObject, key, referrer); RETURN_IF_EXCEPTION(scope, {}); - // Every lookup of a module in the registry, whether from import(), a - // static import of a module being linked, or the entry point, goes through - // resolve() first. Evicting here makes the loader fetch the module again - // when its last load never produced a module. + // Every registry lookup, for import(), a static import, or the entry point, + // resolves first, so this is where a failed load is retried. dropFailedEntryThatNeverEvaluated(globalObject, resolved); return resolved; } diff --git a/src/jsc/bindings/ZigGlobalObject.h b/src/jsc/bindings/ZigGlobalObject.h index aef9c3264cff..1eedd24b6d5d 100644 --- a/src/jsc/bindings/ZigGlobalObject.h +++ b/src/jsc/bindings/ZigGlobalObject.h @@ -729,13 +729,9 @@ class GlobalObject : public Bun::GlobalScope { BunPlugin::OnResolve onResolvePlugins {}; // Resolved keys of the top-level module loads (import(), Module.runMain) - // whose promise has not settled yet. moduleLoaderResolve must not drop a - // failed registry entry for such a key: the loader's ModuleLoadTopRejected - // microtask still looks the key up by name after the failure is recorded, - // and would store the stale error on a fresh entry. + // whose promise has not settled; moduleLoaderResolve keeps their failed + // registry entries until then. WTF::HashCountedSet, JSC::IdentifierRepHash> pendingModuleLoads; - // Keeps `key` in pendingModuleLoads until `promise`, the result of a - // top-level load of that key, settles. void trackPendingModuleLoad(const JSC::Identifier& key, JSC::JSPromise* promise); // This increases the cache hit rate for JSC::VM's SourceProvider cache diff --git a/src/jsc/modules/NodeModuleModule.cpp b/src/jsc/modules/NodeModuleModule.cpp index beaf2f8e8b11..965fa4101597 100644 --- a/src/jsc/modules/NodeModuleModule.cpp +++ b/src/jsc/modules/NodeModuleModule.cpp @@ -786,8 +786,7 @@ JSC_DEFINE_HOST_FUNCTION(jsFunctionRunMain, (JSGlobalObject * lexicalGlobalObjec auto name = arg1.toWTFString(globalObject); RETURN_IF_EXCEPTION(scope, {}); - // Resolve first: the load is a top-level load like import() and is tracked - // under the key the loader uses while its promise is pending. + // Resolve first so the load is tracked under the loader's key, as import() is. auto key = Zig::GlobalObject::moduleLoaderResolve(globalObject, globalObject->moduleLoader(), JSC::jsString(vm, name), JSC::jsUndefined(), nullptr, false); RETURN_IF_EXCEPTION(scope, {}); From b8665de69ac81deb1d4f42f82e6fe315d2c61ba4 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Mon, 24 Aug 2026 01:02:00 +0000 Subject: [PATCH 04/11] test: compare trimmed stderr --- test/js/bun/resolve/import-retry-after-failure.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/js/bun/resolve/import-retry-after-failure.test.ts b/test/js/bun/resolve/import-retry-after-failure.test.ts index 49fbc01f6711..d62631e755eb 100644 --- a/test/js/bun/resolve/import-retry-after-failure.test.ts +++ b/test/js/bun/resolve/import-retry-after-failure.test.ts @@ -141,6 +141,6 @@ test("import() of a module that failed to load retries after the file changes", E1 ERR boom E2 ERR boom" `); - expect(stderr).toBe(""); + expect(stderr.trim()).toBe(""); expect(exitCode).toBe(0); }); From 13683963593c22e1accc22ba6057a0639cee27a4 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Mon, 24 Aug 2026 02:25:25 +0000 Subject: [PATCH 05/11] test: cover the Module.runMain load in the same-tick retry case --- .../bun/resolve/import-retry-after-failure.test.ts | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/test/js/bun/resolve/import-retry-after-failure.test.ts b/test/js/bun/resolve/import-retry-after-failure.test.ts index d62631e755eb..44e85b82cff2 100644 --- a/test/js/bun/resolve/import-retry-after-failure.test.ts +++ b/test/js/bun/resolve/import-retry-after-failure.test.ts @@ -9,6 +9,7 @@ import { bunEnv, bunExe, normalizeBunSnapshot, tempDir } from "harness"; // module whose body threw stays cached (that is the spec, and Node does it too). test("import() of a module that failed to load retries after the file changes", async () => { using dir = tempDir("import-retry", { + "h3.virt": "", "main.mjs": ` import fs from "node:fs"; import { createRequire } from "node:module"; @@ -99,6 +100,16 @@ test("import() of a module that failed to load retries after the file changes", console.log("H2", h2.map(r => (r.status === "fulfilled" ? "OK " + r.value.v : "ERR")).join(" ")); await t("H2b", "./h2.virt"); console.log("H2 loads", loads.get("h2.virt")); + // Module.runMain starts the same kind of top-level load as import(). + // h3.virt exists before bun starts: runMain resolves with no referrer and + // does not re-read a directory listed before the file was written. + require("module").runMain("./h3.virt"); + const h3 = await Promise.resolve() + .then(() => import("./h3.virt")) + .then(ns => "OK " + ns.v, () => "ERR"); + console.log("H3", h3); + await t("H3b", "./h3.virt"); + console.log("H3 loads", loads.get("h3.virt")); // A module whose body threw stays cached even after the file changes. fs.writeFileSync("e.mjs", 'throw new Error("boom"); export const v = 1;'); @@ -138,6 +149,9 @@ test("import() of a module that failed to load retries after the file changes", H2 ERR ERR H2b OK 42 H2 loads 2 + H3 ERR + H3b OK 42 + H3 loads 2 E1 ERR boom E2 ERR boom" `); From cc2410434d1e1222f43b3504da59ed7e2b059792 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 27 Aug 2026 02:55:04 +0000 Subject: [PATCH 06/11] Join an in-flight import() of the same key instead of starting a second load Since WebKit 8c4fd56347 a top-level load registers its entry only once the fetch succeeds, and ModuleLoadTopRejected records a failure by key one microtask after it happened. Two import() calls of one failing module in the same tick made the second load's fresh entry inherit the first load's error: the module failed for the rest of the process in release builds, and fetchComplete's status assertion fired in debug builds. Keep the in-flight promise per key in a WeakGCMap and hand a second import() a promise piped from it, as Node shares the in-flight job. --- src/jsc/bindings/ZigGlobalObject.cpp | 25 ++++++++++++++++--- src/jsc/bindings/ZigGlobalObject.h | 14 +++++++---- .../import-retry-after-failure.test.ts | 20 +++++++++++---- 3 files changed, 45 insertions(+), 14 deletions(-) diff --git a/src/jsc/bindings/ZigGlobalObject.cpp b/src/jsc/bindings/ZigGlobalObject.cpp index 8fbea544e988..d745ac4a849b 100644 --- a/src/jsc/bindings/ZigGlobalObject.cpp +++ b/src/jsc/bindings/ZigGlobalObject.cpp @@ -777,7 +777,7 @@ static void dropFailedEntryThatNeverEvaluated(Zig::GlobalObject* globalObject, c // ModuleLoadTopRejected looks the key up by name one microtask after the // failure is recorded; a fresh entry there would inherit the stale error. - if (globalObject->pendingModuleLoads.contains(impl)) + if (globalObject->pendingModuleLoad(key)) return; auto* loader = globalObject->moduleLoader(); @@ -801,7 +801,8 @@ static void dropFailedEntryThatNeverEvaluated(Zig::GlobalObject* globalObject, c loader->removeEntry(key); } -// Argument 1 is the resolved key passed by trackPendingModuleLoad. +// Reaction on a tracked load's promise. Argument 1 is the resolved key passed +// by trackPendingModuleLoad. BUN_DEFINE_HOST_FUNCTION(Bun__onModuleLoadSettled, (JSC::JSGlobalObject * globalObject, JSC::CallFrame* callFrame)) { auto& vm = JSC::getVM(globalObject); @@ -811,14 +812,18 @@ BUN_DEFINE_HOST_FUNCTION(Bun__onModuleLoadSettled, (JSC::JSGlobalObject * global return JSValue::encode(jsUndefined()); auto key = JSC::Identifier::fromString(vm, keyString->value(globalObject)); RETURN_IF_EXCEPTION(scope, {}); - static_cast(globalObject)->pendingModuleLoads.remove(key.impl()); + auto* thisObject = static_cast(globalObject); + // Module.runMain may have started a newer load of the key: keep that one. + auto* pending = thisObject->pendingModuleLoad(key); + if (!pending || pending->status() != JSC::JSPromise::Status::Pending) + thisObject->pendingModuleLoads.remove(key.impl()); return JSValue::encode(jsUndefined()); } void GlobalObject::trackPendingModuleLoad(const JSC::Identifier& key, JSC::JSPromise* promise) { auto& vm = this->vm(); - pendingModuleLoads.add(key.impl()); + pendingModuleLoads.set(key.impl(), JSC::Weak(promise)); // The string shares the Identifier's atom, so the handler gets the same impl back. JSFunction* onSettled = thenable(Bun__onModuleLoadSettled); promise->performPromiseThenWithContext(vm, this, onSettled, onSettled, jsUndefined(), jsString(vm, key.string())); @@ -830,6 +835,16 @@ static JSC::JSPromise* importResolvedModule(Zig::GlobalObject* globalObject, con auto& vm = JSC::getVM(globalObject); auto scope = DECLARE_THROW_SCOPE(vm); + // A load of this key is in flight: join it, as Node shares the in-flight + // job. A second top-level load would fetch again, and the loader reports + // the first load's failure by key one microtask later, onto whatever entry + // the second load registered in between. + if (auto* pending = globalObject->pendingModuleLoad(key)) { + auto* joined = JSC::JSPromise::create(vm, globalObject->promiseStructure()); + joined->pipeFrom(vm, pending); + return joined; + } + dropFailedEntryThatNeverEvaluated(globalObject, key); auto* result = JSC::importModule(globalObject, key, JSC::Identifier(), WTF::move(parameters), nullptr, /* deferred */ false, referrerAsyncOrder); if (scope.exception()) [[unlikely]] @@ -1145,6 +1160,7 @@ GlobalObject::GlobalObject(JSC::VM& vm, JSC::Structure* structure, const JSC::Gl , m_builtinInternalFunctions(makeUnique(vm)) , m_scriptExecutionContext(new WebCore::ScriptExecutionContext(&vm, this)) , globalEventScope(adoptRef(*new Bun::GlobalEventScope(m_scriptExecutionContext))) + , pendingModuleLoads(vm) { // m_scriptExecutionContext = globalEventScope.m_context; mockModule = Bun::JSMockModule::create(this); @@ -1159,6 +1175,7 @@ GlobalObject::GlobalObject(JSC::VM& vm, JSC::Structure* structure, WebCore::Scri , m_builtinInternalFunctions(makeUnique(vm)) , m_scriptExecutionContext(new WebCore::ScriptExecutionContext(&vm, this, contextId)) , globalEventScope(adoptRef(*new Bun::GlobalEventScope(m_scriptExecutionContext))) + , pendingModuleLoads(vm) { // m_scriptExecutionContext = globalEventScope.m_context; mockModule = Bun::JSMockModule::create(this); diff --git a/src/jsc/bindings/ZigGlobalObject.h b/src/jsc/bindings/ZigGlobalObject.h index 1eedd24b6d5d..f707fd68d0c6 100644 --- a/src/jsc/bindings/ZigGlobalObject.h +++ b/src/jsc/bindings/ZigGlobalObject.h @@ -56,7 +56,8 @@ struct node_module; #include #include #include -#include +#include +#include #include #include #include "DOMConstructors.h" @@ -728,10 +729,13 @@ class GlobalObject : public Bun::GlobalScope { BunPlugin::OnLoad onLoadPlugins {}; BunPlugin::OnResolve onResolvePlugins {}; - // Resolved keys of the top-level module loads (import(), Module.runMain) - // whose promise has not settled; moduleLoaderResolve keeps their failed - // registry entries until then. - WTF::HashCountedSet, JSC::IdentifierRepHash> pendingModuleLoads; + // The top-level module loads (import(), Module.runMain) whose promise has + // not settled, by resolved key. A second import() of the key joins the load + // instead of starting another, and moduleLoaderResolve keeps the key's + // failed registry entry until then. Weak: a load that can still settle is + // reachable from its own reaction chain. + JSC::WeakGCMap, JSC::JSPromise, JSC::IdentifierRepHash> pendingModuleLoads; + JSC::JSPromise* pendingModuleLoad(const JSC::Identifier& key) const { return pendingModuleLoads.get(key.impl()); } void trackPendingModuleLoad(const JSC::Identifier& key, JSC::JSPromise* promise); // This increases the cache hit rate for JSC::VM's SourceProvider cache diff --git a/test/js/bun/resolve/import-retry-after-failure.test.ts b/test/js/bun/resolve/import-retry-after-failure.test.ts index 44e85b82cff2..89154a3df033 100644 --- a/test/js/bun/resolve/import-retry-after-failure.test.ts +++ b/test/js/bun/resolve/import-retry-after-failure.test.ts @@ -7,6 +7,9 @@ import { bunEnv, bunExe, normalizeBunSnapshot, tempDir } from "harness"; // import() still rejected with the stale error. Node re-reads the file on the // next import() because it never caches a module that failed to load. Only a // module whose body threw stays cached (that is the spec, and Node does it too). +// Two import() calls of the same key in one tick must share one load: the +// loader reports the first load's failure by key one microtask later, and a +// second load's fresh entry would keep that error for the whole process. test("import() of a module that failed to load retries after the file changes", async () => { using dir = tempDir("import-retry", { "h3.virt": "", @@ -84,16 +87,22 @@ test("import() of a module that failed to load retries after the file changes", console.log("P loads", loads.get("p.virt")); // With a plugin registered every load runs on this thread, so the two - // loads below settle in the same microtask checkpoint. The failed entry - // must stay in the registry until the first import() has settled: the - // loader reports a top-level failure one microtask after it records it, - // by key, so a fetch registered in between would inherit the stale error. + // loads below settle in the same microtask checkpoint. A second import() + // of a key whose load is in flight joins that load, as in Node. A second + // top-level load would fetch again, and the loader reports the first + // load's failure by key one microtask later, onto the entry the second + // load registered: that module then fails forever (or trips + // ModuleRegistryEntry::fetchComplete's status assertion). // Two import() calls in the same tick, then a retry after both settled. fs.writeFileSync("h1.virt", ""); const h1 = await Promise.allSettled([import("./h1.virt"), import("./h1.virt")]); console.log("H1", h1.map(r => (r.status === "fulfilled" ? "OK " + r.value.v : "ERR")).join(" ")); await t("H1b", "./h1.virt"); console.log("H1 loads", loads.get("h1.virt")); + // The same for a load that succeeds: both get the one namespace. + fs.writeFileSync("j.mjs", "export const v = 42;"); + const j = await Promise.all([import("./j.mjs"), import("./j.mjs")]); + console.log("J", j[0].v, j[1].v, j[0] === j[1]); // An import() issued from a microtask queued right after the failing one. fs.writeFileSync("h2.virt", ""); const h2 = await Promise.allSettled([import("./h2.virt"), Promise.resolve().then(() => import("./h2.virt"))]); @@ -145,7 +154,8 @@ test("import() of a module that failed to load retries after the file changes", P loads 2 H1 ERR ERR H1b OK 42 - H1 loads 3 + H1 loads 2 + J 42 42 true H2 ERR ERR H2b OK 42 H2 loads 2 From 69d94dd52bd550e4554513af2b5979a1cc44d564 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 27 Aug 2026 03:49:44 +0000 Subject: [PATCH 07/11] ci: retrigger From 4059d38eb426b0a292ef843cc305629dbab4d346 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 27 Aug 2026 04:13:01 +0000 Subject: [PATCH 08/11] Key pending module loads by module type and join a runMain load with its namespace A second import() joined any in-flight load of the same key. The loader's registry is keyed by (key, type), so import("./d.json") and import("./d.json", { with: { type: "text" } }) in one tick gave the second the first one's module. Key the pending loads the same way, and keep a failed entry while a load of any type of the key is pending. The promise loadAndEvaluateModule returns for Module.runMain settles with the evaluation result. An import() that joined it got that value instead of the namespace. Track a derived promise that fulfills with the key's namespace, marked handled because the VM reports a rejection through the original one. --- src/jsc/bindings/ZigGlobalObject.cpp | 80 +++++++++++++------ src/jsc/bindings/ZigGlobalObject.h | 32 ++++++-- src/jsc/bindings/headers.h | 2 + src/jsc/modules/NodeModuleModule.cpp | 8 +- .../import-retry-after-failure.test.ts | 19 ++++- 5 files changed, 109 insertions(+), 32 deletions(-) diff --git a/src/jsc/bindings/ZigGlobalObject.cpp b/src/jsc/bindings/ZigGlobalObject.cpp index d745ac4a849b..7beeea0b8e5e 100644 --- a/src/jsc/bindings/ZigGlobalObject.cpp +++ b/src/jsc/bindings/ZigGlobalObject.cpp @@ -759,27 +759,24 @@ static bool isFailedEntryThatNeverEvaluated(JSC::ModuleRegistryEntry* entry) } } +// The loader's registry is keyed by (specifier, type). +static constexpr JSC::ScriptFetchParameters::Type moduleTypes[] = { + JSC::ScriptFetchParameters::Type::None, + JSC::ScriptFetchParameters::Type::JavaScript, + JSC::ScriptFetchParameters::Type::WebAssembly, + JSC::ScriptFetchParameters::Type::JSON, + JSC::ScriptFetchParameters::Type::Text, + JSC::ScriptFetchParameters::Type::HostDefined, +}; + static void dropFailedEntryThatNeverEvaluated(Zig::GlobalObject* globalObject, const JSC::Identifier& key) { - // Probe each (specifier, type) bucket: registryEntry() scans the whole map - // for a key without a JavaScript entry, and this runs on every resolve. - static constexpr JSC::ScriptFetchParameters::Type moduleTypes[] = { - JSC::ScriptFetchParameters::Type::None, - JSC::ScriptFetchParameters::Type::JavaScript, - JSC::ScriptFetchParameters::Type::WebAssembly, - JSC::ScriptFetchParameters::Type::JSON, - JSC::ScriptFetchParameters::Type::Text, - JSC::ScriptFetchParameters::Type::HostDefined, - }; auto* impl = key.impl(); if (!impl) return; - // ModuleLoadTopRejected looks the key up by name one microtask after the - // failure is recorded; a fresh entry there would inherit the stale error. - if (globalObject->pendingModuleLoad(key)) - return; - + // Probe each type bucket: registryEntry() scans the whole map for a key + // without a JavaScript entry, and this runs on every resolve. auto* loader = globalObject->moduleLoader(); bool found = false; const auto& moduleMap = loader->moduleMap(); @@ -795,12 +792,26 @@ static void dropFailedEntryThatNeverEvaluated(Zig::GlobalObject* globalObject, c if (!found) return; + // ModuleLoadTopRejected looks the key up by name one microtask after the + // failure is recorded; a fresh entry there would inherit the stale error. + if (globalObject->hasPendingModuleLoad(key)) + return; + // JSModuleLoader::visitChildrenImpl iterates these maps on the GC thread // under cellLock(); take the same lock so the removal can't race it. WTF::Locker locker { loader->cellLock() }; loader->removeEntry(key); } +bool GlobalObject::hasPendingModuleLoad(const JSC::Identifier& key) const +{ + for (auto type : moduleTypes) { + if (pendingModuleLoads.get({ key.impl(), type })) + return true; + } + return false; +} + // Reaction on a tracked load's promise. Argument 1 is the resolved key passed // by trackPendingModuleLoad. BUN_DEFINE_HOST_FUNCTION(Bun__onModuleLoadSettled, (JSC::JSGlobalObject * globalObject, JSC::CallFrame* callFrame)) @@ -813,17 +824,37 @@ BUN_DEFINE_HOST_FUNCTION(Bun__onModuleLoadSettled, (JSC::JSGlobalObject * global auto key = JSC::Identifier::fromString(vm, keyString->value(globalObject)); RETURN_IF_EXCEPTION(scope, {}); auto* thisObject = static_cast(globalObject); - // Module.runMain may have started a newer load of the key: keep that one. - auto* pending = thisObject->pendingModuleLoad(key); - if (!pending || pending->status() != JSC::JSPromise::Status::Pending) - thisObject->pendingModuleLoads.remove(key.impl()); + // Module.runMain may have started a newer load of the key: keep a pending one. + for (auto type : moduleTypes) { + Zig::PendingModuleLoadKey mapKey { key.impl(), type }; + auto* pending = thisObject->pendingModuleLoads.get(mapKey); + if (pending && pending->status() != JSC::JSPromise::Status::Pending) + thisObject->pendingModuleLoads.remove(mapKey); + } return JSValue::encode(jsUndefined()); } -void GlobalObject::trackPendingModuleLoad(const JSC::Identifier& key, JSC::JSPromise* promise) +// Fulfillment reaction on a Module.runMain load, whose promise settles with the +// evaluation result. Argument 1 is the resolved key; returns its namespace. +BUN_DEFINE_HOST_FUNCTION(Bun__moduleNamespaceForKey, (JSC::JSGlobalObject * globalObject, JSC::CallFrame* callFrame)) +{ + auto& vm = JSC::getVM(globalObject); + auto scope = DECLARE_THROW_SCOPE(vm); + auto* keyString = dynamicDowncast(callFrame->argument(1)); + if (!keyString) [[unlikely]] + return JSValue::encode(jsUndefined()); + auto key = JSC::Identifier::fromString(vm, keyString->value(globalObject)); + RETURN_IF_EXCEPTION(scope, {}); + auto* entry = globalObject->moduleLoader()->moduleMap().get({ key.impl(), JSC::ScriptFetchParameters::Type::JavaScript }).get(); + if (!entry || !entry->record()) + return JSValue::encode(jsUndefined()); + RELEASE_AND_RETURN(scope, JSValue::encode(entry->record()->getModuleNamespace(globalObject))); +} + +void GlobalObject::trackPendingModuleLoad(const JSC::Identifier& key, JSC::ScriptFetchParameters::Type type, JSC::JSPromise* promise) { auto& vm = this->vm(); - pendingModuleLoads.set(key.impl(), JSC::Weak(promise)); + pendingModuleLoads.set({ key.impl(), type }, JSC::Weak(promise)); // The string shares the Identifier's atom, so the handler gets the same impl back. JSFunction* onSettled = thenable(Bun__onModuleLoadSettled); promise->performPromiseThenWithContext(vm, this, onSettled, onSettled, jsUndefined(), jsString(vm, key.string())); @@ -839,7 +870,8 @@ static JSC::JSPromise* importResolvedModule(Zig::GlobalObject* globalObject, con // job. A second top-level load would fetch again, and the loader reports // the first load's failure by key one microtask later, onto whatever entry // the second load registered in between. - if (auto* pending = globalObject->pendingModuleLoad(key)) { + auto type = parameters ? parameters->type() : JSC::ScriptFetchParameters::Type::JavaScript; + if (auto* pending = globalObject->pendingModuleLoad(key, type)) { auto* joined = JSC::JSPromise::create(vm, globalObject->promiseStructure()); joined->pipeFrom(vm, pending); return joined; @@ -850,7 +882,7 @@ static JSC::JSPromise* importResolvedModule(Zig::GlobalObject* globalObject, con if (scope.exception()) [[unlikely]] return JSC::JSPromise::rejectedPromiseWithCaughtException(globalObject, scope); ASSERT(result); - globalObject->trackPendingModuleLoad(key, result); + globalObject->trackPendingModuleLoad(key, type, result); return result; } @@ -4334,6 +4366,8 @@ GlobalObject::PromiseFunctions GlobalObject::promiseHandlerID(Zig::FFIFunction h return GlobalObject::PromiseFunctions::Bun__HTMLRewriter__onRejectInputStream; } else if (handler == Bun__onModuleLoadSettled) { return GlobalObject::PromiseFunctions::Bun__onModuleLoadSettled; + } else if (handler == Bun__moduleNamespaceForKey) { + return GlobalObject::PromiseFunctions::Bun__moduleNamespaceForKey; } else { RELEASE_ASSERT_NOT_REACHED(); } diff --git a/src/jsc/bindings/ZigGlobalObject.h b/src/jsc/bindings/ZigGlobalObject.h index f707fd68d0c6..99dcbb2ecac1 100644 --- a/src/jsc/bindings/ZigGlobalObject.h +++ b/src/jsc/bindings/ZigGlobalObject.h @@ -57,6 +57,7 @@ struct node_module; #include #include #include +#include #include #include #include @@ -104,6 +105,18 @@ class JSCStackTrace; using DOMGuardedObjectSet = UncheckedKeyHashSet; +// JSC::ModuleMapKey with an owning key, for a map whose values are weak. +using PendingModuleLoadKey = std::pair, JSC::ScriptFetchParameters::Type>; + +struct PendingModuleLoadKeyHash { + static unsigned hash(const PendingModuleLoadKey& key) + { + return WTF::pairIntHash(key.first ? key.first->existingSymbolAwareHash() : 0, static_cast(key.second)); + } + static bool equal(const PendingModuleLoadKey& a, const PendingModuleLoadKey& b) { return a.first == b.first && a.second == b.second; } + static constexpr bool safeToCompareToEmptyOrDeleted = false; +}; + class GlobalObject : public Bun::GlobalScope { using Base = Bun::GlobalScope; @@ -414,6 +427,7 @@ class GlobalObject : public Bun::GlobalScope { Bun__HTMLRewriter__onResolveInputStream, Bun__HTMLRewriter__onRejectInputStream, Bun__onModuleLoadSettled, + Bun__moduleNamespaceForKey, Count_, }; static constexpr size_t promiseFunctionsSize = static_cast(PromiseFunctions::Count_); @@ -730,13 +744,19 @@ class GlobalObject : public Bun::GlobalScope { BunPlugin::OnResolve onResolvePlugins {}; // The top-level module loads (import(), Module.runMain) whose promise has - // not settled, by resolved key. A second import() of the key joins the load - // instead of starting another, and moduleLoaderResolve keeps the key's - // failed registry entry until then. Weak: a load that can still settle is + // not settled, keyed like the loader's registry: (resolved key, module + // type). Each promise fulfills with the module namespace. A second import() + // of the same pair joins the load instead of starting another, and + // moduleLoaderResolve keeps the key's failed registry entries while any + // load of the key is pending. Weak: a load that can still settle is // reachable from its own reaction chain. - JSC::WeakGCMap, JSC::JSPromise, JSC::IdentifierRepHash> pendingModuleLoads; - JSC::JSPromise* pendingModuleLoad(const JSC::Identifier& key) const { return pendingModuleLoads.get(key.impl()); } - void trackPendingModuleLoad(const JSC::Identifier& key, JSC::JSPromise* promise); + JSC::WeakGCMap pendingModuleLoads; + JSC::JSPromise* pendingModuleLoad(const JSC::Identifier& key, JSC::ScriptFetchParameters::Type type) const + { + return pendingModuleLoads.get({ key.impl(), type }); + } + bool hasPendingModuleLoad(const JSC::Identifier& key) const; + void trackPendingModuleLoad(const JSC::Identifier& key, JSC::ScriptFetchParameters::Type type, JSC::JSPromise* promise); // This increases the cache hit rate for JSC::VM's SourceProvider cache // It also avoids an extra allocation for the SourceProvider diff --git a/src/jsc/bindings/headers.h b/src/jsc/bindings/headers.h index 84214de787ab..7070199770da 100644 --- a/src/jsc/bindings/headers.h +++ b/src/jsc/bindings/headers.h @@ -781,5 +781,7 @@ BUN_DECLARE_HOST_FUNCTION(Bun__S3UploadStream__onRejectStream); BUN_DECLARE_HOST_FUNCTION(Bun__HTMLRewriter__onResolveInputStream); BUN_DECLARE_HOST_FUNCTION(Bun__HTMLRewriter__onRejectInputStream); +BUN_DECLARE_HOST_FUNCTION(Bun__onModuleLoadSettled); +BUN_DECLARE_HOST_FUNCTION(Bun__moduleNamespaceForKey); #endif diff --git a/src/jsc/modules/NodeModuleModule.cpp b/src/jsc/modules/NodeModuleModule.cpp index 965fa4101597..73c485d7c428 100644 --- a/src/jsc/modules/NodeModuleModule.cpp +++ b/src/jsc/modules/NodeModuleModule.cpp @@ -792,7 +792,13 @@ JSC_DEFINE_HOST_FUNCTION(jsFunctionRunMain, (JSGlobalObject * lexicalGlobalObjec auto* promise = JSC::loadAndEvaluateModule(globalObject, key.string(), nullptr, nullptr); RETURN_IF_EXCEPTION(scope, {}); - globalObject->trackPendingModuleLoad(key, promise); + // The tracked promise must settle like an import() promise: with the + // module namespace, not the evaluation result. The VM reports a rejection + // through `promise`, so this one is marked handled. + auto* namespacePromise = JSC::JSPromise::create(vm, globalObject->promiseStructure()); + namespacePromise->markAsHandled(); + promise->performPromiseThenWithContext(vm, globalObject, globalObject->thenable(Bun__moduleNamespaceForKey), JSC::jsUndefined(), namespacePromise, JSC::jsString(vm, key.string())); + globalObject->trackPendingModuleLoad(key, JSC::ScriptFetchParameters::Type::JavaScript, namespacePromise); Bun__VirtualMachine__setOverrideModuleRunMainPromise(globalObject->bunVM(), promise); return JSC::JSValue::encode(JSC::jsUndefined()); diff --git a/test/js/bun/resolve/import-retry-after-failure.test.ts b/test/js/bun/resolve/import-retry-after-failure.test.ts index 89154a3df033..2155adc65214 100644 --- a/test/js/bun/resolve/import-retry-after-failure.test.ts +++ b/test/js/bun/resolve/import-retry-after-failure.test.ts @@ -12,7 +12,11 @@ import { bunEnv, bunExe, normalizeBunSnapshot, tempDir } from "harness"; // second load's fresh entry would keep that error for the whole process. test("import() of a module that failed to load retries after the file changes", async () => { using dir = tempDir("import-retry", { + // runMain resolves with no referrer and does not re-read a directory + // listed before the file was written, so these exist before bun starts. "h3.virt": "", + "h5.mjs": "export const v = 42;", + "d.json": '{"v":42}', "main.mjs": ` import fs from "node:fs"; import { createRequire } from "node:module"; @@ -110,8 +114,6 @@ test("import() of a module that failed to load retries after the file changes", await t("H2b", "./h2.virt"); console.log("H2 loads", loads.get("h2.virt")); // Module.runMain starts the same kind of top-level load as import(). - // h3.virt exists before bun starts: runMain resolves with no referrer and - // does not re-read a directory listed before the file was written. require("module").runMain("./h3.virt"); const h3 = await Promise.resolve() .then(() => import("./h3.virt")) @@ -119,6 +121,17 @@ test("import() of a module that failed to load retries after the file changes", console.log("H3", h3); await t("H3b", "./h3.virt"); console.log("H3 loads", loads.get("h3.virt")); + // An import() that joins a runMain load gets the namespace, not runMain's + // evaluation result. + require("module").runMain("./h5.mjs"); + const h5 = await Promise.resolve() + .then(() => import("./h5.mjs")) + .then(ns => "OK " + ns.v, () => "ERR"); + console.log("H5", h5); + // Loads are keyed by module type too: a typed import() in the same tick + // does not join the untyped one. + const d = await Promise.all([import("./d.json"), import("./d.json", { with: { type: "text" } })]); + console.log("D", typeof d[0].default, d[0].default.v, typeof d[1].default, JSON.parse(d[1].default).v); // A module whose body threw stays cached even after the file changes. fs.writeFileSync("e.mjs", 'throw new Error("boom"); export const v = 1;'); @@ -162,6 +175,8 @@ test("import() of a module that failed to load retries after the file changes", H3 ERR H3b OK 42 H3 loads 2 + H5 OK 42 + D object 42 string 42 E1 ERR boom E2 ERR boom" `); From 327b7e24327d0fc789c0427f8629b06a3655d01c Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Mon, 7 Sep 2026 05:20:44 +0000 Subject: [PATCH 09/11] test: K same-tick import() calls of one key share one fetch and one outcome Eight import() calls of a module whose fetches 2 to 8 would fail (a file caught mid-save): with the join there is one plugin load, one namespace for all eight, and the next import() returns it. Without it, main gives OK then seven errors, and the key stays poisoned. --- .../import-retry-after-failure.test.ts | 23 +++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/test/js/bun/resolve/import-retry-after-failure.test.ts b/test/js/bun/resolve/import-retry-after-failure.test.ts index 2155adc65214..0d632696551c 100644 --- a/test/js/bun/resolve/import-retry-after-failure.test.ts +++ b/test/js/bun/resolve/import-retry-after-failure.test.ts @@ -73,15 +73,19 @@ test("import() of a module that failed to load retries after the file changes", } // A plugin onLoad hook that returned unparseable code runs again. The - // first load of each .virt file fails, every later one succeeds. + // first load of each .virt file fails, every later one succeeds. k.virt + // is good on its first load, bad on loads 2 to 8, then good again: a + // file caught mid-save by some of the callers. const loads = new Map(); Bun.plugin({ name: "retry", setup(build) { build.onLoad({ filter: /\\.virt$/ }, ({ path }) => { - const n = (loads.get(basename(path)) ?? 0) + 1; - loads.set(basename(path), n); - return { contents: n === 1 ? "export const v == 1;" : "export const v = 42;", loader: "js" }; + const name = basename(path); + const n = (loads.get(name) ?? 0) + 1; + loads.set(name, n); + const bad = name === "k.virt" ? n > 1 && n <= 8 : n === 1; + return { contents: bad ? "export const v == 1;" : "export const v = 42;", loader: "js" }; }); }, }); @@ -107,6 +111,14 @@ test("import() of a module that failed to load retries after the file changes", fs.writeFileSync("j.mjs", "export const v = 42;"); const j = await Promise.all([import("./j.mjs"), import("./j.mjs")]); console.log("J", j[0].v, j[1].v, j[0] === j[1]); + // K callers of one key share one fetch: one outcome for all of them and + // one transpile, not K. Without the join, a sibling fetch that failed + // after another one evaluated (a file caught mid-save) poisoned the key. + fs.writeFileSync("k.virt", ""); + const k = await Promise.allSettled(Array.from({ length: 8 }, () => import("./k.virt"))); + console.log("K", k.map(r => (r.status === "fulfilled" ? "OK" : "ERR")).join(" "), k.every(r => r.value === k[0].value)); + await t("Kb", "./k.virt"); + console.log("K loads", loads.get("k.virt")); // An import() issued from a microtask queued right after the failing one. fs.writeFileSync("h2.virt", ""); const h2 = await Promise.allSettled([import("./h2.virt"), Promise.resolve().then(() => import("./h2.virt"))]); @@ -169,6 +181,9 @@ test("import() of a module that failed to load retries after the file changes", H1b OK 42 H1 loads 2 J 42 42 true + K OK OK OK OK OK OK OK OK true + Kb OK 42 + K loads 1 H2 ERR ERR H2b OK 42 H2 loads 2 From a6f88fc23d95771f7c3e5e5e54fbf7816d97d8c6 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Tue, 15 Sep 2026 15:55:23 +0000 Subject: [PATCH 10/11] Module.runMain joins an in-flight load of the same key A second runMain of the key, or a runMain after an import() of it that has not settled, started another top-level load. It now joins the pending one, as import() does. --- src/jsc/modules/NodeModuleModule.cpp | 7 +++++++ .../bun/resolve/import-retry-after-failure.test.ts | 13 +++++++++++++ 2 files changed, 20 insertions(+) diff --git a/src/jsc/modules/NodeModuleModule.cpp b/src/jsc/modules/NodeModuleModule.cpp index 05e93943f198..54f503649907 100644 --- a/src/jsc/modules/NodeModuleModule.cpp +++ b/src/jsc/modules/NodeModuleModule.cpp @@ -797,6 +797,13 @@ JSC_DEFINE_HOST_FUNCTION(jsFunctionRunMain, (JSGlobalObject * lexicalGlobalObjec auto key = Zig::GlobalObject::moduleLoaderResolve(globalObject, globalObject->moduleLoader(), JSC::jsString(vm, name), JSC::jsUndefined(), nullptr, false); RETURN_IF_EXCEPTION(scope, {}); + // A load of this key is in flight, from import() or an earlier runMain: join + // it, for the reason importResolvedModule does. + if (auto* pending = globalObject->pendingModuleLoad(key, JSC::ScriptFetchParameters::Type::JavaScript)) { + Bun__VirtualMachine__setOverrideModuleRunMainPromise(globalObject->bunVM(), pending); + return JSC::JSValue::encode(JSC::jsUndefined()); + } + auto* promise = JSC::loadAndEvaluateModule(globalObject, key.string(), nullptr, nullptr); RETURN_IF_EXCEPTION(scope, {}); // The tracked promise must settle like an import() promise: with the diff --git a/test/js/bun/resolve/import-retry-after-failure.test.ts b/test/js/bun/resolve/import-retry-after-failure.test.ts index 0d632696551c..d731d1c76252 100644 --- a/test/js/bun/resolve/import-retry-after-failure.test.ts +++ b/test/js/bun/resolve/import-retry-after-failure.test.ts @@ -16,6 +16,7 @@ test("import() of a module that failed to load retries after the file changes", // listed before the file was written, so these exist before bun starts. "h3.virt": "", "h5.mjs": "export const v = 42;", + "h6.virt": "", "d.json": '{"v":42}', "main.mjs": ` import fs from "node:fs"; @@ -140,6 +141,15 @@ test("import() of a module that failed to load retries after the file changes", .then(() => import("./h5.mjs")) .then(ns => "OK " + ns.v, () => "ERR"); console.log("H5", h5); + // Two runMain calls of one key share the load as well. + require("module").runMain("./h6.virt"); + require("module").runMain("./h6.virt"); + const h6 = await Promise.resolve() + .then(() => import("./h6.virt")) + .then(ns => "OK " + ns.v, () => "ERR"); + console.log("H6", h6); + await t("H6b", "./h6.virt"); + console.log("H6 loads", loads.get("h6.virt")); // Loads are keyed by module type too: a typed import() in the same tick // does not join the untyped one. const d = await Promise.all([import("./d.json"), import("./d.json", { with: { type: "text" } })]); @@ -191,6 +201,9 @@ test("import() of a module that failed to load retries after the file changes", H3b OK 42 H3 loads 2 H5 OK 42 + H6 ERR + H6b OK 42 + H6 loads 2 D object 42 string 42 E1 ERR boom E2 ERR boom" From 5eaecc519de122b878cad9a3e2d6c921d23f1fc9 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 18 Sep 2026 14:13:54 +0000 Subject: [PATCH 11/11] Keep pending module loads in a map that the GC does not prune The GC prunes a WeakGCMap in its end phase, where JSC clears the thread's atom table. A key of this map owns a ref of the module key atom. When that ref is the last one, the prune destroys the atom, and AtomStringImpl::remove reads the null table: SEGV under Heap::pruneStaleEntriesFromWeakGCHashTables. A top-level load whose own fetch fails registers no entry, so the map's ref is often the last one. Use a HashMap of Weak handles. trackPendingModuleLoad prunes the settled and the collected ones on the JS thread, at a size that doubles. --- src/jsc/bindings/ZigGlobalObject.cpp | 14 +++++++++++-- src/jsc/bindings/ZigGlobalObject.h | 20 +++++++++---------- .../import-retry-after-failure.test.ts | 3 +++ 3 files changed, 25 insertions(+), 12 deletions(-) diff --git a/src/jsc/bindings/ZigGlobalObject.cpp b/src/jsc/bindings/ZigGlobalObject.cpp index 6a48bcd31b3c..00e075d06aa8 100644 --- a/src/jsc/bindings/ZigGlobalObject.cpp +++ b/src/jsc/bindings/ZigGlobalObject.cpp @@ -813,6 +813,18 @@ bool GlobalObject::hasPendingModuleLoad(const JSC::Identifier& key) const return false; } +void GlobalObject::trackPendingModuleLoad(const JSC::Identifier& key, JSC::ScriptFetchParameters::Type type, JSC::JSPromise* promise) +{ + if (pendingModuleLoads.size() >= m_pendingModuleLoadsPruneAt) { + pendingModuleLoads.removeIf([](auto& entry) { + auto* tracked = entry.value.get(); + return !tracked || tracked->status() != JSC::JSPromise::Status::Pending; + }); + m_pendingModuleLoadsPruneAt = std::max(16, pendingModuleLoads.size() * 2); + } + pendingModuleLoads.set(PendingModuleLoadKey { key.impl(), type }, JSC::Weak(promise)); +} + // Fulfillment reaction on a Module.runMain load, whose promise settles with the // evaluation result. Argument 1 is the resolved key; returns its namespace. BUN_DEFINE_HOST_FUNCTION(Bun__moduleNamespaceForKey, (JSC::JSGlobalObject * globalObject, JSC::CallFrame* callFrame)) @@ -1230,7 +1242,6 @@ GlobalObject::GlobalObject(JSC::VM& vm, JSC::Structure* structure, const JSC::Gl , m_builtinInternalFunctions(makeUnique(vm)) , m_scriptExecutionContext(new WebCore::ScriptExecutionContext(&vm, this)) , globalEventScope(adoptRef(*new Bun::GlobalEventScope(m_scriptExecutionContext))) - , pendingModuleLoads(vm) { // m_scriptExecutionContext = globalEventScope.m_context; mockModule = Bun::JSMockModule::create(this); @@ -1245,7 +1256,6 @@ GlobalObject::GlobalObject(JSC::VM& vm, JSC::Structure* structure, WebCore::Scri , m_builtinInternalFunctions(makeUnique(vm)) , m_scriptExecutionContext(new WebCore::ScriptExecutionContext(&vm, this, contextId)) , globalEventScope(adoptRef(*new Bun::GlobalEventScope(m_scriptExecutionContext))) - , pendingModuleLoads(vm) { // m_scriptExecutionContext = globalEventScope.m_context; mockModule = Bun::JSMockModule::create(this); diff --git a/src/jsc/bindings/ZigGlobalObject.h b/src/jsc/bindings/ZigGlobalObject.h index 5c558f54e580..6da10580e1a6 100644 --- a/src/jsc/bindings/ZigGlobalObject.h +++ b/src/jsc/bindings/ZigGlobalObject.h @@ -60,7 +60,8 @@ struct node_module; #include #include #include -#include +#include +#include #include #include #include "DOMConstructors.h" @@ -763,20 +764,19 @@ class GlobalObject : public Bun::GlobalScope { // fulfills with the module namespace. While one is pending, a second // import() of the pair joins it and moduleLoaderResolve keeps the key's // failed registry entries. The loader settles the promise after it has - // recorded a failure, so a settled one guards nothing; it stays until it is - // collected or the pair loads again. Weak: a load that can still settle is - // reachable from its own reaction chain. - JSC::WeakGCMap pendingModuleLoads; + // recorded a failure, so a settled one guards nothing. + // Not a WeakGCMap: the GC prunes those with no atom table set, and a key + // here can hold the last ref of its atom. trackPendingModuleLoad prunes. + WTF::HashMap, PendingModuleLoadKeyHash> pendingModuleLoads; JSC::JSPromise* pendingModuleLoad(const JSC::Identifier& key, JSC::ScriptFetchParameters::Type type) const { - auto* promise = pendingModuleLoads.get({ key.impl(), type }); + auto it = pendingModuleLoads.find({ key.impl(), type }); + auto* promise = it == pendingModuleLoads.end() ? nullptr : it->value.get(); return promise && promise->status() == JSC::JSPromise::Status::Pending ? promise : nullptr; } bool hasPendingModuleLoad(const JSC::Identifier& key) const; - void trackPendingModuleLoad(const JSC::Identifier& key, JSC::ScriptFetchParameters::Type type, JSC::JSPromise* promise) - { - pendingModuleLoads.set({ key.impl(), type }, JSC::Weak(promise)); - } + void trackPendingModuleLoad(const JSC::Identifier& key, JSC::ScriptFetchParameters::Type type, JSC::JSPromise* promise); + size_t m_pendingModuleLoadsPruneAt { 16 }; // This increases the cache hit rate for JSC::VM's SourceProvider cache // It also avoids an extra allocation for the SourceProvider diff --git a/test/js/bun/resolve/import-retry-after-failure.test.ts b/test/js/bun/resolve/import-retry-after-failure.test.ts index d731d1c76252..487d660b92e8 100644 --- a/test/js/bun/resolve/import-retry-after-failure.test.ts +++ b/test/js/bun/resolve/import-retry-after-failure.test.ts @@ -112,6 +112,9 @@ test("import() of a module that failed to load retries after the file changes", fs.writeFileSync("j.mjs", "export const v = 42;"); const j = await Promise.all([import("./j.mjs"), import("./j.mjs")]); console.log("J", j[0].v, j[1].v, j[0] === j[1]); + // The import() promises above are garbage now. Collecting them leaves + // dead handles in the pending-load map, which the loads below prune. + Bun.gc(true); // K callers of one key share one fetch: one outcome for all of them and // one transpile, not K. Without the join, a sibling fetch that failed // after another one evaluated (a file caught mid-save) poisoned the key.