From 88a35cf4e737ea6a4d6012f9df74534306403eca Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Thu, 29 Oct 2026 13:06:28 -0700 Subject: [PATCH] fix(recovery): retain workspaces when restart recovery blocks (#17839) ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Native restart recovery decides whether an interrupted provider session can continue. > - An unusable checkpoint must stop provider replay. > - The failed run can still own a remote sandbox and files that need saving. > - The blocked restart path recorded failure but omitted the existing workspace preservation obligation. > - Saved user comments then waited for cleanup that had no file-recovery owner. > - This change records that obligation in the same transaction as the failure. ## Linked Issues or Issue Description **What happened?** A native run failed after its provider reported a terminal connection error. Restart recovery rejected its failed checkpoint and marked the run failed. Its prepared workspace and active sandbox lease had no preservation record. A later user continuation waited for the old environment to stop and could not start. **Expected behavior** When restart recovery blocks a run, preserve its exactly bound workspace through the existing stop-and-save service. Keep provider replay blocked. Stop proof and saved files must still precede any successor. **Steps to reproduce** 1. Prepare a native run with a remote workspace descriptor and its matching lease. 2. Persist a permanently failed provider checkpoint. 3. Run native restart recovery. 4. Observe a failed run with an active lease and no required workspace-recovery record. Refs #17801 and #17439. Those PRs add lease-settlement notifications and remote controller adoption. This change records the missing preservation obligation in the current restart failure producer. ## What Changed - Call the existing native workspace preservation service before the blocked restart transaction commits. - Keep the existing company, coordinator ownership, source lease and descriptor checks. - Add real PostgreSQL cases for exact binding, an unrelated source, a live owner, and rollback when preservation cannot commit. - Verify that preservation creates no replacement run or wake and retains the provider attempt count. - Document the transaction and recovery contract. ## Verification - Updated the existing branch with master at `09c05c55050fede448196637cbaa543a154da76e`. Both sides of the overlapping test additions remain. The production fix is still two added lines. - Current source verification: macOS arm64, Node 25.6.1, and disposable Homebrew PostgreSQL 16.14. No deployment or live task replay was performed for this branch update. - **113 tests pass** across `native-failed-workspace-recovery.test.ts` and `native-restart-recovery.test.ts`. Run them with `PAPERCLIP_TEST_DATABASE_URL` set to a disposable database ending in `_test`. Coverage includes exact source binding, live-owner protection, transaction rollback, no replacement run or wake, cancellation preservation, and orphan preservation. - Full local `pnpm -r typecheck` and `pnpm build` pass. Map checks, module-boundary checks, and the PR diff whitespace check pass. - Local verification limits remain explicit. The embedded PostgreSQL probe fails during `initdb`. The full `pnpm test:run` invocation hit that startup failure and was stopped. A broader restart integration invocation against disposable PostgreSQL did not finish while checkpoint-backlog work remained pending and was stopped. Neither invocation is a passing acceptance result. Hosted CI must qualify the updated head. - Prior-head evidence retained: the preservation regression first failed because its required fields were absent. The original verification passed 105 locally available cases and skipped nine real-runner process cases because the local binaries were unavailable. Previous-head hosted CI passed full typecheck/build, test shards, Runner checks, and browser shards. Those results do not qualify the updated head. - Prior live-repair evidence retained: an audited call to the existing preservation service registered the missing obligation for an exactly bound source. Normal workers confirmed termination, released the source lease, and preserved the prepared files before an assigned continuation started. This was an earlier repair, not acceptance of the current branch update. - Updated-head hosted CI passes on `a22feeb54daa2e9325a55d0ccd86515e64cbd47d`, including typecheck/build, all test shards, Runner checks, and browser shards. Apex is 5/5 on the same commit with no actionable findings or unresolved review threads. GitHub reports a clean merge state. The patch is not deployed. The original provider connection failure remains unexplained by this change. ## Risks - Preservation adds database work only when a restart classification becomes terminal. It adds no recurring scan or timer. - A failed preservation write now rolls back failure settlement. The existing owner can retry instead of leaving cleanup without an owner. - A live coordinator or mismatched source remains protected. This change neither destroys the sandbox nor starts provider work in the recovery transaction. - Existing cleanup must still prove termination and preserve files. A terminal run alone does not authorize continuation. ## Model Used OpenAI `gpt-6.1-sol` through Codex in T3 Code, with xhigh reasoning effort, repository inspection, tool use, and code execution. The runtime does not report the context-window size. Earlier implementation used the OpenAI Codex model recorded in the PR history. ## 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 --- doc/execution-semantics.md | 8 ++ maps/feature-map/recovery.md | 3 + .../native-failed-workspace-recovery.test.ts | 78 ++++++++++++++++++- .../native-runtime/native-restart-recovery.ts | 2 + 4 files changed, 87 insertions(+), 4 deletions(-) diff --git a/doc/execution-semantics.md b/doc/execution-semantics.md index 34562168efd..61ab5136574 100644 --- a/doc/execution-semantics.md +++ b/doc/execution-semantics.md @@ -1439,6 +1439,14 @@ keeps ownership throughout that interval. Recovery leaves its coordinator unchan and does not dispatch a second executor. An inactive run still requires the normal ownership, workspace integrity, and provider evidence checks. +When restart recovery blocks an unusable native session, it records any required +workspace preservation in the same transaction as the failed run. The exact +prepared descriptor and source lease must match, and no current coordinator may +own the source. Existing cleanup saves the files and verifies termination before +saved user direction can start another run. This does not retry provider work or +infer the outcome of previous actions. A failed preservation write rolls back +terminalization so the recovery owner can try again. + A preparation that resumes after another execution acquired the coordinator lease yields through the controller-detachment path. It does not fail the shared run, release its environment, or start another provider. Lease expiry alone does diff --git a/maps/feature-map/recovery.md b/maps/feature-map/recovery.md index 54a1bfc2843..2deba6248e4 100644 --- a/maps/feature-map/recovery.md +++ b/maps/feature-map/recovery.md @@ -49,6 +49,9 @@ Agent access: [companion](../agent-access-map/recovery.md). provider session state. Native finalization deadline failures also retain the exact prepared source for the same stop-and-save workflow before Retry admits another executor. + A blocked native restart records that preservation obligation atomically with + its failure. PostgreSQL regressions cover exact source binding, live ownership, + rollback, and no replacement run or wake from file preservation. Retry reconstructs missing preservation for historical failures only from an exact prepared source without a live finalization owner. New messages and orphan cleanup can register missing preservation automatically diff --git a/server/src/services/native-runtime/native-failed-workspace-recovery.test.ts b/server/src/services/native-runtime/native-failed-workspace-recovery.test.ts index 0b740cee829..eaf445caa85 100644 --- a/server/src/services/native-runtime/native-failed-workspace-recovery.test.ts +++ b/server/src/services/native-runtime/native-failed-workspace-recovery.test.ts @@ -5,18 +5,17 @@ import { createHeartbeatLifecycle } from "../heartbeat/run-lifecycle.js"; import { createHeartbeatRunState } from "../heartbeat/run-state.js"; import { createHeartbeatRunPreparation } from "../heartbeat/run-preparation.js"; import { randomUUID } from "node:crypto"; -import { and, eq, inArray } from "drizzle-orm"; +import { and, eq, inArray, sql } from "drizzle-orm"; import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from "vitest"; -import { activityLog, agents, companies, createDb, applyPendingMigrations, environmentLeases, environments, externalAgentHolds, heartbeatRuns, issueRecoveryActions, issues, nativeRunFinalizations } from "@paperclipai/db"; +import { activityLog, agents, agentWakeupRequests, companies, createDb, applyPendingMigrations, environmentLeases, environments, externalAgentHolds, heartbeatRuns, issueRecoveryActions, issues, nativeRunFinalizations } from "@paperclipai/db"; import { getEmbeddedPostgresTestSupport, startEmbeddedPostgresTestDatabase } from "../../__tests__/helpers/embedded-postgres.js"; import { getExecutionBlocker } from "../execution-blocker.js"; import { markExecutionReconciliation, settleUnrecoverableExecutions, validateExecutionReconciliation } from "../execution-recovery-resolution.js"; import { prepareSandboxStopAndRetain, settleStopOnlyCleanup } from "../sandbox-stop-and-retain.js"; import { preserveCancelledNativeWorkspaces, preserveNativeFailedWorkspace, preserveOrphanedNativeWorkspace, registerCancelledWorkspacePreservationWork } from "./native-failed-workspace-recovery.js"; -import type { NativeRestartRecoveryClaim } from "./native-restart-recovery.js"; +import { claimNativeRestartRecoveries, currentNativeControllerIdentity, type NativeRestartRecoveryClaim } from "./native-restart-recovery.js"; import { NativeWorkspaceFinalizationBusyError } from "./native-workspace-finalization-ownership.js"; import { preserveWorkspaceRestoreRecoveryMetadataSql } from "../legacy-workspace-restore-recovery.js"; -import { claimNativeRestartRecoveries } from "./native-restart-recovery.js"; import { claimNativeSessionResumptions } from "./native-finalization-reconciler.js"; const externalUrl = resolveExternalTestDatabaseUrl("PAPERCLIP_TEST_DATABASE_URL"); @@ -62,6 +61,77 @@ const support = externalUrl ? { supported: true } : await getEmbeddedPostgresTes const readLease = async (id: string) => (await db.select().from(environmentLeases).where(eq(environmentLeases.id, id)))[0]!; const actions = async (issueId: string) => db.select().from(issueRecoveryActions).where(eq(issueRecoveryActions.sourceIssueId, issueId)); + async function seedRestartFailure() { + const f = await seed(); + await db.update(heartbeatRuns).set({ status: "running", nativePhase: "observed", error: null, errorCode: null, + runnerProfileJson: { ...f.run.runnerProfileJson, sessionCheckpoint: { + terminal: { runTerminalState: "failed", turnTerminalState: "failed" }, + providerSessionId: "failed-provider-session", + } }, + }).where(eq(heartbeatRuns.id, f.runId)); + await db.update(nativeRunFinalizations).set({ phase: "observed", attempt: 1 }).where(eq(nativeRunFinalizations.runId, f.runId)); + await db.update(issues).set({ executionRunId: f.runId }).where(eq(issues.id, f.issueId)); + return f; + } + + it.each(["bound", "foreign", "owned"])("pins blocked restart files only for an unowned, bound source: %s", async kind => { + const f = await seedRestartFailure(); + const controller = await currentNativeControllerIdentity(); + if (kind === "owned") await db.update(nativeRunFinalizations).set({ leaseOwner: "active-controller", + leaseExpiresAt: new Date(Date.now() + 60_000), controllerBootId: controller.bootId, + controllerPid: controller.pid, controllerProcessStartedAt: controller.processStartedAt, + }).where(eq(nativeRunFinalizations.runId, f.runId)); + if (kind === "foreign") await db.update(environmentLeases).set({ metadata: { ...f.lease.metadata, remoteCwd: "/other" } }) + .where(eq(environmentLeases.id, f.lease.id)); + + expect(await claimNativeRestartRecoveries({ db, restartKind: "hard", runIds: [f.runId] })).toEqual([ + { kind: kind === "owned" ? "awaiting_evidence" : "blocked", runId: f.runId, + reason: kind === "owned" ? "controller_still_alive" : "provider_checkpoint_permanently_failed" }, + ]); + const current = await readRun(f.runId); + expect(current.status).toBe(kind === "owned" ? "running" : "failed"); + expect(current.resultJson).toMatchObject({ originalDiagnostic: true }); + if (kind === "bound") { + expect(current.resultJson).toMatchObject({ workspaceRestoreFailure: "restore_failed", + workspaceRestoreRecovery: { leaseIds: [f.lease.id] } }); + expect(await readLease(f.lease.id)).toMatchObject({ status: "pending_cleanup", leasePolicy: "retain_on_failure", + metadata: { sandboxStopAndRetain: { runId: f.runId, leaseId: f.lease.id } } }); + expect((await actions(f.issueId)).some(action => action.evidence.workspaceRestoreRecovery)).toBe(true); + } else { + expect(current.resultJson?.workspaceRestoreRecovery).toBeUndefined(); + expect(await readLease(f.lease.id)).toMatchObject({ status: "active", leasePolicy: "ephemeral" }); + } + expect(await db.select().from(heartbeatRuns).where(eq(heartbeatRuns.companyId, f.companyId))).toHaveLength(1); + expect(await db.select().from(agentWakeupRequests).where(eq(agentWakeupRequests.companyId, f.companyId))).toHaveLength(0); + expect((await db.select().from(nativeRunFinalizations).where(eq(nativeRunFinalizations.runId, f.runId)))[0].attempt).toBe(1); + }); + + it("rolls back restart terminalization when workspace retention cannot commit", async () => { + const f = await seedRestartFailure(); + const name = sql.identifier(`reject_retention_${f.runId.replaceAll("-", "")}`); + await db.execute(sql`create function ${name}() returns trigger language plpgsql as $$ + begin + if new.result_json ? 'workspaceRestoreRecovery' then + raise exception 'retention write failed'; + end if; + return new; + end; + $$`); + await db.execute(sql`create trigger ${name} before update on heartbeat_runs for each row execute function ${name}()`); + try { + await expect(claimNativeRestartRecoveries({ db, restartKind: "hard", runIds: [f.runId] })).rejects.toThrow(); + expect(await readRun(f.runId)).toMatchObject({ status: "running", nativePhase: "observed", resultJson: { originalDiagnostic: true } }); + expect(await readLease(f.lease.id)).toMatchObject({ status: "active", leasePolicy: "ephemeral" }); + expect(await actions(f.issueId)).toHaveLength(0); + expect((await db.select().from(issues).where(eq(issues.id, f.issueId)))[0].executionRunId).toBe(f.runId); + expect((await db.select().from(nativeRunFinalizations).where(eq(nativeRunFinalizations.runId, f.runId)))[0]) + .toMatchObject({ phase: "observed", attempt: 1 }); + } finally { + await db.execute(sql`drop trigger ${name} on heartbeat_runs`); + await db.execute(sql`drop function ${name}()`); + } + }); + describe("cancelled workspace preservation worker", () => { const fixtureCompanies = new Set(); afterEach(async () => { diff --git a/server/src/services/native-runtime/native-restart-recovery.ts b/server/src/services/native-runtime/native-restart-recovery.ts index e3448b840b1..8ac4d9bd918 100644 --- a/server/src/services/native-runtime/native-restart-recovery.ts +++ b/server/src/services/native-runtime/native-restart-recovery.ts @@ -5,6 +5,7 @@ import { notifyUnsafeWorkspaceExportRunChange } from "../unsafe-workspace-export import { notifyCancelledWorkspacePreservation } from "../cancelled-workspace-preservation-notifications.js"; import { notifyInstructionCollection } from "../instruction-collection-notifications.js"; import { notifyExecutionRunWork } from "../execution-control-notifications.js"; +import { preserveNativeFailedWorkspace } from "./native-failed-workspace-recovery.js"; import { hasNativeRunnerCheckpoint } from "./native-runner-checkpoint.js"; import { readNativeWorkspaceSyncReference } from "./native-workspace-sync.js"; import { activeNativeExecutionRunIds, NATIVE_PREPARATION_FENCING_VERSION } from "./native-active-runs.js"; @@ -993,6 +994,7 @@ export async function claimNativeRestartRecoveries(input: { wakePolicy: null, supersedeOnIdentityChange: true, }); + if (updatedRun) await preserveNativeFailedWorkspace(tx as unknown as Db, updatedRun); return { kind: "blocked", runId: row.run.id, reason } as const; }