Use terminateWithTreeKill for ACP agent child process cleanup (#1460)

* 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 <noreply@anthropic.com> * fix(server): address acp tree kill review * test(server): cover acp release terminal tree kill --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
2026-06-14 23:55:27 +08:00
parent 0d76654ba0
commit a6ec352f07
2 changed files with 238 additions and 29 deletions
@@ -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<string, { child: ChildProcess; exit: unknown }>;
+  subscribers: Map<unknown, unknown>;
+  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<ACPCloseInternals>(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<ACPCloseInternals>(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<ACPCloseInternals>(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<ACPCloseInternals>(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<SpawnedACPProcess> {
+        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"]);
+  });
+});
@@ -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<void> {
    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<Record<string, never>> {
    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<AgentSessionConfig>;
 }

-async function waitForChildExit(
-  child: ChildProcessWithoutNullStreams,
-  timeoutMs: number,
-): Promise<void> {
-  if (child.exitCode !== null || child.signalCode !== null) {
-    return;
-  }
-  await Promise.race([
-    new Promise<void>((resolve) => child.once("exit", () => resolve())),
-    new Promise<void>((resolve) => setTimeout(resolve, timeoutMs)),
-  ]);
-  if (child.exitCode === null && child.signalCode === null) {
-    child.kill("SIGKILL");
-  }
-}
-
 async function terminateChildProcess(
  child: ChildProcessWithoutNullStreams,
  timeoutMs: number,
 ): Promise<void> {
-  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();
+  }
 }