From e7936fb1adba6e707ec680221f0b480ff8a3f80c Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Sat, 7 Nov 2026 10:37:39 -0600 Subject: [PATCH] fix(heartbeat): gate process-loss retries on the shared failure budget (#19856) 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 > - When a server restart loses a run's process, the orphan reaper decides whether to retry the run > - `doc/execution-semantics.md` says process loss uses the shared durable failure retry counter, and `enqueueProcessLossRetry` already schedules against that budget > - The reaper still guards on `processLossRetryCount < 1`, but nothing has written that column since the move to the shared budget, so the guard does nothing > - So an exhausted chain still says "retrying once" and skips the interaction continuation path, and a legacy row with a non-zero counter is refused a retry it is owed > - This pull request replaces the dead guard with `failedRunRetryBudgetSpent`, the budget that the retry actually spends > - The benefit is one restart budget, read and written in one place, with honest run messages ## Linked Issues or Issue Description **Bug description** The Phase 0 ownership inventory (finding 5) found that `processLossRetryCount` has no writer and that its only reader is the reaper guard at `heartbeat/recovery.ts:2675`. The guard always passes for new rows. For rows written by earlier releases, it refuses a retry even when the shared failure budget has room. **Steps to reproduce** 1. Seed a legacy run that a restart stopped before adapter dispatch. Spend its whole failure retry budget (`executionRetryAccounting.failureRetries = max`). 2. Run `reapOrphanedRuns()`. 3. The run fails with "Process lost -- server may have restarted; retrying once", but no retry is scheduled. **Expected behavior** An exhausted budget gives no promise of a retry. A legacy counter value does not block a retry that the shared budget allows. Refs #19846 ## What Changed - `heartbeat/recovery.ts`: `shouldRetry` now checks `!failedRunRetryBudgetSpent(run)` instead of `processLossRetryCount < 1`. When the budget is spent, the reaper falls through to `scheduleInteractionContinuationInfrastructureRetryIfEligible`, which is what the old guard intended. - `packages/db/src/schema/heartbeat_runs.ts`: a comment marks `process_loss_retry_count` as legacy. There is no schema or migration change. - Tests: the three cases that used `processLossRetryCount: 1` to mean "retry already spent" now spend the real failure budget. Two new cases cover a legacy counter (a retry is still scheduled) and a spent budget (no "retrying once", `retryEligible: false`, no retry run). ## Verification - `npx vitest run src/__tests__/heartbeat-process-recovery.test.ts -t "process-loss|failure budget|bypass unknown process outcomes|immediate terminal-run cleanup|paused-tree work|restart stopped before adapter dispatch|reached adapter dispatch"`: 7 passed. - Red before the fix: with the old `recovery.ts`, both new cases fail. Green after. - Server `tsc --noEmit` shows no errors in the changed files. - The full `heartbeat-process-recovery.test.ts` file (419 cases) stalled locally at 0% CPU after about 15 minutes. That run was on a worktree whose package builds were stale at first. I ran the focused subset instead, and required CI runs the full file. ## Risks - Low risk. The retry decision now follows the same budget that `enqueueProcessLossRetry` already enforced. A run with budget left behaves as before. - Previous-release story: rows saved with `process_loss_retry_count > 0` are no longer refused for that reason alone. The shared failure budget, which earlier releases also wrote, bounds them. The column stays, so earlier-release readers (`queue.ts` startup resume) keep working. ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`), Anthropic, used 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 --- packages/db/src/schema/heartbeat_runs.ts | 3 + .../heartbeat-process-recovery.test.ts | 57 +++++++++++++++++-- server/src/services/heartbeat/recovery.ts | 9 ++- 3 files changed, 61 insertions(+), 8 deletions(-) diff --git a/packages/db/src/schema/heartbeat_runs.ts b/packages/db/src/schema/heartbeat_runs.ts index 1d2e25466f0..247624e48c0 100644 --- a/packages/db/src/schema/heartbeat_runs.ts +++ b/packages/db/src/schema/heartbeat_runs.ts @@ -104,6 +104,9 @@ export const heartbeatRuns = pgTable( retryOfRunId: uuid("retry_of_run_id").references((): AnyPgColumn => heartbeatRuns.id, { onDelete: "set null", }), + // Legacy: no writer since process loss moved onto the durable failure + // retry budget (contextSnapshot.executionRetryAccounting). Kept so rows and + // readers from earlier releases stay valid; never gate retries on it. processLossRetryCount: integer("process_loss_retry_count").notNull().default(0), scheduledRetryAt: timestamp("scheduled_retry_at", { withTimezone: true }), scheduledRetryAttempt: integer("scheduled_retry_attempt").notNull().default(0), diff --git a/server/src/__tests__/heartbeat-process-recovery.test.ts b/server/src/__tests__/heartbeat-process-recovery.test.ts index 86db90d91ad..518efef8187 100644 --- a/server/src/__tests__/heartbeat-process-recovery.test.ts +++ b/server/src/__tests__/heartbeat-process-recovery.test.ts @@ -1,4 +1,5 @@ import { persistHeartbeatRunProcessMetadata } from "../services/heartbeat/run-lifecycle.js"; +import { BOUNDED_TRANSIENT_HEARTBEAT_RETRY_MAX_ATTEMPTS } from "../services/heartbeat/retries.js"; import { enableRunFailureReporting } from "./helpers/run-failure-reporting.js"; import { EMBEDDED_POSTGRES_TEST_TIMEOUT_MS } from "@paperclipai/db"; import { createSystemRecovery } from "../services/system-recovery-state.js"; @@ -499,6 +500,14 @@ async function spawnOrphanedProcessGroup() { }; } + +/** Failure retry accounting with every automatic failure retry already used. */ +const spentFailureRetryAccounting = { + version: 1, + failureRetries: BOUNDED_TRANSIENT_HEARTBEAT_RETRY_MAX_ATTEMPTS, + maxTurnContinuations: 0, +}; + describeEmbeddedPostgres("heartbeat orphaned process recovery", () => { let db!: ReturnType; let tempDb: Awaited< @@ -851,6 +860,7 @@ describeEmbeddedPostgres("heartbeat orphaned process recovery", () => { processPid?: number | null; processGroupId?: number | null; processLossRetryCount?: number; + failureRetryBudgetSpent?: boolean; runtimeMode?: "legacy" | "native"; includeIssue?: boolean; runErrorCode?: string | null; @@ -914,10 +924,12 @@ describeEmbeddedPostgres("heartbeat orphaned process recovery", () => { triggerDetail: "system", status: input?.runStatus ?? "running", wakeupRequestId, - contextSnapshot: - input?.includeIssue === false + contextSnapshot: { + ...(input?.includeIssue === false ? (input?.contextSnapshot ?? {}) - : { ...(input?.contextSnapshot ?? {}), issueId }, + : { ...(input?.contextSnapshot ?? {}), issueId }), + ...(input?.failureRetryBudgetSpent ? { executionRetryAccounting: spentFailureRetryAccounting } : {}), + }, processPid: input?.processPid ?? null, processGroupId: input?.processGroupId ?? null, processLossRetryCount: input?.processLossRetryCount ?? 0, @@ -3542,6 +3554,39 @@ describeEmbeddedPostgres("heartbeat orphaned process recovery", () => { expect(mockAdapterExecute).not.toHaveBeenCalled(); }); + it("retries within the failure budget even when a legacy process-loss counter is set", async () => { + const { companyId, runId } = await seedRestartOrphanedLegacySetup("preparing"); + // Earlier releases wrote this counter; nothing writes it now. + await db.update(heartbeatRuns).set({ processLossRetryCount: 1 }).where(eq(heartbeatRuns.id, runId)); + + await heartbeatService(db).reapOrphanedRuns(); + + const retries = await db.select().from(heartbeatRuns).where(and( + eq(heartbeatRuns.companyId, companyId), + eq(heartbeatRuns.retryOfRunId, runId), + )); + expect(retries).toEqual([expect.objectContaining({ status: "scheduled_retry" })]); + }); + + it("does not promise a process-loss retry once the failure budget is spent", async () => { + const { companyId, runId } = await seedRestartOrphanedLegacySetup("preparing"); + const [seeded] = await db.select().from(heartbeatRuns).where(eq(heartbeatRuns.id, runId)); + await db.update(heartbeatRuns).set({ + contextSnapshot: { ...(seeded!.contextSnapshot ?? {}), executionRetryAccounting: spentFailureRetryAccounting }, + }).where(eq(heartbeatRuns.id, runId)); + + await heartbeatService(db).reapOrphanedRuns(); + + const [reaped] = await db.select().from(heartbeatRuns).where(eq(heartbeatRuns.id, runId)); + expect(reaped).toMatchObject({ status: "failed", errorCode: "process_lost", + error: "Process lost -- server may have restarted" }); + expect(reaped?.resultJson).toMatchObject({ processLossDiagnostic: { retryEligible: false } }); + expect(await db.select().from(heartbeatRuns).where(and( + eq(heartbeatRuns.companyId, companyId), + eq(heartbeatRuns.retryOfRunId, runId), + ))).toEqual([]); + }); + it("keeps an orphaned legacy run that reached adapter dispatch held for reconciliation", async () => { const { companyId, runId, issueId } = await seedRestartOrphanedLegacySetup("dispatching"); @@ -5110,7 +5155,7 @@ describeEmbeddedPostgres("heartbeat orphaned process recovery", () => { adapterType: "process", agentStatus: "idle", processPid: 999_999_999, - processLossRetryCount: 1, + failureRetryBudgetSpent: true, }); const heartbeat = heartbeatService(db); await heartbeat.reapOrphanedRuns(); @@ -5135,7 +5180,7 @@ describeEmbeddedPostgres("heartbeat orphaned process recovery", () => { adapterType: "process", agentStatus: "idle", processPid: 999_999_999, - processLossRetryCount: 1, + failureRetryBudgetSpent: true, runErrorCode: "process_lost", runError: "Authorization: Bearer sk-test-recovery-secret", }); @@ -5212,7 +5257,7 @@ describeEmbeddedPostgres("heartbeat orphaned process recovery", () => { const { companyId, agentId, runId, issueId } = await seedRunFixture({ agentStatus: "idle", processPid: 999_999_999, - processLossRetryCount: 1, + failureRetryBudgetSpent: true, }); await db.insert(issueTreeHolds).values({ companyId, diff --git a/server/src/services/heartbeat/recovery.ts b/server/src/services/heartbeat/recovery.ts index f52151df613..a12a74bfbfb 100644 --- a/server/src/services/heartbeat/recovery.ts +++ b/server/src/services/heartbeat/recovery.ts @@ -108,7 +108,7 @@ import type { environmentService } from "../environments.js"; import type { environmentRuntimeService, ProviderResourceDisposition } from "../environment-runtime.js"; import type { agentInstructionWorkingCopyService } from "../agent-instruction-working-copies.js"; import type { createHeartbeatRunState } from "./run-state.js"; -import type { HeartbeatRetryDependencies, createHeartbeatRetries } from "./retries.js"; +import { failedRunRetryBudgetSpent, type HeartbeatRetryDependencies, type createHeartbeatRetries } from "./retries.js"; import type { CancelRunOptions } from "./run-control.js"; import { requestRestartInterruptedTurnRetry } from "./restart-interrupted-turn.js"; @@ -2672,7 +2672,12 @@ export function createHeartbeatRecovery(db: Db, dependencies: HeartbeatRecoveryD !revokedState.processGroupId; const shouldRetry = !await pendingCaseHandoffForRun(db, run.id, run.companyId) && - (run.processLossRetryCount ?? 0) < 1 && + // Process loss spends the durable failure retry budget that + // enqueueProcessLossRetry schedules against (execution-semantics + // "Shutdown, process loss, and provider failure"). Checking it here + // keeps "retrying once" honest and lets an exhausted chain fall + // through to the interaction continuation path below. + !failedRunRetryBudgetSpent(run) && ((tracksLegacyLocalChild && (!!run.processPid || !!run.processGroupId)) || restartInterruptedBeforeDispatch ||