diff --git a/packages/server/src/server/agent/providers/acp-agent.test.ts b/packages/server/src/server/agent/providers/acp-agent.test.ts index f847745d69..95d7e25a21 100644 --- a/packages/server/src/server/agent/providers/acp-agent.test.ts +++ b/packages/server/src/server/agent/providers/acp-agent.test.ts @@ -1,8 +1,10 @@ import { type ChildProcess, type ChildProcessWithoutNullStreams } from "node:child_process"; import { EventEmitter } from "node:events"; -import { mkdtemp, rm } from "node:fs/promises"; +import { mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; +import { createRequire } from "node:module"; import { tmpdir } from "node:os"; import path from "node:path"; +import { pathToFileURL } from "node:url"; import { afterEach, describe, expect, test, vi } from "vitest"; import { AgentSideConnection, @@ -3436,6 +3438,8 @@ interface ACPCloseInternals { child: ChildProcess | null; connection: unknown; sessionId: string | null; + activeForegroundTurnId: string | null; + agentCapabilities: { sessionCapabilities?: { close?: unknown } } | null; } async function startTerminal( @@ -3502,6 +3506,38 @@ describe("ACPAgentSession close() tree-kill", () => { expect(child.kill).not.toHaveBeenCalled(); }); + test("close() terminates the provider when cancel and closeSession never settle", async () => { + vi.useFakeTimers(); + try { + const terminator = new FakeTerminator(); + const session = createSession({ terminateProcess: terminator.terminate }); + const child = createTerminalChildStub(); + const cancel = vi.fn(() => new Promise(() => undefined)); + const unstableCloseSession = vi.fn(() => new Promise(() => undefined)); + const internals = asInternals(session); + internals.child = child; + internals.sessionId = "session-1"; + internals.activeForegroundTurnId = "turn-1"; + internals.agentCapabilities = { sessionCapabilities: { close: {} } }; + internals.connection = { cancel, unstable_closeSession: unstableCloseSession }; + + let settled = false; + const closing = (async () => { + await session.close(); + settled = true; + })(); + await vi.advanceTimersByTimeAsync(5_000); + + expect(settled).toBe(true); + expect(cancel).toHaveBeenCalledWith({ sessionId: "session-1" }); + expect(unstableCloseSession).toHaveBeenCalledWith({ sessionId: "session-1" }); + expect(terminator.terminated).toContain(child); + await closing; + } finally { + vi.useRealTimers(); + } + }); + test("close() terminates running terminal child processes", async () => { const terminator = new FakeTerminator(); const session = createSession({ terminateProcess: terminator.terminate }); @@ -4220,3 +4256,117 @@ describe("ACP session/load invariant — cwd and mcpServers always passed", () = }); }); }); + +const SILENT_CLOSE_ACP_AGENT = ` +import { readFileSync, writeFileSync } from "node:fs"; +import { Readable, Writable } from "node:stream"; +const { AgentSideConnection, PROTOCOL_VERSION, ndJsonStream } = await import(process.env.ACP_SDK_URL); +writeFileSync(process.env.ACP_PID_FILE, String(process.pid)); +new AgentSideConnection( + () => ({ + async initialize() { + return { + protocolVersion: PROTOCOL_VERSION, + agentCapabilities: { sessionCapabilities: { close: {} } }, + }; + }, + async newSession() { + return { sessionId: "silent-close-session" }; + }, + async authenticate() {}, + async cancel() {}, + unstable_closeSession() { + return new Promise(() => {}); + }, + }), + ndJsonStream(Writable.toWeb(process.stdout), Readable.toWeb(process.stdin)), +); +`; + +function isProcessAlive(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch { + return false; + } +} + +async function settlesWithin(promise: Promise, timeoutMs: number): Promise { + let timer: NodeJS.Timeout | undefined; + const timedOut = new Promise((resolve) => { + timer = setTimeout(() => resolve(false), timeoutMs); + }); + try { + return await Promise.race([promise.then(() => true), timedOut]); + } finally { + clearTimeout(timer); + } +} + +interface SilentCloseProvider { + session: ACPAgentSession; + readPid(): Promise; + dispose(): Promise; +} + +async function startSilentCloseProvider(): Promise { + const dir = await mkdtemp(path.join(tmpdir(), "paseo-acp-silent-close-")); + const agentScript = path.join(dir, "agent.mjs"); + const pidFile = path.join(dir, "agent.pid"); + await writeFile(agentScript, SILENT_CLOSE_ACP_AGENT); + const session = new ACPAgentSession( + { provider: "silent-close-acp", cwd: dir }, + { + provider: "silent-close-acp", + logger: createTestLogger(), + defaultCommand: [process.execPath, agentScript], + defaultModes: [], + capabilities: { + supportsStreaming: true, + supportsSessionPersistence: true, + supportsDynamicModes: true, + supportsMcpServers: true, + supportsReasoningStream: true, + supportsToolInvocations: true, + }, + launchEnv: { + ACP_SDK_URL: pathToFileURL( + createRequire(import.meta.url).resolve("@agentclientprotocol/sdk"), + ).href, + ACP_PID_FILE: pidFile, + }, + }, + ); + const readPid = async () => Number(await readFile(pidFile, "utf8")); + return { + session, + readPid, + async dispose() { + const pid = await readPid().catch(() => null); + if (pid !== null && isProcessAlive(pid)) { + process.kill(pid, "SIGKILL"); + } + await rm(dir, { recursive: true, force: true }); + }, + }; +} + +describe("ACPAgentSession close() with an unresponsive provider", () => { + let provider: SilentCloseProvider | null = null; + + afterEach(async () => { + await provider?.dispose(); + provider = null; + }); + + test("terminates a provider that never answers session/close", async () => { + provider = await startSilentCloseProvider(); + await provider.session.initializeNewSession(); + const pid = await provider.readPid(); + expect(isProcessAlive(pid)).toBe(true); + + expect(await settlesWithin(provider.session.close(), 8_000)).toBe(true); + expect(isProcessAlive(pid)).toBe(false); + }, 15_000); +}); diff --git a/packages/server/src/server/agent/providers/acp-agent.ts b/packages/server/src/server/agent/providers/acp-agent.ts index 1b47ccaccb..a647deb549 100644 --- a/packages/server/src/server/agent/providers/acp-agent.ts +++ b/packages/server/src/server/agent/providers/acp-agent.ts @@ -301,6 +301,10 @@ export function buildACPClientCapabilities( const PROBE_ENV: Record = { NO_BROWSER: "true" }; const ACP_DIAGNOSTIC_PHASE_TIMEOUT_MS = 20_000; const ACP_PROBE_CLOSE_TIMEOUT_MS = 2_000; +// Archive and delete await close(). The ACP SDK does not reject a pending +// request when the provider never answers, so these calls must be bounded +// or the provider process is never terminated. +const ACP_CLOSE_REQUEST_TIMEOUT_MS = 2_000; const ACP_IMPORT_HISTORY_LOAD_TIMEOUT_MS = 30_000; const ACP_IMPORT_HISTORY_BUDGET_MS = 60_000; @@ -2476,13 +2480,21 @@ export class ACPAgentSession implements AgentSession, ACPClient { if (this.connection && this.sessionId) { try { if (this.activeForegroundTurnId) { - await this.connection.cancel({ sessionId: this.sessionId }); + await withTimeout( + this.connection.cancel({ sessionId: this.sessionId }), + ACP_CLOSE_REQUEST_TIMEOUT_MS, + `ACP cancel timed out after ${ACP_CLOSE_REQUEST_TIMEOUT_MS}ms`, + ); } } catch {} try { if (this.agentCapabilities?.sessionCapabilities?.close) { - await this.connection.unstable_closeSession({ sessionId: this.sessionId }); + await withTimeout( + this.connection.unstable_closeSession({ sessionId: this.sessionId }), + ACP_CLOSE_REQUEST_TIMEOUT_MS, + `ACP closeSession timed out after ${ACP_CLOSE_REQUEST_TIMEOUT_MS}ms`, + ); } } catch (error) { this.logger.debug({ err: error }, "ACP closeSession failed during shutdown");