mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-12 14:19:00 +01:00
fix(recovery): judge an orphaned run's pid only in the PID space that recorded it (#19871)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The orphan reaper (`terminalizeOrphanedRunningRun`) ends a `running` run whose recorded process is gone > - It judged "gone" with `kill(pid, 0)` on the local host, wherever the process actually ran: another host, another PID namespace, or a sandbox > - So a run whose process is alive elsewhere could be terminalized while it still works > - #19868 fixed the same gap for native coordinator takeover and the runtime-control sweep. This pull request finishes finding 2 for the reaper > - The benefit is that a dead local pid never proves an owner on another host is gone ## Linked Issues or Issue Description **Bug description** The Phase 0 ownership inventory (finding 2) names the orphan reaper `terminalizeOrphanedRunningRun` (`recovery/service.ts`) as a proof that accepts "the pid is dead here" for a process anywhere. `heartbeat_runs` recorded `process_pid` with no PID space, and a sandbox pid could collide with a local one. **Steps to reproduce** 1. A run records `process_pid` on host A, or in a sandbox. 2. On host B, that pid number is not alive. 3. B's stale-lock sweep terminalizes the run as `orphaned_running_run`. **Expected behavior** The process-death authority applies only in the PID space that recorded the pid. Issue-terminal authority is unchanged. Refs #19846, follows #19868 ## What Changed - Migration `0442`: add nullable `heartbeat_runs.process_host_hash`. - `heartbeat/run-lifecycle.ts` `persistHeartbeatRunProcessMetadata`, the one writer of `process_pid`, stores `currentProcessSpaceHash()` for a local pid and `"remote"` for a sandbox pid. - `recovery/service.ts`: the process-death authority runs only when `process_host_hash` is null (rows saved before this column, for one release) or `sameProcessSpace` matches. - Tests: the reaper leaves a dead pid alone for another host, a sandbox, and an unreadable space, and terminalizes it in this space. The writer stamps local and remote pids. ## Verification - `npx vitest run src/__tests__/recovery-stale-issue-lock-sweep.test.ts`: 46 passed. - Red before the fix: with the old `recovery/service.ts`, the three cross-space cases fail (the run is terminalized). Green after. - `pnpm db:generate` produced `0442_low_salo.sql` (one `ADD COLUMN`), and the snapshot set was pruned to five. `pnpm --filter @paperclipai/db check:migrations` passed. Server `tsc --noEmit` shows no errors in the changed files. ## Risks - Previous-release story: runs saved with `process_host_hash = null` keep the previous local proof for this release. That matches #19868, because stranding in-flight runs at upgrade would be a restart failure. New rows are strict. - A run whose process really lived on another host now stays `running` until issue-terminal authority, its controller lease, or another owner settles it. - No new I/O before `turn.start`. The hash is computed at module load, and the stamp rides the existing process-metadata update. ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`), Anthropic, used with tool use and code execution in Claude Code. ## 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 - [ ] All Paperclip CI gates are green - [ ] 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>
This commit is contained in:
1 parent
00e627a23e
commit
5a20fca9f7
11 files changed
+533
-8
No files matched your search
@@ -512,7 +512,7 @@ describe("residual recovery production matrix", () => {
|
||||
} finally { db.$client.options.debug = prior; }
|
||||
expect(await getRun(run.id)).toMatchObject({ processPid: pid, processStartedAt: null });
|
||||
await quiet();
|
||||
await persistHeartbeatRunProcessMetadata(db, run.id, { pid, processGroupId: null, startedAt: startedAt.toISOString() });
|
||||
await persistHeartbeatRunProcessMetadata(db, run.id, { pid, processGroupId: null, startedAt: startedAt.toISOString(), targetKind: "local" });
|
||||
await advance(1_000);
|
||||
await vi.waitFor(async () => expect((await getRun(run.id)).processPid).toBeNull());
|
||||
expect(await db.select().from(heartbeatRunEvents).where(and(eq(heartbeatRunEvents.runId, run.id),
|
||||
|
||||
@@ -1,3 +1,5 @@
|
||||
import { persistHeartbeatRunProcessMetadata } from "../services/heartbeat/run-lifecycle.js";
|
||||
import { currentProcessSpaceHash } from "../services/controller-process-identity.js";
|
||||
import { resolveExternalTestDatabaseUrl } from "./helpers/external-test-database.js";
|
||||
import { randomUUID } from "node:crypto";
|
||||
import { eq, sql } from "drizzle-orm";
|
||||
@@ -394,6 +396,34 @@ describeEmbeddedPostgres("recovery sweepStaleIssueLocks", () => {
|
||||
expect(second.cleared).toBe(0);
|
||||
});
|
||||
|
||||
it.each([["local", true], ["remote", false]] as const)("stamps a %s process pid with its PID space", async (targetKind, local) => {
|
||||
const { runningRunId } = await seed();
|
||||
await persistHeartbeatRunProcessMetadata(db, runningRunId, { pid: process.pid, processGroupId: null,
|
||||
startedAt: new Date().toISOString(), targetKind });
|
||||
const [run] = await db.select({ processHostHash: heartbeatRuns.processHostHash }).from(heartbeatRuns)
|
||||
.where(eq(heartbeatRuns.id, runningRunId));
|
||||
expect(run?.processHostHash).toBe(local ? currentProcessSpaceHash() : "remote");
|
||||
});
|
||||
|
||||
it.each([
|
||||
["another host", "another-host-pid-space", false],
|
||||
["a sandbox", "remote", false],
|
||||
["an unreadable PID space", "unknown", false],
|
||||
["this PID space", "current", true],
|
||||
] as const)("judges a dead recorded pid only in its own PID space (%s)", async (_name, space, terminalized) => {
|
||||
const { companyId, agentId, runningRunId } = await seed();
|
||||
await db.update(heartbeatRuns).set({ processPid: 2_000_000_000,
|
||||
processHostHash: space === "current" ? currentProcessSpaceHash() : space })
|
||||
.where(eq(heartbeatRuns.id, runningRunId));
|
||||
await db.insert(issues).values({ id: randomUUID(), companyId, title: "Cross-host orphan", status: "in_progress",
|
||||
priority: "high", assigneeAgentId: agentId, checkoutRunId: runningRunId, executionRunId: runningRunId,
|
||||
executionLockedAt: new Date() });
|
||||
const result = await heartbeatService(db).sweepStaleIssueLocks();
|
||||
expect(result.terminalizedRunIds.includes(runningRunId)).toBe(terminalized);
|
||||
const [run] = await db.select({ status: heartbeatRuns.status }).from(heartbeatRuns).where(eq(heartbeatRuns.id, runningRunId));
|
||||
expect(run?.status).toBe(terminalized ? "interrupted" : "running");
|
||||
});
|
||||
|
||||
it("terminalizes an orphaned running run whose process is gone, then clears the lock", async () => {
|
||||
const { companyId, agentId, runningRunId } = await seed();
|
||||
// The run recorded a pid, but the process and its sandbox are gone. A pid
|
||||
|
||||
@@ -1772,7 +1772,7 @@ const support = externalUrl ? { supported: true } : await getEmbeddedPostgresTes
|
||||
expect(await hasNativeLocalProcessStop(db, f.companyId, source.id)).toBe(true);
|
||||
expect(await hasNativeLocalProcessStop(db, randomUUID(), source.id)).toBe(false);
|
||||
expect(await admit(f, true)).toMatchObject({ previousRunId: source.id });
|
||||
await persistHeartbeatRunProcessMetadata(db, source.id, { pid: 999999999, processGroupId: null, startedAt: new Date().toISOString() });
|
||||
await persistHeartbeatRunProcessMetadata(db, source.id, { pid: 999999999, processGroupId: null, startedAt: new Date().toISOString(), targetKind: "local" });
|
||||
await db.update(heartbeatRuns).set({ processPid: null }).where(eq(heartbeatRuns.id, source.id));
|
||||
expect(await admit(f, true)).toBeNull();
|
||||
expect(await db.transaction(tx => recordNativeLocalProcessStop(tx as unknown as typeof db, source))).toBe(true);
|
||||
|
||||
@@ -197,6 +197,7 @@ import {
|
||||
setHeartbeatRunRuntimeStatus,
|
||||
} from "../heartbeat-run-runtime-status.js";
|
||||
import { readProcessStartedAt } from "../hot-restart.js";
|
||||
import { currentProcessSpaceHash } from "../controller-process-identity.js";
|
||||
|
||||
import type { createHeartbeatRunState } from "./run-state.js";
|
||||
import type { createHeartbeatRunPreparation } from "./run-preparation.js";
|
||||
@@ -578,7 +579,7 @@ function buildRunEventRuntimeProgress(input: {
|
||||
export async function persistHeartbeatRunProcessMetadata(
|
||||
db: Db,
|
||||
runId: string,
|
||||
meta: { pid: number; processGroupId: number | null; startedAt: string; targetKind?: "local" | "remote" },
|
||||
meta: { pid: number; processGroupId: number | null; startedAt: string; targetKind: "local" | "remote" },
|
||||
) {
|
||||
// A sandbox PID belongs to a different host and may collide with a local PID.
|
||||
const observedStartedAt = meta.targetKind === "remote" ? null : await readProcessStartedAt(meta.pid).catch(
|
||||
@@ -592,6 +593,8 @@ export async function persistHeartbeatRunProcessMetadata(
|
||||
legacyProcessSettlement: null,
|
||||
processPid: meta.pid,
|
||||
processGroupId: meta.processGroupId,
|
||||
// A sandbox PID never names a process in this PID space.
|
||||
processHostHash: meta.targetKind === "remote" ? "remote" : currentProcessSpaceHash(),
|
||||
processStartedAt: Number.isNaN(startedAt.getTime())
|
||||
? new Date()
|
||||
: startedAt,
|
||||
@@ -2240,7 +2243,7 @@ export function createHeartbeatLifecycle(db: Db, dependencies: HeartbeatLifecycl
|
||||
|
||||
async function persistRunProcessMetadata(
|
||||
runId: string,
|
||||
meta: { pid: number; processGroupId: number | null; startedAt: string; targetKind?: "local" | "remote" },
|
||||
meta: { pid: number; processGroupId: number | null; startedAt: string; targetKind: "local" | "remote" },
|
||||
) {
|
||||
return persistHeartbeatRunProcessMetadata(db, runId, meta);
|
||||
}
|
||||
|
||||
@@ -444,6 +444,19 @@ describe.skipIf(!support.supported)("heartbeat runtime execution boundary", () =
|
||||
expectCleanup(input);
|
||||
});
|
||||
|
||||
it.each([["local", null], ["remote", { kind: "remote", transport: "sandbox", remoteCwd: "/workspace" }]] as const)(
|
||||
"passes the spawn origin through the legacy adapter callback (%s)", async (expected, target) => {
|
||||
const { input, adapter } = await fixture();
|
||||
if (target) input.workspace.executionTarget = target as typeof input.workspace.executionTarget;
|
||||
adapter.execute.mockImplementationOnce(async ctx => {
|
||||
await ctx.onSpawn?.({ pid: 4242, processGroupId: 4242, startedAt: new Date().toISOString() });
|
||||
return { exitCode: 0, signal: null, timedOut: false, provider: "test" };
|
||||
});
|
||||
await executeHeartbeatRuntime(db, input).catch(() => undefined);
|
||||
expect(input.services.persistRunProcessMetadata).toHaveBeenCalledWith(expect.any(String),
|
||||
expect.objectContaining({ pid: 4242, targetKind: expected }));
|
||||
});
|
||||
|
||||
it("preserves Stop metadata and marks no-work accounting without manufacturing a receipt", async () => {
|
||||
const { input, adapter, run } = await fixture();
|
||||
await db.update(heartbeatRuns).set({ status: "cancelled", resultJson: { stoppedByUser: true } }).where(eq(heartbeatRuns.id, run.id));
|
||||
|
||||
@@ -924,6 +924,8 @@ export async function executeHeartbeatRuntime(db: Db, input: HeartbeatRuntimeExe
|
||||
? meta.processGroupId
|
||||
: null,
|
||||
startedAt: meta.startedAt,
|
||||
// A sandbox runner's spawn metadata names a sandbox PID.
|
||||
targetKind: executionTarget?.kind ?? "local",
|
||||
});
|
||||
},
|
||||
authToken: authToken ?? undefined,
|
||||
|
||||
@@ -108,6 +108,7 @@ import {
|
||||
isPidAlive,
|
||||
isProcessGroupAlive,
|
||||
} from "../local-service-supervisor.js";
|
||||
import { sameProcessSpace } from "../controller-process-identity.js";
|
||||
import { redactSensitiveText } from "../../redaction.js";
|
||||
import { isUniqueViolation } from "../../db-errors.js";
|
||||
import {
|
||||
@@ -6180,7 +6181,12 @@ export function recoveryService(
|
||||
let processGone = false;
|
||||
const hasLiveExecution =
|
||||
deps.liveRunExecutions?.has(run.id) ?? runningProcesses.has(run.id);
|
||||
if (!hasLiveExecution) {
|
||||
// A recorded PID is judged only in the PID space that recorded it: a dead
|
||||
// local PID never proves that a process on another host, in another
|
||||
// namespace, or in a sandbox ("remote") is gone. Rows saved before the
|
||||
// space was recorded keep the previous local proof for one release.
|
||||
const processSpaceJudgeable = run.processHostHash == null || sameProcessSpace(run.processHostHash);
|
||||
if (!hasLiveExecution && processSpaceJudgeable) {
|
||||
if (typeof pid === "number" || typeof processGroupId === "number") {
|
||||
const processAlive =
|
||||
(typeof pid === "number" && isPidAlive(pid)) ||
|
||||
|
||||
Reference in new issue
Block a user