module loader: coalesce concurrent embedder fetches of the same module

src/jsc/bindings/ModuleLoader.cpp

@@ -476,21 +476,9 @@ extern "C" void Bun__onFulfillAsyncModule(
     auto* specifierValue = Bun::toJS(globalObject, *specifier);
     RETURN_IF_EXCEPTION(scope, );
 
-    // The new C++ module loader does not create a registry entry until *after*
-    // this fetch promise resolves (provideFetch runs inside the
-    // ModuleLoadTopSettled microtask). Two concurrent dynamic imports of the
-    // same key therefore each get their own embedder fetch promise, and the
-    // loser of that race must still resolve so its loadModule chain can reach
-    // the (idempotent) provideFetch and reuse the already-loaded record.
-    // The old #6946/#12910 short-circuit was for the JS loader's *shared*
-    // entry.fetch promise; under the new loader returning here would strand
-    // the loser's promise pending forever.
-    //
-    // FIXME(module-loader): the loser still re-transpiled the file. The right
-    // fix is for JSModuleLoader::loadModule to ensureRegistered() *before*
-    // calling fetch so concurrent importers share the entry's fetchPromise
-    // instead of each round-tripping through the embedder.
-
+    // Always settle: moduleLoaderFetch handed this promise to the module loader
+    // and to every importer coalesced onto it, so nothing else resolves it. The
+    // old #6946/#12910 short-circuit was for the JS loader's shared entry.fetch.
     if (res->result.value.isCommonJSModule) {
         auto created = Bun::createCommonJSModule(globalObject, specifierValue, res->result.value);
         EXCEPTION_ASSERT(created.has_value() == !scope.exception());

src/jsc/bindings/ZigGlobalObject.cpp

@@ -2412,6 +2412,12 @@ void GlobalObject::finishCreation(VM& vm)
             init.set(map);
         });
 
