mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-08 01:57:31 +01:00
fix(native): settle a bootstrap retry that lost its task lock (#19307)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - A native run that fails before any provider work gets a bounded same-run bootstrap retry. Restart recovery decides whether that retry can run. > - The retry needs the task's issue lock. A user comment or another wake can move that lock to a new run before the retry is due. > - Restart recovery then returned `issue_execution_lock_changed` without a write. The same check ran on every recovery pass and never ended. The coordinator stayed `retryable_failure`. > - The orphaned-lease sweep waits for a terminal coordinator before it preserves the workspace. So the sandbox lease stayed active, and every later run for the task was cancelled as `workspace_busy`. > - This pull request gives a lock-lost bootstrap retry the same bounded admission that a superseded remote retry already has. > - The benefit is that the retry settles, the board gets a reconciliation action, and the sandbox lease is released instead of blocking the task. ## Linked Issues or Issue Description No public issue exists. Bug description: **What happened?** On a hosted instance, a native agent run failed at startup because the provider rejected `session.open` with a rate limit. Paperclip scheduled a same-run bootstrap retry. Seconds later, a new run for the task took its issue lock, most likely woken by a user comment. From then on, every restart recovery pass for the old run returned `blocked` / `issue_execution_lock_changed` and wrote nothing. The run recorded about 1,000 recovery events over 12 hours. Its coordinator stayed `retryable_failure` with `recoveryMode: bootstrap_retry`. The scoped orphaned-lease sweep found a pending workspace export for the lease. It could not preserve the workspace, because `preserveNativeFailedWorkspace` requires `terminal_failure`. So the lease stayed `active`, and 16 later runs for the task were cancelled as `workspace_busy`. Three more runs on the same instance were stuck in the same state. A board Stop on the failed run moved the coordinator to `terminal_failure`, and the sweep then released the lease within about a minute. **Expected behavior** A bootstrap retry that cannot get its task lock back settles within a bounded number of recovery passes. The coordinator reaches `terminal_failure`, the board gets a reconciliation action, and the existing orphaned-lease sweep can preserve the workspace and release the sandbox lease. **Steps to reproduce** 1. Fail a native remote run before provider work, so its coordinator is `retryable_failure` with `recoveryMode: bootstrap_retry`. 2. Move the task's `executionRunId` to a newer run for the same task, or clear it. 3. Run `claimNativeRestartRecoveries` for the failed run. 4. Before this change, it returns `blocked` with no `retryAt`, and the coordinator stays `retryable_failure` on every pass. The new tests in `native-remote-retry-admission.test.ts` reproduce this. **Paperclip version or commit** Observed on a hosted deployment. Reproduced in tests on `master` at `10a7d039c8e`. Related: #19190 (a `session.open` model rejection no longer schedules a doomed bootstrap retry), #19131 (preserve superseded observed workspaces) and #19281 (release the leases of a cancelled legacy run). None of them changes this admission path. ## What Changed - **Restart recovery (`server/src/services/native-runtime/native-restart-recovery.ts`)**: a new condition, `lockLostBootstrapRetry`, matches a failed run whose coordinator is `retryable_failure` with `recoveryMode: bootstrap_retry` and whose issue lock is not on the run. - A newer run for the task: the existing superseded check now covers this case too. It defers with `native_retry_source_superseded` as permanent, so admission is exhausted at once. - No newer run (the lock was released): recovery defers with `issue_execution_lock_changed` through the existing `deferRemoteRetry`. That path persists a deadline, backs off and exhausts after the system recovery budget. - When admission is exhausted, the existing hold path writes `terminal_failure` / `native_restart_recovery_blocked`, keeps the provider evidence and creates the board reconciliation action. This PR adds no new terminal path. - A bootstrap retry that still holds its own lock is unchanged. The exact-checkpoint remote retry path is unchanged. - **Docs (`doc/execution-semantics.md`)**: the bootstrap retry section now states the lock-lost rule. ## Verification - New test `settles a %s bootstrap retry that lost its task lock instead of re-checking forever` in `server/src/services/native-runtime/native-remote-retry-admission.test.ts`, for the `superseded` and `released` cases. The fixture is a genuine pre-provider failure: a `session.open` rejection with no session checkpoint, no runner checkpoint row and no provider evidence. - On `master` without the fix, both cases fail: recovery returns `blocked` without a deadline, and the coordinator stays `retryable_failure`. - With the fix, `superseded` settles on the first pass, and `released` settles after `SYSTEM_RECOVERY_MAX_RETRIES` deferred passes. Each deferred pass persists its `nextAttemptAt`. Both end in `terminal_failure` with one board recovery action, and later passes open no transaction. - `npx vitest run src/services/native-runtime/native-remote-retry-admission.test.ts` (from `server/`): 92 passed. - Related suites, each run on its own with exit code 0: `native-ingress-bootstrap-retry`, `native-startup-retry`, `native-scheduled-resume-lock`, `native-restart-recovery-isolation`, `native-runner-restart-recovery`, `native-runner-restart-preparation`, `native-runner-restart-diskless`, `native-warm-transition-restart`, `native-controller-recovery`, `native-failed-workspace-recovery`, `native-session-resumption`, `native-startup-cleanup-recovery`, `heartbeat-observed-workspace-preservation`, `heartbeat/recovery`, `runtime-selection`, `remote-workspace-migration` and `native-session-executor` (1,149 tests). Two other suites each had one local failure that this change does not cause: - `heartbeat-process-recovery`: "redacts opaque environment-bound credentials from Sentry diagnostics" fails the same way on this machine without the change. - `chat-channels.integration`: "settles a shared-endpoint accepted-response backlog without retrying its own locks" failed once under full-suite load. It passes alone, with and without the change. - `pnpm run typecheck` in `server/`: passed. ## Risks - Low to medium. The change only affects failed bootstrap retries whose lock is not on the run. Before this change, such a retry could never run, so no working path is removed. - A released lock that comes back to the same run during the backoff still resumes normally, because the condition reads the current lock on each pass. - When the retry settles, the task gets a board reconciliation action. Before this change, the task was silently stuck behind `workspace_busy`. - No migration and no API change. ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`) in Claude Code, with tool use (shell, code search, test runs), extended thinking, and a read-only investigation subagent for the root cause. ## 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 --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
This commit is contained in:
1 parent
74b11014c6
commit
8464a8c9ae
3 files changed
+64
-1
No files matched your search
@@ -2022,6 +2022,16 @@ not grant retry authority or change the budget. When the budget is exhausted,
|
||||
the task is blocked and its recovery action states that provider startup failed
|
||||
before any provider work began.
|
||||
|
||||
A failed bootstrap retry cannot take back an issue lock that moved off its run.
|
||||
When a newer run exists for the task, restart recovery settles the old retry at
|
||||
once. When the lock was released and no newer run exists, recovery uses the
|
||||
bounded admission backoff and then settles the retry. In both cases the
|
||||
coordinator reaches `terminal_failure`, the board receives a reconciliation
|
||||
action, and the orphaned-lease sweep can preserve the workspace and release the
|
||||
sandbox lease. Before this rule, the retry re-checked the lock without a
|
||||
deadline and never settled, so its sandbox lease stayed active and every later
|
||||
run for the task was cancelled as `workspace_busy`.
|
||||
|
||||
The same limit evidence also holds new work for the agent. Before any workspace
|
||||
or provider work, a queued run for an agent with an active provider limit is
|
||||
cancelled as `provider_limit_wait`, records the expected reset and the run that
|
||||
|
||||
@@ -567,6 +567,50 @@ describeEmbeddedPostgres("remote native retry execution admission", () => {
|
||||
}
|
||||
});
|
||||
|
||||
it.each(["superseded", "released"] as const)("settles a %s bootstrap retry that lost its task lock instead of re-checking forever", async lock => {
|
||||
const f = await failedRemoteRun("remote");
|
||||
// A bootstrap retry follows a pre-provider failure: no checkpoint, provider session or provider events.
|
||||
const [run] = await f.db.select().from(heartbeatRuns).where(eq(heartbeatRuns.id, f.runId));
|
||||
const { sessionCheckpoint: _checkpoint, ...profile } = run.runnerProfileJson ?? {};
|
||||
await f.db.update(heartbeatRuns).set({ errorCode: "provider_session_open_rejected", runnerProfileJson: profile })
|
||||
.where(eq(heartbeatRuns.id, f.runId));
|
||||
await f.db.delete(nativeRunnerCheckpoints)
|
||||
.where(and(eq(nativeRunnerCheckpoints.companyId, f.companyId), eq(nativeRunnerCheckpoints.runId, f.runId)));
|
||||
await f.db.update(nativeRunFinalizations).set({ failureCode: "provider_session_open_rejected", failureDetail: {
|
||||
recoveryMode: "bootstrap_retry", providerSessionEstablished: false, checkpointExists: false, providerEventsExist: false,
|
||||
} }).where(eq(nativeRunFinalizations.runId, f.runId));
|
||||
if (lock === "superseded") {
|
||||
const [newer] = await f.db.insert(heartbeatRuns).values({ companyId: f.companyId, agentId: f.agentId, issueId: f.issueId,
|
||||
status: "running", createdAt: new Date(Date.now() + 1_000) }).returning({ id: heartbeatRuns.id });
|
||||
await f.db.update(issues).set({ executionRunId: newer.id }).where(eq(issues.id, f.issueId));
|
||||
}
|
||||
const onClaim = vi.fn();
|
||||
const attempt = (now: Date) => claimNativeRestartRecoveries({ db: f.db, controller: environment.current.successor,
|
||||
restartKind: "hard", runIds: [f.runId], now, onClaim });
|
||||
let now = new Date();
|
||||
let passes = 0;
|
||||
for (; passes <= SYSTEM_RECOVERY_MAX_RETRIES + 1; passes++) {
|
||||
const [disposition] = await attempt(now);
|
||||
expect(disposition).toMatchObject({ kind: "blocked" });
|
||||
const [coordinator] = await f.db.select().from(nativeRunFinalizations).where(eq(nativeRunFinalizations.runId, f.runId));
|
||||
if (coordinator.phase === "terminal_failure") break;
|
||||
if (!("retryAt" in disposition) || !disposition.retryAt) throw new Error("a deferred bootstrap retry must persist its deadline");
|
||||
expect(coordinator.nextAttemptAt).toEqual(disposition.retryAt);
|
||||
now = disposition.retryAt;
|
||||
}
|
||||
expect(passes).toBe(lock === "superseded" ? 0 : SYSTEM_RECOVERY_MAX_RETRIES);
|
||||
expect(onClaim).not.toHaveBeenCalled();
|
||||
expect((await f.db.select().from(nativeRunFinalizations).where(eq(nativeRunFinalizations.runId, f.runId)))[0])
|
||||
.toMatchObject({ phase: "terminal_failure", nextAttemptAt: null, attempt: 1, failureCode: "native_restart_recovery_blocked" });
|
||||
expect(await f.db.select().from(issueRecoveryActions).where(eq(issueRecoveryActions.sourceIssueId, f.issueId)))
|
||||
.toMatchObject([{ ownerType: "board", wakePolicy: null }]);
|
||||
const transaction = vi.spyOn(f.db, "transaction");
|
||||
try {
|
||||
for (const elapsed of [30_000, 3_600_000]) expect(await attempt(new Date(now.getTime() + elapsed))).toEqual([]);
|
||||
expect(transaction).not.toHaveBeenCalled();
|
||||
} finally { transaction.mockRestore(); }
|
||||
});
|
||||
|
||||
it.each(["paused", "Stop", "missing-authority"] as const)("bounds %s holds and preserves the provider and own task lock for manual reconciliation", async gate => {
|
||||
const f = await failedRemoteRun("remote");
|
||||
if (gate === "paused") await f.db.update(agents).set({ status: "paused" }).where(eq(agents.id, f.agentId));
|
||||
|
||||
@@ -658,6 +658,11 @@ export async function claimNativeRestartRecoveries(input: {
|
||||
)).orderBy(desc(environmentLeases.createdAt)).limit(1);
|
||||
const failedRemoteRetry = row.run.status === "failed" && row.coordinator.phase === "retryable_failure"
|
||||
&& row.coordinator.failureDetail?.recoveryMode === "exact_checkpoint_resume" && (remoteWorkspace !== null || pluginTask !== undefined);
|
||||
// A same-run bootstrap retry cannot take back an issue lock that moved
|
||||
// off it. Without a bounded admission it would re-check forever, never
|
||||
// settle, and keep its sandbox lease from the orphan-lease sweep.
|
||||
const lockLostBootstrapRetry = row.run.status === "failed" && row.coordinator.phase === "retryable_failure"
|
||||
&& row.coordinator.failureDetail?.recoveryMode === "bootstrap_retry" && row.issueExecutionRunId !== row.run.id;
|
||||
const releasedRemoteRetry = failedRemoteRetry && row.issueExecutionRunId === null;
|
||||
const exhaustedRemoteRetry = failedRemoteRetry && row.coordinator.attempt >= 3;
|
||||
const needsResumptionAdmission = releasedRemoteRetry || exhaustedRemoteRetry ||
|
||||
@@ -706,7 +711,7 @@ export async function claimNativeRestartRecoveries(input: {
|
||||
if (!updated.length) throw new Error("native_retry_deferral_claim_changed");
|
||||
return { kind: "blocked", runId: row.run.id, reason, retryAt, transitionUnchanged } as const;
|
||||
};
|
||||
if (failedRemoteRetry) {
|
||||
if (failedRemoteRetry || lockLostBootstrapRetry) {
|
||||
const source = alias(heartbeatRuns, "native_retry_source");
|
||||
const [newer] = await tx.select({ id: heartbeatRuns.id }).from(heartbeatRuns)
|
||||
.innerJoin(source, and(eq(source.companyId, row.run.companyId), eq(source.id, row.run.id)))
|
||||
@@ -730,6 +735,10 @@ export async function claimNativeRestartRecoveries(input: {
|
||||
const deferred = await deferRemoteRetry("native_retry_admission_ineligible", ["done", "cancelled"].includes(issue.status));
|
||||
if (deferred) return deferred;
|
||||
}
|
||||
if (lockLostBootstrapRetry && !admission.exhausted) {
|
||||
const deferred = await deferRemoteRetry("issue_execution_lock_changed");
|
||||
if (deferred) return deferred;
|
||||
}
|
||||
if (row.issueExecutionRunId !== row.run.id && !(stopCleanup && row.issueExecutionRunId === null) && !releasedRemoteRetry && !admission.exhausted) {
|
||||
return {
|
||||
kind: "blocked",
|
||||
|
||||
Reference in new issue
Block a user