mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-13 15:55:46 +01:00
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - A deploy replaces the server. The old server stops its running legacy runs at SIGTERM (`server_shutdown_interrupted`). The new server finds runs whose owner died without that drain (`process_lost`, "server may have restarted"). > - Each of these runs got its follow-up from the bounded transient retry. That retry spends the failure budget (2 attempts) and waits 30 to 60 seconds. > - So the first deploy spent one failure retry. A run that was already the last automatic retry got no follow-up at all. The task then rested on a manual Retry. > - A deploy is routine. An interruption by the server is not a failed attempt, and it must never leave a task waiting for a person. > - This pull request moves these runs to their own `server_restart` retry lane. The lane never spends the failure budget, starts the first continuation at once, and has its own bound. > - The benefit is that a task continues by itself through several deploys a day, and a real loop still stops and shows why. ## Linked Issues or Issue Description **What happened?** A zero-downtime deploy replaced a hosted server. In the 11 minutes around the swap, 11 legacy runs ended with `server_shutdown_interrupted` and 7 ended with `process_lost`. At least 7 tasks were left with no follow-up run and no scheduled retry, so each needed a manual Retry. Two of the stranded runs were already automatic retries. **Expected behavior** Each interrupted run continues once, automatically, on the surviving server, with its original wake. **Paperclip version or commit** Core commit7d9973405c. The behavior is the same on masterbf18b348c5. **Steps to reproduce** 1. Start a legacy run with a conversation adapter (for example `codex_local`) as the second automatic retry of its chain (failure budget spent). 2. Send SIGTERM to the server (`drainRunningRunsForShutdown`), or let a new server boot find the run without its owner (`reapOrphanedRuns`). 3. See that no successor run is created. The scheduler logs "Bounded retry exhausted". **Root cause** - `enqueueProcessLossRetry` (`server/src/services/heartbeat/recovery.ts`) calls `scheduleBoundedRetryForRun`. That function put these runs in the `transient_failure` lane, with `maxAttempts = 2` counted from `executionRetryAccounting.failureRetries` (`server/src/services/heartbeat/retries.ts`). A first attempt got a charged retry 30 to 60 seconds later. A run with 2 spent failure retries got `retry_exhausted`. - The orphan reaper (`recoverActiveExecutions`) skipped the retry when `failedRunRetryBudgetSpent(run)` was true. It also skipped a conversation run that had no local child process recorded (for example a run that was dispatching), because `shouldRetry` needed a local child, a preparing stage, or a monitor wake. - The stranded-task sweeper then saw the spent budget and moved the task to `blocked`. ## What Changed - `execution-recovery-attempt.ts`: add the `server_restart` retry lane. It is an uncharged lane. Its counter (`serverRestarts`, `serverRestartWindowStartedAt`) is saved in `executionRetryAccounting`, so it is visible on each run. The bound is 6 continuations in one 60-minute window. A later interruption after the window starts a new window. - New `server-replacement-interruption.ts`: `isServerReplacementInterruption(run)` names a legacy `server_shutdown_interrupted` run, or a `process_lost` run whose owner was another server boot. It reads the new `processLossDiagnostic.controllerReplaced` verdict. For rows saved by an older release, it reads the owner boot id or `runPredatesObserver`. - `retries.ts`: `scheduleBoundedRetryForRun` uses the `server_restart` lane for these runs (`usesServerRestartLane`). The first continuation is due at once; later ones in the window back off like the interrupted-preparation lane. `failedRunRetryBudgetSpent` reports the restart bound for these runs, so the sweeper escalates only when that bound trips. An interrupted preparation keeps its own lane while its chain has failure budget left. - `recovery.ts` (orphan reaper): record `controllerReplaced`. A conversation run that a restart ended is eligible for continuation even without a local child process, and it checks the restart bound, not the failure budget. - `wake-queue/adapters/postgres.ts`: carry the restart counter to successors that the wake queue creates. - No schema change. No change to how runs start (no new waits before a turn). Behavior that did not change: - At most one successor per run: every path still uses the predecessor row lock and the `retry_of_run_id` check in `scheduleBoundedRetryForRun`. Two servers alive at once reuse the same successor. - A non-conversation adapter that reached dispatch stays held for reconciliation, because a replay can repeat its actions. - Native runtime runs keep their own recovery. - A server that began shutdown already starts no queued run (`closeProcessRunAdmission` in `server/src/index.ts`, checked in `heartbeat/queue.ts`). Its queued and newly scheduled runs stay in the database for the new server. ## Verification - Red on `origin/master`, green on this branch (`server/src/__tests__/heartbeat-process-recovery.test.ts`, block "runs ended by a server restart or replacement"): - Graceful shutdown, first attempt: master scheduled a charged `transient_failure` retry. Now one immediate `server_restart` continuation. - Graceful shutdown, already the last retry: master scheduled nothing. Now one continuation; the failure count stays 2. - Owner lost at boot (`process_lost`), first attempt and already-retried: same results, with two servers running the boot sweep at the same time. Only one successor. - Old server exited after it committed the interruption and before it scheduled the continuation: the new server's sweep continues the run. - Run saved by an older release (no `controllerReplaced`, spent failure budget): the sweep continues it instead of moving the task to `blocked`. - The restart bound trips after 6 in one window, records `retryReason: server_restart` on the run, and the sweeper escalates. The same count in a closed window continues. - A run that failed by itself (`adapter_failed`) with a spent failure budget still gets no retry. - Each continuation's execution continuation reports the original wake (`trigger.retriedReason = issue_monitor_due`). - Unit tests: `server/src/services/server-replacement-interruption.test.ts`. - Six earlier tests asserted escalation after "three deploy restarts" spent the failure budget. They now spend the restart bound instead, so they still prove escalation when the bound trips. - Existing test `server/src/services/heartbeat/queue.test.ts` covers "starts no run once shutdown closed run admission". - `pnpm verify`: see the PR checks. ## Risks - A run that a restart ends now continues more often (up to 6 times per hour per chain, not 2 in total). A run that itself crashes the server is still stopped by the bound, and the stop is shown on the task. - Tasks that an older release already moved to `blocked` stay blocked. Tasks that are still `in_progress` or `todo` with a stranded run continue on the next sweep. - A `process_lost` row from an older release whose owner was the same boot that later restarted is read as a restart. That case is a lost in-memory handle, which is also not a failure of the agent's work. ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`), Anthropic, with tool use (code search, shell, test 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 <noreply@anthropic.com>
76 lines
3.4 KiB
TypeScript
76 lines
3.4 KiB
TypeScript
import type { heartbeatRuns } from "@paperclipai/db";
|
|
|
|
type Run = Pick<typeof heartbeatRuns.$inferSelect,
|
|
"startedAt" | "lastOutputAt" | "processPid" | "processGroupId">;
|
|
const MAX_AGE_MS = 7 * 24 * 60 * 60 * 1_000;
|
|
const LOCAL_CHECKS = ["not_observed_alive", "not_checked", "no_identifiers"] as const;
|
|
|
|
export interface ProcessLossDiagnostic {
|
|
pidRecorded: boolean;
|
|
groupRecorded: boolean;
|
|
localCheck: typeof LOCAL_CHECKS[number];
|
|
retryEligible: boolean;
|
|
runPredatesObserver?: boolean;
|
|
/** The run's recorded owner was another server boot: a restart or replacement ended it. */
|
|
controllerReplaced?: boolean;
|
|
lastOutputAgeMs?: number;
|
|
observerUptimeMs?: number;
|
|
}
|
|
|
|
function age(now: number, then: number | undefined): number | undefined {
|
|
if (then === undefined) return undefined;
|
|
const elapsed = now - then;
|
|
return Number.isSafeInteger(elapsed) && elapsed >= 0 && elapsed <= MAX_AGE_MS
|
|
? elapsed : undefined;
|
|
}
|
|
|
|
/** Snapshot before status writes and cleanup. Never infer why ownership was lost. */
|
|
export function buildProcessLossDiagnostic(input: {
|
|
run: Run;
|
|
nowMs: number;
|
|
observerStartedAtMs: number;
|
|
checksPersistedChildLiveness: boolean;
|
|
retryEligible: boolean;
|
|
controllerReplaced?: boolean;
|
|
}): ProcessLossDiagnostic {
|
|
const { run, nowMs, observerStartedAtMs } = input;
|
|
const pidRecorded = !!run.processPid;
|
|
const groupRecorded = !!run.processGroupId;
|
|
const startedAt = run.startedAt?.getTime();
|
|
const validObserverStart = Number.isFinite(observerStartedAtMs) && observerStartedAtMs <= nowMs;
|
|
const lastOutputAgeMs = age(nowMs, run.lastOutputAt?.getTime());
|
|
const observerUptimeMs = validObserverStart ? age(nowMs, Math.floor(observerStartedAtMs)) : undefined;
|
|
return {
|
|
pidRecorded,
|
|
groupRecorded,
|
|
localCheck: !pidRecorded && !groupRecorded ? "no_identifiers"
|
|
: input.checksPersistedChildLiveness ? "not_observed_alive" : "not_checked",
|
|
retryEligible: input.retryEligible,
|
|
...(validObserverStart && startedAt !== undefined && Number.isFinite(startedAt) && startedAt <= nowMs
|
|
? { runPredatesObserver: startedAt < observerStartedAtMs } : {}),
|
|
...(input.controllerReplaced !== undefined ? { controllerReplaced: input.controllerReplaced } : {}),
|
|
...(lastOutputAgeMs !== undefined ? { lastOutputAgeMs } : {}),
|
|
...(observerUptimeMs !== undefined ? { observerUptimeMs } : {}),
|
|
};
|
|
}
|
|
|
|
/** Persisted adapter data is untrusted. Only closed labels, booleans and ages leave the instance. */
|
|
export function readProcessLossDiagnostic(value: unknown): Partial<ProcessLossDiagnostic> {
|
|
const result: Partial<ProcessLossDiagnostic> = {};
|
|
if (!value || typeof value !== "object" || Array.isArray(value)) return result;
|
|
const read = (key: string): unknown => {
|
|
try { return (value as Record<string, unknown>)[key]; } catch { return undefined; }
|
|
};
|
|
for (const key of ["pidRecorded", "groupRecorded", "retryEligible", "runPredatesObserver", "controllerReplaced"] as const) {
|
|
const entry = read(key);
|
|
if (typeof entry === "boolean") result[key] = entry;
|
|
}
|
|
const localCheck = read("localCheck");
|
|
if (LOCAL_CHECKS.some(check => check === localCheck)) result.localCheck = localCheck as ProcessLossDiagnostic["localCheck"];
|
|
for (const key of ["lastOutputAgeMs", "observerUptimeMs"] as const) {
|
|
const entry = read(key);
|
|
if (typeof entry === "number" && Number.isSafeInteger(entry) && entry >= 0 && entry <= MAX_AGE_MS) result[key] = entry;
|
|
}
|
|
return result;
|
|
}
|