fix(runner): accept the indeterminate command result after a runner restart (#12646)
## 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 <noreply@anthropic.com>
This commit is contained in:
parent
584031af66
commit
0cc40037ac
|
|
@ -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<string, unknown>;
|
||||
}
|
||||
|
|
@ -79,7 +95,13 @@ export interface DurableRecoveryCoreCommand {
|
|||
type: string;
|
||||
issuedAt: string;
|
||||
payload: Record<string, unknown>;
|
||||
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<string, unknown> | null;
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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 });
|
||||
}
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -14,7 +14,13 @@ export interface DurableRecoveryCoreCommand {
|
|||
type: string;
|
||||
issuedAt: string;
|
||||
payload: Record<string, unknown>;
|
||||
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<string, unknown> | null;
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue