diff --git a/server/src/modules/wake-queue/adapters/postgres.test.ts b/server/src/modules/wake-queue/adapters/postgres.test.ts index c7bb2ca43a..8c86764371 100644 --- a/server/src/modules/wake-queue/adapters/postgres.test.ts +++ b/server/src/modules/wake-queue/adapters/postgres.test.ts @@ -21,7 +21,8 @@ import { createWakeAdmissionWriter, } from "./postgres.js"; import type { WakeQueuePostgresAdapterDeps } from "./postgres.js"; -import type { TransactionScope } from "../application/ports.js"; +import { createReleaseIssueExecution } from "../application/use-cases.js"; +import type { RecoveryEscalationPort, TransactionScope } from "../application/ports.js"; // Proves the atomicity and company-scope properties the security review // requires: every mutation names `companyId` in its own SQL `WHERE` clause, @@ -478,6 +479,79 @@ describeEmbeddedPostgres("wake-queue postgres adapter", () => { }); }); + // Regression test for the Greptile P1 finding on PR #13156: an unresolved + // responsible user must not leave the review-participant recovery issue + // locked. This drives the real application-layer release use case (not a + // hand-written `fn`) against a real transaction, so a throw inside the + // lock would roll back the clearing of `executionRunId`/`checkoutRunId` + // the same way it did before the fix. + it("clears the execution lock and blocks the issue, instead of leaving it locked, when the review-participant recovery run has no responsible user", async () => { + const companyId = await seedCompany(); + const finishingAgentId = await seedAgent({ companyId, name: "Finishing Agent" }); + const issueId = await seedIssue({ companyId, assigneeAgentId: null, status: "in_review" }); + // A finishing run status other than the legacy-reconciliation set + // (failed, timed_out, interrupted, cancelled) reaches the module's own + // drain logic, so this call runs. + const finishingRunId = await seedRun({ + companyId, + agentId: finishingAgentId, + contextSnapshot: { issueId, wakeReason: "execution_review_requested" }, + status: "succeeded", + }); + await db + .update(issues) + .set({ + executionRunId: finishingRunId, + checkoutRunId: finishingRunId, + executionState: { + status: "pending", + currentStageId: randomUUID(), + currentStageIndex: 0, + currentStageType: "review", + currentParticipant: { type: "agent", agentId: finishingAgentId }, + returnAssignee: null, + completedStageIds: [], + lastDecisionId: null, + lastDecisionOutcome: null, + }, + }) + .where(eq(issues.id, issueId)); + + const issueLock = createPostgresWakeQueueAdapter(db, { + ...stubDeps, + // The condition under test: identity resolution finds no responsible user. + resolveResponsibleUserId: async () => null, + }); + const escalatedInputs: Array<{ issueId: string; noticeKind: string }> = []; + const recovery: RecoveryEscalationPort = { + escalateStrandedAssignedIssue: async (input) => { + escalatedInputs.push({ issueId: input.issue.id, noticeKind: input.noticeKind }); + }, + escalateStrandedRecoveryIssueInPlace: async () => {}, + }; + const releaseIssueExecution = createReleaseIssueExecution({ issueLock, recovery }); + + const result = await releaseIssueExecution({ companyId, runId: finishingRunId, now: new Date() }); + + expect(result.outcome.kind).toBe("blocked"); + expect(result.outcome.kind === "blocked" && result.outcome.noticeKind).toBe("execution_review_participant"); + + // The lock must clear even though identity resolution failed: this is + // the bug this test guards against. + const issueRow = (await db.select().from(issues).where(eq(issues.id, issueId)))[0]; + expect(issueRow?.executionRunId).toBeNull(); + expect(issueRow?.checkoutRunId).toBeNull(); + + expect(escalatedInputs).toHaveLength(1); + expect(escalatedInputs[0]?.issueId).toBe(issueId); + expect(escalatedInputs[0]?.noticeKind).toBe("execution_review_participant"); + + // No review-participant recovery run was queued for the unresolved identity. + const runs = await db.select().from(heartbeatRuns).where(eq(heartbeatRuns.companyId, companyId)); + expect(runs).toHaveLength(1); + expect(runs[0]?.id).toBe(finishingRunId); + }); + it("locks the context issue and every sibling issue in id order, and two concurrent releases do not deadlock", async () => { const companyId = await seedCompany(); const agentId = await seedAgent({ companyId }); diff --git a/server/src/modules/wake-queue/application/use-cases.test.ts b/server/src/modules/wake-queue/application/use-cases.test.ts index 6729c0f0d4..57e6f087f2 100644 --- a/server/src/modules/wake-queue/application/use-cases.test.ts +++ b/server/src/modules/wake-queue/application/use-cases.test.ts @@ -559,19 +559,22 @@ describe("releaseIssueExecution", () => { expect((queueCall.contextSnapshot as Record).executionIdentityCause).toBe("company_default"); }); - it("throws WakeQueueApplicationError with code responsible_user_unresolved for a review-participant recovery run, without queuing it", async () => { + it("blocks the issue instead of throwing when the responsible user cannot resolve for a review-participant recovery run, so the release still commits", async () => { const transaction = createFakeTransaction(); const host = createFakeHost({ resolveResponsibleUserId: vi.fn(async () => null) }); const issueLock = createReviewParticipantIssueLock(host, transaction); - const releaseIssueExecution = createReleaseIssueExecution({ issueLock, recovery: createFakeRecovery() }); + const recovery = createFakeRecovery(); + const releaseIssueExecution = createReleaseIssueExecution({ issueLock, recovery }); - await expect( - releaseIssueExecution({ companyId: "company-1", runId: "run-1", now: new Date() }), - ).rejects.toMatchObject({ - constructor: WakeQueueApplicationError, - code: "responsible_user_unresolved", - }); + const result = await releaseIssueExecution({ companyId: "company-1", runId: "run-1", now: new Date() }); + + // A throw here would unwind the lock transaction and leave the issue's + // execution and checkout references stuck on the finished run. The + // release must commit and the issue must end up "blocked" instead. + expect(result.outcome.kind).toBe("blocked"); + expect(result.outcome.kind === "blocked" && result.outcome.noticeKind).toBe("execution_review_participant"); expect(transaction.queueReviewParticipantRecoveryRun).not.toHaveBeenCalled(); + expect(recovery.escalateStrandedAssignedIssue).toHaveBeenCalledTimes(1); }); it("escalates through the recovery port for a blocked outcome, after the transaction resolves", async () => { diff --git a/server/src/modules/wake-queue/application/use-cases.ts b/server/src/modules/wake-queue/application/use-cases.ts index 427b9f95f0..73712070ad 100644 --- a/server/src/modules/wake-queue/application/use-cases.ts +++ b/server/src/modules/wake-queue/application/use-cases.ts @@ -588,17 +588,22 @@ async function runReleaseRecoveryTail( existingRunResponsibleUserId: run.responsibleUserId, }); if (!reviewParticipantResponsibleUserId) { - throw new WakeQueueApplicationError( - "responsible_user_unresolved", - "Unable to resolve responsible user for review-participant recovery heartbeat run", - { - runId: run.id, - agentId: recoveryAgent.id, - companyId: issue.companyId, - issueId: issue.id, - wakeReason: EXECUTION_REVIEW_PARTICIPANT_RECOVERY_RETRY_REASON, + // Do not throw here: a throw would unwind the surrounding issue-execution + // lock transaction and undo the release it already ran, leaving the + // issue's execution and checkout references stuck on this finished run. + // Treat an unresolved identity the same way the sibling checks just + // above treat a missing or non-invokable recovery agent: block the + // issue in the same committed transaction that clears the lock, so a + // human can assign a responsible user or resolve the issue directly. + return { + outcome: { + kind: "blocked", + issue, + previousStatus: statusForBlock(issue), + noticeKind: "execution_review_participant", }, - ); + postCommitEffects, + }; } const queuedRun = await transaction.queueReviewParticipantRecoveryRun({