mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-11 05:11:49 +01:00
fix(heartbeat): retry a legacy run whose lease this process lost before dispatch (#20694)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The heartbeat orphan reaper settles legacy (non-native) runs that lost their controller lease > - #16355 taught the reaper to retry a legacy run that a restart stopped while setup was still `preparing`, because revocation fences the old controller and `preparing` proves no adapter dispatch > - That proof was applied only when the lapsed lease belonged to a different boot > - A stall can make the current process lose its own lease while setup is still `preparing`, and its executor can end without settling the row > - The reaper then wrote "retrying once" but recorded no bootstrap evidence, so the retry was refused and the task was blocked for reconciliation > - This pull request applies the same revocation proof to a lease from the current boot > - The benefit is that these tasks recover by themselves instead of waiting for a person to press Retry ## Linked Issues or Issue Description Refs #16355 **What happened?** On a hosted instance, one server process stalled for about 100 seconds (lease renewals were 42–46 s late). Several legacy `paperclip_runner` runs on a remote sandbox were still in setup (`executionStage: "preparing"`, no process id). Their controller leases expired. Some executors settled their own runs as `controller_lease_lost`. Two runs had no executor left, so the orphan reaper of the same process settled them 4 minutes later. Each run got `error: "Process lost -- server may have restarted; retrying once"` and `processLossDiagnostic.retryEligible: true`, but no `executionRecovery` evidence. `enqueueProcessLossRetry` then saw a run that needs reconciliation and scheduled nothing. The next sweep recorded `legacy_execution_requires_reconciliation` with `preserve_without_replay_v1`, and both tasks stayed `blocked` until a person pressed Retry. **Expected behavior** Revocation commits the stage. Renewal requires the old boot id, so no controller can move a revoked run to `dispatching`. A revoked run that is still `preparing` with no process identity did no provider work, whichever boot owned the lease. The reaper should record the bootstrap evidence and use the bounded retry. **Steps to reproduce** 1. Claim a legacy run in this process: `controllerBootId` is the current boot, `executionStage: "preparing"`, no process id. 2. Let its controller lease expire with no local executor (for example after an event-loop or database stall). 3. Run the orphan reaper. 4. Before this change: `process_lost; retrying once`, no `executionRecovery`, no retry, and a reconciliation hold. After this change: bootstrap evidence and a `scheduled_retry` successor. ## What Changed - `server/src/services/heartbeat/recovery.ts`: `restartInterruptedBeforeDispatch` no longer requires the lease to come from another boot. The other clauses stay: legacy runtime, a stamped controller boot id, and `preparing` with no process id or group in the state committed by the revocation. Locally tracked runs and runs with a live lease are still skipped before this point. - `server/src/__tests__/heartbeat-process-recovery.test.ts`: two cases in the hosted shape (assignment wake and due-monitor wake, `paperclip_runner`, prepared remote workspace, same-boot expired lease, `preparing`). They expect bootstrap evidence, a scheduled retry, no recovery hold after a second sweep, and no adapter call. A control case keeps a same-boot `dispatching` run held for reconciliation. ## Verification - Red on `origin/master` (source change reverted): both new cases fail. The monitor case shows the exact hosted shape: `retryEligible: true` with no `executionRecovery`. - Green with the change: `pnpm --filter @paperclipai/server exec vitest run src/__tests__/heartbeat-process-recovery.test.ts -t "lease this process lost|this process dispatched held|restart stopped before adapter dispatch|reached adapter dispatch held|lost monitor|older build without a controller stamp|failure budget"` (10 passed). - Related files, run together: `heartbeat-process-recovery`, `live-recovery-work`, `interrupted-preparation-hold-settle`, `resource-wait-stop`, `legacy-controller-lease`, `execution-recovery-dead-controller`, `legacy-process-settlement`, `run-failure-report` (876 passed; 2 timing failures under load in unrelated setup-failure tests, both pass when run alone). - `pnpm --filter @paperclipai/server exec tsc --noEmit` passes. ## Risks - Low risk. The proof is the one #16355 already relies on across boots: the revocation compare-and-set replaces the boot id, so the in-memory controller of this process also fails its next renewal and cannot dispatch. - A same-boot run reaches this code only when it is not in `activeRunExecutions` or `runningProcesses` and its lease expired. If an executor is stalled before it registers, it aborts at its first renewal. Setup work can run again in the retry; provider work cannot. - Upgrade safety: no persisted shape changes. The reaper reads the same fields (`executionStage`, `controllerBootId`, process ids) that older releases write. Runs that older code already parked keep their hold and their working Retry. - Runs that reached `dispatching`, recorded a process, or have no controller stamp keep the current behavior. ## Model Used - Anthropic Claude Opus 5.5 (`claude-opus-5-5`), used through Claude Code with tool use, shell, and code 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 (the code comment carries the contract) - [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 <noreply@anthropic.com>
This commit is contained in:
1 parent
a1ec05a1a4
commit
8c0dbffa70
2 files changed
+66
-3
No files matched your search
@@ -3557,6 +3557,70 @@ describeEmbeddedPostgres("heartbeat orphaned process recovery", () => {
|
||||
expect(mockAdapterExecute).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it.each([
|
||||
["an assignment", {}],
|
||||
["a due monitor", { wakeReason: "issue_monitor_due" }],
|
||||
] as const)("retries %s run whose lease this process lost before adapter dispatch", async (_label, contextSnapshot) => {
|
||||
const { companyId, runId, issueId } = await seedRunFixture({
|
||||
adapterType: "paperclip_runner", agentStatus: "idle", runtimeMode: "legacy", contextSnapshot,
|
||||
});
|
||||
await db.update(heartbeatRuns).set({
|
||||
controllerBootId: controllerLeases.legacyControllerBootId,
|
||||
controllerProcess: currentControllerProcessStamp(),
|
||||
controllerLeaseExpiresAt: sql`clock_timestamp() - interval '1 second'`,
|
||||
executionStage: "preparing",
|
||||
runnerProfileJson: {
|
||||
adapterDispatch: { adapterType: "paperclip_runner" },
|
||||
remoteWorkspace: { schema: "paperclip.remote-workspace-run.v1", epoch: 6, state: "prepared",
|
||||
leaseId: randomUUID(), providerLeaseId: randomUUID(), workspaceId: randomUUID(),
|
||||
remoteCwd: "/home/daytona/paperclip-workspace", snapshotId: randomUUID() },
|
||||
},
|
||||
}).where(eq(heartbeatRuns.id, runId));
|
||||
|
||||
const heartbeat = heartbeatService(db);
|
||||
expect(await heartbeat.reapOrphanedRuns()).toEqual({ reaped: 1, runIds: [runId] });
|
||||
|
||||
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; retrying once" });
|
||||
expect(reaped?.resultJson).toMatchObject({
|
||||
executionRecovery: { kind: "bootstrap", providerWorkStarted: false },
|
||||
processLossDiagnostic: { localCheck: "no_identifiers", retryEligible: true },
|
||||
});
|
||||
expect(await db.select().from(heartbeatRuns).where(and(
|
||||
eq(heartbeatRuns.companyId, companyId), eq(heartbeatRuns.retryOfRunId, runId),
|
||||
))).toEqual([expect.objectContaining({ status: "scheduled_retry" })]);
|
||||
await heartbeat.reapOrphanedRuns();
|
||||
expect(await db.select().from(issueRecoveryActions)
|
||||
.where(eq(issueRecoveryActions.sourceIssueId, issueId))).toEqual([]);
|
||||
expect(mockAdapterExecute).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("keeps a run this process dispatched held for reconciliation after its lease lapses", async () => {
|
||||
const { companyId, runId, issueId } = await seedRunFixture({
|
||||
adapterType: "paperclip_runner", agentStatus: "idle", runtimeMode: "legacy",
|
||||
});
|
||||
await db.update(heartbeatRuns).set({
|
||||
controllerBootId: controllerLeases.legacyControllerBootId,
|
||||
controllerProcess: currentControllerProcessStamp(),
|
||||
controllerLeaseExpiresAt: sql`clock_timestamp() - interval '1 second'`,
|
||||
executionStage: "dispatching",
|
||||
}).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" });
|
||||
expect(reaped?.resultJson).not.toHaveProperty("executionRecovery");
|
||||
expect(await db.select().from(heartbeatRuns).where(and(
|
||||
eq(heartbeatRuns.companyId, companyId), eq(heartbeatRuns.retryOfRunId, runId),
|
||||
))).toEqual([]);
|
||||
expect(await db.select().from(issueRecoveryActions)
|
||||
.where(eq(issueRecoveryActions.sourceIssueId, issueId))).toEqual([
|
||||
expect.objectContaining({ cause: "legacy_execution_requires_reconciliation" }),
|
||||
]);
|
||||
});
|
||||
|
||||
it("keeps a lost preparation from an older build without a controller stamp in the charged lane", async () => {
|
||||
const { companyId, runId } = await seedRestartOrphanedLegacySetup("preparing");
|
||||
await db.update(heartbeatRuns).set({ runnerProfileJson: { adapterDispatch: { adapterType: "paperclip_runner" } } })
|
||||
|
||||
@@ -2660,8 +2660,8 @@ export function createHeartbeatRecovery(db: Db, dependencies: HeartbeatRecoveryD
|
||||
continue;
|
||||
}
|
||||
// A legacy controller persists "dispatching" in the same lease renewal
|
||||
// that admits the adapter. An expired lease from another boot that is
|
||||
// still "preparing" proves a restart stopped setup before provider work.
|
||||
// that admits the adapter. An expired lease that is still "preparing"
|
||||
// proves setup stopped before provider work.
|
||||
// Use the state committed with the revocation, not the earlier snapshot:
|
||||
// the old controller may have dispatched before its lease expired.
|
||||
// The revocation matched the snapshot's boot ID before replacing it.
|
||||
@@ -2669,7 +2669,6 @@ export function createHeartbeatRecovery(db: Db, dependencies: HeartbeatRecoveryD
|
||||
run.runtimeMode === "legacy" &&
|
||||
revokedState.executionStage === "preparing" &&
|
||||
!!run.controllerBootId &&
|
||||
run.controllerBootId !== legacyControllerBootId &&
|
||||
!revokedState.processPid &&
|
||||
!revokedState.processGroupId;
|
||||
const shouldRetry =
|
||||
|
||||
Reference in new issue
Block a user