diff --git a/.qwen/e2e-tests/vscode-acp-graceful-shutdown.md b/.qwen/e2e-tests/vscode-acp-graceful-shutdown.md new file mode 100644 index 00000000000..dcdc6a86089 --- /dev/null +++ b/.qwen/e2e-tests/vscode-acp-graceful-shutdown.md @@ -0,0 +1,21 @@ +# VS Code ACP graceful shutdown E2E plan + +## Scenario + +1. Start a VS Code Companion chat that launches an MCP stdio child. +2. Close or reload the companion while the chat is idle. +3. Confirm the ACP CLI exits normally and its MCP child does not remain alive. +4. Repeat with an intentionally unresponsive ACP child and confirm the host escalates only after the graceful deadline. +5. Reopen the companion during teardown and confirm the replacement connection remains usable after the old child exits. + +## Expected result + +- Ordinary teardown reaches the CLI cleanup path before the host uses a forced termination. +- No old child exit, notification, or response clears the replacement connection. +- POSIX escalation targets the ACP process group; Windows escalation targets the process tree. + +## Automated coverage + +The focused companion and CLI tests cover the shutdown ladder, replacement races, overlapping shutdown phases, and hook deadline. Native Windows process-tree behavior and a real VS Code host remain manual validation boundaries. + +The global `qwen` CLI cannot reproduce the companion-owned child lifecycle, so a standalone CLI dry run is not a faithful baseline for this scenario. diff --git a/docs/design/vscode-acp-graceful-shutdown.md b/docs/design/vscode-acp-graceful-shutdown.md new file mode 100644 index 00000000000..68538c24e23 --- /dev/null +++ b/docs/design/vscode-acp-graceful-shutdown.md @@ -0,0 +1,30 @@ +# VS Code ACP graceful shutdown + +[中文版](./vscode-acp-graceful-shutdown.zh-CN.md) + +## Problem + +The VS Code companion terminated its ACP child immediately when the panel disconnected. On Windows this bypassed the CLI shutdown path and could leave shells and ConPTY descendants alive. A graceful child can also overlap a replacement connection, so an old exit or response must not clear state owned by the replacement. + +## Decision + +The companion closes the ACP child's stdin first. The CLI treats the closed transport as a normal shutdown, fires SessionEnd hooks, drains MCP clients, disposes sessions, and runs process cleanup. + +Shutdown is bounded: + +- On POSIX, the ACP child leads a process group. After 75 seconds the companion sends SIGTERM to that group, then SIGKILL after another 75 seconds. +- On Windows, after 75 seconds the companion invokes the absolute System32 `taskkill.exe` path with `/f /t`. +- Exit handlers and asynchronous responses are tied to the child and connection that created them, so a retired process cannot mutate a replacement connection. +- Overlapping EOF and signal shutdown paths share the same SessionEnd, MCP drain, session-disposal, and registered process-cleanup work. SessionEnd hooks start concurrently and share a 30-second abort budget. + +## Scope + +This change covers ACP process teardown and the connection races introduced by graceful teardown. It does not close sessions when the user switches conversations and does not change hook process-tree ownership, which is handled separately. + +## Verification + +- Disconnect closes stdin before any forced termination. +- POSIX escalation targets the ACP child's process group; Windows escalation targets the process tree via `taskkill /f /t`, degrading to the direct child if taskkill fails. +- A normal child exit cancels escalation. +- A retired child or response cannot clear or update its replacement. +- EOF and signal shutdown execute each cleanup phase once, and all SessionEnd hooks begin within the shared deadline. diff --git a/docs/design/vscode-acp-graceful-shutdown.zh-CN.md b/docs/design/vscode-acp-graceful-shutdown.zh-CN.md new file mode 100644 index 00000000000..405ba5341ef --- /dev/null +++ b/docs/design/vscode-acp-graceful-shutdown.zh-CN.md @@ -0,0 +1,30 @@ +# VS Code ACP 优雅退出 + +[English](./vscode-acp-graceful-shutdown.md) + +## 问题 + +VS Code companion 在面板断开时会立即终止 ACP 子进程。Windows 上这种方式会绕过 CLI 的退出流程，可能遗留 shell 和 ConPTY 后代进程。优雅退出期间旧进程还可能与新连接短暂并存，因此旧进程的退出事件或异步响应不能清理新连接持有的状态。 + +## 决策 + +companion 首先关闭 ACP 子进程的 stdin。CLI 将传输关闭作为正常退出处理，触发 SessionEnd hooks、排空 MCP 客户端、释放会话并执行进程清理。 + +退出流程有明确上限： + +- POSIX 上 ACP 子进程作为进程组组长。75 秒后 companion 向该进程组发送 SIGTERM，再等待 75 秒后发送 SIGKILL。 +- Windows 上等待 75 秒后，companion 使用 System32 下的绝对 `taskkill.exe` 路径和 `/f /t` 参数终止进程树。 +- 退出处理器和异步响应绑定到创建它们的子进程与连接，已退役进程不能修改替代连接。 +- EOF 与信号触发的重叠退出共享同一次 SessionEnd、MCP 排空、会话释放和已注册进程清理。所有 SessionEnd hooks 并发启动，并共享 30 秒的中止时限。 + +## 范围 + +本次只处理 ACP 进程退出，以及优雅退出引入的连接竞态。不处理用户切换会话时关闭旧会话，也不改变 hook 进程树的归属；后者由独立改动处理。 + +## 验证 + +- 断开连接时先关闭 stdin，不立即强制终止。 +- POSIX 升级路径针对 ACP 子进程所在的进程组；Windows 升级路径通过 `taskkill /f /t` 针对进程树，taskkill 失败时退化为只终止直接子进程。 +- 子进程正常退出后取消升级计时器。 +- 已退役子进程的退出或响应不能清理或更新替代连接。 +- EOF 与信号重叠时每个清理阶段只执行一次，且所有 SessionEnd hooks 都在共享时限内启动。 diff --git a/packages/cli/src/acp-integration/acpAgent.test.ts b/packages/cli/src/acp-integration/acpAgent.test.ts index 48d1978f916..64917d28cdf 100644 --- a/packages/cli/src/acp-integration/acpAgent.test.ts +++ b/packages/cli/src/acp-integration/acpAgent.test.ts @@ -1686,6 +1686,44 @@ describe('runAcpAgent shutdown cleanup', () => { await agentPromise; }); + it('shares in-flight cleanup when SIGTERM overlaps an IDE close', async () => { + let resolveHook!: () => void; + const fireSessionEndEvent = vi.fn( + () => + new Promise((resolve) => { + resolveHook = resolve; + }), + ); + mockConfig.getHookSystem = vi.fn().mockReturnValue({ + fireSessionEndEvent, + }); + mockConfig.hasHooksForEvent = vi.fn().mockReturnValue(true); + const { agent, agentPromise } = await startPreloadTestAgent(); + expect(agent).toBeDefined(); + const shutdownMcpPool = vi.fn().mockResolvedValue(undefined); + const disposeSessions = vi.fn().mockResolvedValue(undefined); + Object.assign(agent!, { shutdownMcpPool, disposeSessions }); + + mockConnectionState.resolve(); + await vi.waitFor(() => { + expect(fireSessionEndEvent).toHaveBeenCalledWith( + SessionEndReason.PromptInputExit, + expect.any(AbortSignal), + ); + }); + sigTermListeners[0]('SIGTERM'); + await flushImmediate(); + expect(disposeSessions).not.toHaveBeenCalled(); + + resolveHook(); + await agentPromise; + await vi.waitFor(() => expect(processExitSpy).toHaveBeenCalledWith(0)); + + expect(fireSessionEndEvent).toHaveBeenCalledTimes(1); + expect(shutdownMcpPool).toHaveBeenCalledTimes(1); + expect(disposeSessions).toHaveBeenCalledTimes(1); + }); + it('still exits even if runExitCleanup throws', async () => { mockRunExitCleanup.mockRejectedValueOnce(new Error('cleanup failed')); @@ -1959,6 +1997,7 @@ describe('runAcpAgent SessionEnd hooks', () => { await vi.waitFor(() => { expect(mockHookSystem.fireSessionEndEvent).toHaveBeenCalledWith( SessionEndReason.Other, + expect.any(AbortSignal), ); }); @@ -1978,6 +2017,7 @@ describe('runAcpAgent SessionEnd hooks', () => { await vi.waitFor(() => { expect(mockHookSystem.fireSessionEndEvent).toHaveBeenCalledWith( SessionEndReason.Other, + expect.any(AbortSignal), ); }); @@ -1994,6 +2034,7 @@ describe('runAcpAgent SessionEnd hooks', () => { await vi.waitFor(() => { expect(mockHookSystem.fireSessionEndEvent).toHaveBeenCalledWith( SessionEndReason.PromptInputExit, + expect.any(AbortSignal), ); }); @@ -2057,6 +2098,7 @@ describe('runAcpAgent SessionEnd hooks', () => { await vi.waitFor(() => { expect(mockHookSystem.fireSessionEndEvent).toHaveBeenCalledWith( SessionEndReason.Other, + expect.any(AbortSignal), ); }); @@ -2069,6 +2111,36 @@ describe('runAcpAgent SessionEnd hooks', () => { // SessionEnd should have been called exactly once expect(mockHookSystem.fireSessionEndEvent).toHaveBeenCalledTimes(1); }); + + it('aborts a SessionEnd hook after the shutdown budget', async () => { + mockHookSystem.fireSessionEndEvent.mockImplementation( + (_reason: SessionEndReason, signal?: AbortSignal) => + new Promise((resolve) => { + signal?.addEventListener('abort', () => resolve(), { once: true }); + }), + ); + const agentPromise = runAcpAgent(mockConfig, mockSettings, mockArgv); + await vi.waitFor(() => expect(sigTermListeners.length).toBeGreaterThan(0)); + + vi.useFakeTimers(); + try { + sigTermListeners[0]('SIGTERM'); + await Promise.resolve(); + expect(mockHookSystem.fireSessionEndEvent).toHaveBeenCalledWith( + SessionEndReason.Other, + expect.any(AbortSignal), + ); + await vi.advanceTimersByTimeAsync(30_000); + await vi.waitFor(() => { + expect(mockRunExitCleanup).toHaveBeenCalledTimes(1); + }); + } finally { + vi.useRealTimers(); + } + + mockConnectionState.resolve(); + await agentPromise; + }); }); // --------------------------------------------------------------------------- @@ -21169,8 +21241,14 @@ describe('QwenAgent MCP SSE/HTTP support', () => { .mockImplementation((event: string) => event === 'SessionEnd'); const innerConfigA = await setupSessionMocks('session-end-a'); + let resolveSessionEndA!: () => void; const sessionHookSystemA = { - fireSessionEndEvent: vi.fn().mockResolvedValue(undefined), + fireSessionEndEvent: vi.fn( + () => + new Promise((resolve) => { + resolveSessionEndA = resolve; + }), + ), fireSessionStartEvent: vi.fn().mockResolvedValue(undefined), }; innerConfigA.getHookSystem = vi.fn().mockReturnValue(sessionHookSystemA); @@ -21235,16 +21313,24 @@ describe('QwenAgent MCP SSE/HTTP support', () => { await agent.newSession({ cwd: '/tmp', mcpServers: [] }); mockConnectionState.resolve(); + await vi.waitFor(() => { + expect(sessionHookSystemA.fireSessionEndEvent).toHaveBeenCalled(); + expect(sessionHookSystemB.fireSessionEndEvent).toHaveBeenCalled(); + }); + resolveSessionEndA(); await agentPromise; expect(bootstrapHookSystem.fireSessionEndEvent).toHaveBeenCalledWith( SessionEndReason.PromptInputExit, + expect.any(AbortSignal), ); expect(sessionHookSystemA.fireSessionEndEvent).toHaveBeenCalledWith( SessionEndReason.PromptInputExit, + expect.any(AbortSignal), ); expect(sessionHookSystemB.fireSessionEndEvent).toHaveBeenCalledWith( SessionEndReason.PromptInputExit, + expect.any(AbortSignal), ); }); diff --git a/packages/cli/src/acp-integration/acpAgent.ts b/packages/cli/src/acp-integration/acpAgent.ts index cfadfc7719b..452dc36c6c0 100644 --- a/packages/cli/src/acp-integration/acpAgent.ts +++ b/packages/cli/src/acp-integration/acpAgent.ts @@ -2912,72 +2912,116 @@ export async function runAcpAgent( // Both the SIGTERM handler and the IDE-initiated close path need // to drain the MCP pool before runExitCleanup. Single helper // closure keeps the timeout + log labels consistent. + let drainPoolPromise: Promise | undefined; const drainPoolBeforeExit = async ( label: string, strict = false, ): Promise => { if (!agentInstance) return; try { - await agentInstance.shutdownMcpPool(8_000); + drainPoolPromise ??= agentInstance.shutdownMcpPool(8_000); + await drainPoolPromise; } catch (err) { debugLogger.error(`[ACP] MCP pool drain (${label}) error:`, err); if (strict) throw err; } }; + let disposeSessionsPromise: Promise | undefined; + const disposeSessionsOnce = (): Promise => { + if (!agentInstance) return Promise.resolve(); + disposeSessionsPromise ??= agentInstance.disposeSessions(); + return disposeSessionsPromise; + }; + // 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; let managedShutdownPromise: Promise | undefined; - let sessionEndFired = false; + let sessionEndPromise: Promise | undefined; // Helper to fire SessionEnd hook once, preventing double-fire from both // shutdown handler path and connection.closed path. - const fireSessionEndOnce = async ( + const fireSessionEndOnce = ( reason: SessionEndReason, managedConfigs?: Config[], - ) => { - if (sessionEndFired) return; - sessionEndFired = true; - - const configs = new Set(managedConfigs ?? [config]); - if (!managedConfigs) { - const sessions = agentInstance?.getActiveSessions(); - if (sessions) { - for (const session of sessions) { - const sessionConfig = session.getConfig?.(); - if (sessionConfig) { - configs.add(sessionConfig); + ): Promise => { + if (sessionEndPromise) return sessionEndPromise; + + sessionEndPromise = (async () => { + const configs = new Set(managedConfigs ?? [config]); + if (!managedConfigs) { + const sessions = agentInstance?.getActiveSessions(); + if (sessions) { + for (const session of sessions) { + const sessionConfig = session.getConfig?.(); + if (sessionConfig) { + configs.add(sessionConfig); + } } } } - } - const failures: unknown[] = []; - for (const cfg of configs) { - const hookSystem = cfg.getHookSystem?.(); - const hooksEnabled = !cfg.getDisableAllHooks?.(); - if ( - !hooksEnabled || - !hookSystem || - !cfg.hasHooksForEvent?.('SessionEnd') - ) { - continue; - } + const controller = new AbortController(); + const timeout = setTimeout(() => controller.abort(), 30_000); + timeout.unref(); try { - await hookSystem.fireSessionEndEvent(reason); - } catch (err) { - if (managedConfigs) failures.push(err); - debugLogger.warn( - `SessionEnd hook failed: ${err instanceof Error ? err.message : String(err)}`, + const results = await Promise.allSettled( + [...configs].flatMap((cfg) => { + const hookSystem = cfg.getHookSystem?.(); + if ( + cfg.getDisableAllHooks?.() || + !hookSystem || + typeof hookSystem.fireSessionEndEvent !== 'function' || + !cfg.hasHooksForEvent?.('SessionEnd') + ) { + return []; + } + return [ + Promise.resolve().then(() => + hookSystem.fireSessionEndEvent(reason, controller.signal), + ), + ]; + }), ); + const failures = results + .filter( + (result): result is PromiseRejectedResult => + result.status === 'rejected', + ) + .map((result) => result.reason); + // A SessionEnd hook that outlives the 30s budget is cancelled rather + // than rejected: `fireSessionEndEvent` resolves to `undefined` for a + // cancelled hook (the `{ success: false, outcome: 'cancelled' }` + // result never rejects), so `Promise.allSettled` cannot observe it and + // `failures` above stays empty. Detect the abort directly so the + // cancellation is recorded at all, instead of the CLI exiting 0 as + // though every hook had run. What recording it buys depends on the + // caller: the throw below is gated on `managedConfigs`, so a managed + // shutdown turns it into a non-zero exit while an unmanaged one gets + // the warning line only. + if (controller.signal.aborted) { + failures.push( + new Error( + 'SessionEnd hook did not complete within 30s (cancelled)', + ), + ); + } + for (const failure of failures) { + debugLogger.warn( + `SessionEnd hook failed: ${failure instanceof Error ? failure.message : String(failure)}`, + ); + } + if (managedConfigs && failures.length > 0) { + throw new AggregateError(failures, 'SessionEnd hook shutdown failed'); + } + } finally { + clearTimeout(timeout); } - } - if (failures.length > 0) { - throw new AggregateError(failures, 'SessionEnd hook shutdown failed'); - } + })(); + return sessionEndPromise; }; const shutdownManagedAgent = ( @@ -3080,7 +3124,7 @@ export async function runAcpAgent( try { // Fire SessionEnd hook for all active sessions (aligned with core path) await fireSessionEndOnce(SessionEndReason.Other); - await agentInstance?.disposeSessions(); + await disposeSessionsOnce(); try { process.stdin.destroy(); @@ -3132,7 +3176,7 @@ export async function runAcpAgent( // Mirror the SIGTERM handler's pool drain on the IDE-initiated // normal close path to avoid leaking shared MCP entries. await drainPoolBeforeExit('ide_close'); - await agentInstance?.disposeSessions(); + await disposeSessionsOnce(); } } finally { process.off('SIGTERM', shutdownHandler); diff --git a/packages/cli/src/utils/cleanup.test.ts b/packages/cli/src/utils/cleanup.test.ts index 80f2fe5afba..d5280017981 100644 --- a/packages/cli/src/utils/cleanup.test.ts +++ b/packages/cli/src/utils/cleanup.test.ts @@ -52,6 +52,26 @@ describe('cleanup', () => { expect(asyncFn).toHaveBeenCalledTimes(1); }); + it('shares an in-flight cleanup pass between concurrent callers', async () => { + let finishCleanup: (() => void) | undefined; + const cleanupFn = vi.fn( + () => + new Promise((resolve) => { + finishCleanup = resolve; + }), + ); + registerCleanup(cleanupFn); + + const first = runExitCleanup(); + await vi.waitFor(() => expect(cleanupFn).toHaveBeenCalledOnce()); + const second = runExitCleanup(); + + expect(second).toBe(first); + finishCleanup?.(); + await Promise.all([first, second]); + expect(cleanupFn).toHaveBeenCalledOnce(); + }); + it('should let a caller unregister a cleanup', async () => { const cleanupFn = vi.fn(); const unregister = registerCleanup(cleanupFn); diff --git a/packages/cli/src/utils/cleanup.ts b/packages/cli/src/utils/cleanup.ts index 8a0c28e68de..234fc51f6e4 100644 --- a/packages/cli/src/utils/cleanup.ts +++ b/packages/cli/src/utils/cleanup.ts @@ -8,6 +8,7 @@ import { promises as fs } from 'node:fs'; import { join } from 'node:path'; const cleanupFunctions: Array<(() => void) | (() => Promise)> = []; +let exitCleanupPromise: Promise | undefined; export function registerCleanup( fn: (() => void) | (() => Promise), @@ -65,8 +66,19 @@ export interface RunExitCleanupOptions { _testOverallTimeoutMs?: number; } -export async function runExitCleanup( +export function runExitCleanup( options: RunExitCleanupOptions = {}, +): Promise { + if (exitCleanupPromise) return exitCleanupPromise; + const cleanup = runExitCleanupPass(options).finally(() => { + if (exitCleanupPromise === cleanup) exitCleanupPromise = undefined; + }); + exitCleanupPromise = cleanup; + return cleanup; +} + +async function runExitCleanupPass( + options: RunExitCleanupOptions, ): Promise { const perFn = options._testPerFnTimeoutMs ?? PER_CLEANUP_TIMEOUT_MS; const overall = options._testOverallTimeoutMs ?? OVERALL_CLEANUP_TIMEOUT_MS; @@ -106,6 +118,7 @@ export async function runExitCleanup( */ export function _resetCleanupFunctionsForTest(): void { cleanupFunctions.length = 0; + exitCleanupPromise = undefined; } export async function cleanupCheckpoints() { diff --git a/packages/vscode-ide-companion/src/services/acpConnection.test.ts b/packages/vscode-ide-companion/src/services/acpConnection.test.ts index 716cacf644a..7acf6f02d8d 100644 --- a/packages/vscode-ide-companion/src/services/acpConnection.test.ts +++ b/packages/vscode-ide-companion/src/services/acpConnection.test.ts @@ -4,29 +4,50 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, expect, it, vi } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { RequestError } from '@agentclientprotocol/sdk'; -import type { ContentBlock } from '@agentclientprotocol/sdk'; +import type { + ContentBlock, + LoadSessionResponse, + NewSessionResponse, + PromptResponse, +} from '@agentclientprotocol/sdk'; const spawnMock = vi.hoisted(() => vi.fn()); +const execFileMock = vi.hoisted(() => vi.fn()); // AcpConnection imports AcpFileHandler which imports vscode. // Mock vscode so it can be resolved without the actual VS Code runtime. vi.mock('vscode', () => ({})); vi.mock('child_process', async (importOriginal) => { const actual = await importOriginal(); - return { ...actual, spawn: spawnMock }; + return { ...actual, spawn: spawnMock, execFile: execFileMock }; }); import { AcpConnection } from './acpConnection.js'; import { ACP_ERROR_CODES } from '../constants/acpSchema.js'; +type MockChild = { + killed: boolean; + exitCode: number | null; + signalCode?: NodeJS.Signals | null; + pid?: number; + kill?: (signal?: NodeJS.Signals) => boolean; + stdin?: { + destroyed?: boolean; + writableEnded?: boolean; + end: () => void; + once: (event: string, listener: () => void) => unknown; + } | null; + stderr?: { on: (event: string, listener: (data: Buffer) => void) => unknown }; + on?: (event: string, listener: (...args: unknown[]) => void) => unknown; + once?: (event: string, listener: () => void) => unknown; +}; + type AcpConnectionInternal = { - child: { killed: boolean; exitCode: number | null; kill?: () => void } | null; + child: MockChild | null; sdkConnection: unknown; sessionId: string | null; - lastExitCode: number | null; - lastExitSignal: string | null; mapReadTextFileError: (error: unknown, filePath: string) => unknown; ensureConnection: () => unknown; }; @@ -43,12 +64,29 @@ function createMockChild(overrides?: Record) { return { killed: false, exitCode: null, - kill: vi.fn(), + signalCode: null, + pid: 4242, + kill: vi.fn().mockReturnValue(true), + stdin: { + destroyed: false, + writableEnded: false, + end: vi.fn(), + once: vi.fn(), + }, + once: vi.fn(), ...overrides, - } as unknown as AcpConnectionInternal['child']; + } as MockChild; } describe('AcpConnection process spawning', () => { + beforeEach(() => { + spawnMock.mockReset(); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + it('runs the managed ACP child in Electron Node mode', async () => { vi.stubEnv('ELECTRON_RUN_AS_NODE', ''); vi.stubEnv('QWEN_CODE_SCRUB_ELECTRON_RUN_AS_NODE', ''); @@ -71,6 +109,36 @@ describe('AcpConnection process spawning', () => { vi.unstubAllEnvs(); } }); + + it('creates a POSIX process group for shutdown escalation', async () => { + vi.spyOn(process, 'platform', 'get').mockReturnValue('linux'); + spawnMock.mockReturnValue(createMockChild()); + const conn = new AcpConnection() as unknown as { + connect: (cliEntryPath: string) => Promise; + setupChildProcessHandlers: () => Promise; + }; + conn.setupChildProcessHandlers = vi.fn().mockResolvedValue(undefined); + + await conn.connect(process.execPath); + + const options = spawnMock.mock.calls.at(-1)?.[2] as { detached?: boolean }; + expect(options.detached).toBe(true); + }); + + it('does not detach the ACP child on Windows', async () => { + vi.spyOn(process, 'platform', 'get').mockReturnValue('win32'); + spawnMock.mockReturnValue(createMockChild()); + const conn = new AcpConnection() as unknown as { + connect: (cliEntryPath: string) => Promise; + setupChildProcessHandlers: () => Promise; + }; + conn.setupChildProcessHandlers = vi.fn().mockResolvedValue(undefined); + + await conn.connect(process.execPath); + + const options = spawnMock.mock.calls.at(-1)?.[2] as { detached?: boolean }; + expect(options.detached).toBe(false); + }); }); describe('AcpConnection readTextFile error mapping', () => { @@ -203,6 +271,16 @@ describe('AcpConnection.ensureConnection', () => { }); describe('AcpConnection child exit cleanup', () => { + beforeEach(() => { + vi.useFakeTimers(); + execFileMock.mockReset(); + }); + + afterEach(() => { + vi.useRealTimers(); + vi.restoreAllMocks(); + }); + it('disconnect clears child, sdkConnection, and sessionId', () => { const conn = createConnection({ child: createMockChild(), @@ -218,16 +296,282 @@ describe('AcpConnection child exit cleanup', () => { expect(acpConn.currentSessionId).toBeNull(); }); - it('disconnect calls kill on the child process', () => { + it('disconnect closes stdin before escalating', () => { const mockKill = vi.fn(); + const end = vi.fn(); + const stdinOnce = vi.fn(); const conn = createConnection({ - child: createMockChild({ kill: mockKill }), + child: createMockChild({ + kill: mockKill, + stdin: { + destroyed: false, + writableEnded: false, + end, + once: stdinOnce, + }, + }), sdkConnection: {}, sessionId: 'test-session', }); (conn as unknown as AcpConnection).disconnect(); - expect(mockKill).toHaveBeenCalledOnce(); + expect(end).toHaveBeenCalledOnce(); + expect(stdinOnce).toHaveBeenCalledWith('error', expect.any(Function)); + expect(mockKill).not.toHaveBeenCalled(); + }); + + it('disconnect closes stdin even when the child has no pid', () => { + const end = vi.fn(); + const conn = createConnection({ + child: createMockChild({ + pid: undefined, + stdin: { + destroyed: false, + writableEnded: false, + end, + once: vi.fn(), + }, + }), + }); + + (conn as unknown as AcpConnection).disconnect(); + + expect(end).toHaveBeenCalledOnce(); + expect(vi.getTimerCount()).toBe(0); + }); + + it('escalates to the POSIX process group after both grace periods', () => { + if (process.platform === 'win32') return; + const kill = vi.spyOn(process, 'kill').mockReturnValue(true); + const conn = createConnection({ child: createMockChild() }); + + (conn as unknown as AcpConnection).disconnect(); + vi.advanceTimersByTime(75_000); + expect(kill).toHaveBeenCalledWith(-4242, 'SIGTERM'); + vi.advanceTimersByTime(75_000); + expect(kill).toHaveBeenCalledWith(-4242, 'SIGKILL'); + }); + + it('falls back to signalling the child when POSIX group signalling fails', () => { + if (process.platform === 'win32') return; + vi.spyOn(process, 'kill').mockImplementation(() => { + throw new Error('missing process group'); + }); + const childKill = vi.fn().mockReturnValue(true); + const conn = createConnection({ + child: createMockChild({ kill: childKill }), + }); + + (conn as unknown as AcpConnection).disconnect(); + vi.advanceTimersByTime(75_000); + expect(childKill).toHaveBeenCalledWith('SIGTERM'); + vi.advanceTimersByTime(75_000); + expect(childKill).toHaveBeenCalledWith('SIGKILL'); + }); + + it('uses taskkill for an unresponsive Windows process tree', () => { + vi.spyOn(process, 'platform', 'get').mockReturnValue('win32'); + const childKill = vi.fn(); + const conn = createConnection({ + child: createMockChild({ kill: childKill }), + }); + + (conn as unknown as AcpConnection).disconnect(); + vi.advanceTimersByTime(75_000); + + expect(execFileMock).toHaveBeenCalledWith( + expect.stringMatching(/\\System32\\taskkill\.exe$/i), + ['/f', '/t', '/pid', '4242'], + expect.objectContaining({ windowsHide: true }), + expect.any(Function), + ); + expect(childKill).not.toHaveBeenCalled(); + }); + + it('degrades to child.kill() when taskkill fails', () => { + vi.spyOn(process, 'platform', 'get').mockReturnValue('win32'); + const childKill = vi.fn().mockReturnValue(true); + const conn = createConnection({ + child: createMockChild({ kill: childKill }), + }); + execFileMock.mockImplementation( + ( + _file: unknown, + _args: unknown, + _options: unknown, + callback: (error: Error) => void, + ) => { + callback(new Error('spawn taskkill.exe ENOENT')); + }, + ); + + (conn as unknown as AcpConnection).disconnect(); + vi.advanceTimersByTime(75_000); + + expect(execFileMock).toHaveBeenCalled(); + expect(childKill).toHaveBeenCalled(); + }); + + it('cancels escalation when the child exits normally', () => { + let onExit: (() => void) | undefined; + const kill = vi.spyOn(process, 'kill').mockReturnValue(true); + const child = createMockChild({ + once: vi.fn((event: string, listener: () => void) => { + if (event === 'exit') onExit = listener; + }), + }); + const conn = createConnection({ child }); + + (conn as unknown as AcpConnection).disconnect(); + onExit?.(); + vi.advanceTimersByTime(150_000); + + expect(kill).not.toHaveBeenCalled(); + expect(child.kill).not.toHaveBeenCalled(); + }); + + it('ignores an exit from a child replaced during graceful shutdown', async () => { + let exitHandler: + | ((code: number | null, signal: string | null) => void) + | undefined; + const oldChild = createMockChild({ + stderr: { on: vi.fn() }, + on: vi.fn((event: string, listener: (...args: unknown[]) => void) => { + if (event === 'exit') { + exitHandler = listener as typeof exitHandler; + } + }), + }); + const onDisconnected = vi.fn(); + const conn = createConnection({ child: oldChild }); + (conn as unknown as AcpConnection).onDisconnected = onDisconnected; + const setup = ( + conn as unknown as { setupChildProcessHandlers: () => Promise } + ).setupChildProcessHandlers(); + void setup.catch(() => {}); + const replacement = createMockChild(); + conn.child = replacement; + conn.sdkConnection = {}; + conn.sessionId = 'replacement'; + + exitHandler?.(3, 'SIGTERM'); + await vi.advanceTimersByTimeAsync(1_000); + await expect(setup).rejects.toThrow( + 'Qwen ACP process failed to start (exit code: 3, signal: SIGTERM)', + ); + + expect(conn.child).toBe(replacement); + expect(conn.sdkConnection).toEqual({}); + expect(conn.sessionId).toBe('replacement'); + expect(onDisconnected).not.toHaveBeenCalled(); + }); + + it('invokes onDisconnected with the exit info when the current child exits', async () => { + let exitHandler: + | ((code: number | null, signal: string | null) => void) + | undefined; + const child = createMockChild({ + stderr: { on: vi.fn() }, + on: vi.fn((event: string, listener: (...args: unknown[]) => void) => { + if (event === 'exit') { + exitHandler = listener as typeof exitHandler; + } + }), + }); + const onDisconnected = vi.fn(); + const conn = createConnection({ child }); + (conn as unknown as AcpConnection).onDisconnected = onDisconnected; + const setup = ( + conn as unknown as { setupChildProcessHandlers: () => Promise } + ).setupChildProcessHandlers(); + void setup.catch(() => {}); + + exitHandler?.(1, 'SIGTERM'); + await vi.advanceTimersByTimeAsync(1_000); + + expect(onDisconnected).toHaveBeenCalledWith(1, 'SIGTERM'); + expect(conn.child).toBeNull(); + expect(conn.sdkConnection).toBeNull(); + expect(conn.sessionId).toBeNull(); + }); +}); + +describe('AcpConnection stale responses', () => { + it('does not apply session or prompt responses from a retired connection', async () => { + let resolveNew!: (value: NewSessionResponse) => void; + let resolveLoad!: (value: LoadSessionResponse) => void; + let resolvePrompt!: (value: PromptResponse) => void; + const sdk = { + newSession: vi.fn( + () => + new Promise((resolve) => (resolveNew = resolve)), + ), + loadSession: vi.fn( + () => + new Promise( + (resolve) => (resolveLoad = resolve), + ), + ), + prompt: vi.fn( + () => + new Promise((resolve) => (resolvePrompt = resolve)), + ), + }; + const onEndTurn = vi.fn(); + const conn = createConnection({ + child: createMockChild(), + sdkConnection: sdk, + sessionId: 'old-session', + }); + (conn as unknown as AcpConnection).onEndTurn = onEndTurn; + const acp = conn as unknown as AcpConnection; + + const create = acp.newSession(); + const load = acp.loadSession('loaded-session'); + const prompt = acp.sendPrompt('hello'); + void create.catch(() => {}); + void load.catch(() => {}); + void prompt.catch(() => {}); + conn.sdkConnection = {}; + conn.sessionId = 'old-session'; + resolveNew({ sessionId: 'created-session' }); + resolveLoad({}); + resolvePrompt({ stopReason: 'end_turn' }); + + await expect(create).rejects.toThrow('connection superseded'); + await expect(load).rejects.toThrow('connection superseded'); + await expect(prompt).rejects.toThrow('connection superseded'); + expect(conn.sessionId).toBe('old-session'); + expect(onEndTurn).not.toHaveBeenCalled(); + }); + + it('delivers a prompt that completes after a session switch on the same connection', async () => { + let resolvePrompt!: (value: PromptResponse) => void; + const sdk = { + prompt: vi.fn( + () => + new Promise((resolve) => (resolvePrompt = resolve)), + ), + }; + const onEndTurn = vi.fn(); + const conn = createConnection({ + child: createMockChild(), + sdkConnection: sdk, + sessionId: 'session-a', + }); + (conn as unknown as AcpConnection).onEndTurn = onEndTurn; + const acp = conn as unknown as AcpConnection; + + const prompt = acp.sendPrompt('hello'); + void prompt.catch(() => {}); + // The user switches to another session on the SAME live connection while + // the prompt is in flight. This must not be reported as a superseded + // connection: the turn completed, so it resolves and emits end-of-turn. + conn.sessionId = 'session-b'; + resolvePrompt({ stopReason: 'end_turn' }); + + await expect(prompt).resolves.toMatchObject({ stopReason: 'end_turn' }); + expect(onEndTurn).toHaveBeenCalledWith('end_turn'); }); }); @@ -248,14 +592,6 @@ describe('AcpConnection onDisconnected callback', () => { }); }); -describe('AcpConnection lastExitCode/lastExitSignal', () => { - it('initializes exit info as null', () => { - const conn = createConnection(); - expect(conn.lastExitCode).toBeNull(); - expect(conn.lastExitSignal).toBeNull(); - }); -}); - describe('AcpConnection extension notifications', () => { it('parses end_turn reason and source', () => { const conn = new AcpConnection(); diff --git a/packages/vscode-ide-companion/src/services/acpConnection.ts b/packages/vscode-ide-companion/src/services/acpConnection.ts index 1d65d6de2ee..9cbc2df0632 100644 --- a/packages/vscode-ide-companion/src/services/acpConnection.ts +++ b/packages/vscode-ide-companion/src/services/acpConnection.ts @@ -37,12 +37,16 @@ import type { } from '../types/acpTypes.js'; import type { ApprovalModeValue } from '../types/approvalModeValueTypes.js'; import type { ChildProcess, SpawnOptions } from 'child_process'; -import { spawn } from 'child_process'; +import { execFile, spawn } from 'child_process'; import { Readable, Writable } from 'node:stream'; import * as fs from 'node:fs'; import { AcpFileHandler } from './acpFileHandler.js'; import { ACP_ERROR_CODES } from '../constants/acpSchema.js'; +const SHUTDOWN_GRACE_MS = 75_000; +const SIGTERM_GRACE_MS = 75_000; +const WINDOWS_TASKKILL = `${process.env['SystemRoot'] || 'C:\\Windows'}\\System32\\taskkill.exe`; + /** * ACP Connection Handler for VSCode Extension * @@ -55,8 +59,6 @@ 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<{ @@ -88,8 +90,6 @@ export class AcpConnection { this.disconnect(); } - this.lastExitCode = null; - this.lastExitSignal = null; this.workingDir = workingDir; const env = { ...process.env }; @@ -130,6 +130,7 @@ export class AcpConnection { stdio: ['pipe', 'pipe', 'pipe'], env, shell: false, + detached: process.platform !== 'win32', }; this.child = spawn(spawnCommand, spawnArgs, options); @@ -139,13 +140,17 @@ export class AcpConnection { private async setupChildProcessHandlers(): Promise { let spawnError: Error | null = null; const stderrChunks: string[] = []; + const ownChild = this.child!; + let ownExitCode: number | null = null; + let ownExitSignal: string | null = null; let rejectOnExit: ((error: Error) => void) | null = null; const processExitPromise = new Promise((_resolve, reject) => { rejectOnExit = reject; }); + void processExitPromise.catch(() => {}); - this.child!.stderr?.on('data', (data: Buffer) => { + ownChild.stderr?.on('data', (data: Buffer) => { const message = data.toString(); stderrChunks.push(message); if ( @@ -158,16 +163,16 @@ export class AcpConnection { } }); - this.child!.on('error', (error: Error) => { + ownChild.on('error', (error: Error) => { spawnError = error; }); - this.child!.on('exit', (code: number | null, signal: string | null) => { + ownChild.on('exit', (code: number | null, signal: string | null) => { logger.error( `[ACP qwen] Process exited with code: ${code}, signal: ${signal}`, ); - this.lastExitCode = code; - this.lastExitSignal = signal; + ownExitCode = code; + ownExitSignal = signal; const stderrOutput = stderrChunks.join('').trim(); const stderrSuffix = stderrOutput @@ -179,7 +184,7 @@ export class AcpConnection { ), ); - if (this.child) { + if (this.child === ownChild) { this.sdkConnection = null; this.sessionId = null; this.child = null; @@ -193,9 +198,9 @@ export class AcpConnection { throw spawnError; } - if (!this.child || this.child.killed) { - const code = this.lastExitCode ?? this.child?.exitCode ?? null; - const signal = this.lastExitSignal; + if (this.child !== ownChild || ownChild.killed) { + const code = ownExitCode ?? ownChild.exitCode ?? null; + const signal = ownExitSignal ?? ownChild.signalCode ?? null; const stderrOutput = stderrChunks.join('').trim(); const stderrSuffix = stderrOutput ? `\nCLI stderr: ${stderrOutput.slice(-500)}` @@ -207,16 +212,19 @@ export class AcpConnection { // Convert Node.js child process streams to Web Streams for SDK const stdout = Readable.toWeb( - this.child.stdout!, + ownChild.stdout!, ) as ReadableStream; - const stdin = Writable.toWeb(this.child.stdin!) as WritableStream; + const stdin = Writable.toWeb(ownChild.stdin!) as WritableStream; const stream = ndJsonStream(stdin, stdout); // Build the SDK Client implementation that bridges to our callbacks. - this.sdkConnection = new ClientSideConnection( + const wiredConnection = new ClientSideConnection( (_agent: Agent): Client => ({ sessionUpdate: (params: SessionNotification): Promise => { + if (this.sdkConnection !== wiredConnection) { + return Promise.resolve(); + } this.onSessionUpdate(params as unknown as SessionNotification); return Promise.resolve(); }, @@ -224,6 +232,12 @@ export class AcpConnection { requestPermission: async ( params: RequestPermissionRequest, ): Promise => { + if (this.sdkConnection !== wiredConnection) { + throw RequestError.internalError( + { details: 'connection superseded' }, + 'connection superseded', + ); + } const permissionData = params as unknown as RequestPermissionRequest; try { // Check if this is an ask_user_question request by inspecting rawInput @@ -310,6 +324,12 @@ export class AcpConnection { readTextFile: async ( params: ReadTextFileRequest, ): Promise => { + if (this.sdkConnection !== wiredConnection) { + throw RequestError.internalError( + { details: 'connection superseded' }, + 'connection superseded', + ); + } try { const result = await this.fileHandler.handleReadTextFile({ path: params.path, @@ -326,6 +346,12 @@ export class AcpConnection { writeTextFile: async ( params: WriteTextFileRequest, ): Promise => { + if (this.sdkConnection !== wiredConnection) { + throw RequestError.internalError( + { details: 'connection superseded' }, + 'connection superseded', + ); + } await this.fileHandler.handleWriteTextFile({ path: params.path, content: params.content, @@ -337,16 +363,20 @@ export class AcpConnection { extNotification: async ( method: string, params: Record, - ): Promise => this.handleExtNotification(method, params), + ): Promise => { + if (this.sdkConnection !== wiredConnection) return; + this.handleExtNotification(method, params); + }, }), stream, ); + this.sdkConnection = wiredConnection; // Race the SDK initialize against process exit so we don't hang forever // if the CLI crashes before responding. logger.log('[ACP] Sending initialize request...'); const initResponse = await Promise.race([ - this.sdkConnection.initialize({ + wiredConnection.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: { fs: { @@ -358,6 +388,13 @@ export class AcpConnection { processExitPromise, ]); + if (this.sdkConnection !== wiredConnection || this.child !== ownChild) { + throw RequestError.internalError( + { details: 'connection superseded' }, + 'connection superseded', + ); + } + logger.log('[ACP] Initialize successful'); logger.log( '[ACP] Initialization response protocol:', @@ -465,6 +502,12 @@ export class AcpConnection { cwd, mcpServers: [], }); + if (this.sdkConnection !== conn) { + throw RequestError.internalError( + { details: 'connection superseded' }, + 'connection superseded', + ); + } this.sessionId = response.sessionId || null; logger.log('[ACP] Session created with ID:', this.sessionId); return response; @@ -472,7 +515,8 @@ export class AcpConnection { async sendPrompt(prompt: string | ContentBlock[]): Promise { const conn = this.ensureConnection(); - if (!this.sessionId) { + const promptSessionId = this.sessionId; + if (!promptSessionId) { throw new Error('No active ACP session'); } const promptBlocks = @@ -480,9 +524,21 @@ export class AcpConnection { ? [{ type: 'text' as const, text: prompt }] : prompt; const response: PromptResponse = await conn.prompt({ - sessionId: this.sessionId, + sessionId: promptSessionId, prompt: promptBlocks, }); + // Only a truly superseded connection (or a prompt rejected/cancelled by + // the SDK) should error. Switching to another session on the SAME live + // connection reassigns `this.sessionId` (see `newSession`) without + // replacing `sdkConnection`, so folding `this.sessionId !== + // promptSessionId` into this guard would misreport a successfully + // completed turn as "connection superseded". + if (this.sdkConnection !== conn) { + throw RequestError.internalError( + { details: 'connection superseded' }, + 'connection superseded', + ); + } // Emit end-of-turn from stopReason if (response.stopReason) { this.onEndTurn(response.stopReason); @@ -527,15 +583,13 @@ export class AcpConnection { const conn = this.ensureConnection(); logger.log('[ACP] Sending session/load request for session:', sessionId); const cwd = cwdOverride || this.workingDir; + let response: LoadSessionResponse; try { - const response = await conn.loadSession({ + response = await conn.loadSession({ sessionId, cwd, mcpServers: [], }); - logger.log('[ACP] Session load succeeded for session:', sessionId); - this.sessionId = sessionId; - return response; } catch (error) { logger.error( '[ACP] Session load request failed:', @@ -543,6 +597,15 @@ export class AcpConnection { ); throw error; } + if (this.sdkConnection !== conn) { + throw RequestError.internalError( + { details: 'connection superseded' }, + 'connection superseded', + ); + } + logger.log('[ACP] Session load succeeded for session:', sessionId); + this.sessionId = sessionId; + return response; } async listSessions(options?: { @@ -676,12 +739,75 @@ export class AcpConnection { } disconnect(): void { - if (this.child) { - this.child.kill(); - this.child = null; - } + const child = this.child; + this.child = null; this.sdkConnection = null; this.sessionId = null; + if (!child) return; + + if (child.stdin && !child.stdin.destroyed && !child.stdin.writableEnded) { + child.stdin.once('error', () => {}); + try { + child.stdin.end(); + } catch (error) { + logger.error( + '[ACP] Failed to close CLI stdin during disconnect:', + error, + ); + } + } + + const childPid = child.pid; + if (!childPid) return; + + let killTimer: NodeJS.Timeout | undefined; + const graceTimer = setTimeout(() => { + if (child.exitCode !== null || child.signalCode !== null) return; + + if (process.platform === 'win32') { + execFile( + WINDOWS_TASKKILL, + ['/f', '/t', '/pid', String(childPid)], + { windowsHide: true, timeout: 2_000 }, + (error) => { + if (!error) return; + logger.error('[ACP] taskkill failed for the CLI tree:', error); + try { + child.kill(); + } catch { + // Already gone. + } + }, + ); + return; + } + + try { + process.kill(-childPid, 'SIGTERM'); + } catch { + try { + child.kill('SIGTERM'); + } catch { + // Already gone. + } + } + killTimer = setTimeout(() => { + if (child.exitCode !== null || child.signalCode !== null) return; + try { + process.kill(-childPid, 'SIGKILL'); + } catch { + try { + child.kill('SIGKILL'); + } catch { + // Already gone. + } + } + }, SIGTERM_GRACE_MS); + }, SHUTDOWN_GRACE_MS); + child.once('exit', () => { + clearTimeout(graceTimer); + if (killTimer) clearTimeout(killTimer); + }); } get isConnected(): boolean {