Kill a timed-out test's whole process group in the test runner

scripts/run-tests.ts

@@ -16,7 +16,7 @@
 //
 // Pass names or substrings to run a subset: `npx tsx scripts/run-tests.ts cliche`
 import { readdirSync, readFileSync } from 'node:fs';
-import { spawn } from 'node:child_process';
+import { spawn, type ChildProcess } from 'node:child_process';
 
 /** Tests that never terminate on their own and are not batch-runnable. */
 const INTERACTIVE = new Set([
@@ -41,7 +41,8 @@ const CHROME_ANYWAY = new Set([
  * in screenshot.ts that every browser-driven test reaches for. */
 const CHROME_MARKERS = /takeScreenshot|computeLayout|withPage|withIsolatedPage|puppeteer|from '\.\/src\/screenshot\.js'/;
 
-const TIMEOUT_MS = 180_000;
+// FRAMESMITH_TEST_TIMEOUT_MS overrides it (test-runner-timeout.ts uses 3s).
+const TIMEOUT_MS = Number(process.env.FRAMESMITH_TEST_TIMEOUT_MS) || 180_000;
 
 type Tier = 'fast' | 'full';
 const tier: Tier = process.argv.includes('--full') ? 'full' : 'fast';
@@ -59,14 +60,28 @@ const selected = all
   .filter((f) => tier === 'full' || !needsChrome(f))
   .filter((f) => filters.length === 0 || filters.some((q) => f.includes(q)));
 
+/** The test running now. Its own process group doesn't get the terminal's
+ * Ctrl-C, so the runner passes an interrupt on before exiting. */
+let current: ChildProcess | undefined;
+for (const signal of ['SIGINT', 'SIGTERM'] as const) {
+  process.on(signal, () => {
+    if (current?.pid) { try { process.kill(-current.pid, 'SIGKILL'); } catch { /* already gone */ } }
+    process.exit(130);
+  });
+}
+
 function run(file: string): Promise<{ file: string; ok: boolean; why: string; ms: number }> {
   return new Promise((resolve) => {
     const started = Date.now();
-    const child = spawn('npx', ['tsx', file], { stdio: ['ignore', 'pipe', 'pipe'] });
+    // Its own process group, so a timeout kills npx AND the tsx under it.
+    // Killing npx alone orphaned tsx, which kept the pipes open, and 'close'
+    // never fired: one hung test stalled the whole run.
+    const child = spawn('npx', ['tsx', file], { stdio: ['ignore', 'pipe', 'pipe'], detached: true });
+    current = child;
     let out = '';
     child.stdout.on('data', (d) => { out += d; });
     child.stderr.on('data', (d) => { out += d; });
-    const timer = setTimeout(() => child.kill('SIGKILL'), TIMEOUT_MS);
+    const timer = setTimeout(() => process.kill(-child.pid!, 'SIGKILL'), TIMEOUT_MS);
     child.on('close', (code, signal) => {
       clearTimeout(timer);
       const ms = Date.now() - started;

test-runner-timeout.ts

@@ -0,0 +1,64 @@
+// scripts/run-tests.ts must survive a hung test. Its timeout used to SIGKILL
+// only the `npx` wrapper; the `tsx` process under it was orphaned, kept the
+// output pipes open, and the runner waited on them forever (a Chrome hang in
+// test-text-props.ts stalled a full-tier run for 25 minutes). This runs the
+// real runner in a temp dir holding one hanging test and one passing test,
+// with a 3s timeout, and checks it reports the timeout, finishes, and leaves
+// no process of the hung test behind.
+//
+// Usage: npx tsx test-runner-timeout.ts
+
+import { mkdtempSync, symlinkSync, writeFileSync } from 'node:fs';
+import { tmpdir } from 'node:os';
+import { join, resolve } from 'node:path';
+import { spawn, execFileSync } from 'node:child_process';
+
+let allPass = true;
+function check(name: string, cond: boolean, extra?: string) {
+  if (!cond) allPass = false;
+  console.log(`${cond ? 'PASS' : 'FAIL'}  ${name}${extra ? ` — ${extra}` : ''}`);
+}
+
+const dir = mkdtempSync(join(tmpdir(), 'framesmith-runner-timeout-'));
+symlinkSync(resolve('node_modules'), join(dir, 'node_modules'));
+writeFileSync(join(dir, 'test-hang.ts'), 'setInterval(() => {}, 1_000);\n');
+writeFileSync(join(dir, 'test-quick.ts'), "console.log('PASS  quick');\n");
+
+/** PIDs of processes still running the hung test. */
+const leftovers = (): number[] => {
+  try {
+    return execFileSync('pgrep', ['-f', 'tsx test-hang\\.ts'], { encoding: 'utf-8' }).split('\n').filter(Boolean).map(Number);
+  } catch {
+    return []; // pgrep exits 1 when nothing matches
+  }
+};
+
+const started = Date.now();
+const runner = spawn(resolve('node_modules/.bin/tsx'), [resolve('scripts/run-tests.ts')], {
+  cwd: dir,
+  env: { ...process.env, FRAMESMITH_TEST_TIMEOUT_MS: '3000' },
+  stdio: ['ignore', 'pipe', 'pipe'],
+  detached: true,
+});
+let out = '';
+runner.stdout.on('data', (d) => { out += d; });
+runner.stderr.on('data', (d) => { out += d; });
+
+// Our own guard: a runner that never finishes must not hang this test too.
+const finished = await new Promise<boolean>((done) => {
+  const guard = setTimeout(() => done(false), 45_000);
+  runner.on('close', () => { clearTimeout(guard); done(true); });
+});
+const secs = (Date.now() - started) / 1000;
+
+check('the runner finishes despite a hung test', finished, `${secs.toFixed(1)}s`);
+check('the hung test is reported as timed out', /FAIL {2}test-hang\.ts {2}timed out after 3s/.test(out), out.split('\n').find((l) => l.includes('test-hang')));
+check('the other test still runs and passes', /ok {4}test-quick\.ts/.test(out) && /1\/2 passed/.test(out));
+check('no process of the hung test is left running', leftovers().length === 0, leftovers().join(', '));
+
+// Clean up whatever the old behaviour left behind, by exact PID.
+if (!finished) { try { process.kill(-runner.pid!, 'SIGKILL'); } catch { /* already gone */ } }
+for (const pid of leftovers()) { try { process.kill(pid, 'SIGKILL'); } catch { /* already gone */ } }
+
+console.log(allPass ? '\nAll runner-timeout checks passed.' : '\nSome runner-timeout checks FAILED.');
+process.exit(allPass ? 0 : 1);