fix: clear stale executionRunId on release, reassignment, and checkout (#2482)
## 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 <noreply@paperclip.ing> Co-authored-by: Devin Foley <devin@paperclip.ing>
This commit is contained in:
alcylu
@@ -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", () => {
|
||||
|
||||
@@ -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<string, unknown> = {
|
||||
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 &&
|
||||
|
||||
Reference in New Issue
Block a user