Fix source mapping of service workers (#572)

cloudflare/workerd#190 changed the script name for service workers to
their service name. This change updates Miniflare's heuristics for
locating source maps to work with this. It also adds some tests to
ensure source mapping is working correctly for the different ways of
specifying a script.

Author: mrbbot

packages/tre/src/index.ts

@@ -43,7 +43,7 @@ import {
 } from "./plugins";
 import {
   JsonErrorSchema,
-  SourceOptions,
+  NameSourceOptions,
   getUserServiceName,
   handlePrettyErrorRequest,
   reviveError,
@@ -517,8 +517,8 @@ export class Miniflare {
     }
   }
 
-  get #workerSrcOpts(): SourceOptions[] {
-    return this.#workerOpts.map<SourceOptions>(({ core }) => core);
+  get #workerSrcOpts(): NameSourceOptions[] {
+    return this.#workerOpts.map<NameSourceOptions>(({ core }) => core);
   }
 
   #handleLoopback = async (

packages/tre/src/plugins/core/constants.ts

@@ -0,0 +1,23 @@
+export const CORE_PLUGIN_NAME = "core";
+
+// Service for HTTP socket entrypoint (for checking runtime ready, routing, etc)
+export const SERVICE_ENTRY = `${CORE_PLUGIN_NAME}:entry`;
+// Service prefix for all regular user workers
+const SERVICE_USER_PREFIX = `${CORE_PLUGIN_NAME}:user`;
+// Service prefix for `workerd`'s builtin services (network, external, disk)
+const SERVICE_BUILTIN_PREFIX = `${CORE_PLUGIN_NAME}:builtin`;
+// Service prefix for custom fetch functions defined in `serviceBindings` option
+const SERVICE_CUSTOM_PREFIX = `${CORE_PLUGIN_NAME}:custom`;
+
+export function getUserServiceName(workerName = "") {
+  return `${SERVICE_USER_PREFIX}:${workerName}`;
+}
+export function getBuiltinServiceName(
+  workerIndex: number,
+  bindingName: string
+) {
+  return `${SERVICE_BUILTIN_PREFIX}:${workerIndex}:${bindingName}`;
+}
+export function getCustomServiceName(workerIndex: number, bindingName: string) {
+  return `${SERVICE_CUSTOM_PREFIX}:${workerIndex}:${bindingName}`;
+}

packages/tre/src/plugins/core/errors/index.ts

@@ -4,6 +4,7 @@ import type { UrlAndMap } from "source-map-support";
 import { z } from "zod";
 import { Request, Response } from "../../../http";
 import { Log } from "../../../shared";
+import { getUserServiceName } from "../constants";
 import {
   SourceOptions,
   contentsToString,
@@ -13,33 +14,31 @@ import { getSourceMapper } from "./sourcemap";
 
 // Subset of core worker options that define Worker source code.
 // These are the possible cases, and corresponding reported source files in
-// workerd stack traces. Note that all service-worker scripts will be called
-// "worker.js" in `workerd`, so we can't differentiate between multiple workers.
-// TODO: see if we can add a service-worker script path config option to handle
-//  case (i)[i]
+// workerd stack traces.
 //
 // Single Worker:
-// (a) { script: "<contents>" }                                       -> "worker.js"
-// (b) { script: "<contents>", modules: true }                        -> "<script:0>"
-// (c) { script: "<contents>", scriptPath: "<path>" }                 -> "worker.js"
-// (d) { script: "<contents>", scriptPath: "<path>", modules: true }  -> "<path>"
-// (e) { scriptPath: "<path>" }                                       -> "worker.js"
-// (f) { scriptPath: "<path>", modules: true }                        -> "<path>"
+// (a) { script: "<contents>" }                                          -> "core:user:"
+// (b) { script: "<contents>", modules: true }                           -> "<script:0>"
+// (c) { script: "<contents>", scriptPath: "<path>" }                    -> "core:user:"
+// (d) { script: "<contents>", scriptPath: "<path>", modules: true }     -> "<path>"
+// (e) { scriptPath: "<path>" }                                          -> "core:user:"
+// (f) { scriptPath: "<path>", modules: true }                           -> "<path>"
 // (g) { modules: [
-//   [i]  { ..., path: "<path:0>", contents: "<contents:0>" },        -> "<path:0>" relative to cwd
-//  [ii]  { ..., path: "<path:1>" },                                  -> "<path:1>" relative to cwd
+//   [i]  { ..., path: "<path:0>", contents: "<contents:0>" },           -> "<path:0>" relative to cwd
+//  [ii]  { ..., path: "<path:1>" },                                     -> "<path:1>" relative to cwd
 //     ] }
 // (h) { modulesRoot: "<root>", modules: [
-//   [i]  { ..., path: "<path:0>", contents: "<contents:0>" },        -> "<path:0>" relative to "<root>"
-//  [ii]  { ..., path: "<path:1>" },                                  -> "<path:1>" relative to "<root>"
+//   [i]  { ..., path: "<path:0>", contents: "<contents:0>" },           -> "<path:0>" relative to "<root>"
+//  [ii]  { ..., path: "<path:1>" },                                     -> "<path:1>" relative to "<root>"
 //     ] }
 //
 // Multiple Workers (array of `SourceOptions`):
-// (i) [                                                                 (note cannot differentiate "worker.js"s)
-//   [i]  { script: "<contents:0>" },                                 -> "worker.js"
-//        { script: "<contents:1>" },                                 -> "worker.js"
-//        { script: "<contents:2>", scriptPath: "<path:2>" },         -> "worker.js"
-//  [ii]  { script: "<contents:3>", modules: true },                  -> "<script:3>"
+// (i) [
+//   [i]  { script: "<contents:0>" },                                    -> "core:user:"
+//  [ii]  { name: "a", script: "<contents:1>" },                         -> "core:user:a"
+// [iii]  { name: "b", script: "<contents:2>", scriptPath: "<path:2>" }, -> "core:user:b"
+//  [iv]  { name: "c", scriptPath: "<path:3>" },                         -> "core:user:c"
+//   [v]  { script: "<contents:3>", modules: true },                     -> "<script:3>"
 //     ]
 //
 
@@ -59,11 +58,13 @@ function maybeGetDiskFile(filePath: string): SourceFile | undefined {
   }
 }
 
