From ce7766dc11c4515190d48d395f26a90bba900f8a Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Tue, 3 Nov 2026 21:06:50 -0600 Subject: [PATCH] fix(recovery): resume retries with remote workspace storage (#18910) ## Thinking Path > - Paperclip manages work performed by AI agents. > - Native recovery must resume an interrupted run under its saved ownership. > - Remote workspace storage records that ownership in `runnerProfileJson.remoteWorkspace`. > - Recovery recognized only the older workspace record and plugin task leases. > - A retained remote PID could therefore leave a due retry without a dispatch. > - This PR routes the new record through the existing remote recovery verifier. > - Checkpoint, lease, process and single-executor checks remain authoritative. ## Linked Issues or Issue Description **What happened?** After a native sandbox turn returned without a semantic result, same-run recovery was saved with a retry deadline. The run retained its sandbox PID and the new remote workspace reference. The recovery selector skipped it. Later task attempts could then fail on the unfinished workspace writer. **What did you expect to happen?** The due retry must enter existing remote checkpoint and ownership verification. Recovery must resume the original run once or expose the real verification blocker. **Steps to reproduce** Persist a retryable native failure with a remote PID, a prepared `paperclip.remote-workspace-run.v1` reference, an existing lease and checkpoint, then run heartbeat recovery. The added PostgreSQL cases reproduce this shape. **Deployment mode** Native Paperclip Runner in a sandbox with remote workspace storage. Related: #18841 handles legacy workspace recovery during storage migration. #18908 handles failed native preparation. This repair covers a later, provider-started retry. ## What Changed - Recognize the current remote workspace record when classifying process location and selecting remote retries. - Extend the existing database and real-process fixtures to cover the new record, including a retained PID without a process group. - Reuse existing restart recovery. Add no new retry loop, ownership bypass, migration or workflow change. ## Verification - Before the repair: three new behavioral cases failed. Two retries never dispatched; a missing checkpoint never reached its visible recovery hold. - After the repair: all 82 heartbeat recovery tests pass. - All 57 neighboring resumption-binding, restart-recovery and remote-probe tests pass. - Safety cases retain retry deadlines, live controller ownership, active execution, missing-checkpoint rejection, single dispatch and zero host signaling of the sandbox PID. - Local platform: macOS arm64, Node 24.22.2, PostgreSQL 18.1. The tests use disposable databases and an owned live process as the remote PID collision control. Provider execution is outside this routing regression. - Full `pnpm -r typecheck` and `pnpm build` pass at `34aa2d08d38e8afec7f8d98ce984a74551caeda6`. - Current-head GitHub checks: 53 successful; `ci / verify` passed. Paperclip Review completed neutral with a 5/5 assessment and no findings. Two Storybook jobs were skipped by scope; the external security scan was neutral. No failures, pending checks, or merge conflicts remain. - Greptile: 5/5 at the same commit, with no review threads. - The duplicate full local `pnpm test:run` was interrupted after current-head CI passed. Its first two chat shards passed (574 cases); it has no complete local full-suite verdict. This is separate from the passing targeted recovery suites and required CI test lanes. - Staging diagnosis used read-only run, finalization, workspace and lease evidence. The fixed revision has not been deployed. The original task's useful continuation remains unverified. - Initial local attempts stopped before test execution because generated SDK output and PostgreSQL library aliases were missing. Those setup failures are retained separately from the behavioral red/green evidence. ## Risks A wrong remote classification could send a run to the wrong ownership path. The existing verifier still checks the saved checkpoint, controller lease and target. Tests require one dispatch, preserve process identifiers, and prove that recovery never inspects or signals a coincident host PID. Legacy workspace and plugin task coverage remains green. This repairs missed continuation; it does not establish the cause of the earlier missing semantic result. ## Model Used OpenAI GPT-6 through Codex, with reasoning and tool execution. The exact model snapshot and context window were not exposed. ## 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 Co-authored-by: Paperclip --- .../src/services/heartbeat/recovery.test.ts | 28 +++++++++++++------ server/src/services/heartbeat/recovery.ts | 6 ++-- 2 files changed, 23 insertions(+), 11 deletions(-) diff --git a/server/src/services/heartbeat/recovery.test.ts b/server/src/services/heartbeat/recovery.test.ts index 73a665bb53d..d109fcec13c 100644 --- a/server/src/services/heartbeat/recovery.test.ts +++ b/server/src/services/heartbeat/recovery.test.ts @@ -310,7 +310,7 @@ describePostgres("heartbeat recovery module ownership", () => { } finally { try { await f.cleanup(); } finally { await child.stop(); } } }); - async function remoteRetryFixture(pid: number, target: "workspace" | "plugin_task" = "workspace") { + async function remoteRetryFixture(pid: number, target: "workspace" | "remote_workspace" | "plugin_task" = "workspace") { const fixture = await nativeProcessFixture(pid, "remote", "failed"); const sessionId = randomUUID(); const [lease] = await db.insert(environmentLeases).values({ companyId: fixture.run.companyId, @@ -319,13 +319,17 @@ describePostgres("heartbeat recovery module ownership", () => { }).returning(); await db.update(agents).set({ adapterType: "paperclip_runner" }).where(eq(agents.id, fixture.run.agentId)); await db.update(issues).set({ executionRunId: fixture.run.id }).where(eq(issues.id, fixture.run.issueId!)); - await db.update(heartbeatRuns).set({ nativeSessionId: sessionId, runnerProfileJson: { + await db.update(heartbeatRuns).set({ nativeSessionId: sessionId, + ...(target === "remote_workspace" ? { processGroupId: null } : {}), runnerProfileJson: { sessionCheckpoint: { identity: { runId: fixture.run.id, companyId: fixture.run.companyId, issueId: fixture.run.issueId, agentId: fixture.run.agentId, sessionId }, providerSessionId: "existing-provider" }, ...(target === "workspace" ? { nativeWorkspaceSync: { schema: "paperclip.native-workspace-sync/v3", state: "prepared", workspaceId: randomUUID(), leaseId: lease.id, providerLeaseId: lease.providerLeaseId, remoteCwd: "/work", descriptorSha256: "a".repeat(64), baselineSha256: "b".repeat(64), finalHostSha256: null, resourceDisposition: "keep_running" } } : {}), + ...(target === "remote_workspace" ? { remoteWorkspace: { schema: "paperclip.remote-workspace-run.v1", + workspaceId: randomUUID(), epoch: 3, leaseId: lease.id, providerLeaseId: lease.providerLeaseId, + remoteCwd: "/work", state: "prepared", snapshotId: randomUUID() } } : {}), } }).where(eq(heartbeatRuns.id, fixture.run.id)); await db.insert(nativeRunnerCheckpoints).values({ runId: fixture.run.id, companyId: fixture.run.companyId, issueId: fixture.run.issueId!, part: "opaque-checkpoint-record", material: { fixture: true } }); @@ -335,9 +339,12 @@ describePostgres("heartbeat recovery module ownership", () => { return { ...fixture, lease }; } - it.each(["failed", "running"] as const)("routes a due remote %s retry through verified recovery without local PID inspection", async status => { + it.each((["workspace", "remote_workspace"] as const).flatMap(target => + (["failed", "running"] as const).map(status => ({ target, status })), + ))("routes a due $target $status retry through verified recovery without local PID inspection", async ({ target, status }) => { const child = await ownedProcess(); - const f = await remoteRetryFixture(child.pid); + const f = await remoteRetryFixture(child.pid, target); + const processGroupId = target === "remote_workspace" ? null : child.pid; const originalKill = process.kill.bind(process); const kill = vi.spyOn(process, "kill").mockImplementation(originalKill); try { @@ -351,7 +358,7 @@ describePostgres("heartbeat recovery module ownership", () => { remote: { providerLeaseId: f.lease.providerLeaseId, remoteCwd: "/work" } }), })); expect(native.claims).toHaveBeenCalledWith(expect.objectContaining({ runIds: [f.run.id] })); - expect(await f.current()).toMatchObject({ status: "running", processPid: child.pid, processGroupId: child.pid }); + expect(await f.current()).toMatchObject({ status: "running", processPid: child.pid, processGroupId }); expect(kill.mock.calls.filter(([pid]) => Math.abs(pid as number) === child.pid)).toEqual([]); expect(await hasNativeLocalProcessStop(db, f.run.companyId, f.run.id)).toBe(false); await recovery.reapOrphanedRuns(); @@ -378,10 +385,13 @@ describePostgres("heartbeat recovery module ownership", () => { } finally { try { await f.cleanup(); } finally { await child.stop(); } } }); - it.each(["future_retry", "live_owner", "missing_checkpoint", "active_execution"] as const)( - "does not execute a remote retry without current authority: %s", async condition => { + it.each((["workspace", "remote_workspace"] as const).flatMap(target => + (["future_retry", "live_owner", "missing_checkpoint", "active_execution"] as const).map(condition => ({ target, condition })), + ))( + "does not execute a $target retry without current authority: $condition", async ({ target, condition }) => { const child = await ownedProcess(); - const f = await remoteRetryFixture(child.pid); + const f = await remoteRetryFixture(child.pid, target); + const processGroupId = target === "remote_workspace" ? null : child.pid; try { if (condition === "future_retry") await db.update(nativeRunFinalizations) .set({ nextAttemptAt: new Date(Date.now() + 60_000) }).where(eq(nativeRunFinalizations.runId, f.run.id)); @@ -397,7 +407,7 @@ describePostgres("heartbeat recovery module ownership", () => { await createHeartbeatRecovery(db, f.deps).reapOrphanedRuns(); expect(f.deps.executeRun).not.toHaveBeenCalled(); expect(f.deps.terminateHeartbeatRunProcess).not.toHaveBeenCalled(); - expect(await f.current()).toMatchObject({ processPid: child.pid, processGroupId: child.pid }); + expect(await f.current()).toMatchObject({ processPid: child.pid, processGroupId }); if (condition === "future_retry") expect(native.claims).not.toHaveBeenCalled(); if (condition === "missing_checkpoint") { const [coordinator] = await db.select().from(nativeRunFinalizations).where(eq(nativeRunFinalizations.runId, f.run.id)); diff --git a/server/src/services/heartbeat/recovery.ts b/server/src/services/heartbeat/recovery.ts index 2912b041065..739a7584444 100644 --- a/server/src/services/heartbeat/recovery.ts +++ b/server/src/services/heartbeat/recovery.ts @@ -1862,7 +1862,8 @@ export function createHeartbeatRecovery(db: Db, dependencies: HeartbeatRecoveryD } async function nativeProcessIsLocal(run: HeartbeatRun) { - if (parseObject(run.runnerProfileJson).nativeWorkspaceSync != null) return false; + const profile = parseObject(run.runnerProfileJson); + if (profile.nativeWorkspaceSync != null || profile.remoteWorkspace != null) return false; const [identity] = await db.select({ eventType: heartbeatRunEvents.eventType, agentId: heartbeatRunEvents.agentId, @@ -2068,7 +2069,8 @@ export function createHeartbeatRecovery(db: Db, dependencies: HeartbeatRecoveryD if (!await nativeProcessIsLocal(run)) { if (nextAttemptAt && nextAttemptAt > now) continue; if (activeRunExecutions.has(run.id) || runningProcesses.has(run.id)) continue; - if (parseObject(run.runnerProfileJson).nativeWorkspaceSync != null) { + const profile = parseObject(run.runnerProfileJson); + if (profile.nativeWorkspaceSync != null || profile.remoteWorkspace != null) { remoteRetryRunIds.push(run.id); } else { const [taskLease] = await db.select({ id: environmentLeases.id }).from(environmentLeases).where(and(