From c75dd296e64f518288a0f4bde49b6daeeacba1a2 Mon Sep 17 00:00:00 2001 From: ojowwalker77 Date: Fri, 24 Jul 2026 13:34:31 -0300 Subject: [PATCH] Terminate MCP child process trees on codex session close MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closing/cancelling/replacing a codex session (or graceful app shutdown) previously sent a bare SIGTERM to only the codex app-server process on POSIX — no tree/group kill, no forced escalation. Any MCP server the agent CLI spawned could orphan and accumulate, degrading low-RAM Macs. - Add childProcessTreeTerminator: capture the descendant tree, graceful SIGTERM to the process group + tree + handle, then force-SIGKILL survivors after a bounded grace window (command-verified against PID reuse). Reuses the tested terminal/processTreeKiller primitive; emits structured lifecycle logs. - Spawn codex app-server detached on POSIX so it leads its own process group and teardown can signal codex + MCP children as a unit. - Route codex session/discovery teardown through the shared terminator with session-scoped lifecycle logs. Deferred (need cross-restart infra): startup orphan reaping, ref-counted shared MCP services, packaged-app verification. Refs #21 --- .../src/childProcessTreeTerminator.test.ts | 206 ++++++++++++++++ apps/server/src/childProcessTreeTerminator.ts | 221 ++++++++++++++++++ apps/server/src/codexAppServerManager.ts | 55 +++-- 3 files changed, 466 insertions(+), 16 deletions(-) create mode 100644 apps/server/src/childProcessTreeTerminator.test.ts create mode 100644 apps/server/src/childProcessTreeTerminator.ts diff --git a/apps/server/src/childProcessTreeTerminator.test.ts b/apps/server/src/childProcessTreeTerminator.test.ts new file mode 100644 index 00000000..c6a8dea8 --- /dev/null +++ b/apps/server/src/childProcessTreeTerminator.test.ts @@ -0,0 +1,206 @@ +// FILE: childProcessTreeTerminator.test.ts +// Purpose: Verifies graceful->forced teardown of an agent child and its tree. +// Layer: Server process-lifecycle tests +// Depends on: Vitest, injectable terminator dependencies, and a real child tree. +import { spawn } from "node:child_process"; + +import { describe, expect, it } from "vitest"; + +import { + terminateChildProcessTree, + type ChildTreeTerminationEvent, + type TerminableChildProcess, +} from "./childProcessTreeTerminator.ts"; +import type { + CapturedProcess, + ProcessTreeKiller, + TerminalKillSignal, +} from "./terminal/processTreeKiller.ts"; + +interface FakeChild extends TerminableChildProcess { + readonly kills: Array; + emitExit(): void; +} + +function fakeChild(pid: number | undefined): FakeChild { + const exitListeners: Array<() => void> = []; + const kills: Array = []; + return { + pid, + killed: false, + kills, + kill(signal) { + kills.push(signal); + return true; + }, + once(_event, listener) { + exitListeners.push(listener); + return this; + }, + emitExit() { + for (const listener of exitListeners) listener(); + }, + }; +} + +function isProcessAlive(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch (error) { + // EPERM means the process exists but we may not signal it — still alive. + if ((error as NodeJS.ErrnoException).code === "EPERM") return true; + return false; + } +} + +function recordingKiller(descendants: CapturedProcess[]) { + const signals: Array<{ rootPid: number; signal: TerminalKillSignal; includeRootTree: boolean }> = + []; + const killer: ProcessTreeKiller = { + capture: () => ({ descendants }), + signal: ({ rootPid, signal, includeRootTree = true }) => { + signals.push({ rootPid, signal, includeRootTree }); + }, + }; + return { killer, signals }; +} + +function makeHarness(input: { + pid: number | undefined; + descendants?: CapturedProcess[]; + platform?: NodeJS.Platform; +}) { + const child = fakeChild(input.pid); + const { killer, signals } = recordingKiller(input.descendants ?? []); + const groupSignals: Array<{ pgid: number; signal: TerminalKillSignal }> = []; + const windowsKills: number[] = []; + const events: ChildTreeTerminationEvent[] = []; + let escalate: (() => void) | null = null; + let cancelled = false; + + terminateChildProcessTree(child, { + graceMs: 500, + platform: input.platform ?? "linux", + killer, + signalProcessGroup: (pgid, signal) => { + groupSignals.push({ pgid, signal }); + return null; + }, + runWindowsTreeKill: (pid) => windowsKills.push(pid), + scheduleEscalation: (fn) => { + escalate = fn; + return { + cancel: () => { + cancelled = true; + }, + }; + }, + log: (event) => events.push(event), + }); + + return { + child, + signals, + groupSignals, + windowsKills, + events, + runEscalation: () => escalate?.(), + wasCancelled: () => cancelled, + }; +} + +describe("terminateChildProcessTree", () => { + it("sends graceful SIGTERM to the process group, tree, and handle", () => { + const h = makeHarness({ pid: 1000, descendants: [{ pid: 2000, command: "mcp-server" }] }); + + expect(h.groupSignals).toEqual([{ pgid: 1000, signal: "SIGTERM" }]); + expect(h.signals).toEqual([{ rootPid: 1000, signal: "SIGTERM", includeRootTree: true }]); + expect(h.child.kills).toEqual(["SIGTERM"]); + const terminate = h.events.find((event) => event.phase === "terminate"); + expect(terminate).toMatchObject({ signal: "SIGTERM", pid: 1000, descendantPids: [2000] }); + }); + + it("escalates to SIGKILL of the full tree when the root ignores SIGTERM", () => { + const h = makeHarness({ pid: 1000, descendants: [{ pid: 2000, command: "mcp-server" }] }); + + h.runEscalation(); + + expect(h.groupSignals).toContainEqual({ pgid: 1000, signal: "SIGKILL" }); + expect(h.signals).toContainEqual({ rootPid: 1000, signal: "SIGKILL", includeRootTree: true }); + expect(h.child.kills).toEqual(["SIGTERM", "SIGKILL"]); + expect(h.events.some((event) => event.phase === "escalate")).toBe(true); + }); + + it("after root exit, escalation reaps only captured descendants (never the reused root pid)", () => { + const h = makeHarness({ pid: 1000, descendants: [{ pid: 2000, command: "mcp-server" }] }); + + h.child.emitExit(); + expect(h.wasCancelled()).toBe(false); + + h.runEscalation(); + + // No SIGKILL to the root pid/group — its pid may have been reused. + expect(h.groupSignals).toEqual([{ pgid: 1000, signal: "SIGTERM" }]); + expect(h.child.kills).toEqual(["SIGTERM"]); + // Captured descendants are still swept (killer verifies commands internally). + expect(h.signals).toContainEqual({ rootPid: 1000, signal: "SIGKILL", includeRootTree: false }); + }); + + it("cancels escalation when the root exits leaving no descendants", () => { + const h = makeHarness({ pid: 1000, descendants: [] }); + + h.child.emitExit(); + + expect(h.wasCancelled()).toBe(true); + }); + + it("skips a child with no pid", () => { + const h = makeHarness({ pid: undefined }); + + expect(h.groupSignals).toEqual([]); + expect(h.signals).toEqual([]); + expect(h.child.kills).toEqual([]); + expect(h.events).toEqual([expect.objectContaining({ phase: "skip", detail: "no-pid" })]); + }); + + it("uses taskkill on Windows and skips POSIX signaling", () => { + const h = makeHarness({ pid: 42, platform: "win32" }); + + expect(h.windowsKills).toEqual([42]); + expect(h.groupSignals).toEqual([]); + expect(h.signals).toEqual([]); + expect(h.child.kills).toEqual([]); + expect(h.events).toEqual([expect.objectContaining({ phase: "terminate", signal: "taskkill" })]); + }); + + it.skipIf(process.platform === "win32")( + "terminates a real detached child process tree", + async () => { + // sh (group leader) forks a long sleep grandchild and prints its pid. + const child = spawn("sh", ["-c", "sleep 30 & echo $!; wait"], { + detached: true, + stdio: ["ignore", "pipe", "ignore"], + }); + + const grandchildPid = await new Promise((resolve, reject) => { + child.stdout.once("data", (chunk: Buffer) => { + const pid = Number.parseInt(chunk.toString().trim(), 10); + if (Number.isInteger(pid) && pid > 0) resolve(pid); + else reject(new Error(`unexpected grandchild pid output: ${chunk.toString()}`)); + }); + child.once("error", reject); + }); + + expect(isProcessAlive(child.pid ?? -1)).toBe(true); + expect(isProcessAlive(grandchildPid)).toBe(true); + + terminateChildProcessTree(child, { graceMs: 150 }); + + await new Promise((resolve) => setTimeout(resolve, 600)); + + expect(isProcessAlive(child.pid ?? -1)).toBe(false); + expect(isProcessAlive(grandchildPid)).toBe(false); + }, + ); +}); diff --git a/apps/server/src/childProcessTreeTerminator.ts b/apps/server/src/childProcessTreeTerminator.ts new file mode 100644 index 00000000..6e6ce2e6 --- /dev/null +++ b/apps/server/src/childProcessTreeTerminator.ts @@ -0,0 +1,221 @@ +// FILE: childProcessTreeTerminator.ts +// Purpose: Terminates a spawned agent child AND its descendant process tree +// (e.g. MCP servers launched by the agent CLI) so nothing outlives the +// session that owns it. Graceful SIGTERM first, forced SIGKILL after a +// bounded grace window. +// Layer: Server process-lifecycle utility +// Depends on: node child_process signals + the shared process-tree killer. +import { spawnSync } from "node:child_process"; + +import { + defaultProcessTreeKiller, + type ProcessTreeKiller, + type TerminalKillSignal, +} from "./terminal/processTreeKiller.ts"; + +export const DEFAULT_CHILD_TREE_TERMINATION_GRACE_MS = 2_000; + +// The minimal surface we need from a Node ChildProcess. Keeping it structural +// lets callers pass real children and tests pass lightweight doubles. +export interface TerminableChildProcess { + readonly pid?: number | undefined; + readonly killed?: boolean; + kill(signal?: NodeJS.Signals | number): boolean; + once(event: "exit", listener: () => void): unknown; +} + +export interface ChildTreeTerminationEvent { + readonly phase: "terminate" | "escalate" | "skip" | "error"; + readonly signal: string; + readonly pid: number | undefined; + // On POSIX the child is spawned detached, so its process-group id equals its + // pid at spawn time; we surface it so operators can correlate group kills. + readonly pgid: number | undefined; + readonly descendantPids: readonly number[]; + readonly graceMs: number; + readonly detail?: string; +} + +export interface ChildTreeTerminatorOptions { + readonly graceMs?: number; + readonly log?: (event: ChildTreeTerminationEvent) => void; + readonly killer?: ProcessTreeKiller; + readonly platform?: NodeJS.Platform; + readonly signalProcessGroup?: (pgid: number, signal: TerminalKillSignal) => Error | null; + readonly runWindowsTreeKill?: (pid: number) => void; + readonly scheduleEscalation?: (fn: () => void, ms: number) => { cancel: () => void }; +} + +function signalProcessGroupDefault(pgid: number, signal: TerminalKillSignal): Error | null { + try { + // Negative pid targets the whole process group. Because agent children are + // spawned detached (their own group leader), this reaches every descendant + // still in the group in a single call — including any spawned during the + // window between capture and signal. + globalThis.process.kill(-pgid, signal); + return null; + } catch (error) { + const errno = error as NodeJS.ErrnoException; + // Group already gone — success. EPERM means we do not own the group (child + // was not detached); the captured-descendant sweep below still covers it. + if (errno?.code === "ESRCH" || errno?.code === "EPERM") { + return null; + } + return error instanceof Error ? error : new Error(String(error)); + } +} + +function runWindowsTreeKillDefault(pid: number): void { + try { + // `.cmd` shims run under a cmd.exe wrapper; taskkill /T /F tears down the + // whole tree so cancellation never leaves the real provider process behind. + spawnSync("taskkill", ["/pid", String(pid), "/T", "/F"], { stdio: "ignore" }); + } catch { + // Best effort — nothing else we can do if taskkill itself fails to launch. + } +} + +function scheduleEscalationDefault(fn: () => void, ms: number): { cancel: () => void } { + const timer = setTimeout(fn, ms); + // Never keep the event loop (or a shutting-down server) alive for the timer. + timer.unref?.(); + return { cancel: () => clearTimeout(timer) }; +} + +// Terminates `child` and its descendant process tree. Returns immediately after +// the graceful signal; the forced SIGKILL escalation runs asynchronously if the +// tree has not exited within the grace window. Safe to describe as fire-and- +// forget: the escalation timer is unref'd and idempotent. +export function terminateChildProcessTree( + child: TerminableChildProcess, + options: ChildTreeTerminatorOptions = {}, +): void { + const platform = options.platform ?? globalThis.process.platform; + const graceMs = options.graceMs ?? DEFAULT_CHILD_TREE_TERMINATION_GRACE_MS; + const log = options.log; + const pid = child.pid; + + if (platform === "win32") { + if (pid === undefined) { + log?.({ + phase: "skip", + signal: "none", + pid, + pgid: undefined, + descendantPids: [], + graceMs: 0, + detail: "no-pid", + }); + return; + } + (options.runWindowsTreeKill ?? runWindowsTreeKillDefault)(pid); + log?.({ + phase: "terminate", + signal: "taskkill", + pid, + pgid: undefined, + descendantPids: [], + graceMs: 0, + }); + return; + } + + if (pid === undefined) { + log?.({ + phase: "skip", + signal: "none", + pid, + pgid: undefined, + descendantPids: [], + graceMs, + detail: "no-pid", + }); + return; + } + + const killer = options.killer ?? defaultProcessTreeKiller; + const signalProcessGroup = options.signalProcessGroup ?? signalProcessGroupDefault; + + // Capture descendants BEFORE signaling: once the root dies its grandchildren + // may be reparented to init and become invisible to a ppid walk. + const tree = killer.capture(pid); + const descendantPids = tree.descendants.map((descendant) => descendant.pid); + + const emitError = (detail: string, atPid: number | undefined) => { + log?.({ + phase: "error", + signal: "SIGTERM", + pid: atPid, + pgid: pid, + descendantPids, + graceMs, + detail, + }); + }; + + const signalGroup = (signal: TerminalKillSignal) => { + const error = signalProcessGroup(pid, signal); + if (error) { + emitError(`process-group ${signal}: ${error.message}`, pid); + } + }; + + // Graceful phase: group + tree + direct handle. Redundant on purpose so a + // single reparented or race-spawned descendant still receives the signal. + signalGroup("SIGTERM"); + killer.signal({ + rootPid: pid, + signal: "SIGTERM", + tree, + includeRootTree: true, + onError: (error, context) => + emitError(`${context.source} SIGTERM: ${error.message}`, context.pid), + }); + try { + child.kill("SIGTERM"); + } catch { + // Handle already reaped — group/tree signals above still applied. + } + log?.({ phase: "terminate", signal: "SIGTERM", pid, pgid: pid, descendantPids, graceMs }); + + const schedule = options.scheduleEscalation ?? scheduleEscalationDefault; + let rootExited = false; + let escalated = false; + + const escalation = schedule(() => { + if (escalated) return; + escalated = true; + // Force phase: SIGKILL survivors. The killer re-verifies each captured pid's + // command before SIGKILL to avoid killing a reused pid. Once the root has + // exited we skip its pid/group (pgid may be reused) and only reap the + // command-verified captured descendants. + if (!rootExited) { + signalGroup("SIGKILL"); + } + killer.signal({ + rootPid: pid, + signal: "SIGKILL", + tree, + includeRootTree: !rootExited, + onError: (error, context) => + emitError(`${context.source} SIGKILL: ${error.message}`, context.pid), + }); + if (!rootExited) { + try { + child.kill("SIGKILL"); + } catch { + // Already gone. + } + } + log?.({ phase: "escalate", signal: "SIGKILL", pid, pgid: pid, descendantPids, graceMs }); + }, graceMs); + + child.once("exit", () => { + rootExited = true; + // If the root leaves no descendants behind, there is nothing to escalate + // against — cancel the timer and avoid a needless post-exit process scan. + if (descendantPids.length === 0) { + escalation.cancel(); + } + }); +} diff --git a/apps/server/src/codexAppServerManager.ts b/apps/server/src/codexAppServerManager.ts index 488fb6a3..e93d8a6b 100644 --- a/apps/server/src/codexAppServerManager.ts +++ b/apps/server/src/codexAppServerManager.ts @@ -44,6 +44,7 @@ import { isCodexCliVersionSupported, parseCodexCliVersion, } from "./provider/codexCliVersion"; +import { terminateChildProcessTree } from "./childProcessTreeTerminator.ts"; import { isNonFatalCodexErrorMessage } from "./codexErrorClassification.ts"; import { buildCodexProcessEnv } from "./codexProcessEnv.ts"; import { ensureIsolatedScratchWorkspace } from "./scratchWorkspaces.ts"; @@ -428,20 +429,34 @@ export function resolveCodexModelForAccount( return CODEX_DEFAULT_MODEL; } -// Windows `.cmd` shims still run under an explicit cmd.exe wrapper; taskkill -// keeps cancellation from leaving the real provider process behind. -function killChildTree(child: ChildProcessWithoutNullStreams): void { - if (process.platform === "win32" && child.pid !== undefined) { - try { - spawnSync("taskkill", ["/pid", String(child.pid), "/T", "/F"], { - stdio: "ignore", - }); - return; - } catch { - // fallback to direct kill - } - } - child.kill(); +// Tears down the codex app-server AND every process it spawned (MCP servers, +// shells) so a closed session leaves nothing behind. Delegates to the shared +// tree terminator: graceful SIGTERM, then forced SIGKILL of survivors, with +// structured lifecycle logs. See childProcessTreeTerminator.ts. +function killChildTree( + child: ChildProcessWithoutNullStreams, + logContext: { readonly threadId?: string; readonly kind: "session" | "discovery" }, +): void { + terminateChildProcessTree(child, { + log: (event) => { + const context = { + threadId: logContext.threadId, + kind: logContext.kind, + pid: event.pid, + pgid: event.pgid, + descendants: event.descendantPids.length, + descendantPids: event.descendantPids.length > 0 ? event.descendantPids : undefined, + signal: event.signal, + graceMs: event.graceMs, + detail: event.detail, + }; + if (event.phase === "escalate" || event.phase === "error") { + log.warn("codex app-server force-terminated child tree", context); + } else if (event.phase === "terminate") { + log.info("codex app-server terminating child tree", context); + } + }, + }); } function spawnCodexAppServer(input: { @@ -459,6 +474,11 @@ function spawnCodexAppServer(input: { stdio: ["pipe", "pipe", "pipe"], shell: prepared.shell, windowsHide: prepared.windowsHide, + // POSIX: give the app-server its own process group so session teardown can + // signal the whole group (codex + any MCP children) at once. We never + // unref the child — its lifecycle stays owned by this manager. Windows uses + // taskkill /T for tree teardown instead. + detached: process.platform !== "win32", }); } @@ -1615,7 +1635,7 @@ export class CodexAppServerManager extends EventEmitter