mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-08 01:57:31 +01:00
fix(recovery): keep pending Retry unclickable and scope failure causes (#19268)
## 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 <noreply@anthropic.com>
This commit is contained in:
1 parent
6293e581f6
commit
629abc8509
5 files changed
+28
-4
No files matched your search
@@ -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 })))!;
|
||||
|
||||
@@ -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))
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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 } : {}),
|
||||
|
||||
Reference in new issue
Block a user