diff --git a/packages/cli/src/acp-integration/acpAgent.test.ts b/packages/cli/src/acp-integration/acpAgent.test.ts new file mode 100644 index 000000000..07473e97d --- /dev/null +++ b/packages/cli/src/acp-integration/acpAgent.test.ts @@ -0,0 +1,207 @@ +/** + * @license + * Copyright 2025 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; + +// Mock cleanup module before importing anything else +const { mockRunExitCleanup } = vi.hoisted(() => ({ + mockRunExitCleanup: vi.fn().mockResolvedValue(undefined), +})); +vi.mock('../utils/cleanup.js', () => ({ + runExitCleanup: mockRunExitCleanup, +})); + +// Mock the ACP SDK +const { mockConnectionState } = vi.hoisted(() => { + const state = { + resolve: () => {}, + promise: null as unknown as Promise, + reset() { + state.promise = new Promise((r) => { + state.resolve = r; + }); + }, + }; + state.reset(); + return { mockConnectionState: state }; +}); + +vi.mock('@agentclientprotocol/sdk', () => ({ + AgentSideConnection: vi.fn().mockImplementation(() => ({ + get closed() { + return mockConnectionState.promise; + }, + })), + ndJsonStream: vi.fn().mockReturnValue({}), + RequestError: class RequestError extends Error {}, + PROTOCOL_VERSION: '1.0.0', +})); + +// Mock stream conversion +vi.mock('node:stream', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + Writable: { ...actual.Writable, toWeb: vi.fn().mockReturnValue({}) }, + Readable: { ...actual.Readable, toWeb: vi.fn().mockReturnValue({}) }, + }; +}); + +// Mock core dependencies +vi.mock('@qwen-code/qwen-code-core', () => ({ + createDebugLogger: () => ({ + debug: vi.fn(), + error: vi.fn(), + warn: vi.fn(), + }), + APPROVAL_MODE_INFO: {}, + APPROVAL_MODES: [], + AuthType: {}, + clearCachedCredentialFile: vi.fn(), + QwenOAuth2Event: {}, + qwenOAuth2Events: { on: vi.fn(), off: vi.fn() }, + MCPServerConfig: {}, + SessionService: vi.fn(), + tokenLimit: vi.fn(), +})); + +vi.mock('./authMethods.js', () => ({ buildAuthMethods: vi.fn() })); +vi.mock('./service/filesystem.js', () => ({ + AcpFileSystemService: vi.fn(), +})); +vi.mock('../config/settings.js', () => ({ SettingScope: {} })); +vi.mock('../config/config.js', () => ({ loadCliConfig: vi.fn() })); +vi.mock('./session/Session.js', () => ({ Session: vi.fn() })); +vi.mock('../utils/acpModelUtils.js', () => ({ + formatAcpModelId: vi.fn(), +})); + +import { runAcpAgent } from './acpAgent.js'; +import type { Config } from '@qwen-code/qwen-code-core'; +import type { LoadedSettings } from '../config/settings.js'; +import type { CliArgs } from '../config/config.js'; + +describe('runAcpAgent shutdown cleanup', () => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + let processExitSpy: any; + let sigTermListeners: NodeJS.SignalsListener[]; + let sigIntListeners: NodeJS.SignalsListener[]; + + const mockConfig = {} as Config; + const mockSettings = { merged: {} } as LoadedSettings; + const mockArgv = {} as CliArgs; + + beforeEach(() => { + vi.clearAllMocks(); + mockRunExitCleanup.mockResolvedValue(undefined); + mockConnectionState.reset(); + sigTermListeners = []; + sigIntListeners = []; + + // Intercept signal handler registration + vi.spyOn(process, 'on').mockImplementation((( + event: string, + listener: (...args: unknown[]) => void, + ) => { + if (event === 'SIGTERM') + sigTermListeners.push(listener as NodeJS.SignalsListener); + if (event === 'SIGINT') + sigIntListeners.push(listener as NodeJS.SignalsListener); + return process; + }) as typeof process.on); + + vi.spyOn(process, 'off').mockImplementation( + (() => process) as typeof process.off, + ); + + // Mock process.exit to prevent actually exiting + processExitSpy = vi + .spyOn(process, 'exit') + .mockImplementation((() => undefined) as unknown as typeof process.exit); + + // Mock stdin/stdout destroy + vi.spyOn(process.stdin, 'destroy').mockImplementation(() => process.stdin); + vi.spyOn(process.stdout, 'destroy').mockImplementation( + () => process.stdout, + ); + }); + + afterEach(() => { + processExitSpy.mockRestore(); + vi.restoreAllMocks(); + }); + + it('calls runExitCleanup and process.exit on SIGTERM', async () => { + // Start runAcpAgent (it will await connection.closed) + const agentPromise = runAcpAgent(mockConfig, mockSettings, mockArgv); + + // Simulate SIGTERM from IDE + expect(sigTermListeners.length).toBeGreaterThan(0); + sigTermListeners[0]('SIGTERM'); + + // runExitCleanup is async, wait for it + await vi.waitFor(() => { + expect(mockRunExitCleanup).toHaveBeenCalledTimes(1); + }); + + await vi.waitFor(() => { + expect(processExitSpy).toHaveBeenCalledWith(0); + }); + + // Resolve connection.closed so the promise settles + mockConnectionState.resolve(); + await agentPromise; + }); + + it('calls runExitCleanup and process.exit on SIGINT', async () => { + const agentPromise = runAcpAgent(mockConfig, mockSettings, mockArgv); + + expect(sigIntListeners.length).toBeGreaterThan(0); + sigIntListeners[0]('SIGINT'); + + await vi.waitFor(() => { + expect(mockRunExitCleanup).toHaveBeenCalledTimes(1); + }); + + await vi.waitFor(() => { + expect(processExitSpy).toHaveBeenCalledWith(0); + }); + + mockConnectionState.resolve(); + await agentPromise; + }); + + it('only runs shutdown once even if multiple signals arrive', async () => { + const agentPromise = runAcpAgent(mockConfig, mockSettings, mockArgv); + + // Send SIGTERM twice + sigTermListeners[0]('SIGTERM'); + sigTermListeners[0]('SIGTERM'); + + await vi.waitFor(() => { + expect(mockRunExitCleanup).toHaveBeenCalledTimes(1); + }); + + mockConnectionState.resolve(); + await agentPromise; + }); + + it('still exits even if runExitCleanup throws', async () => { + mockRunExitCleanup.mockRejectedValueOnce(new Error('cleanup failed')); + + const agentPromise = runAcpAgent(mockConfig, mockSettings, mockArgv); + + sigTermListeners[0]('SIGTERM'); + + // process.exit should still be called via .finally() + await vi.waitFor(() => { + expect(processExitSpy).toHaveBeenCalledWith(0); + }); + + mockConnectionState.resolve(); + await agentPromise; + }); +}); diff --git a/packages/cli/src/acp-integration/acpAgent.ts b/packages/cli/src/acp-integration/acpAgent.ts index b78a8a0d6..fc1dceef1 100644 --- a/packages/cli/src/acp-integration/acpAgent.ts +++ b/packages/cli/src/acp-integration/acpAgent.ts @@ -65,6 +65,7 @@ import { loadCliConfig } from '../config/config.js'; import { Session } from './session/Session.js'; import { formatAcpModelId } from '../utils/acpModelUtils.js'; import { runWithAcpRuntimeOutputDir } from './runtimeOutputDirContext.js'; +import { runExitCleanup } from '../utils/cleanup.js'; const debugLogger = createDebugLogger('ACP_AGENT'); @@ -107,6 +108,16 @@ export async function runAcpAgent( } catch { // stdout may already be closed } + // Clean up child processes (MCP servers, etc.) and force exit. + // Without this, orphan subprocesses keep the Node.js event loop alive + // and the CLI process never terminates after the IDE disconnects. + runExitCleanup() + .catch((err) => { + debugLogger.error('[ACP] Cleanup error:', err); + }) + .finally(() => { + process.exit(0); + }); }; process.on('SIGTERM', shutdownHandler); process.on('SIGINT', shutdownHandler); diff --git a/packages/cli/src/gemini.tsx b/packages/cli/src/gemini.tsx index aebb67993..897310729 100644 --- a/packages/cli/src/gemini.tsx +++ b/packages/cli/src/gemini.tsx @@ -407,7 +407,10 @@ export async function main() { const initializationResult = await initializeApp(config, settings); if (config.getExperimentalZedIntegration()) { - return runAcpAgent(config, settings, argv); + await runAcpAgent(config, settings, argv); + // Clean up child processes and force exit, matching other non-interactive modes + await runExitCleanup(); + process.exit(0); } let input = config.getQuestion(); diff --git a/packages/vscode-ide-companion/src/webview/providers/WebViewProvider.ts b/packages/vscode-ide-companion/src/webview/providers/WebViewProvider.ts index 182025917..b1cebfe4c 100644 --- a/packages/vscode-ide-companion/src/webview/providers/WebViewProvider.ts +++ b/packages/vscode-ide-companion/src/webview/providers/WebViewProvider.ts @@ -76,6 +76,8 @@ export class WebViewProvider { this.pendingAskUserQuestionResolve = null; this.pendingAskUserQuestionRequest = null; } + // Disconnect the ACP agent process to prevent orphan processes + this.agentManager.disconnect(); this.disposables.forEach((d) => d.dispose()); }); this.messageHandler = new MessageHandler( @@ -578,6 +580,8 @@ export class WebViewProvider { // Clean up when the view is disposed webviewView.onDidDispose(() => { this.attachedWebview = null; + // Disconnect the ACP agent process to prevent orphan processes + this.agentManager.disconnect(); this.disposables.forEach((d) => d.dispose()); });