yukey

e2e: signal handlers before Chrome starts; a dead Chrome fails fast

commit 3bee2a2df839f16d05a9e97cf25b215d82dd8b8c · parent d32b1d6ae9 · browse this tree
Ximianth, 2026-10-10 05:55:44 UTC

SIGINT during Harness.start() exited 130 but left Chrome (its own process
group) and the profile dir behind, because main.ts installed the handlers
only after start() returned.  Reproduced with kill -INT at 0.25s: 6 Chrome
processes and 1 profile dir orphaned.  The handlers now go in inside start(),
right after spawn, with the same kill() the throw path uses; main.ts no
longer installs its own.

chrome.on("exit") fails the run with the code/signal as soon as Chrome dies
on its own.  Before, YUKEY_E2E_CHROME=/bin/true sat through the 15s poll
deadline; now it exits in under a second.  kill() sets a flag so our own
SIGKILL at teardown isn't reported as unexpected.

Harness keeps only the kill closure now, rather than the server, child,
and profile dir separately; stop() is cdp.close() plus kill().

make check-e2e 215/215 after.

diff --git a/packages/e2e/harness.ts b/packages/e2e/harness.ts
index a485a96..4f9941c 100644
--- a/packages/e2e/harness.ts
+++ b/packages/e2e/harness.ts
@@ -6,7 +6,7 @@
 // Everything here is per-run and ephemeral: fresh Chrome profile, ephemeral
 // port for the static server, no shared state with anything else.
 
-import { type ChildProcess, spawn } from "node:child_process";
+import { spawn } from "node:child_process";
 import { accessSync, constants, mkdtempSync, readFileSync, rmSync } from "node:fs";
 import { createServer, type Server } from "node:http";
 import { tmpdir } from "node:os";
@@ -79,23 +79,12 @@ function serveStatic(): Promise<{ baseUrl: string; server: Server }> {
 export class Harness {
 	readonly baseUrl: string;
 	readonly cdp: Cdp;
-	private readonly server: Server;
-	private readonly chrome: ChildProcess;
-	private readonly profileDir: string;
-	private stopped = false;
-
-	private constructor(
-		baseUrl: string,
-		cdp: Cdp,
-		server: Server,
-		chrome: ChildProcess,
-		profileDir: string,
-	) {
+	private readonly kill: () => void;
+
+	private constructor(baseUrl: string, cdp: Cdp, kill: () => void) {
 		this.baseUrl = baseUrl;
 		this.cdp = cdp;
-		this.server = server;
-		this.chrome = chrome;
-		this.profileDir = profileDir;
+		this.kill = kill;
 	}
 
 	static async start(): Promise<Harness> {
@@ -131,7 +120,13 @@ export class Harness {
 			{ stdio: "ignore", detached: true },
 		);
 
+		// Unconditional, idempotent teardown of everything spawned above: stop the
+		// server, kill the Chrome tree, remove the profile dir.  Every exit path
+		// below funnels through it, including stop().
+		let killed = false;
 		const kill = () => {
+			if (killed) return;
+			killed = true;
 			server.close();
 			if (chrome.pid !== undefined) {
 				try {
@@ -141,6 +136,26 @@ export class Harness {
 			rmSync(profileDir, { recursive: true, force: true });
 		};
 
+		// Signal handlers go in before anything waits, so a Ctrl-C during Chrome's
+		// startup still takes Chrome and the profile dir with it.
+		process.on("SIGINT", () => {
+			kill();
+			process.exit(130);
+		});
+		process.on("SIGTERM", () => {
+			kill();
+			process.exit(143);
+		});
+
+		// A Chrome that dies on its own fails the run now, not at a poll deadline.
+		// Our own SIGKILL in kill() fires this too; `killed` tells the two apart.
+		chrome.on("exit", (code, signal) => {
+			if (killed) return;
+			console.error(`chrome exited unexpectedly (code ${code}, signal ${signal})`);
+			kill();
+			process.exit(1);
+		});
+
 		try {
 			// Chrome publishes its chosen debugging port in the profile dir.
 			let debugPort = 0;
@@ -170,27 +185,18 @@ export class Harness {
 			});
 
 			const cdp = await Cdp.connect(wsUrl);
-			return new Harness(baseUrl, cdp, server, chrome, profileDir);
+			return new Harness(baseUrl, cdp, kill);
 		} catch (err) {
 			kill();
 			throw err;
 		}
 	}
 
-	// Idempotent, unconditional teardown: close the socket, stop the server,
-	// kill the Chrome process tree, remove the profile dir.
+	// Close the socket and tear down the server, Chrome, and profile dir.
 	stop(): void {
-		if (this.stopped) return;
-		this.stopped = true;
 		try {
 			this.cdp.close();
 		} catch {}
-		this.server.close();
-		if (this.chrome.pid !== undefined) {
-			try {
-				process.kill(-this.chrome.pid, "SIGKILL");
-			} catch {}
-		}
-		rmSync(this.profileDir, { recursive: true, force: true });
+		this.kill();
 	}
 }
diff --git a/packages/e2e/main.ts b/packages/e2e/main.ts
index 77a8c29..f23b4c4 100644
--- a/packages/e2e/main.ts
+++ b/packages/e2e/main.ts
@@ -24,15 +24,8 @@ async function main(): Promise<void> {
 	}
 	const chosen = wanted.length > 0 ? ALL.filter((s) => wanted.includes(s.name)) : ALL;
 
+	// Harness.start owns the SIGINT/SIGTERM handlers (and the exit codes 130/143).
 	const harness = await Harness.start();
-	process.on("SIGINT", () => {
-		harness.stop();
-		process.exit(130);
-	});
-	process.on("SIGTERM", () => {
-		harness.stop();
-		process.exit(143);
-	});
 
 	let failedScenarios = 0;
 	const summary: string[] = [];