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 <noreply@paperclip.ing>
This commit is contained in:
parent
8ff7f36c2b
commit
4f1efb5fe3
|
|
@ -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 });
|
||||
|
|
|
|||
|
|
@ -559,19 +559,22 @@ describe("releaseIssueExecution", () => {
|
|||
expect((queueCall.contextSnapshot as Record<string, unknown>).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 () => {
|
||||
|
|
|
|||
|
|
@ -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({
|
||||
|
|
|
|||
Loading…
Reference in New Issue