+    m_inFlightModuleFetches.initLater(
+        [](const JSC::LazyProperty<JSC::JSGlobalObject, JSC::JSMap>::Initializer& init) {
+            auto* map = JSC::JSMap::create(init.vm, init.owner->mapStructure());
+            init.set(map);
+        });
+
     m_requireFunctionUnbound.initLater(
         [](const JSC::LazyProperty<JSC::JSGlobalObject, JSC::JSObject>::Initializer& init) {
             init.set(
@@ -3326,6 +3332,8 @@ void GlobalObject::reload()
     }
     this->requireMap()->clear(this);
     RETURN_IF_EXCEPTION(scope, );
+    this->clearInFlightModuleFetches();
+    RETURN_IF_EXCEPTION(scope, );
 
     // If we run the GC every time, we will never get the SourceProvider cache hit.
     // So we run the GC every other time.
@@ -3575,6 +3583,58 @@ static JSC::JSPromise* resolvedInternalPromise(JSC::JSGlobalObject* globalObject
     return promise;
 }
 
+// Mirrors the module registry key: (specifier, fetch type, host-defined import
+// type). Length-prefixed so no triple can alias another's flattened key. Coarser
+// keying would share one JSSourceCode across two registry entries. The generation
+// scopes the key to one clearInFlightModuleFetches() epoch.
+static String inFlightModuleFetchKey(unsigned generation, const String& moduleKey, ScriptFetchParameters::Type type, const String& typeAttribute)
+{
+    return makeString(generation, ':', static_cast<unsigned>(type), ':', typeAttribute.length(), ':', typeAttribute, moduleKey);
+}
+
+// Passed as both the fulfill and the reject handler, with the flattened fetch
+// key as the reaction's user context (argument 1).
+JSC_DEFINE_HOST_FUNCTION(jsFunctionInFlightModuleFetchSettled, (JSC::JSGlobalObject * globalObject, JSC::CallFrame* callFrame))
+{
+    auto& vm = JSC::getVM(globalObject);
+    auto scope = DECLARE_THROW_SCOPE(vm);
+    auto* thisObject = defaultGlobalObject(globalObject);
+    thisObject->inFlightModuleFetches()->remove(globalObject, callFrame->argument(1));
+    RETURN_IF_EXCEPTION(scope, {});
+    return JSValue::encode(jsUndefined());
+}
+
+JSC::JSPromise* GlobalObject::inFlightModuleFetch(JSC::JSString* fetchKey)
+{
+    // A miss (and a pending exception) yields jsUndefined(), never an empty
+    // JSValue, so the downcast is the miss check. Callers check the exception.
+    return dynamicDowncast<JSC::JSPromise>(inFlightModuleFetches()->get(this, fetchKey));
+}
+
+void GlobalObject::trackInFlightModuleFetch(JSC::JSString* fetchKey, JSC::JSPromise* promise)
+{
+    auto scope = DECLARE_THROW_SCOPE(vm());
+    inFlightModuleFetches()->set(this, fetchKey, promise);
+    RETURN_IF_EXCEPTION(scope, void());
+
+    // Attached before the loader attaches its own reaction, so the entry is gone
+    // before the module registry entry that supersedes it is created.
+    JSFunction* onSettled = thenable(jsFunctionInFlightModuleFetchSettled);
+    scope.release();
+    promise->performPromiseThenWithContext(vm(), this, onSettled, onSettled, jsUndefined(), fetchKey);
+}
+
+void GlobalObject::clearInFlightModuleFetches()
+{
+    // Clearing the map cannot detach the settle reactions already attached to the
+    // promises it held. Retire the generation so a pre-clear fetch that settles
+    // later removes its own (now absent) key instead of a newer fetch's entry.
+    inFlightModuleFetchGeneration++;
+    if (!m_inFlightModuleFetches.isInitialized())
+        return;
+    inFlightModuleFetches()->clear(this);
+}
+
 JSC::JSPromise* GlobalObject::moduleLoaderFetch(JSGlobalObject* globalObject,
     JSModuleLoader* loader, JSValue key,
     RefPtr<JSC::ScriptFetchParameters> parameters, RefPtr<JSC::ScriptFetcher>)
@@ -3636,8 +3696,19 @@ JSC::JSPromise* GlobalObject::moduleLoaderFetch(JSGlobalObject* globalObject,
         return rejectedInternalPromise(globalObject, result ? result : JSC::jsUndefined());
     }
 
+    // JSC registers the module registry entry (whose fetch promise later
+    // importers share) only once this fetch settles, so coalesce until then:
+    // concurrent dynamic imports of one specifier run the loader exactly once.
+    auto* zigGlobalObject = static_cast<Zig::GlobalObject*>(globalObject);
+    auto fetchType = parameters ? parameters->type() : ScriptFetchParameters::Type::JavaScript;
+    JSString* fetchKey = jsString(vm, inFlightModuleFetchKey(zigGlobalObject->inFlightModuleFetchGeneration, moduleKey, fetchType, typeAttributeString));
+    JSC::JSPromise* inFlight = zigGlobalObject->inFlightModuleFetch(fetchKey);
+    RETURN_IF_EXCEPTION(scope, rejectedInternalPromise(globalObject, scope.exception()->value()));
+    if (inFlight)
+        return inFlight;
+
     JSValue result = Bun::fetchESMSourceCodeAsync(
-        static_cast<Zig::GlobalObject*>(globalObject),
+        zigGlobalObject,
         moduleKeyJS,
         &res,
         &moduleKeyBun,
@@ -3647,6 +3718,8 @@ JSC::JSPromise* GlobalObject::moduleLoaderFetch(JSGlobalObject* globalObject,
     RETURN_IF_EXCEPTION(scope, rejectedInternalPromise(globalObject, scope.exception()->value()));
     ASSERT(result);
     if (auto* promise = dynamicDowncast<JSC::JSPromise>(result)) {
+        zigGlobalObject->trackInFlightModuleFetch(fetchKey, promise);
+        RETURN_IF_EXCEPTION(scope, rejectedInternalPromise(globalObject, scope.exception()->value()));
         return promise;
     }
     return rejectedInternalPromise(globalObject, result);
@@ -3871,6 +3944,8 @@ GlobalObject::PromiseFunctions GlobalObject::promiseHandlerID(Zig::FFIFunction h
         return GlobalObject::PromiseFunctions::Bun__HTTPRequestContextDebugH3__onResolve;
     } else if (handler == Bun__HTTPRequestContextDebugH3__onResolveStream) {
         return GlobalObject::PromiseFunctions::Bun__HTTPRequestContextDebugH3__onResolveStream;
+    } else if (handler == jsFunctionInFlightModuleFetchSettled) {
+        return GlobalObject::PromiseFunctions::jsFunctionInFlightModuleFetchSettled;
     } else {
         RELEASE_ASSERT_NOT_REACHED();
     }

src/jsc/bindings/ZigGlobalObject.h

@@ -279,6 +279,19 @@ class GlobalObject : public Bun::GlobalScope {
     // moduleLoader()->registryEntry(key) / moduleMap() / removeEntry(key) /
     // clearAll() instead.
 
+    // Embedder module fetches that have not settled yet, keyed by module key +
+    // type attribute. JSC only registers a module registry entry once the first
+    // fetch settles, so without this every concurrent import() of one specifier
+    // re-runs the loader. Entries are dropped when the fetch settles.
+    JSC::JSMap* inFlightModuleFetches() const { return m_inFlightModuleFetches.getInitializedOnMainThread(this); }
+    JSC::JSPromise* inFlightModuleFetch(JSC::JSString* fetchKey);
+    void trackInFlightModuleFetch(JSC::JSString* fetchKey, JSC::JSPromise*);
+    void clearInFlightModuleFetches();
+    // Part of the fetch key, retired by clearInFlightModuleFetches(). A fetch left
+    // in flight across a reload settles into a reaction that removes its own key,
+    // which by then must no longer name a live entry.
+    unsigned inFlightModuleFetchGeneration = 0;
+
     JSC::Structure* callSiteStructure() const { return m_callSiteStructure.getInitializedOnMainThread(this); }
 
     JSC::JSObject* performanceObject() const { return m_performanceObject.getInitializedOnMainThread(this); }
@@ -412,8 +425,9 @@ class GlobalObject : public Bun::GlobalScope {
         Bun__HTTPRequestContextDebugH3__onRejectStream,
         Bun__HTTPRequestContextDebugH3__onResolve,
         Bun__HTTPRequestContextDebugH3__onResolveStream,
+        jsFunctionInFlightModuleFetchSettled,
     };
-    static constexpr size_t promiseFunctionsSize = 42;
+    static constexpr size_t promiseFunctionsSize = 43;
 
     static PromiseFunctions promiseHandlerID(SYSV_ABI EncodedJSValue (*handler)(JSC::JSGlobalObject* arg0, JSC::CallFrame* arg1));
 
@@ -595,6 +609,7 @@ class GlobalObject : public Bun::GlobalScope {
     V(private, LazyPropertyOfGlobalObject<JSFunction>, m_wasmStreamingConsumeStreamFunction)                 \
     V(private, LazyPropertyOfGlobalObject<WebCore::JSStreamsRuntime>, m_streamsRuntime)                      \
     V(private, LazyPropertyOfGlobalObject<JSMap>, m_requireMap)                                              \
+    V(private, LazyPropertyOfGlobalObject<JSMap>, m_inFlightModuleFetches)                                   \
     V(private, LazyPropertyOfGlobalObject<JSObject>, m_JSArrayBufferControllerPrototype)                     \
     V(private, LazyPropertyOfGlobalObject<JSObject>, m_JSHTTPSResponseControllerPrototype)                   \
     V(private, LazyPropertyOfGlobalObject<JSObject>, m_JSFetchTaskletChunkedRequestControllerPrototype)      \