diff --git a/packages/cli/src/acp-integration/acpAgent.ts b/packages/cli/src/acp-integration/acpAgent.ts index 246d80019..0cbace59e 100644 --- a/packages/cli/src/acp-integration/acpAgent.ts +++ b/packages/cli/src/acp-integration/acpAgent.ts @@ -87,7 +87,33 @@ export async function runAcpAgent( stream, ); + // Handle SIGTERM/SIGINT for graceful shutdown. + // Without this, signal handlers registered elsewhere in the CLI + // (e.g., stdin raw mode restoration) override the default exit behavior, + // causing the ACP process to ignore termination signals. + let shuttingDown = false; + const shutdownHandler = () => { + if (shuttingDown) return; + shuttingDown = true; + debugLogger.debug('[ACP] Shutdown signal received, closing streams'); + try { + process.stdin.destroy(); + } catch { + // stdin may already be closed + } + try { + process.stdout.destroy(); + } catch { + // stdout may already be closed + } + }; + process.on('SIGTERM', shutdownHandler); + process.on('SIGINT', shutdownHandler); + await connection.closed; + + process.off('SIGTERM', shutdownHandler); + process.off('SIGINT', shutdownHandler); } function toStdioServer(server: McpServer): McpServerStdio | undefined { diff --git a/packages/vscode-ide-companion/src/services/acpConnection.test.ts b/packages/vscode-ide-companion/src/services/acpConnection.test.ts index 32977171a..881d40d01 100644 --- a/packages/vscode-ide-companion/src/services/acpConnection.test.ts +++ b/packages/vscode-ide-companion/src/services/acpConnection.test.ts @@ -14,11 +14,36 @@ vi.mock('vscode', () => ({})); import { AcpConnection } from './acpConnection.js'; import { ACP_ERROR_CODES } from '../constants/acpSchema.js'; +type AcpConnectionInternal = { + child: { killed: boolean; exitCode: number | null; kill?: () => void } | null; + sdkConnection: unknown; + sessionId: string | null; + lastExitCode: number | null; + lastExitSignal: string | null; + mapReadTextFileError: (error: unknown, filePath: string) => unknown; + ensureConnection: () => unknown; +}; + +function createConnection(overrides?: Partial) { + const conn = new AcpConnection() as unknown as AcpConnectionInternal; + if (overrides) { + Object.assign(conn, overrides); + } + return conn; +} + +function createMockChild(overrides?: Record) { + return { + killed: false, + exitCode: null, + kill: vi.fn(), + ...overrides, + } as unknown as AcpConnectionInternal['child']; +} + describe('AcpConnection readTextFile error mapping', () => { it('maps ENOENT to RESOURCE_NOT_FOUND RequestError', () => { - const conn = new AcpConnection() as unknown as { - mapReadTextFileError: (error: unknown, filePath: string) => unknown; - }; + const conn = createConnection(); const enoent = Object.assign(new Error('missing file'), { code: 'ENOENT' }); expect(() => @@ -31,9 +56,7 @@ describe('AcpConnection readTextFile error mapping', () => { }); it('keeps non-ENOENT RequestError unchanged', () => { - const conn = new AcpConnection() as unknown as { - mapReadTextFileError: (error: unknown, filePath: string) => unknown; - }; + const conn = createConnection(); const requestError = new RequestError( ACP_ERROR_CODES.INTERNAL_ERROR, 'Internal error', @@ -44,3 +67,121 @@ describe('AcpConnection readTextFile error mapping', () => { ); }); }); + +describe('AcpConnection.isConnected', () => { + it('returns true when child is alive', () => { + const conn = createConnection({ + child: { killed: false, exitCode: null }, + }); + expect((conn as unknown as AcpConnection).isConnected).toBe(true); + }); + + it('returns false when child is null', () => { + const conn = createConnection({ child: null }); + expect((conn as unknown as AcpConnection).isConnected).toBe(false); + }); + + it('returns false when child was killed', () => { + const conn = createConnection({ + child: { killed: true, exitCode: null }, + }); + expect((conn as unknown as AcpConnection).isConnected).toBe(false); + }); + + it('returns false when child exited on its own (exitCode set)', () => { + // 143 = 128 + 15 (SIGTERM) + const conn = createConnection({ + child: { killed: false, exitCode: 143 }, + }); + expect((conn as unknown as AcpConnection).isConnected).toBe(false); + }); +}); + +describe('AcpConnection.ensureConnection', () => { + it('throws when sdkConnection is null', () => { + const conn = createConnection({ + sdkConnection: null, + child: { killed: false, exitCode: null }, + }); + expect(() => conn.ensureConnection()).toThrow('Not connected to ACP agent'); + }); + + it('throws when process has exited (exitCode set)', () => { + const conn = createConnection({ + sdkConnection: {}, + child: { killed: false, exitCode: 1 }, + }); + expect(() => conn.ensureConnection()).toThrow('Not connected to ACP agent'); + }); + + it('throws when child is null (process exited and cleaned up)', () => { + const conn = createConnection({ + sdkConnection: {}, + child: null, + }); + expect(() => conn.ensureConnection()).toThrow('Not connected to ACP agent'); + }); + + it('returns sdkConnection when process is alive', () => { + const fakeSdk = { send: vi.fn() }; + const conn = createConnection({ + sdkConnection: fakeSdk, + child: { killed: false, exitCode: null }, + }); + expect(conn.ensureConnection()).toBe(fakeSdk); + }); +}); + +describe('AcpConnection child exit cleanup', () => { + it('disconnect clears child, sdkConnection, and sessionId', () => { + const conn = createConnection({ + child: createMockChild(), + sdkConnection: {}, + sessionId: 'test-session', + }); + + const acpConn = conn as unknown as AcpConnection; + acpConn.disconnect(); + + expect(acpConn.isConnected).toBe(false); + expect(acpConn.hasActiveSession).toBe(false); + expect(acpConn.currentSessionId).toBeNull(); + }); + + it('disconnect calls kill on the child process', () => { + const mockKill = vi.fn(); + const conn = createConnection({ + child: createMockChild({ kill: mockKill }), + sdkConnection: {}, + sessionId: 'test-session', + }); + + (conn as unknown as AcpConnection).disconnect(); + expect(mockKill).toHaveBeenCalledOnce(); + }); +}); + +describe('AcpConnection onDisconnected callback', () => { + it('has a default no-op onDisconnected handler', () => { + const acpConn = new AcpConnection(); + expect(acpConn.onDisconnected).toBeTypeOf('function'); + expect(() => acpConn.onDisconnected(143, 'SIGTERM')).not.toThrow(); + }); + + it('allows setting a custom onDisconnected handler', () => { + const acpConn = new AcpConnection(); + const spy = vi.fn(); + acpConn.onDisconnected = spy; + + acpConn.onDisconnected(1, null); + expect(spy).toHaveBeenCalledWith(1, null); + }); +}); + +describe('AcpConnection lastExitCode/lastExitSignal', () => { + it('initializes exit info as null', () => { + const conn = createConnection(); + expect(conn.lastExitCode).toBeNull(); + expect(conn.lastExitSignal).toBeNull(); + }); +}); diff --git a/packages/vscode-ide-companion/src/services/acpConnection.ts b/packages/vscode-ide-companion/src/services/acpConnection.ts index 5d263b618..736c8a73a 100644 --- a/packages/vscode-ide-companion/src/services/acpConnection.ts +++ b/packages/vscode-ide-companion/src/services/acpConnection.ts @@ -52,6 +52,8 @@ export class AcpConnection { private sessionId: string | null = null; private workingDir: string = process.cwd(); private fileHandler = new AcpFileHandler(); + private lastExitCode: number | null = null; + private lastExitSignal: string | null = null; onSessionUpdate: (data: SessionNotification) => void = () => {}; onPermissionRequest: (data: RequestPermissionRequest) => Promise<{ @@ -63,6 +65,9 @@ export class AcpConnection { onAuthenticateUpdate: (data: AuthenticateUpdateNotification) => void = () => {}; onEndTurn: (reason?: string) => void = () => {}; + /** Invoked when the child process exits (expected or unexpected). */ + onDisconnected: (code: number | null, signal: string | null) => void = + () => {}; onAskUserQuestion: (data: AskUserQuestionRequest) => Promise<{ optionId: string; answers?: Record; @@ -78,6 +83,8 @@ export class AcpConnection { this.disconnect(); } + this.lastExitCode = null; + this.lastExitSignal = null; this.workingDir = workingDir; const env = { ...process.env }; @@ -145,6 +152,14 @@ export class AcpConnection { console.error( `[ACP qwen] Process exited with code: ${code}, signal: ${signal}`, ); + this.lastExitCode = code; + this.lastExitSignal = signal; + if (this.child) { + this.sdkConnection = null; + this.sessionId = null; + this.child = null; + this.onDisconnected(code, signal); + } }); await new Promise((resolve) => setTimeout(resolve, 1000)); @@ -154,7 +169,11 @@ export class AcpConnection { } if (!this.child || this.child.killed) { - throw new Error(`Qwen ACP process failed to start`); + const code = this.lastExitCode ?? this.child?.exitCode ?? null; + const signal = this.lastExitSignal; + throw new Error( + `Qwen ACP process failed to start (exit code: ${code}, signal: ${signal})`, + ); } // Convert Node.js child process streams to Web Streams for SDK @@ -334,7 +353,9 @@ export class AcpConnection { } private ensureConnection(): ClientSideConnection { - if (!this.sdkConnection) { + // sdkConnection is cleared asynchronously by the exit handler; + // isConnected (via exitCode) catches the race window before the exit event fires. + if (!this.sdkConnection || !this.isConnected) { throw new Error('Not connected to ACP agent'); } return this.sdkConnection; @@ -541,7 +562,9 @@ export class AcpConnection { } get isConnected(): boolean { - return this.child !== null && !this.child.killed; + return ( + this.child !== null && !this.child.killed && this.child.exitCode === null + ); } get hasActiveSession(): boolean {