mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-15 01:52:55 +01:00
fix(native): continue a task whose quarantined completed turn's holds settle (#20662)
<!-- Write all pull request text in Simplified Technical English (ASD-STE100): short sentences, one instruction per sentence, simple approved vocabulary, and the active voice. --> ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Before a remote run starts, workspace preparation attaches the verified saved work of earlier runs of the same task. > - Preparation failed with `remote_workspace_recovery_receipt_invalid` when any earlier run had a recovery receipt that it could not verify. > - A refusal cannot change the saved state, so every Retry failed in the same way, and the task stayed blocked. Recoveries must fall back automatically. > - This pull request skips an unverifiable source. It attaches nothing unverified and starts from the workspace's newest verified save. > - The benefit is that Retry and continuations start again, while strict receipt checks still decide what is attached, and no saved work is changed. ## Linked Issues or Issue Description Refs #20631 **What happened?** On staging, after the deploy that included #20631, a user pressed Retry on a task whose previous run failed with `native_controller_recovery_missing_evidence:runner_not_settled`. The new run failed in setup in under a minute with `setup_failed` / `remote_workspace_recovery_receipt_invalid` (`executionRecovery: bootstrap`, `providerWorkStarted: false`). The task stayed blocked. Another Retry would fail the same way. #20631 accepted one shape of this refusal (a source that saved only the snapshot its own restore loaded). This task had a different shape. **Expected behavior** A Retry starts. A source whose receipt cannot be verified is not attached, and its saved state stays intact for workspace recovery. **Steps to reproduce** 1. Let a remote native run fail before provider work and leave a recovery receipt or restore hold that does not verify (for example a receipt on an unsaved reference, a receipt from another epoch, or a saved reference without a receipt). 2. Press Retry on the task. 3. See the new run fail in setup with `remote_workspace_recovery_receipt_invalid`. **Paperclip version or commit** Staging deploy that includes `e3bc4ca798`. The gap remains on `master` at `a3b34a9d88`. ## What Changed - `attachPreservedRemoteWorkspaceSnapshots` no longer throws for an unverifiable source. Each of the five refusal shapes now skips the source and logs `remote_workspace_recovery_receipt_invalid` with a reason: `receipt_without_reference`, `reference_unreadable`, `receipt_on_unsaved_reference`, `receipt_unverified`, and `receipt_snapshot_missing` (was `remote_workspace_recovery_snapshot_missing`). - `retainRemoteRecoveryAttachments` no longer throws for a retained recovered copy that it cannot verify. It does not publish that copy again. The files stay in the workspace. - The writer check (`remote_workspace_recovery_writer_changed`), the workspace access check, and snapshot read and digest failures still refuse. - `doc/execution-semantics.md` describes the fallback rule. ## Verification - Red on unfixed code: `remote-workspace-recovery-receipt-fallback.test.ts` (moved out of `workspace-recovery-preparation.test.ts` to keep that file under 1,000 lines) "starts a Retry from the newest verified save when a predecessor has %s" (6 shapes: `receipt_without_reference`, `unreadable_reference_with_hold`, `receipt_on_prepared_reference`, `superseded_snapshot_with_hold`, `receipt_from_other_epoch`, `receipt_for_other_run`). All 6 failed with `remote_workspace_recovery_receipt_invalid`. The source is a native run that failed with `runner_not_settled`, and the writer is a Retry (`retryOfRunId`, `taskRetryRequestId`) that follows the real ancestry query. - Green: the same 6 tests pass. They also assert that nothing is attached, and that the source run row, its workspace operations, and its snapshot row are unchanged. - Updated safety tests: the canonical-attachment fault matrix now expects no attachment for receipt faults, and still expects a refusal for a missing or corrupt archive and for a wrong company or task. The retained-history test expects an unverifiable retained copy to be left out of the published list, while verified copies stay published. - `npx vitest run src/services/remote-workspace-recovery-receipt-fallback.test.ts src/services/workspace-recovery-preparation.test.ts`: 91 passed. - `npx tsc --noEmit -p server/tsconfig.json` passes. - `pnpm verify` returns exit 2, because a global doc input changed (`doc/execution-semantics.md`). Required CI supplies the broad coverage. - Not verified live on Daytona. I could not read the staging rows for this task: the browser had no free tab. The tests cover every refusal shape of this function, so the exact staging shape is one of them. ## Risks - Previous-release state: the rule reads the same rows as before (run reference, result hold, workspace operations, recovery actions, snapshots). Rows that earlier releases saved carry forward. No migration. A blocked task recovers on its next Retry or wake. - A Retry can now start without files from an unverifiable source. Those files are not lost: the source's snapshot, restore hold, receipts and recovery action stay unchanged, and workspace recovery still owns them. The skip is logged with a named reason. - A retained copy that fails verification is dropped from the published list. Its directory stays in the workspace and is no longer retired automatically. - A missing or corrupt snapshot archive still refuses. A permanently lost archive therefore still blocks attach. This PR does not change that. > 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, via Claude Code with tool use (shell, file edits). Extended reasoning enabled. ## 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
8f6ca25976
commit
48c6e996da
5 files changed
+322
-48
No files matched your search
@@ -1,10 +1,16 @@
|
||||
import { and, eq, isNull, sql, type SQL } from "drizzle-orm";
|
||||
import { heartbeatRuns, issueRecoveryActions, nativeRunFinalizations, type Db } from "@paperclipai/db";
|
||||
import { and, eq, isNull, or, sql, type SQL } from "drizzle-orm";
|
||||
import { agentWakeupRequests, heartbeatRuns, issueRecoveryActions, issues, nativeRunFinalizations, type Db } from "@paperclipai/db";
|
||||
import type { ExecutionReconciliation } from "@paperclipai/shared";
|
||||
import { logActivity } from "../activity-log.js";
|
||||
import { DELIVERY_QUEUES, notifyDeliveryWork } from "../delivery-work-notifications.js";
|
||||
import { issueService } from "../issues.js";
|
||||
import { logger } from "../../middleware/logger.js";
|
||||
import type { InProgressRecord, SettleOutcome } from "../recovery/ownership-registry.js";
|
||||
import type { OwnerProof } from "../owner-proof.js";
|
||||
import { OWNER_GONE_ATTESTATION_KEY } from "../recovery/owner-gone-attestation.js";
|
||||
import { holdOwnsBlockedStatus } from "../recovery-hold-blocked-status.js";
|
||||
import { notifyRoutineExecutionStatus } from "../routine-execution-status-notifications.js";
|
||||
import { notifyStatusCardIssueChange } from "../status-card-notifications.js";
|
||||
import { completedWorkspaceRecoveryCondition } from "../workspace-recovery-receipt.js";
|
||||
import { NATIVE_SESSION_CLEANUP_QUARANTINED } from "./completed-turn-cleanup.js";
|
||||
|
||||
@@ -30,6 +36,20 @@ import { NATIVE_SESSION_CLEANUP_QUARANTINED } from "./completed-turn-cleanup.js"
|
||||
* (an assigned person, an owner-gone attestation, a reconciliation or explicit
|
||||
* continuation) are never reopened. Every rule reads fields that earlier
|
||||
* releases already saved, so their holds settle without a migration.
|
||||
*
|
||||
* Settling the holds is not enough when the run's own failure set the task's
|
||||
* Blocked status: nothing else ever releases it, so the task rested Blocked
|
||||
* with no blocker, no Retry and no next action. In the same
|
||||
* transaction, a Blocked status that a settled hold of the run still owns
|
||||
* (`holdOwnsBlockedStatus`) is released to `todo`, and one durable
|
||||
* continuation is recorded on that hold (`continuationDelivery: "pending"`).
|
||||
* The execution-continuation worker delivers it as a fresh session. Saved user
|
||||
* input owns the continuation instead, so the task never starts two turns. A task
|
||||
* that still waits on unfinished blocker tasks stays Blocked; their completion
|
||||
* wakes it. A Blocked status that a newer decision owns is kept. Holds that the
|
||||
* previous release settled without this decision are listed again until it is
|
||||
* recorded (`completedTurnCleanupSettlement.blockedStatus`), so those tasks
|
||||
* continue on the first pass after deploy, without a migration.
|
||||
*/
|
||||
|
||||
const COMPLETED_TURN_CHECKPOINT = sql`run.runner_profile_json->'sessionCheckpoint'->'identity'->>'runId' = run.id::text
|
||||
@@ -52,16 +72,36 @@ function settleableHoldCondition(runId: SQL | string): SQL {
|
||||
)!;
|
||||
}
|
||||
|
||||
/** A hold of `runId` that this kind settled, in this or an earlier release,
|
||||
* with no recorded decision about the task's Blocked status yet. */
|
||||
function settledHoldAwaitingBlockedStatusCondition(runId: SQL | string): SQL {
|
||||
return and(
|
||||
sql`${issueRecoveryActions.evidence}->>'runId' = ${runId}::text`,
|
||||
sql`${issueRecoveryActions.evidence}->'completedTurnCleanupSettlement'->>'runId' = ${runId}::text`,
|
||||
sql`${issueRecoveryActions.evidence}->'completedTurnCleanupSettlement'->'blockedStatus' is null`,
|
||||
eq(issueRecoveryActions.status, "resolved"),
|
||||
eq(issueRecoveryActions.outcome, "restored"),
|
||||
)!;
|
||||
}
|
||||
|
||||
/** Coordinators of completed-turn cleanup quarantines that still have a
|
||||
* settleable no-replay hold. */
|
||||
* settleable no-replay hold, or a settled hold whose task is still Blocked
|
||||
* with no recorded decision about that status. */
|
||||
export function completedTurnCleanupHoldWhere(): SQL {
|
||||
return and(
|
||||
isNull(nativeRunFinalizations.leaseOwner),
|
||||
sql`exists (select 1 from heartbeat_runs run where run.company_id = ${nativeRunFinalizations.companyId}
|
||||
and run.id = ${nativeRunFinalizations.runId} and run.runtime_mode = 'native' and run.status = 'failed'
|
||||
and run.error_code = ${NATIVE_SESSION_CLEANUP_QUARANTINED} and ${COMPLETED_TURN_CHECKPOINT})`,
|
||||
sql`exists (select 1 from ${issueRecoveryActions} where ${issueRecoveryActions.companyId} = ${nativeRunFinalizations.companyId}
|
||||
and ${settleableHoldCondition(sql`${nativeRunFinalizations.runId}`)})`,
|
||||
or(
|
||||
sql`exists (select 1 from ${issueRecoveryActions} where ${issueRecoveryActions.companyId} = ${nativeRunFinalizations.companyId}
|
||||
and ${settleableHoldCondition(sql`${nativeRunFinalizations.runId}`)})`,
|
||||
sql`exists (select 1 from ${issueRecoveryActions} join ${issues} on ${issues.id} = ${issueRecoveryActions.sourceIssueId}
|
||||
and ${issues.companyId} = ${issueRecoveryActions.companyId}
|
||||
where ${issueRecoveryActions.companyId} = ${nativeRunFinalizations.companyId}
|
||||
and ${issueRecoveryActions.sourceIssueId} = ${nativeRunFinalizations.issueId} and ${issues.status} = 'blocked'
|
||||
and ${settledHoldAwaitingBlockedStatusCondition(sql`${nativeRunFinalizations.runId}`)})`,
|
||||
),
|
||||
)!;
|
||||
}
|
||||
|
||||
@@ -89,8 +129,11 @@ export async function settleCompletedTurnCleanupHolds(db: Db, record: InProgress
|
||||
.from(nativeRunFinalizations).where(and(eq(nativeRunFinalizations.companyId, companyId),
|
||||
eq(nativeRunFinalizations.runId, record.recordId), completedTurnCleanupHoldWhere())).for("update");
|
||||
if (!coordinator) return null;
|
||||
const [run] = await tx.select({ agentId: heartbeatRuns.agentId }).from(heartbeatRuns)
|
||||
const [issue] = await tx.select().from(issues).where(and(eq(issues.companyId, companyId),
|
||||
eq(issues.id, coordinator.issueId))).for("update");
|
||||
const [run] = await tx.select().from(heartbeatRuns)
|
||||
.where(and(eq(heartbeatRuns.companyId, companyId), eq(heartbeatRuns.id, coordinator.runId)));
|
||||
if (!issue || !run) return null;
|
||||
const holds = await tx.update(issueRecoveryActions).set({
|
||||
outcome: "restored", ownerType: "system", updatedAt: now,
|
||||
resolutionNote: "The run finished its turn and saved its work; its session cleanup no longer blocks the task.",
|
||||
@@ -99,20 +142,103 @@ export async function settleCompletedTurnCleanupHolds(db: Db, record: InProgress
|
||||
})}::jsonb`,
|
||||
}).where(and(eq(issueRecoveryActions.companyId, companyId), eq(issueRecoveryActions.sourceIssueId, coordinator.issueId),
|
||||
settleableHoldCondition(coordinator.runId))).returning({ id: issueRecoveryActions.id });
|
||||
if (!holds.length) return null;
|
||||
await logActivity(tx as unknown as Db, { companyId, actorType: "system", actorId: "system",
|
||||
action: "issue.recovery_action_resolved", entityType: "issue", entityId: coordinator.issueId,
|
||||
agentId: run?.agentId ?? null, runId: coordinator.runId,
|
||||
details: { reason: "native_completed_turn_cleanup_settled", recoveryActionIds: holds.map(hold => hold.id),
|
||||
ownerProof: context.proof.reason },
|
||||
});
|
||||
return coordinator.runId;
|
||||
if (holds.length) {
|
||||
await logActivity(tx as unknown as Db, { companyId, actorType: "system", actorId: "system",
|
||||
action: "issue.recovery_action_resolved", entityType: "issue", entityId: coordinator.issueId,
|
||||
agentId: run.agentId, runId: coordinator.runId,
|
||||
details: { reason: "native_completed_turn_cleanup_settled", recoveryActionIds: holds.map(hold => hold.id),
|
||||
ownerProof: context.proof.reason },
|
||||
});
|
||||
}
|
||||
const decided = await decideBlockedStatus(tx as unknown as Db, { companyId, issue, run, now });
|
||||
return holds.length || decided ? coordinator.runId : null;
|
||||
});
|
||||
if (!settled) return { settled: false, reason: "hold_changed_since_proof" };
|
||||
// Settling starts no turn. Saved input and the task's own wait continue
|
||||
// through normal admission, which still checks every other gate.
|
||||
// Settling starts no turn itself. Saved input, the recorded continuation and
|
||||
// the task's own wait continue through normal admission, which still checks
|
||||
// every other gate.
|
||||
await settledHoldResumer?.({ companyId, runId: settled }).catch(error => {
|
||||
logger.warn({ err: error, runId: settled }, "settled completed-turn cleanup hold could not resume saved input");
|
||||
});
|
||||
return { settled: true, reason: context.proof.reason };
|
||||
}
|
||||
|
||||
type BlockedStatusDecision = "released" | "dependency_wait" | "not_owned";
|
||||
|
||||
/** Recorded with the continuation. Admission of a reconciled continuation
|
||||
* requires workspace repair evidence after a restore failure; the settled
|
||||
* holds already required the run's files to be verified as saved. */
|
||||
const CONTINUATION_EVIDENCE = "The run completed its provider turn and saved its work before its session cleanup was quarantined. "
|
||||
+ "The controller that held the cleanup gate is proven gone, so the task continues in a fresh session.";
|
||||
const WORKSPACE_EVIDENCE = "Workspace recovery verified this run's saved files, or the run had no unsaved files; "
|
||||
+ "the continuation starts from that saved workspace.";
|
||||
|
||||
/**
|
||||
* Decides, once, what happens to the task's Blocked status after the run's
|
||||
* holds are settled, and records that decision on every settled hold of the
|
||||
* run so the kind does not list them again. Runs in the settlement
|
||||
* transaction with the issue row locked. Returns whether a decision was
|
||||
* recorded.
|
||||
*/
|
||||
async function decideBlockedStatus(tx: Db, input: { companyId: string; now: Date;
|
||||
issue: typeof issues.$inferSelect; run: typeof heartbeatRuns.$inferSelect }): Promise<boolean> {
|
||||
const { companyId, issue, run, now } = input;
|
||||
if (issue.status !== "blocked") return false;
|
||||
const settledHolds = await tx.select().from(issueRecoveryActions).where(and(eq(issueRecoveryActions.companyId, companyId),
|
||||
eq(issueRecoveryActions.sourceIssueId, issue.id), settledHoldAwaitingBlockedStatusCondition(run.id)))
|
||||
.orderBy(issueRecoveryActions.createdAt, issueRecoveryActions.id).for("update");
|
||||
if (!settledHolds.length) return false;
|
||||
// The hold that recorded the run's failure block is its receipt. Only holds
|
||||
// saved before receipts existed fall back to the block's timing.
|
||||
const receipts = settledHolds.filter(hold => hold.evidence.nativeFailureBlock !== undefined);
|
||||
const owner = (receipts.length ? receipts : settledHolds).find(hold => holdOwnsBlockedStatus(hold, issue, run)) ?? null;
|
||||
// Only the task's current assignee continues it.
|
||||
const ownsTask = Boolean(owner && issue.assigneeAgentId && issue.assigneeAgentId === run.agentId);
|
||||
// The shared dependency rule: an unfinished blocker task keeps the task
|
||||
// Blocked, and its completion wakes it (issue_blockers_resolved).
|
||||
const readiness = ownsTask ? await issueService(tx).listDependencyReadiness(companyId, [issue.id], tx) : null;
|
||||
const decision: BlockedStatusDecision = !ownsTask ? "not_owned"
|
||||
: readiness?.get(issue.id)?.isDependencyReady === true ? "released" : "dependency_wait";
|
||||
// Saved user input continues the task itself through normal admission (the
|
||||
// registered resumer). Recording a second continuation would start another
|
||||
// turn after it, so the input owns the continuation, as in
|
||||
// continueFromLastSavedWorkspace. A saved Retry that dispatch declined for
|
||||
// its requester is shown with a Retry that any user with access can press
|
||||
// (retry_authority_denied), so it is not a dead end either; a second,
|
||||
// automatic continuation could race that Retry once access returns.
|
||||
const [waitingInput] = decision === "released" ? await tx.select({ id: agentWakeupRequests.id }).from(agentWakeupRequests)
|
||||
.where(and(eq(agentWakeupRequests.companyId, companyId), eq(agentWakeupRequests.status, "deferred_issue_execution"),
|
||||
eq(agentWakeupRequests.requestedByActorType, "user"), sql`${agentWakeupRequests.payload}->>'issueId' = ${issue.id}`))
|
||||
.limit(1) : [];
|
||||
const continuationHoldId = decision === "released" && !waitingInput ? owner!.id : null;
|
||||
// Register the work before the writes it describes.
|
||||
if (decision === "released") {
|
||||
await notifyDeliveryWork(tx, DELIVERY_QUEUES.executionContinuation);
|
||||
await notifyDeliveryWork(tx, DELIVERY_QUEUES.issueMonitors);
|
||||
await notifyDeliveryWork(tx, DELIVERY_QUEUES.agentTimers);
|
||||
await notifyStatusCardIssueChange(tx, companyId);
|
||||
await notifyRoutineExecutionStatus(tx, issue.id, companyId);
|
||||
}
|
||||
const reconciliation: ExecutionReconciliation = { runId: run.id, providerStopped: true, actionOutcome: "completed",
|
||||
outcomeEvidence: CONTINUATION_EVIDENCE, workspaceRepairEvidence: WORKSPACE_EVIDENCE };
|
||||
for (const hold of settledHolds) {
|
||||
const continuation = hold.id === continuationHoldId;
|
||||
const marked = sql`jsonb_set(${issueRecoveryActions.evidence}, '{completedTurnCleanupSettlement,blockedStatus}', ${JSON.stringify(decision)}::jsonb)`;
|
||||
await tx.update(issueRecoveryActions).set({ updatedAt: now,
|
||||
...(continuation ? {
|
||||
returnOwnerAgentId: run.agentId, nextAction: "The task continues in a fresh session.",
|
||||
evidence: sql`${marked} || ${JSON.stringify({
|
||||
executionReconciliation: { ...reconciliation, actorId: "completed-turn-cleanup", recordedAt: now.toISOString() },
|
||||
continuationDelivery: "pending" })}::jsonb`,
|
||||
} : { evidence: marked }),
|
||||
}).where(and(eq(issueRecoveryActions.companyId, companyId), eq(issueRecoveryActions.id, hold.id)));
|
||||
}
|
||||
if (decision !== "released") return true;
|
||||
await tx.update(issues).set({ status: "todo", unblockDescriptor: null, blockedTransitionAt: null,
|
||||
blockedOwnerNotifiedAt: null, updatedAt: now }).where(and(eq(issues.companyId, companyId), eq(issues.id, issue.id)));
|
||||
await logActivity(tx, { companyId, actorType: "system", actorId: "system", action: "issue.updated",
|
||||
entityType: "issue", entityId: issue.id, agentId: run.agentId, runId: run.id,
|
||||
details: { status: "todo", previousStatus: "blocked", reason: "native_completed_turn_cleanup_continued",
|
||||
recoveryActionId: owner!.id, continuation: continuationHoldId ? "recorded" : "saved_input", _previous: { status: "blocked" } } });
|
||||
return true;
|
||||
}
|
||||
Reference in new issue
Block a user