[wrangler] fix: ensure wrangler (pages) dev exits cleanly (#4693)

* fix: ensure `wrangler dev` exits with code `0` on clean exit

We were previously calling `process.disconnect()` twice: once in `dev`
specific code, and once in the CLI `main()` function. The 2nd call was
throwing with an already disconnected error causing `wrangler dev` to
exit with code `1`. This exception was being swallowed as `cli.ts`'s
`catch()` assumes the error has already been logged.

This change makes sure we only call `process.disconnect()` once, and
also logs errors in this `finally` block.

* fix: ensure `wrangler pages dev` exits cleanly

This change ensures the `onReady` function passed as a prop to
`InteractiveDevSession` is called when the hostname/port match the
initial values (the case for `wrangler pages dev` without flags).

This prevented the `Promise` returned by `unstable_dev()` in non-test
mode from resolving. This in turn prevented Pages' cleanup callbacks
being registered and `run pages dev` metrics being sent.

* test: add tests for interactive dev sessions and clean exits

This adds a new fixture test that runs `wrangler (pages) dev` in a
pseudo-TTY to test `InteractiveDevSession`. This allows us to test
`CTRL+C`/`x` clean exits without zombie `workerd` processes.

.changeset/chilled-kiwis-accept.md

@@ -0,0 +1,7 @@
+---
+"wrangler": patch
+---
+
+fix: ensure `wrangler dev` exits with code `0` on clean exit
+
+Previously, `wrangler dev` would exit with a non-zero exit code when pressing <kbd>CTRL</kbd>+<kbd>C</kbd> or <kbd>x</kbd>. This change ensures `wrangler` exits with code `0` in these cases.

.changeset/happy-waves-lie.md

@@ -0,0 +1,7 @@
+---
+"wrangler": patch
+---
+
+fix: ensure `wrangler pages dev` exits cleanly
+
+Previously, pressing <kbd>CTRL</kbd>+<kbd>C</kbd> or <kbd>x</kbd> when running `wrangler pages dev` wouldn't actually exit `wrangler`. You'd need to press <kbd>CTRL</kbd>+<kbd>C</kbd> a second time to exit the process. This change ensures `wrangler` exits the first time.

.prettierignore

@@ -21,6 +21,7 @@ fixtures/pages-functions-app/public/cdn-cgi/
 fixtures/remix-pages-app/functions/**
 fixtures/**/dist/**
 **/vendor/**
+**/.wrangler/**
 
 **/*.d.ts
 

fixtures/interactive-dev-tests/README.md

@@ -0,0 +1,3 @@
+# `interactive-dev-tests`
+
+This package contains tests for interactive `wrangler dev` sessions running in a pseudo-TTY. These have slightly different behaviour to non-interactive sessions tested by other fixtures.

fixtures/interactive-dev-tests/package.json

@@ -0,0 +1,17 @@
+{
+	"name": "interactive-dev-tests",
+	"version": "0.0.1",
+	"private": true,
+	"scripts": {
+		"test": "vitest run",
+		"test:ci": "vitest run",
+		"test:watch": "vitest"
+	},
+	"devDependencies": {
+		"strip-ansi": "^7.1.0",
+		"undici": "^5.28.2"
+	},
+	"optionalDependencies": {
+		"node-pty": "^1.0.0"
+	}
+}

fixtures/interactive-dev-tests/public/index.html

@@ -0,0 +1 @@
+<p>body</p>

fixtures/interactive-dev-tests/src/index.mjs

@@ -0,0 +1,5 @@
+export default {
+	async fetch() {
+		return new Response("body");
+	},
+};

fixtures/interactive-dev-tests/tests/index.test.ts

@@ -0,0 +1,229 @@
+import assert from "node:assert";
+import childProcess from "node:child_process";
+import fs from "node:fs";
+import path from "node:path";
+import rl from "node:readline";
+import stream from "node:stream";
+import stripAnsi from "strip-ansi";
+import { fetch } from "undici";
+import { afterEach, describe as baseDescribe, expect, it } from "vitest";
+import { wranglerEntryPath } from "../../shared/src/run-wrangler-long-lived";
+import type pty from "node-pty";
+
+// These tests are failing with `Error: read EPIPE` on Windows in CI. There's
+// still value running them on macOS and Linux.
+const RUN_IF = process.platform !== "win32";
+
+// 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" && !fs.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);
+}
+
+const pkgRoot = path.resolve(__dirname, "..");
+const ptyOptions: pty.IPtyForkOptions = {
+	name: "xterm-color",
+	cols: 80,
+	rows: 30,
+	cwd: pkgRoot,
+	env: process.env,
+};
+
+// Check `node-pty` installed and working correctly, skipping tests if not
+let nodePtySupported = true;
+try {
+	const pty = await import("node-pty");
+	const ptyProcess = pty.spawn(
+		process.execPath,
+		["-p", "'ran node'"],
+		ptyOptions
+	);
+	let output = "";
+	ptyProcess.onData((data) => (output += data));
+	const code = await new Promise<number>((resolve) =>
+		ptyProcess.onExit(({ exitCode }) => resolve(exitCode))
+	);
+	assert.strictEqual(code, 0);
+	assert(output.includes("ran node"));
+} catch (e) {
+	nodePtySupported = false;
+	const message = [
+		"=".repeat(80),
+		"`node-pty` unsupported, skipping interactive dev session tests... :(",
+		"",
+		"Ensure its dependencies (https://github.com/microsoft/node-pty#dependencies)",
+		"are installed, then re-run `pnpm install` in the repository root.",
+		"",
+		"On Windows, make sure you have `Desktop development with C++`, `Windows SDK`,",
+		"`MSVC VS C++ build tools`, and `MSVC VS C++ Spectre-mitigated libs` Visual",
+		"Studio components installed.",
+		"",
+		e instanceof Error ? e.stack : String(e),
+		"=".repeat(80),
+	].join("\n");
+	console.error(message);
+}
+const describe = baseDescribe.runIf(RUN_IF && nodePtySupported);
+
+interface PtyProcess {
+	pty: pty.IPty;
+	stdout: string;
+	exitCode: number | null;
+	exitPromise: Promise<number>;
+	url: string;
+}
+const processes: PtyProcess[] = [];
+afterEach(() => {
+	for (const p of processes.splice(0)) {
+		// If the process didn't exit cleanly, log its output for debugging
+		if (p.exitCode !== 0) console.log(stripAnsi(p.stdout));
+		// If the process hasn't exited yet, kill it
+		if (p.exitCode === null) {
+			// `node-pty` throws if signal passed on Windows
+			if (process.platform === "win32") p.pty.kill();
+			else p.pty.kill("SIGKILL");
+		}
+	}
+});
+
+const readyRegexp = /Ready on (http:\/\/[a-z0-9.]+:[0-9]+)/;
+async function startWranglerDev(args: string[]) {
+	const stdoutStream = new stream.PassThrough();
+	const stdoutInterface = rl.createInterface(stdoutStream);
+
+	let exitResolve: ((code: number) => void) | undefined;
+	const exitPromise = new Promise<number>((resolve) => (exitResolve = resolve));
+
+	const pty = await import("node-pty");
+	const ptyProcess = pty.spawn(
+		process.execPath,
+		[
+			wranglerEntryPath,
+			...args,
+			"--ip=127.0.0.1",
+			"--port=0",
+			"--inspector-port=0",
+		],
+		ptyOptions
+	);
+	const result: PtyProcess = {
+		pty: ptyProcess,
+		stdout: "",
+		exitCode: null,
+		exitPromise,
+		url: "",
+	};
+	processes.push(result);
+	ptyProcess.onData((data) => {
+		result.stdout += data;
+		stdoutStream.write(data);
+	});
+	ptyProcess.onExit(({ exitCode }) => {
+		result.exitCode = exitCode;
+		exitResolve?.(exitCode);
+		stdoutStream.end();
+	});
+
+	let readyMatch: RegExpMatchArray | null = null;
+	for await (const line of stdoutInterface) {
+		if ((readyMatch = readyRegexp.exec(line)) !== null) break;
+	}
+	assert(readyMatch !== null, "Expected ready message");
+	result.url = readyMatch[1];
+
+	return result;
+}
+
+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(): Process[] {
+	return getProcesses().filter(
+		({ cmd, pid }) => cmd.includes("workerd") && getProcessCwd(pid) === pkgRoot
+	);
+}
+
+const devCommands = [
+	{ args: ["dev"], expectedBody: "body" },
+	{ args: ["pages", "dev", "public"], expectedBody: "<p>body</p>" },
+];
+const exitKeys = [
+	{ name: "CTRL-C", key: "\x03" },
+	{ name: "x", key: "x" },
+];
+
+describe.each(devCommands)("wrangler $args", ({ args, expectedBody }) => {
+	it.each(exitKeys)("cleanly exits with $name", async ({ key }) => {
+		const beginProcesses = getStartedWorkerdProcesses();
+
+		const wrangler = await startWranglerDev(args);
+		const duringProcesses = getStartedWorkerdProcesses();
+
+		// Check dev server working correctly
+		const res = await fetch(wrangler.url);
+		expect((await res.text()).trim()).toBe(expectedBody);
+
+		// Check key cleanly exits dev server
+		wrangler.pty.write(key);
+		expect(await wrangler.exitPromise).toBe(0);
+		const endProcesses = getStartedWorkerdProcesses();
+
+		// Check no hanging workerd processes
+		if (process.platform !== "win32" || windowsProcessCwdSupported) {
+			expect(beginProcesses.length).toBe(endProcesses.length);
+			expect(duringProcesses.length).toBeGreaterThan(beginProcesses.length);
+		}
+	});
+});

