mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-08 22:13:59 +01:00
fix(native): release a fresh claim when task preparation fails (#19065)
## 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 <noreply@anthropic.com>
This commit is contained in:
1 parent
9cdbcbeddc
commit
5723fd82ca
5 files changed
+114
-2
No files matched your search
@@ -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
|
||||
|
||||
@@ -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<boolean> {
|
||||
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;
|
||||
|
||||
@@ -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() })
|
||||
|
||||
@@ -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<string, unknown> }> = [];
|
||||
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<string, unknown> }> = [];
|
||||
const onEvent = vi.fn(async () => undefined);
|
||||
|
||||
@@ -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<void>;
|
||||
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`;
|
||||
|
||||
Reference in new issue
Block a user