From 3cf02badfd7f611bd054e8a7392a02140df46b01 Mon Sep 17 00:00:00 2001
From: MrBBot <bcoll@cloudflare.com>
Date: Mon, 15 May 2023 17:08:58 +0100
Subject: [PATCH] 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.
---
 packages/miniflare/src/index.ts               |   6 +-
 .../miniflare/src/plugins/core/constants.ts   |  23 ++++
 .../src/plugins/core/errors/index.ts          |  71 +++++-----
 packages/miniflare/src/plugins/core/index.ts  |  26 +---
 .../src/workers/core/entry.worker.ts          |  11 +-
 .../test/fixtures/source-maps/modules.ts      |  11 ++
 .../test/fixtures/source-maps/reduce.ts       |   7 +
 .../fixtures/source-maps/service-worker.ts    |  13 ++
 .../miniflare/test/fixtures/tsconfig.json     |  13 ++
 .../test/plugins/core/errors/index.spec.ts    | 121 ++++++++++++++++++
 10 files changed, 242 insertions(+), 60 deletions(-)
 create mode 100644 packages/miniflare/src/plugins/core/constants.ts
 create mode 100644 packages/miniflare/test/fixtures/source-maps/modules.ts
 create mode 100644 packages/miniflare/test/fixtures/source-maps/reduce.ts
 create mode 100644 packages/miniflare/test/fixtures/source-maps/service-worker.ts
 create mode 100644 packages/miniflare/test/fixtures/tsconfig.json
 create mode 100644 packages/miniflare/test/plugins/core/errors/index.spec.ts

diff --git a/packages/miniflare/src/index.ts b/packages/miniflare/src/index.ts
index a6f330551f4f7..4a520679c8db4 100644
--- a/packages/miniflare/src/index.ts
+++ b/packages/miniflare/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 (
diff --git a/packages/miniflare/src/plugins/core/constants.ts b/packages/miniflare/src/plugins/core/constants.ts
new file mode 100644
index 0000000000000..e73d69aec8af3
--- /dev/null
+++ b/packages/miniflare/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}`;
+}
diff --git a/packages/miniflare/src/plugins/core/errors/index.ts b/packages/miniflare/src/plugins/core/errors/index.ts
index 8367d52058b32..ba1b03c20ccf6 100644
--- a/packages/miniflare/src/plugins/core/errors/index.ts
+++ b/packages/miniflare/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
diff --git a/packages/miniflare/src/plugins/core/index.ts b/packages/miniflare/src/plugins/core/index.ts
index 13f22e85e9654..0637cf2c6f6bf 100644
--- a/packages/miniflare/src/plugins/core/index.ts
+++ b/packages/miniflare/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";
diff --git a/packages/miniflare/src/workers/core/entry.worker.ts b/packages/miniflare/src/workers/core/entry.worker.ts
index 1073fb8fd3b40..3c4785e5f4694 100644
--- a/packages/miniflare/src/workers/core/entry.worker.ts
+++ b/packages/miniflare/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;
diff --git a/packages/miniflare/test/fixtures/source-maps/modules.ts b/packages/miniflare/test/fixtures/source-maps/modules.ts
new file mode 100644
index 0000000000000..44969dc7773ce
--- /dev/null
+++ b/packages/miniflare/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" },
+		});
+	},
+};
diff --git a/packages/miniflare/test/fixtures/source-maps/reduce.ts b/packages/miniflare/test/fixtures/source-maps/reduce.ts
new file mode 100644
index 0000000000000..bf70ee788b73a
--- /dev/null
+++ b/packages/miniflare/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,
+	};
+}
diff --git a/packages/miniflare/test/fixtures/source-maps/service-worker.ts b/packages/miniflare/test/fixtures/source-maps/service-worker.ts
new file mode 100644
index 0000000000000..e55e1d287941c
--- /dev/null
+++ b/packages/miniflare/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" },
+		})
+	);
+});
diff --git a/packages/miniflare/test/fixtures/tsconfig.json b/packages/miniflare/test/fixtures/tsconfig.json
new file mode 100644
index 0000000000000..4f7f1cdf943a2
--- /dev/null
+++ b/packages/miniflare/test/fixtures/tsconfig.json
@@ -0,0 +1,13 @@
+{
+	"compilerOptions": {
+		"module": "esnext",
+		"target": "esnext",
+		"lib": ["esnext"],
+		"strict": true,
+		"moduleResolution": "node16",
+		"isolatedModules": true,
+		"noEmit": true,
+		"types": ["@cloudflare/workers-types/experimental"]
+	},
+	"include": ["**/*"]
+}
diff --git a/packages/miniflare/test/plugins/core/errors/index.spec.ts b/packages/miniflare/test/plugins/core/errors/index.spec.ts
new file mode 100644
index 0000000000000..91bcddeeb7aa2
--- /dev/null
+++ b/packages/miniflare/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/);
+});