fixtures/interactive-dev-tests/tsconfig.json

@@ -0,0 +1,12 @@
+{
+	"compilerOptions": {
+		"isolatedModules": true,
+		"esModuleInterop": true,
+		"allowSyntheticDefaultImports": true,
+		"moduleResolution": "node",
+		"target": "esnext",
+		"module": "esnext",
+		"strict": true,
+		"lib": ["esnext"]
+	}
+}

fixtures/interactive-dev-tests/vitest.config.ts

@@ -0,0 +1,12 @@
+import { defineConfig } from "vitest/config";
+
+export default defineConfig({
+	test: {
+		testTimeout: 30_000,
+		hookTimeout: 30_000,
+		teardownTimeout: 30_000,
+		useAtomics: true,
+		// `node-pty` doesn't work inside worker threads
+		threads: false,
+	},
+});

fixtures/interactive-dev-tests/wrangler.toml

@@ -0,0 +1,2 @@
+main = "src/index.mjs"
+compatibility_date = "2023-12-01"

package.json

@@ -73,6 +73,13 @@
 			"toucan-js@3.2.2": "patches/toucan-js@3.2.2.patch",
 			"@cloudflare/component-listbox@1.10.6": "patches/@cloudflare__component-listbox@1.10.6.patch",
 			"capnp-ts@0.7.0": "patches/capnp-ts@0.7.0.patch"
