From e491b23eb5d66b0cf81b0ae9988ef3c4d8788325 Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Tue, 3 Nov 2026 13:42:21 -0600 Subject: [PATCH] fix(runner): retire sandbox suspension receipts after admission (#18829) ## Thinking Path > - Paperclip helps people manage AI agents and their work. > - Native Runner sessions preserve a conversation across task turns. > - Remote suspension records a local receipt for the stopped Runner identity. > - The built-in sandbox path now writes the same receipt as plugin tasks. > - Admission cleared that receipt only for plugin tasks. > - A later warm turn could reject a clean journal because the old receipt named an earlier run. > - This pull request clears the receipt after successful admission for both sandbox paths. ## Linked Issues or Issue Description Refs #18343 and #18554 for the shared suspension receipt producer. Refs #18718 for the related controller projection refresh. **What happened?** After a suspended built-in sandbox session resumed, its next warm turn could fail with `runner_state_identity_mismatch: prior_owner_terminal_state_indeterminate`. Its journal named the completed resumed run. Its local suspension receipt still named the older run. **Expected behavior** Successful admission must retire the prior suspension receipt. The next compatible warm turn must reuse the admitted session and provider history once. A failed admission must retain recovery evidence. **Steps to reproduce** 1. Complete and suspend remote turn A. 2. Resume turn B through the built-in sandbox transport. 3. Keep the controller warm, with full Runner state owned by the sandbox. 4. Admit turn C. The old local receipt conflicts with B's journal identity. **Paperclip version or commit** Reproduced in the executor regression on base `367f29478d`. **Deployment mode** Native Runner with the built-in remote sandbox transport and a warm lifecycle. ## What Changed - Clear the local suspension receipt after a remote sandbox session passes provider recovery and durable admission. Apply this to built-in runners and plugin tasks. - Extend the existing three-turn warm admission regression to cover a suspended receipt and remote-only Runner state. - Keep an active-owner control that rejects admission and retains its receipt. Keep the existing foreign-owner, retirement, configuration, plugin, and unchanged-session controls. - Add a real Linux Runner continuity case that suspends A, resumes B, and keeps B warm for C. Register the test fixture's owned local API listener. - Document receipt ownership and the new continuity case. ## Verification - The new behavioral regression failed before the production change. Turn C raised the exact identity mismatch above. The active-owner control passed and retained its receipt. - All 1,087 native executor tests pass after the fix. - Both opt-in Linux process cases pass at the final commit: ordinary warm reuse and suspension followed by warm reuse. They use PostgreSQL, TLS, current-source Rust binaries, and a deterministic Codex fixture. Each preserves one conversation, starts exactly three turns, and reuses the warm Runner and provider on turn C. - Full repository typecheck passes with `node scripts/typecheck-low-memory.mjs`. Full production build passes with `pnpm build`. - All current-head CI checks pass at `a95a6fdfe8`, including the full test shards, browser shards, Rust checks, build, and typecheck. The policy job needed one retry after repository checkout timed out before checks ran. - Greptile and Ponytail each give 5/5 at the final head. There are no open review threads or findings. - The local full `pnpm test:run` attempt passed 351 cases in its first chat shard. The session interruption stopped it during the second shard, so it has no complete local verdict. The complete CI test run passes. - Module boundary checks and `git diff --check` pass. ## Risks - A receipt removed before admission could discard recovery evidence. Removal stays in the successful `onSession` callback. The negative admission control checks retention. - A stale owner must not gain authority. Journal, run, lease, and scope checks remain in place before the callback. - Historical contradictory or quarantined state still requires the existing verified recovery path. This change does not delete those records. - No migration or provider configuration change is required. ## Model Used - OpenAI Codex, based on GPT-6. The runtime does not expose a more specific model ID or context limit. Used reasoning, repository tools, code execution, and tests. ## 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 #` 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 --- doc/execution-semantics.md | 5 ++-- doc/testing-native-remote-continuity.md | 4 +++ .../src/__tests__/helpers/native-restart.ts | 2 ++ ...unner-restart-recovery.integration.test.ts | 13 +++++---- .../native-session-executor.test.ts | 29 ++++++++++++++----- .../native-runtime/native-session-executor.ts | 3 +- 6 files changed, 41 insertions(+), 15 deletions(-) diff --git a/doc/execution-semantics.md b/doc/execution-semantics.md index d93ae631b6c..7d50d479125 100644 --- a/doc/execution-semantics.md +++ b/doc/execution-semantics.md @@ -1602,8 +1602,9 @@ agent, workspace, retained session, and termination of prior execution. The checkpoint must match the exact Runner, lease, run, session, turn, and item. A missing restart claim does not remove these checks or authorize a fresh session. -After a plugin session passes provider recovery and durable run admission, its -local suspension receipt is removed. The running session then owns authority; +After a remote sandbox session passes provider recovery and durable run admission, +its local suspension receipt is removed. This applies to built-in sandbox runners +and plugin tasks. The running session then owns authority; the old receipt must not conflict with a later warm run's journal identity. Failed admission retains the receipt. Normal suspension writes a new receipt. diff --git a/doc/testing-native-remote-continuity.md b/doc/testing-native-remote-continuity.md index 65b00378d69..0f06c319e23 100644 --- a/doc/testing-native-remote-continuity.md +++ b/doc/testing-native-remote-continuity.md @@ -19,6 +19,10 @@ same Runner and provider history through another healthy follow-up. This is a controlled cache fault, not a reconstruction of a historical incident. Each recovery case includes another healthy turn after the recovered turn and asserts that it does not start another runner or provider process. +The `warm-suspended` case suspends the first turn, resumes the second turn, +then keeps that controller warm for the third turn. It must retire the first +turn's local suspension receipt so the third turn can reuse the resumed Runner +and provider history without a second process or repeated turn. The same-server case also starts preparation lease renewal during recovery and holds instruction collection open past a renewal tick after coordinator release. The executor must acquire renewal before preparation releases its reference, then diff --git a/server/src/__tests__/helpers/native-restart.ts b/server/src/__tests__/helpers/native-restart.ts index a05f5149f05..deccd592af1 100644 --- a/server/src/__tests__/helpers/native-restart.ts +++ b/server/src/__tests__/helpers/native-restart.ts @@ -16,6 +16,7 @@ import { prepareNativeHeartbeatRun } from "../../services/native-runtime/prepare import { createIsolatedTestHome } from "./isolated-test-home.js"; import { type NativeControllerIdentity } from "../../services/native-runtime/native-restart-recovery.js"; import { waitForPendingRunFailureReports } from "../../services/run-failure-report.js"; +import { registerLocalRuntimeApiServer } from "../../runtime-api.js"; // An explicitly supplied disposable database also permits qualification on hosts // where native Postgres cannot allocate semaphores. Never fall back silently. @@ -106,6 +107,7 @@ export function nativeRestartFixture() { : new Date(), }; server = createServer(); + registerLocalRuntimeApiServer(server); await new Promise((resolveListen) => server!.listen(0, "127.0.0.1", resolveListen), ); diff --git a/server/src/services/native-runtime/native-runner-restart-recovery.integration.test.ts b/server/src/services/native-runtime/native-runner-restart-recovery.integration.test.ts index aa23a2d35b0..b7d3403eb9a 100644 --- a/server/src/services/native-runtime/native-runner-restart-recovery.integration.test.ts +++ b/server/src/services/native-runtime/native-runner-restart-recovery.integration.test.ts @@ -150,12 +150,13 @@ describeEmbeddedPostgres("native runner restart recovery with real processes", ( expect(next.kind).toBe("bootstrap_incomplete"); }); - it.skipIf(!process.env.PAPERCLIP_TEST_RUNNER_CONTAINER).each(["warm", "hard", "stale-projection", "legacy", "changed", "same-server", "diskless", "diskless-per-turn", "diskless-per-turn-read-retry", "diskless-warm-suspended", "diskless-warm-suspended-read-retry", "diskless-live-inspection"])("preserves remote warm continuity across completed turns: %s", async (scenario) => { - const hardRestart = !["warm", "same-server"].includes(scenario); + it.skipIf(!process.env.PAPERCLIP_TEST_RUNNER_CONTAINER).each(["warm", "hard", "warm-suspended", "stale-projection", "legacy", "changed", "same-server", "diskless", "diskless-per-turn", "diskless-per-turn-read-retry", "diskless-warm-suspended", "diskless-warm-suspended-read-retry", "diskless-live-inspection"])("preserves remote warm continuity across completed turns: %s", async (scenario) => { + const hardRestart = !["warm", "same-server", "warm-suspended"].includes(scenario); const diskless = scenario.startsWith("diskless"); const perTurn = scenario.startsWith("diskless-per-turn"); - const warmSuspended = scenario.startsWith("diskless-warm-suspended"); + const warmSuspended = scenario === "warm-suspended" || scenario.startsWith("diskless-warm-suspended"); const resumedAfterSuspension = perTurn || warmSuspended; + const suspendedBeforeThird = perTurn || (diskless && warmSuspended); const container = process.env.PAPERCLIP_TEST_RUNNER_CONTAINER!; const tunnel = await createRunnerTestTlsTunnel({ port: Number(process.env.PAPERCLIP_TEST_RUNNER_PORT), key: process.env.PAPERCLIP_TEST_RUNNER_TLS_KEY!, cert: process.env.NODE_EXTRA_CA_CERTS! }); @@ -310,6 +311,8 @@ describeEmbeddedPostgres("native runner restart recovery with real processes", ( }; const first = hardRestart ? await executeInController(firstOptions) : await executeFirst(); expect(first.exitCode).toBe(0); + if (scenario === "warm-suspended") expect(await closeIdleWarmNativeSessionsForRestart()) + .toEqual({ closed: 1, busy: 0, failed: 0 }); await finalizeNativeRun({ db: fixture.db, runId: fixture.runId, workspaceFinalizeStatus: "succeeded", projectRunStatus: true }); const snapshot = await runner.execute({ command: "sh", args: ["-c", `cat '${remoteCwd}'/.paperclip-runtime/paperclip-runner/sessions/*/runner/runner-state.json`] }); expect(snapshot.exitCode).toBe(0); @@ -476,9 +479,9 @@ describeEmbeddedPostgres("native runner restart recovery with real processes", ( const finalCalls = await runner.execute({ command: "cat", args: [`${remoteCwd}/provider-calls.log`] }); expect(finalCalls.stdout.split("\n").filter(line => line === "turn/start")).toHaveLength(3); expect(finalCalls.stdout.split("\n").filter(line => line === "thread/start")).toHaveLength(1); - expect(finalCalls.stdout.split("\n").filter(line => line === "process-start")).toHaveLength(providerStarts + (resumedAfterSuspension ? 1 : 0)); + expect(finalCalls.stdout.split("\n").filter(line => line === "process-start")).toHaveLength(providerStarts + (suspendedBeforeThird ? 1 : 0)); const finalProcess = await runner.execute({ command: "sh", args: ["-c", `cat '${remoteCwd}'/.paperclip-runtime/paperclip-runner/sessions/*/runner/runner-process.identity`] }); - if (resumedAfterSuspension) expect(finalProcess.stdout).not.toBe(processAfter.stdout); + if (suspendedBeforeThird) expect(finalProcess.stdout).not.toBe(processAfter.stdout); else expect(finalProcess.stdout).toBe(processAfter.stdout); expect(await closeIdleWarmNativeSessionsForRestart()).toEqual({ closed: perTurn ? 0 : 1, busy: 0, failed: 0 }); const suspended = await runner.execute({ command: "sh", args: ["-c", `cat '${remoteCwd}'/.paperclip-runtime/paperclip-runner/sessions/*/runner/runner-state.json`] }); diff --git a/server/src/services/native-runtime/native-session-executor.test.ts b/server/src/services/native-runtime/native-session-executor.test.ts index 725f07f2ea0..94fa60242b5 100644 --- a/server/src/services/native-runtime/native-session-executor.test.ts +++ b/server/src/services/native-runtime/native-session-executor.test.ts @@ -12967,9 +12967,10 @@ describe("runnerd provider runtime wiring", () => { }, ); - it.each((["codex", "claude"] as const).flatMap(provider => (["unchanged", "external-checkpoint", "changed", "runtime-context", "retirement-failed", "stale-owner", "active-owner", "missing-owner", "foreign-environment", "plugin-replaced", "plugin-managed-github"] as const).filter(scenario => provider === "codex" || scenario.startsWith("plugin-")).map(scenario => [provider, scenario] as const)))("rotates an owned warm %s remote runner before checking changed configuration: %s", async (provider, scenario) => { + it.each((["codex", "claude"] as const).flatMap(provider => (["unchanged", "external-checkpoint", "suspension-receipt", "suspension-active-owner", "changed", "runtime-context", "retirement-failed", "stale-owner", "active-owner", "missing-owner", "foreign-environment", "plugin-replaced", "plugin-managed-github"] as const).filter(scenario => provider === "codex" || scenario.startsWith("plugin-")).map(scenario => [provider, scenario] as const)))("rotates an owned warm %s remote runner before checking changed configuration: %s", async (provider, scenario) => { const plugin = scenario === "plugin-replaced" || scenario === "plugin-managed-github"; - const reusable = ["unchanged", "external-checkpoint", "plugin-managed-github"].includes(scenario); + const suspensionReceipt = scenario.startsWith("suspension-"); + const reusable = suspensionReceipt || ["unchanged", "external-checkpoint", "plugin-managed-github"].includes(scenario); const { permissionPolicy: _permissionPolicy, ...claudeProfile } = QUALIFIED_ACPX_PROFILES.claude; const first = { ...scopedNativeExecution(`warm-admission-${provider}-${scenario}`), schema: "paperclip.native-execution-input.v5" as const, @@ -12978,7 +12979,7 @@ describe("runnerd provider runtime wiring", () => { if (provider === "claude") first.session.driverKind = "acpx_runtime"; first.session.lifecyclePolicy = { mode: "warm", idleTimeoutMs: 60_000 }; const next = { ...first, binding: { ...first.binding, runId: `${first.binding.runId}-next` }, - session: { ...first.session, lifecyclePolicy: { mode: "warm" as const, idleTimeoutMs: ["unchanged", "external-checkpoint", "runtime-context", "stale-owner", "missing-owner", "plugin-replaced", "plugin-managed-github"].includes(scenario) ? 60_000 : 90_000 } } }; + session: { ...first.session, lifecyclePolicy: { mode: "warm" as const, idleTimeoutMs: suspensionReceipt || ["unchanged", "external-checkpoint", "runtime-context", "stale-owner", "missing-owner", "plugin-replaced", "plugin-managed-github"].includes(scenario) ? 60_000 : 90_000 } } }; if (scenario === "runtime-context") { next.runtimeContext = { ...first.runtimeContext, mcp: { ...first.runtimeContext.mcp, digest: "1".repeat(64) } }; @@ -13084,11 +13085,17 @@ describe("runnerd provider runtime wiring", () => { const ordinarySelect = nextDb.select.bind(nextDb); nextDb.select = ((fields?: Record) => fields?.status === heartbeatRuns.status && fields?.runnerProfileJson === heartbeatRuns.runnerProfileJson - ? priorNativeRunDb(scenario === "stale-owner" ? older : first, scenario === "active-owner" ? "running" : "succeeded").select(fields as never) + ? priorNativeRunDb(scenario === "stale-owner" ? older : first, ["active-owner", "suspension-active-owner"].includes(scenario) ? "running" : "succeeded").select(fields as never) : ordinarySelect(fields as never)) as Db["select"]; try { await executePaperclipNativeSession({ db: firstDb, execution: first, runnerInstanceId: identity.runnerInstanceId, useRunnerd: true, runnerExecutionTarget: pluginTarget as never,...pluginOptions }); + if (suspensionReceipt) { + // Remote suspension leaves a local identity receipt, while the Runner + // owns its full state in the sandbox. A resumed turn must retire it. + await writeFile(join(root, "plugin-runner-state.json"), JSON.stringify(durableRunnerState(identity, "suspended"))); + await rm(join(root, "runner/runner-state.json")); + } if (scenario === "stale-owner") await writeRunnerAuthority(root, { ...identity, runId: older.binding.runId }, "ready"); if (scenario === "missing-owner") { await closeWarmNativeSessionsForEnvironment({ environmentId, reason: "fixture owner loss" }); @@ -13101,12 +13108,14 @@ describe("runnerd provider runtime wiring", () => { runnerInstanceId: `runner-next-${scenario}`, useRunnerd: true, runnerExecutionTarget: (scenario === "foreign-environment" ? { ...target, environmentId: `${environmentId}-other` } : plugin ? {...pluginTarget,leaseId:"next-core-lease"} : target) as never,...pluginOptions }); - if (["stale-owner", "active-owner", "missing-owner", "foreign-environment"].includes(scenario)) { - await expect(continuation).rejects.toThrow(scenario === "active-owner" + if (["stale-owner", "active-owner", "suspension-active-owner", "missing-owner", "foreign-environment"].includes(scenario)) { + await expect(continuation).rejects.toThrow(["active-owner", "suspension-active-owner"].includes(scenario) ? "prior_owner_active" : "prior_owner_terminal_state_indeterminate"); expect(state.execute).toHaveBeenCalledOnce(); expect(firstSession.close).not.toHaveBeenCalled(); expect(order).toEqual([]); + if (suspensionReceipt) expect(JSON.parse(await readFile(join(root, "plugin-runner-state.json"), "utf8"))) + .toMatchObject({ ...identity, lifecycle: "suspended" }); } else if (scenario === "retirement-failed") { await expect(continuation).rejects.toThrow("retirement unconfirmed"); expect(state.execute).toHaveBeenCalledOnce(); @@ -13120,8 +13129,9 @@ describe("runnerd provider runtime wiring", () => { } if (scenario === "plugin-managed-github") expect(rebind).toHaveBeenCalledOnce(); expect(order).toEqual(reusable ? ["execute-next"] : ["retire", "execute-next"]); - if (["unchanged", "external-checkpoint"].includes(scenario)) { + if (["unchanged", "external-checkpoint", "suspension-receipt"].includes(scenario)) { if (!persistedController) await writeRunnerAuthority(root, { ...identity, runId: next.binding.runId }, "ready"); + if (suspensionReceipt) await rm(join(root, "runner/runner-state.json")); const third = { ...next, binding: { ...next.binding, runId: `${next.binding.runId}-third` } }; const thirdDb = leaseDb(third); const thirdSelect = thirdDb.select.bind(thirdDb); @@ -13139,6 +13149,11 @@ describe("runnerd provider runtime wiring", () => { runnerExecutionTarget: pluginTarget as never,...pluginOptions })).resolves.toMatchObject({ exitCode: 0 }); expect(firstSession.close).not.toHaveBeenCalled(); expect(state.execute).toHaveBeenCalledTimes(3); + if (suspensionReceipt) { + expect(existsSync(join(root, "plugin-runner-state.json"))).toBe(false); + expect(JSON.parse(await readFile(join(root, "control-plane/control-plane-state.json"), "utf8")).identity) + .toMatchObject({ ...identity, runId: next.binding.runId }); + } if (restore) { expect(restore.mock.calls.map(([, binding]) => binding.runId)).toEqual([first.binding.runId, next.binding.runId]); expect(existsSync(root)).toBe(true); diff --git a/server/src/services/native-runtime/native-session-executor.ts b/server/src/services/native-runtime/native-session-executor.ts index c68407087e2..7264f55e6f1 100644 --- a/server/src/services/native-runtime/native-session-executor.ts +++ b/server/src/services/native-runtime/native-session-executor.ts @@ -9844,7 +9844,8 @@ async function executePaperclipNativeSessionWithinScope( ); }, onSession: async (session) => { - if (session && usesPluginTask) { + if (session && input.runnerExecutionTarget?.kind === "remote" + && input.runnerExecutionTarget.transport === "sandbox") { rmSync(resolve(scopedRunnerdStateRoot(input.execution), "plugin-runner-state.json"), { force: true }); } if (session && session === existingWarmSession) remoteWarmRecoveryPending = false;