From 2075859f5361b70bf54e56e35fbf74bd850068c8 Mon Sep 17 00:00:00 2001 From: Roy Bales Date: Thu, 10 Sep 2026 09:03:14 -0400 Subject: [PATCH] fix(heartbeat): our own terminal-result cleanup is not the run's failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `runWithTerminalResultCleanup` SIGTERMs the process group once the CLI prints its terminal result line, so an unmanaged background task cannot outlive the run. The process then exits 143 with a good result already parsed. `execute()` knows this — `parsedSucceeded` is computed and then discarded — and the server judges the outcome on `exitCode` alone, so a COMPLETED run is recorded `failed` / `adapter_failed`. Because `adapter_failed` is in TRANSIENT_INFRA_CONTINUATION_ERROR_CODES, the recovery service then continues the issue and re-runs work that already finished. Measured on one board over its last 200 heartbeat runs: 26 completed runs recorded failed/adapter_failed/exit 143, and every one carried subtype "success", is_error false and unmanagedBackgroundTask.terminalResultSeen true — checked individually, not sampled. $28.62 of finished work booked as failed, and 12 retries costing $10.54 redoing it. The other 4 exit-143 runs in the window were genuine (claude_auth_required, max_turns_exhausted) and stay failures. `heartbeat-stop-metadata.ts` already has a branch for exactly this case, keyed on errorCode "unmanaged_background_task_stopped". It is unreachable: no adapter sets that code. The fix keeps `exitCode` and `signal` truthful rather than faking a zero. A new optional `AdapterExecutionResult.stoppedAfterTerminalResult` carries the verdict the adapter already reached, and the server treats a non-zero exit as succeeded only when the adapter says it stopped a process whose result it had already parsed as a success. Additive and optional, so no other adapter changes. Tests: four cases on the claude_local adapter — the self-stop is flagged with exitCode 143 preserved; a genuine failure with cleanup also fired is NOT flagged; a non-zero exit with no cleanup is NOT flagged; a clean exit is untouched. Red before green. --- packages/adapter-utils/src/types.ts | 8 ++ .../execute.terminal-result-cleanup.test.ts | 109 ++++++++++++++++++ .../claude-local/src/server/execute.ts | 6 + server/src/services/heartbeat.ts | 8 ++ 4 files changed, 131 insertions(+) create mode 100644 packages/adapters/claude-local/src/server/execute.terminal-result-cleanup.test.ts diff --git a/packages/adapter-utils/src/types.ts b/packages/adapter-utils/src/types.ts index aab6143296..15341e73b8 100644 --- a/packages/adapter-utils/src/types.ts +++ b/packages/adapter-utils/src/types.ts @@ -90,6 +90,14 @@ export interface AdapterExecutionResult { errorFamily?: AdapterExecutionErrorFamily | null; retryNotBefore?: string | null; errorMeta?: Record; + /** + * True when the adapter itself stopped a process it had already seen a SUCCESSFUL terminal result from — + * i.e. `terminalResultCleanup` fired (see `runWithTerminalResultCleanup` in server-utils) and the parsed + * result was a success. The process then exits non-zero because Paperclip signalled it, not because the + * work failed, so `exitCode` alone cannot decide the outcome. `exitCode`/`signal` stay truthful; the + * server reads this to avoid recording its own cleanup as an adapter failure. Absent means "not applicable". + */ + stoppedAfterTerminalResult?: boolean; usage?: UsageSummary; /** * How `usage` totals are scoped. "per_run" means the tokens cover only this diff --git a/packages/adapters/claude-local/src/server/execute.terminal-result-cleanup.test.ts b/packages/adapters/claude-local/src/server/execute.terminal-result-cleanup.test.ts new file mode 100644 index 0000000000..00a0708e36 --- /dev/null +++ b/packages/adapters/claude-local/src/server/execute.terminal-result-cleanup.test.ts @@ -0,0 +1,109 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +// Paperclip stops a process itself once it has seen the CLI's terminal result line +// (`runWithTerminalResultCleanup`), so an unmanaged background task cannot outlive the run. The process then +// exits 143 with a good result already parsed. The adapter knew that and discarded it, so the server judged the +// run on `exitCode` alone and recorded our own cleanup as an adapter failure. +// +// Measured on one board over 200 runs: 26 completed runs recorded `failed`/`adapter_failed`/exit 143, every one +// carrying subtype "success", is_error false and unmanagedBackgroundTask.terminalResultSeen true. Because +// `adapter_failed` is in TRANSIENT_INFRA_CONTINUATION_ERROR_CODES, recovery then re-ran work that had finished. +const { runAdapterExecutionTargetProcess } = vi.hoisted(() => ({ + runAdapterExecutionTargetProcess: vi.fn(), +})); + +vi.mock("./acp.js", () => ({ + createClaudeAcpExecutor: () => vi.fn(), + formatClaudeAcpFallbackMessage: () => "", + resolveClaudeExecutionEngineForRun: async () => ({ engine: "cli", explicit: true }), +})); + +vi.mock("@paperclipai/adapter-utils/execution-target", async () => { + const actual = await vi.importActual( + "@paperclipai/adapter-utils/execution-target", + ); + return { + ...actual, + ensureAdapterExecutionTargetCommandResolvable: vi.fn(async () => undefined), + ensureAdapterExecutionTargetRuntimeCommandInstalled: vi.fn(async () => undefined), + resolveAdapterExecutionTargetCommandForLogs: vi.fn(async () => "claude"), + runAdapterExecutionTargetProcess, + }; +}); + +import { execute } from "./execute.js"; + +const resultLine = (over: Record = {}) => + JSON.stringify({ + type: "result", + subtype: "success", + is_error: false, + session_id: "s-1", + result: "the work is done", + total_cost_usd: 1.4, + usage: { input_tokens: 10, cache_read_input_tokens: 0, output_tokens: 5 }, + ...over, + }); + +const proc = (over: Record = {}) => ({ + exitCode: 143, + signal: null, + timedOut: false, + stdout: resultLine(), + stderr: "", + pid: 1, + startedAt: new Date().toISOString(), + terminalResultCleanup: { + kind: "terminal_result_cleanup", + stopped: true, + stopReason: "unmanaged_background_task_stopped", + reason: "unmanaged background task stopped; no durable live path", + terminalResultSeen: true, + signal: "SIGTERM", + forceKilled: false, + }, + ...over, +}); + +const ctx = () => ({ + runId: "run-1", + agent: { id: "a-1", companyId: "c-1", name: "Claude", adapterType: "claude_local", adapterConfig: {} }, + runtime: { sessionId: null, sessionParams: null, sessionDisplayId: null, taskKey: null }, + config: { engine: "cli" }, + context: {}, + onLog: vi.fn(async () => {}), +}); + +describe("claude_local: our own terminal-result cleanup is not the run's failure", () => { + beforeEach(() => vi.clearAllMocks()); + + it("flags a success we stopped ourselves, and keeps exitCode/signal truthful", async () => { + runAdapterExecutionTargetProcess.mockResolvedValue(proc()); + const r = await execute(ctx() as never); + expect(r.stoppedAfterTerminalResult).toBe(true); + expect(r.exitCode).toBe(143); // forensics survive: the fix is not to fake a zero + expect(r.errorCode ?? null).toBeNull(); + expect(r.resultJson?.unmanagedBackgroundTask).toMatchObject({ terminalResultSeen: true }); + }); + + it("does NOT flag a run that genuinely failed, even when cleanup also fired", async () => { + runAdapterExecutionTargetProcess.mockResolvedValue( + proc({ stdout: resultLine({ subtype: "error_during_execution", is_error: true }) }), + ); + const r = await execute(ctx() as never); + expect(r.stoppedAfterTerminalResult).toBeUndefined(); + }); + + it("does NOT flag a non-zero exit with no cleanup — an ordinary failure stays a failure", async () => { + runAdapterExecutionTargetProcess.mockResolvedValue(proc({ exitCode: 1, terminalResultCleanup: null })); + const r = await execute(ctx() as never); + expect(r.stoppedAfterTerminalResult).toBeUndefined(); + }); + + it("leaves a clean exit alone — the flag is only for a non-zero exit we caused", async () => { + runAdapterExecutionTargetProcess.mockResolvedValue(proc({ exitCode: 0, terminalResultCleanup: null })); + const r = await execute(ctx() as never); + expect(r.exitCode).toBe(0); + expect(r.stoppedAfterTerminalResult).toBeUndefined(); + }); +}); diff --git a/packages/adapters/claude-local/src/server/execute.ts b/packages/adapters/claude-local/src/server/execute.ts index 02230de87a..8525fbd323 100644 --- a/packages/adapters/claude-local/src/server/execute.ts +++ b/packages/adapters/claude-local/src/server/execute.ts @@ -1227,10 +1227,16 @@ export async function execute(ctx: AdapterExecutionContext): Promise