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>
212 lines
10 KiB
TypeScript
212 lines
10 KiB
TypeScript
import { spawn, type ChildProcess } from "node:child_process";
|
|
import { expect, it } from "vitest";
|
|
import { createProcessTreeOwner } from "./process-tree-owner.js";
|
|
import { readProcessTable, type ProcessObservation } from "./process-tree.js";
|
|
|
|
function row(pid: number, parentPid: number, processGroupId: number, started = `start-${pid}`, state = "S"): ProcessObservation {
|
|
return { pid, parentPid, processGroupId, started, state, kind: "node" };
|
|
}
|
|
function fixture(initial: ProcessObservation[]) {
|
|
let table = [row(process.pid, 1, 10), ...initial];
|
|
const root = { pid: 100, exitCode: null as number | null, signalCode: null as NodeJS.Signals | null };
|
|
const signals: Array<[number, NodeJS.Signals]> = [];
|
|
const owner = createProcessTreeOwner(root as ChildProcess, {
|
|
readTable: async () => table,
|
|
signalGroup: (pid, signal) => { signals.push([pid, signal]); },
|
|
});
|
|
return { root, owner, signals, replace: (rows: ProcessObservation[]) => { table = [row(process.pid, 1, 10), ...rows]; } };
|
|
}
|
|
|
|
it.skipIf(process.platform === "win32")("revalidates PID/start before signaling and refuses a recycled group", async () => {
|
|
const f = fixture([row(100, process.pid, 100), row(200, 100, 200)]);
|
|
try {
|
|
await f.owner.observe();
|
|
f.replace([row(100, process.pid, 100), row(200, 1, 200, "new-start")]);
|
|
await expect(f.owner.signal("SIGKILL")).rejects.toThrow("identity became uncertain");
|
|
expect(f.signals).toEqual([]);
|
|
} finally { f.owner.stopObserving(); }
|
|
});
|
|
|
|
it.skipIf(process.platform === "win32")("keeps surviving peer identities when a descendant traversal ends at a short-lived child", async () => {
|
|
const f = fixture([row(100, process.pid, 100), row(101, 100, 100), row(200, 100, 200), row(201, 200, 200)]);
|
|
try {
|
|
await f.owner.observe();
|
|
await f.owner.observe();
|
|
f.replace([row(100, process.pid, 100), row(200, 100, 200)]);
|
|
await f.owner.signal("SIGKILL");
|
|
expect(f.signals).toEqual([[200, "SIGKILL"], [100, "SIGKILL"]]);
|
|
} finally { f.owner.stopObserving(); }
|
|
});
|
|
|
|
it.skipIf(process.platform === "win32")("admits replacement group members only through a still-owned ancestor", async () => {
|
|
const f = fixture([row(100, process.pid, 100), row(200, 100, 200)]);
|
|
try {
|
|
await f.owner.observe();
|
|
// The short-lived group leader exited between polls. Its new member is
|
|
// independently owned through the original, still-live controller.
|
|
f.replace([row(100, process.pid, 100), row(300, 100, 200)]);
|
|
await f.owner.signal("SIGKILL");
|
|
expect(f.signals).toEqual([[200, "SIGKILL"], [100, "SIGKILL"]]);
|
|
} finally { f.owner.stopObserving(); }
|
|
});
|
|
|
|
it.skipIf(process.platform === "win32")("keeps surviving peers after the launcher exits without admitting an unobserved group", async () => {
|
|
const f = fixture([row(100, process.pid, 100), row(200, 100, 200), row(201, 200, 200)]);
|
|
try {
|
|
await f.owner.observe();
|
|
f.root.exitCode = 0;
|
|
f.replace([row(200, 1, 200), row(201, 200, 200)]);
|
|
await f.owner.observe();
|
|
f.replace([row(200, 1, 200), row(999, 1, 999)]);
|
|
await f.owner.signal("SIGTERM");
|
|
expect(f.signals).toEqual([[200, "SIGTERM"]]);
|
|
} finally { f.owner.stopObserving(); }
|
|
});
|
|
|
|
it.skipIf(process.platform === "win32")("refuses replacement groups containing any unowned live member", async () => {
|
|
const f = fixture([row(100, process.pid, 100), row(200, 100, 200)]);
|
|
try {
|
|
await f.owner.observe();
|
|
f.replace([row(100, process.pid, 100), row(300, 100, 200), row(400, 1, 200)]);
|
|
await expect(f.owner.signal("SIGKILL")).rejects.toThrow("identity became uncertain");
|
|
expect(f.signals).toEqual([]);
|
|
} finally { f.owner.stopObserving(); }
|
|
});
|
|
|
|
it.skipIf(process.platform === "win32")("does not use a recycled controller as a replacement member's ownership anchor", async () => {
|
|
const f = fixture([row(100, process.pid, 100), row(200, 100, 200)]);
|
|
try {
|
|
await f.owner.observe();
|
|
f.replace([row(100, process.pid, 100, "recycled-controller"), row(300, 100, 200)]);
|
|
await expect(f.owner.signal("SIGKILL")).rejects.toThrow("identity became uncertain");
|
|
expect(f.signals).toEqual([]);
|
|
} finally { f.owner.stopObserving(); }
|
|
});
|
|
|
|
it.skipIf(process.platform === "win32")("retains observed descendants when the root exits and excludes the caller group", async () => {
|
|
const f = fixture([row(100, process.pid, 100), row(200, 100, 200), row(300, 100, 10), row(999, process.pid, 999)]);
|
|
try {
|
|
await f.owner.observe();
|
|
f.root.exitCode = 1;
|
|
f.replace([row(200, 1, 200), row(300, 1, 10), row(999, process.pid, 999)]);
|
|
await f.owner.signal("SIGTERM");
|
|
expect(f.signals).toEqual([[200, "SIGTERM"]]);
|
|
} finally { f.owner.stopObserving(); }
|
|
});
|
|
|
|
it.skipIf(process.platform === "win32")("treats an unreaped zombie as stopped without signaling a replacement", async () => {
|
|
const f = fixture([row(100, process.pid, 100), row(200, 100, 200)]);
|
|
try {
|
|
await f.owner.observe();
|
|
f.root.exitCode = 1;
|
|
f.replace([row(200, 1, 200, "start-200", "Z")]);
|
|
await f.owner.signal("SIGKILL");
|
|
expect(f.owner.liveGroups()).toEqual([]);
|
|
expect(f.signals).toEqual([]);
|
|
} finally { f.owner.stopObserving(); }
|
|
});
|
|
|
|
it.skipIf(process.platform === "win32")("sends launcher grace only to the root while retaining nested groups for escalation", async () => {
|
|
const f = fixture([row(100, process.pid, 100), row(200, 100, 200), row(300, 200, 300)]);
|
|
try {
|
|
await f.owner.observe();
|
|
await f.owner.signal("SIGTERM", f.owner.gracefulRoots());
|
|
expect(f.signals).toEqual([[100, "SIGTERM"]]);
|
|
f.root.exitCode = 1;
|
|
f.replace([row(200, 1, 200), row(300, 200, 300)]);
|
|
await f.owner.observe();
|
|
expect([...f.owner.gracefulRoots()]).toEqual([200]);
|
|
await f.owner.signal("SIGKILL");
|
|
expect(f.signals.slice(1).map(([pid]) => pid).sort()).toEqual([200, 300]);
|
|
} finally { f.owner.stopObserving(); }
|
|
});
|
|
|
|
it.skipIf(process.platform === 'win32')('reselects a vanished owner only when no graceful signal was delivered', async () => {
|
|
let table = [row(process.pid, 1, 10), row(100, process.pid, 100), row(200, 100, 200)];
|
|
const child = { pid: 100, exitCode: null as number | null, signalCode: null };
|
|
const delivered: number[] = [];
|
|
let reads = 0;
|
|
let stopping = false;
|
|
const owner = createProcessTreeOwner(child as ChildProcess, {
|
|
readTable: async () => {
|
|
if (stopping && ++reads === 2) { child.exitCode = 0; table = [row(process.pid, 1, 10), row(200, 1, 200)]; }
|
|
return table;
|
|
},
|
|
signalGroup: (pid, signal) => {
|
|
expect(signal).toBe('SIGTERM');
|
|
if (pid === 100) {
|
|
child.exitCode = 0; table = [row(process.pid, 1, 10), row(200, 1, 200)];
|
|
throw Object.assign(new Error('vanished'), { code: 'ESRCH' });
|
|
}
|
|
delivered.push(pid); table = [row(process.pid, 1, 10)];
|
|
},
|
|
});
|
|
try {
|
|
await owner.observe(); stopping = true;
|
|
const { stopOwnedProcessTree } = await import('./process-tree-owner.js');
|
|
await stopOwnedProcessTree(child as ChildProcess, owner, 150, 100);
|
|
expect(delivered).toEqual([200]);
|
|
} finally { owner.stopObserving(); }
|
|
});
|
|
|
|
|
|
it.skipIf(process.platform === "win32")("rejects an overflowing inspector instead of returning a partial identity table", async () => {
|
|
const inspector = spawn(process.execPath, ["-e", "process.stdout.write('x'.repeat(2 * 1024 * 1024)); setInterval(() => {}, 1000)"], {
|
|
stdio: ["ignore", "pipe", "ignore"],
|
|
});
|
|
const exited = new Promise<void>(resolve => inspector.once("exit", () => resolve()));
|
|
try {
|
|
expect(await readProcessTable(() => inspector)).toBeNull();
|
|
await exited;
|
|
expect(inspector.signalCode).toBe("SIGKILL");
|
|
} finally { if (inspector.exitCode === null && inspector.signalCode === null) inspector.kill("SIGKILL"); }
|
|
});
|
|
|
|
it.skipIf(process.platform === 'win32')('bounds fallback even when the known direct child cannot be killed', async () => {
|
|
const signals: NodeJS.Signals[] = [];
|
|
const child = { pid: 100, exitCode: null, signalCode: null,
|
|
kill: (signal: NodeJS.Signals) => { signals.push(signal); return false; } } as ChildProcess;
|
|
const owner = createProcessTreeOwner(child, { readTable: async () => null,
|
|
signalGroup: () => { throw new Error('must not signal an unobserved group'); } });
|
|
try {
|
|
const { stopOwnedProcessTree } = await import('./process-tree-owner.js');
|
|
await expect(stopOwnedProcessTree(child, owner, 10, 10)).rejects.toThrow('Known direct child remained');
|
|
expect(signals).toEqual(['SIGTERM', 'SIGKILL']);
|
|
} finally { owner.stopObserving(); }
|
|
});
|
|
|
|
function epermFixture(afterSignal: ProcessObservation[]) {
|
|
let table = [row(process.pid, 1, 10), row(100, process.pid, 100), row(200, 100, 200)];
|
|
const root = { pid: 100, exitCode: null as number | null, signalCode: null as NodeJS.Signals | null };
|
|
const attempts: Array<[number, NodeJS.Signals]> = [];
|
|
const owner = createProcessTreeOwner(root as ChildProcess, {
|
|
readTable: async () => table,
|
|
signalGroup: (pid, signal) => {
|
|
attempts.push([pid, signal]);
|
|
if (pid !== 200) return;
|
|
// Darwin killpg reports EPERM, not ESRCH, once every group member exited
|
|
// but is not reaped yet. The group exits between inspection and signal.
|
|
table = [row(process.pid, 1, 10), row(100, process.pid, 100), ...afterSignal];
|
|
throw Object.assign(new Error("kill EPERM"), { code: "EPERM" });
|
|
},
|
|
});
|
|
return { owner, attempts };
|
|
}
|
|
|
|
it.skipIf(process.platform === "win32")("treats EPERM from a group that is now only zombies as already exited", async () => {
|
|
const f = epermFixture([row(200, 100, 200, "start-200", "Z")]);
|
|
try {
|
|
await f.owner.observe();
|
|
await expect(f.owner.signal("SIGKILL")).resolves.toEqual([100]);
|
|
expect(f.attempts).toEqual([[200, "SIGKILL"], [100, "SIGKILL"]]);
|
|
} finally { f.owner.stopObserving(); }
|
|
});
|
|
|
|
it.skipIf(process.platform === "win32")("keeps EPERM fatal while the group still has a running member", async () => {
|
|
const f = epermFixture([row(200, 100, 200, "start-200", "Z"), row(201, 200, 200)]);
|
|
try {
|
|
await f.owner.observe();
|
|
await expect(f.owner.signal("SIGKILL")).rejects.toThrow("kill EPERM");
|
|
} finally { f.owner.stopObserving(); }
|
|
});
|