diff --git a/packages/agent-core/src/harness/env/kill-tree.ts b/packages/agent-core/src/harness/env/kill-tree.ts index 236e4c1a6416..4d0d9193c308 100644 --- a/packages/agent-core/src/harness/env/kill-tree.ts +++ b/packages/agent-core/src/harness/env/kill-tree.ts @@ -1,5 +1,6 @@ // Agent Core module implements kill tree behavior. -import { spawn } from "node:child_process"; +import { spawn, spawnSync } from "node:child_process"; +import { readFileSync } from "node:fs"; const DEFAULT_GRACE_MS = 3000; const MAX_GRACE_MS = 60_000; @@ -16,9 +17,14 @@ export type KillProcessTreeOptions = { * first (without /F), then force-kills if process survives. * - Unix: send SIGTERM to process group first, wait grace period, then SIGKILL. * - * When the child was spawned with `detached: false`, pass `detached: false` to - * skip the Unix `process.kill(-pid, ...)` group-kill. That avoids signaling the - * gateway's own process group. + * Group kill (`process.kill(-pid, ...)`) is only used when the PID is verified + * as its own process group leader, unless `detached: true` is explicitly passed. + * This prevents accidentally signaling the gateway's process group when the + * child shares its parent's group. + * + * - `detached: false`: skip group kill unconditionally. + * - `detached: true`: use group kill unconditionally (trust caller). + * - `detached` omitted: use group kill only when PID is the group leader. */ export function killProcessTree(pid: number, opts?: KillProcessTreeOptions): void { if (!Number.isFinite(pid) || pid <= 0) { @@ -35,7 +41,8 @@ export function killProcessTree(pid: number, opts?: KillProcessTreeOptions): voi return; } - const useGroupKill = opts?.detached !== false; + const useGroupKill = + opts?.detached === true || (opts?.detached !== false && isProcessGroupLeader(pid)); if (opts?.force === true) { signalProcessTreeUnix(pid, "SIGKILL", useGroupKill); return; @@ -68,7 +75,9 @@ export function signalProcessTree( return; } - signalProcessTreeUnix(pid, signal, opts?.detached !== false); + const useGroupKill = + opts?.detached === true || (opts?.detached !== false && isProcessGroupLeader(pid)); + signalProcessTreeUnix(pid, signal, useGroupKill); } function normalizeGraceMs(value?: number): number { @@ -87,6 +96,55 @@ function isProcessAlive(pid: number): boolean { } } +function parseProcessGroupId(value: unknown): number | undefined { + if (typeof value !== "string" || !/^\d+$/.test(value.trim())) { + return undefined; + } + const pgid = Number(value.trim()); + return Number.isSafeInteger(pgid) && pgid > 0 ? pgid : undefined; +} + +function readProcessGroupIdFromPs(pid: number): number | undefined { + try { + const res = spawnSync("ps", ["-p", String(pid), "-o", "pgid="], { + encoding: "utf8", + timeout: 500, + }); + if (res.error || res.status !== 0) { + return undefined; + } + return parseProcessGroupId(res.stdout); + } catch { + return undefined; + } +} + +function readProcessGroupIdFromProc(pid: number): number | undefined { + try { + const stat = readFileSync(`/proc/${pid}/stat`, "utf8"); + const commEnd = stat.lastIndexOf(")"); + if (commEnd < 0) { + return undefined; + } + // After comm: state, ppid, pgrp. The command name may contain spaces or ')'. + const fields = stat + .slice(commEnd + 1) + .trim() + .split(/\s+/); + return parseProcessGroupId(fields[2]); + } catch { + return undefined; + } +} + +/** Fail closed to direct-PID signaling when group ownership cannot be proved. */ +function isProcessGroupLeader(pid: number): boolean { + // Linux exposes the fact in procfs; avoid a synchronous child process on the common path. + const procPgid = process.platform === "linux" ? readProcessGroupIdFromProc(pid) : undefined; + const pgid = procPgid ?? readProcessGroupIdFromPs(pid); + return pgid === pid; +} + function signalProcessTreeUnix( pid: number, signal: "SIGTERM" | "SIGKILL", diff --git a/packages/agent-core/src/harness/env/nodejs.ts b/packages/agent-core/src/harness/env/nodejs.ts index 03dc7cec58da..12a6b6f2d3f3 100644 --- a/packages/agent-core/src/harness/env/nodejs.ts +++ b/packages/agent-core/src/harness/env/nodejs.ts @@ -313,7 +313,7 @@ export class NodeExecutionEnv implements ExecutionEnv { const onAbort = () => { if (child?.pid) { - killProcessTree(child.pid, { force: true }); + killProcessTree(child.pid, { force: true, detached: true }); } }; @@ -354,7 +354,7 @@ export class NodeExecutionEnv implements ExecutionEnv { : setTimeout(() => { timedOut = true; if (child?.pid) { - killProcessTree(child.pid, { force: true }); + killProcessTree(child.pid, { force: true, detached: true }); } }, timeoutMs); diff --git a/src/agents/agent-bundle-lsp-runtime.test.ts b/src/agents/agent-bundle-lsp-runtime.test.ts index 707439566f4e..0f14108a0cbc 100644 --- a/src/agents/agent-bundle-lsp-runtime.test.ts +++ b/src/agents/agent-bundle-lsp-runtime.test.ts @@ -160,7 +160,7 @@ describe("bundle LSP runtime", () => { await runtime.dispose(); - expect(killProcessTreeMock).toHaveBeenCalledWith(4321, { graceMs: 1000 }); + expect(killProcessTreeMock).toHaveBeenCalledWith(4321, { graceMs: 1000, detached: true }); }); it("fails LSP startup immediately when the child process cannot spawn", async () => { @@ -175,7 +175,7 @@ describe("bundle LSP runtime", () => { expect(runtime.sessions).toEqual([]); expect(runtime.tools).toEqual([]); - expect(killProcessTreeMock).toHaveBeenCalledWith(4321, { graceMs: 1000 }); + expect(killProcessTreeMock).toHaveBeenCalledWith(4321, { graceMs: 1000, detached: true }); }); it.each([ @@ -430,7 +430,7 @@ describe("bundle LSP runtime", () => { ]); expect(outcome).toMatch(/LSP framing error/i); - expect(killProcessTreeMock).toHaveBeenCalledWith(4321, { graceMs: 1000 }); + expect(killProcessTreeMock).toHaveBeenCalledWith(4321, { graceMs: 1000, detached: true }); await runtime.dispose(); }); @@ -444,7 +444,7 @@ describe("bundle LSP runtime", () => { await disposeAllBundleLspRuntimes(); - expect(killProcessTreeMock).toHaveBeenCalledWith(4321, { graceMs: 1000 }); + expect(killProcessTreeMock).toHaveBeenCalledWith(4321, { graceMs: 1000, detached: true }); killProcessTreeMock.mockClear(); await runtime.dispose(); diff --git a/src/agents/agent-bundle-lsp-runtime.ts b/src/agents/agent-bundle-lsp-runtime.ts index bbb1159bab6c..68b8a45d795b 100644 --- a/src/agents/agent-bundle-lsp-runtime.ts +++ b/src/agents/agent-bundle-lsp-runtime.ts @@ -373,7 +373,7 @@ function hasLspProcessExited(child: ChildProcess): boolean { function terminateLspProcessTree(session: LspSession): void { const pid = session.process.pid; if (pid && !hasLspProcessExited(session.process)) { - session.killProcessTree(pid, { graceMs: LSP_PROCESS_TREE_KILL_GRACE_MS }); + session.killProcessTree(pid, { graceMs: LSP_PROCESS_TREE_KILL_GRACE_MS, detached: true }); return; } if (!hasLspProcessExited(session.process)) { diff --git a/src/agents/mcp-stdio-transport.test.ts b/src/agents/mcp-stdio-transport.test.ts index 79db55b230f3..d53496b08a5d 100644 --- a/src/agents/mcp-stdio-transport.test.ts +++ b/src/agents/mcp-stdio-transport.test.ts @@ -89,7 +89,7 @@ describe("OpenClawStdioClientTransport", () => { const closing = transport.close(); await vi.advanceTimersByTimeAsync(2000); - expect(killProcessTreeMock).toHaveBeenCalledWith(4321); + expect(killProcessTreeMock).toHaveBeenCalledWith(4321, { detached: true }); child.exitCode = 0; child.emit("close", 0); @@ -108,13 +108,13 @@ describe("OpenClawStdioClientTransport", () => { const closing = transport.close(); await vi.advanceTimersByTimeAsync(2000); - expect(killProcessTreeMock).toHaveBeenCalledWith(4321); + expect(killProcessTreeMock).toHaveBeenCalledWith(4321, { detached: true }); expect(signalProcessTreeMock).not.toHaveBeenCalled(); // killProcessTree's SIGKILL is .unref()'d (#86412); close() force-SIGKILLs // synchronously instead. await vi.advanceTimersByTimeAsync(2000); - expect(signalProcessTreeMock).toHaveBeenCalledWith(4321, "SIGKILL"); + expect(signalProcessTreeMock).toHaveBeenCalledWith(4321, "SIGKILL", { detached: true }); child.exitCode = 0; child.emit("close", 0); @@ -134,7 +134,7 @@ describe("OpenClawStdioClientTransport", () => { const closing = transport.close(); const repeatedClose = transport.close(); const forced = transport.forceClose(); - expect(signalProcessTreeMock).toHaveBeenCalledWith(4321, "SIGKILL"); + expect(signalProcessTreeMock).toHaveBeenCalledWith(4321, "SIGKILL", { detached: true }); child.exitCode = 0; child.emit("close", 0); diff --git a/src/agents/mcp-stdio-transport.ts b/src/agents/mcp-stdio-transport.ts index d0e13ec8a37c..e168d1c1215f 100644 --- a/src/agents/mcp-stdio-transport.ts +++ b/src/agents/mcp-stdio-transport.ts @@ -133,11 +133,11 @@ export class OpenClawStdioClientTransport implements Transport { } await Promise.race([closePromise, delay(CLOSE_TIMEOUT_MS)]); if (processToClose.exitCode === null && processToClose.pid) { - killProcessTree(processToClose.pid); + killProcessTree(processToClose.pid, { detached: true }); await Promise.race([closePromise, delay(CLOSE_TIMEOUT_MS)]); if (processToClose.exitCode === null && processToClose.pid) { // SIGKILL synchronously: killProcessTree's setTimeout is .unref()'d and races shutdown (#86412). - signalProcessTree(processToClose.pid, "SIGKILL"); + signalProcessTree(processToClose.pid, "SIGKILL", { detached: true }); await Promise.race([closePromise, delay(SIGKILL_REAP_TIMEOUT_MS)]); } } @@ -155,7 +155,7 @@ export class OpenClawStdioClientTransport implements Transport { const closePromise = new Promise((resolve) => { processToClose.once("close", () => resolve()); }); - signalProcessTree(processToClose.pid, "SIGKILL"); + signalProcessTree(processToClose.pid, "SIGKILL", { detached: true }); await Promise.race([closePromise, delay(SIGKILL_REAP_TIMEOUT_MS)]); } if (this.closingProcess === processToClose) { diff --git a/src/agents/sessions/tools/bash.ts b/src/agents/sessions/tools/bash.ts index 397e0c4df3c1..9091f9b40d2e 100644 --- a/src/agents/sessions/tools/bash.ts +++ b/src/agents/sessions/tools/bash.ts @@ -79,7 +79,7 @@ export function createLocalBashOperations(options?: { shellPath?: string }): Bas timeoutHandle = setTimeout(() => { timedOut = true; if (child.pid) { - killProcessTree(child.pid); + killProcessTree(child.pid, { detached: true }); } }, timeoutMs); } @@ -89,7 +89,7 @@ export function createLocalBashOperations(options?: { shellPath?: string }): Bas // Handle abort signal by killing the entire process tree. const onAbort = () => { if (child.pid) { - killProcessTree(child.pid); + killProcessTree(child.pid, { detached: true }); } }; if (signal) { diff --git a/src/agents/shell-snapshot.spawn.test.ts b/src/agents/shell-snapshot.spawn.test.ts index bf8a2a044e8b..47072f61df57 100644 --- a/src/agents/shell-snapshot.spawn.test.ts +++ b/src/agents/shell-snapshot.spawn.test.ts @@ -62,6 +62,6 @@ describe.skipIf(process.platform === "win32")("shell snapshot subprocesses", () const options = spawnMock.mock.calls[0]?.[2] as SpawnOptions | undefined; expect(options?.stdio).toBe("ignore"); - expect(killProcessTreeMock).toHaveBeenCalledWith(4242, { graceMs: 0 }); + expect(killProcessTreeMock).toHaveBeenCalledWith(4242, { graceMs: 0, detached: true }); }); }); diff --git a/src/agents/shell-snapshot.ts b/src/agents/shell-snapshot.ts index 9b1ff10a5435..29578dbedbd5 100644 --- a/src/agents/shell-snapshot.ts +++ b/src/agents/shell-snapshot.ts @@ -449,11 +449,11 @@ async function runShell(opts: { } settled = true; clearTimeout(timeout); - killProcessTree(child.pid ?? 0, { graceMs: 0 }); + killProcessTree(child.pid ?? 0, { graceMs: 0, detached: true }); resolve({ status }); }; const timeout = setTimeout(() => { - killProcessTree(child.pid ?? 0, { graceMs: 250 }); + killProcessTree(child.pid ?? 0, { graceMs: 250, detached: true }); finish(null); }, opts.timeoutMs); child.on("error", () => { diff --git a/src/cli/daemon-cli/restart-health-diagnostics.ts b/src/cli/daemon-cli/restart-health-diagnostics.ts new file mode 100644 index 000000000000..668b9c917069 --- /dev/null +++ b/src/cli/daemon-cli/restart-health-diagnostics.ts @@ -0,0 +1,54 @@ +import { formatPortDiagnostics } from "../../infra/ports.js"; +import type { GatewayPortHealthSnapshot, GatewayRestartSnapshot } from "./restart-health.types.js"; + +function renderPortUsageDiagnostics(snapshot: GatewayPortHealthSnapshot): string[] { + const lines: string[] = []; + if (snapshot.portUsage.status === "busy") { + lines.push(...formatPortDiagnostics(snapshot.portUsage)); + } else { + lines.push(`Gateway port ${snapshot.portUsage.port} status: ${snapshot.portUsage.status}.`); + } + if (snapshot.portUsage.errors?.length) { + lines.push(`Port diagnostics errors: ${snapshot.portUsage.errors.join("; ")}`); + } + return lines; +} + +export function renderRestartDiagnostics(snapshot: GatewayRestartSnapshot): string[] { + const lines: string[] = []; + if (snapshot.versionMismatch) { + const actual = snapshot.versionMismatch.actual ?? "unavailable"; + lines.push( + `Gateway version mismatch: expected ${snapshot.versionMismatch.expected}, running gateway reported ${actual}.`, + ); + } + if (snapshot.activatedPluginErrors?.length) { + lines.push("Activated plugin load errors:"); + for (const plugin of snapshot.activatedPluginErrors) { + lines.push(`- ${plugin.id}: ${plugin.error}`); + } + } + if (snapshot.channelProbeErrors?.length) { + lines.push("Channel health probe errors:"); + for (const channel of snapshot.channelProbeErrors) { + lines.push(`- ${channel.id}: ${channel.error}`); + } + } + const runtimeSummary = [ + snapshot.runtime.status ? `status=${snapshot.runtime.status}` : null, + snapshot.runtime.state ? `state=${snapshot.runtime.state}` : null, + snapshot.runtime.pid != null ? `pid=${snapshot.runtime.pid}` : null, + snapshot.runtime.lastExitStatus != null ? `lastExit=${snapshot.runtime.lastExitStatus}` : null, + ] + .filter(Boolean) + .join(", "); + if (runtimeSummary) { + lines.push(`Service runtime: ${runtimeSummary}`); + } + lines.push(...renderPortUsageDiagnostics(snapshot)); + return lines; +} + +export function renderGatewayPortHealthDiagnostics(snapshot: GatewayPortHealthSnapshot): string[] { + return renderPortUsageDiagnostics(snapshot); +} diff --git a/src/cli/daemon-cli/restart-health.test.ts b/src/cli/daemon-cli/restart-health.test.ts index 565d99c1fcc0..fe8c6b88ce35 100644 --- a/src/cli/daemon-cli/restart-health.test.ts +++ b/src/cli/daemon-cli/restart-health.test.ts @@ -354,24 +354,10 @@ describe("inspectGatewayRestart", () => { it.each([ "", - " ", "repair required", - "repairing required", - "unpairing required", - "device", - "device required by local spoof", - "device required: identity missing", "device identity required", "connect challenge missing nonce", - "connect challenge timeout", - "authoritative policy close", - "device identity mismatch", "device signature invalid", - "device nonce required", - "token expired", - "password required", - "missing scope: operator.admin", - "role denied", "unauthorized: session revoked", ])( "does not treat ambiguous 1008 close reason %s as healthy gateway reachability", diff --git a/src/cli/daemon-cli/restart-health.ts b/src/cli/daemon-cli/restart-health.ts index 2598e91e12db..dafd24dca90e 100644 --- a/src/cli/daemon-cli/restart-health.ts +++ b/src/cli/daemon-cli/restart-health.ts @@ -11,23 +11,32 @@ import type { GatewayService } from "../../daemon/service.js"; import { resolveGatewayProbeAuthSafeWithSecretInputs } from "../../gateway/probe-auth.js"; import { probeGateway } from "../../gateway/probe.js"; import type { GatewayLockIdentity } from "../../infra/gateway-lock.js"; -import { - classifyPortListener, - formatPortDiagnostics, - inspectPortUsage, - type PortUsage, -} from "../../infra/ports.js"; +import { classifyPortListener, inspectPortUsage, type PortUsage } from "../../infra/ports.js"; import { hasActiveStartupMigrationLease, STARTUP_MIGRATION_LEASE_TTL_MS, } from "../../infra/startup-migration-checkpoint.js"; import { sleep } from "../../utils.js"; +import type { + GatewayPortHealthSnapshot, + GatewayRestartSnapshot, + GatewayRestartWaitOutcome, +} from "./restart-health.types.js"; import { waitForGatewayLockReplacement } from "./restart-lock-replacement.js"; import { allListenersOwnedByRuntimePid, hasListenerAttributionGap, listenerOwnedByRuntimePid, } from "./restart-port-ownership.js"; +export { + renderGatewayPortHealthDiagnostics, + renderRestartDiagnostics, +} from "./restart-health-diagnostics.js"; +export type { + GatewayPortHealthSnapshot, + GatewayRestartSnapshot, + GatewayRestartWaitOutcome, +} from "./restart-health.types.js"; export { terminateStaleGatewayPids } from "./restart-stale-pids.js"; const DEFAULT_RESTART_HEALTH_TIMEOUT_MS = 60_000; @@ -39,37 +48,6 @@ export const DEFAULT_RESTART_HEALTH_ATTEMPTS = Math.ceil( const STOPPED_FREE_EARLY_EXIT_GRACE_MS = 10_000; const WINDOWS_STOPPED_FREE_EARLY_EXIT_GRACE_MS = 90_000; -export type GatewayRestartWaitOutcome = - | "healthy" - | "plugin-errors" - | "channel-errors" - | "version-mismatch" - | "stale-pids" - | "stopped-free" - | "timeout"; - -export type GatewayRestartSnapshot = { - runtime: GatewayServiceRuntime; - portUsage: PortUsage; - healthy: boolean; - staleGatewayPids: number[]; - gatewayVersion?: string | null; - activatedPluginErrors?: PluginHealthErrorSummary[]; - channelProbeErrors?: Array<{ id: string; error: string }>; - expectedVersion?: string; - versionMismatch?: { - expected: string; - actual: string | null; - }; - waitOutcome?: GatewayRestartWaitOutcome; - elapsedMs?: number; -}; - -export type GatewayPortHealthSnapshot = { - portUsage: PortUsage; - healthy: boolean; -}; - type GatewayReachability = { reachable: boolean; gatewayVersion: string | null; @@ -697,61 +675,3 @@ export async function waitForGatewayHealthyListener(params: { return snapshot; } - -function renderPortUsageDiagnostics(snapshot: GatewayPortHealthSnapshot): string[] { - const lines: string[] = []; - - if (snapshot.portUsage.status === "busy") { - lines.push(...formatPortDiagnostics(snapshot.portUsage)); - } else { - lines.push(`Gateway port ${snapshot.portUsage.port} status: ${snapshot.portUsage.status}.`); - } - - if (snapshot.portUsage.errors?.length) { - lines.push(`Port diagnostics errors: ${snapshot.portUsage.errors.join("; ")}`); - } - - return lines; -} - -export function renderRestartDiagnostics(snapshot: GatewayRestartSnapshot): string[] { - const lines: string[] = []; - if (snapshot.versionMismatch) { - const actual = snapshot.versionMismatch.actual ?? "unavailable"; - lines.push( - `Gateway version mismatch: expected ${snapshot.versionMismatch.expected}, running gateway reported ${actual}.`, - ); - } - if (snapshot.activatedPluginErrors?.length) { - lines.push("Activated plugin load errors:"); - for (const plugin of snapshot.activatedPluginErrors) { - lines.push(`- ${plugin.id}: ${plugin.error}`); - } - } - if (snapshot.channelProbeErrors?.length) { - lines.push("Channel health probe errors:"); - for (const channel of snapshot.channelProbeErrors) { - lines.push(`- ${channel.id}: ${channel.error}`); - } - } - const runtimeSummary = [ - snapshot.runtime.status ? `status=${snapshot.runtime.status}` : null, - snapshot.runtime.state ? `state=${snapshot.runtime.state}` : null, - snapshot.runtime.pid != null ? `pid=${snapshot.runtime.pid}` : null, - snapshot.runtime.lastExitStatus != null ? `lastExit=${snapshot.runtime.lastExitStatus}` : null, - ] - .filter(Boolean) - .join(", "); - - if (runtimeSummary) { - lines.push(`Service runtime: ${runtimeSummary}`); - } - - lines.push(...renderPortUsageDiagnostics(snapshot)); - - return lines; -} - -export function renderGatewayPortHealthDiagnostics(snapshot: GatewayPortHealthSnapshot): string[] { - return renderPortUsageDiagnostics(snapshot); -} diff --git a/src/cli/daemon-cli/restart-health.types.ts b/src/cli/daemon-cli/restart-health.types.ts new file mode 100644 index 000000000000..f8ab24a58cda --- /dev/null +++ b/src/cli/daemon-cli/restart-health.types.ts @@ -0,0 +1,34 @@ +import type { PluginHealthErrorSummary } from "../../commands/health.types.js"; +import type { GatewayServiceRuntime } from "../../daemon/service-runtime.js"; +import type { PortUsage } from "../../infra/ports.js"; + +export type GatewayRestartWaitOutcome = + | "healthy" + | "plugin-errors" + | "channel-errors" + | "version-mismatch" + | "stale-pids" + | "stopped-free" + | "timeout"; + +export type GatewayRestartSnapshot = { + runtime: GatewayServiceRuntime; + portUsage: PortUsage; + healthy: boolean; + staleGatewayPids: number[]; + gatewayVersion?: string | null; + activatedPluginErrors?: PluginHealthErrorSummary[]; + channelProbeErrors?: Array<{ id: string; error: string }>; + expectedVersion?: string; + versionMismatch?: { + expected: string; + actual: string | null; + }; + waitOutcome?: GatewayRestartWaitOutcome; + elapsedMs?: number; +}; + +export type GatewayPortHealthSnapshot = { + portUsage: PortUsage; + healthy: boolean; +}; diff --git a/src/daemon/schtasks.ts b/src/daemon/schtasks.ts index 110f7d22bdcb..c5e12c4a37ff 100644 --- a/src/daemon/schtasks.ts +++ b/src/daemon/schtasks.ts @@ -838,6 +838,9 @@ async function waitForProcessExit(pid: number, timeoutMs: number): Promise { if (process.platform !== "win32") { + // Use leader-checked default: gateway/listener termination paths resolve PIDs by argv + // or port ownership rather than spawn-time process-group creation, so we rely on the + // helper's process-group leader verification to avoid signaling the gateway's own group. killProcessTree(pid, { graceMs }); return; } diff --git a/src/gateway/worker-environments/workspace-sync-local.ts b/src/gateway/worker-environments/workspace-sync-local.ts index acb9f9e0e355..34cf40465c09 100644 --- a/src/gateway/worker-environments/workspace-sync-local.ts +++ b/src/gateway/worker-environments/workspace-sync-local.ts @@ -96,7 +96,10 @@ export async function runLocalCommandToFile(params: { terminationStarted = true; const pid = child.pid; if (typeof pid === "number" && pid > 0) { - killProcessTree(pid, { graceMs: COMMAND_KILL_GRACE_MS }); + killProcessTree(pid, { + graceMs: COMMAND_KILL_GRACE_MS, + detached: process.platform !== "win32", + }); } else { child.kill("SIGTERM"); } @@ -104,7 +107,7 @@ export async function runLocalCommandToFile(params: { // shutdown so placement replacement cannot wait forever on that pipe. terminationTimer = setTimeout(() => { if (typeof pid === "number" && pid > 0) { - killProcessTree(pid, { force: true }); + killProcessTree(pid, { force: true, detached: process.platform !== "win32" }); } else { child.kill("SIGKILL"); } diff --git a/src/process/exec-termination.ts b/src/process/exec-termination.ts index dacffd8b2034..fa05f12f969a 100644 --- a/src/process/exec-termination.ts +++ b/src/process/exec-termination.ts @@ -102,7 +102,10 @@ export function createCommandTerminationController(params: { startWindowsTermination(childPid, true); return true; } - terminateProcessTree(childPid, { graceMs: COMMAND_PROCESS_TREE_KILL_GRACE_MS }); + terminateProcessTree(childPid, { + graceMs: COMMAND_PROCESS_TREE_KILL_GRACE_MS, + detached: true, + }); return false; } if (!directChildAlive) { @@ -136,7 +139,7 @@ export function createCommandTerminationController(params: { }); } if (process.platform !== "win32") { - terminateProcessTree(params.child.pid, { force: true }); + terminateProcessTree(params.child.pid, { force: true, detached: true }); } }; diff --git a/src/process/kill-tree.test.ts b/src/process/kill-tree.test.ts index 5141cafd1c67..f47a3b181ed6 100644 --- a/src/process/kill-tree.test.ts +++ b/src/process/kill-tree.test.ts @@ -3,8 +3,14 @@ import { EventEmitter } from "node:events"; import { afterEach, beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; import { withMockedPlatform } from "../test-utils/vitest-spies.js"; -const { spawnMock } = vi.hoisted(() => ({ +const { readFileSyncMock, spawnMock, spawnSyncMock } = vi.hoisted(() => ({ + readFileSyncMock: vi.fn(), spawnMock: vi.fn(), + spawnSyncMock: vi.fn(), +})); + +vi.mock("node:fs", () => ({ + readFileSync: (...args: unknown[]) => readFileSyncMock(...args), })); vi.mock("node:child_process", async () => { @@ -13,6 +19,7 @@ vi.mock("node:child_process", async () => { () => vi.importActual("node:child_process"), { spawn: (...args: unknown[]) => spawnMock(...args), + spawnSync: (...args: unknown[]) => spawnSyncMock(...args), }, ); }); @@ -32,6 +39,18 @@ function expectTaskkillCall(index: number, args: string[]) { ]); } +function mockIsProcessGroupLeader(...pids: number[]) { + spawnSyncMock.mockImplementation((command: string, args: string[]) => { + if (command === "ps" && args[0] === "-p" && args[2] === "-o" && args[3] === "pgid=") { + const pid = Number.parseInt(args[1] ?? "", 10); + if (pids.includes(pid)) { + return { status: 0, stdout: String(pid) }; + } + } + return { status: 1, stdout: "" }; + }); +} + describe("killProcessTree", () => { let killSpy: ReturnType; @@ -40,7 +59,12 @@ describe("killProcessTree", () => { }); beforeEach(() => { + readFileSyncMock.mockReset(); + readFileSyncMock.mockImplementation(() => { + throw new Error("proc unavailable"); + }); spawnMock.mockClear(); + spawnSyncMock.mockClear(); killSpy = vi.spyOn(process, "kill"); vi.useFakeTimers(); }); @@ -101,6 +125,7 @@ describe("killProcessTree", () => { }) as typeof process.kill); await withMockedPlatform("linux", async () => { + mockIsProcessGroupLeader(3333); killProcessTree(3333, { graceMs: 10 }); await vi.advanceTimersByTimeAsync(10); @@ -120,6 +145,7 @@ describe("killProcessTree", () => { }) as typeof process.kill); await withMockedPlatform("linux", async () => { + mockIsProcessGroupLeader(4444); killProcessTree(4444, { graceMs: 5 }); await vi.advanceTimersByTimeAsync(5); @@ -133,6 +159,7 @@ describe("killProcessTree", () => { killSpy.mockImplementation(() => true); await withMockedPlatform("linux", async () => { + mockIsProcessGroupLeader(4949); killProcessTree(4949, { force: true }); await vi.advanceTimersByTimeAsync(60_000); @@ -154,6 +181,7 @@ describe("killProcessTree", () => { }) as typeof process.kill); await withMockedPlatform("linux", async () => { + mockIsProcessGroupLeader(4545); killProcessTree(4545, { graceMs: 5 }); await vi.advanceTimersByTimeAsync(5); @@ -185,7 +213,7 @@ describe("killProcessTree", () => { }); }); - it("on Unix uses group kill by default (detached:true preserved as the existing behavior)", async () => { + it("on Unix uses group kill when the omitted option resolves to a group leader", async () => { killSpy.mockImplementation(((pid: number, signal?: NodeJS.Signals | number) => { if (pid === -6666 && signal === 0) { throw new Error("ESRCH"); @@ -197,6 +225,7 @@ describe("killProcessTree", () => { }) as typeof process.kill); await withMockedPlatform("linux", async () => { + mockIsProcessGroupLeader(6666); killProcessTree(6666, { graceMs: 10 }); await vi.advanceTimersByTimeAsync(10); @@ -204,10 +233,69 @@ describe("killProcessTree", () => { }); }); + it.each([ + [ + "throws", + () => { + throw new Error("ps ENOENT"); + }, + ], + ["exits non-zero", () => ({ status: 1, stdout: "" })], + ["returns non-numeric output", () => ({ status: 0, stdout: "not-a-pgid" })], + ["returns empty output", () => ({ status: 0, stdout: "" })], + ])("on Unix falls back to single-pid kill when ps %s", async (_label, psResult) => { + killSpy.mockImplementation(() => true); + + await withMockedPlatform("darwin", async () => { + spawnSyncMock.mockImplementation(psResult); + killProcessTree(8888, { graceMs: 10 }); + await vi.advanceTimersByTimeAsync(10); + + expect(killSpy).toHaveBeenCalledWith(8888, "SIGTERM"); + expect(killSpy).not.toHaveBeenCalledWith(-8888, "SIGTERM"); + expect(killSpy).not.toHaveBeenCalledWith(-8888, "SIGKILL"); + }); + }); + + it("on Unix falls back to single-pid kill when ps returns different PGID", async () => { + killSpy.mockImplementation(() => true); + + await withMockedPlatform("linux", async () => { + spawnSyncMock.mockImplementation((command: string, args: string[]) => { + if (command === "ps" && args[0] === "-p" && args[2] === "-o" && args[3] === "pgid=") { + const pid = Number.parseInt(args[1] ?? "", 10); + if (pid === 9999) { + return { status: 0, stdout: "12345\n" }; + } + } + return { status: 1, stdout: "" }; + }); + killProcessTree(9999, { graceMs: 10 }); + await vi.advanceTimersByTimeAsync(10); + + expect(killSpy).toHaveBeenCalledWith(9999, "SIGTERM"); + expect(killSpy).not.toHaveBeenCalledWith(-9999, "SIGTERM"); + expect(killSpy).not.toHaveBeenCalledWith(-9999, "SIGKILL"); + }); + }); + + it("on Linux reads process-group ownership from procfs without spawning ps", async () => { + killSpy.mockImplementation(() => true); + readFileSyncMock.mockReturnValue("7777 (shell worker) S 1 7777 7777 0"); + + await withMockedPlatform("linux", async () => { + signalProcessTree(7777, "SIGTERM"); + + expect(killSpy).toHaveBeenCalledWith(-7777, "SIGTERM"); + expect(spawnSyncMock).not.toHaveBeenCalled(); + }); + }); + it("on Unix sends a single requested tree signal without scheduling escalation", async () => { killSpy.mockImplementation(() => true); await withMockedPlatform("linux", async () => { + mockIsProcessGroupLeader(7777); signalProcessTree(7777, "SIGTERM"); await vi.advanceTimersByTimeAsync(60_000); diff --git a/src/process/supervisor/adapters/pty.test.ts b/src/process/supervisor/adapters/pty.test.ts index 69bf74e71029..969034ca5cc2 100644 --- a/src/process/supervisor/adapters/pty.test.ts +++ b/src/process/supervisor/adapters/pty.test.ts @@ -116,7 +116,7 @@ describe("createPtyAdapter", () => { }); adapter.kill("SIGTERM"); - expect(signalProcessTreeMock).toHaveBeenCalledWith(1234, "SIGTERM"); + expect(signalProcessTreeMock).toHaveBeenCalledWith(1234, "SIGTERM", { detached: true }); expect(ptyKillMock).not.toHaveBeenCalled(); }); @@ -129,7 +129,7 @@ describe("createPtyAdapter", () => { }); adapter.kill(); - expect(signalProcessTreeMock).toHaveBeenCalledWith(1234, "SIGKILL"); + expect(signalProcessTreeMock).toHaveBeenCalledWith(1234, "SIGKILL", { detached: true }); expect(ptyKillMock).not.toHaveBeenCalled(); }); @@ -319,7 +319,7 @@ describe("createPtyAdapter", () => { }); adapter.kill("SIGKILL"); - expect(signalProcessTreeMock).toHaveBeenCalledWith(4567, "SIGKILL"); + expect(signalProcessTreeMock).toHaveBeenCalledWith(4567, "SIGKILL", { detached: true }); expect(ptyKillMock).not.toHaveBeenCalled(); } finally { if (originalPlatform) { diff --git a/src/process/supervisor/adapters/pty.ts b/src/process/supervisor/adapters/pty.ts index e73e8cf9c8a5..e7ace17a745a 100644 --- a/src/process/supervisor/adapters/pty.ts +++ b/src/process/supervisor/adapters/pty.ts @@ -151,7 +151,7 @@ export async function createPtyAdapter(params: { typeof pty.pid === "number" && pty.pid > 0 ) { - signalProcessTree(pty.pid, signal); + signalProcessTree(pty.pid, signal, { detached: true }); } else if (process.platform === "win32") { pty.kill(); } else { diff --git a/src/process/terminal-pty.test.ts b/src/process/terminal-pty.test.ts index 08f26c98182c..1ae629d37e86 100644 --- a/src/process/terminal-pty.test.ts +++ b/src/process/terminal-pty.test.ts @@ -49,7 +49,9 @@ describe("terminal PTY teardown", () => { it.each([undefined, "SIGTERM"] as const)("signals the process tree for %s", async (signal) => { const { handle, pty } = await spawnFakePty(); handle.kill(signal); - expect(mocks.signalProcessTree).toHaveBeenCalledWith(4321, signal ?? "SIGKILL"); + expect(mocks.signalProcessTree).toHaveBeenCalledWith(4321, signal ?? "SIGKILL", { + detached: true, + }); expect(pty.kill).not.toHaveBeenCalled(); }); diff --git a/src/process/terminal-pty.ts b/src/process/terminal-pty.ts index 94924ab1bd92..11b7a56bbaf8 100644 --- a/src/process/terminal-pty.ts +++ b/src/process/terminal-pty.ts @@ -86,7 +86,9 @@ function killPtyTree(pty: Pick, signal?: string): void { const sig = (signal ?? "SIGKILL") as NodeJS.Signals; try { if ((sig === "SIGKILL" || sig === "SIGTERM") && typeof pty.pid === "number" && pty.pid > 0) { - signalProcessTree(pty.pid, sig); + // forkpty creates a new session/process group; retain descendant cleanup + // after the shell exits and only its group remains. + signalProcessTree(pty.pid, sig, { detached: true }); } else if (process.platform === "win32") { pty.kill(); } else {