mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-09 16:02:36 +01:00
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The runner E2E suite restarts the Paperclip server to prove that agent work recovers after a controller restart > - The restart supervisor in `tests/runner-e2e` owns the server process tree and escalates to SIGKILL when graceful stop takes too long > - On macOS, `kill(-pgid)` returns EPERM, not ESRCH, when every member of the group has exited but is not reaped yet > - The supervisor treated that EPERM as fatal, so a server that exited at the same moment as the SIGKILL escalation aborted the restart with "Owned tree cleanup incomplete: Error: kill EPERM" > - This pull request makes the supervisor re-inspect the process table on EPERM and skip the group only when no running member remains > - The benefit is that restart recovery cases no longer fail on a harmless exit race on macOS, while a real permission failure still fails cleanup ## Linked Issues or Issue Description No public issue exists. This describes the bug. **What happened?** The runner E2E cell `everyday-workflows.runner-codex.local.recover-controller` failed on macOS with failure class `cleanup_failure`: "Stopped waiting for isolated server restart ...: Owned tree cleanup incomplete: Error: kill EPERM". The server log shows that the graceful stop took 30 seconds, the supervisor sent SIGKILL, and the server exited with code 0 in the same instant. The restart did not start, so the case never checked recovery. **Steps to reproduce** 1. On macOS, fork a child that calls `setpgid(0, 0)` and exits. Do not reap it. 2. Call `killpg(child, SIGKILL)`. The kernel returns EPERM (errno 1), not ESRCH. After the reap, it returns ESRCH. 3. In the runner E2E supervisor, any group that becomes zombie-only between the `ps` inspection and the signal causes the same EPERM, which the code rethrows. **Expected behavior** A group whose members have all exited counts as stopped, the same as ESRCH. EPERM stays fatal while a running member remains. **Environment** macOS 26.5 (Darwin 25.5.0), arm64, Node 26.4. The behavior is the same with and without the Claude Code sandbox. ## What Changed - `tests/runner-e2e/process-tree-owner.ts`: on EPERM from a group signal, read a fresh process table. Skip the group (as with ESRCH) only when it has no running (non-zombie) member. Otherwise rethrow. `signalSnapshot` is now async. - `tests/runner-e2e/process-tree-owner.test.ts`: two regressions. One covers a group that becomes zombie-only when signaled. One proves that EPERM stays fatal while a running member remains. - `tests/runner-e2e/README.md`: document the EPERM rule next to the existing zombie rule. ## Verification - Red before the fix: `node node_modules/vitest/vitest.mjs run --config tests/runner-e2e/vitest.config.ts tests/runner-e2e/process-tree-owner.test.ts` failed with `promise rejected "Error: kill EPERM"`. - Green after the fix: `process-tree-owner.test.ts`, `server-stop.test.ts`, and `process-tree.test.ts` pass (35 tests). `pnpm test:e2e:runner:typecheck` passes. - Kernel behavior: a Python fork probe on this Mac shows that `killpg` on an unreaped zombie group returns EPERM, both inside and outside the sandbox. - Live cell: `pnpm test:e2e:runner -- --max-automatic-retries 0 --id everyday-workflows.runner-codex.local.recover-controller` on macOS. The fixed revision passed. The server stop escalated to SIGKILL, the replacement server started, and the run recovered. See the comment below for the second rerun. ## Risks - Low risk. The change is in the test harness only. - EPERM is ignored only after a fresh inspection shows that no running process holds that group ID. A process group ID cannot be reused while zombie members still hold it, so the supervisor does not signal or skip an unrelated group. - The extra `ps` read runs only on EPERM. ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`), Anthropic, via Claude Code with tool use (shell, file edits). Default context window. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
281 lines
15 KiB
TypeScript
281 lines
15 KiB
TypeScript
import type { ChildProcess } from "node:child_process";
|
|
import {
|
|
observeDescendantProcessTree, readProcessTable, revalidateObservedProcessGroups,
|
|
refreshContinuouslyLiveProcessGroups, safeProcessGroupTerminationOrder,
|
|
type ObservedProcessGroup, type ProcessObservation,
|
|
} from "./process-tree.js";
|
|
|
|
/** Supplied by the company-scoped run API, never by process-name discovery. */
|
|
export interface RestartRunnerIdentity {
|
|
processPid: number;
|
|
processGroupId: number;
|
|
processStartedAt: string;
|
|
}
|
|
export function parseRestartRunnerIdentity(value: unknown): RestartRunnerIdentity {
|
|
const v = value as RestartRunnerIdentity | null;
|
|
if (!v || !Number.isSafeInteger(v.processPid) || v.processPid <= 1
|
|
|| v.processGroupId !== v.processPid || typeof v.processStartedAt !== "string"
|
|
|| !Number.isFinite(Date.parse(v.processStartedAt))) throw new Error("Invalid restart runner identity");
|
|
return { processPid: v.processPid, processGroupId: v.processGroupId, processStartedAt: v.processStartedAt };
|
|
}
|
|
|
|
/** Linux run receipts retain milliseconds; ps lstart retains whole seconds. */
|
|
export function restartProcessStartMatches(observed: string, expected: string, platform = process.platform): boolean {
|
|
const actualTime = Date.parse(observed), expectedTime = Date.parse(expected);
|
|
if (!Number.isFinite(actualTime) || !Number.isFinite(expectedTime)) return false;
|
|
return platform === "linux"
|
|
? Math.floor(actualTime / 1000) === Math.floor(expectedTime / 1000)
|
|
: actualTime === expectedTime;
|
|
}
|
|
|
|
export function createProcessTreeOwner(root: ChildProcess, options: {
|
|
readTable?: () => Promise<ProcessObservation[] | null>;
|
|
signalGroup?: (pid: number, signal: NodeJS.Signals) => void;
|
|
} = {}) {
|
|
let groups: ObservedProcessGroup[] = [];
|
|
let table: ProcessObservation[] = [];
|
|
let rootStarted: string | undefined;
|
|
let preserved: ObservedProcessGroup[] = [];
|
|
let restartRunner: ProcessObservation | undefined;
|
|
let observing: Promise<void> | undefined;
|
|
const covered = new Set<number>();
|
|
const readTable = options.readTable ?? readProcessTable;
|
|
const signalGroup = options.signalGroup ?? ((pid, signal) => process.kill(-pid, signal));
|
|
const exited = () => root.exitCode !== null || root.signalCode !== null || !root.pid;
|
|
const running = (candidate: ProcessObservation) => !candidate.state?.startsWith("Z");
|
|
|
|
function observe(): Promise<void> {
|
|
if (process.platform === "win32") return Promise.resolve();
|
|
if (observing) return observing;
|
|
observing = (async () => {
|
|
const next = await readTable();
|
|
if (!next) throw new Error("Could not inspect owned process identities");
|
|
const callerGroup = next.find(row => row.pid === process.pid)?.processGroupId;
|
|
if (callerGroup === undefined) throw new Error("Cleanup caller group identity is unavailable");
|
|
const rootRow = next.find(row => row.pid === root.pid);
|
|
if (!rootStarted && !exited()) rootStarted = rootRow?.started;
|
|
const retained = revalidateObservedProcessGroups(groups, next);
|
|
const trustedAnchors = retained.flatMap(group => group.members
|
|
.filter(member => next.some(row => row.pid === member.pid && row.started === member.started
|
|
&& row.processGroupId === group.processGroupId)).map(member => member.pid));
|
|
if (rootRow && rootStarted === rootRow.started) trustedAnchors.push(rootRow.pid);
|
|
const ownedNow = new Set(trustedAnchors.flatMap(pid =>
|
|
observeDescendantProcessTree(next, pid).members.map(member => member.process.pid)));
|
|
// A short-lived leader can exit between polls while its replacement
|
|
// remains a descendant of a separately validated owner. Admit that
|
|
// ancestry, never a recycled numeric group or a mixed ownership group.
|
|
const lost = groups.filter(group => !retained.includes(group) && next.some(row => row.processGroupId === group.processGroupId && running(row)));
|
|
const uncertain = lost.filter(group => next.some(row => row.processGroupId === group.processGroupId
|
|
&& running(row) && !ownedNow.has(row.pid)));
|
|
if (uncertain.length) throw new Error(`Owned process group identity became uncertain: ${uncertain.map(group => group.processGroupId).join(",")}`);
|
|
const refreshed = refreshContinuouslyLiveProcessGroups([...retained, ...lost], next)
|
|
.filter(group => group.processGroupId !== callerGroup);
|
|
const anchors = refreshed.flatMap(group => group.members.map(member => member.pid));
|
|
if (rootRow && rootStarted === rootRow.started) anchors.push(rootRow.pid);
|
|
const byGroup = new Map(refreshed.map(group => [group.processGroupId, group]));
|
|
for (const pid of anchors) {
|
|
for (const group of observeDescendantProcessTree(next, pid).groups) {
|
|
if (group.processGroupId !== callerGroup) {
|
|
// A descendant traversal can cover only part of an already
|
|
// validated group. Keep every observed peer as an identity anchor.
|
|
const previous = byGroup.get(group.processGroupId);
|
|
const members = new Map(previous?.members.map(member => [member.pid, member]) ?? []);
|
|
for (const member of group.members) members.set(member.pid, member);
|
|
byGroup.set(group.processGroupId, { ...group,
|
|
depth: Math.max(group.depth, previous?.depth ?? group.depth),
|
|
members: [...members.values()],
|
|
});
|
|
if (covered.has(next.find(row => row.pid === pid)!.processGroupId)) covered.add(group.processGroupId);
|
|
}
|
|
}
|
|
}
|
|
groups = [...byGroup.values()];
|
|
// Follow only continuously owned members of the admitted daemon tree.
|
|
// Orphaned descendants stay owned, but unrelated controller children do not.
|
|
const retainedPreserved = refreshContinuouslyLiveProcessGroups(revalidateObservedProcessGroups(preserved, next), next);
|
|
const preservedIds = new Set(retainedPreserved.map(group => group.processGroupId));
|
|
for (const member of retainedPreserved.flatMap(group => group.members)) {
|
|
for (const group of observeDescendantProcessTree(next, member.pid).groups) preservedIds.add(group.processGroupId);
|
|
}
|
|
preserved = groups.filter(group => preservedIds.has(group.processGroupId));
|
|
table = next;
|
|
})().finally(() => { observing = undefined; });
|
|
return observing;
|
|
}
|
|
const timer = process.platform === "win32" ? undefined : setInterval(() => {
|
|
void observe().catch(() => { /* Stop must obtain a fresh successful inspection. */ });
|
|
}, 250);
|
|
timer?.unref();
|
|
void observe().catch(() => {});
|
|
|
|
function liveGroups() {
|
|
return groups.filter(group => table.some(row => row.processGroupId === group.processGroupId && running(row)));
|
|
}
|
|
async function preserveRunnerForRestart(identity: RestartRunnerIdentity) {
|
|
const expected = parseRestartRunnerIdentity(identity);
|
|
await observe();
|
|
if (restartRunner || exited()) throw new Error("Restart runner admission must precede controller exit");
|
|
const tree = observeDescendantProcessTree(table, root.pid!);
|
|
const candidate = tree.members.map(member => member.process).find(row => row.pid === expected.processPid);
|
|
if (!candidate || !running(candidate) || candidate.kind !== "paperclip-runnerd"
|
|
|| candidate.processGroupId !== expected.processGroupId
|
|
|| !restartProcessStartMatches(candidate.started, expected.processStartedAt)
|
|
|| candidate.processGroupId === root.pid) throw new Error("Restart runner is not the exact owned durable daemon");
|
|
const subtree = observeDescendantProcessTree(table, candidate.pid);
|
|
const members = new Set(subtree.members.map(member => member.process.pid));
|
|
const ids = new Set(subtree.groups.map(group => group.processGroupId));
|
|
if (table.some(row => ids.has(row.processGroupId) && !members.has(row.pid) && running(row))) {
|
|
throw new Error("Restart runner shares a process group with an unrelated process");
|
|
}
|
|
restartRunner = { ...candidate };
|
|
preserved = groups.filter(group => ids.has(group.processGroupId));
|
|
}
|
|
function assertRestartRunnerAlive() {
|
|
if (!restartRunner || !table.some(row => row.pid === restartRunner!.pid && row.started === restartRunner!.started
|
|
&& row.processGroupId === restartRunner!.processGroupId && running(row))) throw new Error("Admitted restart runner did not survive controller restart");
|
|
}
|
|
function restartCleanupGroups() {
|
|
const ids = new Set(preserved.map(group => group.processGroupId));
|
|
return new Set(liveGroups().filter(group => !ids.has(group.processGroupId)).map(group => group.processGroupId));
|
|
}
|
|
/** Darwin killpg reports EPERM, not ESRCH, when every remaining group member
|
|
* has exited but is not yet reaped. A zombie-only group needs no signal; any
|
|
* running member keeps EPERM a real failure. */
|
|
async function exitedSinceSnapshot(pid: number) {
|
|
const fresh = await readTable();
|
|
return !!fresh && !fresh.some(row => row.processGroupId === pid && running(row));
|
|
}
|
|
async function signalSnapshot(signal: NodeJS.Signals, selected?: ReadonlySet<number>) {
|
|
const currentProcessGroupId = table.find(row => row.pid === process.pid)?.processGroupId ?? null;
|
|
if (currentProcessGroupId === null) throw new Error("Cleanup caller group identity is unavailable");
|
|
const live = liveGroups();
|
|
const ordered = safeProcessGroupTerminationOrder({ rootProcessGroupId: root.pid ?? -1, currentProcessGroupId, groups: live });
|
|
const signaled: number[] = [];
|
|
for (const pid of ordered) {
|
|
if (selected && !selected.has(pid)) continue;
|
|
try { signalGroup(pid, signal); }
|
|
catch (error) {
|
|
const code = (error as NodeJS.ErrnoException).code;
|
|
if (code === "ESRCH" || (code === "EPERM" && await exitedSinceSnapshot(pid))) continue;
|
|
throw error;
|
|
}
|
|
signaled.push(pid);
|
|
}
|
|
return signaled;
|
|
}
|
|
async function signal(signal: NodeJS.Signals, selected?: ReadonlySet<number>) {
|
|
await observe();
|
|
return await signalSnapshot(signal, selected);
|
|
}
|
|
function gracefulRoots() {
|
|
const live = liveGroups();
|
|
const ids = new Set(live.map(group => group.processGroupId));
|
|
if (root.pid && ids.has(root.pid)) return new Set([root.pid]);
|
|
// After the launcher's leader exits, request shutdown only from the
|
|
// outermost remaining owners; their wrappers still own child signaling.
|
|
const byPid = new Map(table.map(row => [row.pid, row]));
|
|
return new Set(live.filter(group => !table.some(row => {
|
|
if (row.processGroupId !== group.processGroupId) return false;
|
|
const seen = new Set<number>();
|
|
let parent = byPid.get(row.parentPid);
|
|
while (parent && !seen.has(parent.pid)) {
|
|
seen.add(parent.pid);
|
|
if (parent.processGroupId !== group.processGroupId && ids.has(parent.processGroupId)) return true;
|
|
parent = byPid.get(parent.parentPid);
|
|
}
|
|
return false;
|
|
})).map(group => group.processGroupId));
|
|
}
|
|
let graceComplete = false;
|
|
let directGraceDelivered = false;
|
|
async function signalGracefully(signal: "SIGTERM" | "SIGINT" = "SIGTERM") {
|
|
if (graceComplete) return;
|
|
await observe();
|
|
// Choose and deliver from one validated table. ESRCH grants no ownership
|
|
// coverage: the next grace poll can select that branch's surviving owner.
|
|
const selected = new Set([...gracefulRoots()].filter(pid => !covered.has(pid)));
|
|
const delivered = await signalSnapshot(signal, selected);
|
|
for (const pid of delivered) {
|
|
if (pid === root.pid) directGraceDelivered = true;
|
|
covered.add(pid);
|
|
for (const member of table.filter(row => row.processGroupId === pid)) {
|
|
for (const group of observeDescendantProcessTree(table, member.pid).groups) covered.add(group.processGroupId);
|
|
}
|
|
}
|
|
// Never reselect descendants after their owner received grace, even if it
|
|
// exits before their asynchronous close completes.
|
|
graceComplete = selected.size > 0 && delivered.length === selected.size;
|
|
}
|
|
return { observe, signal, liveGroups, gracefulRoots, signalGracefully,
|
|
preserveRunnerForRestart, assertRestartRunnerAlive, restartCleanupGroups,
|
|
directGraceDelivered: () => directGraceDelivered, stopObserving: () => { if (timer) clearInterval(timer); } };
|
|
}
|
|
|
|
|
|
/** Inspection failure grants no group authority. The unreaped ChildProcess
|
|
* handle still owns its direct child, so retire only that child within the
|
|
* original grace deadline, then report the incomplete tree audit. */
|
|
export async function stopKnownDirectChild(child: ChildProcess, phase: { gracefulSent: boolean; forcedDeadline?: number },
|
|
deadline: number, forcedMs: number, signal: NodeJS.Signals = "SIGTERM") {
|
|
const exited = () => child.exitCode !== null || child.signalCode !== null || !child.pid;
|
|
const wait = () => new Promise(resolve => setTimeout(resolve, 25));
|
|
if (!exited() && !phase.gracefulSent) {
|
|
phase.gracefulSent = child.kill(signal);
|
|
}
|
|
while (!exited() && Date.now() < deadline) await wait();
|
|
if (exited()) return;
|
|
child.kill("SIGKILL");
|
|
phase.forcedDeadline ??= Date.now() + forcedMs;
|
|
while (!exited() && Date.now() < phase.forcedDeadline) await wait();
|
|
if (!exited()) throw new Error("Known direct child remained after bounded fallback");
|
|
}
|
|
|
|
export async function incompleteTreeFallback(error: unknown, child: ChildProcess,
|
|
phase: { gracefulSent: boolean; forcedDeadline?: number }, deadline: number, forcedMs: number,
|
|
signal: "SIGTERM" | "SIGINT" = "SIGTERM") {
|
|
let fallbackError: unknown;
|
|
try { await stopKnownDirectChild(child, phase, deadline, forcedMs, signal); }
|
|
catch (failure) { fallbackError = failure; }
|
|
throw new Error(`Owned tree cleanup incomplete: ${String(error)}${fallbackError ? `; ${String(fallbackError)}` : ""}`);
|
|
}
|
|
|
|
/** Give the launcher/wrapper chain sole graceful-signal ownership, then retire
|
|
* only still-observed descendants. Used on normal exit as well as cancellation. */
|
|
export async function stopOwnedProcessTree(
|
|
child: ChildProcess,
|
|
owner: ReturnType<typeof createProcessTreeOwner>,
|
|
gracefulMs = 45_000,
|
|
forcedMs = 5_000,
|
|
gracefulSignal: "SIGTERM" | "SIGINT" = "SIGTERM",
|
|
): Promise<void> {
|
|
const wait = (ms: number) => new Promise(resolve => setTimeout(resolve, ms));
|
|
const deadline = Date.now() + gracefulMs;
|
|
const phase: { gracefulSent: boolean; forcedDeadline?: number } = { gracefulSent: false };
|
|
try {
|
|
if (process.platform === "win32") {
|
|
await stopKnownDirectChild(child, phase, deadline, forcedMs, gracefulSignal);
|
|
return;
|
|
}
|
|
const stopped = async () => {
|
|
await owner.observe();
|
|
return (child.exitCode !== null || child.signalCode !== null) && owner.liveGroups().length === 0;
|
|
};
|
|
do {
|
|
await owner.signalGracefully(gracefulSignal);
|
|
phase.gracefulSent ||= owner.directGraceDelivered();
|
|
if (await stopped()) return;
|
|
await wait(50);
|
|
} while (Date.now() < deadline);
|
|
await owner.signal("SIGKILL");
|
|
phase.forcedDeadline = Date.now() + forcedMs;
|
|
do {
|
|
if (await stopped()) return;
|
|
await wait(50);
|
|
} while (Date.now() < phase.forcedDeadline);
|
|
throw new Error("Observed launcher descendants remained after bounded cleanup");
|
|
} catch (error) {
|
|
phase.gracefulSent ||= owner.directGraceDelivered();
|
|
await incompleteTreeFallback(error, child, phase, deadline, forcedMs, gracefulSignal);
|
|
}
|
|
}
|