From a7aed3e53ec59e427fbbe5bc245ae3d7ac3fe7e1 Mon Sep 17 00:00:00 2001 From: Paperclip CTO Date: Sun, 2 Aug 2026 16:45:15 +0000 Subject: [PATCH 1/4] fix(authz): refuse delegate-recovery unpark when blockers are unresolved (BLO-20385) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `isCreatorOrManagerChainRecoveryPatch` gates the blocked -> todo delegate recovery PATCH purely on the request body shape, which mandates `blockedByIssueIds: []`. That empty array is then applied — so admitting the bypass on an issue whose blockers are still live did not merely unpark it, it silently deleted dependency edges the actor had no other way to remove, and returned 200 with no indication it had happened. Probed in production on BLO-18946 (unresolvedBlockerCount 1, live edge to BLO-17770, itself blocked): the PATCH returned 200 and left blockedBy empty. Edge restored via the #870 coordination path. Gate the admit on dependency readiness. Blockers that are all terminal still clear — that is the intended use — but any unresolved blocker now yields 409 `delegate_recovery_unresolved_blockers` naming the offending ids, and no write reaches the service. Deliberately not another opaque boundary 403: an unexplained deny on this exact path already cost a full diagnostic cycle. Scoped to the authorization admit in assertAgentIssueMutationAllowed. The shape check at the write-time concurrency guard is unchanged, as is the in_progress 409 guard and the #870 coordination-metadata allowlist. Co-Authored-By: Claude --- ...ue-agent-mutation-ownership-routes.test.ts | 70 +++++++++++++++++++ server/src/routes/issues.ts | 24 +++++++ 2 files changed, 94 insertions(+) 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 4e360db3ce7d..b6244ae1d0de 100644 --- a/server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts +++ b/server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts @@ -2878,6 +2878,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/routes/issues.ts b/server/src/routes/issues.ts index b5e144e45ddd..c20c97eb2894 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -4506,6 +4506,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) { From 3adcf397e0a588eda9377766d335a76b62c7da80 Mon Sep 17 00:00:00 2001 From: Paperclip CTO Date: Sat, 8 Aug 2026 08:35:10 +0000 Subject: [PATCH 2/4] fix(authz): re-assert blocker readiness under the row lock before clearing edges (BLO-20385) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ally's exact-head review of #970 found the guard it added was a non-atomic snapshot. The route reads dependency readiness before calling `svc.update`, but a concurrent writer can add a live blocker without touching the issue's status or assignee — so both BLO-18797 preconditions (`expectedCurrentStatus`, `expectedCurrentAssigneeAgentId`) still match, and `syncBlockedByIssueIds(..., [])` then deletes the newly-live edge. That is the same silent data loss #970 set out to close, reachable under a race. Re-assert readiness inside the transaction, after the UPDATE has taken the issue row's exclusive lock and before any relation is cleared, via a new `requireDependencyReadyBeforeClearingBlockers` option. The ordering is what makes it sound: every blocker-add path goes through `syncBlockedByIssueIds`, which takes `FOR UPDATE` on the blocked row. So a concurrent adder either commits before our UPDATE — and the in-transaction re-read, under READ COMMITTED, sees its edge and we 409 — or it parks behind our lock and re-adds after we commit. Neither interleaving loses the edge. The route keeps its pre-write check so the common case still gets the explicit 409 without opening a transaction; the in-transaction assertion is the authoritative one. Both are keyed off `delegateRecoveryPatchInFlight`, the same predicate BLO-18797 used, so the checkout-management-override and recovery-action-owner paths that reach this exact two-key patch shape are covered too. Genuine blocker edits are unaffected — they carry a different body shape and go through the #870 coordination path. Tests: six cases in `issues-service.test.ts`, including a genuinely concurrent one that holds `FOR UPDATE` on the row and commits the blocker while the unpark is blocked on that lock. Verified as real regression tests: with the new guard disabled, the three refusal cases fail and the three "still unparks" cases pass. Co-Authored-By: Claude --- server/src/__tests__/issues-service.test.ts | 223 ++++++++++++++++++++ server/src/routes/issues.ts | 6 + server/src/services/issues.ts | 49 +++++ 3 files changed, 278 insertions(+) diff --git a/server/src/__tests__/issues-service.test.ts b/server/src/__tests__/issues-service.test.ts index 7d6c981e0952..d43203d35e95 100644 --- a/server/src/__tests__/issues-service.test.ts +++ b/server/src/__tests__/issues-service.test.ts @@ -9236,3 +9236,226 @@ describeEmbeddedPostgres("issueService.update expectedCurrentStatus (BLO-18797)" expect(updated?.status).toBe("in_progress"); }); }); + +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"); + }); + + 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 c20c97eb2894..8a4b910c8750 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -9266,6 +9266,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, } : {}), }, @@ -9299,6 +9303,8 @@ export function issueRoutes( // See the transactional branch above: the assignee is an // authorization-relevant snapshot field for allow_manager_chain. expectedCurrentAssigneeAgentId: existing.assigneeAgentId, + // BLO-20385: and neither pin covers a concurrent blocker add. + requireDependencyReadyBeforeClearingBlockers: true, } : {}), }); diff --git a/server/src/services/issues.ts b/server/src/services/issues.ts index a71c185e981f..3bc3f8f15709 100644 --- a/server/src/services/issues.ts +++ b/server/src/services/issues.ts @@ -8386,6 +8386,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; }, dbOrTx: any = db, ) => { @@ -8403,6 +8424,7 @@ export function issueService(db: Db) { actorUserId, expectedCurrentStatus, expectedCurrentAssigneeAgentId, + requireDependencyReadyBeforeClearingBlockers, ...issueData } = data; @@ -8796,6 +8818,33 @@ export function issueService(db: Db) { await syncIssueLabels(updated.id, existing.companyId, nextLabelIds, tx); } if (blockedByIssueIds !== undefined) { + // 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, and every blocker-add + // path takes `FOR UPDATE` on the blocked row inside + // `syncBlockedByIssueIds`, 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, + }, + ); + } + } await syncBlockedByIssueIds( updated.id, existing.companyId, From 9256b10ef04c97c22c27940777ca421fd559f72d Mon Sep 17 00:00:00 2001 From: CTO Date: Wed, 12 Aug 2026 19:27:39 +0000 Subject: [PATCH 3/4] test(issues): pin advisory-before-row lock order in runUpdate (BLO-26403) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ally's exact-head review on #970 raised an Important finding claiming a lock-order inversion between the guarded recovery clear and a normal blocker update. The inversion claim is disputed and remains disputed: `runUpdate` takes `lockIssueBlockerRelations` before the row `FOR UPDATE`, and `syncBlockedByIssueIds` takes the same advisory lock before its own row locks, so both flows acquire advisory -> row and cannot cycle. The test half of that recommendation stands on its own, and this commit is only that half. No production lock acquisition, ordering, or #970 guard semantics change here. Three cases, all inside the existing BLO-20385 describe block: - Two race the guarded unpark against a concurrent blocker add driven through the production `update()` -> `syncBlockedByIssueIds` entry point rather than a direct `issueRelations` insert, which is what the existing coverage did. A controller transaction pins the blocked row so each interleaving is deterministic instead of a timing coin-flip. Both assert no 40P01 and the surviving edge set: add-first leaves the edge intact behind a 409, unpark-first unparks and the add re-applies. - One pins the acquisition order directly. It holds the blocker-relation advisory lock, waits (via pg_locks, not a blind sleep) for the unpark to park on it, then probes the issue row with `FOR UPDATE NOWAIT`. The row is still free only because the advisory lock is taken first. Verified non-vacuous: temporarily taking the row lock at the top of `runUpdate` — simulating `.update(issues)` hoisted above the advisory — fails the ordering test with `acquired: "row-already-locked"` / `could not obtain lock on row in relation "issues"` (55P03), while the other eight cases still pass. Reverted before commit; `git diff` against the branch point touches the test file only. Errors are unwrapped through the `cause` chain: drizzle wraps driver errors in a `Failed query: ...` error whose own `code` is undefined, so matching on the top-level code alone would have missed both 55P03 and a real 40P01. Full `issues-service.test.ts`: 214 passed. `pnpm typecheck`: exit 0. --- server/src/__tests__/issues-service.test.ts | 185 ++++++++++++++++++++ 1 file changed, 185 insertions(+) diff --git a/server/src/__tests__/issues-service.test.ts b/server/src/__tests__/issues-service.test.ts index 442bfb054b46..174fff97cf26 100644 --- a/server/src/__tests__/issues-service.test.ts +++ b/server/src/__tests__/issues-service.test.ts @@ -10701,6 +10701,191 @@ describeEmbeddedPostgres( 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(); From 74de877f8aba094624f2b0bf49017e71d15edd3a Mon Sep 17 00:00:00 2001 From: Omar Ramadan Date: Fri, 14 Aug 2026 04:51:40 +0000 Subject: [PATCH 4/4] fix(issues): keep blocker graph locks across guarded update --- server/src/services/issues.ts | 71 ++++++++++++++++++++++++++--------- 1 file changed, 53 insertions(+), 18 deletions(-) diff --git a/server/src/services/issues.ts b/server/src/services/issues.ts index 4c881a4bf825..e339bd51a462 100644 --- a/server/src/services/issues.ts +++ b/server/src/services/issues.ts @@ -5795,28 +5795,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 }) @@ -5852,6 +5858,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 }) @@ -9416,8 +9443,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 @@ -9719,9 +9749,10 @@ export function issueService(db: Db) { if (blockedByIssueIds !== undefined) { // 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, and every blocker-add - // path takes `FOR UPDATE` on the blocked row inside - // `syncBlockedByIssueIds`, so by this point a concurrent adder has + // 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 @@ -9744,7 +9775,11 @@ export function issueService(db: Db) { ); } } - await syncBlockedByIssueIds( + // 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,