mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-10 12:37:55 +01:00
fix(server): preserve sessions and defer busy overlap recovery (#18552)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - A web server replacement must preserve remote execution and later task continuation. > - The active-run handoff now survives deletion of the departing server home. > - The next admission still compared disposable instruction and skill paths as configuration. > - A native receipt could also settle without the old adapter returning its terminal metadata. > - A replacement can also collide with a run row locked by the serving controller. > - This change preserves durable session identity and terminal evidence, and defers locked recovery work so overlap can reach readiness. ## Linked Issues or Issue Description Refs #18543, #18366, #9977, #18118. **What happened?** A real Codex/Daytona journey replaced both control-plane servers and deleted their homes. Reads, saves, saved files and active execution passed. A later useful answer used a new provider session. All four run records lacked the native terminal contract. The whole qualification therefore failed. A later attempt also exposed startup failure: the replacement waited for a row lock held by the serving controller and aborted on its one-second lock timeout. **Expected behavior** An unchanged managed instruction entry and immutable skill version retain the session across server cache relocation. Actual content and configuration changes still reset it. A committed native receipt retains its terminal contract even when the originating adapter never returns. A busy recovery row defers through the existing scheduler while the replacement reaches readiness. **Steps to reproduce** Run the `rolling-web-deployment.runner-codex.daytona.recover-controller` Product E2E cell with separate controller homes and shared PostgreSQL/S3. The focused regressions reproduce the failures without a model call. ## What Changed - Read the managed instruction entry digest from the company-scoped durable file head. Do not restore or read the cache for this check. - Omit disposable instruction paths only when that durable authority exists. Fingerprint selected immutable skill versions in both runtime skill inputs. - Preserve the existing local and external instruction path checks, and unversioned skill identity. - Publish the accepted native terminal contract in the same transaction as the canonical result receipt. - Skip locked recovery rows and schedule the same run for retry. Avoid an audit write that would block on the same row. No ownership is transferred while busy. - Keep replacement readiness independent of an in-progress skill selection; migration and cache eviction still require exact pins. Missing durable skill versions still refuse startup. - Project only the instruction digest in SQL instead of loading the entire agent-file manifest per admission. - Add real PostgreSQL lock contention tests for the issue, run and coordinator, including the heartbeat startup caller and later claim. - Limit native test teardown to its owned database on a shared test host. The old behavior also fails on clean master. - Add cache-loss, scope, changed-content, changed-version and finalization regressions. Update the deployment qualification record. ## Verification - Both focused regressions failed before the source changes. - All 249 tests pass across `heartbeat-workspace-session`, `agent-file-durability`, and `paperclip-control-plane-port`. - Server typecheck and exact-revision build pass. - Assembled admission ran 59 passing cases but failed during native-suite teardown. The teardown failure reproduced on clean master. All 16 native resumption cases and teardown pass after the database-scope fix. - All three row-lock regressions fail with the pre-fix lock timeout and pass with deferred recovery. The full recovery suite passes 74 cases, including the startup caller; eight Docker-specific cases were not enabled. - The real attempt at `d2165dec5d` failed when B aborted on the run lock. Its credential and bucket were removed; all three provider sandboxes were independently confirmed absent. The next real attempt at `aae8a53d01` failed a different readiness gate while a run was preparing skill pins. That race now has a failing-before regression; all 33 readiness, durable-file and managed-skill tests pass after the fix. Its owned resources are cleaned. The `a15a12651c` live attempt passed both web replacements: 765 reads, 78 saves and 78 verified saves with zero HTTP probe failures. The first three turns preserved the provider session and native terminal contract. The fourth continuation failed because a safely suspended remote Runner was treated as live/idle; that distinct recovery gap is being handled in a follow-up. All three owned sandboxes were independently confirmed absent. No claim of full Railway qualification yet. ## Risks - The new fingerprint shape may reset a session once when upgrading from older fingerprint metadata. It is stable across subsequent cache replacements. - A versioned skill is identified by its immutable selected version; materialization and admission still enforce availability. Unversioned skills retain their existing path boundary. - A native terminal describes the accepted execution result. Workspace and issue settlement remain separate authorities. - This does not yet prove the ten-deploy staging campaign, natural database autosuspension or the production canary. ## Model Used OpenAI GPT-6 Codex, with reasoning, repository editing, shell execution and live integration-test tools. The exact runtime model variant and context-window size are not exposed in this session. ## 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
7751fc3107
commit
c0550e7010
15 files changed
+239
-56
No files matched your search
@@ -560,11 +560,17 @@ export async function claimNativeRestartRecoveries(input: {
|
||||
await tx.execute(
|
||||
sql`select set_config('statement_timeout', '15000', true), set_config('lock_timeout', '1000', true)`,
|
||||
);
|
||||
await tx
|
||||
const busy = () => ({ kind: "awaiting_evidence" as const, runId: candidate.runId,
|
||||
reason: "recovery_rows_busy", retryAt: new Date(now.getTime() + 1_000) });
|
||||
// During overlap the serving controller can be committing this run.
|
||||
// Defer that concrete recovery obligation instead of failing startup
|
||||
// or waiting while holding a lock the serving controller needs.
|
||||
const [issue] = await tx
|
||||
.select({ id: issues.id })
|
||||
.from(issues)
|
||||
.where(eq(issues.id, candidate.issueId))
|
||||
.for("update");
|
||||
.for("update", { skipLocked: true });
|
||||
if (!issue) return busy();
|
||||
const row = await tx
|
||||
.select({
|
||||
run: heartbeatRuns,
|
||||
@@ -586,15 +592,11 @@ export async function claimNativeRestartRecoveries(input: {
|
||||
),
|
||||
)
|
||||
.where(eq(heartbeatRuns.id, candidate.runId))
|
||||
.for("update")
|
||||
.for("update", { skipLocked: true })
|
||||
.limit(1)
|
||||
.then((rows) => rows[0] ?? null);
|
||||
if (!row) {
|
||||
return {
|
||||
kind: "blocked",
|
||||
runId: candidate.runId,
|
||||
reason: "recovery_rows_missing",
|
||||
} as const;
|
||||
return busy();
|
||||
}
|
||||
if (isNativeRunnerOwnershipHeld(row.run)) {
|
||||
return {
|
||||
|
||||
@@ -36,6 +36,7 @@ import {
|
||||
EMBEDDED_POSTGRES_TEST_TIMEOUT_MS,
|
||||
} from "@paperclipai/db";
|
||||
import { afterAll, beforeAll, describe, expect, it, vi } from "vitest";
|
||||
import { heartbeatService } from "../heartbeat.js";
|
||||
|
||||
import {
|
||||
canonicalNativeRuntimeContextDigest,
|
||||
@@ -322,6 +323,41 @@ describeEmbeddedPostgres("native runner restart recovery with real processes", (
|
||||
};
|
||||
}
|
||||
|
||||
it.each(["issue", "run", "coordinator"] as const)("defers a busy %s row without blocking replacement startup", async kind => {
|
||||
const fixture = await seedRun(`OVERLAP-BUSY-${kind}`);
|
||||
let acquired!: () => void, release!: () => void;
|
||||
const locked = new Promise<void>(resolve => { acquired = resolve; });
|
||||
const released = new Promise<void>(resolve => { release = resolve; });
|
||||
const holder = fixture.db.transaction(async tx => {
|
||||
if (kind === "issue") await tx.select().from(issues).where(eq(issues.id, fixture.issueId)).for("update");
|
||||
else if (kind === "run") await tx.select().from(heartbeatRuns).where(eq(heartbeatRuns.id, fixture.runId)).for("update");
|
||||
else await tx.select().from(nativeRunFinalizations).where(eq(nativeRunFinalizations.runId, fixture.runId)).for("update");
|
||||
acquired(); await released;
|
||||
});
|
||||
await locked;
|
||||
const now = new Date();
|
||||
const heartbeat = heartbeatService(fixture.db);
|
||||
try {
|
||||
const dispositions = await claimNativeRestartRecoveries({ db: fixture.db, controller: successor,
|
||||
restartKind: "hard", runIds: [fixture.runId], now });
|
||||
expect(dispositions).toEqual([{ kind: "awaiting_evidence", runId: fixture.runId,
|
||||
reason: "recovery_rows_busy", retryAt: new Date(now.getTime() + 1_000) }]);
|
||||
const [coordinator] = await fixture.db.select().from(nativeRunFinalizations).where(eq(nativeRunFinalizations.runId, fixture.runId));
|
||||
expect(coordinator.controllerGeneration).toBe(0);
|
||||
let deadline: ReturnType<typeof setTimeout> | undefined;
|
||||
try {
|
||||
const startup = await Promise.race([
|
||||
heartbeat.recoverNativeRunsAfterRestart(now, [fixture.runId]),
|
||||
new Promise<never>((_resolve, reject) => { deadline = setTimeout(() => reject(new Error("startup blocked on the serving controller")), 2_000); }),
|
||||
]);
|
||||
expect(startup).toMatchObject({ claims: [], awaitingEvidenceRunIds: [fixture.runId] });
|
||||
} finally { clearTimeout(deadline); }
|
||||
} finally { await heartbeat.drainActiveRunExecutions(); release(); await holder; }
|
||||
const [next] = await claimNativeRestartRecoveries({ db: fixture.db, controller: successor,
|
||||
restartKind: "hard", runIds: [fixture.runId] });
|
||||
expect(next.kind).toBe("bootstrap_incomplete");
|
||||
});
|
||||
|
||||
it.skipIf(!process.env.PAPERCLIP_TEST_RUNNER_CONTAINER).each(["warm", "hard", "legacy", "changed", "same-server", "diskless", "diskless-per-turn", "diskless-per-turn-read-retry"])("preserves remote warm continuity across completed turns: %s", async (scenario) => {
|
||||
const hardRestart = !["warm", "same-server"].includes(scenario);
|
||||
const diskless = scenario.startsWith("diskless");
|
||||
|
||||
@@ -733,6 +733,10 @@ describe("PaperclipControlPlanePort conformance", () => {
|
||||
.where(eq(nativeRunFinalizations.runId, identity.runId))).resolves.toEqual([
|
||||
expect.objectContaining({ phase: "committed" }),
|
||||
]);
|
||||
// Completion and restart reconciliation must retain the contract even if
|
||||
// the originating heartbeat never returns from its adapter completion.
|
||||
const [completedRun] = await db.select().from(heartbeatRuns).where(eq(heartbeatRuns.id, identity.runId));
|
||||
expect(completedRun.resultJson?.nativeTerminal).toEqual(CONTROL_PLANE_CONFORMANCE_TERMINAL);
|
||||
await expect(db.select().from(issues).where(eq(issues.id, identity.issueId))).resolves.toEqual([
|
||||
expect.objectContaining({ status: "in_progress", statusVersion: 1 }),
|
||||
]);
|
||||
|
||||
@@ -568,6 +568,9 @@ export class PaperclipControlPlanePort implements ControlPlanePort {
|
||||
nativePhaseUpdatedAt: new Date(),
|
||||
resultJson: {
|
||||
...record(run.resultJson),
|
||||
// Publish the accepted contract with its durable receipt. Recovery
|
||||
// may settle this run without the original adapter ever returning.
|
||||
nativeTerminal: value.terminal,
|
||||
prpTurnTerminalState: value.terminal.turnTerminalState,
|
||||
prpRunTerminalState: value.terminal.runTerminalState,
|
||||
prpReportedWorkDisposition: value.terminal.reportedWorkDisposition,
|
||||
|
||||
Reference in new issue
Block a user