diff --git a/.changeset/vitest-pool-workers-unavailable-builtin-crash.md b/.changeset/vitest-pool-workers-unavailable-builtin-crash.md new file mode 100644 index 00000000000..34174691c0e --- /dev/null +++ b/.changeset/vitest-pool-workers-unavailable-builtin-crash.md @@ -0,0 +1,9 @@ +--- +"@cloudflare/vitest-pool-workers": patch +--- + +Report built-in modules that a Worker's compatibility settings don't provide as module errors, instead of crashing workerd + +Previously, a Worker whose module graph statically reached a compatibility-gated built-in that wasn't enabled — for example `import "node:child_process"` without `nodejs_compat` — took down the runtime with `*** Received signal #11: Segmentation fault` before any test ran. Vitest reported only `Worker exited unexpectedly`, naming neither the module nor the file that imported it, which made the cause very hard to find. The import didn't even have to be called; being reachable from the entrypoint was enough. + +The module fallback service answered these specifiers with a redirect to the modules root, but workerd already resolves `node:`/`cloudflare:`/`workerd:` specifiers there, so the redirect pointed back at the module workerd was in the middle of resolving and it recursed until the stack overflowed. Such a specifier only reaches the fallback service when workerd's own registry has already missed, so it's now reported as not found: workerd raises `No such module "node:child_process"`, matching what `wrangler dev` does for the same Worker. The accompanying pool error names the module and points at compatibility flags rather than suggesting you bundle it, which can't help for a module built into the runtime. diff --git a/packages/vitest-pool-workers/src/pool/module-fallback.ts b/packages/vitest-pool-workers/src/pool/module-fallback.ts index a58a378e959..acbdb767a41 100644 --- a/packages/vitest-pool-workers/src/pool/module-fallback.ts +++ b/packages/vitest-pool-workers/src/pool/module-fallback.ts @@ -215,6 +215,11 @@ function maybeGetTargetFilePath( } } +// Specifiers `workerd` resolves at the modules root rather than relative to the +// referrer, and strips the leading `/` from before asking the fallback service +// about them. +const prefixedSpecifierRegExp = /^(node|cloudflare|workerd):/; + /** * `target` is the path to the "file" `workerd` is trying to load, * `referrer` is the path to the file that imported/required the `target`, @@ -237,7 +242,7 @@ function maybeGetTargetFilePath( * ES module resolution, so must be handled by `maybeGetTargetFilePath()`. */ function getApproximateSpecifier(target: string, referrerDir: string): string { - if (/^(node|cloudflare|workerd):/.test(target)) { + if (prefixedSpecifierRegExp.test(target)) { return target; } return posixPath.relative(referrerDir, target); @@ -332,6 +337,12 @@ async function resolve( // *import*ing `node:*`/`cloudflare:*` modules, but not when *require()*ing // them. For the sake of consistency (and a nice return type on this function) // we return a redirect for `import`s too. + // + // Careful: `workerd` roots prefixed specifiers itself, so this "redirect to + // the root" is frequently a no-op that points straight back at the specifier + // `workerd` is already resolving. `load()` detects that case and reports the + // module as missing instead, because redirecting crashes `workerd`. See the + // comment on `UnavailableBuiltinModuleError`. if (referrerDir !== "/" && workerdBuiltinModules.has(specifier)) { return `/${specifier}`; } @@ -491,6 +502,36 @@ function buildModuleResponse(name: string, contents: ModuleContents) { return Response.json(result); } +/** + * Thrown when a `node:*`/`cloudflare:*`/`workerd:*` builtin isn't provided by + * the `workerd` the Worker under test is running on, so there is nothing we can + * serve for it. + * + * `workerd` resolves prefixed specifiers at the modules root rather than + * relative to the referrer (`kj::Path::parse(spec)` in `jsg/modules.c++`), and + * strips the leading `/` before asking us about them. A redirect to + * `/${target}` therefore points straight back at the specifier `workerd` is + * already resolving. Its module registry caches that redirect and re-enters + * resolution with the identical path, with no self-redirect check and no + * recursion bound, so it recurses until the stack overflows — killing the + * runtime with `*** Received signal #11: Segmentation fault` and no indication + * of which module was at fault. + * + * Reporting the module as missing instead lets `workerd` raise its own + * `No such module ""`, which is what `wrangler dev` does for the + * same Worker. This is safe because `workerd` only consults the fallback + * service *after* its own registry misses: any builtin that reaches us is, by + * definition, not available at this Worker's compatibility date and flags. + * + * See https://github.com/cloudflare/workers-sdk/issues/14590 + */ +class UnavailableBuiltinModuleError extends Error { + constructor(specifier: string) { + super(`No such module "${specifier}"`); + this.name = "UnavailableBuiltinModuleError"; + } +} + async function load( vite: Vite.ViteDevServer, logBase: string, @@ -514,6 +555,14 @@ async function load( return buildModuleResponse(rawTarget, { commonJsModule: wrapper }); } + // A redirect whose only difference from `target` is the leading slash that + // `workerd` strips from prefixed specifiers would send `workerd` back to the + // specifier it is already resolving, crashing it. Report the builtin as + // missing instead. See `UnavailableBuiltinModuleError`. + if (prefixedSpecifierRegExp.test(target) && filePath === `/${target}`) { + throw new UnavailableBuiltinModuleError(target); + } + if (target !== filePath) { // We might `import` and `require` the same CommonJS package. In this case, // we want to respond with an ES module shim for the `import`, and the @@ -683,11 +732,21 @@ export async function handleModuleFallbackRequest( ); } catch (e) { debuglog(logBase, "error:", e); - console.error( - `[vitest-pool-workers] Failed to ${method} ${JSON.stringify(target)} from ${JSON.stringify(referrer)}.`, - "To resolve this, try bundling the relevant dependency with Vite.", - "For more details, refer to https://developers.cloudflare.com/workers/testing/vitest-integration/known-issues/#module-resolution" - ); + if (e instanceof UnavailableBuiltinModuleError) { + // Bundling can't help here — the module is built into `workerd` and + // simply isn't switched on for this Worker. + console.error( + `[vitest-pool-workers] ${JSON.stringify(target)}, ${method === "import" ? "imported" : "required"} from ${JSON.stringify(referrer)}, is not available at this Worker's compatibility date and flags.`, + "To resolve this, enable the compatibility flag that provides it (`nodejs_compat` for `node:*` modules), or remove the import.", + "For more details, refer to https://developers.cloudflare.com/workers/configuration/compatibility-flags/" + ); + } else { + console.error( + `[vitest-pool-workers] Failed to ${method} ${JSON.stringify(target)} from ${JSON.stringify(referrer)}.`, + "To resolve this, try bundling the relevant dependency with Vite.", + "For more details, refer to https://developers.cloudflare.com/workers/testing/vitest-integration/known-issues/#module-resolution" + ); + } } return new Response(null, { status: 404 }); diff --git a/packages/vitest-pool-workers/test/module-fallback.test.ts b/packages/vitest-pool-workers/test/module-fallback.test.ts index 7760a66c0d9..5ee9f9c6b8e 100644 --- a/packages/vitest-pool-workers/test/module-fallback.test.ts +++ b/packages/vitest-pool-workers/test/module-fallback.test.ts @@ -24,6 +24,21 @@ function fakeVite(): Vite.ViteDevServer { } as unknown as Vite.ViteDevServer; } +// As above, but Vite resolves every specifier to `id`. Used to drive the +// handler to a specific `filePath` without touching the filesystem. +// +// Note the built-in module list is a build-time define, stubbed to `[]` for +// these unit tests (see `vitest.config.mts`), so the `workerdBuiltinModules` +// branch in `resolve()` can't be exercised here. Resolving through Vite to the +// same rooted path that branch would have produced reaches the same place. +function fakeViteResolvingTo(id: string): Vite.ViteDevServer { + return { + pluginContainer: { + resolveId: async () => ({ id }), + }, + } as unknown as Vite.ViteDevServer; +} + function moduleFallbackRequest(options: { method: "import" | "require"; specifier: string; @@ -256,3 +271,112 @@ describe("handleModuleFallbackRequest non-ASCII paths", () => { } }); }); + +// `workerd` resolves `node:*`/`cloudflare:*`/`workerd:*` specifiers at the +// modules root rather than relative to the referrer, and strips the leading `/` +// before asking us about them. Answering with a redirect to `/${target}` names +// the module `workerd` is already resolving, and its module registry follows +// that self-redirect with no cycle check and no recursion bound — recursing +// until the stack overflows and the runtime dies with +// `*** Received signal #11: Segmentation fault`, naming no module. +// See https://github.com/cloudflare/workers-sdk/issues/14590 +describe("built-ins unavailable at the Worker's compatibility settings", () => { + const referrer = "/repro/node_modules/vitest/dist/module-evaluator.js"; + + async function fallbackFor(options: { + method: "import" | "require"; + specifier: string; + resolvesTo: string; + }) { + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + try { + const res = await handleModuleFallbackRequest( + fakeViteResolvingTo(options.resolvesTo), + moduleFallbackRequest({ + method: options.method, + specifier: options.specifier, + referrer, + }) + ); + // Snapshot the calls before restoring, which clears them. + const logged = errorSpy.mock.calls.map((call) => call.join(" ")); + return { res, logged }; + } finally { + errorSpy.mockRestore(); + } + } + + it("404s instead of self-redirecting an imported `node:*` built-in", async ({ + expect, + }) => { + const { res } = await fallbackFor({ + method: "import", + specifier: "node:child_process", + resolvesTo: "/node:child_process", + }); + // Previously a 301 to `/node:child_process` — the specifier `workerd` was + // already resolving. + expect(res.status).toBe(404); + expect(res.headers.get("Location")).toBe(null); + }); + + it("404s instead of self-redirecting a required `node:*` built-in", async ({ + expect, + }) => { + // The redirect was previously emitted for `require()` too, so guarding + // only the `import` path would leave this crashing. + const { res } = await fallbackFor({ + method: "require", + specifier: "node:child_process", + resolvesTo: "/node:child_process", + }); + expect(res.status).toBe(404); + expect(res.headers.get("Location")).toBe(null); + }); + + it("404s instead of self-redirecting a `cloudflare:*` built-in", async ({ + expect, + }) => { + // `cloudflare:*` modules are compatibility-gated too, so the guard must + // not be `node:`-only. + const { res } = await fallbackFor({ + method: "import", + specifier: "cloudflare:sockets", + resolvesTo: "/cloudflare:sockets", + }); + expect(res.status).toBe(404); + expect(res.headers.get("Location")).toBe(null); + }); + + it("advises on compatibility flags rather than bundling", async ({ + expect, + }) => { + // Bundling can't provide a module that's built into `workerd` and simply + // switched off, so the generic module-resolution advice is wrong here. + const { logged } = await fallbackFor({ + method: "import", + specifier: "node:child_process", + resolvesTo: "/node:child_process", + }); + expect(logged).toHaveLength(1); + expect(logged[0]).toContain("node:child_process"); + expect(logged[0]).toContain("nodejs_compat"); + expect(logged[0]).not.toContain("bundling"); + }); + + it("still redirects a prefixed specifier that resolves elsewhere", async ({ + expect, + }) => { + // Pool-internal modules such as `cloudflare:test` resolve to a real file, + // so the redirect is genuine progress and must be preserved. + const { res } = await fallbackFor({ + method: "import", + specifier: "cloudflare:test-internal", + resolvesTo: "/pool/dist/worker/lib/cloudflare/test-internal.mjs", + }); + expect(res.status).toBe(301); + expect(res.headers.get("Location")).toBe( + "/pool/dist/worker/lib/cloudflare/test-internal.mjs" + ); + }); +}); diff --git a/packages/vitest-pool-workers/test/unavailable-builtin.test.ts b/packages/vitest-pool-workers/test/unavailable-builtin.test.ts new file mode 100644 index 00000000000..37815ad724c --- /dev/null +++ b/packages/vitest-pool-workers/test/unavailable-builtin.test.ts @@ -0,0 +1,53 @@ +import dedent from "ts-dedent"; +import { test, vitestConfig } from "./helpers"; + +// A Worker whose module graph reaches a compatibility-gated built-in that isn't +// enabled used to take down `workerd` with +// `*** Received signal #11: Segmentation fault: 11` before any test ran, +// reporting only "Worker exited unexpectedly" and naming no module. +// See https://github.com/cloudflare/workers-sdk/issues/14590 +test( + "reports a `node:*` built-in missing at the Worker's compatibility settings", + { timeout: 60_000 }, + async ({ expect, seed, vitestRun }) => { + await seed({ + "vitest.config.mts": vitestConfig({ + main: "./index.ts", + miniflare: { + compatibilityDate: "2025-12-02", + // Deliberately no `nodejs_compat`, so `workerd` doesn't provide + // `node:child_process`. + compatibilityFlags: [], + }, + }), + "index.ts": dedent /* javascript */ ` + // Never called — being statically reachable is enough to load it. + import "node:child_process"; + export default { + fetch() { + return new Response("ok"); + } + } + `, + // Importing `cloudflare:test` is what forces the Worker's module graph + // (and therefore `node:child_process`) to load. + "index.test.ts": dedent /* javascript */ ` + import { SELF } from "cloudflare:test"; + import { expect, it } from "vitest"; + it("sends request", async () => { + const response = await SELF.fetch("https://example.com"); + expect(response.ok).toBe(true); + }); + `, + }); + + const result = await vitestRun(); + const output = result.stdout + result.stderr; + + expect(output).not.toMatch("Segmentation fault"); + expect(output).not.toMatch("Received signal"); + // The failure must name the offending module. + expect(output).toMatch("node:child_process"); + expect(await result.exitCode).toBe(1); + } +);