Skip to content

Commit 7e4ce96

Browse files
committed
fix: SIGKILL escalation in killProcessTree + unit tests
Remaining review follow-ups: - killProcessTree now escalates to SIGKILL after 500ms if SIGTERM doesn't kill the process (same pattern as killTrackedProcesses). Covers orphan cleanup and dev/local mode tree kill. - Added unit tests for both new modules: - processTracker.test.ts (6 tests): track/remove on exit/error, kill running processes, SIGKILL escalation for SIGTERM-resistant processes, idempotency. - orphanCleanup.test.ts (5 tests): tree kill with children, SIGKILL escalation, non-existent PID handling, orphan detection returns 0 when clean.
1 parent 84edce9 commit 7e4ce96

3 files changed

Lines changed: 145 additions & 6 deletions

File tree

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
import { describe, it, expect } from "vitest";
2+
import { spawn } from "node:child_process";
3+
import { killProcessTree, killOrphanedProcesses } from "./orphanCleanup.js";
4+
5+
const IS_UNIX = process.platform !== "win32";
6+
7+
describe.skipIf(!IS_UNIX)("killProcessTree", () => {
8+
it("kills a process and all its children", async () => {
9+
// Spawn a parent that spawns two sleeping children
10+
const parent = spawn("bash", ["-c", "sleep 60 & sleep 60 & wait"], { stdio: "ignore" });
11+
// Let children spawn
12+
await new Promise((r) => setTimeout(r, 200));
13+
14+
const exitPromise = new Promise<void>((resolve) => parent.on("close", resolve));
15+
killProcessTree(parent.pid!);
16+
17+
await exitPromise;
18+
19+
// Verify parent is dead
20+
expect(() => process.kill(parent.pid!, 0)).toThrow();
21+
}, 5000);
22+
23+
it("handles non-existent PID gracefully", () => {
24+
// Should not throw for a PID that doesn't exist
25+
killProcessTree(999999999);
26+
});
27+
28+
it("escalates to SIGKILL after grace period", async () => {
29+
// Spawn a process that traps SIGTERM
30+
const proc = spawn("bash", ["-c", "trap '' TERM; sleep 60"], { stdio: "ignore" });
31+
await new Promise((r) => setTimeout(r, 100));
32+
33+
const exitPromise = new Promise<void>((resolve) => proc.on("close", resolve));
34+
killProcessTree(proc.pid!);
35+
36+
// Should die within 1s (500ms SIGKILL grace + buffer)
37+
await exitPromise;
38+
expect(() => process.kill(proc.pid!, 0)).toThrow();
39+
}, 5000);
40+
});
41+
42+
describe.skipIf(!IS_UNIX)("killOrphanedProcesses", () => {
43+
it("returns 0 when no orphans exist", () => {
44+
const killed = killOrphanedProcesses();
45+
expect(killed).toBe(0);
46+
});
47+
48+
it("does not kill non-orphaned Chrome processes", () => {
49+
// Our current process is not an orphan (PPID !== 1), so any
50+
// chrome-headless-shell processes we'd find with our PID as
51+
// ancestor wouldn't be killed.
52+
const killed = killOrphanedProcesses();
53+
expect(killed).toBe(0);
54+
});
55+
});

packages/cli/src/utils/orphanCleanup.ts

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -41,17 +41,27 @@ export function killProcessTree(pid: number, signal: NodeJS.Signals = "SIGTERM")
4141
if (process.platform === "win32") return;
4242

4343
const descendants = getDescendants(pid);
44-
for (const child of descendants.reverse()) {
44+
const allPids = [...descendants.reverse(), pid];
45+
46+
for (const p of allPids) {
4547
try {
46-
process.kill(child, signal);
48+
process.kill(p, signal);
4749
} catch {
4850
// Already exited.
4951
}
5052
}
51-
try {
52-
process.kill(pid, signal);
53-
} catch {
54-
// Already exited.
53+
54+
// Escalate to SIGKILL after a short grace period for any survivors.
55+
if (signal !== "SIGKILL") {
56+
setTimeout(() => {
57+
for (const p of allPids) {
58+
try {
59+
process.kill(p, "SIGKILL");
60+
} catch {
61+
// Already exited.
62+
}
63+
}
64+
}, 500).unref();
5565
}
5666
}
5767

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
import { describe, it, expect, beforeEach } from "vitest";
2+
import { spawn } from "node:child_process";
3+
import { trackChildProcess, killTrackedProcesses } from "./processTracker.js";
4+
5+
// Reset tracked set between tests by killing everything
6+
beforeEach(() => {
7+
killTrackedProcesses();
8+
});
9+
10+
describe("trackChildProcess", () => {
11+
it("tracks a spawned process and removes it after exit", async () => {
12+
const proc = spawn("echo", ["hello"], { stdio: "ignore" });
13+
trackChildProcess(proc);
14+
15+
await new Promise<void>((resolve) => proc.on("close", resolve));
16+
17+
// After exit, killTrackedProcesses should be a no-op (nothing to kill)
18+
killTrackedProcesses();
19+
});
20+
21+
it("removes the process on spawn error", async () => {
22+
const proc = spawn("/nonexistent-binary-that-does-not-exist", { stdio: "ignore" });
23+
trackChildProcess(proc);
24+
25+
await new Promise<void>((resolve) => proc.on("error", () => resolve()));
26+
27+
killTrackedProcesses();
28+
});
29+
});
30+
31+
describe("killTrackedProcesses", () => {
32+
it("kills a running process", async () => {
33+
const proc = spawn("sleep", ["60"], { stdio: "ignore" });
34+
trackChildProcess(proc);
35+
36+
const exitPromise = new Promise<number | null>((resolve) => proc.on("close", resolve));
37+
killTrackedProcesses();
38+
39+
const code = await exitPromise;
40+
// SIGTERM exit: code is null (killed by signal)
41+
expect(code).toBeNull();
42+
});
43+
44+
it("handles already-exited processes gracefully", async () => {
45+
const proc = spawn("true", { stdio: "ignore" });
46+
trackChildProcess(proc);
47+
48+
await new Promise<void>((resolve) => proc.on("close", resolve));
49+
50+
// Should not throw even though process already exited
51+
killTrackedProcesses();
52+
});
53+
54+
it("escalates to SIGKILL for processes that ignore SIGTERM", async () => {
55+
// Spawn a process that traps SIGTERM (bash ignoring it)
56+
const proc = spawn("bash", ["-c", "trap '' TERM; sleep 60"], { stdio: "ignore" });
57+
trackChildProcess(proc);
58+
59+
const exitPromise = new Promise<void>((resolve) => proc.on("close", resolve));
60+
killTrackedProcesses();
61+
62+
// The 500ms SIGKILL escalation should kill it
63+
await exitPromise;
64+
expect(proc.killed).toBe(true);
65+
}, 5000);
66+
67+
it("is idempotent — second call is a no-op", () => {
68+
const proc = spawn("sleep", ["60"], { stdio: "ignore" });
69+
trackChildProcess(proc);
70+
71+
killTrackedProcesses();
72+
killTrackedProcesses();
73+
});
74+
});

0 commit comments

Comments
 (0)