From 5723fd82ca2adc1c508b5a7f1b3a066c09d21e5f Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Wed, 4 Nov 2026 18:25:36 -0600 Subject: [PATCH] fix(native): release a fresh claim when task preparation fails (#19065) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - A native run claims its finalization coordinator before task preparation. Then it starts the provider turn. > - Task preparation can fail after the claim and before any provider turn. > - In that case the executor kept its claim. Failure settlement read the claim as a live owner and stopped. Nothing failed the run. > - The 60-second execution-control watchdog then failed the run as `execution_finalization_deadline_exceeded`. The original error was lost. > - This pull request releases only that exact fresh, unstarted claim after renewal stops. > - The benefit is that ordinary settlement fails the run at once with its real error and keeps the workspace files. The watchdog stays a backstop. ## Linked Issues or Issue Description Refs #19028. That PR keeps the earlier failure code when the watchdog fails a run. In this case no earlier code is saved, so it cannot help. This PR removes the wait for the watchdog. I searched open PRs for duplicates. #18820, #19034 and #19038 touch near code, but none release a preparation claim. **What happened?** A native task with a prepared sandbox workspace failed during preparation, after the controller claim committed. The coordinator stayed `observed` and kept this controller's lease owner. The heartbeat failure path calls workspace preservation. Preservation refuses any lease owner without a matching restart claim, so it raised `NativeWorkspaceFinalizationBusyError`. The heartbeat then treated ownership as held and returned without a terminal write. About 60 seconds later the watchdog failed the run with a generic "verify the provider stopped" action. The real error (for example `native_harness_storage_owner_missing`) stayed only in the workspace operation record. **Expected behavior** When no provider turn started under a fresh claim, settle the failure like a failure before the claim: retain the source, fail the run with its own error, and do not wait for the watchdog. **Steps to reproduce** 1. Start a native run with a prepared remote workspace. 2. Make task preparation fail after `native.coordinator.claim` commits (for example, a preparation callback throws). 3. See that the run stays `running` until the control deadline fails it as `execution_finalization_deadline_exceeded`. ## What Changed - `native-execution-lease.ts`: add `releaseUnstartedNativeExecutionClaim`. It clears the lease owner and expiry only for the exact run, company, issue, owner and attempt, while the phase is `observed` and no result exists. - `native-session-executor.ts`: when task preparation fails after a fresh claim, mark that claim. The startup owner releases it after renewal stops. A restart-recovery claim, a startup-cleanup context, Stop, restart detach, or a different owner keeps the old behavior. - Tests: an executor regression for the fresh and restart cases, and a real-database test for release and workspace retention against the exact owner, a foreign owner, a different attempt and an accepted result. - `doc/execution-semantics.md`: state that the deadline is a backstop and describe the release. ## Verification - Red before the fix: `cd server && npx vitest run src/services/native-runtime/native-session-executor.test.ts -t "releases only a fresh claim"` fails with "releaseUnstartedNativeExecutionClaim ... Number of calls: 0". - Green after the fix, with the same command. - `npx vitest run src/services/native-runtime/native-session-executor.test.ts src/services/native-runtime/native-failed-workspace-recovery.test.ts src/services/native-runtime/native-execution-lease.test.ts src/services/execution-control-reconciliation.test.ts` passes (embedded Postgres for the retention test). - `pnpm --filter @paperclipai/server typecheck` passes. - Not done: a live run against a real sandbox at this revision. The staging evidence is read-only. ## Risks - The release is fenced by the exact owner, attempt, `observed` phase and no result. Renewal stops before the release, so a renewal cannot see a missing owner and detach or cancel the run. - The fix covers failures inside task preparation only. Failures later in startup, or after a restart-recovery claim expires, still reach the watchdog. Those keep fail-closed behavior and need separate evidence. - If the release write fails, the run keeps the old watchdog path. > For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and discuss it in `#dev` before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See `CONTRIBUTING.md`. ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`), Anthropic, 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 - [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 --- doc/execution-semantics.md | 8 ++++ .../native-runtime/native-execution-lease.ts | 20 +++++++++- .../native-failed-workspace-recovery.test.ts | 28 ++++++++++++++ .../native-session-executor.test.ts | 37 +++++++++++++++++++ .../native-runtime/native-session-executor.ts | 23 +++++++++++- 5 files changed, 114 insertions(+), 2 deletions(-) diff --git a/doc/execution-semantics.md b/doc/execution-semantics.md index 0ae90ed415a..f83ff2811ee 100644 --- a/doc/execution-semantics.md +++ b/doc/execution-semantics.md @@ -1983,6 +1983,14 @@ ordinary Retry can continue. Finalized workspaces, mismatched sources and accepted results keep their existing settlement paths. Reassignment does not discard the stopped run's files or grant authority over the new assignee. +The deadline is a backstop, not the normal failure path. When native task +preparation fails after a fresh controller claim and before any provider turn, +the controller stops renewal and releases only that exact claim. Ordinary +failure settlement then retains the source and fails the run with its original +error, as it does for a failure before the claim. A restart-recovery claim, +startup cleanup, Stop, restart detach, accepted result or different owner keeps +its existing owner and settlement path. + For older failed native runs that lack this bookkeeping, a deliberate Retry registers preservation from the exact prepared workspace descriptor and assigns the existing bounded maintenance owner. A live finalization owner or mismatched diff --git a/server/src/services/native-runtime/native-execution-lease.ts b/server/src/services/native-runtime/native-execution-lease.ts index dc28b1ad9fe..4e17b04eccf 100644 --- a/server/src/services/native-runtime/native-execution-lease.ts +++ b/server/src/services/native-runtime/native-execution-lease.ts @@ -1,7 +1,7 @@ import { notifyLiveRunRecovery } from "../live-run-recovery-notifications.js"; import type { Db } from "@paperclipai/db"; import { heartbeatRuns, livenessDb, nativeRunFinalizations } from "@paperclipai/db"; -import { and, eq, gt, sql } from "drizzle-orm"; +import { and, eq, gt, isNull, sql } from "drizzle-orm"; import { currentNativeControllerIdentity, type NativeControllerIdentity } from "./native-restart-recovery.js"; import { REMOTE_CONTROLLER_LEASE_MS } from "./native-runner-recovery-contract.js"; @@ -30,6 +30,24 @@ export async function assertNativeSessionPreparationAuthority(input: { if (!owned) throw new Error("native_session_lease_lost"); } +/** + * Preparation failed after a fresh claim and before any provider turn. Release + * only this exact claim, so ordinary failure settlement can retain the source + * as it does for failures before the claim. Any other owner or a result keeps it. + */ +export async function releaseUnstartedNativeExecutionClaim(input: { + db: Db; runId: string; companyId: string; issueId: string; leaseOwner: string; attempt: number; +}): Promise { + const released = await input.db.update(nativeRunFinalizations) + .set({ leaseOwner: null, leaseExpiresAt: null, updatedAt: new Date() }) + .where(and(eq(nativeRunFinalizations.runId, input.runId), eq(nativeRunFinalizations.companyId, input.companyId), + eq(nativeRunFinalizations.issueId, input.issueId), eq(nativeRunFinalizations.leaseOwner, input.leaseOwner), + eq(nativeRunFinalizations.attempt, input.attempt), eq(nativeRunFinalizations.phase, "observed"), + isNull(nativeRunFinalizations.resultId))) + .returning({ runId: nativeRunFinalizations.runId }); + return released.length > 0; +} + export async function renewNativeSessionExecutionLease(input: { db: Db; runId: string; 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 6f94d461e24..e00347330c5 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 @@ -15,6 +15,7 @@ import { prepareSandboxStopAndRetain, settleStopOnlyCleanup } from "../sandbox-s import { preserveCancelledNativeWorkspaces, preserveNativeFailedWorkspace, preserveOrphanedNativeWorkspace, registerCancelledWorkspacePreservationWork } from "./native-failed-workspace-recovery.js"; import { claimNativeRestartRecoveries, currentNativeControllerIdentity, NATIVE_RESTART_RECOVERY_FAILED, type NativeRestartRecoveryClaim } from "./native-restart-recovery.js"; import { NativeWorkspaceFinalizationBusyError } from "./native-workspace-finalization-ownership.js"; +import { releaseUnstartedNativeExecutionClaim } from "./native-execution-lease.js"; import { preserveWorkspaceRestoreRecoveryMetadataSql } from "../legacy-workspace-restore-recovery.js"; import { claimNativeSessionResumptions } from "./native-finalization-reconciler.js"; @@ -779,6 +780,33 @@ const support = externalUrl ? { supported: true } : await getEmbeddedPostgresTes expect(await readRun(f.runId)).toMatchObject({ status: "running", nativePhase: "observed" }); }); + it.each(["own", "foreign", "attempt", "accepted"])("retains files after an unstarted preparation claim is released: %s", async kind => { + const f = await seed(); + const leaseOwner = randomUUID(); + await db.update(heartbeatRuns).set({ status: "running", nativePhase: "observed", errorCode: null }) + .where(eq(heartbeatRuns.id, f.runId)); + await db.update(nativeRunFinalizations).set({ phase: "observed", attempt: 1, leaseOwner, + leaseExpiresAt: new Date(Date.now() + 60_000) }).where(eq(nativeRunFinalizations.runId, f.runId)); + // The heartbeat cannot tell a finished preparation claim from a live owner. + await expect(preserveNativeFailedWorkspace(db, await readRun(f.runId), { allowPreterminal: true })) + .rejects.toBeInstanceOf(NativeWorkspaceFinalizationBusyError); + if (kind === "accepted") await db.update(nativeRunFinalizations).set({ phase: "result_accepted" }).where(eq(nativeRunFinalizations.runId, f.runId)); + const released = await releaseUnstartedNativeExecutionClaim({ db, runId: f.runId, companyId: f.companyId, issueId: f.issueId, + leaseOwner: kind === "foreign" ? randomUUID() : leaseOwner, attempt: kind === "attempt" ? 2 : 1 }); + expect(released).toBe(kind === "own"); + if (kind !== "own") { + expect((await db.select().from(nativeRunFinalizations).where(eq(nativeRunFinalizations.runId, f.runId)))[0].leaseOwner).toBe(leaseOwner); + expect(await readLease(f.lease.id)).toMatchObject({ status: "active" }); + return; + } + await preserveNativeFailedWorkspace(db, await readRun(f.runId), { allowPreterminal: true }); + expect(await db.select().from(nativeRunFinalizations).where(eq(nativeRunFinalizations.runId, f.runId))) + .toMatchObject([{ phase: "terminal_failure", leaseOwner: null, recoveryState: "blocked" }]); + expect(await readRun(f.runId)).toMatchObject({ status: "running", nativePhase: "terminal_failure", + resultJson: { workspaceRestoreFailure: "restore_failed", workspaceRestoreRecovery: { leaseIds: [f.lease.id] } } }); + expect(await readLease(f.lease.id)).toMatchObject({ status: "pending_cleanup", leasePolicy: "retain_on_failure" }); + }); + it("refuses cleanup when another execution still owns the coordinator", async () => { const f = await seed(); await db.update(nativeRunFinalizations).set({ phase: "observed", leaseOwner: randomUUID() }) diff --git a/server/src/services/native-runtime/native-session-executor.test.ts b/server/src/services/native-runtime/native-session-executor.test.ts index b7ced95d40c..440341a6d13 100644 --- a/server/src/services/native-runtime/native-session-executor.test.ts +++ b/server/src/services/native-runtime/native-session-executor.test.ts @@ -11791,6 +11791,43 @@ describe("native session bounded recovery", () => { } finally { release(); await running; renew.mockRestore(); } }); + it.each([false, true])("releases only a fresh claim after preparation fails before provider work (restart claim: %s)", async restarted => { + const fresh = { ...structuredClone(execution), provider: { kind: "acpx", agent: "claude", + model: "claude-sonnet-4-6", profile: QUALIFIED_ACPX_PROFILES.claude } } as NativeExecutionInput; + const updates: Array<{ table: unknown; values: Record }> = []; + const { currentNativeControllerIdentity } = await import("./native-restart-recovery.js"); + const controller = await currentNativeControllerIdentity(); + const restartRecovery = restarted ? { kind: "reattach_remote_runner", runId: fresh.binding.runId, leaseOwner: "restart-owner", + controllerGeneration: 2, providerAttempt: 1, restartKind: "hard", recoveryRequestId: null, + remote: { providerLeaseId: "fresh-provider-lease", remoteCwd: "/workspace" } } as const : undefined; + const db = leaseDb(fresh, restarted ? { leaseOwner: "restart-owner", controllerBootId: controller.bootId, + controllerPid: controller.pid, controllerGeneration: 2, attempt: 0 } : {}, {}, updates); + // Fails inside task preparation, after the claim commits and renewal starts. + const failure = new Error("native_harness_storage_owner_missing"); + const order: string[] = []; + const stop = vi.fn(async () => { order.push("stop"); }); + const renew = vi.spyOn(nativeLeaseRenewal, "startNativeSessionExecutionLeaseRenewal").mockReturnValue({ stop, remainingMs: () => 60_000 }); + const release = vi.spyOn(nativeLeaseRenewal, "releaseUnstartedNativeExecutionClaim") + .mockImplementation(async () => { order.push("release"); return true; }); + try { + await expect(executePaperclipNativeSession({ db, execution: fresh, runnerInstanceId: "fresh-runner", + onExecutionLeaseAcquired: async () => { throw failure; }, + ...(restartRecovery ? { leaseOwner: restartRecovery.leaseOwner, restartRecovery } : {}), + runnerExecutionTarget: { kind: "remote", transport: "sandbox", remoteCwd: "/workspace", environmentId: "fresh-environment", leaseId: "fresh-lease" }, + })).rejects.toBe(failure); + expect(updates.some(update => update.table === nativeRunFinalizations && "phase" in update.values + && update.values.phase !== "observed")).toBe(false); + if (restarted) { + // A restart claim keeps its exact recovery fence for the heartbeat's retention check. + expect(release).not.toHaveBeenCalled(); + } else { + expect(release).toHaveBeenCalledExactlyOnceWith({ db, runId: fresh.binding.runId, companyId: fresh.binding.companyId, + issueId: fresh.binding.issueId, leaseOwner: expect.stringMatching(/^fresh-runner:/), attempt: 1 }); + expect(order).toEqual(["stop", "release"]); + } + } finally { renew.mockRestore(); release.mockRestore(); } + }); + it("persists closed remote recovery evidence without replacing saved run results", async () => { const updates: Array<{ table: unknown; values: Record }> = []; const onEvent = vi.fn(async () => undefined); diff --git a/server/src/services/native-runtime/native-session-executor.ts b/server/src/services/native-runtime/native-session-executor.ts index d36f7395977..e923de72117 100644 --- a/server/src/services/native-runtime/native-session-executor.ts +++ b/server/src/services/native-runtime/native-session-executor.ts @@ -29,7 +29,7 @@ import { providerPackWorkspaceId, recordAdmittedRemoteProviderPack, resolveRemot import { nativeProviderPackAdmissionPending, rememberAdmittedNativeProviderPack, rememberRecoveredNativeProviderPack, recordNativeProviderPackAdmission, type NativeProviderPackAdmissionSource } from "./native-provider-pack-admission.js"; import { type ProviderPackInstallation, verifyRemoteProviderPack as verifyRemoteProviderPackIdentity } from "./remote-provider-pack-verification.js"; import { writeEnvironmentCleanup } from "../environment-cleanup-notifications.js"; -import { NATIVE_SESSION_EXECUTION_LEASE_TTL_MS, startNativeSessionExecutionLeaseRenewal, type NativeExecutionLeaseRenewal } from "./native-execution-lease.js"; +import { NATIVE_SESSION_EXECUTION_LEASE_TTL_MS, releaseUnstartedNativeExecutionClaim, startNativeSessionExecutionLeaseRenewal, type NativeExecutionLeaseRenewal } from "./native-execution-lease.js"; export { renewNativeSessionExecutionLease } from "./native-execution-lease.js"; import { DELIVERY_QUEUES, notifyDeliveryWork } from "../delivery-work-notifications.js"; import { writeExecutionControl } from "../execution-control-notifications.js"; @@ -353,6 +353,8 @@ type NativeSessionStartup = { priorRestartDetach?: symbol; leaseOwner?: string; executionLeaseRenewal?: NativeExecutionLeaseRenewal; + /** A fresh claim whose preparation failed before any provider turn. */ + unstartedClaim?: { leaseOwner: string; attempt: number }; executionContext?: NativeExecutionContext; settled: Promise; settle: () => void; @@ -8607,6 +8609,12 @@ export async function executePaperclipNativeSession(input: { // Remove the settled startup before yielding, so detachment cannot reacquire // its old resolved session while renewal cleanup drains. await startup.executionLeaseRenewal?.stop().catch(() => undefined); + if (startup.unstartedClaim && !nativeRunsDetachingForRestart.has(runId)) { + await releaseUnstartedNativeExecutionClaim({ db: input.db, runId, companyId: input.execution.binding.companyId, + issueId: input.execution.binding.issueId, ...startup.unstartedClaim }).catch(error => { + logger.warn({ err: error, runId }, "Native preparation claim release failed; the control deadline will settle it"); + }); + } try { await cleanupStagedAttachments(); } catch (cleanupError) { @@ -8838,6 +8846,7 @@ async function executePaperclipNativeSessionWithinScope( let providerPackAdmissionSource: NativeProviderPackAdmissionSource | undefined; let confirmedLeaseDeadline = 0; let claimedStartupContext: NativeExecutionContext | undefined; + let freshClaimAttempt: number | undefined; try { attempt = await trace.measure( "native.coordinator.claim", @@ -9061,6 +9070,7 @@ async function executePaperclipNativeSessionWithinScope( const startup = nativeSessionStartups.get(input.execution.binding.runId); if (!startup) throw new Error("native_session_startup_owner_missing"); startup.leaseOwner = leaseOwner; + if (!input.restartRecovery) freshClaimAttempt = claimedAttempt; // Own renewal as soon as the claim commits, including slow tracing, // accounting history and retained-session cleanup before dispatch. startup.executionLeaseRenewal = startNativeSessionExecutionLeaseRenewal({ @@ -9106,6 +9116,17 @@ async function executePaperclipNativeSessionWithinScope( } catch (error) { await trace.end(taskPrepareScope, { outcome: "failed" }); await trace.finish("failed"); + // No provider turn started under this fresh claim. Leaving it held makes + // failure settlement treat this finished attempt as a live owner, so the + // run waits for the control deadline. The startup owner releases only this + // exact claim after renewal stops; a restart claim, startup cleanup, Stop or + // restart detach keeps its owner. + const startup = nativeSessionStartups.get(input.execution.binding.runId); + if (startup && freshClaimAttempt !== undefined && !claimedStartupContext + && !(error instanceof NativeCancellationPendingRecoveryError) + && !(error instanceof NativeControllerDetachedForRestartError)) { + startup.unstartedClaim = { leaseOwner, attempt: freshClaimAttempt }; + } throw error; } const controlPlaneInstanceId = `${effectiveRunnerInstanceId}:control`;