diff --git a/integration-tests/test-helper.test.ts b/integration-tests/test-helper.test.ts index 8b6cff3a220..927423407b4 100644 --- a/integration-tests/test-helper.test.ts +++ b/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([ [ diff --git a/integration-tests/test-helper.ts b/integration-tests/test-helper.ts index 05e17029dc7..66ab754f16d 100644 --- a/integration-tests/test-helper.ts +++ b/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, + ms: number, +): Promise { + 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; + }> = []; constructor() { this.bundlePath = join(__dirname, '..', 'dist/cli.js'); @@ -512,13 +571,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 @@ -988,7 +1060,10 @@ export class TestRig { ...e2eRendererEnv(renderer), } as { [key: string]: string }, }); - this.interactiveProcesses.push(ptyProcess); + const exited = new Promise((resolve) => { + ptyProcess.onExit(() => resolve()); + }); + this.interactiveProcesses.push({ ptyProcess, exited }); ptyProcess.onData((data) => { this._interactiveOutput += data;