From c58356003f6790b9c74d026a7ef8f0919aec0e45 Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Tue, 10 Nov 2026 09:45:39 -0600 Subject: [PATCH] fix(native): give run finalization its own retry budget (#20555) 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 > - Native runs save an accepted result. Then a finalizer applies that result to the issue. > - After a control-plane restart, recovery can resume the same run. Each resume uses one provider attempt (`coordinator.attempt`). > - The finalizer used the same `coordinator.attempt` value as its own retry budget. After three resumes, `attempt` was 3. So the first transient finalization failure made the coordinator `terminal_failure`. > - The result was then lost: the issue stayed `in_review` and no Retry was available. This breaks the rule that a restart must not leave a task stuck. > - This pull request gives finalization its own retry count. Provider attempts no longer use up finalization retries. > - The benefit: a resumed run that finishes its turn also finishes its task, and finalization stays bounded. ## Linked Issues or Issue Description **What happened?** In restart-chaos wave `w-20261110T044338-s20385`, task t03 (local, Codex, three K1 SIGKILL restarts, `--db-pressure 4`), recovery resumed run `d5d0e839` three times (`providerAttempt: 3`). The resumed turn completed and the server logged `run.result.accepted`, and the run ended `succeeded`. The first finalization hit `NativeStatusRaceError` three times under row contention. `recordRetryableFailure` then compared `coordinator.attempt` (3) with its limit of 3 and wrote `terminal_failure` / `native_finalization_retry_exhausted`. The run's `execution.nextAction` is "Reassess against the latest authoritative issue status version.", which only the `status_cas_exhausted` branch writes. The second finalize call 2 s later got `native_finalization_terminal_failure` from `claimCoordinator`. The issue stayed `in_review` with `canRetry=false`. **Expected behavior** A transient finalization failure must stay retryable. The finalizer must apply the saved result without another provider turn. Finalization must still have a limit. **Steps to reproduce** 1. Resume a native run until its coordinator `attempt` is 3, for example with three restarts. 2. Let the resumed run save an accepted result. 3. Make the first finalization fail with a retryable error. 4. Before this fix, the coordinator goes directly to `terminal_failure`. The new test `server/src/__tests__/native-finalization-retry-budget.test.ts` reproduces this order. **Paperclip version or commit** The wave ran at e2b0290213. The bug is still on master at 902f376796. I found no later merged PR that changes this check. ## What Changed - `native-run-finalizer.ts`: `recordRetryableFailure` counts failed finalizations of the saved result in `heartbeat_runs.result_json.finalizationFailureCount`. It ends retries after 3 failures. It no longer reads `coordinator.attempt`, which counts provider attempts. The count goes on the run because each claim clears the coordinator's `failure_detail`. Workspace-scope failures keep their existing `workspaceFinalizeAttempt` budget. - New regression test `native-finalization-retry-budget.test.ts`: a coordinator saved at `attempt: 3` with no count stays `retryable_failure` after the first failure. It goes to `terminal_failure` only after the third failure. ## Verification - `cd server && npx vitest run src/__tests__/native-finalization-retry-budget.test.ts src/__tests__/native-finalization-recovery.test.ts`: 37 tests pass. - The new test fails on the unchanged finalizer (`terminal_failure` received, `retryable_failure` expected). It passes with the fix. - `pnpm verify` (selected tests). - I did not run a live chaos replay. The evidence is the wave's run events, its server log, and the code path. ## Risks - Low risk. The only behavior change: runs that used provider attempts now get their full 3 finalization retries. - Previous-release story: rows that an older server saved have no `finalizationFailureCount`. Their count starts at 0, so they get at most 3 more finalization tries, with no provider work. A coordinator that an older server already set to `terminal_failure` stays terminal. This PR migrates nothing. - If another writer replaces `result_json` without spreading it, the count can reset. That allows only a few more cheap retries, 30 s apart. It cannot cause an unbounded loop unless the overwrite repeats. - Not addressed here: when finalization really runs out of retries for a succeeded run, the issue still rests `in_review` with a board recovery card and no Retry. A safe Retry for that case must re-finalize the saved result, not start a new provider run. That needs a separate change. ## Model Used - Anthropic Claude, model ID `claude-opus-5-5`, used through Claude Code with tool use (shell, code editing, test execution). ## 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 --- .../native-finalization-retry-budget.test.ts | 132 ++++++++++++++++++ .../native-run-finalizer-telemetry.test.ts | 19 ++- .../native-runtime/native-run-finalizer.ts | 19 ++- 3 files changed, 168 insertions(+), 2 deletions(-) create mode 100644 server/src/__tests__/native-finalization-retry-budget.test.ts diff --git a/server/src/__tests__/native-finalization-retry-budget.test.ts b/server/src/__tests__/native-finalization-retry-budget.test.ts new file mode 100644 index 00000000000..2291f0bf82e --- /dev/null +++ b/server/src/__tests__/native-finalization-retry-budget.test.ts @@ -0,0 +1,132 @@ +import { resolveExternalTestDatabaseUrl } from "./helpers/external-test-database.js"; +import { afterAll, beforeAll, describe, expect, it } from "vitest"; +import { eq } from "drizzle-orm"; +import { + agents, + companies, + completionContracts, + createDb, + heartbeatRuns, + issues, + nativeRunFinalizations, + nativeRunResults, + workspaceOperations, +} from "@paperclipai/db"; +import { + CONTROL_PLANE_CONFORMANCE_OPEN, + CONTROL_PLANE_CONFORMANCE_RESULT, + CONTROL_PLANE_CONFORMANCE_TERMINAL, +} from "../vendor/paperclip-runner/testing.js"; +import { startEmbeddedPostgresTestDatabase } from "./helpers/embedded-postgres.js"; +import { reconcileNativeFinalizations } from "../services/native-runtime/native-finalization-reconciler.js"; +import { PaperclipControlPlanePort } from "../services/native-runtime/paperclip-control-plane-port.js"; + +const external = resolveExternalTestDatabaseUrl("PAPERCLIP_NATIVE_FINALIZATION_TEST_DATABASE_URL"); + +// Restart chaos w-20261110T044338-s20385/t03: three restarts resumed the run +// (provider attempt 3), the resumed turn's result was accepted, and the first +// transient finalization failure was judged against the provider attempt +// count. The coordinator went terminal at once and the issue rested in review +// with no Retry. +describe("native finalization retry budget after restart resumes", () => { + let temporary: Awaited>; + let db: ReturnType; + const companyId = "72100000-0000-4000-8000-000000000001"; + const agentId = "72100000-0000-4000-8000-000000000002"; + const issueId = "72100000-0000-4000-8000-000000000003"; + const contractId = "72100000-0000-4000-8000-000000000004"; + const runId = "72100000-0000-4000-8000-000000000005"; + + beforeAll(async () => { + if (!external) temporary = await startEmbeddedPostgresTestDatabase("paperclip-native-retry-budget-"); + db = createDb(external ?? temporary.connectionString); + await db.insert(companies).values({ id: companyId, name: "Native retry budget", issuePrefix: "NRB" }); + await db.insert(agents).values({ id: agentId, companyId, name: "Budget agent", adapterType: "codex_local", status: "running" }); + await db.insert(issues).values({ + id: issueId, companyId, title: "Finalize after three resumes", status: "in_progress", + assigneeAgentId: agentId, workMode: "standard", + }); + await db.insert(completionContracts).values({ + id: contractId, companyId, issueId, revision: 1, + schemaVersion: "paperclip.completion-contract.v1", policyVersion: "phase6-v1", risk: "standard", + completionAuthority: "server_arbiter", incompleteCriteriaPolicy: "preserve_non_terminal", + contractJson: { + revision: "phase6-v1", objective: "Finalize after three resumes", + criteria: [{ id: "objective", requirement: "Finalization keeps its own budget" }], + }, + canonicalSha256: "native-retry-budget-contract", createdByActorType: "system", createdByActorId: "test", + }); + await db.insert(heartbeatRuns).values({ + id: runId, companyId, agentId, status: "running", runtimeMode: "native", + runtimeModeReason: "persisted_before_kill_switch", nativeIssueId: issueId, nativeSessionId: runId, + runnerInstanceId: contractId, completionContractId: contractId, + completionContractSha256: "native-retry-budget-contract", contextSnapshot: { issueId }, + }); + const port = new PaperclipControlPlanePort(db, { + companyId, issueId, runId, agentId, sessionId: runId, completionContractId: contractId, + completionContractSha256: "native-retry-budget-contract", sourceInstanceId: contractId, + controlPlaneSourceInstanceId: "budget-control", + }); + await port.openRun({ + ...CONTROL_PLANE_CONFORMANCE_OPEN, + identity: { companyId, issueId, runId, agentId, sessionId: runId }, + sourceInstanceId: contractId, + }); + await port.completeRun({ + result: CONTROL_PLANE_CONFORMANCE_RESULT, + terminal: CONTROL_PLANE_CONFORMANCE_TERMINAL, + callerResultId: "budget-result", + }); + // Each finalization attempt fails, as a status race under contention did. + const stored = await db.select().from(nativeRunResults).where(eq(nativeRunResults.runId, runId)) + .limit(1).then((rows) => rows[0]!); + await db.update(nativeRunResults).set({ + resultJson: { + ...(stored.resultJson as Record), + terminal: { ...CONTROL_PLANE_CONFORMANCE_TERMINAL, runTerminalState: "unknown" }, + }, + }).where(eq(nativeRunResults.id, stored.id)); + await db.insert(workspaceOperations).values({ + companyId, heartbeatRunId: runId, issueId, phase: "workspace_finalize", status: "succeeded", + }); + // Saved as the previous release left it after three restart resumes: the + // provider attempt count is spent and no finalization count exists yet. + await db.update(nativeRunFinalizations).set({ attempt: 3 }).where(eq(nativeRunFinalizations.runId, runId)); + }, 30_000); + + afterAll(async () => { + await db?.$client.end({ timeout: 1 }); + await temporary?.cleanup(); + }); + + it("keeps a resumed run's first finalization failure retryable and still bounds finalization", async () => { + await expect(reconcileNativeFinalizations(db, [runId])).resolves.toEqual([ + expect.objectContaining({ phase: "retryable_failure", failureCode: "native_finalization_invalid" }), + ]); + await expect(db.select().from(issues).where(eq(issues.id, issueId))).resolves.toEqual([ + expect.objectContaining({ status: "in_progress" }), + ]); + await expect(db.select().from(heartbeatRuns).where(eq(heartbeatRuns.id, runId))).resolves.toEqual([ + expect.objectContaining({ + nativePhase: "retryable_failure", + resultJson: expect.objectContaining({ finalizationFailureCount: 1 }), + }), + ]); + + await db.update(nativeRunFinalizations).set({ nextAttemptAt: new Date(0) }) + .where(eq(nativeRunFinalizations.runId, runId)); + await expect(reconcileNativeFinalizations(db, [runId])).resolves.toEqual([ + expect.objectContaining({ phase: "retryable_failure" }), + ]); + + await db.update(nativeRunFinalizations).set({ nextAttemptAt: new Date(0) }) + .where(eq(nativeRunFinalizations.runId, runId)); + await reconcileNativeFinalizations(db, [runId]); + await expect(db.select().from(nativeRunFinalizations).where(eq(nativeRunFinalizations.runId, runId))).resolves.toEqual([ + expect.objectContaining({ phase: "terminal_failure", failureCode: "native_finalization_retry_exhausted" }), + ]); + await expect(db.select().from(heartbeatRuns).where(eq(heartbeatRuns.id, runId))).resolves.toEqual([ + expect.objectContaining({ resultJson: expect.objectContaining({ finalizationFailureCount: 3 }) }), + ]); + }); +}); diff --git a/server/src/services/native-runtime/native-run-finalizer-telemetry.test.ts b/server/src/services/native-runtime/native-run-finalizer-telemetry.test.ts index df7aa4b6c04..7ec2ea198b1 100644 --- a/server/src/services/native-runtime/native-run-finalizer-telemetry.test.ts +++ b/server/src/services/native-runtime/native-run-finalizer-telemetry.test.ts @@ -858,6 +858,9 @@ describeEmbeddedPostgres("native run finalizer / status decision committer — a it("records the finalization error code when provider-scope retries fail an unsucceeded run", async () => { const fixture = await seedNativeRun(); + // Two earlier finalizations of this result failed; this is the third. + await db.update(heartbeatRuns).set({ resultJson: { finalizationFailureCount: 2 } }) + .where(eq(heartbeatRuns.id, fixture.runId)); await db.insert(nativeRunFinalizations).values({ runId: fixture.runId, companyId, issueId: fixture.issueId, phase: "observed", attempt: 3, }); @@ -869,6 +872,20 @@ describeEmbeddedPostgres("native run finalizer / status decision committer — a expect(run?.error).toBeTruthy(); }); + it("keeps a provider-succeeded run's first finalization failure retryable after three provider attempts", async () => { + const fixture = await seedNativeRun(); + await db.update(heartbeatRuns).set({ resultJson: { prpRunTerminalState: "succeeded" } }) + .where(eq(heartbeatRuns.id, fixture.runId)); + await db.insert(nativeRunFinalizations).values({ + runId: fixture.runId, companyId, issueId: fixture.issueId, phase: "observed", attempt: 3, + }); + const failure = await recordNativeFinalizationFailure({ + db, runId: fixture.runId, error: new Error("native_finalization_invalid"), projectRunStatus: true, + }); + expect(failure).toMatchObject({ phase: "retryable_failure", failureCode: "native_finalization_invalid" }); + await expect(readRunError(fixture.runId)).resolves.toEqual({ status: "running", errorCode: null, error: null }); + }); + it("fails a provider-succeeded run with an error code when workspace retries are exhausted", async () => { // The reported case: the provider turn succeeded, then the workspace // copy-back failed three times. Workspace scope fails the run regardless @@ -887,7 +904,7 @@ describeEmbeddedPostgres("native run finalizer / status decision committer — a it("does not attach a finalization error to a provider-succeeded run when provider retries are exhausted", async () => { const fixture = await seedNativeRun(); - await db.update(heartbeatRuns).set({ resultJson: { prpRunTerminalState: "succeeded" } }) + await db.update(heartbeatRuns).set({ resultJson: { prpRunTerminalState: "succeeded", finalizationFailureCount: 2 } }) .where(eq(heartbeatRuns.id, fixture.runId)); await db.insert(nativeRunFinalizations).values({ runId: fixture.runId, companyId, issueId: fixture.issueId, phase: "observed", attempt: 3, diff --git a/server/src/services/native-runtime/native-run-finalizer.ts b/server/src/services/native-runtime/native-run-finalizer.ts index 80768cfc70d..3b62586d7e7 100644 --- a/server/src/services/native-runtime/native-run-finalizer.ts +++ b/server/src/services/native-runtime/native-run-finalizer.ts @@ -326,6 +326,14 @@ function terminalFinalizationFailureMessage(failureCode: string, failureScope: " : `Native run finalization failed (${failureCode}).`; } +const NATIVE_FINALIZATION_RETRY_LIMIT = 3; + +/** Failed finalizations of this run's saved result. Rows saved before the count existed start at zero. */ +function readFinalizationFailureCount(resultJson: unknown) { + const count = record(resultJson).finalizationFailureCount; + return typeof count === "number" && Number.isInteger(count) && count >= 0 ? count : 0; +} + async function recordRetryableFailure(input: { db: Db; run: typeof heartbeatRuns.$inferSelect; @@ -449,11 +457,19 @@ async function recordRetryableFailure(input: { ? priorFailureDetail.workspaceFinalizeAttempt : 0) + 1 : null; + // Finalization has its own budget. `coordinator.attempt` counts provider + // attempts, so a run resumed three times after restarts would otherwise + // turn its first transient finalization failure into a terminal one and + // strand an accepted result. The count lives on the run's result because + // every claim clears the coordinator's failure detail. + const finalizationFailureCount = input.failureScope === "workspace" + ? null + : readFinalizationFailureCount(input.run.resultJson) + 1; const exhausted = input.permanent === true || (workspaceFinalizeAttempt !== null ? workspaceFinalizeAttempt >= 3 - : input.coordinator.attempt >= 3); + : finalizationFailureCount! >= NATIVE_FINALIZATION_RETRY_LIMIT); const phase = supersededByNewerRun || exhausted ? ("terminal_failure" as const) @@ -541,6 +557,7 @@ async function recordRetryableFailure(input: { originalFailureCode: input.failureCode, // The latest attempt's redacted cause; cleared when it named none. failureCause, + ...(finalizationFailureCount === null ? {} : { finalizationFailureCount }), nextAttemptAt: supersededByNewerRun || exhausted ? null : nextAttemptAt.toISOString(), })}::jsonb`, updatedAt: now,