From 4f1efb5fe3123212ee69fa97fac88678e89daba2 Mon Sep 17 00:00:00 2001 From: nickyleach <331803+nickyleach@users.noreply.github.com> Date: Thu, 10 Sep 2026 19:33:43 +0000 Subject: [PATCH] fix(server): stop the review-participant recovery release from staying locked on an unresolved identity The review-participant recovery release ran inside the issue-execution lock transaction. When responsible-user resolution returned null, the release code threw. The throw unwound that same transaction, so it undid the lock release the transaction had already run: the issue's execution and checkout references stayed on the finished run instead of clearing. Stop the release from throwing for this one case. Return a "blocked" outcome instead, the same way the release already handles a missing or non-invokable recovery agent. The lock-clearing writes now commit together with the blocked decision, and the board sees the usual stranded-issue notice instead of the release silently failing. - server/src/modules/wake-queue/application/use-cases.ts: return a blocked outcome (notice kind execution_review_participant) instead of throwing WakeQueueApplicationError when the review-participant recovery run has no responsible user. - server/src/modules/wake-queue/application/use-cases.test.ts: update the unit test for this branch to assert the blocked outcome and the stranded-issue escalation call, instead of the removed throw. - server/src/modules/wake-queue/adapters/postgres.test.ts: add an integration test that drives the real release use case against a real Postgres transaction and proves the issue's execution and checkout references clear even when identity resolution fails. Co-authored-by: Paperclip --- .../wake-queue/adapters/postgres.test.ts | 76 ++++++++++++++++++- .../wake-queue/application/use-cases.test.ts | 19 +++-- .../wake-queue/application/use-cases.ts | 25 +++--- 3 files changed, 101 insertions(+), 19 deletions(-) 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({