From f672a9e2e52f5b14b15898d8e413f888f8be5172 Mon Sep 17 00:00:00 2001 From: Tattoofamily Date: Sat, 20 Jun 2026 01:35:53 +1000 Subject: [PATCH] fix(runtime): keep agent pause durable at execution-start (#8317) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Issue (described inline; no existing tracking issue):** Pausing an agent is not durable. Pausing cancels the in-flight run, but a queued or recovery-dispatched run can clobber the agent back to `running` because the execution-start status update is unconditional — so a "paused" agent silently resumes work while `paused_at` is still set. ## Thinking Path > - Paperclip orchestrates AI agents for zero-human companies; agent work executes as "runs" tracked in `heartbeat_runs`, with a recovery/automation layer that re-dispatches work when a run disappears. > - The agent lifecycle has a pause control (status `paused`, `paused_at` set) meant to stop an agent from taking or continuing work. > - The problem: pause is not durable. Pausing cancels the in-flight run, but the execution-start path then sets `agents.status = 'running'` with an unconditional `UPDATE ... WHERE id = ?`, so any queued or recovery-dispatched run can clobber the paused agent back to `running` and execute. > - Why it matters: a "paused" agent silently resuming undermines the core operational control operators rely on to halt runaway, cost-sensitive, or unsafe work. > - This pull request guards the execution-start status flip with an atomic conditional UPDATE, and tags pause-cancellations for observability without changing resume behaviour. > - The benefit is that a paused agent can no longer transition back to `running`; queued/recovery-dispatched runs are cancelled cleanly instead of clobbering status, while un-pausing still resumes in-flight work. ## What Changed - Execution-start guard: replaced the unconditional `UPDATE agents SET status='running' WHERE id = ?` with an atomic conditional `UPDATE ... WHERE id = ? AND status NOT IN ('paused','terminated','pending_approval')`. On a zero-row match the run is cancelled (`errorCode: "agent_not_invokable"`), the issue execution lock is released, and the path returns — instead of clobbering status. - Exported `DIRECT_NON_INVOKABLE_STATUSES` from `agent-invokability.ts` and reused it in `heartbeat.ts` as the single source of truth for the guard. - Pause observability: `cancelActiveForAgentInternal` now accepts an `errorCode` (default `"cancelled"`); the pause-route wrapper `cancelActiveForAgent` passes `"agent_paused"`. This is classification-neutral — `agent_paused` is NOT added to `NON_RETRYABLE_CONTINUATION_ERROR_CODES`, so on un-pause the issue's continuation re-enqueues and work resumes. - Exported `classifyContinuationFailure` from `recovery/service.ts` for unit testing (no logic change). - Added `server/src/services/recovery/service.pause-durability.test.ts` covering continuation classification. ## Verification - `pnpm --filter @paperclipai/server typecheck` — clean. - `pnpm exec vitest run server/src/services/recovery/service.pause-durability.test.ts` — 5 passed. - `pnpm exec vitest run server` — full server suite passes locally. The only failures are pre-existing and environment-specific, unrelated to this change (a git default-branch test fixture, and a known checkout-lock race) — both reproduce identically on clean `master` with this change stashed out. - Behavioural: a paused agent's execution-start now aborts cleanly with no status clobber; non-pause cancellations keep `errorCode "cancelled"` and existing behaviour; un-pausing resumes the in-flight issue. ## Risks - Low risk. No schema change, no migration, no new dependency; four files. The change narrows a single UPDATE to be conditional and adds a rarely-taken abort branch on the run-start path; behaviour for invokable agents is unchanged. The abort's `agent_not_invokable` code is already in `NON_RETRYABLE_CONTINUATION_ERROR_CODES`. The only caller of `cancelActiveForAgent` is the pause route. ## Related upstream work — not duplicates This area has prior and in-flight PRs; #8317 was checked against them and is intentionally distinct: - **#4503** (`fix(heartbeat): make agent pause status guard atomic with status update`) targets a different TOCTOU race on the **post-run / finalize** path (`finalizeAgentStatus`). #8317 targets the **execution-start** race, where a recovery-dispatched run flips a paused agent back to `running` *before the run begins*. #4503 does not cover the proven failure path here: `pause → active run cancelled → recovery dispatches a new run → execution-start overwrites the paused state`. #4503 also does not add the resume semantics below. - **#4356** (`honor system/manual/auto pause at all heartbeat-run enqueue sites`) and **#1067** (`pause guard on queue drain`) protect the **enqueue / queue-drain** layer. They are complementary to — not substitutes for — the execution-start guard, which is the last gate before a run actually starts. - **#6944** (`guard executeRun against paused agent`), **#7140**, and **#7141** attempted similar execution-start ideas but were closed for implementation hygiene / build issues, not because the guard concept was wrong. #8317 implements that concept cleanly: a single atomic conditional UPDATE, a clean abort with `errorCode: "agent_not_invokable"`, a shared `DIRECT_NON_INVOKABLE_STATUSES` source of truth, and passing tests + typecheck. Intentional, minor difference (not a criticism of #4503): #8317's execution-start deny-list is `paused`, `terminated`, and `pending_approval` — the full non-invokable set for run-start invokability — whereas #4503 appears focused on `paused`/`terminated`. The broader set is deliberate for the execution-start guard. Resume semantics: #8317 keeps `agent_paused` as observability-only and classification-neutral (retryable), so a paused agent's in-flight work resumes on un-pause rather than escalating to blocked. ## Model Used - Provider: Anthropic. Model: Claude Opus 4 (`claude-opus-4-8`), via the Claude desktop "Cowork" agent. Mode: agentic/extended reasoning with tool use (shell, file editing, running `tsc`/`vitest`, git). Used to investigate the root cause in source, design the fix, implement it, and validate locally. ## 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 run tests locally and they pass - [x] I have added or updated tests where applicable - [ ] If this change affects the UI, I have included before/after screenshots (N/A — no UI change) - [ ] I have updated relevant documentation to reflect my changes (N/A — no doc-facing change) - [x] I have considered and documented any risks above - [x] I will address all Greptile and reviewer comments before requesting merge - [x] I have searched the open and closed PR list for similar/duplicate PRs and found none --- server/src/services/agent-invokability.ts | 2 +- server/src/services/heartbeat.ts | 60 ++++++++++++++----- .../recovery/service.pause-durability.test.ts | 38 ++++++++++++ server/src/services/recovery/service.ts | 2 +- 4 files changed, 86 insertions(+), 16 deletions(-) create mode 100644 server/src/services/recovery/service.pause-durability.test.ts diff --git a/server/src/services/agent-invokability.ts b/server/src/services/agent-invokability.ts index 75726c92..b9b39d59 100644 --- a/server/src/services/agent-invokability.ts +++ b/server/src/services/agent-invokability.ts @@ -32,7 +32,7 @@ export type AgentInvokability = invalidOrgChain: boolean; }; -const DIRECT_NON_INVOKABLE_STATUSES = new Set([ +export const DIRECT_NON_INVOKABLE_STATUSES = new Set([ "paused", "terminated", "pending_approval", diff --git a/server/src/services/heartbeat.ts b/server/src/services/heartbeat.ts index 66e0d612..dc089b71 100644 --- a/server/src/services/heartbeat.ts +++ b/server/src/services/heartbeat.ts @@ -163,6 +163,7 @@ import { evaluateAgentInvokability, evaluateAgentInvokabilityFromDb, shouldCancelRunsForNonInvokableAgent, + DIRECT_NON_INVOKABLE_STATUSES, type AgentOrgRow, } from "./agent-invokability.js"; import { @@ -8926,25 +8927,51 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {}) .then((rows) => rows[0] ?? null); if (runningWithSession) run = runningWithSession; + // Pause Durability: flip to "running" ONLY if the agent is still invokable. + // Atomic conditional UPDATE is the sole gate (no read-then-write); 0 rows => abort. const runningAgent = await db .update(agents) .set({ status: "running", updatedAt: new Date() }) - .where(eq(agents.id, agent.id)) + .where(and(eq(agents.id, agent.id), notInArray(agents.status, [...DIRECT_NON_INVOKABLE_STATUSES]))) .returning() .then((rows) => rows[0] ?? null); - if (runningAgent) { - publishLiveEvent({ - companyId: runningAgent.companyId, - type: "agent.status", - payload: { - agentId: runningAgent.id, - status: runningAgent.status, - outcome: "running", - }, + if (!runningAgent) { + logger.warn( + { agentId: agent.id, runId: run.id, previousStatus: agent.status }, + "execution-start aborted: agent not invokable", + ); + const abortReason = "Cancelled: agent not invokable at execution-start"; + await setRunStatus(run.id, "cancelled", { + finishedAt: new Date(), + error: abortReason, + errorCode: "agent_not_invokable", + ...(agent ? { + resultJson: mergeRunStopMetadataForAgent(agent, "cancelled", { + resultJson: parseObject(run.resultJson), + errorCode: "agent_not_invokable", + errorMessage: abortReason, + }), + } : {}), }); + await setWakeupStatus(run.wakeupRequestId, "cancelled", { + finishedAt: new Date(), + error: abortReason, + }); + await releaseIssueExecutionAndPromote(run); + return; } + publishLiveEvent({ + companyId: runningAgent.companyId, + type: "agent.status", + payload: { + agentId: runningAgent.id, + status: runningAgent.status, + outcome: "running", + }, + }); + const currentRun = run; await appendRunEvent(currentRun, seq++, { eventType: "lifecycle", @@ -11317,7 +11344,7 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {}) return cancelled; } - async function cancelActiveForAgentInternal(agentId: string, reason = "Cancelled due to agent pause") { + async function cancelActiveForAgentInternal(agentId: string, reason = "Cancelled due to agent pause", errorCode = "cancelled") { const agent = await getAgent(agentId); const runs = await db .select() @@ -11328,11 +11355,11 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {}) await setRunStatus(run.id, "cancelled", { finishedAt: new Date(), error: reason, - errorCode: "cancelled", + errorCode, ...(agent ? { resultJson: mergeRunStopMetadataForAgent(agent, "cancelled", { resultJson: parseObject(run.resultJson), - errorCode: "cancelled", + errorCode, errorMessage: reason, }), } : {}), @@ -11755,7 +11782,12 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {}) cancelRun: (runId: string, reason?: string, options?: CancelRunOptions) => cancelRunInternal(runId, reason, options), - cancelActiveForAgent: (agentId: string, reason?: string) => cancelActiveForAgentInternal(agentId, reason), + /** + * Pause-only. Emits errorCode "agent_paused" unconditionally; its sole caller is the + * agent pause route. For non-pause cancellations use cancelRun, or call the internal + * cancelActiveForAgentInternal(agentId, reason, errorCode) with an explicit errorCode. + */ + cancelActiveForAgent: (agentId: string, reason?: string) => cancelActiveForAgentInternal(agentId, reason, "agent_paused"), cancelInvocationsForAgents: (agentIds: string[], reason: string) => cancelInvocationsForAgentsInternal(agentIds, reason), diff --git a/server/src/services/recovery/service.pause-durability.test.ts b/server/src/services/recovery/service.pause-durability.test.ts new file mode 100644 index 00000000..5e63043f --- /dev/null +++ b/server/src/services/recovery/service.pause-durability.test.ts @@ -0,0 +1,38 @@ +import { describe, expect, it } from "vitest"; +import { classifyContinuationFailure } from "./service.js"; + +const run = (errorCode: string | null) => + ({ errorCode } as unknown as Parameters[0]); + +describe("pause durability: continuation retry classification", () => { + it("agent_paused is retryable so work resumes (Option A: Resume Continues Work)", () => { + // Pause still emits errorCode agent_paused for observability, but it is NOT + // non-retryable. On resume the agent becomes invokable again and this classifies + // as default/retryable, so the continuation re-enqueues and the issue continues + // rather than escalating to blocked. Durability is guaranteed separately by the + // execution-start guard (Change B), not by this classification. + const c = classifyContinuationFailure(run("agent_paused")); + expect(c.kind).toBe("default"); + expect(c.maxAttempts).toBeGreaterThan(0); + }); + + it("agent_not_invokable (execution-start abort) is non-retryable", () => { + expect(classifyContinuationFailure(run("agent_not_invokable")).kind).toBe("non_retryable"); + }); + + it("timed_out (timeout) still retries as transient infra", () => { + const c = classifyContinuationFailure(run("timeout")); + expect(c.kind).toBe("transient_infra"); + expect(c.maxAttempts).toBeGreaterThan(0); + }); + + it("generic cancelled (non-pause cancellation) is NOT non-retryable", () => { + // non-pause cancellations (the internal invokability cancel and budget pause) keep errorCode "cancelled" -> default branch + expect(classifyContinuationFailure(run("cancelled")).kind).toBe("default"); + }); + + it("genuine failure with no/unknown code retries via default branch", () => { + expect(classifyContinuationFailure(run(null)).kind).toBe("default"); + expect(classifyContinuationFailure(run("some_adapter_error")).kind).toBe("default"); + }); +}); diff --git a/server/src/services/recovery/service.ts b/server/src/services/recovery/service.ts index 49c01851..f0e33540 100644 --- a/server/src/services/recovery/service.ts +++ b/server/src/services/recovery/service.ts @@ -204,7 +204,7 @@ type ContinuationRetryClassification = { errorCode: string | null; }; -function classifyContinuationFailure(latestRun: LatestIssueRun): ContinuationRetryClassification { +export function classifyContinuationFailure(latestRun: LatestIssueRun): ContinuationRetryClassification { const errorCode = readNonEmptyString(latestRun?.errorCode); if (errorCode && NON_RETRYABLE_CONTINUATION_ERROR_CODES.has(errorCode)) { return { kind: "non_retryable", maxAttempts: 0, baseBackoffMs: 0, errorCode };