diff --git a/server/src/__tests__/pr-issue-backlink-lock.test.ts b/server/src/__tests__/pr-issue-backlink-lock.test.ts index e0820aab4a0b..e37b0ed894f1 100644 --- a/server/src/__tests__/pr-issue-backlink-lock.test.ts +++ b/server/src/__tests__/pr-issue-backlink-lock.test.ts @@ -239,6 +239,57 @@ describeEmbeddedPostgres("PR→issue back-link is posted at most once per PR (PE } }, 60_000); + it("hands the callback the lock-holding transaction, not a second pool connection", async () => { + // The bounds asserted above only protect the pool while everything inside + // the critical section runs on the transaction that holds the lock. A + // caller that needs a DB read and reaches for the outer `db` instead takes + // a *second* connection while this one is still held, which is the + // exhaustion those bounds exist to make recoverable — and the shape that + // caused a measured production incident on the recovery path (#1879, + // #1887, #1897). So `post` is handed `tx`, and this asserts the handle is + // genuinely that transaction rather than merely being typed as one. + // + // The discriminator is that `set_config(..., true)` is transaction-local: + // the helper's own timeouts are readable through the real transaction and + // through nothing else. Non-round values are used so neither can pass by + // matching a server default, and so Postgres prints them in `ms` rather + // than normalising to a coarser unit. + const ref = { repoFullName: "Blockcast/paperclip", prNumber: 1742 }; + + const readSettings = async (handle: { + execute: (q: ReturnType) => Promise; + }): Promise<{ lock: string; idle: string }> => { + // postgres-js returns the rows as an array; node-postgres wraps them in + // `.rows`. Narrow across both rather than asserting an intersection of + // the two, which would describe a value neither driver can return. Same + // shape as `toRows` in `services/approval-gate-reconciler.ts`. + type Settings = { lock: string; idle: string }; + const result = (await handle.execute( + sql`select current_setting('lock_timeout') as lock, current_setting('idle_in_transaction_session_timeout') as idle`, + )) as Array | { rows?: Array }; + const rows = Array.isArray(result) ? result : (result.rows ?? []); + const row = rows[0]; + return { lock: row.lock, idle: row.idle }; + }; + + const insideOnTx = await withPrIssueBackLinkLock( + db, + ref, + async (tx) => readSettings(tx), + { waitMs: 7_777, holdMs: 23_456 }, + ); + + expect(insideOnTx).toEqual({ lock: "7777ms", idle: "23456ms" }); + + // Negative control: the same read on the pooled handle is a different + // session and cannot see those transaction-local values. Without this, the + // assertion above would also pass if `current_setting` simply returned + // whatever was configured process-wide. + const outsideOnPool = await readSettings(db); + expect(outsideOnPool.lock).not.toBe("7777ms"); + expect(outsideOnPool.idle).not.toBe("23456ms"); + }, 30_000); + it("keys the lock on the normalized repo and the PR number", () => { expect(__test_prIssueBackLinkLockKey({ repoFullName: " Blockcast/Paperclip ", prNumber: 1738 })).toBe( "github:pr-issue-backlink:blockcast/paperclip:1738", diff --git a/server/src/services/pr-issue-backlink-lock.ts b/server/src/services/pr-issue-backlink-lock.ts index 040a9ac02b91..48761eb4cf3e 100644 --- a/server/src/services/pr-issue-backlink-lock.ts +++ b/server/src/services/pr-issue-backlink-lock.ts @@ -2,6 +2,8 @@ import { sql } from "drizzle-orm"; import type { Db } from "@paperclipai/db"; import { normalizePrReviewRepoFullName } from "./pr-review-duplicate-issue-guard.js"; +type DbTransaction = Parameters[0]>[0]; + const PR_ISSUE_BACKLINK_LOCK_PREFIX = "github:pr-issue-backlink:"; // How long a second delivery may queue for this PR's lock before giving up, and @@ -57,11 +59,18 @@ export type PrIssueBackLinkLockTimeouts = { * back-link is a cosmetic loss, a double-post is the defect being fixed. In the * healthy case no timeout is reached at all — the second delivery blocks * briefly, then reads the marker the first one wrote and correctly skips. + * + * `post` receives the transaction handle. Today's only caller performs GitHub + * I/O and no database work, so it ignores the argument — but reaching for the + * outer `db` from inside the critical section would take a *second* pool + * connection while this one is still held, which is precisely what makes the + * exhaustion above reachable. Handing `tx` over makes the safe handle the one + * already in scope, as `withGithubStatusDeliveryLock` does for the same reason. */ export async function withPrIssueBackLinkLock( db: Db, ref: { repoFullName: string; prNumber: number }, - post: () => Promise, + post: (tx: DbTransaction) => Promise, timeouts: PrIssueBackLinkLockTimeouts = {}, ): Promise { const waitMs = timeouts.waitMs ?? BACKLINK_LOCK_WAIT_TIMEOUT_MS; @@ -75,7 +84,9 @@ export async function withPrIssueBackLinkLock( sql`select set_config('idle_in_transaction_session_timeout', ${`${holdMs}ms`}, true)`, ); await tx.execute(sql`select pg_advisory_xact_lock(hashtextextended(${key}, 0))`); - return post(); + // Hand the transaction handle to the caller: taking a second pool + // connection here is what makes the exhaustion above reachable. + return post(tx); }); }