Skip to content

Commit b03aca0

Browse files
committed
fix(coding-agent): stop counting zombies as live pids in tree-kill proof
pidAlive used process.kill(pid, 0), which succeeds for a zombie: an exited process keeps its pid entry until the parent reaps it. The tree-kill proof resolves on pipe EOF, which only shows both processes released fd 1 while exiting, so the reparented grandchild is still an unreaped zombie for a moment afterwards. On a slow runner the assertion landed in that window and reported the tree as alive. Treat a zombie as dead by checking process state, leaving Windows untouched since it has no zombies. Cover it with a deterministic zombie built from a perl parent that forks, prints the child pid, releases fd 3 and never waits, so EOF on that pipe proves the child exited without adding a sleep.
1 parent a4c5f92 commit b03aca0

1 file changed

Lines changed: 62 additions & 2 deletions

File tree

packages/coding-agent/test/omo-local-update.test.ts

Lines changed: 62 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { execFileSync, spawnSync } from "node:child_process";
1+
import { execFileSync, spawn, spawnSync } from "node:child_process";
22
import { createHash } from "node:crypto";
33
import {
44
chmodSync,
@@ -112,10 +112,23 @@ function makeOmoLayout(
112112
return { repoRoot, pluginPath, agentDir };
113113
}
114114

115+
/**
116+
* A zombie has exited and holds no resources, but its pid entry survives until the
117+
* parent reaps it, so `process.kill(pid, 0)` still succeeds. Killing a process tree
118+
* therefore leaves a window where the reparented grandchild looks alive, which made
119+
* the tree-kill proof below fail on slow runners. Windows has no zombies.
120+
*/
115121
function pidAlive(pid: number): boolean {
116122
try {
117123
process.kill(pid, 0);
118-
return true;
124+
} catch {
125+
return false;
126+
}
127+
if (process.platform === "win32") return true;
128+
try {
129+
return !execFileSync("ps", ["-o", "stat=", "-p", String(pid)], { encoding: "utf-8" })
130+
.trim()
131+
.startsWith("Z");
119132
} catch {
120133
return false;
121134
}
@@ -1093,4 +1106,51 @@ describe("runOmoLocalUpdateBeta orchestrator", () => {
10931106
expect(existsSync(omoLocalUpdateLockPath(agentDir))).toBe(false);
10941107
expect(existsSync(omoLocalUpdateBuildWorktreePath(agentDir))).toBe(false);
10951108
});
1109+
1110+
it("does not report an unreaped zombie as alive", async () => {
1111+
if (process.platform === "win32") return;
1112+
// A perl parent forks a child that exits immediately, prints the child pid, releases
1113+
// fd 3, then sleeps WITHOUT reaping. EOF on fd 3 therefore proves the child exited AND
1114+
// the parent released its copy, so from that moment the child is a zombie until the
1115+
// parent is killed. Event-driven like the tree-kill proof: no sleeps, no polling.
1116+
const script =
1117+
'use POSIX (); my $pid = fork(); if ($pid == 0) { exit 0; } $| = 1; print "$pid\\n"; POSIX::close(3); sleep 30;';
1118+
const parent = spawn("perl", ["-e", script], { stdio: ["ignore", "pipe", "ignore", "pipe"] });
1119+
try {
1120+
let stdout = "";
1121+
const parentStdout = parent.stdout;
1122+
if (parentStdout === null) throw new Error("expected a readable stdout pipe");
1123+
parentStdout.on("data", (chunk) => {
1124+
stdout += String(chunk);
1125+
});
1126+
const zombieFd = parent.stdio[3];
1127+
if (zombieFd === null || zombieFd === undefined || typeof zombieFd === "number") {
1128+
throw new Error("expected a readable pipe on fd 3");
1129+
}
1130+
await new Promise<void>((resolveEof) => zombieFd.on("end", () => resolveEof()));
1131+
1132+
const zombiePid = Number(stdout.trim());
1133+
expect(Number.isInteger(zombiePid)).toBe(true);
1134+
expect(execFileSync("ps", ["-o", "stat=", "-p", String(zombiePid)], { encoding: "utf-8" }).trim()).toMatch(
1135+
/^Z/,
1136+
);
1137+
1138+
expect(pidAlive(zombiePid)).toBe(false);
1139+
} finally {
1140+
parent.kill("SIGKILL");
1141+
}
1142+
});
1143+
1144+
it("reports a running process as alive and a reaped pid as dead", async () => {
1145+
expect(pidAlive(process.pid)).toBe(true);
1146+
1147+
// Node reaps a child before emitting "exit", so once that fires the pid is fully gone
1148+
// rather than a zombie. Event-driven: the exit event is the signal, not a wait.
1149+
const shortLived = spawn(process.execPath, ["-e", "process.exit(0)"], { stdio: "ignore" });
1150+
const reapedPid = shortLived.pid;
1151+
if (reapedPid === undefined) throw new Error("expected a pid for the spawned process");
1152+
await new Promise<void>((resolveExit) => shortLived.on("exit", () => resolveExit()));
1153+
1154+
expect(pidAlive(reapedPid)).toBe(false);
1155+
});
10961156
});

0 commit comments

Comments
 (0)