+		},
+		"packageExtensions": {
+			"node-pty": {
+				"optionalDependencies": {
+					"node-gyp": "^10.0.1"
+				}
+			}
 		}
 	}
 }

packages/wrangler/src/dev.tsx

@@ -488,27 +488,11 @@ export async function startDev(args: StartDevOptions) {
 		}
 		const devReactElement = render(await getDevReactElement(config));
 
-		// In the bootstrapper script `bin/wrangler.js`, we open an IPC channel, so
-		// IPC messages from this process are propagated through the bootstrapper.
-		// Normally, Node's SIGINT handler would close this for us, but interactive
-		// mode enables raw mode on stdin which disables the built-in handler. The
-		// following line disconnects from the IPC channel when we press `x` or
-		// CTRL-C in interactive mode, ensuring no open handles, and allowing for a
-		// clean exit. Note, if we called `stop()` using the dev API, we don't want
-		// to disconnect here, as the user may still need IPC. We also don't want
-		// to disconnect if this file was imported in Jest (not the case with E2E
-		// tests), as that would stop communication with the test runner.
-		let apiStopped = false;
-		void devReactElement.waitUntilExit().then(() => {
-			if (!apiStopped && typeof jest === "undefined") process.disconnect?.();
-		});
-
 		rerender = devReactElement.rerender;
 		return {
 			devReactElement,
 			watcher,
 			stop: async () => {
-				apiStopped = true;
 				devReactElement.unmount();
 				await watcher?.close();
 			},

packages/wrangler/src/dev/dev.tsx

@@ -216,13 +216,9 @@ function InteractiveDevSession(props: DevProps) {
 	useTunnel(toggles.tunnel);
 
 	const onReady = (newIp: string, newPort: number, proxyData: ProxyData) => {
-		if (newIp !== props.initialIp || newPort !== props.initialPort) {
-			ip = newIp;
-			port = newPort;
-			if (props.onReady) {
-				props.onReady(newIp, newPort, proxyData);
-			}
-		}
+		ip = newIp;
+		port = newPort;
+		props.onReady?.(newIp, newPort, proxyData);
 	};
 
 	return (