require.cache: deleting an ES module that is still loading is a no-op

src/jsc/bindings/ZigGlobalObject.cpp

@@ -738,6 +738,28 @@ static bool isModuleEvaluating(JSC::AbstractModuleRecord* record)
     return cyclic && cyclic->status() == JSC::CyclicModuleRecord::Status::Evaluating;
 }
 
+// No load of this entry is in flight: it evaluated (maybe with an error) or its load failed.
+static bool isModuleLoadSettled(JSC::ModuleRegistryEntry* entry)
+{
+    switch (entry->status()) {
+    case JSC::ModuleRegistryEntry::Status::New:
+    case JSC::ModuleRegistryEntry::Status::Fetching:
+        return false;
+    case JSC::ModuleRegistryEntry::Status::FetchFailed:
+    case JSC::ModuleRegistryEntry::Status::InstantiationFailed:
+    case JSC::ModuleRegistryEntry::Status::EvaluationFailed:
+        return true;
+    case JSC::ModuleRegistryEntry::Status::Fetched:
+        break;
+    }
+    auto* record = entry->record();
+    if (!record)
+        return false;
+    if (auto* cyclic = dynamicDowncast<JSC::CyclicModuleRecord>(record))
+        return cyclic->status() == JSC::CyclicModuleRecord::Status::Evaluated;
+    return record->moduleEnvironmentMayBeNull() != nullptr;
+}
+
 JSC_DEFINE_HOST_FUNCTION(functionEsmNamespaceForCjs, (JSC::JSGlobalObject * globalObject, JSC::CallFrame* callFrame))
 {
     auto& vm = JSC::getVM(globalObject);
@@ -764,7 +786,13 @@ JSC_DEFINE_HOST_FUNCTION(functionEsmRegistryDelete, (JSC::JSGlobalObject * globa
         return JSValue::encode(jsBoolean(false));
     auto key = JSC::Identifier::fromString(vm, asString(keyValue)->value(globalObject));
     RETURN_IF_EXCEPTION(scope, {});
-    return JSValue::encode(jsBoolean(globalObject->moduleLoader()->removeEntry(key))); // takes the loader's cellLock itself
+    auto* moduleLoader = globalObject->moduleLoader();
+    // A module whose load is in flight is not in require.cache and is not evicted.
+    for (auto& [mapKey, entry] : moduleLoader->moduleMap()) {
+        if (mapKey.first == key.impl() && entry && !isModuleLoadSettled(entry.get()))
+            return JSValue::encode(jsBoolean(false));
+    }
+    return JSValue::encode(jsBoolean(moduleLoader->removeEntry(key))); // takes the loader's cellLock itself
 }
 
 JSC_DEFINE_HOST_FUNCTION(functionEsmRegistryEvaluatedKeys, (JSC::JSGlobalObject * globalObject, JSC::CallFrame*))

test/cli/run/require-cache.test.ts

@@ -309,6 +309,173 @@ describe.concurrent("require.cache", () => {
     }, 20000);
   });
 
