mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-14 00:26:28 +01:00
fix(native): start a fresh session when an accepted run's provider state was not saved (#20811)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - A native run in a per-turn remote sandbox saves a provider state backup after its result is accepted. The sandbox is then released, so the backup is the only copy of the provider session. > - When that backup fails to save (for example, #20772: storage throttling), the run keeps its accepted result. Nothing records that its checkpoint can no longer be resumed. > - The next run on the session rebinds that checkpoint and asks the Runner to resume. The Runner has no state and refuses (#20757). With the normal policy, the run opens a replacement session after that failed attempt. With a governed-wait or unfinished-goal checkpoint, the run fails with `native_provider_state_incompatible`, and only Retry or the next message starts fresh. > - The server already knew the state was gone when the backup failed. The failed resume is wasted work. > - This pull request marks the accepted run's checkpoint as not resumable at the moment the backup fails. The next run then starts a fresh provider session directly. > - The benefit is that a lost backup no longer costs a failed run or a failed resume attempt before the task continues. ## Linked Issues or Issue Description Refs #20757 (the missing-state refusal that the next run reaches today). Refs #20772 (the throttled backup upload that loses the backup). Refs #20809 (a related fix for governed-wait follow-ups that reach the missing state; this PR stops most of them from reaching it). **What happened?** 1. A native run in a per-turn sandbox finished, and its result was accepted. 2. `harness_state.backup.persist` failed. The run recorded `native.post_result_failure`. Its run row kept a normal `sessionCheckpoint`. 3. The sandbox was released. 4. The next run on the same session selected that run as its resume source, and `rebindNativeSessionCheckpoint` returned the checkpoint. 5. The Runner found no saved state and refused with `PRP provider resume state is unavailable`. The run then opened a replacement session after the failed attempt, or (governed-wait or unfinished-goal checkpoint) failed with `native_provider_state_incompatible`. Retry or the next message then started fresh. **Expected behavior** The next run starts a fresh provider session at once, because the server already knows the saved state is not there. The accepted result and the task outcome stay as they are. **Steps to reproduce** 1. Give a run row an accepted result and a normal `sessionCheckpoint`. 2. Make its close checkpoint fail in a per-turn sandbox (`reusableLeaseConfigured: false`). 3. Select the next run's resume source with `findNativeSessionResumeRun` and call `rebindNativeSessionCheckpoint`. 4. On `master` it returns the checkpoint, so the next run tries to resume state that does not exist. **Paperclip version or commit** `master` at `556b249897`. ## What Changed - `server/src/services/native-runtime/native-provider-state-unsaved.ts` (new): `markAcceptedRunProviderStateUnsaved` adds `nativeProviderStateUnsaved: { code: "native_provider_state_incompatible", reason: "harness_backup_not_saved" }` to the run's `runnerProfileJson`. One conditional `UPDATE` writes it only when the run's coordinator holds a result with `schema_status = 'accepted'`. `clearRunProviderStateUnsaved` removes it. - `server/src/services/native-runtime/native-session-executor.ts`: the remote close checkpoint (`checkpointRemoteRunner`) now wraps the existing body. When the checkpoint throws in a sandbox without a reusable lease, it calls the marker, writes one run-log line, and throws the original error. A marker or log failure cannot replace that error. If a later checkpoint of the same run saves, it clears the mark. - `server/src/services/native-runtime/native-session-resume.ts`: `rebindNativeSessionCheckpoint` returns `null` for a source with the mark, as it already does for `errorCode = native_provider_state_incompatible`. `findNativeSessionResumeRun` projects the new key. - `doc/execution-semantics.md`: one additive paragraph. - Tests in `native-session-resume.test.ts` and `native-session-executor.test.ts`. Guard conditions. The mark is written only when all of these are true: - The close checkpoint threw before the durable backup was stored (after the upload's own bounded retries). A later local publication, stamp or cleanup failure does not mark the run. - The target is a remote `sandbox` with `reusableLeaseConfigured !== true`. A retained sandbox or an SSH host keeps the live state, so its resume path is unchanged. - The run holds an accepted result. A failure before acceptance changes nothing. Saved state from the previous release: run rows without the key keep today's resume path. An older server that reads a row with the key ignores it, and so it behaves as it does today. No schema change. User-visible: the next run's transcript shows "Paperclip session started" instead of "resumed", with a new session id. The source run's log shows `provider state was not saved after the accepted result; the next run starts a fresh provider session`. The fresh session gets the full task context, as for other fresh starts. ## Verification - `cd server && npx vitest run src/services/native-runtime/native-session-resume.test.ts -t "not resumable only after|provider state was not saved"`: 9 pass (6 pure, 3 embedded Postgres). - `cd server && npx vitest run src/services/native-runtime/native-session-executor.test.ts -t "marks the provider state unsaved|uses the bundled Linux image authority for remote opencode-new-durable"`: 8 pass. They include a durable store failure (marked) and a local publication failure after the store (not marked). - Red on `master` (source files reverted, tests kept): the 3 "starts the next run in a fresh provider session directly" cases fail (`expected {…} to be null`), the DB "accepted" case fails the same way, and the executor per-turn case fails (`mark` called 0 times). - Guard removal checks: - Sandbox guard replaced with `true`: the "retained sandbox" case fails (`mark` called once). - `!checkpointBackupStored` removed: the "publish-failed" case fails. - `schema_status = 'accepted'` removed from the marker: the "rejected" case fails (`expected true to be false`). - Negative cases that pass: no result, rejected result, retained sandbox, records without the key or with another code (all three recovery policies), and a later save that clears the mark. - `cd server && npx tsc --noEmit -p .`: clean. - Not related to this change and failing on `master` in this worktree too: `runtime-selection.test.ts` connector skill cases (missing `.claude/skills/*/SKILL.md`) and `native-session-resume.test.ts` "archives a damaged prior epoch" (`ENOENT … authority-epochs`). Required CI is the authority for those suites. ## Risks - Low. The change only skips a resume that cannot succeed. The fresh session uses the same workspace and full task context, like a Retry after `native_provider_state_incompatible`. - If the backup failed but the provider state were still somewhere recoverable, the next run would lose provider context. For a per-turn sandbox without a reusable lease, the restore path uses only the backup, so this does not apply. Retained sandboxes and SSH hosts are excluded. - One extra conditional `UPDATE` runs only on the failure path. The normal path before `turn.start` has no new waits or work. - `doc/execution-semantics.md` is also edited by #20809. The paragraphs are at different places; whichever merges second rebases. > 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 edit, test execution). ## 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 <noreply@anthropic.com>
This commit is contained in:
1 parent
1b39cf1794
commit
c8ae02e5ce
7 files changed
+332
-5
No files matched your search
@@ -369,6 +369,15 @@ vi.mock("./native-runner-file-handoff.js", () => ({
|
||||
state.renderNativeRunnerStagedAttachmentPrompt,
|
||||
}));
|
||||
|
||||
const providerStateUnsaved = vi.hoisted(() => ({
|
||||
mark: vi.fn(async (_db: unknown, _binding: unknown) => true),
|
||||
clear: vi.fn(async (_db: unknown, _binding: unknown) => undefined),
|
||||
}));
|
||||
vi.mock("./native-provider-state-unsaved.js", () => ({
|
||||
markAcceptedRunProviderStateUnsaved: providerStateUnsaved.mark,
|
||||
clearRunProviderStateUnsaved: providerStateUnsaved.clear,
|
||||
}));
|
||||
|
||||
vi.mock("./current-wake-comments.js", async importOriginal => ({
|
||||
...await importOriginal<typeof import("./current-wake-comments.js")>(),
|
||||
resolveCurrentWakeCommentsBinding: state.resolveCurrentWakeCommentsBinding,
|
||||
@@ -2326,7 +2335,7 @@ describe("remote provider pack manifest", () => {
|
||||
return JSON.stringify(value);
|
||||
};
|
||||
|
||||
it.each(["opencode", "opencode-new-durable", "opencode-new-durable-reset", "opencode-new-durable-inspect-reset", "opencode-new-durable-provider-reset", "opencode-continuation", "opencode-continuation-missing", "opencode-continuation-no-receipt", "opencode-continuation-replacement", "opencode-continuation-cold", "opencode-continuation-legacy-durable", "opencode-dirty", "opencode-diagnostics", "opencode-diagnostics-stale", "cursor", "claude-x64", "claude-arm64"] as const)("uses the bundled Linux image authority for remote %s without controller overrides", async provider => {
|
||||
it.each(["opencode", "opencode-new-durable", "opencode-new-durable-reset", "opencode-new-durable-inspect-reset", "opencode-new-durable-provider-reset", "opencode-new-durable-store-failed", "opencode-new-durable-publish-failed", "opencode-continuation", "opencode-continuation-missing", "opencode-continuation-no-receipt", "opencode-continuation-replacement", "opencode-continuation-cold", "opencode-continuation-legacy-durable", "opencode-dirty", "opencode-diagnostics", "opencode-diagnostics-stale", "cursor", "claude-x64", "claude-arm64"] as const)("uses the bundled Linux image authority for remote %s without controller overrides", async provider => {
|
||||
const firstDurable = provider.startsWith("opencode-new-durable");
|
||||
// A sandbox connection drops once while the settled state is read out.
|
||||
let checkpointReadResets = provider === "opencode-new-durable-reset" ? 1 : 0;
|
||||
@@ -2422,6 +2431,10 @@ describe("remote provider pack manifest", () => {
|
||||
const capture = vi.spyOn(providerArtifactCapture, "captureRemoteNativeProviderArtifacts");
|
||||
const backup = vi.spyOn(harnessStorage, "persistNativeHarnessBackup").mockImplementation(async input => {
|
||||
expect(JSON.parse(await readFile(join(input.root, "manifest.json"), "utf8")).providerArtifacts).toEqual(newReceipt);
|
||||
// Only the first save fails; a later checkpoint of the same run saves.
|
||||
if (provider === "opencode-new-durable-store-failed" && backup.mock.calls.length === 1) throw new Error("SlowDown: Please reduce your request rate.");
|
||||
// The durable backup is stored; only the later local publication fails.
|
||||
if (provider === "opencode-new-durable-publish-failed") await rm(input.root, { recursive: true, force: true });
|
||||
return undefined as never;
|
||||
});
|
||||
onTestFinished(() => { storage.mockRestore(); owner.mockRestore(); validate.mockRestore(); retain.mockRestore(); capture.mockRestore(); backup.mockRestore(); });
|
||||
@@ -2525,7 +2538,27 @@ describe("remote provider pack manifest", () => {
|
||||
expect(providerArtifactStorage.retainNativeProviderArtifacts).toHaveBeenCalledWith(expect.objectContaining({
|
||||
owner: expect.objectContaining({ runId: remoteExecution.binding.runId }), source: expect.objectContaining({ manifest: installed, launchRoot, accountUid: 1000 }) }));
|
||||
if (inspectionReset) inspectionResets = 1;
|
||||
// Each case starts with no recorded mark or clear calls.
|
||||
providerStateUnsaved.mark.mockClear();
|
||||
providerStateUnsaved.clear.mockClear();
|
||||
if (provider === "opencode-new-durable-store-failed" || provider === "opencode-new-durable-publish-failed") {
|
||||
await expect(driver.mock.calls.at(-1)![0].checkpoint!("settled")).rejects.toThrow();
|
||||
// Only a backup that was never stored makes the next run start fresh.
|
||||
if (provider === "opencode-new-durable-store-failed") {
|
||||
expect(providerStateUnsaved.mark).toHaveBeenCalledWith(preparedDb, remoteExecution.binding);
|
||||
// A later checkpoint of the same run that saves clears the mark once.
|
||||
providerStateUnsaved.clear.mockClear();
|
||||
await driver.mock.calls.at(-1)![0].checkpoint!("settled");
|
||||
expect(providerStateUnsaved.clear).toHaveBeenCalledTimes(1);
|
||||
expect(providerStateUnsaved.clear).toHaveBeenCalledWith(preparedDb, remoteExecution.binding);
|
||||
} else {
|
||||
expect(providerStateUnsaved.mark).not.toHaveBeenCalled();
|
||||
expect(providerStateUnsaved.clear).not.toHaveBeenCalled();
|
||||
}
|
||||
return;
|
||||
}
|
||||
await driver.mock.calls.at(-1)![0].checkpoint!("settled");
|
||||
expect(providerStateUnsaved.mark).not.toHaveBeenCalled();
|
||||
expect(harnessStorage.persistNativeHarnessBackup).toHaveBeenCalledTimes(1);
|
||||
expect(checkpointReadResets).toBe(0);
|
||||
expect(inspectionResets).toBe(0);
|
||||
@@ -17745,6 +17778,70 @@ describe("runnerd provider runtime wiring", () => {
|
||||
}
|
||||
});
|
||||
|
||||
it.each([
|
||||
["a per-turn sandbox", false, true],
|
||||
["a retained sandbox", true, false],
|
||||
] as const)("marks the provider state unsaved when the close checkpoint fails in %s", async (_name, reusable, marked) => {
|
||||
providerStateUnsaved.mark.mockClear();
|
||||
const remoteCwd = join(isolatedStateDirectory, `remote-unsaved-${reusable}`);
|
||||
const current: NativeExecutionInputV1 = { ...execution,
|
||||
binding: { ...execution.binding, companyId: randomUUID(), agentId: randomUUID(), issueId: randomUUID(), runId: randomUUID() },
|
||||
session: { ...execution.session, normalizedSessionId: randomUUID(), lifecyclePolicy: { mode: "per_turn", idleTimeoutMs: null } },
|
||||
};
|
||||
const runnerInstanceId = "unsaved-state-runner";
|
||||
const priorIdentity = { runnerInstanceId, environmentLeaseId: "lease-unsaved", runId: randomUUID(),
|
||||
normalizedSessionId: current.session.normalizedSessionId!, turnId: "turn-prior", itemId: "item-prior" };
|
||||
const remoteExecute = vi.fn(async (command: { command: string; args?: string[] }) => {
|
||||
if (command.args?.[0] === "--build-metadata") return { exitCode: 0, timedOut: false, stderr: "", stdout: JSON.stringify({
|
||||
schema: "paperclip-runner/runnerd-build-metadata/v1", binaryName: "paperclip-runnerd",
|
||||
packageName: "@paperclipai/paperclip-runner", binaryContractVersion: 2,
|
||||
durableSessionCapabilities: ["unlimited_runtime", "connection_lease_renewal"], prpTransportModes: ["listen_ws"],
|
||||
}) };
|
||||
if (command.args?.[0] === "--version") return { exitCode: 0, timedOut: false, stdout: "codex-cli 0.160.0", stderr: "" };
|
||||
if (command.args?.[1]?.includes("base64")) {
|
||||
try { return { exitCode: 0, timedOut: false, stdout: execFileSync("sh", command.args, { encoding: "utf8" }), stderr: "" }; }
|
||||
catch { return { exitCode: 1, timedOut: false, stdout: "", stderr: "" }; }
|
||||
}
|
||||
return { exitCode: 0, timedOut: false, stdout: "", stderr: "" };
|
||||
});
|
||||
const controllerArtifact = join(isolatedStateDirectory, `unsaved-fixture-runnerd-${reusable}`);
|
||||
await writeFile(controllerArtifact, "fixture bootstrap artifact");
|
||||
const logs: string[] = [];
|
||||
await createRunnerdBackend({ db: leaseDb(current), execution: current, runnerInstanceId, runnerIngressAuthorized: true,
|
||||
runnerRemoteBinaryPath: controllerArtifact, runnerRemoteCodexPath: controllerArtifact,
|
||||
onLog: async (_stream: string, chunk: string) => { logs.push(chunk); },
|
||||
runnerExecutionTarget: { kind: "remote", transport: "sandbox", remoteCwd, environmentId: "environment",
|
||||
leaseId: priorIdentity.environmentLeaseId, providerKey: "daytona", reusableLeaseConfigured: reusable,
|
||||
effectiveCapabilities: { runnerWebSocketIngress: true },
|
||||
sandboxLeaseAcquisition: { outcome: "resumed", providerLeaseId: "unsaved-sandbox" },
|
||||
runner: { execute: remoteExecute, syncIn: vi.fn(async () => undefined) },
|
||||
getRunnerIngressEndpoint: async () => ({ kind: "authenticated_websocket", websocketUrl: "wss://sandbox.invalid/runner",
|
||||
secretHeaders: [{ name: "Authorization", value: "fixture-ingress" }], generation: "1", refresh: vi.fn(), close: vi.fn(async () => undefined) }),
|
||||
} as never,
|
||||
} as never);
|
||||
state.createBackend.mock.calls.at(-1)![1].codexTransportFactory!();
|
||||
const options = state.createTransport.mock.calls.at(-1)![0] as RunnerTransportOptions & {
|
||||
controlPlaneRegistration: NonNullable<import("../../vendor/paperclip-runner/index.js").RunnerExecutionDriver["controlPlaneRegistration"]>;
|
||||
};
|
||||
// The sandbox holds no suspended Runner state, so no backup can be saved.
|
||||
const controller = new DurablePrpControlPlane({ stateDirectory: join(options.stateDirectory!, "control-plane"),
|
||||
identity: options.prpIdentity! as ConstructorParameters<typeof DurablePrpControlPlane>[0]["identity"],
|
||||
expectedRunnerVersion: "unsaved-fixture", expectedRunnerDigest: `sha256:${"a".repeat(64)}` });
|
||||
let registration: Awaited<ReturnType<typeof options.controlPlaneRegistration>> | undefined;
|
||||
onTestFinished(async () => { await registration?.release(); await controller.stop(); });
|
||||
registration = await options.controlPlaneRegistration(controller);
|
||||
await expect(registration.checkpoint!("settled")).rejects.toThrow("runner_remote_checkpoint_incomplete");
|
||||
if (marked) {
|
||||
expect(providerStateUnsaved.mark).toHaveBeenCalledTimes(1);
|
||||
expect(providerStateUnsaved.mark.mock.calls[0]![1]).toEqual(current.binding);
|
||||
expect(logs.join("")).toContain("the next run starts a fresh provider session");
|
||||
} else {
|
||||
// A retained sandbox keeps the live state, so the resume path stays.
|
||||
expect(providerStateUnsaved.mark).not.toHaveBeenCalled();
|
||||
expect(logs.join("")).not.toContain("fresh provider session");
|
||||
}
|
||||
});
|
||||
|
||||
it("makes remote authority archival idempotent and returns the archived state", async () => {
|
||||
const remoteExecute = vi.fn();
|
||||
const remoteTarget = {
|
||||
|
||||
Reference in new issue
Block a user