Remove special handling for 'Failed to start ProxyWorker or InspectorProxyWorker' (#7169)

Author: penalosa

.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";
@@ -211,6 +212,102 @@ describe.each([
 	});
 });
 
+interface Process {
+	pid: string;
+	cmd: string;
+}
+
+function getProcesses(): Process[] {
+	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) {
+	return childProcess
+		.execSync(`lsof -p ${pid} | awk '$4=="cwd" {print $9}'`, {
+			encoding: "utf8",
+		})
+		.trim();
+}
+function getStartedWorkerdProcesses(cwd: string): Process[] {
+	return getProcesses()
+		.filter(({ cmd }) => cmd.includes("workerd"))
+		.filter((c) => getProcessCwd(c.pid).includes(cwd));
+}
+
+// This fails on Windows because of https://github.com/cloudflare/workerd/issues/1664
+it.runIf(process.platform !== "win32")(
+	`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);
+
+		expect(beginProcesses.length).toBe(0);
+		expect(endProcesses.length).toBe(0);
+	}
+);
+
 // Skipping remote python tests because they consistently flake with timeouts
 // Unskip once remote dev with python workers is more stable
 describe.each([

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;
 		}
 	}