mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-15 18:56:05 +01:00
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - A run has an execution recovery budget: three provider attempts for native runs, and two failure retries for legacy runs > - In the restart chaos campaign, control-plane restarts used up that budget for runs that were healthy (RC-22, 22b, 22c, 40 and 60) > - #19720 and #19747 fixed two variants with watermark heuristics, and legacy interrupted preparation still spent the failure budget > - The durable-ownership plan (section F, invariant 5) asks for one rule: charge an attempt only when a recorded outcome proves that provider work started and no progress followed > - This pull request adds that rule and moves the native restart charge and the legacy preparation retry onto it > - The benefit is that a restart never spends budget for work the provider never started, while real failure loops and crash loops stay bounded ## Linked Issues or Issue Description Refs #19720, #19747, #19856 **What happened?** A control-plane restart charges a provider attempt by heuristics. The first crash restart always charges. A later restart charges unless the previous resume had no start evidence (#19747) or completed work (#19720). A runner that spawned but whose provider never accepted a turn counted as started, so a K6 crash loop could still spend the budget. Legacy runs whose preparation a restart interrupted (`providerWorkStarted: false`) spent `failureRetries`, the same counter as real failures (`execution-recovery-attempt.ts`). **Expected behavior** An attempt is charged only when the provider accepted a turn (`turn.accepted` or `turn.started`) and no accepted turn then completed work. A startup before spawn, a runner that spawned but was never admitted, and a `turn.start` the provider did not accept are not charged. The generation-12 cap still bounds uncharged restarts. **Steps to reproduce** Start a native run at its last attempt. Kill the controller after the runner spawns but before the provider accepts a turn. Before this change, the next restart claim blocks the run with `execution_recovery_budget_exhausted`. ## What Changed - New `server/src/services/native-runtime/provider-attempt-outcome.ts`: the one rule. `readProviderAttemptOutcome()` returns `provider_not_started`, `provider_started_without_progress` or `provider_progressed` from run events. `providerAttemptChargesBudget()` charges only the second. `providerAttemptWindowStart()` starts the attempt window at the latest `native.process_start_requested`, so earlier work never counts for a later attempt. `PROVIDER_ATTEMPT_CREDIT_GENERATION_LIMIT` (12) is the shared cap. - `restartResumeKeepsAttempt()` (`native-restart-recovery.ts`) now applies that rule to the attempt the restart interrupted, including the first restart. It keeps the unstarted-resume carry from #19747, the reattachment rule (an adopted provider counts as started), the drain exemption from #19625, and the generation-12 cap. A history entry from an older release without a recorded position still charges. - Legacy runs: `interrupted_preparation` is now an uncharged retry lane (`execution-recovery-attempt.ts`). It keeps its own counter, bounded at the same cap (12), and never changes `failureRetries`. `retries.ts` uses the longest transient delay after the second lane attempt. - `doc/execution-semantics.md` states the rule. - Accounting has its own "provider work started" readers (inventory Summary finding 5). Phase 3 owns those; this PR exposes the rule for them to adopt and does not change them. ## Verification - `native-runner-restart-recovery.integration.test.ts` (embedded Postgres, real `claimNativeRestartRecoveries`): 129 passed. New cases, red on master: - not charged at the last attempt for a claimed remote bootstrap, a remote bootstrap retry, and a claimed unlaunched remote run; - not charged when the runner spawned but was never admitted, and when the provider did not accept `turn.start`; - not charged on the first restart when the original attempt already made progress; - a normal retry that spawned but was never admitted is not charged, and earlier work still does not count for it. - Counter-cases, green on master and with the change: an accepted `turn.accepted` or `turn.started` with no progress is charged; the generation cap blocks at the last attempt; crash loops with and without progress stay bounded; the #19720 and #19747 cases still pass. - `execution-recovery-attempt.test.ts`: new lane tests (red on master). - `workspace-recovery-preparation-dispatch.test.ts` (25 passed), `heartbeat-process-recovery.test.ts` (interrupted-run cases), `run-dispatch/adapters/postgres.test.ts` (interrupted cases). - `server` typecheck (`scripts/typecheck-layers.mjs`, low-memory mode): no errors. - Not run: chaos waves (the coordinator schedules them on this host), and the full server suite (CI covers it). ## Risks - Behavior change: the first crash restart no longer charges when no accepted turn is recorded. If a crash lands before the controller projects the provider's `turn.accepted`, that attempt is not charged. This fails toward more retries, and the generation-12 cap bounds it. - A legacy preparation crash loop now gets up to 12 retries instead of 2. Each retry still passes the normal issue gates and keeps the transient delay. - The new queries use the `(run_id, seq)` index, once per restart claim. Nothing new runs before `turn.start`. > For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and discuss it in `#dev` before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See `CONTRIBUTING.md`. ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`), Anthropic, via Claude Code with tool use (shell, file edits, test execution). Extended reasoning enabled. ## 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>
122 lines
5.7 KiB
TypeScript
122 lines
5.7 KiB
TypeScript
import { and, desc, eq, gt, inArray, sql } from "drizzle-orm";
|
|
import type { Db } from "@paperclipai/db";
|
|
import { heartbeatRunEvents } from "@paperclipai/db";
|
|
import { PROCESS_START_REQUESTED } from "../native-local-process-stop.js";
|
|
|
|
/**
|
|
* Work-based attempt budgets (durable-ownership plan, section F).
|
|
*
|
|
* An attempt spends execution budget only when a recorded outcome proves that
|
|
* the provider accepted a turn, and that attempt then made no progress before
|
|
* it ended. An intent alone is never that proof:
|
|
*
|
|
* - `provider_not_started`: no accepted turn is recorded. This covers a
|
|
* startup still marked `beforeSpawn`, a runner that spawned but was never
|
|
* admitted, and a `turn.start` the provider did not accept. Not charged.
|
|
* - `provider_started_without_progress`: the provider accepted a turn
|
|
* (`turn.accepted`, or the provider's own `turn.started`), and no accepted
|
|
* turn completed work afterwards. Charged.
|
|
* - `provider_progressed`: an accepted turn completed work: a tool execution or
|
|
* a completed item other than a usage report (#19720). Not charged.
|
|
*
|
|
* Restore activity is not progress: lineage items carry no turn, and work on a
|
|
* turn accepted before the window (the interrupted turn) does not count unless
|
|
* the attempt adopted that live provider work.
|
|
*/
|
|
export type ProviderAttemptOutcome =
|
|
| "provider_not_started"
|
|
| "provider_started_without_progress"
|
|
| "provider_progressed";
|
|
|
|
export const PROVIDER_TURN_ACCEPTED_EVENT_TYPES = ["turn.accepted", "turn.started"] as const;
|
|
export const PROVIDER_PROGRESS_EVENT_TYPES = ["tool.execution.completed", "item.completed"] as const;
|
|
|
|
/**
|
|
* Uncharged restarts are still bounded: from this controller generation on
|
|
* (native runs), or after this many uncharged preparation retries (legacy
|
|
* runs), every restart charges.
|
|
*/
|
|
export const PROVIDER_ATTEMPT_CREDIT_GENERATION_LIMIT = 12;
|
|
|
|
/** The one charge rule. Every attempt-budget reader should use it. */
|
|
export function providerAttemptChargesBudget(outcome: ProviderAttemptOutcome): boolean {
|
|
return outcome === "provider_started_without_progress";
|
|
}
|
|
|
|
/**
|
|
* Where the outcome window of `providerAttempt` starts, after `sinceSeq`. The
|
|
* executor records `native.process_start_requested` with its provider attempt
|
|
* before any backend may spawn a runner, so a later normal retry starts a new
|
|
* window and earlier work never counts for it.
|
|
* - `{ seq }`: the latest start of that attempt (a reattachment writes one too).
|
|
* - `"unstarted"`: the latest start names an earlier attempt. The executor
|
|
* saved the new attempt but died before its start, so nothing in the event
|
|
* log belongs to it.
|
|
* - Starts written before they carried an attempt fall back to the latest start.
|
|
*/
|
|
export async function providerAttemptWindowStart(
|
|
tx: Pick<Db, "select">,
|
|
input: { runId: string; sinceSeq: number; providerAttempt: number },
|
|
): Promise<{ seq: number } | "unstarted"> {
|
|
const [start] = await tx
|
|
.select({
|
|
seq: heartbeatRunEvents.seq,
|
|
attempt: sql<unknown>`${heartbeatRunEvents.payload} -> 'providerAttempt'`,
|
|
})
|
|
.from(heartbeatRunEvents)
|
|
.where(and(
|
|
eq(heartbeatRunEvents.runId, input.runId),
|
|
gt(heartbeatRunEvents.seq, input.sinceSeq),
|
|
eq(heartbeatRunEvents.eventType, PROCESS_START_REQUESTED),
|
|
))
|
|
.orderBy(desc(heartbeatRunEvents.seq))
|
|
.limit(1);
|
|
if (!start) return { seq: input.sinceSeq };
|
|
const attempt = typeof start.attempt === "number" ? start.attempt : Number(start.attempt);
|
|
if (start.attempt !== null && start.attempt !== undefined && Number.isSafeInteger(attempt)
|
|
&& attempt < input.providerAttempt) return "unstarted";
|
|
return { seq: Number(start.seq) };
|
|
}
|
|
|
|
/**
|
|
* Reads the outcome of the attempt recorded after `sinceSeq`.
|
|
* `adoptedProviderWork` is set when the attempt adopted a live runner whose
|
|
* provider had already started (a reattachment): it counts as started, and
|
|
* work on a turn accepted before the window counts as its progress.
|
|
*/
|
|
export async function readProviderAttemptOutcome(
|
|
tx: Pick<Db, "select">,
|
|
input: { runId: string; sinceSeq: number; adoptedProviderWork?: boolean },
|
|
): Promise<ProviderAttemptOutcome> {
|
|
const workTurnId = sql`${heartbeatRunEvents.payload} -> 'prpEvent' ->> 'turnId'`;
|
|
const acceptedFrom = input.adoptedProviderWork ? 0 : input.sinceSeq;
|
|
const [work] = await tx
|
|
.select({ seq: heartbeatRunEvents.seq })
|
|
.from(heartbeatRunEvents)
|
|
.where(and(
|
|
eq(heartbeatRunEvents.runId, input.runId),
|
|
gt(heartbeatRunEvents.seq, input.sinceSeq),
|
|
inArray(heartbeatRunEvents.eventType, [...PROVIDER_PROGRESS_EVENT_TYPES]),
|
|
sql`coalesce(${heartbeatRunEvents.payload} -> 'prpEvent' -> 'payload' ->> 'kind', '') <> 'usage'`,
|
|
// The inner alias hides the table name, so unqualified outer references
|
|
// below resolve to the outer completed-work row.
|
|
sql`exists (select 1 from ${heartbeatRunEvents} as accepted_turn where accepted_turn.run_id = ${input.runId}
|
|
and accepted_turn.seq > ${acceptedFrom} and accepted_turn.seq < ${heartbeatRunEvents.seq}
|
|
and accepted_turn.event_type in (${sql.join(PROVIDER_TURN_ACCEPTED_EVENT_TYPES.map(type => sql`${type}`), sql`, `)})
|
|
and accepted_turn.payload -> 'prpEvent' ->> 'turnId' = ${workTurnId})`,
|
|
))
|
|
.limit(1);
|
|
if (work) return "provider_progressed";
|
|
if (input.adoptedProviderWork) return "provider_started_without_progress";
|
|
const [accepted] = await tx
|
|
.select({ seq: heartbeatRunEvents.seq })
|
|
.from(heartbeatRunEvents)
|
|
.where(and(
|
|
eq(heartbeatRunEvents.runId, input.runId),
|
|
gt(heartbeatRunEvents.seq, input.sinceSeq),
|
|
inArray(heartbeatRunEvents.eventType, [...PROVIDER_TURN_ACCEPTED_EVENT_TYPES]),
|
|
))
|
|
.limit(1);
|
|
return accepted ? "provider_started_without_progress" : "provider_not_started";
|
|
}
|