Remove special handling for 'Failed to start ProxyWorker or InspectorProxyWorker'

.changeset/famous-keys-walk.md

@@ -0,0 +1,5 @@
+---
+"wrangler": patch
+---
+
+Ensure `workerd` processes are cleaned up after address-in-use errors

packages/wrangler/e2e/dev.test.ts

@@ -1,4 +1,5 @@
 import assert from "node:assert";
+import childProcess from "node:child_process";
 import { existsSync } from "node:fs";
 import * as nodeNet from "node:net";
 import { setTimeout } from "node:timers/promises";
@@ -141,8 +142,140 @@
 	});
 });
 
+// Windows doesn't have a built-in way to get the CWD of a process by its ID.
+// This functionality is provided by the Windows Driver Kit which is installed
+// on GitHub actions Windows runners.
+const tlistPath =
+	"C:\\Program Files (x86)\\Windows Kits\\10\\Debuggers\\x86\\tlist.exe";
+let windowsProcessCwdSupported = true;
+if (process.platform === "win32" && !existsSync(tlistPath)) {
+	windowsProcessCwdSupported = false;
+	const message = [
+		"=".repeat(80),
+		"Unable to find Windows Driver Kit, skipping zombie process tests... :(",
+		"=".repeat(80),
+	].join("\n");
+	console.error(message);
+}
+
+interface Process {
+	pid: string;
+	cmd: string;
+}
+function getProcesses(): Process[] {
+	if (process.platform === "win32") {
+		return childProcess
+			.execSync("tasklist /fo csv", { encoding: "utf8" })
+			.trim()
+			.split("\r\n")
+			.slice(1)
+			.map((line) => {
+				const [cmd, pid] = line.replaceAll('"', "").split(",");
+				return { pid, cmd };
+			});
+	} else {
+		return childProcess
+			.execSync("ps -e | awk '{print $1,$4}'", { encoding: "utf8" })
+			.trim()
+			.split("\n")
+			.map((line) => {
+				const [pid, cmd] = line.split(" ");
+				return { pid, cmd };
+			});
+	}
+}
+function getProcessCwd(pid: string | number) {
+	if (process.platform === "win32") {
+		if (windowsProcessCwdSupported) {
+			return (
+				childProcess
+					.spawnSync(tlistPath, [String(pid)], { encoding: "utf8" })
+					.stdout.match(/^\s*CWD:\s*(.+)\\$/m)?.[1] ?? ""
+			);
+		} else {
+			return "";
+		}
+	} else {
+		return childProcess
+			.execSync(`lsof -p ${pid} | awk '$4=="cwd" {print $9}'`, {
+				encoding: "utf8",
+			})
+			.trim();
+	}
+}
+function getStartedWorkerdProcesses(cwd: string): Process[] {
+	return getProcesses()
+		.filter(({ cmd, pid }) => cmd.includes("workerd"))
+		.filter((c) => getProcessCwd(c.pid).includes(cwd));
+}
+
+it(`leaves no orphaned workerd processes with port conflict`, async () => {
+	const initial = new WranglerE2ETestHelper();
+	await initial.seed({
+		"wrangler.toml": dedent`
+						name = "${workerName}"
+						main = "src/index.ts"
+						compatibility_date = "2023-01-01"
+				`,
+		"src/index.ts": dedent`
+						export default {
+							fetch(request) {
+								return new Response("Hello World!")
+							}
+						}`,
+		"package.json": dedent`
+						{
+							"name": "worker",
+							"version": "0.0.0",
+							"private": true
+						}
+						`,
+	});
+	const initialWorker = initial.runLongLived(`wrangler dev`);
+
+	const { url: initialWorkerUrl } = await initialWorker.waitForReady();
+
+	const port = new URL(initialWorkerUrl).port;
+
+	const helper = new WranglerE2ETestHelper();
+	await helper.seed({
+		"wrangler.toml": dedent`
+						name = "${workerName}"
+						main = "src/index.ts"
+						compatibility_date = "2023-01-01"
+				`,
+		"src/index.ts": dedent`
+						export default {
+							fetch(request) {
+								return new Response("Hello World!")
+							}
+						}`,
+		"package.json": dedent`
+						{
+							"name": "worker",
+							"version": "0.0.0",
+							"private": true
+						}
+						`,
+	});
+	const beginProcesses = getStartedWorkerdProcesses(helper.tmpPath);
+	// If a port isn't specified, Wrangler will start up on a different random port. In this test we want to force an address-in-use error
+	const worker = helper.runLongLived(`wrangler dev --port ${port}`);
+
+	const exitCode = await worker.exitCode;
+
+	expect(exitCode).not.toBe(0);
+
+	const endProcesses = getStartedWorkerdProcesses(helper.tmpPath);
+
+	// Check no hanging workerd processes
+	if (process.platform !== "win32" || windowsProcessCwdSupported) {
+		expect(beginProcesses.length).toBe(endProcesses.length);
+	}
+});
+
 // Skipping remote python tests because they consistently flake with timeouts
 // Unskip once remote dev with python workers is more stable
 describe.each([
 	{ cmd: "wrangler dev --no-x-dev-env" },
 	{ cmd: "wrangler dev --x-dev-env" },

packages/wrangler/e2e/helpers/command.ts

@@ -77,7 +77,7 @@ export function runCommand(
 export class LongLivedCommand {
 	private lines: string[] = [];
 	private stream: ReadableStream;
-	private exitPromise: Promise<unknown>;
+	private exitPromise: Promise<[number, unknown]>;
 	private commandProcess: ChildProcessWithoutNullStreams;
 
 	constructor(
@@ -93,7 +93,9 @@ export class LongLivedCommand {
 			signal,
 		});
 
-		this.exitPromise = events.once(this.commandProcess, "exit");
+		this.exitPromise = events.once(this.commandProcess, "exit") as Promise<
+			[number, unknown]
+		>;
 
 		// Merge the stdout and stderr into a single output stream
 		const output = new PassThrough();
@@ -143,7 +145,7 @@ export class LongLivedCommand {
 	}
 
 	get exitCode() {
-		return this.exitPromise;
+		return this.exitPromise.then((e) => e[0]);
 	}
 
 	async stop() {

packages/wrangler/src/api/startDevWorker/DevEnv.ts

@@ -110,30 +110,6 @@ export class DevEnv extends EventEmitter {
 
 	emitErrorEvent(ev: ErrorEvent) {
 		if (
-			ev.source === "ProxyController" &&
-			ev.reason === "Failed to start ProxyWorker or InspectorProxyWorker"
-		) {
-			assert(ev.data.config); // we must already have a `config` if we've already tried (and failed) to instantiate the ProxyWorker(s)
-
-			const { config } = ev.data;
-			const port = config.dev?.server?.port;
-			const inspectorPort = config.dev?.inspector?.port;
-			const randomPorts = [0, undefined];
-
-			if (!randomPorts.includes(port) || !randomPorts.includes(inspectorPort)) {
-				// emit the event here while the ConfigController is unimplemented
-				// this will cause the ProxyController to try reinstantiating the ProxyWorker(s)
-				// TODO: change this to `this.config.updateOptions({ dev: { server: { port: 0 }, inspector: { port: 0 } } });` when the ConfigController is implemented
-				this.config.emitConfigUpdateEvent({
-					...config,
-					dev: {
-						...config.dev,
-						server: { ...config.dev?.server, port: 0 }, // override port
-						inspector: { ...config.dev?.inspector, port: 0 }, // override port
-					},
-				});
-			}
-		} else if (
 			ev.source === "ProxyController" &&
 			(ev.reason.startsWith("Failed to send message to") ||
 				ev.reason.startsWith("Could not connect to InspectorProxyWorker"))

packages/wrangler/src/api/startDevWorker/ProxyController.ts

@@ -177,7 +177,9 @@ export class ProxyController extends Controller<ProxyControllerEventMap> {
 		if (proxyWorkerOptionsChanged) {
 			logger.debug("ProxyWorker miniflare options changed, reinstantiating...");
 
-			void this.proxyWorker.setOptions(proxyWorkerOptions);
+			void this.proxyWorker.setOptions(proxyWorkerOptions).catch((error) => {
+				this.emitErrorEvent("Failed to start ProxyWorker", error);
+			});
 
 			// this creates a new .ready promise that will be resolved when both ProxyWorkers are ready
 			// it also respects any await-ers of the existing .ready promise
@@ -316,8 +318,6 @@ export class ProxyController extends Controller<ProxyControllerEventMap> {
 				`Failed to send message to ProxyWorker: ${JSON.stringify(message)}`,
 				error
 			);
-
-			throw error;
 		}
 	}
 	async sendMessageToInspectorProxyWorker(
@@ -351,8 +351,6 @@ export class ProxyController extends Controller<ProxyControllerEventMap> {
 				)}`,
 				error
 			);
-
-			throw error;
 		}
 	}