diff --git a/server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts b/server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts index ad6629bee812..e8c524d56bd5 100644 --- a/server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts +++ b/server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts @@ -4088,6 +4088,76 @@ describe("agent issue mutation checkout ownership", () => { expect(mockIssueService.update).not.toHaveBeenCalled(); }); + // BLO-20385: the patch shape mandates `blockedByIssueIds: []` and that empty + // array is applied, so admitting it on an issue with live blockers deletes + // dependency edges the actor could not otherwise remove. Probed in production + // on BLO-18946: a 200 silently dropped a live edge to a still-blocked issue. + it.each(commentGrantMutationDenialCases)( + "refuses the delegate recovery patch when blockers are unresolved, for a %s comment grant holder", + async (_kind, agentRows, issueOverrides) => { + useProductionIssueAuthorization(agentRows); + mockIssueService.getById.mockResolvedValue( + makeIssue({ status: "blocked", assigneeAgentId: ownerAgentId, ...issueOverrides }), + ); + const liveBlockerId = "11111111-1111-4111-8111-111111111111"; + mockIssueService.getDependencyReadiness.mockResolvedValue({ + issueId, + blockerIssueIds: [liveBlockerId], + unresolvedBlockerCount: 1, + unresolvedBlockerIssueIds: [liveBlockerId], + pendingFinalizeBlockerIssueIds: [], + allBlockersDone: false, + isDependencyReady: false, + }); + + const res = await request(await createApp(peerActor())) + .patch(`/api/issues/${issueId}`) + .send({ status: "todo", blockedByIssueIds: [] }); + + expect(res.status, JSON.stringify(res.body)).toBe(409); + expect(res.body.details).toMatchObject({ + reason: "delegate_recovery_unresolved_blockers", + unresolvedBlockerCount: 1, + unresolvedBlockerIssueIds: [liveBlockerId], + }); + // The edge must survive: no write may reach the service at all. + expect(mockIssueService.update).not.toHaveBeenCalled(); + }, + ); + + it.each(commentGrantMutationDenialCases)( + "still unparks past stale terminal blocker edges for a %s comment grant holder", + async (_kind, agentRows, issueOverrides) => { + useProductionIssueAuthorization(agentRows); + const stored = makeIssue({ status: "blocked", assigneeAgentId: ownerAgentId, ...issueOverrides }); + mockIssueService.getById.mockResolvedValue(stored); + mockIssueService.update.mockImplementation(async (_id: string, patch: Record) => ({ + ...stored, + ...patch, + })); + // Edges exist but every one is terminal: clearing these is the whole point + // of the recovery patch and must keep working. + mockIssueService.getDependencyReadiness.mockResolvedValue({ + issueId, + blockerIssueIds: ["22222222-2222-4222-8222-222222222222"], + unresolvedBlockerCount: 0, + unresolvedBlockerIssueIds: [], + pendingFinalizeBlockerIssueIds: [], + allBlockersDone: true, + isDependencyReady: true, + }); + + const res = await request(await createApp(peerActor())) + .patch(`/api/issues/${issueId}`) + .send({ status: "todo", blockedByIssueIds: [] }); + + expect(res.status, JSON.stringify(res.body)).toBe(200); + const [, patch] = mockIssueService.update.mock.calls.at(-1) as [string, Record]; + expect(patch).toMatchObject({ status: "todo", expectedCurrentStatus: "blocked" }); + expect(patch.blockedByIssueIds).toEqual([]); + }, + ); + it("surfaces 409 when the issue stops being blocked before the delegate recovery write lands", async () => { useProductionIssueAuthorization([ makeAgent(peerAgentId), diff --git a/server/src/__tests__/issues-service.test.ts b/server/src/__tests__/issues-service.test.ts index f80b7cb0eb62..174fff97cf26 100644 --- a/server/src/__tests__/issues-service.test.ts +++ b/server/src/__tests__/issues-service.test.ts @@ -10519,3 +10519,411 @@ describeEmbeddedPostgres("issueService.update expectedCurrentStatus (BLO-18797)" }, ); }); + +describeEmbeddedPostgres( + "issueService.update requireDependencyReadyBeforeClearingBlockers (BLO-20385)", + () => { + let db!: ReturnType; + let svc!: ReturnType; + let tempDb: Awaited> | null = null; + + beforeAll(async () => { + tempDb = await startEmbeddedPostgresTestDatabase("paperclip-issues-unpark-readiness-"); + db = createDb(tempDb.connectionString); + svc = issueService(db); + }); + + afterEach(async () => { + await db.delete(issueComments); + await db.delete(issueRelations); + await db.delete(issueInboxArchives); + await db.delete(activityLog); + await db.delete(issues); + await db.delete(heartbeatRuns); + await db.delete(executionWorkspaces); + await db.delete(projectWorkspaces); + await db.delete(projects); + await db.delete(goals); + await db.delete(agents); + await db.delete(instanceSettings); + await db.delete(companies); + }); + + afterAll(async () => { + await tempDb?.cleanup(); + }); + + async function seedBlockedIssue() { + const companyId = randomUUID(); + const agentId = randomUUID(); + const issueId = 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: "MulticastEngineer", + role: "engineer", + status: "active", + adapterType: "codex_local", + adapterConfig: {}, + runtimeConfig: {}, + permissions: {}, + }); + await db.insert(issues).values({ + id: issueId, + companyId, + title: "Delegate recovery target", + status: "blocked", + priority: "critical", + assigneeAgentId: agentId, + }); + + return { companyId, agentId, issueId }; + } + + async function seedBlocker(companyId: string, blockedIssueId: string, status: string) { + const blockerId = randomUUID(); + await db.insert(issues).values({ + id: blockerId, + companyId, + title: `Upstream blocker (${status})`, + status, + priority: "high", + }); + await db.insert(issueRelations).values({ + id: randomUUID(), + companyId, + issueId: blockerId, + relatedIssueId: blockedIssueId, + type: "blocks", + }); + return blockerId; + } + + async function readBlockerIds(issueId: string) { + const rows = await db + .select({ blockerIssueId: issueRelations.issueId }) + .from(issueRelations) + .where(eq(issueRelations.relatedIssueId, issueId)); + return rows.map((row) => row.blockerIssueId).sort(); + } + + function unpark(issueId: string, assigneeAgentId: string) { + return svc.update(issueId, { + status: "todo", + blockedByIssueIds: [], + expectedCurrentStatus: "blocked", + expectedCurrentAssigneeAgentId: assigneeAgentId, + requireDependencyReadyBeforeClearingBlockers: true, + }); + } + + it("refuses and preserves the edge when a live blocker lands after the route's snapshot", async () => { + const { companyId, issueId, agentId } = await seedBlockedIssue(); + + // The route authorized this patch against a readiness snapshot taken when + // the row had no blockers. This edge is what commits in between. + const blockerId = await seedBlocker(companyId, issueId, "todo"); + + await expect(unpark(issueId, agentId)).rejects.toMatchObject({ + status: 409, + details: { reason: "delegate_recovery_unresolved_blockers" }, + }); + + expect(await readBlockerIds(issueId)).toEqual([blockerId]); + const row = await db + .select({ status: issues.status }) + .from(issues) + .where(eq(issues.id, issueId)) + .then((rows) => rows[0]); + expect(row?.status).toBe("blocked"); + }); + + it("refuses and preserves the edge when the blocker commits while the write is in flight", async () => { + const { companyId, issueId, agentId } = await seedBlockedIssue(); + + const blockerId = randomUUID(); + await db.insert(issues).values({ + id: blockerId, + companyId, + title: "Upstream blocker racing the unpark", + status: "todo", + priority: "high", + }); + + const rowLocked = deferred(); + const addCanCommit = deferred(); + + // `syncBlockedByIssueIds` takes `FOR UPDATE` on the blocked row before it + // inserts, so this transaction holds exactly the lock the unpark's own + // UPDATE has to wait behind. Nothing is visible to the unpark until it + // commits — which is the interleaving that used to lose the edge. + const concurrentAdd = db.transaction(async (tx) => { + await tx.execute( + sql`select ${issues.id} from ${issues} where ${issues.id} = ${issueId} for update`, + ); + rowLocked.resolve(); + await addCanCommit.promise; + await tx.insert(issueRelations).values({ + id: randomUUID(), + companyId, + issueId: blockerId, + relatedIssueId: issueId, + type: "blocks", + }); + }); + + await rowLocked.promise; + + const unparkPromise = unpark(issueId, agentId); + // Let the unpark reach its UPDATE and block on the held row lock. + await new Promise((resolve) => setTimeout(resolve, 50)); + addCanCommit.resolve(); + await concurrentAdd; + + await expect(unparkPromise).rejects.toMatchObject({ + status: 409, + details: { reason: "delegate_recovery_unresolved_blockers" }, + }); + + expect(await readBlockerIds(issueId)).toEqual([blockerId]); + const row = await db + .select({ status: issues.status }) + .from(issues) + .where(eq(issues.id, issueId)) + .then((rows) => rows[0]); + expect(row?.status).toBe("blocked"); + }); + + // BLO-26403: the race above commits its edge with a direct `issueRelations` + // insert, so it never exercises the lock sequence a real blocker add takes. + // These cases drive the concurrent add through the production `update()` + // entry point — the only path that reaches `syncBlockedByIssueIds` for an + // already-existing issue — and pin the acquisition order inside `runUpdate`. + + function sleep(ms: number) { + return new Promise((resolve) => setTimeout(resolve, ms)); + } + + /** + * Drizzle wraps driver errors in a `Failed query: …` error whose own `code` + * is undefined, so both the deadlock assertion and the lock-order probe have + * to walk the cause chain to see PostgreSQL's real SQLSTATE. + */ + function describeError(error: unknown) { + const codes: string[] = []; + const messages: string[] = []; + let current: unknown = error; + for (let depth = 0; current && depth < 10; depth += 1) { + const candidate = current as { code?: unknown; message?: unknown; cause?: unknown }; + if (typeof candidate.code === "string") codes.push(candidate.code); + if (typeof candidate.message === "string") messages.push(candidate.message); + current = candidate.cause; + } + return { codes, message: messages.join(" | ") || String(error) }; + } + + async function seedStandaloneBlocker(companyId: string) { + const blockerId = randomUUID(); + await db.insert(issues).values({ + id: blockerId, + companyId, + title: "Upstream blocker racing the unpark", + status: "todo", + priority: "high", + }); + return blockerId; + } + + /** + * Parks both service calls on a row lock we hold, so the order they were + * started in is the order they commit in. Whichever starts first takes the + * company parent advisory and the blocker-relation advisory and then waits + * on the row; the other queues behind the company advisory. Releasing the + * row lets them drain in start order, which is what makes each interleaving + * deterministic rather than a timing coin-flip. + */ + async function raceUnparkAgainstBlockerAdd(order: "add-first" | "unpark-first") { + const { companyId, issueId, agentId } = await seedBlockedIssue(); + const blockerId = await seedStandaloneBlocker(companyId); + + const rowHeld = deferred(); + const releaseRow = deferred(); + const holder = db.transaction(async (tx) => { + await tx.execute( + sql`select ${issues.id} from ${issues} where ${issues.id} = ${issueId} for update`, + ); + rowHeld.resolve(); + await releaseRow.promise; + }); + await rowHeld.promise; + + const startUnpark = () => unpark(issueId, agentId); + // The production blocker add: `svc.update` -> `runUpdate` -> `syncBlockedByIssueIds`. + const startAdd = () => svc.update(issueId, { blockedByIssueIds: [blockerId] }); + + const first = order === "add-first" ? startAdd() : startUnpark(); + const firstSettled = first.catch(() => undefined); + await sleep(75); + const second = order === "add-first" ? startUnpark() : startAdd(); + const secondSettled = second.catch(() => undefined); + await sleep(75); + + releaseRow.resolve(); + await holder; + + const results = await Promise.allSettled([first, second]); + void firstSettled; + void secondSettled; + + for (const result of results) { + if (result.status === "rejected") { + const described = describeError(result.reason); + // 40P01 is PostgreSQL's deadlock_detected. + expect(described.codes).not.toContain("40P01"); + expect(described.message).not.toMatch(/deadlock/i); + } + } + + const status = await db + .select({ status: issues.status }) + .from(issues) + .where(eq(issues.id, issueId)) + .then((rows) => rows[0]?.status); + + return { results, blockerId, blockerIds: await readBlockerIds(issueId), status }; + } + + it("does not deadlock when a production blocker add commits before the guarded unpark", async () => { + const { results, blockerId, blockerIds, status } = await raceUnparkAgainstBlockerAdd("add-first"); + + // The add lands first, so the guard's readiness re-assert sees a live + // unresolved blocker and refuses rather than deleting the fresh edge. + expect(results[0]?.status).toBe("fulfilled"); + expect(results[1]).toMatchObject({ + status: "rejected", + reason: expect.objectContaining({ + status: 409, + details: expect.objectContaining({ reason: "delegate_recovery_unresolved_blockers" }), + }), + }); + expect(blockerIds).toEqual([blockerId]); + expect(status).toBe("blocked"); + }); + + it("does not deadlock when the guarded unpark commits before a production blocker add", async () => { + const { results, blockerId, blockerIds, status } = await raceUnparkAgainstBlockerAdd("unpark-first"); + + // The unpark wins the row: it clears nothing (there was nothing to clear) + // and the add then applies its edge on top of the unparked row. + expect(results.every((result) => result.status === "fulfilled")).toBe(true); + expect(blockerIds).toEqual([blockerId]); + expect(status).toBe("todo"); + }); + + it("takes the blocker-relation advisory lock before touching the issue row", async () => { + const { companyId, issueId, agentId } = await seedBlockedIssue(); + + const advisoryHeld = deferred(); + const releaseAdvisory = deferred(); + const lockKey = `paperclip:issue-blockers:${companyId}:${issueId}`; + const holder = db.transaction(async (tx) => { + await tx.execute(sql`select pg_advisory_xact_lock(hashtextextended(${lockKey}, 0))`); + advisoryHeld.resolve(); + await releaseAdvisory.promise; + }); + await advisoryHeld.promise; + + const unparkPromise = unpark(issueId, agentId); + const unparkSettled = unparkPromise.catch(() => undefined); + + // Wait for the unpark to actually park on the advisory lock rather than + // sleeping blind — otherwise a probe that ran before the unpark reached + // any lock would pass vacuously. + let parked = false; + for (let attempt = 0; attempt < 100 && !parked; attempt += 1) { + const rows = await db.execute( + sql`select count(*)::int as waiting from pg_locks where locktype = 'advisory' and not granted`, + ); + parked = Number((rows as unknown as Array<{ waiting: number }>)[0]?.waiting ?? 0) > 0; + if (!parked) await sleep(20); + } + expect(parked).toBe(true); + + // `runUpdate` takes `lockIssueBlockerRelations` before the row's + // `FOR UPDATE`, so a unpark parked on the advisory has not touched the + // row yet and this probe succeeds. If `.update(issues)` were hoisted above + // that call, the row would already be write-locked here and PostgreSQL + // would raise 55P03 (lock_not_available) instead. + const probe = await db + .transaction(async (tx) => { + await tx.execute( + sql`select ${issues.id} from ${issues} where ${issues.id} = ${issueId} for update nowait`, + ); + return { acquired: true as const }; + }) + .catch((error: unknown) => { + const described = describeError(error); + return { + // 55P03 is lock_not_available — the row was already write-locked, + // which means the row lock was taken before the advisory lock. + acquired: described.codes.includes("55P03") ? ("row-already-locked" as const) : described.codes, + message: described.message, + }; + }); + + releaseAdvisory.resolve(); + await holder; + await unparkSettled; + + expect(probe).toEqual({ acquired: true }); + await expect(unparkPromise).resolves.toMatchObject({ status: "todo" }); + }); + + it("still unparks a row with no blockers at all", async () => { + const { issueId, agentId } = await seedBlockedIssue(); + + const updated = await unpark(issueId, agentId); + + expect(updated?.status).toBe("todo"); + expect(await readBlockerIds(issueId)).toEqual([]); + }); + + it("still unparks and clears stale edges when every blocker is done", async () => { + const { companyId, issueId, agentId } = await seedBlockedIssue(); + await seedBlocker(companyId, issueId, "done"); + + const updated = await unpark(issueId, agentId); + + expect(updated?.status).toBe("todo"); + expect(await readBlockerIds(issueId)).toEqual([]); + }); + + it("treats a cancelled blocker as unresolved and leaves the edge in place", async () => { + const { companyId, issueId, agentId } = await seedBlockedIssue(); + const blockerId = await seedBlocker(companyId, issueId, "cancelled"); + + await expect(unpark(issueId, agentId)).rejects.toMatchObject({ status: 409 }); + + expect(await readBlockerIds(issueId)).toEqual([blockerId]); + }); + + it("leaves ordinary blocker writes unguarded when the flag is not set", async () => { + const { companyId, issueId } = await seedBlockedIssue(); + await seedBlocker(companyId, issueId, "todo"); + + // The #870 coordination path legitimately rewrites edges on a row with + // live blockers; the new guard must not reach it. + const updated = await svc.update(issueId, { blockedByIssueIds: [] }); + + expect(updated).not.toBeNull(); + expect(await readBlockerIds(issueId)).toEqual([]); + }); + }, +); diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index 880bf0a8255b..7aebdd4fd0a0 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -5443,6 +5443,30 @@ export function issueRoutes( creatorOrManagerChainDecision && isCreatorOrManagerChainRecoveryPatch(issue, req.body as Record) ) { + // BLO-20385: the patch shape *mandates* `blockedByIssueIds: []`, and that + // empty array is then applied — so admitting the bypass on an issue that + // still has live blockers does not merely unpark it, it silently deletes + // dependency edges the actor could not otherwise remove. The shape check + // above only inspects the request body; it never looked at the issue's + // actual blockers. Unparking a row whose blockers are all terminal is the + // intended use and stays allowed (clearing those stale edges is the + // point). Refuse when any blocker is unresolved, and say so explicitly + // rather than reusing the opaque boundary 403 — an unexplained deny on + // this path already cost a full diagnostic cycle once. + const readiness = await svc.getDependencyReadiness(issue.id); + if (readiness.unresolvedBlockerCount > 0) { + res.status(409).json({ + error: + "Cannot unpark an issue that still has unresolved blockers: this patch shape clears blockedByIssueIds and would delete live dependency edges", + details: { + issueId: issue.id, + reason: "delegate_recovery_unresolved_blockers", + unresolvedBlockerCount: readiness.unresolvedBlockerCount, + unresolvedBlockerIssueIds: readiness.unresolvedBlockerIssueIds, + }, + }); + return false; + } return true; } if (creatorOrManagerChainDecision && !options.allowCreatorOrManagerChainOwnership) { @@ -10491,6 +10515,10 @@ export function issueRoutes( // reassignment to an unrelated agent that keeps the row // blocked would still satisfy an id+status predicate. expectedCurrentAssigneeAgentId: existing.assigneeAgentId, + // BLO-20385: neither pin catches a concurrent blocker add, + // which changes no field either one covers. Re-assert + // readiness under the row lock before the edges are cleared. + requireDependencyReadyBeforeClearingBlockers: true, // BLO-22876 review: the reroute grant additionally rests on // that assignee being non-invokable, which lives in `agents` // and so cannot be pinned by an `issues` WHERE clause. Pin @@ -10534,6 +10562,8 @@ export function issueRoutes( // authorization-relevant snapshot field for allow_manager_chain, // and its invokability is one for the BLO-22876 reroute grant. expectedCurrentAssigneeAgentId: existing.assigneeAgentId, + // BLO-20385: and neither pin covers a concurrent blocker add. + requireDependencyReadyBeforeClearingBlockers: true, ...(managerChainNonInvokableRerouteAllowed ? { expectedCurrentAssigneeAgentNonInvokable: true } : {}), diff --git a/server/src/services/issues.ts b/server/src/services/issues.ts index c4881b572b6c..cdbeb47da89b 100644 --- a/server/src/services/issues.ts +++ b/server/src/services/issues.ts @@ -5806,28 +5806,34 @@ export function issueService(db: Db) { ); } - async function syncBlockedByIssueIds( + /** + * Lock the complete blocker graph in its canonical order. Every blocker + * mutation must take the relation advisory lock before it takes any issue + * row lock; callers may then safely update the issue row and mutate its + * relations without reacquiring either lock. + */ + async function lockIssueBlockerGraphForUpdate( issueId: string, companyId: string, blockedByIssueIds: string[], - actor: { agentId?: string | null; userId?: string | null } = {}, dbOrTx: any = db, - ): Promise { - if (dbOrTx === db) { - return db.transaction((tx) => - syncBlockedByIssueIds(issueId, companyId, blockedByIssueIds, actor, tx), - ); - } + ) { + await lockIssueBlockerRelations(dbOrTx, companyId, issueId); + await lockBlockedByIssueRowsForUpdate(issueId, companyId, blockedByIssueIds, dbOrTx); + } + async function syncBlockedByIssueIdsLocked( + issueId: string, + companyId: string, + blockedByIssueIds: string[], + actor: { agentId?: string | null; userId?: string | null } = {}, + dbOrTx: any, + ): Promise { const deduped = [...new Set(blockedByIssueIds)]; if (deduped.some((candidate) => candidate === issueId)) { throw unprocessable("Issue cannot be blocked by itself"); } - await lockIssueBlockerRelations(dbOrTx, companyId, issueId); - - await lockBlockedByIssueRowsForUpdate(issueId, companyId, deduped, dbOrTx); - if (deduped.length > 0) { const relatedIssues = await dbOrTx .select({ id: issues.id }) @@ -5863,6 +5869,27 @@ export function issueService(db: Db) { ); } + async function syncBlockedByIssueIds( + issueId: string, + companyId: string, + blockedByIssueIds: string[], + actor: { agentId?: string | null; userId?: string | null } = {}, + dbOrTx: any = db, + ): Promise { + if (dbOrTx === db) { + return db.transaction((tx) => + syncBlockedByIssueIds(issueId, companyId, blockedByIssueIds, actor, tx), + ); + } + + const deduped = [...new Set(blockedByIssueIds)]; + if (deduped.some((candidate) => candidate === issueId)) { + throw unprocessable("Issue cannot be blocked by itself"); + } + await lockIssueBlockerGraphForUpdate(issueId, companyId, deduped, dbOrTx); + await syncBlockedByIssueIdsLocked(issueId, companyId, deduped, actor, dbOrTx); + } + async function isTerminalOrMissingHeartbeatRun(runId: string, dbOrTx: DbReader = db) { const run = await dbOrTx .select({ status: heartbeatRuns.status }) @@ -9085,6 +9112,27 @@ export function issueService(db: Db) { * version when the statement blocks on a concurrent transaction. */ expectedCurrentAssigneeAgentId?: string | null; + /** + * BLO-20385: the delegate-recovery unpark is authorized *because* the + * issue's blockers are all terminal, and the patch it carries clears + * `blockedByIssueIds`. The route checks readiness before this call, but + * that read is a snapshot: a concurrent writer can add a live blocker + * without touching the status or the assignee, so both preconditions + * above still hold and `syncBlockedByIssueIds(..., [])` then deletes the + * newly-live edge — reinstating the exact data loss this path was + * changed to prevent. + * + * When set, dependency readiness is re-asserted inside the transaction, + * after the UPDATE below has taken the issue row's exclusive lock and + * before any relation is cleared. That ordering is what makes it sound: + * every path that adds a blocker goes through `syncBlockedByIssueIds`, + * which takes `FOR UPDATE` on the blocked issue's row. So a concurrent + * adder either commits before our UPDATE — and the re-read below, under + * READ COMMITTED, sees its edge and we 409 — or it blocks on our lock + * until we commit and re-adds its edge afterwards. Neither interleaving + * loses the edge. + */ + requireDependencyReadyBeforeClearingBlockers?: boolean; /** * BLO-22876 review: the manager-chain reroute grant is conditioned on the * current assignee being *non-invokable*. Unlike the guard above, that @@ -9133,6 +9181,7 @@ export function issueService(db: Db) { actorUserId, expectedCurrentStatus, expectedCurrentAssigneeAgentId, + requireDependencyReadyBeforeClearingBlockers, expectedCurrentAssigneeAgentNonInvokable, expectedCurrentCheckoutRunId, expectedCurrentExecutionRunId, @@ -9431,8 +9480,11 @@ export function issueService(db: Db) { await lockIssueParentMutationCompany(existing.companyId, tx); } if (blockedByIssueIds !== undefined) { - await lockIssueBlockerRelations(tx, existing.companyId, id); - await lockBlockedByIssueRowsForUpdate(id, existing.companyId, blockedByIssueIds, tx); + // Keep the blocker advisory lock outermost. The relation mutation + // below uses these locks rather than reacquiring them after the + // issue UPDATE, so recovery clears and ordinary blocker writes share + // one explicit lock order: graph advisory -> issue rows -> writes. + await lockIssueBlockerGraphForUpdate(id, existing.companyId, blockedByIssueIds, tx); } else if (patch.status === "in_progress") { await lockIssueBlockerRelations(tx, existing.companyId, id); const currentBlockerIssueIds = await tx @@ -9738,7 +9790,39 @@ export function issueService(db: Db) { await syncIssueLabels(updated.id, lockedExisting.companyId, nextLabelIds, tx); } if (blockedByIssueIds !== undefined) { - await syncBlockedByIssueIds( + // BLO-20385 (Ally review on #970): re-assert dependency readiness here + // rather than trusting the route's pre-write snapshot. The UPDATE above + // has already taken this row's exclusive lock. The canonical graph + // locks were acquired before that UPDATE, and every blocker-add + // path takes the same advisory-plus-row sequence before it mutates, + // so by this point a concurrent adder has + // either committed — and this re-read, under READ COMMITTED, sees its + // edge — or is parked behind us and re-adds after we commit. Without + // this, the clear below silently deletes an edge that went live after + // the authorization check. + if (requireDependencyReadyBeforeClearingBlockers) { + const readinessNow = await listIssueDependencyReadinessMap(tx, existing.companyId, [ + updated.id, + ]); + const readiness = + readinessNow.get(updated.id) ?? createIssueDependencyReadiness(updated.id); + if (readiness.unresolvedBlockerCount > 0) { + throw conflict( + "Cannot unpark an issue that still has unresolved blockers: this patch shape clears blockedByIssueIds and would delete live dependency edges", + { + issueId: updated.id, + reason: "delegate_recovery_unresolved_blockers", + unresolvedBlockerCount: readiness.unresolvedBlockerCount, + unresolvedBlockerIssueIds: readiness.unresolvedBlockerIssueIds, + }, + ); + } + } + // The graph locks were taken before the issue-row update above; + // mutate the relations under those held locks instead of calling the + // public helper, which would obscure the lock order by reacquiring + // them after the row write. + await syncBlockedByIssueIdsLocked( updated.id, lockedExisting.companyId, blockedByIssueIds,