From 0cc40037acfdbd19031d483f8a10d0db9591a177 Mon Sep 17 00:00:00 2001 From: Zannis Kalampoukis Date: Tue, 1 Sep 2026 20:21:38 +0300 Subject: [PATCH] fix(runner): accept the indeterminate command result after a runner restart (#12646) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The runner subsystem pairs a Rust runner process with a durable control plane in TypeScript. The control plane is the authority for every command the runner executes > - That pair has a crash-recovery contract. If the runner dies between journaling a command and confirming the command's effect, it must not run the command twice > - The runner keeps its side of the contract. On restart it promotes such a command to the `indeterminate` status and reports that status back > - The control plane did not accept `indeterminate`. It closed the connection without a diagnostic, the runner reconnected and replayed the same result, and the loop repeated forever > - This pull request accepts `indeterminate` as a terminal command status > - The benefit is that a session survives a runner crash during a tool call, instead of hanging until a 30 second deadline expires ## Linked Issues or Issue Description No public issue exists for this defect, so it is described here. **What happened?** A live session cannot resume after the runner process is killed during a governed tool call. The resumed transport waits for the provider identity for 30 seconds and then fails with `runnerd did not report its provider identity`. `packages/paperclip-runner/src/live/live-session.test.ts` covers this exact sequence in "terminates real runnerd after a durable receipt and resumes its exact provider thread". That test has a 15 second budget, so it reports the defect as `Test timed out in 15000ms` and reads like a flake. **Expected behavior** The resumed control plane accepts the runner's recovery report, the runner reports its provider identity, and the session resumes on its original provider thread. **Steps to reproduce** Build the runner binary, then run the test: ``` cargo build --manifest-path packages/paperclip-runner/runner/Cargo.toml --locked --workspace --bins cd packages/paperclip-runner npx vitest run src/live/live-session.test.ts -t "terminates real runnerd" ``` It fails every time on an idle machine. It also fails at `560e7e48b`, the commit that added the test, so the defect is not a recent regression. **Paperclip version or commit** Reproduced on `master` at `0a422fda5`, which is the base of this branch. **Deployment mode** Local development, running the package test suite. **Root cause** `DurablePrpControlPlane.#commandResult` accepted only `completed`, `failed` and `rejected`. The runner reports a journaled-but-unconfirmed command as: ```json { "status": "indeterminate", "result": { "code": "execution_indeterminate", "message": "runner recovered after journaling this command; it will not execute twice" } } ``` That status fell through to a silent `connection.close()`. The runner reconnected after 250 ms, replayed the same result, and was closed again. No durable event ever reached the control plane, so the transport never saw `harness.ready`. `indeterminate` is a deliberate part of the runner's contract. See `reconcile_pending_commands` in `packages/paperclip-runner/runner/crates/runner-core/src/durable/state.rs`. The rest of the TypeScript code already models the status; only this control plane did not. ## What Changed - `DurablePrpControlPlane.#commandResult` accepts `indeterminate` as a terminal command status. - The persisted-state validation accepts `indeterminate`, so a control plane restarted over the same directory can read its own saved state back. Without this, accepting the status would make the next restart throw. - `DurableRecoveryCoreCommand.status` includes `indeterminate` in both declarations of that interface. - Added an integration test that drives the exact recovery frame the runner sends. It asserts the connection stays open, the next command is delivered, the status is persisted, a restarted control plane reloads it, and a replayed duplicate is absorbed rather than treated as a conflict. ## Verification All commands run from `packages/paperclip-runner`. - New test fails before the change and passes after it. Before: `expected null to match object { kind: 'command' }` — `null` is the closed connection. `npx vitest run src/control-plane/durable-prp-control-plane.test.ts` → 4 passed. - The live runner test that exposed this reproduced **deterministically** on an idle machine before the change, and now passes in 3.3 s, well inside its existing 15 s budget. Ran it 10 times in a row: 10/10 pass, 0 failures. `npx vitest run src/live/live-session.test.ts -t "terminates real runnerd"` - Full package suite: `npx vitest run` → 1298 passed, 1 failed. The one failure is `src/mock-core/local-runner.test.ts > cleans up the harness process group when the controller closes`. It fails identically on an unmodified checkout in the same container, so it is a pre-existing environment issue and not related to this change. - Typecheck: `tsc -p tsconfig.json --noEmit` → clean. I did **not** raise the test's timeout. The budget was never the problem — with a 600 s budget the same test still failed, at 31 s, with the real error. ## Risks Low risk, and it widens rather than narrows what is accepted. - Behaviour only changes for a status that is currently rejected, so no previously working path is affected. - `indeterminate` is terminal, not successful. A caller waiting on such a command still receives an error from the transport, which is correct: the effect is genuinely unconfirmed. This change does not make an unconfirmed command look like it succeeded. - The persisted-state change only widens an allow-list, so existing state files stay valid. Open topics for a reviewer: - The control plane closes connections without any diagnostic. That silence is why this defect looked like a flaky test. Adding a diagnostic channel is a larger change and is not included here. - `DurableRecoveryProcessedCommand` in `src/contracts/durable-recovery.ts` drifts from the Rust `StoredCommandResult` by more than this status: it declares `commandDigest` and `logicalEffectCount`, which Rust does not have, and omits `commandType`, which Rust does. That is a separate correction and is deliberately not folded in here. ## Model Used Claude Opus 5 (`claude-opus-5`), extended thinking, with tool use and code execution. Depends-on: none — this is a self-contained fix with no dependent changes. ## 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 --------- Co-authored-by: zannis <1011451+zannis@users.noreply.github.com> Co-authored-by: Claude Opus 5 --- .../src/contracts/durable-recovery.ts | 26 ++++- .../durable-prp-control-plane.test.ts | 94 +++++++++++++++++++ .../durable-prp-control-plane.ts | 18 +++- .../src/control-plane/prp-transport-types.ts | 8 +- 4 files changed, 139 insertions(+), 7 deletions(-) diff --git a/packages/paperclip-runner/src/contracts/durable-recovery.ts b/packages/paperclip-runner/src/contracts/durable-recovery.ts index 5cd271b687..b04d41c1e4 100644 --- a/packages/paperclip-runner/src/contracts/durable-recovery.ts +++ b/packages/paperclip-runner/src/contracts/durable-recovery.ts @@ -37,7 +37,23 @@ export interface DurableRecoveryProcessedCommand { commandId: string; controllerSeq: number; commandDigest: string; - status: "completed" | "failed" | "rejected"; + /** + * The runner's command journal is persisted before a command's effect and + * re-persisted after recovery, so a recovered trace carries the whole + * lifecycle, not just the settled end of it: + * + * - `pending` — journaled, effect not yet confirmed. Written by + * `DurableState::begin_command` and durable from that moment. + * - `indeterminate` — the crash-recovery verdict. + * `DurableState::reconcile_pending_commands` promotes every `pending` + * entry on load and saves the state back, so the command is never + * executed twice. Terminal. + * + * Both are values a consumer can read off `processedCommands`; a union that + * omits them tells the compiler a state the runner routinely writes is + * impossible. + */ + status: "pending" | "completed" | "failed" | "rejected" | "indeterminate"; logicalEffectCount: number; result: Record; } @@ -79,7 +95,13 @@ export interface DurableRecoveryCoreCommand { type: string; issuedAt: string; payload: Record; - status: "pending" | "completed" | "failed" | "rejected"; + /** + * `indeterminate` is the runner's crash-recovery verdict: the command was + * journaled but its effect was never confirmed, so the runner will not + * execute it a second time. It is terminal, like the other non-pending + * statuses. + */ + status: "pending" | "completed" | "failed" | "rejected" | "indeterminate"; result: Record | null; } diff --git a/packages/paperclip-runner/src/control-plane/durable-prp-control-plane.test.ts b/packages/paperclip-runner/src/control-plane/durable-prp-control-plane.test.ts index af7aae9b87..90995500e1 100644 --- a/packages/paperclip-runner/src/control-plane/durable-prp-control-plane.test.ts +++ b/packages/paperclip-runner/src/control-plane/durable-prp-control-plane.test.ts @@ -672,4 +672,98 @@ describe.sequential("DurablePrpControlPlane", () => { rmSync(root, { recursive: true, force: true }); } }); + + it("keeps a recovered runner attached when it reports an indeterminate command", async () => { + const root = mkdtempSync(resolve(tmpdir(), "paperclip-prp-indeterminate-")); + const controlPlane = new DurablePrpControlPlane({ + stateDirectory: root, + identity, + expectedRunnerVersion, + expectedRunnerDigest, + }); + try { + await controlPlane.start(); + const journaled = controlPlane.queueCommand( + "semantic_tool.result", + { callId: "call-1" }, + "command-tool-1", + ); + controlPlane.queueCommand( + "turn.interrupt", + { turnId: "turn-1" }, + "command-interrupt-1", + ); + const client = await authenticate( + controlPlane, + controlPlane.issueBootstrapTicket(), + ); + expect(client?.welcome.payload).toMatchObject({ + pendingCommands: [ + expect.objectContaining({ commandId: "command-tool-1" }), + ], + }); + + // Exactly what runnerd replays after it is killed between journaling a + // command and confirming its effect. Its durable contract promotes such a + // command to `indeterminate` so that it is never executed twice. + const indeterminateResult = { + protocol: "paperclip.runner", + version: 1, + kind: "command_result", + payload: { + commandId: journaled.commandId, + commandType: journaled.type, + controllerSeq: journaled.controllerSeq, + status: "indeterminate", + result: { + code: "execution_indeterminate", + message: + "runner recovered after journaling this command; it will not execute twice", + }, + }, + }; + sendSecure(client!, indeterminateResult); + + // The authority has to accept that terminal status and hand out the next + // command. Closing the connection instead strands the runner in a silent + // reconnect loop that never re-reports its provider identity. + await expect(receiveSecure(client!)).resolves.toMatchObject({ + kind: "command", + payload: { commandId: "command-interrupt-1" }, + }); + expect(controlPlane.store.state.commands).toMatchObject([ + { commandId: "command-tool-1", status: "indeterminate" }, + { commandId: "command-interrupt-1", status: "pending" }, + ]); + + // The runner replays its journal on every reconnect, so the same + // indeterminate result arrives again. It has to be absorbed as a + // duplicate rather than treated as a conflicting result. + sendSecure(client!, indeterminateResult); + await expect(receiveSecure(client!)).resolves.toMatchObject({ + kind: "command", + payload: { commandId: "command-interrupt-1" }, + }); + expect(controlPlane.store.state.duplicateCommandResults).toBe(1); + + client?.socket.destroy(); + await controlPlane.stop(); + + // That result is now persisted, so a control plane restarted over the + // same directory has to be able to read its own state back. + const restarted = new DurablePrpControlPlane({ + stateDirectory: root, + identity, + expectedRunnerVersion, + expectedRunnerDigest, + }); + expect(restarted.store.state.commands).toMatchObject([ + { commandId: "command-tool-1", status: "indeterminate" }, + { commandId: "command-interrupt-1", status: "pending" }, + ]); + } finally { + await controlPlane.stop(); + rmSync(root, { recursive: true, force: true }); + } + }); }); diff --git a/packages/paperclip-runner/src/control-plane/durable-prp-control-plane.ts b/packages/paperclip-runner/src/control-plane/durable-prp-control-plane.ts index e7cb078e5d..408ba27940 100644 --- a/packages/paperclip-runner/src/control-plane/durable-prp-control-plane.ts +++ b/packages/paperclip-runner/src/control-plane/durable-prp-control-plane.ts @@ -380,9 +380,13 @@ function isStoredCoreState( commandTypes.has(command.type) && typeof command.issuedAt === "string" && isRecord(command.payload) && - ["pending", "completed", "failed", "rejected"].includes( - String(command.status), - ) && + [ + "pending", + "completed", + "failed", + "rejected", + "indeterminate", + ].includes(String(command.status)) && (command.result === null || isRecord(command.result)), ) ) { @@ -1641,10 +1645,16 @@ export class DurablePrpControlPlane { return; } const status = result.status; + // `indeterminate` is terminal too: a runner that crashed between journaling + // a command and confirming its effect reports it on recovery and will not + // execute it again. Rejecting it closes the connection, and since the + // runner replays the same result on every reconnect, the session never + // recovers. if ( status !== "completed" && status !== "failed" && - status !== "rejected" + status !== "rejected" && + status !== "indeterminate" ) { connection.close(); return; diff --git a/packages/paperclip-runner/src/control-plane/prp-transport-types.ts b/packages/paperclip-runner/src/control-plane/prp-transport-types.ts index 6a9920573a..dda8167292 100644 --- a/packages/paperclip-runner/src/control-plane/prp-transport-types.ts +++ b/packages/paperclip-runner/src/control-plane/prp-transport-types.ts @@ -14,7 +14,13 @@ export interface DurableRecoveryCoreCommand { type: string; issuedAt: string; payload: Record; - status: "pending" | "completed" | "failed" | "rejected"; + /** + * `indeterminate` is the runner's crash-recovery verdict: the command was + * journaled but its effect was never confirmed, so the runner will not + * execute it a second time. It is terminal, like the other non-pending + * statuses. + */ + status: "pending" | "completed" | "failed" | "rejected" | "indeterminate"; result: Record | null; }