fix(test): wait for interactive PTY sessions to end during cleanup

integration-tests/test-helper.test.ts

@@ -6,7 +6,7 @@
 
 import { existsSync } from 'node:fs';
 import { afterEach, describe, expect, it, vi } from 'vitest';
-import { TestRig } from './test-helper.js';
+import { INTERACTIVE_EXIT_GRACE_MS, TestRig } from './test-helper.js';
 
 function isProcessAlive(pid: number): boolean {
   try {
@@ -17,6 +17,19 @@ function isProcessAlive(pid: number): boolean {
   }
 }
 
+// How long the stand-in below stays alive after it is signalled. The real CLI
+// traps SIGHUP and exits only once its own exit-cleanup chain has drained, so
+// a stand-in that dies on the default action would let cleanup() return early
+// and still look correct.
+const STAND_IN_EXIT_DELAY_MS = 750;
+
+// Every interactive case below stands in for the CLI through rig.bundlePath.
+// The installed-release lane spawns the installed CLI instead and never runs a
+// stand-in, so these cases cannot measure that lane directly — the wrapper
+// stand-in below reproduces its process topology on the bundle lane instead.
+const usesInstalledCli =
+  process.env['INTEGRATION_TEST_USE_INSTALLED_GEMINI'] === 'true';
+
 describe('TestRig', () => {
   const originalKeepOutput = process.env['KEEP_OUTPUT'];
 
@@ -63,31 +76,151 @@ describe('TestRig', () => {
     expect(existsSync(testDir)).toBe(true);
   });
 
-  it('kills an interactive session a test never closed during cleanup', async () => {
-    // KEEP_OUTPUT is what CI sets, and it makes cleanup() keep the test
-    // directory — the spawned child must not survive that path either.
-    process.env['KEEP_OUTPUT'] = 'true';
-    const rig = new TestRig();
-    await rig.setup('cleanup kills interactive session');
-    // Stands in for the CLI bundle: what is under test is that cleanup ends
-    // whatever runInteractive spawned, not what the CLI itself does.
-    rig.bundlePath = rig.createFile(
-      'idle-cli.js',
-      'setInterval(() => {}, 1000);\n',
-    );
+  it.skipIf(usesInstalledCli)(
+    'waits for an interactive session a test never closed to end',
+    async () => {
+      // KEEP_OUTPUT is what CI sets, and it makes cleanup() keep the test
+      // directory — the spawned child must not survive that path either.
+      process.env['KEEP_OUTPUT'] = 'true';
+      const rig = new TestRig();
+      await rig.setup('cleanup kills interactive session');
+      // Stands in for the CLI bundle: what is under test is that cleanup ends
+      // whatever runInteractive spawned, not what the CLI itself does.
+      rig.bundlePath = rig.createFile(
+        'slow-exit-cli.js',
+        'process.on("SIGHUP", () => setTimeout(() => process.exit(129), ' +
+          `${STAND_IN_EXIT_DELAY_MS}));\n` +
+          'setInterval(() => {}, 1000);\n' +
+          'process.stdout.write("STAND_IN_READY\\n");\n',
+      );
 
-    const { ptyProcess } = rig.runInteractive();
-    expect(isProcessAlive(ptyProcess.pid)).toBe(true);
+      const { ptyProcess } = rig.runInteractive();
+      expect(isProcessAlive(ptyProcess.pid)).toBe(true);
+      // Signal before the handler is installed and the default action ends the
+      // child at once, measuring nothing. A real session is booted by the time
+      // its test ends, so wait for the stand-in to report itself up.
+      expect(await rig.waitForText('STAND_IN_READY', 30_000)).toBe(true);
 
-    await rig.cleanup();
+      const cleanupStartedAt = Date.now();
+      await rig.cleanup();
+      const cleanupTookMs = Date.now() - cleanupStartedAt;
 
-    await expect
-      .poll(() => isProcessAlive(ptyProcess.pid), {
-        message: 'the interactive CLI child outlived cleanup()',
-        timeout: 10_000,
-      })
-      .toBe(false);
-  });
+      // Signalling alone returns straight through the delay above, leaving the
+      // child forwarding PTY bytes into a worker vitest is tearing down.
+      expect(
+        cleanupTookMs,
+        'cleanup() returned before the interactive CLI child exited',
+      ).toBeGreaterThanOrEqual(STAND_IN_EXIT_DELAY_MS);
+      // Nor may it fall through the whole grace, which is what cleanup() does
+      // when `exited` never settles — i.e. when the onExit wiring breaks. The
+      // bound sits far above the stand-in's exit delay, far below the grace.
+      expect(cleanupTookMs).toBeLessThan(5_000);
+      await expect
+        .poll(() => isProcessAlive(ptyProcess.pid), {
+          message: 'the interactive CLI child outlived cleanup()',
+          timeout: 10_000,
+        })
+        .toBe(false);
+    },
+  );
+
+  it.skipIf(usesInstalledCli)(
+    'waits for the CLI the installed bin wrapper relaunched to end',
+    async () => {
+      process.env['KEEP_OUTPUT'] = 'true';
+      const rig = new TestRig();
+      await rig.setup('cleanup waits past the bin wrapper');
+      // Mimics the installed-release lane, where the bin wrapper node-pty
+      // spawns relaunches the real CLI with spawnSync and installs no signal
+      // handler: SIGHUP ends the wrapper at once, while the relaunched CLI
+      // traps it and keeps draining. That CLI, not the wrapper, is what
+      // cleanup() must not return early on.
+      const relaunched = rig.createFile(
+        'relaunched-cli.cjs',
+        'process.on("SIGHUP", () => setTimeout(() => process.exit(129), ' +
+          `${STAND_IN_EXIT_DELAY_MS}));\n` +
+          'setInterval(() => {}, 1000);\n' +
+          'process.stdout.write("RELAUNCHED_PID=" + process.pid + ' +
+          '"\\nRELAUNCHED_READY\\n");\n',
+      );
+      rig.bundlePath = rig.createFile(
+        'bin-wrapper.cjs',
+        "const { spawnSync } = require('node:child_process');\n" +
+          `const result = spawnSync(process.execPath, [${JSON.stringify(
+            relaunched,
+          )}], {\n` +
+          "  stdio: 'inherit',\n" +
+          '});\n' +
+          'if (result.signal) process.kill(process.pid, result.signal);\n' +
+          'else process.exit(result.status ?? 1);\n',
+      );
+
+      const { ptyProcess } = rig.runInteractive();
+      expect(await rig.waitForText('RELAUNCHED_READY', 30_000)).toBe(true);
+      const reported = /RELAUNCHED_PID=(\d+)/.exec(rig._interactiveOutput);
+      expect(
+        reported,
+        'the relaunched CLI never reported its pid',
+      ).not.toBeNull();
+      const relaunchedPid = Number(reported![1]);
+      expect(isProcessAlive(relaunchedPid)).toBe(true);
+
+      const cleanupStartedAt = Date.now();
+      await rig.cleanup();
+
+      // The wrapper dies on SIGHUP's default action within milliseconds, so
+      // only a wait that sees past it can still be running here.
+      expect(Date.now() - cleanupStartedAt).toBeGreaterThanOrEqual(
+        STAND_IN_EXIT_DELAY_MS,
+      );
+      expect(
+        isProcessAlive(relaunchedPid),
+        'cleanup() returned while the relaunched CLI was still draining',
+      ).toBe(false);
+      expect(isProcessAlive(ptyProcess.pid)).toBe(false);
+    },
+  );
+
+  it.skipIf(usesInstalledCli)(
+    'stops waiting for an interactive child that never exits',
+    async () => {
+      process.env['KEEP_OUTPUT'] = 'true';
+      const rig = new TestRig();
+      await rig.setup('cleanup gives up on a child that never exits');
+      rig.bundlePath = rig.createFile(
+        'never-exit-cli.js',
+        'process.on("SIGHUP", () => {});\n' +
+          'setInterval(() => {}, 1000);\n' +
+          'process.stdout.write("STAND_IN_READY\\n");\n',
+      );
+
+      const { ptyProcess } = rig.runInteractive();
+      expect(await rig.waitForText('STAND_IN_READY', 30_000)).toBe(true);
+      const warn = vi.spyOn(console, 'warn').mockImplementation(() => {});
+
+      const cleanupStartedAt = Date.now();
+      try {
+        await rig.cleanup();
+      } finally {
+        // cleanup() gave up on this child, so the rig no longer tracks it.
+        ptyProcess.kill('SIGKILL');
+      }
+      const cleanupTookMs = Date.now() - cleanupStartedAt;
+
+      // Literals, not the constant: a bound derived from it retunes with the
+      // value it polices. Floor is the CLI's 5s exit-cleanup chain, ceiling
+      // vitest's 10s default hookTimeout — see INTERACTIVE_EXIT_GRACE_MS.
+      expect(INTERACTIVE_EXIT_GRACE_MS).toBeGreaterThan(5_000);
+      expect(INTERACTIVE_EXIT_GRACE_MS).toBeLessThan(10_000);
+      // Bounded, or a child that ignores SIGHUP hangs teardown forever.
+      expect(cleanupTookMs).toBeLessThan(INTERACTIVE_EXIT_GRACE_MS + 5_000);
+      // Giving up has to name the child it abandoned, or the EPIPE crash this
+      // wait exists to prevent returns with nothing pointing back at it.
+      expect(warn.mock.calls.flat().join(' ')).toContain(
+        String(ptyProcess.pid),
+      );
+    },
+  );
 
   it.each([
     [

integration-tests/test-helper.ts

@@ -129,6 +129,62 @@ export function validateModelOutput(
   return true;
 }
 
+// The CLI traps SIGHUP and exits only once `runExitCleanup()` has drained, a
+// chain it bounds at 5s (packages/cli/src/utils/cleanup.ts), so the grace has
+// to outlast that. It also has to stay inside the 10s hookTimeout vitest
+// defaults to, because cleanup() runs in afterEach hooks: a grace that eats
+// the whole hook budget is reported as a generic "Hook timed out" instead of
+// naming the child that never exited.
+export const INTERACTIVE_EXIT_GRACE_MS = 8_000;
+
+// `process.kill(-pid, 0)` reports whether anything is left in the process
+// group `pid` leads; it throws once the group is empty. Windows has no
+// negative-pid process groups and always throws, so there the child's own exit
+// stays the whole bound.
+function sessionAlive(pid: number): boolean {
+  try {
+    process.kill(-pid, 0);
+    return true;
+  } catch {
+    return false;
+  }
+}
+
+// Resolves true once the whole PTY session is gone, false when `ms` elapses
+// first. `exited` covers only the process node-pty spawned, and node-pty
+// signals only that pid — on the installed-release lane it is the bin wrapper,
+// which relaunches the real CLI with spawnSync and dies on SIGHUP's default
+// action at once, so `exited` settles while that CLI is still draining into
+// the PTY. The spawned process leads its own process group and the
+// relaunched CLI stays in it, and a group reads empty only once every member
+// has exited and been reaped, so wait for both inside the same grace. Timers
+// are unrefed so a lost race leaves no handle holding the worker's event loop
+// open.
+function sessionEndsWithin(
+  pid: number,
+  exited: Promise<unknown>,
+  ms: number,
+): Promise<boolean> {
+  return new Promise((resolve) => {
+    const deadline = Date.now() + ms;
+    let childExited = false;
+    const check = () => {
+      const gone = childExited && !sessionAlive(pid);
+      if (gone || Date.now() >= deadline) {
+        clearInterval(timer);
+        resolve(gone);
+      }
+    };
+    const onChildExit = () => {
+      childExited = true;
+      check();
+    };
+    const timer = setInterval(check, 50);
+    timer.unref();
+    void exited.then(onChildExit, onChildExit);
+  });
+}
+
 // Simulates typing a string one character at a time to avoid paste detection.
 export async function type(ptyProcess: pty.IPty, text: string) {
   const delay = 5;
@@ -205,7 +261,10 @@ export class TestRig {
   testName?: string;
   _lastRunStdout?: string;
   _interactiveOutput = '';
-  private readonly interactiveProcesses: pty.IPty[] = [];
+  private readonly interactiveProcesses: Array<{
+    ptyProcess: pty.IPty;
+    exited: Promise<unknown>;
+  }> = [];
 
   constructor() {
     this.bundlePath = join(__dirname, '..', 'dist/cli.js');
@@ -504,13 +563,26 @@ export class TestRig {
   async cleanup() {
     // A session a test never closed keeps its CLI child forwarding PTY bytes
     // into this worker's stdout; after vitest tears the worker down those
-    // writes EPIPE and fail an otherwise all-green run (#10969).
-    for (const ptyProcess of this.interactiveProcesses.splice(0)) {
+    // writes EPIPE and fail an otherwise all-green run (#10969). Signalling
+    // alone still returns with the child alive and writing, so wait for it to
+    // actually go away.
+    for (const { ptyProcess, exited } of this.interactiveProcesses.splice(0)) {
       try {
         ptyProcess.kill();
       } catch {
         // Process may have already exited
       }
+      const ended = await sessionEndsWithin(
+        ptyProcess.pid,
+        exited,
+        INTERACTIVE_EXIT_GRACE_MS,
+      );
+      if (!ended) {
+        console.warn(
+          `interactive CLI process group ${ptyProcess.pid} did not end ` +
+            `within ${INTERACTIVE_EXIT_GRACE_MS}ms; continuing cleanup`,
+        );
+      }
     }
 
     // Clean up test directory
@@ -964,7 +1036,10 @@ export class TestRig {
         ...e2eRendererEnv(renderer),
       } as { [key: string]: string },
     });
-    this.interactiveProcesses.push(ptyProcess);
+    const exited = new Promise<void>((resolve) => {
+      ptyProcess.onExit(() => resolve());
+    });
+    this.interactiveProcesses.push({ ptyProcess, exited });
 
     ptyProcess.onData((data) => {
       this._interactiveOutput += data;