From 38625e92fd25c68ab7d08accf44f793dd6f5ab94 Mon Sep 17 00:00:00 2001 From: Cto Date: Thu, 17 Sep 2026 01:31:38 +0000 Subject: [PATCH 1/3] refactor(webhook): hand the back-link lock's transaction to its callback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ally's review of ae08a77e (Suggestion, non-blocking): `post` took no handle, where the mirrored `withGithubStatusDeliveryLock` passes `tx` and says why — taking a second pool connection inside the critical section is what makes the exhaustion the timeouts guard against reachable. No defect today: the sole caller does GitHub I/O and no database work, so it ignores the argument and behaviour is unchanged. The point is the next caller. With no handle in scope, one that needs a database read has only the outer `db` to reach for, which checks out a second connection while this transaction still holds the first — against a 10-connection pool, under concurrent delivery, with the holder already parked inside uncapped GitHub calls. Passing `tx` makes the safe handle the one already in hand. Verified: `pnpm --filter @paperclipai/server typecheck` yields a byte-identical 39-error set before and after (all pre-existing implicit-any noise in `tool-access.ts` / shared validators, none in the touched files — CI's Typecheck is green at this head), so the widened signature is accepted at the existing zero-arg call site. `pr-issue-backlink-lock.test.ts` 6/6 pass, including the pool-exhaustion proof at the real `POSTGRES_POOL_MAX`. Refs PEN-2865 Signed-off-by: Cto --- server/src/services/pr-issue-backlink-lock.ts | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) 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); }); } From 5ffb923a0eec59ec15dfe863a282b30d6b7b9d87 Mon Sep 17 00:00:00 2001 From: Cto Date: Thu, 17 Sep 2026 01:46:24 +0000 Subject: [PATCH 2/3] test(webhook): assert the back-link lock hands over its own transaction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The signature change in the parent commit is type-level, so on its own nothing proves the handle is the lock-holding transaction rather than merely typed as one. This asserts the property behaviourally. The discriminator is that `set_config(..., true)` is transaction-local: the helper's own `lock_timeout` / `idle_in_transaction_session_timeout` are readable through the real transaction and through nothing else. Non-round values (7777ms / 23456ms) are used so the read cannot pass by matching a server default, and so Postgres prints them in `ms` rather than normalising to a coarser unit. The pooled handle is read in the same test as a negative control, since without it the assertion would also pass if `current_setting` merely returned a process-wide value. Negative control on the test itself: replacing `post(tx)` with the pooled `db` fails it specifically, reading `{lock: '0', idle: '0'}` — the server defaults — while the other six tests still pass. So this test, and only this test, carries the property. 7/7 pass. Typecheck unchanged at the same pre-existing 39-error baseline, none in the touched files. Refs PEN-2865 Signed-off-by: Cto --- .../__tests__/pr-issue-backlink-lock.test.ts | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/server/src/__tests__/pr-issue-backlink-lock.test.ts b/server/src/__tests__/pr-issue-backlink-lock.test.ts index e0820aab4a0b..62eca216b490 100644 --- a/server/src/__tests__/pr-issue-backlink-lock.test.ts +++ b/server/src/__tests__/pr-issue-backlink-lock.test.ts @@ -239,6 +239,51 @@ 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 }> => { + const result = (await handle.execute( + sql`select current_setting('lock_timeout') as lock, current_setting('idle_in_transaction_session_timeout') as idle`, + )) as { rows?: Array<{ lock: string; idle: string }> } & Array<{ lock: string; idle: string }>; + const row = (result.rows ?? result)[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", From 156036b02857528ea1e6cf4007ca331043d9c324 Mon Sep 17 00:00:00 2001 From: Cto Date: Thu, 17 Sep 2026 15:05:49 +0000 Subject: [PATCH 3/3] test(webhook): narrow the driver-shape defence instead of asserting an impossible intersection (PEN-2865) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `readSettings` cast the execute result to `{ rows?: Array } & Array` — a type no runtime value can inhabit, since nothing is simultaneously an array and an object carrying `rows`. It worked only because `(result.rows ?? result)[0]` exercises one half at a time, so the intersection asserted something false about the driver rather than describing it. Narrow across a union instead. postgres-js (this repo's driver) returns the rows as an array; node-postgres wraps them in `.rows`, and the defence covers both without the type lie. This is the shape `toRows` in `services/approval-gate-reconciler.ts:168` already uses, so the test now matches the established in-repo idiom. Behaviour is unchanged: typecheck passes and all 7 tests in the file still pass against embedded Postgres, including the transaction-handle assertion this helper serves. Raised by Ally review on #1900 (suggestion 2 of 3). Signed-off-by: Cto --- server/src/__tests__/pr-issue-backlink-lock.test.ts | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/server/src/__tests__/pr-issue-backlink-lock.test.ts b/server/src/__tests__/pr-issue-backlink-lock.test.ts index 62eca216b490..e37b0ed894f1 100644 --- a/server/src/__tests__/pr-issue-backlink-lock.test.ts +++ b/server/src/__tests__/pr-issue-backlink-lock.test.ts @@ -259,10 +259,16 @@ describeEmbeddedPostgres("PR→issue back-link is posted at most once per PR (PE 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 { rows?: Array<{ lock: string; idle: string }> } & Array<{ lock: string; idle: string }>; - const row = (result.rows ?? result)[0]; + )) as Array | { rows?: Array }; + const rows = Array.isArray(result) ? result : (result.rows ?? []); + const row = rows[0]; return { lock: row.lock, idle: row.idle }; };