mock.module: validate callback before running resolver

docs/runtime/bunfig.mdx

@@ -512,11 +512,11 @@ prefer = "online"
 
 Valid values are:
 
-| Value       | Description                                                                                       |
-| ----------- | ------------------------------------------------------------------------------------------------- |
-| `"online"`  | Default. Check the registry for stale packages as needed.                                         |
+| Value       | Description                                                                                        |
+| ----------- | -------------------------------------------------------------------------------------------------- |
+| `"online"`  | Default. Check the registry for stale packages as needed.                                          |
 | `"offline"` | Skip staleness checks and resolve packages from the local cache. Equivalent to `--prefer-offline`. |
-| `"latest"`  | Always check npm for the latest matching versions. Equivalent to `--prefer-latest`.               |
+| `"latest"`  | Always check npm for the latest matching versions. Equivalent to `--prefer-latest`.                |
 
 ### `install.frozenLockfile`
 

src/bun.js/bindings/BunPlugin.cpp

@@ -526,6 +526,12 @@ extern "C" JSC_DEFINE_HOST_FUNCTION(JSMock__jsModuleMock, (JSC::JSGlobalObject *
         return {};
     }
 
+    JSC::JSValue callbackValue = callframe->argument(1);
+    if (!callbackValue.isCell() || !callbackValue.isCallable()) {
+        scope.throwException(lexicalGlobalObject, JSC::createTypeError(lexicalGlobalObject, "mock(module, fn) requires a function"_s));
+        return {};
+    }
+
     auto resolveSpecifier = [&]() -> void {
         JSC::SourceOrigin sourceOrigin = callframe->callerSourceOrigin(vm);
         if (sourceOrigin.isNull())
@@ -581,12 +587,6 @@ extern "C" JSC_DEFINE_HOST_FUNCTION(JSMock__jsModuleMock, (JSC::JSGlobalObject *
     resolveSpecifier();
     RETURN_IF_EXCEPTION(scope, {});
 
-    JSC::JSValue callbackValue = callframe->argument(1);
-    if (!callbackValue.isCell() || !callbackValue.isCallable()) {
-        scope.throwException(lexicalGlobalObject, JSC::createTypeError(lexicalGlobalObject, "mock(module, fn) requires a function"_s));
-        return {};
-    }
-
     JSC::JSObject* callback = callbackValue.getObject();
 
     JSModuleMock* mock = JSModuleMock::create(vm, globalObject->mockModule.mockModuleStructure.getInitializedOnMainThread(globalObject), callback);

test/js/bun/test/mock/mock-module-non-string.test.ts

@@ -1,4 +1,5 @@
 import { expect, mock, test } from "bun:test";
+import { bunEnv, bunExe, tempDir } from "harness";
 
 test("mock.module throws TypeError for non-string first argument", () => {
   // @ts-expect-error
@@ -36,3 +37,89 @@ test("mock.module does not crash on specifiers that are not valid npm package na
   }
   Bun.gc(true);
 });
+
+test("mock.module throws TypeError without resolving when callback is missing", () => {
+  // The resolver can reach the package-manager auto-install path and reentrantly
+  // tick the JS event loop. When the caller forgets the callback, we must throw
+  // before running the resolver at all.
+  const specifiers = [
+    // valid npm package name — bypasses the isNPMPackageName gate in the resolver
+    "PbQ",
+    "some-package-that-does-not-exist",
+    "@scope/pkg",
+    "function f3() {}",
+    "() => 1",
+  ];
+  for (const specifier of specifiers) {
+    // @ts-expect-error missing callback on purpose
+    expect(() => mock.module(specifier)).toThrow("mock(module, fn) requires a function");
+  }
+  for (const specifier of specifiers) {
+    // @ts-expect-error non-callable second argument on purpose
+    expect(() => mock.module(specifier, 123)).toThrow("mock(module, fn) requires a function");
+  }
+  Bun.gc(true);
+});
+
+test("mock.module does not run the resolver when callback is missing", async () => {
+  // When auto-install is enabled and the specifier is a valid npm package name,
+  // the resolver reaches the package-manager path, reentrantly ticks the JS
+  // event loop, and blocks waiting on the registry. If the callback is missing
+  // we must throw before the resolver runs so none of that happens.
+  let requests = 0;
+  const { promise: gotRequest, resolve: onRequest } = Promise.withResolvers<void>();
+  await using server = Bun.serve({
+    port: 0,
+    fetch() {
+      requests++;
+      onRequest();
+      return new Response("{}", { status: 404, headers: { "content-type": "application/json" } });
+    },
+  });
+
+  using dir = tempDir("mock-module-no-callback-no-resolve", {
+    "index.js": `
+      const { vi } = Bun.jest();
+      try { vi.mock("PbQ"); } catch (e) { console.error("ERR:", e.message); }
+      try { vi.mock("some-valid-pkg-name-abc123"); } catch (e) { console.error("ERR:", e.message); }
+      Bun.gc(true);
+      console.log("done");
+    `,
+  });
+
+  await using proc = Bun.spawn({
+    cmd: [bunExe(), "--install=force", "index.js"],
+    env: {
+      ...bunEnv,
+      BUN_CONFIG_REGISTRY: server.url.href,
+      NPM_CONFIG_REGISTRY: server.url.href,
+      BUN_INSTALL_CACHE_DIR: String(dir) + "/.cache",
+    },
+    cwd: String(dir),
+    stdout: "pipe",
+    stderr: "pipe",
+  });
+
+  // Before the fix, the resolver blocks on the registry and the process never
+  // exits on its own. Race the exit against the first registry request so the
+  // test fails fast (instead of timing out) on a regression.
+  const result = await Promise.race([
+    Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]).then(([stdout, stderr, exitCode]) => ({
+      kind: "exited" as const,
+      stdout,
+      stderr,
+      exitCode,
+    })),
+    gotRequest.then(() => ({ kind: "request" as const })),
+  ]);
+
+  if (result.kind === "request") {
+    proc.kill();
+    throw new Error("mock.module ran the resolver and hit the registry when callback was missing");
+  }
+
+  expect(result.stderr).toContain("mock(module, fn) requires a function");
+  expect(result.stdout).toContain("done");
+  expect(requests).toBe(0);
+  expect(result.exitCode).toBe(0);
+});