From e719d425571c14727a07899517d5988282115655 Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Tue, 10 Nov 2026 22:53:53 -0600 Subject: [PATCH] fix(runner): accept the acknowledgement of an indeterminate suspend replay (#20714) 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. > - A native run talks to a Runner process (runnerd). The controller in the server sends commands. The Runner journals each command before it acts and replies with a result. > - `runner.suspend` and `runner.shutdown` are terminal commands. The Runner waits for a `command_result_ack` before it exits. It compares that acknowledgement with the receipt that it journaled. > - A Runner that is killed inside `runner.suspend` journals the command but not its result. The next Runner replays the command as `indeterminate`. It sends it on the wire as `failed` with `execution_indeterminate`, and it still treats it as terminal. > - The controller acknowledged the `failed` status that it received. The Runner compared that with its journal status `indeterminate` and exited with "terminal command result acknowledgement changed its durable identity". The controller also lifted its terminal fence for a failed suspend and sent the next command to a Runner that was about to exit. > - This pull request makes both sides agree on the status of that one receipt, and keeps the fence. > - The benefit is that a freeze or restart during a suspend no longer burns the same-run retries and leaves the task at a manual Retry. ## Linked Issues or Issue Description **What happened?** A restart-chaos task (local Runner, Claude, a 28-second freeze of the server during a live turn) needed a manual Retry. Retry worked. 1. The turn completed. The server then sent `runner.suspend`. The server froze before the Runner reported the result. 2. The close found the Runner not durably suspended (`commandStatus: pending`, `runnerLifecycle: ready`). The run failed with `native_runner_checkpoint_owner_lost`, and same-run recovery stopped the old runnerd. The suspend was journaled but not finished. 3. Attempt 2 started a new runnerd on the same state. The welcome delivered the stale suspend. The Runner replayed it as `indeterminate` and sent `failed`. The controller acknowledged `failed`. The Runner exited with code 1: "terminal command result acknowledgement changed its durable identity". The controller had recorded the suspend as `failed`. 4. Attempt 3 found the fence for a suspend that the controller no longer listed as pending. The Runner reconciled it and exited with code 0. The server reported "runnerd exited unexpectedly with code 0", and `native_session_retry_exhausted` stopped automatic recovery. **Expected behavior** The controller acknowledges exactly the receipt that the Runner sent. The Runner accepts that acknowledgement, records `suspended`, and the next attempt starts from a clean suspended Runner. **Steps to reproduce** 1. Start a native run with a local Runner. Let the turn complete so that the server sends `runner.suspend`. 2. Freeze the server (SIGSTOP) for about 30 seconds before the suspend result arrives, so that recovery stops the Runner inside the suspend. 3. Let same-run recovery start a new Runner. It exits with "terminal command result acknowledgement changed its durable identity". The unit tests in this PR reproduce the same sequence without a live run. **Paperclip version or commit** `master` at 200c6e40e2. The failing wave ran on 9296c74e8d plus #20656. **Root cause** `command_result_envelope` changes the journal status `indeterminate` to `failed` on the wire. `wait_for_terminal_result_ack` compared the acknowledgement with the journal status, not with the status that it sent. The controller also treated a `failed` suspend as a refused suspend (the Runner keeps serving), but the Runner treats an indeterminate suspend as terminal. ## What Changed - Runner (`runner.rs`): a new `wire_command_status` gives the status that goes on the wire. `wait_for_terminal_result_ack` accepts the acknowledgement when its status is the wire status (`failed`) or the journal status (`indeterminate`). Command id, type, and controller sequence must still match exactly. - Controller (`durable-prp-control-plane.ts`): for a result with `execution_indeterminate`, the `command_result_ack` names `indeterminate`. Runners that shipped before this change accept only that status. The controller keeps the terminal-lifecycle fence for an indeterminate suspend, so it does not send the next command to a Runner that is exiting. The command stays `failed` in the controller journal, as before. - Runner Rust test `indeterminate_suspend_replay_accepts_the_acknowledged_wire_status`: it seeds a journaled suspend, starts the Runner against a fake controller that acknowledges `failed` and then `indeterminate`, and checks that the Runner finishes as `suspended` with no fence. Before the fix it failed with the production message. - Controller test: it checks the `indeterminate` acknowledgement and that the next queued command is not delivered on that connection. - `docs/durable-recovery.md`: it describes this case. ## Verification - Red, then green: `cargo test -p paperclip-runner-core --lib indeterminate_suspend_replay` failed with `DurableRunnerError("terminal command result acknowledgement changed its durable identity")` before the Runner change and passes after it. - Red, then green: `npx vitest run src/control-plane/durable-prp-control-plane.test.ts -t "indeterminate suspension"` failed (`status: "failed"`, expected `"indeterminate"`) before the controller change and passes after it. - `cargo test -p paperclip-runner-core --lib durable`: 152 passed. - `npx vitest run src/control-plane/durable-prp-control-plane.test.ts`: 215 passed. - `npx vitest run src/live/runnerd-codex-transport-settlement.test.ts src/live/runnerd-codex-transport-suspension.test.ts`: 30 passed. These use the real runnerd binary built from `master`, so they also cover the new controller with an old Runner. - `npx tsc --noEmit -p packages/paperclip-runner`: clean. - `pnpm verify` returned exit 2 (broad input). Required CI supplies the broad coverage. - Not run: a chaos wave. The unit tests reproduce the exact sequence at the Runner and controller boundary. ## Risks - Version mix. Old Runner with new server: the acknowledgement says `indeterminate`, which is the old Runner's journal status, so it is accepted. New Runner with old server: the acknowledgement says `failed`, which is the new Runner's wire status, so it is accepted. New with new: accepted. - No command runs again. The indeterminate suspend is never executed again; only its acknowledgement changes. The Runner still refuses a different command id, type, or controller sequence. - The Runner change takes effect only after a new Runner image. The controller change alone removes the failure for Runners that already shipped. - Remaining gap: the attempt that delivers the stale suspend still ends when that Runner exits as suspended. The next same-run attempt then starts from a clean suspended Runner. A later change can restart the Runner inside the same attempt. > 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, in Claude Code with tool use (shell, file edits, test runs). Extended reasoning. ## 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 --- .../paperclip-runner/docs/durable-recovery.md | 10 ++ .../crates/runner-core/src/durable/runner.rs | 25 ++- .../tests/indeterminate_terminal_ack.rs | 142 ++++++++++++++++++ .../runner-core/src/durable/transport.rs | 1 + .../durable-prp-control-plane.test.ts | 48 ++++++ .../durable-prp-control-plane.ts | 19 ++- 6 files changed, 236 insertions(+), 9 deletions(-) create mode 100644 packages/paperclip-runner/runner/crates/runner-core/src/durable/tests/indeterminate_terminal_ack.rs diff --git a/packages/paperclip-runner/docs/durable-recovery.md b/packages/paperclip-runner/docs/durable-recovery.md index b146dc651a2..040fb5748e2 100644 --- a/packages/paperclip-runner/docs/durable-recovery.md +++ b/packages/paperclip-runner/docs/durable-recovery.md @@ -233,6 +233,16 @@ Attaching starts no turn, and the runner admits the new command only for a settled session, so the run continues instead of failing on the old command. Warm transitions and staged attachments keep their exact replay paths. +A restart can kill the runner inside `runner.suspend` in the same way. The +next runner replays it from the welcome as `execution_indeterminate`, sends it +as `failed`, and still treats it as terminal: it records `suspended` and exits +once the controller acknowledges the result. The controller therefore keeps the +terminal fence for that result (it lifts the fence only for a suspension the +runner refused), and its acknowledgement names the runner's journal status +`indeterminate`. Current runners accept either `indeterminate` or `failed`; older +runners accept only `indeterminate` and otherwise exit with "terminal command +result acknowledgement changed its durable identity". + A kill during that attachment's provider launch can also leave a `spawned` startup fence over a `prepared` state that holds a saved thread. The next runner retires that fence once the recorded provider process and its group have exited, diff --git a/packages/paperclip-runner/runner/crates/runner-core/src/durable/runner.rs b/packages/paperclip-runner/runner/crates/runner-core/src/durable/runner.rs index fb8099b141a..ae1d3bb7aa4 100644 --- a/packages/paperclip-runner/runner/crates/runner-core/src/durable/runner.rs +++ b/packages/paperclip-runner/runner/crates/runner-core/src/durable/runner.rs @@ -1752,13 +1752,18 @@ fn wait_for_terminal_result_ack( "terminal command result acknowledgement payload is required", ) })?; + // The controller acknowledges the status it received. An + // indeterminate journal entry travels as `failed`, so either + // spelling names this exact receipt. + let status = payload.get("status").and_then(Value::as_str); if payload.get("commandId").and_then(Value::as_str) != Some(result.command_id.as_str()) || payload.get("commandType").and_then(Value::as_str) != Some(result.command_type.as_str()) || payload.get("controllerSeq").and_then(Value::as_u64) != Some(result.controller_seq) - || payload.get("status").and_then(Value::as_str) != Some(result.status.as_str()) + || (status != Some(result.status.as_str()) + && status != Some(wire_command_status(result))) { return Err(DurableRunnerError::invalid( "terminal command result acknowledgement changed its durable identity", @@ -2551,18 +2556,24 @@ fn send_outbox( Ok(()) } +/// "indeterminate" is an internal crash-recovery journal state. On the wire it +/// is a failed command with the preserved execution_indeterminate reason so the +/// controller can settle the command and continue replay. +fn wire_command_status(result: &StoredCommandResult) -> &str { + if result.status == "indeterminate" { + "failed" + } else { + result.status.as_str() + } +} + fn command_result_envelope( state: &DurableState, result: &StoredCommandResult, protocol_version: u64, ) -> Value { let mut payload = json!(result); - // "indeterminate" is an internal crash-recovery journal state. On the - // wire it is a failed command with the preserved execution_indeterminate - // reason so the controller can settle the command and continue replay. - if result.status == "indeterminate" { - payload["status"] = json!("failed"); - } + payload["status"] = json!(wire_command_status(result)); json!({ "protocol": PROTOCOL, "version": protocol_version, diff --git a/packages/paperclip-runner/runner/crates/runner-core/src/durable/tests/indeterminate_terminal_ack.rs b/packages/paperclip-runner/runner/crates/runner-core/src/durable/tests/indeterminate_terminal_ack.rs new file mode 100644 index 00000000000..2fd8d69188b --- /dev/null +++ b/packages/paperclip-runner/runner/crates/runner-core/src/durable/tests/indeterminate_terminal_ack.rs @@ -0,0 +1,142 @@ +/// A runnerd killed inside `runner.suspend` leaves it journaled but unfinished. +/// The next process replays it as `indeterminate`, sends it as `failed`, and the +/// controller acknowledges the `failed` it received. That acknowledgement names +/// the same receipt: the runner finishes the suspension instead of exiting with +/// "terminal command result acknowledgement changed its durable identity". +#[test] +fn indeterminate_suspend_replay_accepts_the_acknowledged_wire_status() { + for acknowledged in ["failed", "indeterminate"] { + indeterminate_suspend_replay_is_acknowledged(acknowledged); + } +} + +fn indeterminate_suspend_replay_is_acknowledged(acknowledged_status: &'static str) { + struct NeverExecutes; + + impl super::super::CommandExecutor for NeverExecutes { + fn execute( + &mut self, + command: &Command, + ) -> Result { + panic!("an indeterminate {} must never execute again", command.command_type); + } + } + + let listener = TcpListener::bind("127.0.0.1:0").unwrap(); + let port = listener.local_addr().unwrap().port(); + let mut config = config(port); + config.max_runtime = Duration::from_secs(5); + let directory = std::env::temp_dir().join(format!( + "paperclip-runner-indeterminate-terminal-ack-{}-{acknowledged_status}", + std::process::id() + )); + let _ = std::fs::remove_dir_all(&directory); + config.state_dir = directory.clone(); + let suspend = json!({ + "schema": "paperclip.prp.command.v1", + "commandId": "command_suspend", + "controllerSeq": 1, + "type": "runner.suspend", + "issuedAt": "2026-11-11T01:34:11.000Z", + "payload": {}, + }); + { + // The prior process journaled the suspend before its effect and died. + let store = super::super::DurableStateStore::new(&directory).unwrap(); + let (mut state, _) = store.load_or_create(&config).unwrap(); + let command: Command = serde_json::from_value(suspend.clone()).unwrap(); + assert_eq!( + state.begin_command(&command).unwrap(), + super::super::CommandDisposition::Execute + ); + store.save(&state).unwrap(); + } + + let state = test_state(&config); + let expires = current_unix_ms().unwrap() + 60_000; + let server_config = config.clone(); + let server_state = state.clone(); + let server = thread::spawn(move || { + let (stream, _) = listener.accept().unwrap(); + let mut socket = accept(stream).unwrap(); + let mut secure = server_authenticate( + &mut socket, + &server_config, + &server_state, + ServerCredential { + token: "bootstrap-secret", + kind: "bootstrap", + lease_id: None, + expires_at_unix_ms: expires, + revocation_epoch: 0, + }, + ); + send_secure( + &mut socket, + &mut secure, + &server_config, + &welcome( + &server_state, + "connection_1", + Some("lease-secret"), + expires, + 0, + vec![suspend], + ), + ); + let result = receive_secure(&mut socket, &mut secure, &server_config); + assert_eq!(result["kind"], "command_result"); + assert_eq!(result["payload"]["commandId"], "command_suspend"); + assert_eq!(result["payload"]["status"], "failed"); + assert_eq!( + result["payload"]["result"]["code"], + "execution_indeterminate" + ); + send_secure( + &mut socket, + &mut secure, + &server_config, + &control( + &server_state, + "connection_1", + "command_result_ack", + json!({ + "commandId": "command_suspend", + "commandType": "runner.suspend", + "controllerSeq": 1, + "status": acknowledged_status, + }), + ), + ); + // Acknowledge any terminal suffix until the runner closes. + while let Ok(frame) = receive_plain(&mut socket, server_config.max_frame_bytes) { + let message = secure.decrypt(&frame, true).unwrap(); + if message["kind"] == "event" { + send_secure( + &mut socket, + &mut secure, + &server_config, + &control( + &server_state, + "connection_1", + "ack", + json!({"ackedSourceSeq": message["payload"]["sourceSeq"]}), + ), + ); + } + } + }); + + super::super::run_durable_runner( + config.clone(), + BootstrapTicket::new("bootstrap-secret".to_owned()).unwrap(), + NeverExecutes, + ) + .unwrap(); + server.join().unwrap(); + let store = super::super::DurableStateStore::new(&directory).unwrap(); + let (state, _) = store.load_or_create(&config).unwrap(); + assert_eq!(state.lifecycle, "suspended"); + assert!(state.pending_terminal_delivery.is_none()); + std::fs::remove_dir_all(directory).unwrap(); +} diff --git a/packages/paperclip-runner/runner/crates/runner-core/src/durable/transport.rs b/packages/paperclip-runner/runner/crates/runner-core/src/durable/transport.rs index e64d60ddf9a..8b245375882 100644 --- a/packages/paperclip-runner/runner/crates/runner-core/src/durable/transport.rs +++ b/packages/paperclip-runner/runner/crates/runner-core/src/durable/transport.rs @@ -1764,6 +1764,7 @@ mod tests { #[cfg(unix)] include!("tests/hosted_retention.rs"); include!("tests/ack_release_poll.rs"); + include!("tests/indeterminate_terminal_ack.rs"); use std::net::{IpAddr, Ipv4Addr, TcpListener}; use std::path::PathBuf; 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 fa77aafb347..698f8a475bc 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 @@ -5377,6 +5377,54 @@ describe("DurablePrpControlPlane", () => { } }); + it("acknowledges an indeterminate suspension in the Runner's journal status and keeps its fence", async () => { + // A runnerd killed inside runner.suspend journals it as indeterminate. The + // next runnerd replays it from the welcome as `failed` with + // execution_indeterminate and then exits as suspended. Shipped Runners + // compare the acknowledgement with their journal status and exited with + // "terminal command result acknowledgement changed its durable identity". + const root = mkdtempSync(resolve(tmpdir(), "paperclip-prp-indeterminate-suspend-")); + const controlPlane = new DurablePrpControlPlane({ + stateDirectory: root, + identity, + expectedRunnerVersion, + expectedRunnerDigest, + }); + try { + await controlPlane.start(); + const stale = controlPlane.queueCommand("runner.suspend", {}, "command-suspend-stale"); + const next = controlPlane.queueCommand("session.snapshot", {}, "command-snapshot-next"); + const client = await authenticate(controlPlane, controlPlane.issueBootstrapTicket()); + sendSecure(client!, { + protocol: "paperclip.runner", version: 1, kind: "command_result", + payload: { + commandId: stale.commandId, commandType: stale.type, controllerSeq: stale.controllerSeq, + status: "failed", + result: { + code: "execution_indeterminate", + message: "runner recovered after journaling this command; it will not execute twice", + }, + }, + }); + await expect(receiveSecure(client!)).resolves.toMatchObject({ + kind: "command_result_ack", + payload: { + commandId: "command-suspend-stale", + commandType: "runner.suspend", + controllerSeq: stale.controllerSeq, + status: "indeterminate", + }, + }); + expect(controlPlane.getCommand(stale.commandId)?.status).toBe("failed"); + // The Runner is leaving as suspended; nothing may follow on this connection. + expect(controlPlane.store.state.commandDeliveryCounts[next.commandId]).toBeUndefined(); + client?.socket.destroy(); + } finally { + await controlPlane.stop(); + rmSync(root, { recursive: true, force: true }); + } + }); + it("acknowledges terminal command results after persisting them", async () => { const root = mkdtempSync(resolve(tmpdir(), "paperclip-prp-terminal-ack-")); const controlPlane = new DurablePrpControlPlane({ 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 6e755c227f0..4a0ca8354a8 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 @@ -84,6 +84,15 @@ export function isRunnerRecoveryMaintenanceCommand(type: string): boolean { return ["session.snapshot", "session.goal.get", "runner.drain", "runner.suspend", "runner.shutdown", "turn.stop"].includes(type); } +/** The Runner journaled this command before its effect, then died. It sends + * that `indeterminate` entry as `failed` with `execution_indeterminate`, never + * executes it again, and still treats a lifecycle command as terminal. */ +function isIndeterminateCommandResult(command: DurableRecoveryCoreCommand): boolean { + if (command.status === "indeterminate") return true; + const detail = command.result?.result; + return command.status === "failed" && isRecord(detail) && detail.code === "execution_indeterminate"; +} + const commandTypes = new Set([ "run.prepare", "run.attach", @@ -4102,7 +4111,8 @@ export class DurablePrpControlPlane { * A rejected runner.suspend changes no provider state, and the runner keeps * serving this connection. Lift the terminal-lifecycle fence it installed * so the controller's next command (a fresh suspend) can be delivered. - * Completed suspensions and every shutdown keep the fence. + * Completed suspensions and every shutdown keep the fence. So does an + * indeterminate suspension: the Runner treats it as terminal and exits. */ #releaseServingLifecycle( connection: AuthorityConnection, @@ -4111,6 +4121,7 @@ export class DurablePrpControlPlane { if ( command.type === "runner.suspend" && (command.status === "failed" || command.status === "rejected") && + !isIndeterminateCommandResult(command) && connection.terminalLifecycleCommandId === command.commandId ) { connection.terminalLifecycleCommandId = null; @@ -4159,7 +4170,11 @@ export class DurablePrpControlPlane { commandId: command.commandId, commandType: command.type, controllerSeq: command.controllerSeq, - status: command.status, + // A Runner journals a command it may have started before dying as + // `indeterminate` and sends it as `failed`. Runners compare this + // acknowledgement with their journal, so name that journal status; + // current Runners accept either spelling. + status: isIndeterminateCommandResult(command) ? "indeterminate" : command.status, }, ), );