refactor(webhook): hand the back-link lock's transaction to its callback (PEN-2865)

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<typeof sql>) => Promise<unknown>;
+    }): 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<Settings> | { rows?: Array<Settings> };
+      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",

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<Parameters<Db["transaction"]>[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<T>(
   db: Db,
   ref: { repoFullName: string; prNumber: number },
-  post: () => Promise<T>,
+  post: (tx: DbTransaction) => Promise<T>,
   timeouts: PrIssueBackLinkLockTimeouts = {},
 ): Promise<T> {
   const waitMs = timeouts.waitMs ?? BACKLINK_LOCK_WAIT_TIMEOUT_MS;
@@ -75,7 +84,9 @@ export async function withPrIssueBackLinkLock<T>(
       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);
   });
 }