From 161c784514d843a9060895da2e25693807f7d7f6 Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Fri, 4 Sep 2026 04:40:19 +0000 Subject: [PATCH] fix(test): wait for interactive PTY sessions to end during cleanup (#10990) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cleanup signalled each leaked session but returned without waiting for it to go away. The CLI traps SIGHUP and exits only once runExitCleanup() has drained, a chain it bounds at 5s, so kill() returns with the child still alive and still forwarding PTY bytes into the worker's stdout — measured at 83ms for a booted session, exiting with the CLI's SIGHUP code 129. That is the window #10969 was meant to close. A full interactive leg run on the parent commit shows a CLI child reparented to init at the moment its vitest worker exited; the same run after this change orphans none, with an identical result set. The wait costs each session's real drain (35-42ms measured) and is bounded above the CLI's own 5s ceiling. The witness now pins the wait itself. Its stand-in traps SIGHUP and exits after a delay like the real CLI, and reports itself booted first: signalling a child that has not installed its handler ends it on the default action, which measured nothing. Deleting the wait turns it red at 0ms against a 750ms floor; deleting the kill turns it red on the survival poll. --- integration-tests/test-helper.test.ts | 29 ++++++++++++++++++--- integration-tests/test-helper.ts | 37 ++++++++++++++++++++++++--- 2 files changed, 58 insertions(+), 8 deletions(-) diff --git a/integration-tests/test-helper.test.ts b/integration-tests/test-helper.test.ts index 604e6886acd..6731cd0dfb6 100644 --- a/integration-tests/test-helper.test.ts +++ b/integration-tests/test-helper.test.ts @@ -17,6 +17,12 @@ 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; + describe('TestRig', () => { const originalKeepOutput = process.env['KEEP_OUTPUT']; @@ -63,7 +69,7 @@ describe('TestRig', () => { expect(existsSync(testDir)).toBe(true); }); - it('kills an interactive session a test never closed during cleanup', async () => { + it('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'; @@ -72,15 +78,30 @@ describe('TestRig', () => { // 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', + '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); + // 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); + const cleanupStartedAt = Date.now(); await rig.cleanup(); - + const cleanupTookMs = Date.now() - cleanupStartedAt; + + // 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); await expect .poll(() => isProcessAlive(ptyProcess.pid), { message: 'the interactive CLI child outlived cleanup()', diff --git a/integration-tests/test-helper.ts b/integration-tests/test-helper.ts index 48ecd938cf9..7826826b98f 100644 --- a/integration-tests/test-helper.ts +++ b/integration-tests/test-helper.ts @@ -124,6 +124,26 @@ 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). Waiting longer +// than that bound is what makes cleanup() return with the child actually gone. +const INTERACTIVE_EXIT_GRACE_MS = 10_000; + +// Resolves when `promise` settles, or after `ms` if it never does. The timer +// is cleared and unrefed so a won race leaves no handle holding the worker's +// event loop open. +function settleWithin(promise: Promise, ms: number): Promise { + return new Promise((resolve) => { + const timer = setTimeout(resolve, ms); + timer.unref(); + const settle = () => { + clearTimeout(timer); + resolve(); + }; + void promise.then(settle, settle); + }); +} + // 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; @@ -200,7 +220,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'); @@ -496,13 +519,16 @@ 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 (#10990). + for (const { ptyProcess, exited } of this.interactiveProcesses.splice(0)) { try { ptyProcess.kill(); } catch { // Process may have already exited } + await settleWithin(exited, INTERACTIVE_EXIT_GRACE_MS); } // Clean up test directory @@ -944,7 +970,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;