diff --git a/packages/computer/src/agent-tools.ts b/packages/computer/src/agent-tools.ts index d32099fd02..8917f57ee4 100644 --- a/packages/computer/src/agent-tools.ts +++ b/packages/computer/src/agent-tools.ts @@ -131,6 +131,11 @@ type ExtractedComputerInitArgs = Partial< AgentBehaviorInitArgs >; +export interface ComputerMidsceneToolsOptions { + /** Keep CLI-owned Xvfb alive until process exit so Xlib clients stay valid. */ + keepXvfbAliveUntilProcessExit?: boolean; +} + function adaptComputerInitArgs( extracted: ExtractedComputerInitArgs | undefined, ): ComputerInitArgs | undefined { @@ -186,6 +191,10 @@ export class ComputerMidsceneTools extends BaseMidsceneTools< > { private lastInitArgsSignature?: string; + constructor(private readonly options: ComputerMidsceneToolsOptions = {}) { + super(); + } + protected getCliReportSessionName() { return 'midscene-computer'; } @@ -246,6 +255,9 @@ export class ComputerMidsceneTools extends BaseMidsceneTools< ...(displayId ? { displayId } : {}), ...(headless !== undefined ? { headless } : {}), ...(keyboardTypeDelay !== undefined ? { keyboardTypeDelay } : {}), + ...(this.options.keepXvfbAliveUntilProcessExit + ? { keepXvfbAliveUntilProcessExit: true } + : {}), ...(extractAgentBehaviorInitArgs(opts) ?? {}), ...(reportOptions ?? {}), }; diff --git a/packages/computer/src/agent.ts b/packages/computer/src/agent.ts index 5d07ef91e5..fb90329be7 100644 --- a/packages/computer/src/agent.ts +++ b/packages/computer/src/agent.ts @@ -33,6 +33,7 @@ function createLocalComputerDevice( keyboardDriver: opts?.keyboardDriver, headless: opts?.headless, xvfbResolution: opts?.xvfbResolution, + keepXvfbAliveUntilProcessExit: opts?.keepXvfbAliveUntilProcessExit, }); } diff --git a/packages/computer/src/cli.ts b/packages/computer/src/cli.ts index ccd732b4cd..e696b45c5a 100644 --- a/packages/computer/src/cli.ts +++ b/packages/computer/src/cli.ts @@ -3,7 +3,9 @@ import { reportCLIError, runToolsCLI } from '@midscene/shared/cli'; import { ComputerMidsceneTools } from './agent-tools'; declare const __VERSION__: string; -const tools = new ComputerMidsceneTools(); +const tools = new ComputerMidsceneTools({ + keepXvfbAliveUntilProcessExit: true, +}); runToolsCLI(tools, 'midscene-computer', { stripPrefix: 'computer_', version: __VERSION__, diff --git a/packages/computer/src/device.ts b/packages/computer/src/device.ts index 60b916360c..bfca125aa5 100644 --- a/packages/computer/src/device.ts +++ b/packages/computer/src/device.ts @@ -29,8 +29,9 @@ import { import type { XvfbInstance } from './xvfb'; import { checkXvfbInstalled, - createXvfbSigintCleanup, + createXvfbSignalCleanup, needsXvfb, + scheduleXvfbStopAfterProcessExit, startXvfb, } from './xvfb'; @@ -732,6 +733,14 @@ export interface ComputerDeviceOpt extends ComputerDeviceInputOpt { * Resolution for Xvfb virtual display (default '1920x1080x24') */ xvfbResolution?: string; + /** + * Keep a managed Xvfb server alive until process exit. + * + * @internal The foreground CLI uses this because libnut keeps a process-wide + * X11 connection open. Stopping Xvfb during normal CLI teardown would make + * Xlib terminate an otherwise successful command with exit code 1. + */ + keepXvfbAliveUntilProcessExit?: boolean; } export class ComputerDevice implements AbstractInterface { @@ -743,7 +752,7 @@ export class ComputerDevice implements AbstractInterface { private destroyed = false; private xvfbInstance?: XvfbInstance; private xvfbCleanup?: () => void; - private xvfbSigintCleanup?: () => void; + private xvfbSignalCleanup?: () => void; private readonly inputDriver = new ComputerInputDriver({ getLibnut: () => libnut, useAppleScript: () => this.useAppleScript, @@ -971,22 +980,27 @@ export class ComputerDevice implements AbstractInterface { this.xvfbInstance = await startXvfb({ resolution: this.options?.xvfbResolution, }); + if (this.options?.keepXvfbAliveUntilProcessExit) { + scheduleXvfbStopAfterProcessExit(this.xvfbInstance); + } process.env.DISPLAY = this.xvfbInstance.display; debugDevice(`Xvfb started on display ${this.xvfbInstance.display}`); - // Clean up Xvfb on process exit (stored for removal in destroy()) - this.xvfbCleanup = () => { - if (this.xvfbInstance) { - this.xvfbInstance.stop(); - this.xvfbInstance = undefined; - } - }; - this.xvfbSigintCleanup = createXvfbSigintCleanup(() => - this.xvfbCleanup?.(), - ); - process.on('exit', this.xvfbCleanup); - process.on('SIGINT', this.xvfbSigintCleanup); - process.on('SIGTERM', this.xvfbCleanup); + if (!this.options?.keepXvfbAliveUntilProcessExit) { + // Clean up SDK-owned Xvfb during device teardown or process exit. + this.xvfbCleanup = () => { + if (this.xvfbInstance) { + this.xvfbInstance.stop(); + this.xvfbInstance = undefined; + } + }; + this.xvfbSignalCleanup = createXvfbSignalCleanup(() => + this.xvfbCleanup?.(), + ); + process.on('exit', this.xvfbCleanup); + process.on('SIGINT', this.xvfbSignalCleanup); + process.on('SIGTERM', this.xvfbSignalCleanup); + } } // Load libnut on first connect @@ -1013,17 +1027,19 @@ Available Displays: ${displays.length > 0 ? displays.map((d) => d.name).join(', } catch (error) { // Clean up Xvfb on connection failure if (this.xvfbInstance) { - this.xvfbInstance.stop(); + if (!this.options?.keepXvfbAliveUntilProcessExit) { + this.xvfbInstance.stop(); + } this.xvfbInstance = undefined; } if (this.xvfbCleanup) { process.removeListener('exit', this.xvfbCleanup); - process.removeListener('SIGTERM', this.xvfbCleanup); this.xvfbCleanup = undefined; } - if (this.xvfbSigintCleanup) { - process.removeListener('SIGINT', this.xvfbSigintCleanup); - this.xvfbSigintCleanup = undefined; + if (this.xvfbSignalCleanup) { + process.removeListener('SIGINT', this.xvfbSignalCleanup); + process.removeListener('SIGTERM', this.xvfbSignalCleanup); + this.xvfbSignalCleanup = undefined; } debugDevice(`Failed to connect: ${error}`); throw new Error(`Unable to connect to computer device: ${error}`); @@ -1591,18 +1607,22 @@ $g.Dispose(); $bmp.Dispose(); $ms.Dispose() this.destroyed = true; this.inputDriver.destroy(); + const keepXvfbAliveUntilProcessExit = + this.options?.keepXvfbAliveUntilProcessExit === true; if (this.xvfbInstance) { - this.xvfbInstance.stop(); + if (!keepXvfbAliveUntilProcessExit) { + this.xvfbInstance.stop(); + } this.xvfbInstance = undefined; } - if (this.xvfbCleanup) { + if (this.xvfbCleanup && !keepXvfbAliveUntilProcessExit) { process.removeListener('exit', this.xvfbCleanup); - process.removeListener('SIGTERM', this.xvfbCleanup); this.xvfbCleanup = undefined; } - if (this.xvfbSigintCleanup) { - process.removeListener('SIGINT', this.xvfbSigintCleanup); - this.xvfbSigintCleanup = undefined; + if (this.xvfbSignalCleanup) { + process.removeListener('SIGINT', this.xvfbSignalCleanup); + process.removeListener('SIGTERM', this.xvfbSignalCleanup); + this.xvfbSignalCleanup = undefined; } debugDevice('Computer device destroyed'); diff --git a/packages/computer/src/xvfb.ts b/packages/computer/src/xvfb.ts index f757bb0cca..6f392bd0c9 100644 --- a/packages/computer/src/xvfb.ts +++ b/packages/computer/src/xvfb.ts @@ -20,11 +20,66 @@ export interface XvfbInstance { stop(): void; } +const xvfbCleanupMonitorScript = String.raw` +const parentPid = Number(process.argv[1]); +const xvfbPid = Number(process.argv[2]); +const timer = setInterval(() => { + try { + process.kill(xvfbPid, 0); + } catch { + clearInterval(timer); + process.exit(0); + } + try { + process.kill(parentPid, 0); + return; + } catch { + // The owner is gone, so its X11 clients can no longer receive XIO errors. + } + try { + process.kill(xvfbPid, 'SIGTERM'); + } catch { + // Xvfb may have already exited. + } + clearInterval(timer); +}, 100); +`; + +/** + * Let a detached monitor stop Xvfb only after the owning process has exited. + * + * libnut keeps a process-wide X11 connection open and exposes no close API. + * Killing Xvfb from that same process makes Xlib call exit(1), even after a + * successful CLI command. The monitor runs outside the owner, waits until its + * X11 sockets have closed with process exit, and then stops the server. + */ +export function scheduleXvfbStopAfterProcessExit( + instance: XvfbInstance, + parentPid = process.pid, +): ChildProcess { + const xvfbPid = instance.process.pid; + if (!xvfbPid) { + throw new Error('Cannot schedule Xvfb cleanup before its process starts'); + } + + const monitor = spawn( + process.execPath, + ['-e', xvfbCleanupMonitorScript, String(parentPid), String(xvfbPid)], + { detached: true, stdio: 'ignore' }, + ); + monitor.on('error', (error) => { + debugXvfb(`Xvfb cleanup monitor failed: ${error.message}`); + }); + instance.process.unref(); + monitor.unref(); + return monitor; +} + /** - * Keep Xvfb alive while the foreground recorder handles SIGINT and saves its - * artifact. Other SIGINT listeners do not defer cleanup. + * Keep Xvfb alive while the foreground recorder handles a termination signal + * and saves its artifact. Other signal listeners do not defer cleanup. */ -export function createXvfbSigintCleanup( +export function createXvfbSignalCleanup( cleanup: () => void, source: CliInterruptSource = process, ): () => void { diff --git a/packages/computer/tests/unit-test/agent-tools.test.ts b/packages/computer/tests/unit-test/agent-tools.test.ts index 11a7724388..c77b7d7250 100644 --- a/packages/computer/tests/unit-test/agent-tools.test.ts +++ b/packages/computer/tests/unit-test/agent-tools.test.ts @@ -77,6 +77,26 @@ describe('ComputerMidsceneTools', () => { }); }); + it('keeps CLI-owned Xvfb alive until process exit when configured', async () => { + const tools = new ComputerMidsceneTools({ + keepXvfbAliveUntilProcessExit: true, + }); + await tools.initTools(); + + const takeScreenshotTool = tools + .getToolDefinitions() + .find((tool) => tool.name === 'take_screenshot'); + + await takeScreenshotTool?.handler({ + computer: { headless: true }, + }); + + expect(agentFromComputer).toHaveBeenCalledWith({ + headless: true, + keepXvfbAliveUntilProcessExit: true, + }); + }); + it('passes common agent behavior args to local agent creation', async () => { const tools = new ComputerMidsceneTools(); await tools.initTools(); diff --git a/packages/computer/tests/unit-test/device.test.ts b/packages/computer/tests/unit-test/device.test.ts index ad58ad5306..a50c0cf6dc 100644 --- a/packages/computer/tests/unit-test/device.test.ts +++ b/packages/computer/tests/unit-test/device.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it } from '@rstest/core'; +import { describe, expect, it, rs } from '@rstest/core'; import { ComputerDevice, checkComputerEnvironment } from '../../src'; const needsDisplay = process.platform === 'linux' && !process.env.DISPLAY; @@ -15,6 +15,34 @@ describe('ComputerDevice', () => { expect(device).toBeDefined(); }); + it('leaves CLI-owned Xvfb for the process exit cleanup', async () => { + const device = new ComputerDevice({ + keepXvfbAliveUntilProcessExit: true, + }); + const stop = rs.fn(); + const deviceInternals = device as unknown as { + xvfbInstance?: { stop(): void }; + }; + deviceInternals.xvfbInstance = { stop }; + + await device.destroy(); + + expect(stop).not.toHaveBeenCalled(); + }); + + it('stops API-owned Xvfb during normal device teardown', async () => { + const device = new ComputerDevice({}); + const stop = rs.fn(); + const deviceInternals = device as unknown as { + xvfbInstance?: { stop(): void }; + }; + deviceInternals.xvfbInstance = { stop }; + + await device.destroy(); + + expect(stop).toHaveBeenCalledOnce(); + }); + it.skipIf(needsDisplay)('should list displays', async () => { const displays = await ComputerDevice.listDisplays(); expect(Array.isArray(displays)).toBe(true); diff --git a/packages/computer/tests/unit-test/xvfb.test.ts b/packages/computer/tests/unit-test/xvfb.test.ts index 00c40f8173..61645a2e33 100644 --- a/packages/computer/tests/unit-test/xvfb.test.ts +++ b/packages/computer/tests/unit-test/xvfb.test.ts @@ -1,12 +1,17 @@ +import { spawn } from 'node:child_process'; import { EventEmitter } from 'node:events'; import { existsSync } from 'node:fs'; -import { waitForCliInterrupt } from '@midscene/shared/cli/interrupt'; +import { + createCliInterruptWaiter, + waitForCliInterrupt, +} from '@midscene/shared/cli/interrupt'; import { afterEach, describe, expect, it, rs } from '@rstest/core'; import { checkXvfbInstalled, - createXvfbSigintCleanup, + createXvfbSignalCleanup, findAvailableDisplay, needsXvfb, + scheduleXvfbStopAfterProcessExit, } from '../../src/xvfb'; rs.mock('node:fs', () => ({ @@ -88,12 +93,42 @@ describe('checkXvfbInstalled', () => { }); }); -describe('createXvfbSigintCleanup', () => { +describe('scheduleXvfbStopAfterProcessExit', () => { + it('unrefs Xvfb and a detached process-exit monitor', () => { + const xvfbUnref = rs.fn(); + const monitorUnref = rs.fn(); + const monitorOn = rs.fn(); + rs.mocked(spawn).mockReturnValueOnce({ + on: monitorOn, + unref: monitorUnref, + } as never); + + scheduleXvfbStopAfterProcessExit( + { + display: ':99', + process: { pid: 4321, unref: xvfbUnref } as never, + stop: rs.fn(), + }, + 1234, + ); + + expect(spawn).toHaveBeenCalledWith( + process.execPath, + ['-e', expect.any(String), '1234', '4321'], + { detached: true, stdio: 'ignore' }, + ); + expect(monitorOn).toHaveBeenCalledWith('error', expect.any(Function)); + expect(xvfbUnref).toHaveBeenCalledOnce(); + expect(monitorUnref).toHaveBeenCalledOnce(); + }); +}); + +describe('createXvfbSignalCleanup', () => { it('cleans up when the host only has unrelated SIGINT listeners', () => { const source = new EventEmitter(); const cleanup = rs.fn(); source.on('SIGINT', () => {}); - source.on('SIGINT', createXvfbSigintCleanup(cleanup, source)); + source.on('SIGINT', createXvfbSignalCleanup(cleanup, source)); source.emit('SIGINT'); @@ -103,7 +138,7 @@ describe('createXvfbSigintCleanup', () => { it('defers cleanup while a foreground recorder is handling SIGINT', async () => { const source = new EventEmitter(); const cleanup = rs.fn(); - source.on('SIGINT', createXvfbSigintCleanup(cleanup, source)); + source.on('SIGINT', createXvfbSignalCleanup(cleanup, source)); const stopped = waitForCliInterrupt(0, source); source.emit('SIGINT'); @@ -114,4 +149,22 @@ describe('createXvfbSigintCleanup', () => { source.emit('SIGINT'); expect(cleanup).toHaveBeenCalledOnce(); }); + + it('keeps Xvfb alive through a forwarded SIGTERM while saving', async () => { + const source = new EventEmitter(); + const cleanup = rs.fn(); + const signalCleanup = createXvfbSignalCleanup(cleanup, source); + source.on('SIGINT', signalCleanup); + source.on('SIGTERM', signalCleanup); + const waiter = createCliInterruptWaiter(0, { source }); + + source.emit('SIGINT'); + await expect(waiter.result).resolves.toBe('sigint'); + source.emit('SIGTERM'); + + expect(cleanup).not.toHaveBeenCalled(); + waiter.dispose(); + source.emit('SIGTERM'); + expect(cleanup).toHaveBeenCalledOnce(); + }); }); diff --git a/packages/shared/src/cli/interrupt.ts b/packages/shared/src/cli/interrupt.ts index 0caf76f393..0ade0aeed1 100644 --- a/packages/shared/src/cli/interrupt.ts +++ b/packages/shared/src/cli/interrupt.ts @@ -1,11 +1,72 @@ -export type CliInterruptReason = 'sigint' | 'watchdog'; +export type CliInterruptReason = 'sigint' | 'sigterm' | 'sighup' | 'watchdog'; + +type CliInterruptSignal = 'SIGINT' | 'SIGTERM' | 'SIGHUP'; export interface CliInterruptSource { - once(event: 'SIGINT', listener: () => void): unknown; - removeListener(event: 'SIGINT', listener: () => void): unknown; + on(event: CliInterruptSignal, listener: () => void): unknown; + removeListener(event: CliInterruptSignal, listener: () => void): unknown; +} + +export interface CliInterruptInputSource { + readonly isTTY?: boolean; + readonly isRaw?: boolean; + readonly readableFlowing?: boolean | null; + on(event: 'data', listener: (chunk: unknown) => void): unknown; + removeListener(event: 'data', listener: (chunk: unknown) => void): unknown; + setRawMode?(enabled: boolean): unknown; + pause?(): unknown; +} + +export interface CliInterruptWaiter { + /** Resolves on the first stop signal or watchdog timeout. */ + readonly result: Promise; + /** Release signal handlers after asynchronous finalization has completed. */ + dispose(): void; +} + +export interface CliInterruptWaiterOptions { + source?: CliInterruptSource; + input?: CliInterruptInputSource; + /** Called after restoring the terminal when Ctrl+C is pressed again. */ + forceExit?: (exitCode: number) => void; } const activeInterruptWaiters = new WeakMap(); +const noop = () => {}; +const sigintExitCode = 130; + +function guardTerminalCtrlC( + input: CliInterruptInputSource | undefined, + onInterrupt: () => void, +): () => void { + if (!input?.isTTY || !input.setRawMode) return noop; + + const wasRaw = input.isRaw === true; + const wasFlowing = input.readableFlowing === true; + const onData = (chunk: unknown) => { + const includesCtrlC = + (typeof chunk === 'string' && chunk.includes('\u0003')) || + (chunk instanceof Uint8Array && chunk.includes(3)); + if (includesCtrlC) onInterrupt(); + }; + const dispose = () => { + input.removeListener('data', onData); + try { + if (!wasFlowing) input.pause?.(); + } finally { + if (!wasRaw) input.setRawMode?.(false); + } + }; + + input.setRawMode(true); + try { + input.on('data', onData); + } catch (error) { + dispose(); + throw error; + } + return dispose; +} function registerCliInterruptWaiter(source: CliInterruptSource): () => void { const key = source as object; @@ -33,39 +94,114 @@ export function hasActiveCliInterruptWaiter( } /** - * Wait until the foreground CLI receives Ctrl+C. A positive watchdog keeps a - * forgotten recording from running forever and uses the same graceful save - * path as an explicit interrupt. + * Keep graceful-stop handlers installed until the caller has finished saving. + * + * Package runners such as pnpm can deliver SIGINT to the foreground child, + * immediately follow it with SIGTERM, then cause SIGHUP when the runner exits + * and its pseudo-terminal closes. Resolving on the first signal is not enough: + * removing any handler at that point lets a subsequent signal kill the child + * during asynchronous artifact finalization. + * + * On a TTY, Ctrl+C is captured as raw input so the package runner itself stays + * alive until the child has saved and restored the terminal. Signal handlers + * remain as the graceful-stop path for externally delivered termination. */ -export function waitForCliInterrupt( +export function createCliInterruptWaiter( watchdogMs: number, - source: CliInterruptSource = process, -): Promise { + options: CliInterruptWaiterOptions = {}, +): CliInterruptWaiter { + const source = options.source ?? process; + const input = + options.input ?? (source === process ? process.stdin : undefined); + const forceExit = + options.forceExit ?? + (source === process + ? (exitCode: number) => { + process.exit(exitCode); + } + : undefined); const unregisterWaiter = registerCliInterruptWaiter(source); + let timer: ReturnType | undefined; + let finished = false; + let disposed = false; + let disposeInput = noop; + let resolveResult!: (reason: CliInterruptReason) => void; + let rejectResult!: (error: unknown) => void; - return new Promise((resolve, reject) => { - let timer: ReturnType | undefined; - let finished = false; + const result = new Promise((resolve, reject) => { + resolveResult = resolve; + rejectResult = reject; + }); - const finish = (reason: CliInterruptReason) => { - if (finished) return; - finished = true; - source.removeListener('SIGINT', onSigint); - if (timer) clearTimeout(timer); - unregisterWaiter(); - resolve(reason); - }; - const onSigint = () => finish('sigint'); + const finish = (reason: CliInterruptReason) => { + if (finished) return; + finished = true; + if (timer) { + clearTimeout(timer); + timer = undefined; + } + resolveResult(reason); + }; + const onSigint = () => finish('sigint'); + const onSigterm = () => finish('sigterm'); + const onSighup = () => finish('sighup'); + function dispose() { + if (disposed) return; + disposed = true; + source.removeListener('SIGINT', onSigint); + source.removeListener('SIGTERM', onSigterm); + source.removeListener('SIGHUP', onSighup); try { - source.once('SIGINT', onSigint); - } catch (error) { + disposeInput(); + } finally { + if (timer) { + clearTimeout(timer); + timer = undefined; + } unregisterWaiter(); - reject(error); + } + } + + const onTerminalCtrlC = () => { + if (!finished) { + finish('sigint'); return; } + + // The first Ctrl+C protects asynchronous artifact finalization. A second + // explicit Ctrl+C is the user's escape hatch if device or file I/O hangs. + dispose(); + forceExit?.(sigintExitCode); + }; + + try { + source.on('SIGINT', onSigint); + source.on('SIGTERM', onSigterm); + source.on('SIGHUP', onSighup); + disposeInput = guardTerminalCtrlC(input, onTerminalCtrlC); if (watchdogMs > 0) { timer = setTimeout(() => finish('watchdog'), watchdogMs); } - }); + } catch (error) { + dispose(); + rejectResult(error); + } + + return { result, dispose }; +} + +/** + * Wait for one stop request and release the handlers immediately afterwards. + * Long-running finalizers should use {@link createCliInterruptWaiter} instead. + */ +export function waitForCliInterrupt( + watchdogMs: number, + source: CliInterruptSource = process, + input: CliInterruptInputSource | undefined = source === process + ? process.stdin + : undefined, +): Promise { + const waiter = createCliInterruptWaiter(watchdogMs, { source, input }); + return waiter.result.finally(waiter.dispose); } diff --git a/packages/shared/src/cli/record-command.ts b/packages/shared/src/cli/record-command.ts index 978b5026b3..18ceb0985b 100644 --- a/packages/shared/src/cli/record-command.ts +++ b/packages/shared/src/cli/record-command.ts @@ -9,7 +9,7 @@ import type { ToolResult, ToolSchema, } from '../agent-tools/types'; -import { waitForCliInterrupt } from './interrupt'; +import { type CliInterruptWaiter, createCliInterruptWaiter } from './interrupt'; import { attachCliVerboseDumpListener, emitCliVerboseEvent } from './verbose'; const recordCliMetadata: ToolCliMetadata = { @@ -62,7 +62,7 @@ export function createRecordCliCommand( return { name: 'record', description: - 'Record the page/screen in the foreground until Ctrl+C, then save the ordered frame window for a later assert command.', + 'Record the page/screen in the foreground until Ctrl+C or a termination signal, then save the ordered frame window for a later assert command.', schema: { action: z .literal('start') @@ -125,6 +125,7 @@ export function createRecordCliCommand( let observer: | Awaited>> | undefined; + let interruptWaiter: CliInterruptWaiter | undefined; try { const watchdogMs = (args.watchdogMs as number | undefined) ?? 300_000; observer = await agent.startObserving({ @@ -132,12 +133,13 @@ export function createRecordCliCommand( maxFrames: args.maxFrames as number | undefined, watchdogMs, }); + interruptWaiter = createCliInterruptWaiter(watchdogMs); emitCliVerboseEvent({ event: 'recording_ready', tool: 'record', watchdogMs, }); - const stopReason = await waitForCliInterrupt(watchdogMs); + const stopReason = await interruptWaiter.result; emitCliVerboseEvent({ event: 'recording_stopping', tool: 'record', @@ -158,8 +160,12 @@ export function createRecordCliCommand( ], }; } finally { - await observer?.dispose?.(); - unsubscribeVerbose(); + try { + await observer?.dispose?.(); + } finally { + interruptWaiter?.dispose(); + unsubscribeVerbose(); + } } } catch (error: unknown) { const errorMessage = getErrorMessage(error); diff --git a/packages/shared/src/cli/verbose.ts b/packages/shared/src/cli/verbose.ts index e0dd874e93..67954e8da3 100644 --- a/packages/shared/src/cli/verbose.ts +++ b/packages/shared/src/cli/verbose.ts @@ -587,9 +587,15 @@ function renderCliVerboseEventText( case 'recording_ready': return '[Midscene] Recording. Press Ctrl+C to stop and save.'; case 'recording_stopping': - return event.reason === 'watchdog' - ? '[Midscene] Recording watchdog reached; finalizing and saving.' - : '[Midscene] Ctrl+C received; finalizing and saving.'; + if (event.reason === 'watchdog') { + return '[Midscene] Recording watchdog reached; finalizing and saving.'; + } + if (event.reason === 'sigterm') { + return '[Midscene] SIGTERM received; finalizing and saving.'; + } + return event.reason === 'sighup' + ? '[Midscene] SIGHUP received; finalizing and saving.' + : '[Midscene] Ctrl+C received; finalizing and saving. Press Ctrl+C again to force exit.'; case 'dump_update': { if (isActVerboseEvent(command, tool)) { return undefined; diff --git a/packages/shared/tests/unit-test/cli-interrupt.test.ts b/packages/shared/tests/unit-test/cli-interrupt.test.ts index 743300763a..42051fcdc3 100644 --- a/packages/shared/tests/unit-test/cli-interrupt.test.ts +++ b/packages/shared/tests/unit-test/cli-interrupt.test.ts @@ -1,5 +1,6 @@ import { EventEmitter } from 'node:events'; import { + createCliInterruptWaiter, hasActiveCliInterruptWaiter, waitForCliInterrupt, } from '@/cli/interrupt'; @@ -35,6 +36,146 @@ describe('waitForCliInterrupt', () => { expect(source.listenerCount('SIGINT')).toBe(0); }); + it('keeps termination signals guarded until asynchronous saving finishes', async () => { + const source = new EventEmitter(); + const waiter = createCliInterruptWaiter(0, { source }); + + source.emit('SIGINT'); + await expect(waiter.result).resolves.toBe('sigint'); + + expect(hasActiveCliInterruptWaiter(source)).toBe(true); + expect(source.listenerCount('SIGINT')).toBe(1); + expect(source.listenerCount('SIGTERM')).toBe(1); + expect(source.listenerCount('SIGHUP')).toBe(1); + + // Package runners may forward SIGTERM after the terminal's SIGINT and + // trigger SIGHUP when the parent exits. Both must be absorbed while the + // recorder is still saving. + source.emit('SIGTERM'); + source.emit('SIGHUP'); + expect(hasActiveCliInterruptWaiter(source)).toBe(true); + + waiter.dispose(); + expect(hasActiveCliInterruptWaiter(source)).toBe(false); + expect(source.listenerCount('SIGINT')).toBe(0); + expect(source.listenerCount('SIGTERM')).toBe(0); + expect(source.listenerCount('SIGHUP')).toBe(0); + }); + + it('gracefully stops on SIGTERM', async () => { + const source = new EventEmitter(); + const stopped = waitForCliInterrupt(0, source); + + source.emit('SIGTERM'); + + await expect(stopped).resolves.toBe('sigterm'); + expect(hasActiveCliInterruptWaiter(source)).toBe(false); + expect(source.listenerCount('SIGINT')).toBe(0); + expect(source.listenerCount('SIGTERM')).toBe(0); + expect(source.listenerCount('SIGHUP')).toBe(0); + }); + + it('gracefully stops on SIGHUP', async () => { + const source = new EventEmitter(); + const stopped = waitForCliInterrupt(0, source); + + source.emit('SIGHUP'); + + await expect(stopped).resolves.toBe('sighup'); + expect(hasActiveCliInterruptWaiter(source)).toBe(false); + expect(source.listenerCount('SIGINT')).toBe(0); + expect(source.listenerCount('SIGTERM')).toBe(0); + expect(source.listenerCount('SIGHUP')).toBe(0); + }); + + it('captures terminal Ctrl+C as input until saving finishes', async () => { + const source = new EventEmitter(); + const input = Object.assign(new EventEmitter(), { + isTTY: true, + isRaw: false, + readableFlowing: null as boolean | null, + setRawMode(enabled: boolean) { + this.isRaw = enabled; + }, + pause() { + this.readableFlowing = false; + }, + }); + const waiter = createCliInterruptWaiter(0, { source, input }); + + expect(input.isRaw).toBe(true); + input.emit('data', Buffer.from([3])); + + await expect(waiter.result).resolves.toBe('sigint'); + expect(input.isRaw).toBe(true); + expect(input.listenerCount('data')).toBe(1); + + waiter.dispose(); + expect(input.isRaw).toBe(false); + expect(input.readableFlowing).toBe(false); + expect(input.listenerCount('data')).toBe(0); + }); + + it('releases the terminal guard on a second Ctrl+C during finalization', async () => { + const source = new EventEmitter(); + const forceExit = rs.fn(); + const input = Object.assign(new EventEmitter(), { + isTTY: true, + isRaw: false, + readableFlowing: null as boolean | null, + setRawMode(enabled: boolean) { + this.isRaw = enabled; + }, + pause() { + this.readableFlowing = false; + }, + }); + const waiter = createCliInterruptWaiter(0, { + source, + input, + forceExit, + }); + + input.emit('data', Buffer.from([3])); + await expect(waiter.result).resolves.toBe('sigint'); + + input.emit('data', Buffer.from([3])); + + expect(hasActiveCliInterruptWaiter(source)).toBe(false); + expect(input.isRaw).toBe(false); + expect(input.readableFlowing).toBe(false); + expect(input.listenerCount('data')).toBe(0); + expect(forceExit).toHaveBeenCalledWith(130); + }); + + it('restores the terminal when input listener setup fails', async () => { + const source = new EventEmitter(); + const input = { + isTTY: true, + isRaw: false, + readableFlowing: null as boolean | null, + on() { + throw new Error('input setup failed'); + }, + removeListener: rs.fn(), + setRawMode(enabled: boolean) { + this.isRaw = enabled; + }, + pause() { + this.readableFlowing = false; + }, + }; + const waiter = createCliInterruptWaiter(0, { source, input }); + + await expect(waiter.result).rejects.toThrow('input setup failed'); + expect(input.isRaw).toBe(false); + expect(input.readableFlowing).toBe(false); + expect(hasActiveCliInterruptWaiter(source)).toBe(false); + expect(source.listenerCount('SIGINT')).toBe(0); + expect(source.listenerCount('SIGTERM')).toBe(0); + expect(source.listenerCount('SIGHUP')).toBe(0); + }); + it('does not treat unrelated SIGINT listeners as CLI waiters', () => { const source = new EventEmitter(); source.on('SIGINT', () => {}); diff --git a/packages/shared/tests/unit-test/tool-generator.test.ts b/packages/shared/tests/unit-test/tool-generator.test.ts index 7aebfab0d9..cd1ec3ca0b 100644 --- a/packages/shared/tests/unit-test/tool-generator.test.ts +++ b/packages/shared/tests/unit-test/tool-generator.test.ts @@ -430,9 +430,13 @@ describe('generateToolsFromActionSpace', () => { ), ); const recordTool = createRecordCliCommand(getAgent); + const interruptDispose = rs.fn(); const interruptSpy = rs - .spyOn(cliInterrupt, 'waitForCliInterrupt') - .mockResolvedValue('sigint'); + .spyOn(cliInterrupt, 'createCliInterruptWaiter') + .mockReturnValue({ + result: Promise.resolve('sigint'), + dispose: interruptDispose, + }); const result = await withCliVerboseContext( { @@ -460,9 +464,14 @@ describe('generateToolsFromActionSpace', () => { expect(stop).toHaveBeenCalledOnce(); expect(exportRecord).toHaveBeenCalledOnce(); expect(dispose).toHaveBeenCalledOnce(); + expect(interruptDispose).toHaveBeenCalledOnce(); + expect(existsSync(output)).toBe(true); expect(startObserving.mock.invocationCallOrder[0]).toBeLessThan( stop.mock.invocationCallOrder[0], ); + expect(dispose.mock.invocationCallOrder[0]).toBeLessThan( + interruptDispose.mock.invocationCallOrder[0], + ); expect(result).toEqual({ content: [{ type: 'text', text: `Observation record saved: ${output}` }], });