+  // `delete require.cache[path]` of an ES module evicts its registry entry. A
+  // module that is still loading or evaluating is not in require.cache (`in`
+  // says false), so deleting it must be a no-op: evicting it mid-load made the
+  // next import() of the same path create a second record for it, which the
+  // loader's import cache did not expect (segfault at address 0x10 on the
+  // next import(), or the module evaluating twice).
+  describe("delete require.cache[esm] of a module that is still loading", () => {
+    // Every fixture records into one object and prints it once the event loop
+    // drains, so the test compares the whole outcome instead of line order.
+    const report = `
+      const result = (globalThis.__result ??= { evaluated: 0 });
+      process.on("beforeExit", () => console.log(JSON.stringify(result)));
+      module.exports = result;
+    `;
+
+    async function run(dir: string, entry: string) {
+      await using proc = Bun.spawn({
+        cmd: [bunExe(), entry],
+        env: bunEnv,
+        cwd: dir,
+        stderr: "pipe",
+      });
+      const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+      return { stdout, stderr, exitCode };
+    }
+
+    // a.mjs is fetched, then its CommonJS dependency runs before a.mjs evaluates.
+    // The dependency evicts a.mjs and imports it again.
+    const deleteThenImport = {
+      "report.cjs": report,
+      "a.mjs": `
+        import "./b.cjs";
+        export const x = 1;
+        globalThis.__result.evaluated++;
+      `,
+      "b.cjs": `
+        const result = require("./report.cjs");
+        const key = require("node:path").join(__dirname, "a.mjs");
+        result.inCacheBeforeDelete = key in require.cache;
+        result.deleted = delete require.cache[key];
+        import("./a.mjs").then(
+          ns => { result.depImport = ns.x; },
+          e => { result.depImport = String(e); },
+        );
+      `,
+      "main.mjs": `
+        const ns = await import("./a.mjs");
+        globalThis.__result.mainImport = ns.x;
+        globalThis.__result.secondImportIsSame = (await import("./a.mjs")) === ns;
+      `,
+    };
+
+    test("import() of the module from its dependency", async () => {
+      using dir = tempDir("require-cache-delete-loading-import", deleteThenImport);
+      const { stdout, stderr, exitCode } = await run(String(dir), "main.mjs");
+      expect(stderr).toBe("");
+      expect(JSON.parse(stdout)).toEqual({
+        evaluated: 1,
+        inCacheBeforeDelete: false,
+        deleted: true,
+        mainImport: 1,
+        secondImportIsSame: true,
+        depImport: 1,
+      });
+      expect(exitCode).toBe(0);
+    });
+
+    test("import() of the module from its dependency, module is the entry point", async () => {
+      using dir = tempDir("require-cache-delete-loading-entry", deleteThenImport);
+      const { stdout, stderr, exitCode } = await run(String(dir), "a.mjs");
+      expect(stderr).toBe("");
+      expect(JSON.parse(stdout)).toEqual({ evaluated: 1, inCacheBeforeDelete: false, deleted: true, depImport: 1 });
+      expect(exitCode).toBe(0);
+    });
+
+    test("require() of the module from its dependency", async () => {
+      using dir = tempDir("require-cache-delete-loading-require", {
+        ...deleteThenImport,
+        "b.cjs": `
+          const result = require("./report.cjs");
+          const key = require("node:path").join(__dirname, "a.mjs");
+          result.deleted = delete require.cache[key];
+          result.depRequire = require("./a.mjs").x;
+        `,
+      });
+      const { stdout, stderr, exitCode } = await run(String(dir), "main.mjs");
+      expect(stderr).toBe("");
+      expect(JSON.parse(stdout)).toEqual({
+        evaluated: 1,
+        deleted: true,
+        depRequire: 1,
+        mainImport: 1,
+        secondImportIsSame: true,
+      });
+      expect(exitCode).toBe(0);
+    });
+
+    test("while the module's top level is running", async () => {
+      using dir = tempDir("require-cache-delete-evaluating", {
+        "report.cjs": report,
+        "a.mjs": `
+          import { createRequire } from "node:module";
+          const require = createRequire(import.meta.url);
+          require("./c.cjs");
+          export const x = 1;
+          globalThis.__result.evaluated++;
+        `,
+        "c.cjs": `
+          const result = require("./report.cjs");
+          const key = require("node:path").join(__dirname, "a.mjs");
+          result.inCacheBeforeDelete = key in require.cache;
+          result.deleted = delete require.cache[key];
+          import("./a.mjs").then(
+            ns => { result.depImport = ns.x; },
+            e => { result.depImport = String(e); },
+          );
+        `,
+        "main.mjs": `
+          const ns = await import("./a.mjs");
+          globalThis.__result.mainImport = ns.x;
+        `,
+      });
+      const { stdout, stderr, exitCode } = await run(String(dir), "main.mjs");
+      expect(stderr).toBe("");
+      expect(JSON.parse(stdout)).toEqual({
+        evaluated: 1,
+        inCacheBeforeDelete: false,
+        deleted: true,
+        mainImport: 1,
+        depImport: 1,
+      });
+      expect(exitCode).toBe(0);
+    });
+
+    test("an evaluated module is still evicted", async () => {
+      using dir = tempDir("require-cache-delete-evaluated", {
+        "report.cjs": report,
+        "a.mjs": `
+          export const x = 1;
+          globalThis.__result.evaluated++;
+        `,
+        "main.mjs": `
+          import { createRequire } from "node:module";
+          import { join } from "node:path";
+          const require = createRequire(import.meta.url);
+          const result = require("./report.cjs");
+          const key = join(import.meta.dir, "a.mjs");
+          const first = await import("./a.mjs");
+          result.inCacheBeforeDelete = key in require.cache;
+          result.deleted = delete require.cache[key];
+          result.inCacheAfterDelete = key in require.cache;
+          result.secondImportIsSame = (await import("./a.mjs")) === first;
+        `,
+      });
+      const { stdout, stderr, exitCode } = await run(String(dir), "main.mjs");
+      expect(stderr).toBe("");
+      expect(JSON.parse(stdout)).toEqual({
+        evaluated: 2,
+        inCacheBeforeDelete: true,
+        deleted: true,
+        inCacheAfterDelete: false,
+        secondImportIsSame: false,
+      });
+      expect(exitCode).toBe(0);
+    });
+  });
+
   // These tests are extra slow in debug builds
   describe("files transpiled and loaded don't leak file paths", () => {
     test("via require()", async () => {