From 9e81067678e2bb616290d2994ec881c1206cf11a Mon Sep 17 00:00:00 2001 From: alcylu Date: Fri, 12 Jun 2026 13:58:04 +0900 Subject: [PATCH] fix: clear stale executionRunId on release, reassignment, and checkout (#2482) 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. > - Issues are the unit of agent assignment; each assignment queues a heartbeat run, and the agent claims ownership via a `checkout()` that sets `checkoutRunId` and `executionRunId` on the issue row. > - When a queued run never starts (crash, deploy, lost heartbeat) or a different run picks up the work, the issue is left with a stale `executionRunId` pointing at a terminal/missing run. > - The next checkout attempt fails with "Issue checkout conflict" because the fast-path `UPDATE` requires `executionRunId` to be null or equal to the requester's run id, so the row is permanently locked until an admin clears the column by hand. > - This pull request closes that lifecycle gap in three places — `release()` and `update()` clear the execution lock fields alongside the existing `checkoutRunId` clear, and `checkout()` gains a guarded stale-`executionRunId` adoption path that mirrors the existing `adoptStaleCheckoutRun` pattern. > - The benefit is that assignment-triggered issues self-heal after a lost run instead of paging an admin to unlock them, while the adoption path keeps the caller's `expectedStatuses` guard, preserves any pending `assigneeUserId`, and preserves the original `startedAt` for issues already `in_progress`. ## Linked Issues or Issue Description - Closes #759 - Closes #1015 - Closes #1276 - Closes #1298 - Closes #2265 - Closes #2661 - Closes #2964 - Closes #3559 - Closes #4033 - Closes #4131 ## What Changed - `server/src/services/issues.ts` — `release()` now clears `executionRunId`, `executionAgentNameKey`, and `executionLockedAt` alongside `checkoutRunId`. - `server/src/services/issues.ts` — `update()` clears the same execution-lock fields on status change (away from `in_progress`) and on assignee change. - `server/src/services/issues.ts` — `checkout()` gains a stale `executionRunId` adoption block that runs only when the row's `executionRunId` points at a terminal/missing heartbeat run, the caller's `expectedStatuses` still hold, and the requester is either the existing assignee or the assignee is null. The `SET` clause preserves `assigneeUserId` and only resets `startedAt` when the issue was not already `in_progress` (matches `adoptStaleCheckoutRun` semantics). - `server/src/__tests__/issues-service.test.ts` — two regression tests covering the new adoption guards: (1) checkout refuses to promote a `done` issue when `done` is not in `expectedStatuses`, even with a lingering `executionRunId` pointer; (2) checkout adoption of a stale `checkoutRunId` preserves the issue's `assigneeUserId`. ## Verification - `vitest run src/__tests__/issues-service.test.ts` — 75/75 tests pass, including the two new regression tests. - `tsc --noEmit` clean. - Manual repro of the original stuck-lock case: queue a run, mark the heartbeat run terminal without releasing the issue, attempt a new checkout — the adoption path now succeeds with the caller's `expectedStatuses` guard intact instead of returning a checkout conflict. ## Risks - Low risk. The `release()` and `update()` changes are additive field clears alongside the existing `checkoutRunId` clear and follow the same conditions. The `checkout()` adoption block is gated by the same status / assignee / expected-statuses constraints as the fast-path `UPDATE` and only fires when the prior run is verifiably terminal via `isTerminalOrMissingHeartbeatRun()`. No migration. No public API change. ## Model Used - Claude Opus 4.7 (`claude-opus-4-7`), extended-thinking mode, tool-use enabled (file reads, edits, shell, gh CLI). Used to address review feedback on the original commit by Allen Lu (`alcylu`); follow-up fix commit preserves the `expectedStatuses` guard, `assigneeUserId`, and `startedAt` and adds regression tests. ## 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 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 — server-only) - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge ## Cross-references and status (maintainer) - Closes #759 - Closes #1015 - Closes #1276 - Closes #1298 - Closes #2265 - Closes #2661 - Closes #2964 - Closes #3559 - Closes #4033 - Closes #4131 --------- Co-authored-by: Paperclip Co-authored-by: Devin Foley --- server/src/__tests__/issues-service.test.ts | 173 ++++++++++++++++++++ server/src/services/issues.ts | 46 +++++- 2 files changed, 217 insertions(+), 2 deletions(-) diff --git a/server/src/__tests__/issues-service.test.ts b/server/src/__tests__/issues-service.test.ts index ba6efe59..a7a5fae6 100644 --- a/server/src/__tests__/issues-service.test.ts +++ b/server/src/__tests__/issues-service.test.ts @@ -4079,6 +4079,179 @@ describeEmbeddedPostgres("issueService.clearExecutionRunIfTerminal", () => { executionRunId: successorRunId, }); }); + + it("checkout refuses to promote a 'done' issue when 'done' is not in expectedStatuses, even with a lingering executionRunId pointer", async () => { + // Regression for PR #2482 checkout-adoption review finding: the original + // patch's stale-executionRunId adoption SQL set `status: 'in_progress'` + // unconditionally, bypassing the caller's expectedStatuses guard. With the + // guard restored, attempting to take over a 'done' issue with + // expectedStatuses=['todo'] must fail and leave the row untouched. + const companyId = randomUUID(); + const agentId = randomUUID(); + const issueId = randomUUID(); + const failedRunId = randomUUID(); + const successorRunId = randomUUID(); + + await db.insert(companies).values({ + id: companyId, + name: "Paperclip", + issuePrefix: `T${companyId.replace(/-/g, "").slice(0, 6).toUpperCase()}`, + requireBoardApprovalForNewAgents: false, + }); + await db.insert(agents).values({ + id: agentId, + companyId, + name: "CodexCoder", + role: "engineer", + status: "active", + adapterType: "codex_local", + adapterConfig: {}, + runtimeConfig: {}, + permissions: {}, + }); + await db.insert(heartbeatRuns).values([ + { + id: failedRunId, + companyId, + agentId, + status: "failed", + invocationSource: "manual", + finishedAt: new Date("2026-06-10T10:05:00.000Z"), + }, + { + id: successorRunId, + companyId, + agentId, + status: "running", + invocationSource: "manual", + startedAt: new Date("2026-06-10T10:07:00.000Z"), + }, + ]); + await db.insert(issues).values({ + id: issueId, + companyId, + title: "Stale lock on done issue", + status: "done", + priority: "medium", + assigneeAgentId: agentId, + executionRunId: failedRunId, + executionAgentNameKey: "codexcoder", + executionLockedAt: new Date("2026-06-10T10:00:00.000Z"), + completedAt: new Date("2026-06-10T10:01:00.000Z"), + }); + + await expect(svc.checkout(issueId, agentId, ["todo"], successorRunId)) + .rejects.toMatchObject({ status: 409 }); + + const row = await db + .select({ + status: issues.status, + assigneeAgentId: issues.assigneeAgentId, + checkoutRunId: issues.checkoutRunId, + }) + .from(issues) + .where(eq(issues.id, issueId)) + .then((rows) => rows[0]); + expect(row).toMatchObject({ + status: "done", + assigneeAgentId: agentId, + checkoutRunId: null, + }); + }); + + it("checkout adoption of a stale checkoutRunId preserves the issue's assigneeUserId", async () => { + // Regression for PR #2482 checkout-adoption review finding: any adoption + // helper that re-locks an existing in_progress issue (e.g. when the prior + // checkout/execution run is terminal) must not strip the row's + // assigneeUserId. We exercise this via the adoptStaleCheckoutRun path, + // which fires when checkoutRunId points at a terminal run while + // executionRunId still points at a different, non-terminal run. + const companyId = randomUUID(); + const agentId = randomUUID(); + const userId = randomUUID(); + const issueId = randomUUID(); + const failedCheckoutRunId = randomUUID(); + const queuedExecutionRunId = randomUUID(); + const successorRunId = randomUUID(); + + await db.insert(companies).values({ + id: companyId, + name: "Paperclip", + issuePrefix: `T${companyId.replace(/-/g, "").slice(0, 6).toUpperCase()}`, + requireBoardApprovalForNewAgents: false, + }); + await db.insert(agents).values({ + id: agentId, + companyId, + name: "CodexCoder", + role: "engineer", + status: "active", + adapterType: "codex_local", + adapterConfig: {}, + runtimeConfig: {}, + permissions: {}, + }); + await db.insert(heartbeatRuns).values([ + { + id: failedCheckoutRunId, + companyId, + agentId, + status: "failed", + invocationSource: "manual", + finishedAt: new Date("2026-06-10T10:05:00.000Z"), + }, + { + id: queuedExecutionRunId, + companyId, + agentId, + status: "queued", + invocationSource: "manual", + }, + { + id: successorRunId, + companyId, + agentId, + status: "running", + invocationSource: "manual", + startedAt: new Date("2026-06-10T10:07:00.000Z"), + }, + ]); + await db.insert(issues).values({ + id: issueId, + companyId, + title: "Stale checkout lock with user co-assignee", + status: "in_progress", + priority: "medium", + assigneeAgentId: agentId, + assigneeUserId: userId, + checkoutRunId: failedCheckoutRunId, + executionRunId: queuedExecutionRunId, + executionAgentNameKey: "codexcoder", + executionLockedAt: new Date("2026-06-10T10:00:00.000Z"), + }); + + const result = await svc.checkout(issueId, agentId, ["todo", "in_progress"], successorRunId); + expect(result).toBeTruthy(); + + const row = await db + .select({ + status: issues.status, + assigneeAgentId: issues.assigneeAgentId, + assigneeUserId: issues.assigneeUserId, + checkoutRunId: issues.checkoutRunId, + executionRunId: issues.executionRunId, + }) + .from(issues) + .where(eq(issues.id, issueId)) + .then((rows) => rows[0]); + expect(row).toMatchObject({ + status: "in_progress", + assigneeAgentId: agentId, + assigneeUserId: userId, + checkoutRunId: successorRunId, + executionRunId: successorRunId, + }); + }); }); describeEmbeddedPostgres("accepted plan decomposition", () => { diff --git a/server/src/services/issues.ts b/server/src/services/issues.ts index f77744d2..93e72ddb 100644 --- a/server/src/services/issues.ts +++ b/server/src/services/issues.ts @@ -5098,7 +5098,6 @@ export function issueService(db: Db) { } if (issueData.status && issueData.status !== "in_progress") { patch.checkoutRunId = null; - // Fix B: also clear the execution lock when leaving in_progress patch.executionRunId = null; patch.executionAgentNameKey = null; patch.executionLockedAt = null; @@ -5108,7 +5107,6 @@ export function issueService(db: Db) { (issueData.assigneeUserId !== undefined && issueData.assigneeUserId !== existing.assigneeUserId) ) { patch.checkoutRunId = null; - // Fix B: clear execution lock on reassignment, matching checkoutRunId clear patch.executionRunId = null; patch.executionAgentNameKey = null; patch.executionLockedAt = null; @@ -5496,6 +5494,50 @@ export function issueService(db: Db) { } } + // Adopt stale executionRunId — if the execution lock points to a terminal/missing run, clear it and proceed. + // Only adopts when the caller's expectedStatuses guard still holds; preserves any existing assigneeUserId + // and preserves the original startedAt when the issue is already in_progress. + if ( + checkoutRunId && + current.executionRunId && + current.executionRunId !== checkoutRunId && + (current.assigneeAgentId === agentId || current.assigneeAgentId == null) + ) { + const stale = await isTerminalOrMissingHeartbeatRun(current.executionRunId); + if (stale) { + const now = new Date(); + const adoptionSet: Record = { + assigneeAgentId: agentId, + checkoutRunId, + executionRunId: checkoutRunId, + executionAgentNameKey: null, + executionLockedAt: now, + status: "in_progress", + updatedAt: now, + }; + if (current.status !== "in_progress") { + adoptionSet.startedAt = now; + } + const adopted = await db + .update(issues) + .set(adoptionSet) + .where( + and( + eq(issues.id, id), + inArray(issues.status, expectedStatuses), + eq(issues.executionRunId, current.executionRunId), + or(isNull(issues.assigneeAgentId), eq(issues.assigneeAgentId, agentId)), + ), + ) + .returning() + .then((rows) => rows[0] ?? null); + if (adopted) { + const [enriched] = await withIssueLabels(db, [adopted]); + return enriched; + } + } + } + // If this run already owns it and it's in_progress, return it (no self-409) if ( current.assigneeAgentId === agentId &&