Skip to content

Commit 7eb71e2

Browse files
xydigit-ztclaude
xydigit-zt
and
claude
committed
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 <[email protected]>
1 parent 92e1ad8 commit 7eb71e2

17 files changed

Lines changed: 187 additions & 32 deletions

‎packages/agent-core/src/harness/env/kill-tree.ts‎

Lines changed: 79 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
// Agent Core module implements kill tree behavior.
2-
import { spawn } from "node:child_process";
2+
import { spawn, spawnSync } from "node:child_process";
3+
import { readFileSync } from "node:fs";
34

45
const DEFAULT_GRACE_MS = 3000;
56
const MAX_GRACE_MS = 60_000;
@@ -16,9 +17,14 @@ export type KillProcessTreeOptions = {
1617
* first (without /F), then force-kills if process survives.
1718
* - Unix: send SIGTERM to process group first, wait grace period, then SIGKILL.
1819
*
19-
* When the child was spawned with `detached: false`, pass `detached: false` to
20-
* skip the Unix `process.kill(-pid, ...)` group-kill. That avoids signaling the
21-
* gateway's own process group.
20+
* Group kill (`process.kill(-pid, ...)`) is only used when the PID is verified
21+
* as its own process group leader, unless `detached: true` is explicitly passed.
22+
* This prevents accidentally signaling the gateway's process group when the
23+
* child shares its parent's group.
24+
*
25+
* - `detached: false`: skip group kill unconditionally.
26+
* - `detached: true`: use group kill unconditionally (trust caller).
27+
* - `detached` omitted: use group kill only when PID is the group leader.
2228
*/
2329
export function killProcessTree(pid: number, opts?: KillProcessTreeOptions): void {
2430
if (!Number.isFinite(pid) || pid <= 0) {
@@ -35,7 +41,8 @@ export function killProcessTree(pid: number, opts?: KillProcessTreeOptions): voi
3541
return;
3642
}
3743

38-
const useGroupKill = opts?.detached !== false;
44+
const useGroupKill =
45+
opts?.detached === true || (opts?.detached !== false && isProcessGroupLeader(pid));
3946
if (opts?.force === true) {
4047
signalProcessTreeUnix(pid, "SIGKILL", useGroupKill);
4148
return;
@@ -68,7 +75,9 @@ export function signalProcessTree(
6875
return;
6976
}
7077

71-
signalProcessTreeUnix(pid, signal, opts?.detached !== false);
78+
const useGroupKill =
79+
opts?.detached === true || (opts?.detached !== false && isProcessGroupLeader(pid));
80+
signalProcessTreeUnix(pid, signal, useGroupKill);
7281
}
7382

7483
function normalizeGraceMs(value?: number): number {
@@ -87,6 +96,70 @@ function isProcessAlive(pid: number): boolean {
8796
}
8897
}
8998

99+
/**
100+
* Check whether a PID is its own process group leader.
101+
* Uses `ps -p <pid> -o pgid=` to read the process group ID and compares it
102+
* to the PID. Falls back to `false` on any error, which safely skips group
103+
* kill in environments where `ps` is unavailable.
104+
*
105+
* Linux fallback: if ps is unavailable (ENOENT), attempts to read
106+
* /proc/<pid>/stat field 5 (pgid) as a secondary check.
107+
*/
108+
function isProcessGroupLeader(pid: number): boolean {
109+
try {
110+
const res = spawnSync("ps", ["-p", String(pid), "-o", "pgid="], {
111+
encoding: "utf8",
112+
timeout: 500,
113+
});
114+
if (!res.error && res.status === 0) {
115+
const pgid = Number.parseInt(res.stdout.trim(), 10);
116+
if (Number.isFinite(pgid) && pgid === pid) {
117+
return true;
118+
}
119+
}
120+
} catch {
121+
// ps failed, fall through to secondary check
122+
}
123+
124+
// Secondary: /proc/<pid>/stat (Linux only). No-op on macOS/Windows.
125+
if (process.platform === "linux") {
126+
return isProcessGroupLeaderFromProc(pid);
127+
}
128+
return false;
129+
}
130+
131+
/**
132+
* Linux-only fallback: read /proc/<pid>/stat field 5 (pgid) to check
133+
* whether the process is its own process group leader.
134+
*/
135+
function isProcessGroupLeaderFromProc(pid: number): boolean {
136+
try {
137+
const stat = readFileSync(`/proc/${pid}/stat`, { encoding: "utf8" });
138+
// /proc/<pid>/stat format: pid (comm) state ppid pgrp ...
139+
// Field 5 (1-indexed) is pgrp (process group ID)
140+
const fields = stat.split(" ");
141+
if (fields.length >= 5) {
142+
// The 5th field is the process group ID (pgrp)
143+
// Format is "pid (comm) state ppid pgrp session tty_nr ..."
144+
// We need to find the actual 5th field considering parentheses in comm
145+
// Find the last ')' to reliably locate field 5
146+
const lastParenIdx = stat.lastIndexOf(")");
147+
if (lastParenIdx > 0) {
148+
const afterComm = stat.slice(lastParenIdx + 1).trimStart();
149+
const parts = afterComm.split(/\s+/);
150+
if (parts.length >= 3) {
151+
// parts[0] = state, parts[1] = ppid, parts[2] = pgrp
152+
const pgid = Number.parseInt(parts[2] ?? "", 10);
153+
return Number.isFinite(pgid) && pgid === pid;
154+
}
155+
}
156+
}
157+
return false;
158+
} catch {
159+
return false;
160+
}
161+
}
162+
90163
function signalProcessTreeUnix(
91164
pid: number,
92165
signal: "SIGTERM" | "SIGKILL",

‎packages/agent-core/src/harness/env/nodejs.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -313,7 +313,7 @@ export class NodeExecutionEnv implements ExecutionEnv {
313313

314314
const onAbort = () => {
315315
if (child?.pid) {
316-
killProcessTree(child.pid, { force: true });
316+
killProcessTree(child.pid, { force: true, detached: true });
317317
}
318318
};
319319

@@ -354,7 +354,7 @@ export class NodeExecutionEnv implements ExecutionEnv {
354354
: setTimeout(() => {
355355
timedOut = true;
356356
if (child?.pid) {
357-
killProcessTree(child.pid, { force: true });
357+
killProcessTree(child.pid, { force: true, detached: true });
358358
}
359359
}, timeoutMs);
360360

‎src/agents/agent-bundle-lsp-runtime.test.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -160,7 +160,7 @@ describe("bundle LSP runtime", () => {
160160

161161
await runtime.dispose();
162162

163-
expect(killProcessTreeMock).toHaveBeenCalledWith(4321, { graceMs: 1000 });
163+
expect(killProcessTreeMock).toHaveBeenCalledWith(4321, { graceMs: 1000, detached: true });
164164
});
165165

166166
it("fails LSP startup immediately when the child process cannot spawn", async () => {
@@ -175,7 +175,7 @@ describe("bundle LSP runtime", () => {
175175

176176
expect(runtime.sessions).toEqual([]);
177177
expect(runtime.tools).toEqual([]);
178-
expect(killProcessTreeMock).toHaveBeenCalledWith(4321, { graceMs: 1000 });
178+
expect(killProcessTreeMock).toHaveBeenCalledWith(4321, { graceMs: 1000, detached: true });
179179
});
180180

181181
it.each([
@@ -430,7 +430,7 @@ describe("bundle LSP runtime", () => {
430430
]);
431431

432432
expect(outcome).toMatch(/LSP framing error/i);
433-
expect(killProcessTreeMock).toHaveBeenCalledWith(4321, { graceMs: 1000 });
433+
expect(killProcessTreeMock).toHaveBeenCalledWith(4321, { graceMs: 1000, detached: true });
434434

435435
await runtime.dispose();
436436
});
@@ -444,7 +444,7 @@ describe("bundle LSP runtime", () => {
444444

445445
await disposeAllBundleLspRuntimes();
446446

447-
expect(killProcessTreeMock).toHaveBeenCalledWith(4321, { graceMs: 1000 });
447+
expect(killProcessTreeMock).toHaveBeenCalledWith(4321, { graceMs: 1000, detached: true });
448448

449449
killProcessTreeMock.mockClear();
450450
await runtime.dispose();

‎src/agents/agent-bundle-lsp-runtime.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -373,7 +373,7 @@ function hasLspProcessExited(child: ChildProcess): boolean {
373373
function terminateLspProcessTree(session: LspSession): void {
374374
const pid = session.process.pid;
375375
if (pid && !hasLspProcessExited(session.process)) {
376-
session.killProcessTree(pid, { graceMs: LSP_PROCESS_TREE_KILL_GRACE_MS });
376+
session.killProcessTree(pid, { graceMs: LSP_PROCESS_TREE_KILL_GRACE_MS, detached: true });
377377
return;
378378
}
379379
if (!hasLspProcessExited(session.process)) {

‎src/agents/mcp-stdio-transport.test.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,7 @@ describe("OpenClawStdioClientTransport", () => {
8989

9090
const closing = transport.close();
9191
await vi.advanceTimersByTimeAsync(2000);
92-
expect(killProcessTreeMock).toHaveBeenCalledWith(4321);
92+
expect(killProcessTreeMock).toHaveBeenCalledWith(4321, { detached: true });
9393

9494
child.exitCode = 0;
9595
child.emit("close", 0);
@@ -108,13 +108,13 @@ describe("OpenClawStdioClientTransport", () => {
108108

109109
const closing = transport.close();
110110
await vi.advanceTimersByTimeAsync(2000);
111-
expect(killProcessTreeMock).toHaveBeenCalledWith(4321);
111+
expect(killProcessTreeMock).toHaveBeenCalledWith(4321, { detached: true });
112112
expect(signalProcessTreeMock).not.toHaveBeenCalled();
113113

114114
// killProcessTree's SIGKILL is .unref()'d (#86412); close() force-SIGKILLs
115115
// synchronously instead.
116116
await vi.advanceTimersByTimeAsync(2000);
117-
expect(signalProcessTreeMock).toHaveBeenCalledWith(4321, "SIGKILL");
117+
expect(signalProcessTreeMock).toHaveBeenCalledWith(4321, "SIGKILL", { detached: true });
118118

119119
child.exitCode = 0;
120120
child.emit("close", 0);
@@ -134,7 +134,7 @@ describe("OpenClawStdioClientTransport", () => {
134134
const closing = transport.close();
135135
const repeatedClose = transport.close();
136136
const forced = transport.forceClose();
137-
expect(signalProcessTreeMock).toHaveBeenCalledWith(4321, "SIGKILL");
137+
expect(signalProcessTreeMock).toHaveBeenCalledWith(4321, "SIGKILL", { detached: true });
138138

139139
child.exitCode = 0;
140140
child.emit("close", 0);

‎src/agents/mcp-stdio-transport.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -133,11 +133,11 @@ export class OpenClawStdioClientTransport implements Transport {
133133
}
134134
await Promise.race([closePromise, delay(CLOSE_TIMEOUT_MS)]);
135135
if (processToClose.exitCode === null && processToClose.pid) {
136-
killProcessTree(processToClose.pid);
136+
killProcessTree(processToClose.pid, { detached: true });
137137
await Promise.race([closePromise, delay(CLOSE_TIMEOUT_MS)]);
138138
if (processToClose.exitCode === null && processToClose.pid) {
139139
// SIGKILL synchronously: killProcessTree's setTimeout is .unref()'d and races shutdown (#86412).
140-
signalProcessTree(processToClose.pid, "SIGKILL");
140+
signalProcessTree(processToClose.pid, "SIGKILL", { detached: true });
141141
await Promise.race([closePromise, delay(SIGKILL_REAP_TIMEOUT_MS)]);
142142
}
143143
}
@@ -155,7 +155,7 @@ export class OpenClawStdioClientTransport implements Transport {
155155
const closePromise = new Promise<void>((resolve) => {
156156
processToClose.once("close", () => resolve());
157157
});
158-
signalProcessTree(processToClose.pid, "SIGKILL");
158+
signalProcessTree(processToClose.pid, "SIGKILL", { detached: true });
159159
await Promise.race([closePromise, delay(SIGKILL_REAP_TIMEOUT_MS)]);
160160
}
161161
if (this.closingProcess === processToClose) {

‎src/agents/sessions/tools/bash.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ export function createLocalBashOperations(options?: { shellPath?: string }): Bas
7373
timeoutHandle = setTimeout(() => {
7474
timedOut = true;
7575
if (child.pid) {
76-
killProcessTree(child.pid);
76+
killProcessTree(child.pid, { detached: true });
7777
}
7878
}, timeoutMs);
7979
}
@@ -83,7 +83,7 @@ export function createLocalBashOperations(options?: { shellPath?: string }): Bas
8383
// Handle abort signal by killing the entire process tree.
8484
const onAbort = () => {
8585
if (child.pid) {
86-
killProcessTree(child.pid);
86+
killProcessTree(child.pid, { detached: true });
8787
}
8888
};
8989
if (signal) {

‎src/agents/shell-snapshot.spawn.test.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,6 @@ describe.skipIf(process.platform === "win32")("shell snapshot subprocesses", ()
6666

6767
const options = spawnMock.mock.calls[0]?.[2] as SpawnOptions | undefined;
6868
expect(options?.stdio).toBe("ignore");
69-
expect(killProcessTreeMock).toHaveBeenCalledWith(4242, { graceMs: 0 });
69+
expect(killProcessTreeMock).toHaveBeenCalledWith(4242, { graceMs: 0, detached: true });
7070
});
7171
});

‎src/agents/shell-snapshot.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -456,11 +456,11 @@ async function runShell(opts: {
456456
}
457457
settled = true;
458458
clearTimeout(timeout);
459-
killProcessTree(child.pid ?? 0, { graceMs: 0 });
459+
killProcessTree(child.pid ?? 0, { graceMs: 0, detached: true });
460460
resolve({ status });
461461
};
462462
const timeout = setTimeout(() => {
463-
killProcessTree(child.pid ?? 0, { graceMs: 250 });
463+
killProcessTree(child.pid ?? 0, { graceMs: 250, detached: true });
464464
finish(null);
465465
}, opts.timeoutMs);
466466
child.on("error", () => {

‎src/cli/daemon-cli/restart-health.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -675,6 +675,9 @@ export async function terminateStaleGatewayPids(pids: number[]): Promise<number[
675675
new Set(pids.filter((pid): pid is number => Number.isFinite(pid) && pid > 0)),
676676
);
677677
for (const pid of targets) {
678+
// Use leader-checked default: gateway listener PIDs are resolved by argv/port ownership,
679+
// not by spawn-time detached child fact, so we rely on the helper's process-group leader
680+
// verification to avoid signaling the gateway's own process group.
678681
killProcessTree(pid, { graceMs: 300 });
679682
}
680683
if (targets.length > 0) {

0 commit comments

Comments
 (0)