From 9098a3b03f82bbfb1fb8c8c531fafbfe26a49e59 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Somhairle=20MacLe=C3=B2id?= Date: Thu, 7 Nov 2024 15:31:00 +0000 Subject: [PATCH] Remove special handling for 'Failed to start ProxyWorker or InspectorProxyWorker' (#7169) --- .changeset/famous-keys-walk.md | 5 + packages/wrangler/e2e/dev.test.ts | 97 +++++++++++++++++++ packages/wrangler/e2e/helpers/command.ts | 8 +- .../wrangler/src/api/startDevWorker/DevEnv.ts | 24 ----- .../src/api/startDevWorker/ProxyController.ts | 8 +- 5 files changed, 110 insertions(+), 32 deletions(-) create mode 100644 .changeset/famous-keys-walk.md diff --git a/.changeset/famous-keys-walk.md b/.changeset/famous-keys-walk.md new file mode 100644 index 000000000000..22481a44d6a8 --- /dev/null +++ b/.changeset/famous-keys-walk.md @@ -0,0 +1,5 @@ +--- +"wrangler": patch +--- + +Ensure `workerd` processes are cleaned up after address-in-use errors diff --git a/packages/wrangler/e2e/dev.test.ts b/packages/wrangler/e2e/dev.test.ts index 3af2a1862c39..e586f985c7b7 100644 --- a/packages/wrangler/e2e/dev.test.ts +++ b/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([ diff --git a/packages/wrangler/e2e/helpers/command.ts b/packages/wrangler/e2e/helpers/command.ts index 8942ab9c85ed..4a7f1196d654 100644 --- a/packages/wrangler/e2e/helpers/command.ts +++ b/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; + 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() { diff --git a/packages/wrangler/src/api/startDevWorker/DevEnv.ts b/packages/wrangler/src/api/startDevWorker/DevEnv.ts index 500b3117f112..41c0e0be51d3 100644 --- a/packages/wrangler/src/api/startDevWorker/DevEnv.ts +++ b/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")) diff --git a/packages/wrangler/src/api/startDevWorker/ProxyController.ts b/packages/wrangler/src/api/startDevWorker/ProxyController.ts index 8243afb33954..b07512be8333 100644 --- a/packages/wrangler/src/api/startDevWorker/ProxyController.ts +++ b/packages/wrangler/src/api/startDevWorker/ProxyController.ts @@ -177,7 +177,9 @@ export class ProxyController extends Controller { 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 { `Failed to send message to ProxyWorker: ${JSON.stringify(message)}`, error ); - - throw error; } } async sendMessageToInspectorProxyWorker( @@ -351,8 +351,6 @@ export class ProxyController extends Controller { )}`, error ); - - throw error; } }