+export type NameSourceOptions = SourceOptions & { name?: string };
+
 // Try to extract the path and contents of a `file` reported in a JavaScript
 // stack-trace. See the `SourceOptions` comment for examples of what these look
 // like.
 function maybeGetFile(
-  workerSrcOpts: SourceOptions[],
+  workerSrcOpts: NameSourceOptions[],
   file: string
 ): SourceFile | undefined {
   // Resolve file relative to current working directory
@@ -111,7 +112,7 @@ function maybeGetFile(
     }
   }
 
-  // Cases: (b), (i)[ii]
+  // Cases: (b), (i)[v]
   // 3. If file looks like "<script:n>", and the `n`th worker has a custom
   //    `script`, use that.
   const workerIndex = maybeGetStringScriptPathIndex(file);
@@ -122,22 +123,15 @@ function maybeGetFile(
     }
   }
 
-  // Cases: (a), (c), (e)
-  // 4. If there is a single worker defined with `modules` disabled, the
-  //    file is "worker.js", then...
-  //
-  //    Note: can't handle case (i)[i], as cannot distinguish between multiple
-  //    "worker.js"s, hence the check for a single worker. We'd rather be
-  //    conservative and return no contents (and therefore no source code in the
-  //    error page) over incorrect ones.
-  if (workerSrcOpts.length === 1) {
-    const srcOpts = workerSrcOpts[0];
+  // Cases: (a), (c), (e), (i)[i], (i)[ii], (i)[iii], (i)[iv]
+  // 4. If `file` is the name of a service, use that services' source.
+  for (const srcOpts of workerSrcOpts) {
     if (
-      file === "worker.js" &&
+      file === getUserServiceName(srcOpts.name) &&
       (srcOpts.modules === undefined || srcOpts.modules === false)
     ) {
       if ("script" in srcOpts && srcOpts.script !== undefined) {
-        // Cases: (a), (c)
+        // Cases: (a), (c), (i)[i], (i)[ii], (i)[iii]
         // ...if a custom `script` is defined, use that, with the defined
         // `scriptPath` if any (Case (c))
         return {
@@ -148,7 +142,7 @@ function maybeGetFile(
           contents: srcOpts.script,
         };
       } else if (srcOpts.scriptPath !== undefined) {
-        // Case: (e)
+        // Case: (e), (i)[iv]
         // ...otherwise, if a `scriptPath` is defined, use that
         return maybeGetDiskFile(path.resolve(srcOpts.scriptPath));
       }
@@ -160,7 +154,10 @@ function maybeGetFile(
   return maybeGetDiskFile(filePath);
 }
 
-function getSourceMappedStack(workerSrcOpts: SourceOptions[], error: Error) {
+function getSourceMappedStack(
+  workerSrcOpts: NameSourceOptions[],
+  error: Error
+) {
   // This function needs to match the signature of the `retrieveSourceMap`
   // option from the "source-map-support" package.
   function retrieveSourceMap(file: string): UrlAndMap | null {
@@ -220,7 +217,7 @@ const ALLOWED_ERROR_SUBCLASS_CONSTRUCTORS: StandardErrorConstructor[] = [
   URIError,
 ];
 export function reviveError(
-  workerSrcOpts: SourceOptions[],
+  workerSrcOpts: NameSourceOptions[],
   jsonError: JsonError
 ): Error {
   // At a high level, this function takes a JSON-serialisable representation of
@@ -262,7 +259,7 @@ export function reviveError(
 
 export async function handlePrettyErrorRequest(
   log: Log,
-  workerSrcOpts: SourceOptions[],
+  workerSrcOpts: NameSourceOptions[],
   request: Request
 ): Promise<Response> {
   // Parse and validate the error we've been given from user code

packages/tre/src/plugins/core/index.ts

@@ -29,6 +29,12 @@ import {
   WORKER_BINDING_SERVICE_LOOPBACK,
   parseRoutes,
 } from "../shared";
+import {
+  SERVICE_ENTRY,
+  getBuiltinServiceName,
+  getCustomServiceName,
+  getUserServiceName,
+} from "./constants";
 import {
   ModuleLocator,
   SourceOptions,
@@ -80,25 +86,6 @@ export const CoreSharedOptionsSchema = z.object({
 
 export const CORE_PLUGIN_NAME = "core";
 
-// Service for HTTP socket entrypoint (for checking runtime ready, routing, etc)
-export const SERVICE_ENTRY = `${CORE_PLUGIN_NAME}:entry`;
-// Service prefix for all regular user workers
-const SERVICE_USER_PREFIX = `${CORE_PLUGIN_NAME}:user`;
-// Service prefix for `workerd`'s builtin services (network, external, disk)
-const SERVICE_BUILTIN_PREFIX = `${CORE_PLUGIN_NAME}:builtin`;
-// Service prefix for custom fetch functions defined in `serviceBindings` option
-const SERVICE_CUSTOM_PREFIX = `${CORE_PLUGIN_NAME}:custom`;
-
-export function getUserServiceName(workerName = "") {
-  return `${SERVICE_USER_PREFIX}:${workerName}`;
-}
-function getBuiltinServiceName(workerIndex: number, bindingName: string) {
-  return `${SERVICE_BUILTIN_PREFIX}:${workerIndex}:${bindingName}`;
-}
-function getCustomServiceName(workerIndex: number, bindingName: string) {
-  return `${SERVICE_CUSTOM_PREFIX}:${workerIndex}:${bindingName}`;
-}
-
 const LIVE_RELOAD_SCRIPT_TEMPLATE = (
   port: number
 ) => `<script defer type="application/javascript">
@@ -432,5 +419,6 @@ function getWorkerScript(
 }
 
 export * from "./errors";
+export * from "./constants";
 export * from "./modules";
 export * from "./services";

packages/tre/src/workers/core/entry.worker.ts

@@ -171,6 +171,13 @@ async function handleQueue(
 export default <ExportedHandler<Env>>{
   async fetch(request, env, ctx) {
     const startTime = Date.now();
+
+    // `dispatchFetch()` will always inject the passed URL as a header. When
+    // calling this function, we never want to display the pretty-error page.
+    // Instead, we propagate the error and reject the returned `Promise`.
+    const isDispatchFetch =
+      request.headers.get(CoreHeaders.ORIGINAL_URL) !== null;
+
     request = getUserRequest(request, env);
     const url = new URL(request.url);
     const service = getTargetService(request, url, env);
@@ -186,7 +193,9 @@ export default <ExportedHandler<Env>>{
       }
 
       let response = await service.fetch(request);
-      response = await maybePrettifyError(request, response, env);
+      if (!isDispatchFetch) {
+        response = await maybePrettifyError(request, response, env);
+      }
       response = maybeInjectLiveReload(response, env, ctx);
       maybeLogRequest(request, response, env, ctx, startTime);
       return response;

packages/tre/test/fixtures/source-maps/modules.ts

@@ -0,0 +1,11 @@
+import { reduceError } from "./reduce";
+
+export default <ExportedHandler<{ MESSAGE: string }>>{
+  fetch(request, env) {
+    const error = new Error(env.MESSAGE);
+    return Response.json(reduceError(error), {
+      status: 500,
+      headers: { "MF-Experimental-Error-Stack": "true" },
+    });
+  },
+};

packages/tre/test/fixtures/source-maps/reduce.ts

@@ -0,0 +1,7 @@
+export function reduceError(e: any) {
+  return {
+    name: e?.name,
+    message: e?.message ?? String(e),
+    stack: e?.stack,
+  };
+}

packages/tre/test/fixtures/source-maps/service-worker.ts

@@ -0,0 +1,13 @@
+import { reduceError } from "./reduce";
+
+declare const MESSAGE: string;
+
+addEventListener("fetch", (event) => {
+  const error = new Error(MESSAGE);
+  event.respondWith(
+    Response.json(reduceError(error), {
+      status: 500,
+      headers: { "MF-Experimental-Error-Stack": "true" },
+    })
+  );
+});

packages/tre/test/fixtures/tsconfig.json

@@ -0,0 +1,15 @@
+{
+  "compilerOptions": {
+    "module": "esnext",
+    "target": "esnext",
+    "lib": ["esnext"],
+    "strict": true,
+    "moduleResolution": "node16",
+    "isolatedModules": true,
+    "noEmit": true,
+    "types": ["@cloudflare/workers-types/experimental"]
+  },
+  "include": [
+    "**/*"
+  ]
+}

packages/tre/test/plugins/core/errors/index.spec.ts

@@ -0,0 +1,121 @@
+import fs from "fs/promises";
+import path from "path";
+import { Miniflare } from "@miniflare/tre";
+import test from "ava";
+import esbuild from "esbuild";
+import { useTmp } from "../../../test-shared";
+
+const FIXTURES_PATH = path.resolve(
+  __dirname,
+  "..",
+  "..",
+  "..",
+  "..",
+  "..",
+  "test",
+  "fixtures",
+  "source-maps"
+);
+const SERVICE_WORKER_ENTRY_PATH = path.join(FIXTURES_PATH, "service-worker.ts");
+const MODULES_ENTRY_PATH = path.join(FIXTURES_PATH, "modules.ts");
+
+test("source maps workers", async (t) => {
+  // Build fixtures
+  const tmp = await useTmp(t);
+  await esbuild.build({
+    entryPoints: [SERVICE_WORKER_ENTRY_PATH, MODULES_ENTRY_PATH],
+    format: "esm",
+    bundle: true,
+    sourcemap: true,
+    outdir: tmp,
+  });
+  const serviceWorkerPath = path.join(tmp, "service-worker.js");
+  const modulesPath = path.join(tmp, "modules.js");
+  const serviceWorkerContent = await fs.readFile(serviceWorkerPath, "utf8");
+  const modulesContent = await fs.readFile(modulesPath, "utf8");
+
+  // Check service-workers source mapped
+  const mf = new Miniflare({
+    workers: [
+      {
+        bindings: { MESSAGE: "unnamed" },
+        scriptPath: serviceWorkerPath,
+      },
+      {
+        name: "a",
+        routes: ["*/a"],
+        bindings: { MESSAGE: "a" },
+        script: serviceWorkerContent,
+        scriptPath: serviceWorkerPath,
+      },
+    ],
+  });
+  let error = await t.throwsAsync(mf.dispatchFetch("http://localhost"), {
+    message: "unnamed",
+  });
+  t.regex(String(error?.stack), /service-worker\.ts:6:17/);
+  error = await t.throwsAsync(mf.dispatchFetch("http://localhost/a"), {
+    message: "a",
+  });
+  t.regex(String(error?.stack), /service-worker\.ts:6:17/);
+
+  // Check modules workers source mapped
+  await mf.setOptions({
+    workers: [
+      {
+        modules: true,
+        scriptPath: modulesPath,
+        bindings: { MESSAGE: "unnamed" },
+      },
+      {
+        name: "a",
+        routes: ["*/a"],
+        bindings: { MESSAGE: "a" },
+        modules: true,
+        script: modulesContent,
+        scriptPath: modulesPath,
+      },
+      {
+        name: "b",
+        routes: ["*/b"],
+        bindings: { MESSAGE: "b" },
+        modules: [{ type: "ESModule", path: modulesPath }],
+      },
+      {
+        name: "c",
+        routes: ["*/c"],
+        bindings: { MESSAGE: "c" },
+        modules: [
+          { type: "ESModule", path: modulesPath, contents: modulesContent },
+        ],
+      },
+      {
+        name: "d",
+        routes: ["*/d"],
+        bindings: { MESSAGE: "d" },
+        modulesRoot: tmp,
+        modules: [{ type: "ESModule", path: modulesPath }],
+      },
+    ],
+  });
+  error = await t.throwsAsync(mf.dispatchFetch("http://localhost"), {
+    message: "unnamed",
+  });
+  t.regex(String(error?.stack), /modules\.ts:5:19/);
+  error = await t.throwsAsync(mf.dispatchFetch("http://localhost/a"), {
+    message: "a",
+  });
+  t.regex(String(error?.stack), /modules\.ts:5:19/);
+  error = await t.throwsAsync(mf.dispatchFetch("http://localhost/b"), {
+    message: "b",
+  });
+  t.regex(String(error?.stack), /modules\.ts:5:19/);
+  error = await t.throwsAsync(mf.dispatchFetch("http://localhost/c"), {
+    message: "c",
+  });
+  t.regex(String(error?.stack), /modules\.ts:5:19/);
+  error = await t.throwsAsync(mf.dispatchFetch("http://localhost/d"), {
+    message: "d",
+  });
+  t.regex(String(error?.stack), /modules\.ts:5:19/);
+});