From a6ec352f0745542b907ca7a166ab61edfd7aeda9 Mon Sep 17 00:00:00 2001 From: yeshan333 Date: Sun, 14 Jun 2026 23:55:27 +0800 Subject: [PATCH] Use terminateWithTreeKill for ACP agent child process cleanup (#1460) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(server): use terminateWithTreeKill for ACP agent child process cleanup ACPAgentSession.close() was using child.kill("SIGTERM") which only signals the direct child process (e.g. npx), leaving grandchild processes (sh -c, node qodercli) alive as orphans. Over time these accumulate and consume significant memory (~330MB each). Replace all direct child.kill("SIGTERM") calls in ACP agent lifecycle with terminateWithTreeKill(), which recursively walks the process tree via /proc on Linux and taskkill /T on Windows. This aligns ACP with OpenCode, Pi, and Codex providers which already use tree-kill. Affected code paths: - ACPAgentSession.close() — main child process - ACPAgentSession.close() — terminal child processes - ACPAgentSession.killTerminal() - ACPAgentSession.releaseTerminal() - terminateChildProcess() helper (used by closeProbe and init failure) Removes the now-unused waitForChildExit() helper. Co-Authored-By: Claude Opus 4.6 * fix(server): address acp tree kill review * test(server): cover acp release terminal tree kill --------- Co-authored-by: Claude Opus 4.6 --- .../server/agent/providers/acp-agent.test.ts | 220 +++++++++++++++++- .../src/server/agent/providers/acp-agent.ts | 47 ++-- 2 files changed, 238 insertions(+), 29 deletions(-) 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 cf5e58036..0f63f31ed 100644 --- a/packages/server/src/server/agent/providers/acp-agent.test.ts +++ b/packages/server/src/server/agent/providers/acp-agent.test.ts @@ -1,4 +1,4 @@ -import { type ChildProcess } from "node:child_process"; +import { type ChildProcess, type ChildProcessWithoutNullStreams } from "node:child_process"; import { EventEmitter } from "node:events"; import { afterEach, describe, expect, test, vi } from "vitest"; import { @@ -28,6 +28,7 @@ import { resolveACPModelSelection, summarizeACPRequestError, } from "./acp-agent.js"; +import * as treeKillModule from "../../../utils/tree-kill.js"; import { COPILOT_ALLOW_ALL_MODE_ID, COPILOT_MODES, @@ -144,6 +145,35 @@ function createTerminalChildStub(): ChildProcess { return child; } +function createProbeChildStub(order: string[]): ChildProcessWithoutNullStreams { + const child = new EventEmitter() as ChildProcessWithoutNullStreams; + child.stdin = { + destroy: vi.fn(() => { + order.push("stdin.destroy"); + }), + } as ChildProcessWithoutNullStreams["stdin"]; + child.stdout = { + destroy: vi.fn(() => { + order.push("stdout.destroy"); + }), + } as ChildProcessWithoutNullStreams["stdout"]; + child.stderr = { + destroy: vi.fn(() => { + order.push("stderr.destroy"); + }), + } as ChildProcessWithoutNullStreams["stderr"]; + child.kill = vi.fn(() => true) as ChildProcessWithoutNullStreams["kill"]; + return child; +} + +function createDeferredTermination(): { promise: Promise<"terminated">; resolve: () => void } { + let resolveTermination: () => void = () => {}; + const promise = new Promise<"terminated">((resolve) => { + resolveTermination = () => resolve("terminated"); + }); + return { promise, resolve: resolveTermination }; +} + function selectConfigOption( category: "mode" | "model" | "thought_level", values: string[], @@ -1974,3 +2004,191 @@ describe("ACPAgentSession", () => { }); }); }); + +interface ACPCloseInternals { + child: ChildProcess | null; + closed: boolean; + connection: unknown; + sessionId: string | null; + terminalEntries: Map; + subscribers: Map; + activeForegroundTurnId: string | null; +} + +describe("ACPAgentSession close() tree-kill", () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + test("close() uses terminateWithTreeKill for the main child process", async () => { + const terminateWithTreeKill = vi + .spyOn(treeKillModule, "terminateWithTreeKill") + .mockResolvedValue("terminated"); + const session = createSession(); + const internals = asInternals(session); + + const child = createTerminalChildStub(); + internals.child = child; + internals.connection = null; + internals.sessionId = null; + + await session.close(); + + expect(terminateWithTreeKill).toHaveBeenCalledWith(child, { + gracefulTimeoutMs: 2_000, + forceTimeoutMs: 2_000, + }); + expect(child.kill).not.toHaveBeenCalled(); + }); + + test("close() uses terminateWithTreeKill for terminal child processes", async () => { + const terminateWithTreeKill = vi + .spyOn(treeKillModule, "terminateWithTreeKill") + .mockResolvedValue("terminated"); + const session = createSession(); + const internals = asInternals(session); + + const terminalChild = createTerminalChildStub(); + internals.terminalEntries = new Map([["terminal-1", { child: terminalChild, exit: null }]]); + internals.child = null; + internals.connection = null; + internals.sessionId = null; + + await session.close(); + + expect(terminateWithTreeKill).toHaveBeenCalledWith(terminalChild, { + gracefulTimeoutMs: 2_000, + forceTimeoutMs: 2_000, + }); + expect(terminalChild.kill).not.toHaveBeenCalled(); + }); + + test("close() terminates terminal child processes in parallel", async () => { + const resolvers: Array<() => void> = []; + const terminateWithTreeKill = vi + .spyOn(treeKillModule, "terminateWithTreeKill") + .mockImplementation(() => { + const termination = createDeferredTermination(); + resolvers.push(termination.resolve); + return termination.promise; + }); + const session = createSession(); + const internals = asInternals(session); + + const firstTerminalChild = createTerminalChildStub(); + const secondTerminalChild = createTerminalChildStub(); + internals.terminalEntries = new Map([ + ["terminal-1", { child: firstTerminalChild, exit: null }], + ["terminal-2", { child: secondTerminalChild, exit: null }], + ]); + internals.child = null; + internals.connection = null; + internals.sessionId = null; + + const close = session.close(); + await Promise.resolve(); + + expect(terminateWithTreeKill).toHaveBeenCalledTimes(2); + + for (const resolve of resolvers) { + resolve(); + } + await close; + }); + + test("killTerminal uses terminateWithTreeKill instead of direct SIGTERM", async () => { + const terminateWithTreeKill = vi + .spyOn(treeKillModule, "terminateWithTreeKill") + .mockResolvedValue("terminated"); + const session = createSession(); + const internals = asInternals(session); + + const child = createTerminalChildStub(); + internals.terminalEntries = new Map([["terminal-1", { child, exit: null }]]); + + await session.killTerminal({ sessionId: "session-1", terminalId: "terminal-1" }); + + expect(terminateWithTreeKill).toHaveBeenCalledWith(child, { + gracefulTimeoutMs: 2_000, + forceTimeoutMs: 2_000, + }); + expect(child.kill).not.toHaveBeenCalled(); + }); + + test("releaseTerminal uses terminateWithTreeKill before removing a running terminal", async () => { + const terminateWithTreeKill = vi + .spyOn(treeKillModule, "terminateWithTreeKill") + .mockResolvedValue("terminated"); + const child = createTerminalChildStub(); + vi.spyOn(spawnUtils, "spawnProcess").mockReturnValue(child); + const session = createSession(); + + const terminal = await session.createTerminal({ + sessionId: "session-1", + command: "sleep", + args: ["60"], + }); + + await session.releaseTerminal({ + sessionId: "session-1", + terminalId: terminal.terminalId, + }); + + expect(terminateWithTreeKill).toHaveBeenCalledWith(child, { + gracefulTimeoutMs: 2_000, + forceTimeoutMs: 2_000, + }); + expect(child.kill).not.toHaveBeenCalled(); + await expect( + session.terminalOutput({ sessionId: "session-1", terminalId: terminal.terminalId }), + ).rejects.toThrow(`Unknown terminal '${terminal.terminalId}'`); + }); +}); + +describe("ACPAgentClient probe cleanup", () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + test("signals the probe process tree before destroying stdio", async () => { + const order: string[] = []; + const terminateWithTreeKill = vi + .spyOn(treeKillModule, "terminateWithTreeKill") + .mockImplementation(async () => { + order.push("tree-kill"); + return "terminated"; + }); + const child = createProbeChildStub(order); + + class TestACPAgentClient extends ACPAgentClient { + protected override async spawnProcess(): Promise { + return { + child, + connection: { + newSession: vi.fn().mockResolvedValue({ + modes: null, + models: null, + configOptions: [], + }), + }, + initialize: { agentCapabilities: {} }, + } as SpawnedACPProcess; + } + } + + const client = new TestACPAgentClient({ + provider: "claude-acp", + logger: createTestLogger(), + defaultCommand: ["claude", "--acp"], + defaultModes: [], + }); + + await client.listModels({ cwd: "/tmp/acp-models", force: false }); + + expect(terminateWithTreeKill).toHaveBeenCalledWith(child, { + gracefulTimeoutMs: 2_000, + forceTimeoutMs: 2_000, + }); + expect(order).toEqual(["tree-kill", "stdin.destroy", "stdout.destroy", "stderr.destroy"]); + }); +}); diff --git a/packages/server/src/server/agent/providers/acp-agent.ts b/packages/server/src/server/agent/providers/acp-agent.ts index e22281f5b..3c2ab9b28 100644 --- a/packages/server/src/server/agent/providers/acp-agent.ts +++ b/packages/server/src/server/agent/providers/acp-agent.ts @@ -3,6 +3,8 @@ import { randomUUID } from "node:crypto"; import fs from "node:fs/promises"; import path from "node:path"; import { Readable, Writable } from "node:stream"; + +import { terminateWithTreeKill } from "../../../utils/tree-kill.js"; import type { ReadableStream as NodeReadableStream, WritableStream as NodeWritableStream, @@ -1664,14 +1666,17 @@ export class ACPAgentSession implements AgentSession, ACPClient { } } - for (const terminal of this.terminalEntries.values()) { - terminal.child.kill("SIGTERM"); - } + const terminalTerminations = Array.from(this.terminalEntries.values(), (terminal) => + terminateWithTreeKill(terminal.child, { + gracefulTimeoutMs: 2_000, + forceTimeoutMs: 2_000, + }), + ); + await Promise.all(terminalTerminations); this.terminalEntries.clear(); if (this.child) { - this.child.kill("SIGTERM"); - await waitForChildExit(this.child, 2_000); + await terminateWithTreeKill(this.child, { gracefulTimeoutMs: 2_000, forceTimeoutMs: 2_000 }); } this.subscribers.clear(); @@ -1853,7 +1858,7 @@ export class ACPAgentSession implements AgentSession, ACPClient { async releaseTerminal(params: { sessionId: string; terminalId: string }): Promise { const entry = this.getTerminalEntry(params.terminalId); if (!entry.exit) { - entry.child.kill("SIGTERM"); + await terminateWithTreeKill(entry.child, { gracefulTimeoutMs: 2_000, forceTimeoutMs: 2_000 }); } this.terminalEntries.delete(params.terminalId); } @@ -1861,7 +1866,7 @@ export class ACPAgentSession implements AgentSession, ACPClient { async killTerminal(params: KillTerminalRequest): Promise> { const entry = this.getTerminalEntry(params.terminalId); if (!entry.exit) { - entry.child.kill("SIGTERM"); + await terminateWithTreeKill(entry.child, { gracefulTimeoutMs: 2_000, forceTimeoutMs: 2_000 }); } return {}; } @@ -2884,29 +2889,15 @@ function coerceSessionConfigMetadata( return metadata as Partial; } -async function waitForChildExit( - child: ChildProcessWithoutNullStreams, - timeoutMs: number, -): Promise { - if (child.exitCode !== null || child.signalCode !== null) { - return; - } - await Promise.race([ - new Promise((resolve) => child.once("exit", () => resolve())), - new Promise((resolve) => setTimeout(resolve, timeoutMs)), - ]); - if (child.exitCode === null && child.signalCode === null) { - child.kill("SIGKILL"); - } -} - async function terminateChildProcess( child: ChildProcessWithoutNullStreams, timeoutMs: number, ): Promise { - child.kill("SIGTERM"); - child.stdin.destroy(); - child.stdout.destroy(); - child.stderr.destroy(); - await waitForChildExit(child, timeoutMs); + try { + await terminateWithTreeKill(child, { gracefulTimeoutMs: timeoutMs, forceTimeoutMs: timeoutMs }); + } finally { + child.stdin.destroy(); + child.stdout.destroy(); + child.stderr.destroy(); + } }