diff --git a/packages/shared/src/validators/issue.test.ts b/packages/shared/src/validators/issue.test.ts index 29e165a825..b5ea093007 100644 --- a/packages/shared/src/validators/issue.test.ts +++ b/packages/shared/src/validators/issue.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it } from "vitest"; import { MAX_ISSUE_REQUEST_DEPTH } from "../index.js"; import { addIssueCommentSchema, + checkoutIssueSchema, createIssueSchema, issueBlockedInboxAttentionSchema, resolveIssueRecoveryActionSchema, @@ -14,6 +15,39 @@ import { import { createAgentSchema } from "./agent.js"; describe("issue validators", () => { + it("rejects terminal issue statuses as checkout expectations", () => { + const agentId = "11111111-1111-4111-8111-111111111111"; + + expect( + checkoutIssueSchema.safeParse({ + agentId, + expectedStatuses: [ + "backlog", + "todo", + "in_progress", + "in_review", + "blocked", + ], + }).success, + ).toBe(true); + expect( + checkoutIssueSchema.safeParse({ agentId, expectedStatuses: ["done"] }) + .success, + ).toBe(false); + expect( + checkoutIssueSchema.safeParse({ + agentId, + expectedStatuses: ["cancelled"], + }).success, + ).toBe(false); + expect( + checkoutIssueSchema.safeParse({ + agentId, + expectedStatuses: ["todo", "done"], + }).success, + ).toBe(false); + }); + it("uses the same bounded unique upload ID contract for comment and update requests", () => { const id = "9af8228f-0be7-45ae-a104-6fbe0af6f1d3"; expect( diff --git a/packages/shared/src/validators/issue.ts b/packages/shared/src/validators/issue.ts index 4a333c2580..9cc54b405c 100644 --- a/packages/shared/src/validators/issue.ts +++ b/packages/shared/src/validators/issue.ts @@ -894,7 +894,13 @@ export type StalledReviewDecision = z.infer; export const checkoutIssueSchema = z.object({ agentId: z.string().guid(), - expectedStatuses: z.array(z.enum(ISSUE_STATUSES)).nonempty(), + expectedStatuses: z.array(z.enum([ + "backlog", + "todo", + "in_progress", + "in_review", + "blocked", + ])).nonempty(), }); export type CheckoutIssue = z.infer; diff --git a/server/src/__tests__/issue-stale-execution-lock-routes.test.ts b/server/src/__tests__/issue-stale-execution-lock-routes.test.ts index ed2e39ed79..aee5f64b02 100644 --- a/server/src/__tests__/issue-stale-execution-lock-routes.test.ts +++ b/server/src/__tests__/issue-stale-execution-lock-routes.test.ts @@ -486,9 +486,10 @@ describeEmbeddedPostgres("stale issue execution lock routes", () => { // was cleared by releaseIssueExecutionAndPromote, but checkoutRunId stayed // pinned to the dead run. The new agent's POST /checkout would 409 forever // without the clearCheckoutRunIfTerminal helper in svc.checkout. - const { companyId, agentId, failedRunId, currentRunId } = await seedCompanyAgentAndRuns(); + const { companyId, failedRunId } = await seedCompanyAgentAndRuns(); const issueId = randomUUID(); const otherAgentId = randomUUID(); + const currentRunId = randomUUID(); await db.insert(agents).values({ id: otherAgentId, companyId, @@ -500,6 +501,14 @@ describeEmbeddedPostgres("stale issue execution lock routes", () => { runtimeConfig: {}, permissions: {}, }); + await db.insert(heartbeatRuns).values({ + id: currentRunId, + companyId, + agentId: otherAgentId, + status: "running", + invocationSource: "manual", + startedAt: new Date(), + }); await db.insert(issues).values({ id: issueId, companyId, diff --git a/server/src/__tests__/issues-service.test.ts b/server/src/__tests__/issues-service.test.ts index 9309731a6d..833f1a748b 100644 --- a/server/src/__tests__/issues-service.test.ts +++ b/server/src/__tests__/issues-service.test.ts @@ -6076,6 +6076,229 @@ describeEmbeddedPostgres("issueService.clearExecutionRunIfTerminal", () => { }); }); + it("rejects the exact late checkout from a succeeded run without reopening the done issue", async () => { + const companyId = randomUUID(); + const agentId = randomUUID(); + const issueId = randomUUID(); + const succeededRunId = 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: succeededRunId, + companyId, + agentId, + status: "succeeded", + invocationSource: "manual", + finishedAt: new Date("2026-08-26T11:16:18.729Z"), + }); + await db.insert(issues).values({ + id: issueId, + companyId, + title: "Completed issue with a late tool call", + status: "done", + priority: "critical", + assigneeAgentId: agentId, + checkoutRunId: null, + executionRunId: null, + executionAgentNameKey: null, + executionLockedAt: null, + completedAt: new Date("2026-08-26T11:16:18.000Z"), + }); + + await expect(svc.checkout(issueId, agentId, ["done"], succeededRunId)) + .rejects.toMatchObject({ status: 422 }); + + const row = await db + .select({ + status: issues.status, + checkoutRunId: issues.checkoutRunId, + executionRunId: issues.executionRunId, + executionAgentNameKey: issues.executionAgentNameKey, + executionLockedAt: issues.executionLockedAt, + }) + .from(issues) + .where(eq(issues.id, issueId)) + .then((rows) => rows[0]); + expect(row).toEqual({ + status: "done", + checkoutRunId: null, + executionRunId: null, + executionAgentNameKey: null, + executionLockedAt: null, + }); + }); + + it.each([ + ["succeeded", "succeeded"], + [null, "missing"], + ])("rejects a %s checkout run before acquiring an active issue lock", async (runStatus, expectedRunStatus) => { + const companyId = randomUUID(); + const agentId = randomUUID(); + const issueId = randomUUID(); + const checkoutRunId = 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: {}, + }); + if (runStatus) { + await db.insert(heartbeatRuns).values({ + id: checkoutRunId, + companyId, + agentId, + status: runStatus, + invocationSource: "manual", + finishedAt: new Date("2026-08-26T11:16:18.729Z"), + }); + } + await db.insert(issues).values({ + id: issueId, + companyId, + title: "Active issue with an invalid checkout run", + status: "todo", + priority: "critical", + assigneeAgentId: agentId, + }); + + await expect(svc.checkout(issueId, agentId, ["todo"], checkoutRunId)) + .rejects.toMatchObject({ + status: 409, + details: { + code: "checkout_run_not_active", + checkoutRunId, + runStatus: expectedRunStatus, + }, + }); + + const row = await db + .select({ + status: issues.status, + checkoutRunId: issues.checkoutRunId, + executionRunId: issues.executionRunId, + }) + .from(issues) + .where(eq(issues.id, issueId)) + .then((rows) => rows[0]); + expect(row).toEqual({ + status: "todo", + checkoutRunId: null, + executionRunId: null, + }); + }); + + it("rejects checkout when the run becomes terminal before the issue mutation", async () => { + const companyId = randomUUID(); + const agentId = randomUUID(); + const issueId = randomUUID(); + const checkoutRunId = 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: checkoutRunId, + companyId, + agentId, + status: "running", + invocationSource: "manual", + startedAt: new Date("2026-08-26T11:16:18.000Z"), + }); + await db.insert(issues).values({ + id: issueId, + companyId, + title: "Checkout racing run completion", + status: "todo", + priority: "critical", + assigneeAgentId: agentId, + }); + + const terminalWriteReady = deferred(); + const allowTerminalCommit = deferred(); + const terminalWrite = db.transaction(async (tx) => { + await tx + .update(heartbeatRuns) + .set({ + status: "succeeded", + finishedAt: new Date("2026-08-26T11:16:19.000Z"), + }) + .where(eq(heartbeatRuns.id, checkoutRunId)); + terminalWriteReady.resolve(); + await allowTerminalCommit.promise; + }); + await terminalWriteReady.promise; + + const checkout = svc.checkout(issueId, agentId, ["todo"], checkoutRunId); + const checkoutAssertion = expect(checkout).rejects.toMatchObject({ + status: 409, + details: { + code: "checkout_run_not_active", + checkoutRunId, + runStatus: "succeeded", + }, + }); + await new Promise((resolve) => setImmediate(resolve)); + allowTerminalCommit.resolve(); + await terminalWrite; + await checkoutAssertion; + const row = await db + .select({ + status: issues.status, + checkoutRunId: issues.checkoutRunId, + executionRunId: issues.executionRunId, + }) + .from(issues) + .where(eq(issues.id, issueId)) + .then((rows) => rows[0]); + expect(row).toEqual({ + status: "todo", + checkoutRunId: null, + executionRunId: 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 @@ -7094,6 +7317,42 @@ describeEmbeddedPostgres("issueService.assertCheckoutOwner stale checkout adopti }); }); + it("serializes concurrent checkout and unowned ownership assertion without a deadlock", async () => { + const seeded = await seedOwnershipIssue({ checkoutStatus: "failed" }); + await db + .update(issues) + .set({ + checkoutRunId: null, + executionRunId: null, + executionLockedAt: null, + executionAgentNameKey: null, + }) + .where(eq(issues.id, seeded.issueId)); + + const [checkedOut, ownership] = await Promise.all([ + svc.checkout(seeded.issueId, seeded.actorAgentId, ["in_progress"], seeded.actorRunId), + svc.assertCheckoutOwner(seeded.issueId, seeded.actorAgentId, seeded.actorRunId), + ]); + + expect(checkedOut.checkoutRunId).toBe(seeded.actorRunId); + expect(checkedOut.executionRunId).toBe(seeded.actorRunId); + expect(ownership.checkoutRunId).toBe(seeded.actorRunId); + expect(ownership.executionRunId).toBe(seeded.actorRunId); + + const row = await db + .select({ + checkoutRunId: issues.checkoutRunId, + executionRunId: issues.executionRunId, + }) + .from(issues) + .where(eq(issues.id, seeded.issueId)) + .then((rows) => rows[0]); + expect(row).toEqual({ + checkoutRunId: seeded.actorRunId, + executionRunId: seeded.actorRunId, + }); + }); + }); describeEmbeddedPostgres("issueService.addComment createdByRunId", () => { diff --git a/server/src/services/issues.ts b/server/src/services/issues.ts index b3d297bdf4..55cc9957fc 100644 --- a/server/src/services/issues.ts +++ b/server/src/services/issues.ts @@ -7365,6 +7365,50 @@ export function issueService(db: Db) { return heartbeatRunIsTerminalOrMissing(dbOrTx, runId); } + async function withActiveCheckoutRun(input: { + issueId: string; + companyId: string; + agentId: string; + checkoutRunId: string; + operation: (tx: DbTransaction) => Promise; + }): Promise { + return db.transaction(async (tx) => { + await tx.execute( + sql`select ${issues.id} from ${issues} where ${issues.id} = ${input.issueId} for update`, + ); + await tx.execute( + sql`select ${heartbeatRuns.id} from ${heartbeatRuns} where ${heartbeatRuns.id} = ${input.checkoutRunId} for update`, + ); + const checkoutRun = await tx + .select({ + status: heartbeatRuns.status, + companyId: heartbeatRuns.companyId, + agentId: heartbeatRuns.agentId, + }) + .from(heartbeatRuns) + .where(eq(heartbeatRuns.id, input.checkoutRunId)) + .then((rows) => rows[0] ?? null); + const isOwnedRun = checkoutRun + && checkoutRun.companyId === input.companyId + && checkoutRun.agentId === input.agentId; + if (!isOwnedRun) { + throw conflict("Issue checkout requires an active heartbeat run", { + code: "checkout_run_not_active", + checkoutRunId: input.checkoutRunId, + runStatus: "missing", + }); + } + if (TERMINAL_HEARTBEAT_RUN_STATUSES.has(checkoutRun.status)) { + throw conflict("Issue checkout requires an active heartbeat run", { + code: "checkout_run_not_active", + checkoutRunId: input.checkoutRunId, + runStatus: checkoutRun.status, + }); + } + return input.operation(tx); + }); + } + async function adoptStaleCheckoutRun(input: { issueId: string; actorAgentId: string; @@ -7474,6 +7518,10 @@ export function issueService(db: Db) { actorRunId: string; }) { return db.transaction(async (tx) => { + // Keep the issue -> heartbeat lock order aligned with checkout and stale-lock cleanup. + await tx.execute( + sql`select ${issues.id} from ${issues} where ${issues.id} = ${input.issueId} for update`, + ); await tx.execute( sql`select ${heartbeatRuns.id} from ${heartbeatRuns} where ${heartbeatRuns.id} = ${input.actorRunId} for update`, ); @@ -11222,6 +11270,16 @@ export function issueService(db: Db) { expectedStatuses: string[], checkoutRunId: string | null, ) => { + const terminalExpectedStatuses = expectedStatuses.filter( + (status) => status === "done" || status === "cancelled", + ); + if (terminalExpectedStatuses.length > 0) { + throw unprocessable( + "Issue checkout cannot expect terminal issue statuses", + { terminalExpectedStatuses }, + ); + } + const issueCompany = await db .select({ companyId: issues.companyId }) .from(issues) @@ -11258,6 +11316,16 @@ export function issueService(db: Db) { }); } + if (checkoutRunId) { + await withActiveCheckoutRun({ + issueId: id, + companyId: issueCompany.companyId, + agentId, + checkoutRunId, + operation: async () => undefined, + }); + } + await clearExecutionRunIfTerminal(id); await clearCheckoutRunIfTerminal(id); @@ -11300,7 +11368,7 @@ export function issueService(db: Db) { eq(issues.executionRunId, checkoutRunId), ) : isNull(issues.executionRunId); - const updated = await db + const updateIssue = (dbOrTx: Db | DbTransaction) => dbOrTx .update(issues) .set({ assigneeAgentId: agentId, @@ -11321,6 +11389,15 @@ export function issueService(db: Db) { ) .returning() .then((rows) => rows[0] ?? null); + const updated = checkoutRunId + ? await withActiveCheckoutRun({ + issueId: id, + companyId: issueCompany.companyId, + agentId, + checkoutRunId, + operation: updateIssue, + }) + : await updateIssue(db); if (updated) { const [enriched] = await withIssueLabels(db, [updated]);