Skip to content
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
- Fixed zombie processes being treated as live owners by the daemon supervisor ownership registry, session leases, supervisor launch locks, `daemon ps` process stops, and update-restart liveness checks; all process liveness probes now share the zombie-aware helper.
29 changes: 14 additions & 15 deletions packages/coding-agent/src/cli/daemon-ps.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import {
import { defaultDaemonSocketDir, defaultDaemonSocketPath, normalizeSocketPath } from "../modes/daemon/daemon-socket.js";
import { acquireDaemonShutdownAdmission } from "../modes/daemon/daemon-supervisor-ownership.js";
import type { DaemonWorkerDescriptor } from "../modes/daemon/daemon-worker-protocol.js";
import { signalProcessGroupOrProcess } from "../utils/child-process.js";
import { isProcessAlive, processGroupExists, signalProcessGroupOrProcess } from "../utils/child-process.js";
import { formatDaemonListTable } from "./daemon-ps-format.js";
import { promptYesNo } from "./daemon-stop-confirm.js";

Expand Down Expand Up @@ -1061,7 +1061,7 @@ async function stopTrackedProcess(
expectedStartId: string | undefined,
assertAdmission: () => Promise<void>,
): Promise<boolean> {
if (!isProcessAlive(pid)) {
if (trackedProcessStopped(pid)) {
return true;
}
if (!expectedStartId || getProcessStartId(pid) !== expectedStartId) {
Expand All @@ -1073,10 +1073,10 @@ async function stopTrackedProcess(
}
signalProcessGroupOrProcess(pid, "SIGTERM");
let deadline = Date.now() + 500;
while (isProcessAlive(pid) && Date.now() < deadline) {
while (!trackedProcessStopped(pid) && Date.now() < deadline) {
await delay(25);
}
if (!isProcessAlive(pid)) {
if (trackedProcessStopped(pid)) {
return true;
}
await assertAdmission();
Expand All @@ -1085,10 +1085,18 @@ async function stopTrackedProcess(
}
signalProcessGroupOrProcess(pid, "SIGKILL");
deadline = Date.now() + 1000;
while (isProcessAlive(pid) && Date.now() < deadline) {
while (!trackedProcessStopped(pid) && Date.now() < deadline) {
await delay(25);
}
return !isProcessAlive(pid);
return trackedProcessStopped(pid);
}

/**
* This is a process-GROUP stop: a zombie or reaped leader has exited, but
* descendants in its group can still run, so completion needs both gone.
*/
function trackedProcessStopped(pid: number): boolean {
return !isProcessAlive(pid) && !processGroupExists(pid);
Comment thread
macroscopeapp[bot] marked this conversation as resolved.
Outdated
Comment thread
snimu marked this conversation as resolved.
Outdated
}

export async function runReap(json: boolean, force: boolean): Promise<void> {
Expand Down Expand Up @@ -1220,15 +1228,6 @@ async function forceKillDaemon(pid: number): Promise<void> {
}
}

function isProcessAlive(pid: number): boolean {
try {
process.kill(pid, 0);
return true;
} catch (error) {
return (error as NodeJS.ErrnoException).code === "EPERM";
}
}

function delay(ms: number): Promise<void> {
return new Promise((resolve) => setTimeout(resolve, ms));
}
Expand Down
10 changes: 1 addition & 9 deletions packages/coding-agent/src/cli/daemon-update-restart.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {
DAEMON_WORKER_SUPERVISOR_SOCKET_ENV,
DAEMON_WORKER_TOKEN_ENV,
} from "../modes/daemon/daemon-worker-protocol.js";
import { isProcessAlive } from "../utils/child-process.js";
import { createCliSubprocessLaunchSpec } from "./subprocess-launch.js";

export const DAEMON_UPDATE_RESTART_COORDINATOR_FLAG = "--internal-update-restart-coordinator";
Expand Down Expand Up @@ -354,15 +355,6 @@ async function withCoordinatorRegistryGuard<T>(registryDir: string, action: () =
}
}

function isProcessAlive(pid: number): boolean {
try {
process.kill(pid, 0);
} catch (error) {
return (error as NodeJS.ErrnoException).code !== "ESRCH";
}
return true;
}

function matchesProcessStartId(identity: DaemonUpdateRestartProcessIdentity): boolean {
if (!identity.processStartId) {
return true;
Expand Down
10 changes: 1 addition & 9 deletions packages/coding-agent/src/core/session-lease.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { createHash, randomUUID } from "node:crypto";
import { existsSync, mkdirSync, readFileSync, realpathSync, renameSync, rmSync, writeFileSync } from "node:fs";
import { basename, dirname, join, resolve } from "node:path";
import { lockSync } from "proper-lockfile";
import { isProcessAlive } from "../utils/child-process.js";

export const SESSION_LEASES_ENABLED_ENV = "PRIME_AGENT_INTERNAL_SESSION_LEASES";
export const SESSION_LEASE_OWNER_ID_ENV = "PRIME_AGENT_INTERNAL_SESSION_LEASE_OWNER_ID";
Expand Down Expand Up @@ -101,15 +102,6 @@ function readLeaseOwner(directory: string): SessionLeaseOwner | undefined {
}
}

function isProcessAlive(pid: number): boolean {
try {
process.kill(pid, 0);
return true;
} catch (error) {
return (error as NodeJS.ErrnoException).code === "EPERM";
}
}

interface ProcessQueryOptions {
env?: NodeJS.ProcessEnv;
}
Expand Down
12 changes: 2 additions & 10 deletions packages/coding-agent/src/modes/daemon/daemon-mode.ts
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ import {
import { resolveSessionPath } from "../../core/session-resolver.js";
import type { SessionStats } from "../../core/session-stats.js";
import { type SideQuestionRun, startSideQuestion } from "../../core/side-question.js";
import { isProcessAlive } from "../../utils/child-process.js";
import { killTrackedDetachedChildren } from "../../utils/shell.js";
import {
createAgentConnectionCommands,
Expand Down Expand Up @@ -867,7 +868,7 @@ export class AgentDaemon {
} catch {
// An invalid owner is reclaimed atomically below.
}
if (ownerPid && this.isProcessAlive(ownerPid)) {
if (ownerPid && isProcessAlive(ownerPid)) {
return;
}
const staleDirectory = `${lockDirectory}.stale-${process.pid}-${token}`;
Expand Down Expand Up @@ -925,15 +926,6 @@ export class AgentDaemon {
}
}

private isProcessAlive(pid: number): boolean {
try {
process.kill(pid, 0);
return true;
} catch (error) {
return (error as NodeJS.ErrnoException).code === "EPERM";
}
}

private cleanupSocketPath(): void {
if (!this.ownsSocketPath) {
return;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import { homedir } from "node:os";
import { basename, dirname, join, resolve } from "node:path";
import lockfile from "proper-lockfile";
import { getProcessStartId } from "../../core/session-lease.js";
import { isProcessAlive, isZombieProcess, processIdExists } from "../../utils/child-process.js";
import { defaultDaemonSocketDir, normalizeSocketPath } from "./daemon-socket.js";

const DAEMON_SUPERVISOR_REGISTRY_DIR_ENV = "PRIME_AGENT_INTERNAL_DAEMON_SUPERVISOR_REGISTRY_DIR";
Expand Down Expand Up @@ -506,6 +507,30 @@ export async function acquireDaemonSupervisorOwnership(
return new DaemonSupervisorOwnership(record, registryDir, ownerDirectory);
}

// The fence poll asserts every bound claim every 250ms; the zombie check spawns
// `ps` on macOS/BSD, so confirm it at most this often per owner pid. Existence
// stays checked on every tick, which catches reaped owners immediately.
const OWNER_ZOMBIE_CONFIRM_INTERVAL_MS = 5000;
const ownerZombieConfirmations = new Map<number, number>();
Comment thread
macroscopeapp[bot] marked this conversation as resolved.

function isOwnerProcessAlive(pid: number): boolean {
if (!processIdExists(pid)) {
ownerZombieConfirmations.delete(pid);
return false;
}
const now = Date.now();
const confirmedAt = ownerZombieConfirmations.get(pid);
if (confirmedAt !== undefined && now - confirmedAt < OWNER_ZOMBIE_CONFIRM_INTERVAL_MS) {
return true;
}
if (isZombieProcess(pid)) {
ownerZombieConfirmations.delete(pid);
return false;
}
ownerZombieConfirmations.set(pid, now);
return true;
}

export async function assertDaemonSupervisorOwnerCurrent(
owner: {
generation: string;
Expand All @@ -526,7 +551,7 @@ export async function assertDaemonSupervisorOwnerCurrent(
current.pid !== owner.pid ||
current.processStartId !== owner.processStartId ||
current.socketPath !== normalizeSocketPath(owner.socketPath) ||
!isProcessAlive(current.pid)
!isOwnerProcessAlive(current.pid)
) {
throw new DaemonSupervisorOwnershipLostError(owner.generation, { socketPath: owner.socketPath, registryDir });
}
Expand Down Expand Up @@ -693,15 +718,6 @@ function matchesExactProcessIdentity(identity: ProcessIdentity): boolean {
return identity.processStartId === undefined || getProcessStartId(identity.pid) === identity.processStartId;
}

function isProcessAlive(pid: number): boolean {
try {
process.kill(pid, 0);
} catch (error) {
return (error as NodeJS.ErrnoException).code !== "ESRCH";
}
return true;
}

function canonicalizeFilesystemPath(path: string): string {
let existingAncestor = resolve(path);
const missingSuffix: string[] = [];
Expand Down
16 changes: 16 additions & 0 deletions packages/coding-agent/src/utils/child-process.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,22 @@ export function isProcessAlive(pid: number): boolean {
return processIdExists(pid) && !isZombieProcess(pid);
}

/**
* True while the process group has any member left (zombies included). A group
* can outlive its leader, so group teardown must not complete on leader death.
*/
export function processGroupExists(pgid: number): boolean {
if (process.platform === "win32") {
return false;
}
try {
process.kill(-pgid, 0);
return true;
} catch (error) {
return (error as NodeJS.ErrnoException).code === "EPERM";
}
}

export function signalProcessGroupOrProcess(pid: number, signal: NodeJS.Signals): void {
try {
process.kill(-pid, signal);
Expand Down
35 changes: 34 additions & 1 deletion packages/coding-agent/test/child-process.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,13 @@
import { type ChildProcess, spawn } from "node:child_process";
import { EventEmitter } from "node:events";
import { describe, expect, it } from "vitest";
import { isProcessAlive, isZombieProcess, waitForChildProcess } from "../src/utils/child-process.js";
import {
isProcessAlive,
isZombieProcess,
processGroupExists,
signalProcessGroupOrProcess,
waitForChildProcess,
} from "../src/utils/child-process.js";

describe("waitForChildProcess", () => {
it("reports signaled already-exited children as failures", async () => {
Expand Down Expand Up @@ -58,4 +64,31 @@ describe("process liveness", () => {
parent.kill("SIGKILL");
}
});

it.skipIf(process.platform === "win32")("keeps a process group alive after its leader exits", async () => {
const childless = spawn("sh", ["-c", "exit 0"], { detached: true, stdio: "ignore" });
const childlessExited = new Promise<void>((resolveExit) => childless.once("exit", () => resolveExit()));
const leader = spawn("sh", ["-c", "sleep 30 & echo started"], {
detached: true,
stdio: ["ignore", "pipe", "ignore"],
});
const leaderExited = new Promise<void>((resolveExit) => leader.once("exit", () => resolveExit()));
const pgid = leader.pid!;
try {
await new Promise<void>((resolveStart, rejectStart) => {
const timer = setTimeout(() => rejectStart(new Error("Timed out waiting for the group member")), 5000);
leader.stdout?.once("data", () => {
clearTimeout(timer);
resolveStart();
});
});
await leaderExited;
expect(isProcessAlive(pgid)).toBe(false);
expect(processGroupExists(pgid)).toBe(true);
await childlessExited;
expect(processGroupExists(childless.pid!)).toBe(false);
} finally {
signalProcessGroupOrProcess(pgid, "SIGKILL");
}
});
});
74 changes: 73 additions & 1 deletion packages/coding-agent/test/daemon-supervisor-ownership.test.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,19 @@
import { spawn } from "node:child_process";
import { existsSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, rmSync, writeFileSync } from "node:fs";
import { tmpdir } from "node:os";
import { join } from "node:path";
import lockfile from "proper-lockfile";
import { afterEach, describe, expect, it } from "vitest";
import { afterEach, describe, expect, it, vi } from "vitest";
import { getProcessStartId } from "../src/core/session-lease.js";
import { DaemonSupervisor } from "../src/modes/daemon/daemon-supervisor.js";
import {
acquireDaemonShutdownAdmission,
acquireDaemonSupervisorOwnership,
assertDaemonSupervisorOwnerCurrent,
persistDaemonStartupFenceFromOwner,
} from "../src/modes/daemon/daemon-supervisor-ownership.js";
import * as childProcessModule from "../src/utils/child-process.js";
import { isZombieProcess } from "../src/utils/child-process.js";

type Ownership = Awaited<ReturnType<typeof acquireDaemonSupervisorOwnership>>;

Expand Down Expand Up @@ -175,6 +179,74 @@ describe("daemon supervisor ownership registry", () => {
await reaped.release();
});

it("confirms owner zombie state at most once per interval across fence polls", async () => {
const paths = createPaths();
const owner = await acquire(paths, "fence-owner");
const claim = {
generation: owner.record.generation,
pid: owner.record.pid,
processStartId: owner.record.processStartId,
socketPath: owner.record.socketPath,
};
const fingerprint = await assertDaemonSupervisorOwnerCurrent(claim, undefined, paths.registryDir);
const zombieSpy = vi.spyOn(childProcessModule, "isZombieProcess");
const aliveSpy = vi.spyOn(childProcessModule, "isProcessAlive");
try {
// Steady-state fence polls (validated fingerprint, confirmed owner) must
// not run a ps-backed probe per 250ms tick; existence stays kill(0)-cheap.
await assertDaemonSupervisorOwnerCurrent(claim, fingerprint, paths.registryDir);
await assertDaemonSupervisorOwnerCurrent(claim, fingerprint, paths.registryDir);
expect(zombieSpy.mock.calls.length + aliveSpy.mock.calls.length).toBe(0);
} finally {
zombieSpy.mockRestore();
aliveSpy.mockRestore();
await owner.release();
}
});

it.skipIf(process.platform === "win32")("treats a zombie owner process as reclaimable", async () => {
const paths = createPaths();
const zombieParent = spawn(
"perl",
["-e", '$| = 1; my $pid = fork(); if ($pid) { print "$pid\\n"; sleep 30 } else { exit 0 }'],
{ stdio: ["ignore", "pipe", "ignore"] },
);
try {
const zombiePid = await new Promise<number>((resolvePid, rejectPid) => {
let output = "";
const timer = setTimeout(() => rejectPid(new Error("Timed out waiting for the zombie pid")), 5000);
zombieParent.stdout?.on("data", (chunk: Buffer) => {
output += chunk.toString();
const parsed = Number.parseInt(output.trim(), 10);
if (Number.isInteger(parsed) && parsed > 0) {
clearTimeout(timer);
resolvePid(parsed);
}
});
});
const deadline = Date.now() + 5000;
while (!isZombieProcess(zombiePid) && Date.now() < deadline) {
await new Promise((resolveDelay) => setTimeout(resolveDelay, 25));
}
expect(isZombieProcess(zombiePid)).toBe(true);

const stale = await acquire(paths, "zombie-owner");
const ownerPath = join(ownerDir(paths, "zombie-owner"), "owner.json");
const record = readJson(ownerPath);
record.pid = zombiePid;
const zombieStartId = getProcessStartId(zombiePid);
if (zombieStartId) record.processStartId = zombieStartId;
else delete record.processStartId;
writeFileSync(ownerPath, `${JSON.stringify(record, null, 2)}\n`);

const successor = await acquire(paths, "successor-owner");
await successor.release();
await stale.release();
} finally {
zombieParent.kill("SIGKILL");
}
});

it("does not resurrect the shutdown admission when release overtakes an in-flight renew", async () => {
const paths = createPaths();
mkdirSync(paths.registryDir, { recursive: true, mode: 0o700 });
Expand Down
Loading