From 629abc8509957c341b187bece8a8d23b80894353 Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Thu, 5 Nov 2026 11:14:24 -0600 Subject: [PATCH] fix(recovery): keep pending Retry unclickable and scope failure causes (#19268) 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. > - #19260 shows the saved-work recovery step on a task, and records why a recovery attempt failed. > - The review of #19260 found three gaps after the PR merged. > - A scheduled recovery offered Retry while a Retry preparation was already pending. > - Classified failures could get a misleading `provider_error` cause. > - The new cause text in the board handoff had no test. > - This pull request closes these gaps. > - The benefit is a Retry button that matches what is allowed, and causes that name the correct subsystem. ## Linked Issues or Issue Description Refs #19260 **What happened?** After #19260, a task with a scheduled recovery and a pending (`issued` or `processing`) Retry preparation showed an active Retry button. The server only coalesces the second request. Also, a failure that recovery classified as `archive_unsafe`, `source_unavailable` or `snapshot_verification_failed` still recorded a provider cause. The handoff text could then read "last failure: archive_unsafe, cause: provider_error". **Expected behavior** Retry is not offered while a Retry preparation is pending. Only the generic transient fallback records a provider cause. **Steps to reproduce** 1. Request a chat Retry on a task while its saved-work recovery is scheduled. 2. The task projection returns `retry_scheduled` with `canRetry: true`. **Paperclip version or commit** master at b1d00eec. **Deployment mode** Authenticated cloud deployment (staging). ## What Changed - `task-interruption-presentation.ts`: a scheduled recovery returns `canRetry: false` when a Retry preparation for the same run is pending with a valid deadline. - `workspace-recovery.ts`: a cause is recorded only for the generic transient fallback (`provider_unavailable` or `export_interrupted` from an unclassified error). - Tests: presentation `canRetry` with and without a pending preparation; the board handoff text with and without a cause. ## Verification - `task-interruption-presentation.test.ts`, `system-recovery-state.test.ts` and `workspace-recovery.test.ts` pass (419 tests). One workspace-recovery case failed once under full-suite load. It passes when it runs alone. - Server typecheck passes. ## Risks - Low risk. This change affects presentation and diagnostic labels only. Retry admission still checks all gates on the server. ## 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 - [ ] 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 --- server/src/services/system-recovery-state.test.ts | 9 +++++++++ .../services/task-interruption-presentation.test.ts | 5 +++++ server/src/services/task-interruption-presentation.ts | 10 +++++++--- server/src/services/workspace-recovery.test.ts | 3 +++ server/src/services/workspace-recovery.ts | 5 ++++- 5 files changed, 28 insertions(+), 4 deletions(-) diff --git a/server/src/services/system-recovery-state.test.ts b/server/src/services/system-recovery-state.test.ts index ad9b4e7a1b6..04059e8e678 100644 --- a/server/src/services/system-recovery-state.test.ts +++ b/server/src/services/system-recovery-state.test.ts @@ -1,6 +1,7 @@ import { randomUUID } from "node:crypto"; import { describe, expect, it } from "vitest"; import { createSystemRecovery, failSystemRecovery, readSystemRecovery, reserveSystemRecoveryAttempt, SystemRecoveryFailure } from "./system-recovery-state.js"; +import { exhaustedSystemRecoveryAction } from "./system-recovery.js"; describe("durable system recovery budget", () => { it("makes six attempts with exponential delays that survive serialization and exhaustion", () => { @@ -42,6 +43,14 @@ describe("durable system recovery budget", () => { { now: 0, cause: { label: "/home/daytona/secret", code: null, status: null } }); expect(unsafe.failure).not.toHaveProperty("cause"); }); + it("names the last failure and its cause in the board handoff", () => { + const state = reserveSystemRecoveryAttempt(createSystemRecovery(randomUUID()), 0)!; + const withCause = failSystemRecovery(state, new SystemRecoveryFailure("provider_unavailable", "retry_transient"), + { now: 0, cause: { label: "sandbox_not_running", code: null, status: null } }); + expect(exhaustedSystemRecoveryAction(withCause)).toContain("(last failure: provider_unavailable, cause: sandbox_not_running)"); + const withoutCause = failSystemRecovery(state, new SystemRecoveryFailure("archive_unsafe", "service_required"), { now: 0 }); + expect(exhaustedSystemRecoveryAction(withoutCause)).toContain("(last failure: archive_unsafe)"); + }); it("reconciles a recorded attempt after restart without spending another retry", () => { const running = reserveSystemRecoveryAttempt(createSystemRecovery(randomUUID()), 0)!; const restarted = readSystemRecovery(JSON.parse(JSON.stringify({ systemRecovery: running })))!; diff --git a/server/src/services/task-interruption-presentation.test.ts b/server/src/services/task-interruption-presentation.test.ts index 1a01b402c14..ff2de4ab0c1 100644 --- a/server/src/services/task-interruption-presentation.test.ts +++ b/server/src/services/task-interruption-presentation.test.ts @@ -202,6 +202,11 @@ describe("saved-work recovery step", () => { const running = action({ systemRecovery: system("preserve_files", "running", null), systemRecoveryActiveAt: now - 1_000 }); expect(projectTaskInterruption(issue, { run: failed, agent, wait, paused: false, approval: false, action: running }, now)) .toMatchObject({ state: "working", recoveryStep: "saving_previous_files" }); + const scheduled = action({ systemRecovery: system("verify_execution", "scheduled", now + 30_000) }); + expect(projectTaskInterruption(issue, { run: failed, agent, wait, paused: false, approval: false, action: scheduled }, now)) + .toMatchObject({ state: "retry_scheduled", recoveryStep: "checking_previous_run", canRetry: false }); + expect(projectTaskInterruption(issue, { run: failed, agent, paused: false, approval: false, action: scheduled }, now)) + .toMatchObject({ state: "retry_scheduled", canRetry: true }); expect(projectTaskInterruption(issue, { run: failed, agent, wait: { ...wait, preparationStatus: "failed" }, paused: false, approval: false, action: running }, now)).toMatchObject({ state: "failed" }); expect(projectTaskInterruption(issue, { run: failed, agent, wait, paused: false, approval: false }, now)) diff --git a/server/src/services/task-interruption-presentation.ts b/server/src/services/task-interruption-presentation.ts index 0878f2dad04..6379b274ce9 100644 --- a/server/src/services/task-interruption-presentation.ts +++ b/server/src/services/task-interruption-presentation.ts @@ -115,9 +115,13 @@ export function projectTaskInterruption(issue: ProjectionIssue, input: Projectio const recoveryRunning = system?.state === "running" && typeof activeObservation === "number" && activeObservation > now - 45_000; const recoveryScheduled = Boolean(system?.nextAttemptAt && system.nextAttemptAt > now); + const preparationDeadline = retryWait?.preparationRetryAt == null ? NaN : Number(retryWait.preparationRetryAt); + // A pending Retry preparation is already a Retry; never offer another one. + const preparationPending = retryWait?.sourceRunId === run.id && Boolean(retryWait.preparationStatus) + && ["issued", "processing"].includes(retryWait.preparationStatus!) && Number.isFinite(preparationDeadline); if (retryWait?.sourceRunId === run.id && retryWait.preparationStatus) { - const deadline = retryWait.preparationRetryAt == null ? NaN : Number(retryWait.preparationRetryAt); - const pending = ["issued", "processing"].includes(retryWait.preparationStatus) && Number.isFinite(deadline); + const deadline = preparationDeadline; + const pending = preparationPending; // Saved-work recovery gates a pending preparation, so it names the wait. // Failed or cancelled preparation stays authoritative. if (pending && !recoveryRunning && !recoveryScheduled) @@ -126,7 +130,7 @@ export function projectTaskInterruption(issue: ProjectionIssue, input: Projectio } const retryAt = system?.nextAttemptAt ?? run.scheduledRetryAt?.getTime(); if (retryAt && retryAt > now) return { ...base, state: "retry_scheduled", - nextRetryAt: new Date(retryAt).toISOString(), canRetry: true, + nextRetryAt: new Date(retryAt).toISOString(), canRetry: !preparationPending, ...(system?.nextAttemptAt && recoveryStep ? { recoveryStep } : {}) }; if (recoveryRunning) return { ...base, state: "working", ...(recoveryStep ? { recoveryStep } : {}) }; if (retryWait?.sourceRunId === run.id && retryWait.executionWaitReason) { diff --git a/server/src/services/workspace-recovery.test.ts b/server/src/services/workspace-recovery.test.ts index 659533fe842..ad2d0314ff1 100644 --- a/server/src/services/workspace-recovery.test.ts +++ b/server/src/services/workspace-recovery.test.ts @@ -3312,6 +3312,8 @@ describe("automatic retained workspace recovery", () => { try { await recoverRetainedWorkspace(f.input); } finally { restore.mockRestore(); } expect(readSystemRecovery((await action(f.actionId)).evidence)).toMatchObject({ state: "exhausted", failure: { code: "snapshot_verification_failed", retryClass: "service_required" }, preservationOperationIds: [] }); + // A classified failure names its own subsystem; it never gets a provider cause. + expect(readSystemRecovery((await action(f.actionId)).evidence)?.failure).not.toHaveProperty("cause"); expect(await hasVerifiedWorkspaceRepair(db, await run(f.runId))).toBe(false); expect(await readFile(path.join(f.remote, "saved.txt"), "utf8")).toBe("finished work\n"); expect(await readFile(path.join(f.host, "newer.txt"), "utf8")).toBe("newer local work\n"); @@ -4299,6 +4301,7 @@ describe("automatic retained workspace recovery", () => { expect(readSystemRecovery((await action(f.actionId)).evidence)).toMatchObject({ state: "exhausted", attempts, failure: failure === "disk_full" ? { code: "source_unavailable", retryClass: "use_alternative_strategy" } : { code: "provider_unavailable", retryClass: "retry_transient", cause: { label: "provider_error", code: null, status: null } } }); + if (failure === "disk_full") expect(readSystemRecovery((await action(f.actionId)).evidence)?.failure).not.toHaveProperty("cause"); expect(f.runtime.resumeRunLease).toHaveBeenCalledTimes(attempts); expect(await readFile(path.join(f.remote, "saved.txt"), "utf8")).toBe("finished work\n"); }, 60_000); diff --git a/server/src/services/workspace-recovery.ts b/server/src/services/workspace-recovery.ts index a2f98a733bd..766229bc093 100644 --- a/server/src/services/workspace-recovery.ts +++ b/server/src/services/workspace-recovery.ts @@ -422,7 +422,10 @@ export async function recoverRetainedWorkspace(input: Source & { db: Db; environ const retryAfterAt = (error as { retryAfterAt?: unknown })?.retryAfterAt; // A generic transient code says only that the step can be retried. Keep // why it failed in closed labels so a repeated deferral is diagnosable. - const cause = error instanceof SystemRecoveryFailure ? undefined : sandboxProviderFailureLabel(error); + // A classified failure already names its subsystem. + const generic = !(error instanceof SystemRecoveryFailure) && failure.retryClass === "retry_transient" + && (failure.code === "provider_unavailable" || failure.code === "export_interrupted"); + const cause = generic ? sandboxProviderFailureLabel(error) : undefined; await updateSystemRecovery(db, input, current!, failSystemRecovery(current!, failure, { ...(typeof retryAfterAt === "number" && Number.isFinite(retryAfterAt) ? { retryAfterAt } : {}), ...(cause ? { cause } : {}),