mirror of
https://github.com/openclaw/openclaw.git
synced 2026-08-12 21:53:00 -06:00
fix(kill-tree): verify process group leader before using group kill to prevent gateway SIGTERM (#76259) (#94697)
* fix(kill-tree): verify process group leader before group kill to prevent gateway SIGTERM (#76259) - Add isProcessGroupLeader() to killProcessTree/signalProcessTree: ps -p <pid> -o pgid= primary check with /proc/<pid>/stat fallback on Linux. Group kill only when the PID is its own process group leader; non-leaders fall back to single-pid kill, preventing accidental gateway SIGTERM when a non-detached child shares the gateway's process group. - Propagate detached: true to all detached-spawn cleanup callers (exec-termination, agent-bundle LSP, mcp-stdio, bash, supervisor pty, agent-core nodejs) so detached group cleanup survives leader exit. - Gateway/daemon cleanup paths (schtasks, restart-health) keep the leader-checked default (detached omitted). Closes #76259 Co-Authored-By: Claude <noreply@anthropic.com> * refactor(process): tighten process-group ownership checks * refactor(daemon): split restart diagnostics * refactor(daemon): isolate restart health types --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Peter Steinberger <steipete@gmail.com>
This commit is contained in:
+64
-6
@@ -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",
|
||||
|
||||
+2
-2
@@ -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);
|
||||
|
||||
|
||||
@@ -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();
|
||||
|
||||
@@ -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)) {
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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<void>((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) {
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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 });
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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", () => {
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
@@ -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",
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
|
||||
@@ -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;
|
||||
};
|
||||
@@ -838,6 +838,9 @@ async function waitForProcessExit(pid: number, timeoutMs: number): Promise<boole
|
||||
|
||||
async function terminateGatewayProcessTree(pid: number, graceMs: number): Promise<void> {
|
||||
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;
|
||||
}
|
||||
|
||||
@@ -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");
|
||||
}
|
||||
|
||||
@@ -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 });
|
||||
}
|
||||
};
|
||||
|
||||
|
||||
@@ -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<typeof import("node:child_process")>("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<typeof vi.spyOn>;
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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();
|
||||
});
|
||||
|
||||
|
||||
@@ -86,7 +86,9 @@ function killPtyTree(pty: Pick<IPty, "pid" | "kill">, 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 {
|
||||
|
||||
Reference in New Issue
Block a user