From 55cbe46fa329849bc45647c6774249d0111f8d58 Mon Sep 17 00:00:00 2001 From: kkroo Date: Wed, 5 Aug 2026 01:45:29 +0000 Subject: [PATCH 1/3] fix(issues): restore pre-checkout status when a run releases without advancing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `checkout` promotes an issue to `in_progress` on entry, but every lock-release path cleared only the execution-lock columns and left `status` behind. So `in_progress` was not a statement about current work — it was a high-water mark of every issue any wake had ever touched, and it only ever came back down by hand. Measured on the CTO queue: 124 issues `in_progress`, 96 of them with no `executionRunId`, no lock and no monitor; of 46 hand-demoted one day, 22 were back within 21h. Record the pre-checkout status in a new `issues.checkout_restore_status` column, captured inside checkout's own UPDATE (Postgres reads the old tuple in SET), so the restore is exact rather than guessed — a `backlog` issue returns to `backlog`, not `todo`. `restoreCheckoutPromotedStatus` is one guarded statement, safe to call inside the caller's existing transaction. It no-ops unless the issue is still `in_progress`, a restore marker is present, and neither `checkout_run_id` nor `execution_run_id` points at a live run. Any explicit status write clears the marker, which is what keeps both "the run advanced the issue" and a deliberate `in_progress` write from being clobbered. Wired into the release paths that strand a status: - issues.ts clearExecutionRunIfTerminal / clearCheckoutRunIfTerminal - recovery/service.ts run finalize - heartbeat.ts scheduled-retry gate cancel Rows stranded before this change carry no marker; re-checkout adopts them with a `todo` marker so the existing backlog drains instead of needing hand-demotion. Fixes BLO-20649. --- .../0211_issue_checkout_restore_status.sql | 9 ++ packages/db/src/migrations/meta/_journal.json | 7 + packages/db/src/schema/issues.ts | 6 + server/src/__tests__/issues-service.test.ts | 145 ++++++++++++++++++ server/src/services/heartbeat.ts | 4 + server/src/services/issue-checkout-status.ts | 96 ++++++++++++ server/src/services/issues.ts | 31 ++-- server/src/services/recovery/service.ts | 5 + 8 files changed, 294 insertions(+), 9 deletions(-) create mode 100644 packages/db/src/migrations/0211_issue_checkout_restore_status.sql create mode 100644 server/src/services/issue-checkout-status.ts diff --git a/packages/db/src/migrations/0211_issue_checkout_restore_status.sql b/packages/db/src/migrations/0211_issue_checkout_restore_status.sql new file mode 100644 index 000000000000..604d08e2ff9a --- /dev/null +++ b/packages/db/src/migrations/0211_issue_checkout_restore_status.sql @@ -0,0 +1,9 @@ +-- BLO-20649: `checkout` promotes an issue to `in_progress` on entry, but every +-- lock-release path clears only the execution-lock columns and leaves `status` +-- behind, so `in_progress` degrades into a high-water mark of every issue any +-- wake has ever touched. +-- +-- Record the status the issue held immediately before checkout so a release that +-- did not advance the issue can put it back exactly (a `backlog` issue returns to +-- `backlog`, not `todo`). NULL means "no checkout-promotion to undo". +ALTER TABLE "issues" ADD COLUMN "checkout_restore_status" text; diff --git a/packages/db/src/migrations/meta/_journal.json b/packages/db/src/migrations/meta/_journal.json index e98f771f069b..8acd42c18f8c 100644 --- a/packages/db/src/migrations/meta/_journal.json +++ b/packages/db/src/migrations/meta/_journal.json @@ -1422,6 +1422,13 @@ "when": 1785514532668, "tag": "0210_approval_linked_agent", "breakpoints": true + }, + { + "idx": 211, + "version": "7", + "when": 1785419604000, + "tag": "0211_issue_checkout_restore_status", + "breakpoints": true } ] } diff --git a/packages/db/src/schema/issues.ts b/packages/db/src/schema/issues.ts index e62616dddccb..9b67cbfc4d4a 100644 --- a/packages/db/src/schema/issues.ts +++ b/packages/db/src/schema/issues.ts @@ -45,6 +45,12 @@ export const issues = pgTable( executionRunId: uuid("execution_run_id").references(() => heartbeatRuns.id, { onDelete: "set null" }), executionAgentNameKey: text("execution_agent_name_key"), executionLockedAt: timestamp("execution_locked_at", { withTimezone: true }), + // Status held immediately before `checkout` promoted this row to + // `in_progress`. A release that did not advance the issue restores it from + // here, so `in_progress` stops accumulating as a high-water mark. Null means + // there is no checkout promotion to undo — either the row was never checked + // out, or a run has since written a status of its own. See BLO-20649. + checkoutRestoreStatus: text("checkout_restore_status"), createdByAgentId: uuid("created_by_agent_id").references(() => agents.id), createdByUserId: text("created_by_user_id"), responsibleUserId: text("responsible_user_id"), diff --git a/server/src/__tests__/issues-service.test.ts b/server/src/__tests__/issues-service.test.ts index 89c93127f877..bbb79a0b61ee 100644 --- a/server/src/__tests__/issues-service.test.ts +++ b/server/src/__tests__/issues-service.test.ts @@ -618,6 +618,151 @@ describeEmbeddedPostgres("issueService.list participantAgentId", () => { }; } + // BLO-20649: `checkout` promotes to `in_progress`; releasing the lock has to + // undo that promotion, or `in_progress` becomes a high-water mark of every + // issue any wake ever touched. + describe("checkout status restore", () => { + async function seedCheckoutFixture(status: "todo" | "backlog" | "blocked") { + const companyId = await seedAssignableAgentCompany(); + const agentId = randomUUID(); + await db.insert(agents).values(agentRow(companyId, { id: agentId, name: "RestoreCoder" })); + const issue = await svc.create(companyId, { + title: `Restore round trip from ${status}`, + description: null, + status, + priority: "medium", + }); + const runId = randomUUID(); + await db.insert(heartbeatRuns).values({ + id: runId, + companyId, + agentId, + status: "running", + invocationSource: "manual", + }); + return { companyId, agentId, issue, runId }; + } + + async function finishRun(runId: string, status = "succeeded") { + await db.update(heartbeatRuns).set({ status }).where(eq(heartbeatRuns.id, runId)); + } + + function readIssue(id: string) { + return db + .select({ status: issues.status, restore: issues.checkoutRestoreStatus }) + .from(issues) + .where(eq(issues.id, id)) + .then((rows) => rows[0]!); + } + + it("returns a todo issue to todo when the run releases without advancing it", async () => { + const { agentId, issue, runId } = await seedCheckoutFixture("todo"); + + await svc.checkout(issue.id, agentId, ["todo"], runId); + expect(await readIssue(issue.id)).toMatchObject({ status: "in_progress", restore: "todo" }); + + await finishRun(runId); + await svc.clearCheckoutRunIfTerminal(issue.id); + + expect(await readIssue(issue.id)).toMatchObject({ status: "todo", restore: null }); + }); + + it("returns a backlog issue to backlog, not todo", async () => { + const { agentId, issue, runId } = await seedCheckoutFixture("backlog"); + + await svc.checkout(issue.id, agentId, ["backlog"], runId); + expect(await readIssue(issue.id)).toMatchObject({ status: "in_progress", restore: "backlog" }); + + await finishRun(runId); + await svc.clearCheckoutRunIfTerminal(issue.id); + + expect(await readIssue(issue.id)).toMatchObject({ status: "backlog", restore: null }); + }); + + it("keeps a status the run actually wrote", async () => { + const { agentId, issue, runId } = await seedCheckoutFixture("todo"); + + await svc.checkout(issue.id, agentId, ["todo"], runId); + await svc.update(issue.id, { status: "in_review" }); + expect(await readIssue(issue.id)).toMatchObject({ status: "in_review", restore: null }); + + await finishRun(runId); + await svc.clearCheckoutRunIfTerminal(issue.id); + + expect(await readIssue(issue.id)).toMatchObject({ status: "in_review", restore: null }); + }); + + it("keeps an explicit in_progress write instead of resetting it", async () => { + const { agentId, issue, runId } = await seedCheckoutFixture("todo"); + + await svc.checkout(issue.id, agentId, ["todo"], runId); + // Re-asserting in_progress is a deliberate claim by the run, so it clears + // the marker and must survive the release. + await svc.update(issue.id, { status: "in_progress" }); + expect(await readIssue(issue.id)).toMatchObject({ restore: null }); + + await finishRun(runId); + await svc.clearCheckoutRunIfTerminal(issue.id); + + expect(await readIssue(issue.id)).toMatchObject({ status: "in_progress", restore: null }); + }); + + it("does not reset while the checkout run is still live", async () => { + const { agentId, issue, runId } = await seedCheckoutFixture("todo"); + + await svc.checkout(issue.id, agentId, ["todo"], runId); + // Run is still `running`; both clear paths must decline. + await svc.clearExecutionRunIfTerminal(issue.id); + await svc.clearCheckoutRunIfTerminal(issue.id); + + expect(await readIssue(issue.id)).toMatchObject({ status: "in_progress", restore: "todo" }); + }); + + it("does not reset on execution-lock release while a live checkout still holds the row", async () => { + const { companyId, agentId, issue, runId } = await seedCheckoutFixture("todo"); + await svc.checkout(issue.id, agentId, ["todo"], runId); + + // Execution lock moves to a second, terminal run while the original + // checkout run keeps executing — a retry hand-off, not a release. + const retryRunId = randomUUID(); + await db.insert(heartbeatRuns).values({ + id: retryRunId, + companyId, + agentId, + status: "failed", + invocationSource: "manual", + }); + await db + .update(issues) + .set({ executionRunId: retryRunId }) + .where(eq(issues.id, issue.id)); + + await svc.clearExecutionRunIfTerminal(issue.id); + + expect(await readIssue(issue.id)).toMatchObject({ status: "in_progress", restore: "todo" }); + }); + + it("restores a pre-existing strand to todo when it is re-checked-out and released", async () => { + // Rows stranded before this fix carry no marker. Re-checkout adopts them + // with a `todo` marker so the backlog of strands drains instead of + // needing hand-demotion. + const { companyId, agentId, issue, runId } = await seedCheckoutFixture("todo"); + await db + .update(issues) + .set({ status: "in_progress", assigneeAgentId: agentId, checkoutRestoreStatus: null }) + .where(eq(issues.id, issue.id)); + + await svc.checkout(issue.id, agentId, ["todo", "in_progress"], runId); + expect(await readIssue(issue.id)).toMatchObject({ status: "in_progress", restore: "todo" }); + + await finishRun(runId); + await svc.clearCheckoutRunIfTerminal(issue.id); + + expect(await readIssue(issue.id)).toMatchObject({ status: "todo", restore: null }); + expect(companyId).toBeTruthy(); + }); + }); + it("rejects direct terminated assignees with structured conflict details", async () => { const companyId = await seedAssignableAgentCompany(); const terminatedAgentId = randomUUID(); diff --git a/server/src/services/heartbeat.ts b/server/src/services/heartbeat.ts index d034967d6917..93fe634cba4a 100644 --- a/server/src/services/heartbeat.ts +++ b/server/src/services/heartbeat.ts @@ -201,6 +201,7 @@ import { WorkspaceRepoMismatchError, } from "./workspace-runtime.js"; import { issueService } from "./issues.js"; +import { restoreCheckoutPromotedStatus } from "./issue-checkout-status.js"; import { resolveStaleDependabotAlertWakeIssue } from "./dependabot-alert-issues.js"; import { createToolGatewayService } from "./tool-gateway.js"; import { toolAccessService } from "./tool-access.js"; @@ -12925,6 +12926,9 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {}) eq(issues.executionRunId, cancelled.id), ), ); + // The gate cancelled this run before it could do anything, so undo the + // `in_progress` its checkout wrote (BLO-20649). + await restoreCheckoutPromotedStatus(db, gate.issueId); } await appendRunEvent(cancelled, await nextRunEventSeq(cancelled.id), { diff --git a/server/src/services/issue-checkout-status.ts b/server/src/services/issue-checkout-status.ts new file mode 100644 index 000000000000..40d3fd18e52f --- /dev/null +++ b/server/src/services/issue-checkout-status.ts @@ -0,0 +1,96 @@ +import { and, eq, isNotNull, sql } from "drizzle-orm"; +import { heartbeatRuns, issues } from "@paperclipai/db"; + +/** + * Heartbeat run statuses that hold no further claim on an issue. A run in any of + * these is done — it will not write to the issue again — so the issue's execution + * lock and its checkout-promoted status can both be released. + * + * Canonical list; `issues.ts` re-exports it as `TERMINAL_HEARTBEAT_RUN_STATUSES`. + */ +export const TERMINAL_HEARTBEAT_RUN_STATUS_VALUES = [ + "succeeded", + "interrupted", + "failed", + "error", + "adapter_failed", + "cancelled", + "timed_out", +] as const; + +/** + * Undo a checkout's `in_progress` promotion when the run released without + * advancing the issue. + * + * `checkout` records the pre-checkout status in `checkout_restore_status` and + * promotes the row to `in_progress`. Every lock-release path used to clear only + * the execution-lock columns, so the promotion survived forever and `in_progress` + * decayed into a high-water mark of every issue any wake had ever touched + * (BLO-20649). + * + * This is a single guarded statement, so it is safe to call from inside the same + * transaction that clears the lock. It no-ops unless ALL of: + * + * - the issue is still `in_progress` — a run that advanced it to `in_review`, + * `done` or `blocked` keeps the status it set; + * - a restore marker is present — any explicit status write clears the marker, + * so a deliberate `in_progress` write is never clobbered either; + * - neither `checkout_run_id` nor `execution_run_id` points at a live run — a + * still-executing run keeps its claim. + * + * Note both lock columns are checked, not just the one the caller cleared: a + * caller that releases only the execution lock must not reset the status while a + * live checkout still owns the row. + * + * @returns true when a status was actually restored. + */ +export async function restoreCheckoutPromotedStatus( + dbOrTx: { + update: (table: typeof issues) => any; + }, + issueId: string, +): Promise { + const restored = await dbOrTx + .update(issues) + .set({ + status: sql`${issues.checkoutRestoreStatus}`, + checkoutRestoreStatus: null, + updatedAt: new Date(), + }) + .where( + and( + eq(issues.id, issueId), + eq(issues.status, "in_progress"), + isNotNull(issues.checkoutRestoreStatus), + // `x IN (NULL)` is NULL rather than true, so a row with both lock columns + // already cleared correctly matches NOT EXISTS and is restored. + sql`not exists ( + select 1 from ${heartbeatRuns} + where ${heartbeatRuns.id} in (${issues.checkoutRunId}, ${issues.executionRunId}) + and ${heartbeatRuns.status} not in ${sql.raw( + `(${TERMINAL_HEARTBEAT_RUN_STATUS_VALUES.map((s) => `'${s}'`).join(", ")})`, + )} + )`, + ), + ) + .returning({ id: issues.id }); + + return restored.length > 0; +} + +/** + * The value `checkout` should write to `checkout_restore_status` when it promotes + * a row to `in_progress`. + * + * Evaluated against the row's PRE-update column values (Postgres `UPDATE ... SET` + * reads the old tuple), so it captures the status the issue actually held. + * + * A row already sitting in `in_progress` has no meaningful pre-checkout status to + * capture: either an earlier checkout's marker is still there and must be kept, + * or the row is one of the pre-existing strands this fix exists to drain, which + * restores to `todo`. + */ +export const checkoutRestoreStatusExpression = sql`case + when ${issues.status} = 'in_progress' then coalesce(${issues.checkoutRestoreStatus}, 'todo') + else ${issues.status} +end`; diff --git a/server/src/services/issues.ts b/server/src/services/issues.ts index c67e3ce865fb..bae6dfa30514 100644 --- a/server/src/services/issues.ts +++ b/server/src/services/issues.ts @@ -71,6 +71,11 @@ import { } from "@paperclipai/shared"; import { conflict, HttpError, notFound, unprocessable } from "../errors.js"; import { incrementBlockerResolvedWakeMetric } from "./blocker-resolved-wake-metrics.js"; +import { + checkoutRestoreStatusExpression, + restoreCheckoutPromotedStatus, + TERMINAL_HEARTBEAT_RUN_STATUS_VALUES, +} from "./issue-checkout-status.js"; import { logger } from "../middleware/logger.js"; import { parseObject } from "../adapters/utils.js"; import { @@ -919,15 +924,9 @@ function sameRunLock(checkoutRunId: string | null, actorRunId: string | null) { return checkoutRunId == null; } -export const TERMINAL_HEARTBEAT_RUN_STATUSES = new Set([ - "succeeded", - "interrupted", - "failed", - "error", - "adapter_failed", - "cancelled", - "timed_out", -]); +export const TERMINAL_HEARTBEAT_RUN_STATUSES = new Set( + TERMINAL_HEARTBEAT_RUN_STATUS_VALUES, +); const STALE_ISSUE_CONTEXT_RUN_STATUSES = ["queued", "scheduled_retry"] as const; const ISSUE_LIST_DESCRIPTION_MAX_CHARS = 1200; @@ -5787,6 +5786,8 @@ export function issueService(db: Db) { .returning({ id: issues.id }) .then((rows) => rows[0] ?? null); + if (updated) await restoreCheckoutPromotedStatus(tx, issueId); + return Boolean(updated); }); } @@ -5925,6 +5926,8 @@ export function issueService(db: Db) { .returning({ id: issues.id }) .then((rows) => rows[0] ?? null); + if (updated) await restoreCheckoutPromotedStatus(tx, issueId); + return Boolean(updated); }); } @@ -8611,6 +8614,15 @@ export function issueService(db: Db) { ...issueData, updatedAt: new Date(), }; + // An explicit status write means a run (or a human) decided where this + // issue belongs, so there is no longer a checkout promotion to undo. Drop + // the restore marker and the automatic reset in + // `restoreCheckoutPromotedStatus` becomes a no-op — including for a + // deliberate write of `in_progress`, which must survive the run that set + // it. See BLO-20649. + if (issueData.status && issueData.checkoutRestoreStatus === undefined) { + patch.checkoutRestoreStatus = null; + } if (doneTransitionEvidenceVerdict) { patch.lastEvidenceVerdict = doneTransitionEvidenceVerdict; patch.lastEvidenceVerdictEvaluatedAt = new Date(doneTransitionEvidenceVerdict.evaluatedAt); @@ -9195,6 +9207,7 @@ export function issueService(db: Db) { // whose run later parks at queued/scheduled_retry is never reclaimable. executionLockedAt: now, status: "in_progress", + checkoutRestoreStatus: checkoutRestoreStatusExpression, startedAt: now, updatedAt: now, }) diff --git a/server/src/services/recovery/service.ts b/server/src/services/recovery/service.ts index 38df11885255..04e40b090f0b 100644 --- a/server/src/services/recovery/service.ts +++ b/server/src/services/recovery/service.ts @@ -42,6 +42,7 @@ import { issueTreeControlService, } from "../issue-tree-control.js"; import { TERMINAL_HEARTBEAT_RUN_STATUSES, issueService } from "../issues.js"; +import { restoreCheckoutPromotedStatus } from "../issue-checkout-status.js"; import { applyIssueMonitorPolicyTransition, derivePersistedMonitorState, @@ -2509,6 +2510,10 @@ export function recoveryService( ), ); + // The run is finalized; if it never wrote a status of its own, undo the + // `in_progress` its checkout wrote (BLO-20649). + await restoreCheckoutPromotedStatus(tx, input.sourceIssue.id); + return updatedRun; }); if (!finalizedRun) return { kind: "skipped" as const }; From b7b7db4c20c30b92c2c9b8adef33b5a4df64938d Mon Sep 17 00:00:00 2001 From: Omar Ramadan Date: Wed, 5 Aug 2026 04:28:13 -0700 Subject: [PATCH 2/3] fix(issues): include checkout restore status in list projection --- server/src/services/issues.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/server/src/services/issues.ts b/server/src/services/issues.ts index bae6dfa30514..953f30063b84 100644 --- a/server/src/services/issues.ts +++ b/server/src/services/issues.ts @@ -3248,6 +3248,7 @@ const issueListSelect = { // ~14% of pg CPU under list load with 66% of issues > 2KB description). description: sql`substring(${issues.description}, 1, ${ISSUE_LIST_DESCRIPTION_MAX_CHARS})`, status: issues.status, + checkoutRestoreStatus: issues.checkoutRestoreStatus, workMode: issues.workMode, harnessKind: issues.harnessKind, priority: issues.priority, From 45b8d106533b470d9e03a76e0793e23024a46624 Mon Sep 17 00:00:00 2001 From: kkroo Date: Thu, 6 Aug 2026 11:24:38 +0000 Subject: [PATCH 3/3] fix(issues): restore checkout status on every ownership-ending path (BLO-20649) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first pass wired restoration into four release paths but missed the one that matters most: `releaseIssueExecutionAndPromote`, the primary terminal-run finalizer. It has 13 call sites — including ordinary run completion and the process-loss reap — and bulk-clears both `executionRunId` and `checkoutRunId` without touching status, so the headline leak (a no-op run stranding its issue in `in_progress` forever) survived the fix meant to close it. Cover the remaining ownership-ending paths: - `releaseIssueExecutionAndPromote` — restore across every sibling row the run claimed, in the same transaction as the lock clear. - `promoteScheduledRetryRun` retry exhaustion — the retry budget is spent and no run will resume the issue. - `cancelQueuedRunForBlockedDependencies` / `cancelQueuedRunForStaleIssue` — cancelled before the run could advance anything. Restoration is now company-scoped. Callers reach the helper with an issue id read from persisted run context, which is not guaranteed to belong to the company whose lock was just released; requiring `companyId` in the predicate makes a cross-company reset structurally impossible rather than relying on every caller to pre-check. The finalizer uses a new set-based `restoreCheckoutPromotedStatuses`. Its lock clears are deliberately one statement each so cleanup scales with the orphan count without N round-trips, and per-issue restoration would have made restoration the slow half of that same transaction. Both variants share one predicate so the live-run guard cannot drift between them. Also record `checkoutRestoreStatus` on the two fallback checkout promotions (stale-execution-lock adoption and the stale-lock retry). Both promote to `in_progress` like any other checkout, but left the marker NULL — and the helper requires a marker, so their releases could never restore. Tests drive the real finalizer via `cancelRun` rather than calling the helper directly: todo/backlog round trips across siblings, run-written status non-clobber, the live-retry guard, adoption-path marker recording, and cross-company isolation. 241 passed across the 5 affected suites. --- .../execution-lock-orphan-cleanup.test.ts | 119 ++++++++++++++++++ server/src/__tests__/issues-service.test.ts | 65 ++++++++++ server/src/services/heartbeat.ts | 39 +++++- server/src/services/issue-checkout-status.ts | 93 +++++++++++--- server/src/services/issues.ts | 24 +++- server/src/services/recovery/service.ts | 5 +- 6 files changed, 319 insertions(+), 26 deletions(-) diff --git a/server/src/__tests__/execution-lock-orphan-cleanup.test.ts b/server/src/__tests__/execution-lock-orphan-cleanup.test.ts index b29d8e0dbf9d..841b6fe0b41a 100644 --- a/server/src/__tests__/execution-lock-orphan-cleanup.test.ts +++ b/server/src/__tests__/execution-lock-orphan-cleanup.test.ts @@ -458,4 +458,123 @@ describeEmbeddedPostgres("execution lock orphan cleanup", () => { expect(unrelatedAfter?.executionAgentNameKey).toBe("se1"); }); }); + + // BLO-20649: `checkout` promotes an issue to `in_progress` and records what it + // displaced in `checkout_restore_status`. `releaseIssueExecutionAndPromote` is + // the primary terminal-run finalizer, so it is the path that actually decides + // whether `in_progress` is a statement about live work or a high-water mark of + // every issue any wake ever touched. These drive the real finalizer via + // `cancelRun` rather than calling the restore helper directly. + describe("checkout status restore through run finalization", () => { + // `issues.checkout_run_id` / `execution_run_id` are FKs, so the run row has + // to exist before any issue can point at it. + async function seedRun( + companyId: string, + agentId: string, + runId: string, + overrides: { status?: string; contextSnapshot?: Record } = {}, + ) { + await db.insert(heartbeatRuns).values({ + id: runId, + companyId, + agentId, + invocationSource: "assignment", + status: overrides.status ?? "queued", + ...(overrides.contextSnapshot ? { contextSnapshot: overrides.contextSnapshot } : {}), + } as typeof heartbeatRuns.$inferInsert); + } + + async function seedPromotedIssue( + companyId: string, + runId: string, + restoreStatus: "todo" | "backlog" | null, + ) { + return seedIssue(companyId, { + status: "in_progress", + checkoutRestoreStatus: restoreStatus, + checkoutRunId: runId, + executionRunId: runId, + executionAgentNameKey: "ceo", + executionLockedAt: new Date(), + }); + } + + it("returns a checkout-promoted issue to its pre-checkout status when the run finalizes", async () => { + const companyId = await seedCompany(); + const agentId = await seedAgent(companyId, "CEO"); + + const runId = randomUUID(); + await seedRun(companyId, agentId, runId); + const todoIssueId = await seedPromotedIssue(companyId, runId, "todo"); + const backlogIssueId = await seedPromotedIssue(companyId, runId, "backlog"); + await db + .update(heartbeatRuns) + .set({ contextSnapshot: { issueId: todoIssueId } }) + .where(eq(heartbeatRuns.id, runId)); + + await heartbeatService(db).cancelRun(runId); + + const [todoAfter] = await db.select().from(issues).where(eq(issues.id, todoIssueId)); + const [backlogAfter] = await db.select().from(issues).where(eq(issues.id, backlogIssueId)); + + // Restored to the exact tier each issue held, not a blanket `todo`, and + // across every sibling the run touched — not only its context issue. + expect(todoAfter?.status).toBe("todo"); + expect(todoAfter?.checkoutRestoreStatus).toBeNull(); + expect(backlogAfter?.status).toBe("backlog"); + expect(backlogAfter?.checkoutRestoreStatus).toBeNull(); + }); + + it("leaves a run-written status alone when the run finalizes", async () => { + const companyId = await seedCompany(); + const agentId = await seedAgent(companyId, "CEO"); + + const runId = randomUUID(); + await seedRun(companyId, agentId, runId); + // No marker: an explicit status write clears it, which is how a run that + // genuinely advanced the issue — or deliberately re-asserted + // `in_progress` — is protected from the reset. + const advancedIssueId = await seedPromotedIssue(companyId, runId, null); + await db + .update(heartbeatRuns) + .set({ contextSnapshot: { issueId: advancedIssueId } }) + .where(eq(heartbeatRuns.id, runId)); + + await heartbeatService(db).cancelRun(runId); + + const [after] = await db.select().from(issues).where(eq(issues.id, advancedIssueId)); + expect(after?.status).toBe("in_progress"); + expect(after?.executionRunId).toBeNull(); + }); + + it("does not reset while a live retry run still claims the issue", async () => { + const companyId = await seedCompany(); + const agentId = await seedAgent(companyId, "CEO"); + + const finalizingRunId = randomUUID(); + const retryRunId = randomUUID(); + await seedRun(companyId, agentId, finalizingRunId); + await seedRun(companyId, agentId, retryRunId, { status: "running" }); + const issueId = await seedPromotedIssue(companyId, finalizingRunId, "todo"); + // Retry hand-off: `executionRunId` has moved to a still-running retry while + // `checkoutRunId` stays pinned at the finalizing run. The lock clear + // releases only the checkout column, and the live retry must keep the + // issue in `in_progress` so its work is not demoted underneath it. + await db + .update(issues) + .set({ executionRunId: retryRunId }) + .where(eq(issues.id, issueId)); + await db + .update(heartbeatRuns) + .set({ contextSnapshot: { issueId } }) + .where(eq(heartbeatRuns.id, finalizingRunId)); + + await heartbeatService(db).cancelRun(finalizingRunId); + + const [after] = await db.select().from(issues).where(eq(issues.id, issueId)); + expect(after?.status).toBe("in_progress"); + expect(after?.checkoutRestoreStatus).toBe("todo"); + expect(after?.executionRunId).toBe(retryRunId); + }); + }); }); diff --git a/server/src/__tests__/issues-service.test.ts b/server/src/__tests__/issues-service.test.ts index bbb79a0b61ee..b9191b1012f2 100644 --- a/server/src/__tests__/issues-service.test.ts +++ b/server/src/__tests__/issues-service.test.ts @@ -47,6 +47,7 @@ import { parseExecutiveHoldMarkerTimestamp, } from "../services/issues.ts"; import { issueRecoveryActionService } from "../services/issue-recovery-actions.js"; +import { restoreCheckoutPromotedStatus } from "../services/issue-checkout-status.ts"; import { WORKSPACE_WORKTREE_REQUIRES_PROJECT_CODE, WORKSPACE_WORKTREE_REQUIRES_PROJECT_MESSAGE, @@ -761,6 +762,70 @@ describeEmbeddedPostgres("issueService.list participantAgentId", () => { expect(await readIssue(issue.id)).toMatchObject({ status: "todo", restore: null }); expect(companyId).toBeTruthy(); }); + + // The fallback checkout paths (stale-execution-lock adoption, and the + // clear-then-retry below it) also promote to `in_progress`. A promotion that + // does not record what it displaced is unrestorable, so these paths used to + // strand a row permanently even with the release side wired up. + for (const startStatus of ["todo", "backlog"] as const) { + it(`records a restore marker when adopting a stale execution lock from ${startStatus}`, async () => { + const { companyId, agentId, issue, runId } = await seedCheckoutFixture(startStatus); + + // A previous run holds the execution lock and is already terminal, so + // checkout adopts the row rather than taking the primary path. + const deadRunId = randomUUID(); + await db.insert(heartbeatRuns).values({ + id: deadRunId, + companyId, + agentId, + status: "failed", + invocationSource: "manual", + }); + await db + .update(issues) + .set({ executionRunId: deadRunId, executionLockedAt: new Date() }) + .where(eq(issues.id, issue.id)); + + await svc.checkout(issue.id, agentId, [startStatus], runId); + expect(await readIssue(issue.id)).toMatchObject({ + status: "in_progress", + restore: startStatus, + }); + + await finishRun(runId); + await svc.clearCheckoutRunIfTerminal(issue.id); + + expect(await readIssue(issue.id)).toMatchObject({ status: startStatus, restore: null }); + }); + } + + it("does not restore an issue belonging to another company", async () => { + // `restoreCheckoutPromotedStatus` takes issue ids from run context, which + // is not guaranteed to name an issue in the caller's company. The company + // predicate makes a cross-company reset structurally impossible. + const { agentId, issue, runId } = await seedCheckoutFixture("todo"); + await svc.checkout(issue.id, agentId, ["todo"], runId); + await finishRun(runId); + + const foreignCompanyId = await seedAssignableAgentCompany(); + expect( + await restoreCheckoutPromotedStatus(db, { + issueId: issue.id, + companyId: foreignCompanyId, + }), + ).toBe(false); + expect(await readIssue(issue.id)).toMatchObject({ status: "in_progress", restore: "todo" }); + + // Same call, correct company: the row is otherwise fully qualified, so + // this proves the company predicate is what declined above. + expect( + await restoreCheckoutPromotedStatus(db, { + issueId: issue.id, + companyId: issue.companyId, + }), + ).toBe(true); + expect(await readIssue(issue.id)).toMatchObject({ status: "todo", restore: null }); + }); }); it("rejects direct terminated assignees with structured conflict details", async () => { diff --git a/server/src/services/heartbeat.ts b/server/src/services/heartbeat.ts index 93fe634cba4a..444b98e04f73 100644 --- a/server/src/services/heartbeat.ts +++ b/server/src/services/heartbeat.ts @@ -201,7 +201,10 @@ import { WorkspaceRepoMismatchError, } from "./workspace-runtime.js"; import { issueService } from "./issues.js"; -import { restoreCheckoutPromotedStatus } from "./issue-checkout-status.js"; +import { + restoreCheckoutPromotedStatus, + restoreCheckoutPromotedStatuses, +} from "./issue-checkout-status.js"; import { resolveStaleDependabotAlertWakeIssue } from "./dependabot-alert-issues.js"; import { createToolGatewayService } from "./tool-gateway.js"; import { toolAccessService } from "./tool-access.js"; @@ -12928,7 +12931,10 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {}) ); // The gate cancelled this run before it could do anything, so undo the // `in_progress` its checkout wrote (BLO-20649). - await restoreCheckoutPromotedStatus(db, gate.issueId); + await restoreCheckoutPromotedStatus(db, { + issueId: gate.issueId, + companyId: cancelled.companyId, + }); } await appendRunEvent(cancelled, await nextRunEventSeq(cancelled.id), { @@ -13249,6 +13255,12 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {}) updatedAt: now, }) .where(and(eq(issues.id, depIssueId), eq(issues.executionRunId, exhausted.id))); + // Retry budget is spent and no run will resume this issue, so the + // checkout promotion has to come back off (BLO-20649). + await restoreCheckoutPromotedStatus(db, { + issueId: depIssueId, + companyId: exhausted.companyId, + }); } return { outcome: "not_promoted", run: exhausted }; } @@ -15013,6 +15025,9 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {}) eq(issues.executionRunId, run.id), ), ); + // The run was cancelled before it could advance anything, so undo the + // `in_progress` its checkout wrote (BLO-20649). + await restoreCheckoutPromotedStatus(db, { issueId, companyId: run.companyId }); await appendRunEvent(cancelled, await nextRunEventSeq(cancelled.id), { eventType: "lifecycle", @@ -15248,6 +15263,9 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {}) eq(issues.executionRunId, run.id), ), ); + // The run was cancelled before it could advance anything, so undo the + // `in_progress` its checkout wrote (BLO-20649). + await restoreCheckoutPromotedStatus(db, { issueId, companyId: run.companyId }); await appendRunEvent(cancelled, await nextRunEventSeq(cancelled.id), { eventType: "lifecycle", @@ -22793,6 +22811,23 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {}) and(eq(issues.companyId, run.companyId), eq(issues.checkoutRunId, run.id)), ); + // Ownership of these rows just ended. This is the primary terminal-run + // finalizer, so it is also the primary place the BLO-20649 leak was + // reachable: a run that checked an issue out and finalized without writing + // a status of its own left the checkout's `in_progress` promotion behind + // forever. Restoring here — inside the same transaction as the lock clear, + // over the same rows already held FOR UPDATE — is what bounds the + // `in_progress AND execution_run_id IS NULL` population. + // + // The helper no-ops on any row a live run still claims, which is what + // makes this safe for the retry case described above: when a retry has + // taken `executionRunId`, that pointer survives the clear and the retry's + // non-terminal status blocks restoration. + await restoreCheckoutPromotedStatuses(tx, { + issueIds: candidateIssues.map((candidate) => candidate.id), + companyId: run.companyId, + }); + // Deferred-wake promotion is bound to a single primary issue: the run's context // issue when present, otherwise the first candidate we found (preserves the // legacy rows[0] selection for runs that were not tied to a specific issue). diff --git a/server/src/services/issue-checkout-status.ts b/server/src/services/issue-checkout-status.ts index 40d3fd18e52f..b5b86f810a94 100644 --- a/server/src/services/issue-checkout-status.ts +++ b/server/src/services/issue-checkout-status.ts @@ -1,4 +1,4 @@ -import { and, eq, isNotNull, sql } from "drizzle-orm"; +import { and, eq, inArray, isNotNull, sql } from "drizzle-orm"; import { heartbeatRuns, issues } from "@paperclipai/db"; /** @@ -18,6 +18,34 @@ export const TERMINAL_HEARTBEAT_RUN_STATUS_VALUES = [ "timed_out", ] as const; +/** + * The guard shared by both restore entry points: a checkout promotion may only be + * undone while nothing else has a claim on the row. + * + * Kept as one expression so the single-issue and batch variants can never drift + * apart — a divergence here would show up as an issue demoted out from under a + * live run, which is the failure mode this whole guard exists to prevent. + */ +const restorableCheckoutPromotion = and( + eq(issues.status, "in_progress"), + isNotNull(issues.checkoutRestoreStatus), + // `x IN (NULL)` is NULL rather than true, so a row with both lock columns + // already cleared correctly matches NOT EXISTS and is restored. + sql`not exists ( + select 1 from ${heartbeatRuns} + where ${heartbeatRuns.id} in (${issues.checkoutRunId}, ${issues.executionRunId}) + and ${heartbeatRuns.status} not in ${sql.raw( + `(${TERMINAL_HEARTBEAT_RUN_STATUS_VALUES.map((s) => `'${s}'`).join(", ")})`, + )} + )`, +); + +const restoreCheckoutPromotionSet = () => ({ + status: sql`${issues.checkoutRestoreStatus}`, + checkoutRestoreStatus: null, + updatedAt: new Date(), +}); + /** * Undo a checkout's `in_progress` promotion when the run released without * advancing the issue. @@ -42,35 +70,28 @@ export const TERMINAL_HEARTBEAT_RUN_STATUS_VALUES = [ * caller that releases only the execution lock must not reset the status while a * live checkout still owns the row. * + * `companyId` is required rather than inferred. Callers reach this from run + * context (`gate.issueId`, a run's context snapshot), and an issue id read back + * from persisted context is not guaranteed to belong to the company whose lock + * the caller just released. Scoping the predicate makes a cross-company reset + * structurally impossible instead of relying on every caller to pre-check. + * * @returns true when a status was actually restored. */ export async function restoreCheckoutPromotedStatus( dbOrTx: { update: (table: typeof issues) => any; }, - issueId: string, + target: { issueId: string; companyId: string }, ): Promise { const restored = await dbOrTx .update(issues) - .set({ - status: sql`${issues.checkoutRestoreStatus}`, - checkoutRestoreStatus: null, - updatedAt: new Date(), - }) + .set(restoreCheckoutPromotionSet()) .where( and( - eq(issues.id, issueId), - eq(issues.status, "in_progress"), - isNotNull(issues.checkoutRestoreStatus), - // `x IN (NULL)` is NULL rather than true, so a row with both lock columns - // already cleared correctly matches NOT EXISTS and is restored. - sql`not exists ( - select 1 from ${heartbeatRuns} - where ${heartbeatRuns.id} in (${issues.checkoutRunId}, ${issues.executionRunId}) - and ${heartbeatRuns.status} not in ${sql.raw( - `(${TERMINAL_HEARTBEAT_RUN_STATUS_VALUES.map((s) => `'${s}'`).join(", ")})`, - )} - )`, + eq(issues.id, target.issueId), + eq(issues.companyId, target.companyId), + restorableCheckoutPromotion, ), ) .returning({ id: issues.id }); @@ -78,6 +99,40 @@ export async function restoreCheckoutPromotedStatus( return restored.length > 0; } +/** + * Batch form of {@link restoreCheckoutPromotedStatus}, for callers releasing a + * run's lock across every sibling issue at once. + * + * One statement rather than one per issue: the primary finalizer deliberately + * clears its lock columns set-based so cleanup scales with the orphan count + * without N round-trips, and restoration has to hold that same property or it + * silently becomes the slow half of the same transaction. + * + * @returns the ids actually restored. + */ +export async function restoreCheckoutPromotedStatuses( + dbOrTx: { + update: (table: typeof issues) => any; + }, + target: { issueIds: readonly string[]; companyId: string }, +): Promise { + if (target.issueIds.length === 0) return []; + + const restored = await dbOrTx + .update(issues) + .set(restoreCheckoutPromotionSet()) + .where( + and( + inArray(issues.id, [...target.issueIds]), + eq(issues.companyId, target.companyId), + restorableCheckoutPromotion, + ), + ) + .returning({ id: issues.id }); + + return restored.map((row: { id: string }) => row.id); +} + /** * The value `checkout` should write to `checkout_restore_status` when it promotes * a row to `in_progress`. diff --git a/server/src/services/issues.ts b/server/src/services/issues.ts index 953f30063b84..f56d6cb11460 100644 --- a/server/src/services/issues.ts +++ b/server/src/services/issues.ts @@ -5754,7 +5754,7 @@ export function issueService(db: Db) { sql`select ${issues.id} from ${issues} where ${issues.id} = ${issueId} for update`, ); const issue = await tx - .select({ executionRunId: issues.executionRunId }) + .select({ executionRunId: issues.executionRunId, companyId: issues.companyId }) .from(issues) .where(eq(issues.id, issueId)) .then((rows) => rows[0] ?? null); @@ -5787,7 +5787,9 @@ export function issueService(db: Db) { .returning({ id: issues.id }) .then((rows) => rows[0] ?? null); - if (updated) await restoreCheckoutPromotedStatus(tx, issueId); + if (updated) { + await restoreCheckoutPromotedStatus(tx, { issueId, companyId: issue.companyId }); + } return Boolean(updated); }); @@ -5878,7 +5880,11 @@ export function issueService(db: Db) { sql`select ${issues.id} from ${issues} where ${issues.id} = ${issueId} for update`, ); const issue = await tx - .select({ checkoutRunId: issues.checkoutRunId, executionRunId: issues.executionRunId }) + .select({ + checkoutRunId: issues.checkoutRunId, + executionRunId: issues.executionRunId, + companyId: issues.companyId, + }) .from(issues) .where(eq(issues.id, issueId)) .then((rows) => rows[0] ?? null); @@ -5927,7 +5933,9 @@ export function issueService(db: Db) { .returning({ id: issues.id }) .then((rows) => rows[0] ?? null); - if (updated) await restoreCheckoutPromotedStatus(tx, issueId); + if (updated) { + await restoreCheckoutPromotedStatus(tx, { issueId, companyId: issue.companyId }); + } return Boolean(updated); }); @@ -9339,6 +9347,11 @@ export function issueService(db: Db) { executionAgentNameKey: null, executionLockedAt: now, status: "in_progress", + // This promotion is a checkout like any other, so it must record the + // status it displaced. Without the marker `restoreCheckoutPromotedStatus` + // can never undo it and the row strands in `in_progress` forever — + // the exact BLO-20649 leak, reached through the adoption path. + checkoutRestoreStatus: checkoutRestoreStatusExpression, updatedAt: now, }; if (current.status !== "in_progress") { @@ -9395,6 +9408,9 @@ export function issueService(db: Db) { // BLO-19848: see the checkout site above. executionLockedAt: now, status: "in_progress", + // Same reasoning as the adoption path above: a checkout that does + // not record what it displaced is unrestorable (BLO-20649). + checkoutRestoreStatus: checkoutRestoreStatusExpression, startedAt: now, updatedAt: now, }) diff --git a/server/src/services/recovery/service.ts b/server/src/services/recovery/service.ts index 04e40b090f0b..62b950ebccbd 100644 --- a/server/src/services/recovery/service.ts +++ b/server/src/services/recovery/service.ts @@ -2512,7 +2512,10 @@ export function recoveryService( // The run is finalized; if it never wrote a status of its own, undo the // `in_progress` its checkout wrote (BLO-20649). - await restoreCheckoutPromotedStatus(tx, input.sourceIssue.id); + await restoreCheckoutPromotedStatus(tx, { + issueId: input.sourceIssue.id, + companyId: input.run.companyId, + }); return updatedRun; });