mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-28 12:04:41 +01:00
fix(recovery): preserve live heartbeat preparation ownership (#17838)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Heartbeat prepares each run before it starts the native provider. > - Startup recovery can run while that preparation is still active. > - The native input and coordinator exist before workspace synchronization saves its descriptor. > - Recovery checked the later native executor registry but missed the earlier heartbeat execution registry. > - It could claim the live run and dispatch a second executor against incomplete workspace state. > - This change gives the recovery claim the existing live heartbeat registry and checks it before changing ownership. ## Linked Issues or Issue Description **What happened?** Startup recovery can claim a native run that the same server is still preparing. The second execution reads the admitted native input before workspace synchronization has saved its descriptor. It fails with `native_workspace_sync_descriptor_binding_missing`. The original preparation then finds a failed run and cannot start the provider. **Expected behavior** Leave live preparation with its current executor. Do not change its native coordinator or dispatch another executor. An inactive run must still pass the existing recovery checks. **Steps to reproduce** 1. Start heartbeat runtime selection for a remote sandbox. 2. Hold workspace synchronization after native input and coordinator persistence. 3. Run native startup recovery with the heartbeat execution still active. 4. Observe an unintended `bootstrap_incomplete` claim and a second execution. Refs #17439 for broader remote controller recovery. This change repairs the current heartbeat startup race. It does not add checkpoint storage or remote adoption. A separate diagnostics correction is in #17837. ## What Changed - Pass the existing heartbeat live execution registry to native recovery claims. - Check the current registry after locking the run and before any takeover or coordinator change. - Return the existing waiting disposition for live preparation. Keep workspace descriptor validation unchanged. - Add a PostgreSQL regression at the real runtime-selection boundary and an assembled heartbeat recovery regression with the real claim service. - Document the preparation interval and its recovery behavior. ## Verification - Red: the new runtime-selection test observed an unintended bootstrap claim on the original code. The heartbeat regression also failed because recovery dispatched the live run. - Green on current head `88bec51f10d61eb851eec849e82a80bb198555c7`: all 144 runtime-selection, heartbeat recovery, workspace-preparation and restart-policy tests pass. - These cases check an unchanged coordinator, no duplicate recovery dispatch, a running source run, and normal recovery after the execution leaves the registry. - The full native restart integration file passes all 22 available cases. Nine real-runner process cases are skipped because local Runner test binaries were removed to recover disk space. The passing cases include remote startup, old preparation fencing, ownership conflicts, Stop, and concurrent recovery. - Direct full server TypeScript check passes: `pnpm exec tsc --noEmit -p server/tsconfig.json`. Hosted CI passes on this head, including full test partitions, typecheck, build, native Runner checks, serialized suites, browser shards, canary and both aggregates. Apex is 5/5 with no actionable findings. The current head has 54 successful checks, two expected skips and successful Snyk status. - `git diff --check`, module boundary checks, and map checks pass. - A wider local safety run stopped with PostgreSQL error 53100 and ENOSPC after 64 tests passed. Recursive typecheck also stopped with ENOSPC. These attempts did not pass. Generated files in this worktree were removed to free space. The full local test suite and build have not been run for this change; hosted CI must supply those results. - Live evidence: a production startup recovery claimed a new run during outward workspace staging. The second preparation failed before provider startup. Its exact sandbox was later destroyed with successful cleanup. The patch is not deployed. A later audited retry on the existing deployment completed successfully and saved the requested answer once. An audited operator repair through the normal issue service completed the final user assignment in Review after the agent API handoff failed. This acceptance verifies recovered work, not deployment of the new guard. ## Risks - A live heartbeat remains responsible for its own completion and cleanup. Recovery must not compete with it. The registry is process-local; existing durable ownership checks still govern other controllers. - An executor that is still active remains protected until its cleanup removes its registry entry. This adds no timer, polling loop, schema or credential change. - Missing or corrupt workspace descriptors still fail closed. This change does not authorize replay of unknown provider actions. ## Model Used OpenAI Codex, GPT-6 family, with reasoning, source inspection, tool use and code execution. The runtime does not expose the exact serving model ID or context window size. ## 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 - [x] All Paperclip CI gates are green - [x] 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
3edba11576
commit
94819b6ded
6 files changed
+75
-1
No files matched your search
@@ -1352,6 +1352,13 @@ fresh native run identities. Unmarked preparations from older servers remain
|
||||
held during rolling upgrades, because those servers cannot safely yield to a
|
||||
recovery owner. Revisiting an existing preparation cannot add that marker.
|
||||
|
||||
Restart recovery checks the heartbeat process's live execution registry before
|
||||
claiming an observed run. Native input can exist before workspace synchronization
|
||||
has saved its descriptor or the executor has claimed its lease. A live preparation
|
||||
keeps ownership throughout that interval. Recovery leaves its coordinator unchanged
|
||||
and does not dispatch a second executor. An inactive run still requires the normal
|
||||
ownership, workspace integrity, and provider evidence checks.
|
||||
|
||||
A preparation that resumes after another execution acquired the coordinator
|
||||
lease yields through the controller-detachment path. It does not fail the shared
|
||||
run, release its environment, or start another provider. Lease expiry alone does
|
||||
|
||||
@@ -19,6 +19,9 @@ Agent access: [companion](../agent-access-map/recovery.md).
|
||||
progress, and next action without requiring a raw-log interpretation.
|
||||
- `bounded-continuation`: `recovery/bounded-continuation` — supported interrupted work continues with preserved
|
||||
conversation context and bounded attempts, without blindly replaying tools.
|
||||
Restart recovery respects live heartbeat preparation before workspace
|
||||
synchronization finishes. PostgreSQL regressions cover unchanged ownership,
|
||||
no duplicate dispatch, and later recovery after preparation leaves the registry.
|
||||
- `source-action`: `recovery/source-action` — source-scoped recovery actions remain attached to the task;
|
||||
independent repair work may have its own issue and return owner.
|
||||
- `exhaustion`: `recovery/exhaustion` — an exhausted infrastructure attempt offers ordinary Retry.
|
||||
|
||||
@@ -11,6 +11,7 @@ import { createHeartbeatRunState } from "./run-state.js";
|
||||
import type { HotRestartIntent } from "../hot-restart.js";
|
||||
import type { NativeRestartRecoveryClaim, NativeRestartRecoveryDisposition } from "../native-runtime/index.js";
|
||||
import { NATIVE_OWNERSHIP_UNVERIFIED_ERROR_CODE } from "../native-runtime/native-runner-ownership.js";
|
||||
import { claimNativeRestartRecoveries } from "../native-runtime/native-restart-recovery.js";
|
||||
import { allowLegacyShutdownWorkspaceCleanup, beginLegacyShutdownWorkspaceSettlement, finishLegacyShutdownWorkspaceSettlement, maintainLegacyShutdownWorkspaceSettlement, legacyShutdownWorkspaceResourceProtected, sweepLegacyShutdownWorkspaceSettlements } from "../legacy-shutdown-workspace-settlement.js";
|
||||
import { adapterExecutionControls, createAdapterExecutionControl } from "../adapter-execution-control.js";
|
||||
import { logger } from "../../middleware/logger.js";
|
||||
@@ -23,7 +24,7 @@ const native = vi.hoisted(() => ({
|
||||
finalizations: vi.fn(async (_db: Db, _ids?: string[], _options?: unknown) => undefined),
|
||||
cleanups: vi.fn(async () => undefined),
|
||||
drainMaintenance: vi.fn<() => Promise<void>>(async () => undefined),
|
||||
claims: vi.fn<() => Promise<NativeRestartRecoveryDisposition[]>>(async () => []),
|
||||
claims: vi.fn<typeof claimNativeRestartRecoveries>(async () => []),
|
||||
}));
|
||||
const hotRestart = vi.hoisted(() => ({
|
||||
read: vi.fn<() => Promise<HotRestartIntent | null>>(async () => null),
|
||||
@@ -608,6 +609,33 @@ describePostgres("heartbeat recovery module ownership", () => {
|
||||
await vi.waitFor(() => expect(deps.activeRunExecutionPromises.size).toBe(0));
|
||||
});
|
||||
|
||||
it("does not dispatch recovery for a run still preparing in this heartbeat process", async () => {
|
||||
const run = await seedRun();
|
||||
await db.update(agents).set({ adapterType: "paperclip_runner" }).where(eq(agents.id, run.agentId));
|
||||
await db.update(issues).set({ executionRunId: run.id }).where(eq(issues.id, run.issueId!));
|
||||
await db.update(heartbeatRuns).set({ runtimeMode: "native", nativeIssueId: run.issueId }).where(eq(heartbeatRuns.id, run.id));
|
||||
await db.insert(nativeRunFinalizations).values({ runId: run.id, companyId: run.companyId, issueId: run.issueId!, phase: "observed" });
|
||||
const deps = callbacks(db);
|
||||
deps.activeRunExecutions.add(run.id);
|
||||
deps.getRun.mockImplementation(async id => (await db.select().from(heartbeatRuns).where(eq(heartbeatRuns.id, id)))[0] ?? null);
|
||||
native.claims.mockImplementationOnce(claimNativeRestartRecoveries);
|
||||
const before = await db.select().from(nativeRunFinalizations).where(eq(nativeRunFinalizations.runId, run.id));
|
||||
try {
|
||||
const result = await createHeartbeatRecovery(db, deps).recoverNativeRunsAfterRestart();
|
||||
expect(result.awaitingEvidenceRunIds).toEqual([run.id]);
|
||||
expect(result.claims).toEqual([]);
|
||||
expect(deps.executeRun).not.toHaveBeenCalled();
|
||||
expect(await db.select().from(nativeRunFinalizations).where(eq(nativeRunFinalizations.runId, run.id))).toEqual(before);
|
||||
expect(await db.select().from(issueRecoveryActions).where(eq(issueRecoveryActions.sourceIssueId, run.issueId!))).toHaveLength(0);
|
||||
expect((await deps.getRun(run.id))?.status).toBe("running");
|
||||
} finally {
|
||||
deps.activeRunExecutions.delete(run.id);
|
||||
await Promise.all([...deps.activeRunExecutionPromises]);
|
||||
await db.delete(nativeRunFinalizations).where(eq(nativeRunFinalizations.runId, run.id));
|
||||
await db.delete(heartbeatRuns).where(eq(heartbeatRuns.id, run.id));
|
||||
}
|
||||
});
|
||||
|
||||
it("does not claim or execute recovery when persisted finalization fails", async () => {
|
||||
const deps = callbacks(db);
|
||||
native.finalizations.mockRejectedValueOnce(new Error("result settlement failed"));
|
||||
|
||||
@@ -959,6 +959,7 @@ export function createHeartbeatRecovery(db: Db, dependencies: HeartbeatRecoveryD
|
||||
}
|
||||
const dispositions = await claimNativeRestartRecoveries({
|
||||
db,
|
||||
liveRunExecutions: activeRunExecutions,
|
||||
restartKind,
|
||||
recoveryRequestId: intent?.recoveryRequestId ?? null,
|
||||
coordinatedPreviousController: intent
|
||||
|
||||
@@ -11,6 +11,7 @@ import { buildNativeRuntimeContext } from "../native-runtime/runtime-context.js"
|
||||
import { nativeRuntimeContextFixture } from "../native-runtime/runtime-context.test-fixture.js";
|
||||
import { prepareNativeWorkspaceSync } from "../native-runtime/native-workspace-sync.js";
|
||||
import { resolveHeartbeatNativeRuntimeMode } from "../native-runtime/runtime-mode.js";
|
||||
import { claimNativeRestartRecoveries } from "../native-runtime/native-restart-recovery.js";
|
||||
import { canonicalNativeRuntimeContextDigest } from "../../vendor/paperclip-runner/index.js";
|
||||
import { selectHeartbeatRuntime, type HeartbeatRuntimeSelectionInput } from "./runtime-selection.js";
|
||||
|
||||
@@ -74,6 +75,31 @@ describe.skipIf(!support.supported)("heartbeat runtime selection boundary", () =
|
||||
const coordinators = (id: string) => db.select().from(nativeRunFinalizations).where(eq(nativeRunFinalizations.runId, id));
|
||||
const sandbox = { kind: "remote" as const, transport: "sandbox" as const, remoteCwd: "/workspace", reusableLeaseConfigured: false };
|
||||
|
||||
it("keeps recovery out of live preparation before the workspace descriptor exists", async () => {
|
||||
const { input, run, issue } = await fixture();
|
||||
input.workspace.executionTarget = sandbox;
|
||||
await db.update(issues).set({ executionRunId: run.id }).where(eq(issues.id, issue.id));
|
||||
const liveRunExecutions = new Set([run.id]);
|
||||
const recoveryInput = { db, restartKind: "hard" as const, runIds: [run.id], liveRunExecutions };
|
||||
vi.mocked(prepareNativeWorkspaceSync).mockImplementationOnce(async () => {
|
||||
const saved = await read(run.id);
|
||||
expect(saved.runnerProfileJson).toHaveProperty("nativeExecutionInput");
|
||||
expect(saved.runnerProfileJson).not.toHaveProperty("nativeWorkspaceSync");
|
||||
const before = await coordinators(run.id);
|
||||
expect(await claimNativeRestartRecoveries(recoveryInput)).toEqual([
|
||||
{ kind: "awaiting_evidence", runId: run.id, reason: "current_controller_preparing" },
|
||||
]);
|
||||
expect(await coordinators(run.id)).toEqual(before);
|
||||
expect((await read(run.id)).status).toBe("running");
|
||||
return null;
|
||||
});
|
||||
expect((await selectHeartbeatRuntime(db, input)).selected).toBe(true);
|
||||
liveRunExecutions.delete(run.id);
|
||||
expect(await claimNativeRestartRecoveries(recoveryInput)).toEqual([
|
||||
expect.objectContaining({ kind: "bootstrap_incomplete", runId: run.id, controllerGeneration: 1 }),
|
||||
]);
|
||||
});
|
||||
|
||||
it("persists the native input, completion contract, and coordinator before transferring cleanup ownership", async () => {
|
||||
const { input, run } = await fixture();
|
||||
input.workspace.onCheckpoint = vi.fn(async () => {});
|
||||
|
||||
@@ -465,6 +465,7 @@ export async function claimNativeRestartRecoveries(input: {
|
||||
restartKind: NativeRestartKind;
|
||||
recoveryRequestId?: string | null;
|
||||
runIds?: string[];
|
||||
liveRunExecutions?: Pick<ReadonlySet<string>, "has">;
|
||||
now?: Date;
|
||||
limit?: number;
|
||||
coordinatedPreviousController?: {
|
||||
@@ -580,6 +581,14 @@ export async function claimNativeRestartRecoveries(input: {
|
||||
} as const;
|
||||
}
|
||||
|
||||
if (input.liveRunExecutions?.has(row.run.id)) {
|
||||
return {
|
||||
kind: "awaiting_evidence",
|
||||
runId: row.run.id,
|
||||
reason: "current_controller_preparing",
|
||||
} as const;
|
||||
}
|
||||
|
||||
const takeover = await evaluateNativeControllerTakeover({
|
||||
owner: row.coordinator,
|
||||
now,
|
||||
|
||||
Reference in new issue
Block a user