[codex] fix(github-webhook): bound reviewer-wake lock retries (BLO-21582)

[kkroo]

Thinking Path

• Paperclip coordinates AI-agent work, including GitHub-driven reviewer wakes.
• A GitHub webhook must quickly acknowledge deliveries while still producing one durable, observable reviewer-wake outcome.
• The per-PR advisory lock serializes reviewer assignment, but pool checkout and database work can contend during delivery bursts.
• Racing a Drizzle transaction in JavaScript does not cancel the eventual database operation, so a timed-out request can leave detached work and an unobserved loss.
• This successor reimplements the still-needed BLO-21582 path on current master, preserving the delivery funnel and bounding both lock acquisition and fallback reads.
• The result is a bounded, observable webhook path that avoids a false success response for a dropped reviewer wake.

Linked Issues or Issue Description

No GitHub issue exists for this control-plane incident. Related Paperclip issue: BLO-21582.

Related PRs: #1003 is the App-authored original this draft supersedes; #1155 is a separate App-authored durable-retry approach for a later issue.

What happened: under per-PR lock or connection-pool contention, a reviewer wake could time out before the normal received metric was recorded. The webhook still returned HTTP 200, so GitHub did not redeliver the loss. A JavaScript timeout around db.transaction() also could not cancel a late pool checkout or query.

Expected behavior: lock acquisition and lock-exhaustion rechecks stay bounded, an abandoned reservation never runs detached SQL, an equivalent concurrent wake is not falsely dead-lettered, and a real loss is recorded as received plus dead_lettered.

Reproduction: hold the per-PR advisory lock or saturate the application pool while delivering a reviewer-triggering PR webhook. The new route tests exercise transient contention, exhausted contention, a blocked fallback read, and pool-checkout starvation.

Affected environment: current master (82154e5c) with PostgreSQL-backed reviewer wakes.

What Changed

• Replaced transaction-level timeout races with reservation-based lock acquisition and database-side statement_timeout bounds.
• Bound fallback rechecks and preserve a distinct timeout outcome so uncertain reads are not treated as confirmed no-ops.
• Record the delivery funnel correctly on a real exhausted lock timeout while suppressing false dead-letters for an equivalent durable wake or no active reviewer.
• Added lock-contention regressions plus a focused test that verifies the fallback timeout is installed before BEGIN and cleared before release.

Verification

• pnpm --filter @paperclipai/server typecheck — passed.
• pnpm exec vitest run server/src/__tests__/github-webhook.test.ts -t 'bounds fallback transaction setup' --reporter=verbose — passed (1 selected test).
• Full webhook suite: all 129 assertions passed. Vitest then exited nonzero because the suite's afterAll cleanup timed out after 60 seconds.
• Clean master comparison: all 120 assertions passed and reproduced the same afterAll timeout, establishing that cleanup failure as baseline test-harness behavior rather than this change.

Risks

• This uses reserved PostgreSQL connections and session-scoped statement_timeout; the implementation resets the timeout unconditionally before releasing each connection.
• The change is localized to reviewer-wake handling and its route tests; no schema, UI, or lockfile changes are included.

Model Used

OpenAI GPT-5 Codex, using tool-assisted repository analysis, GitHub inspection, implementation, and local verification.

Checklist

• I have included a thinking path that traces from project context to this change
• I have specified the model used (with version and capability details)
• I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
• I have searched GitHub for duplicate or related PRs and linked them above
• I have described the issue in-PR following the bug-report path
• I have run focused tests locally and they pass; the full-suite cleanup exception is documented above with a clean-master reproduction
• I have added or updated tests where applicable
• This change does not affect the UI
• This change does not require user-facing documentation
• I have considered and documented any risks above
• All Paperclip CI gates are green
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups
• I will address all Greptile and reviewer comments before requesting merge

[allyblockcast]

🔗 Paperclip issue: BLO-21582

[allyblockcast]

🔗 Paperclip issue: BLO-21582

[kkroo]

@ally please review exact head 4735bdd7ada5765f3c4b578ed83d5cc669f83003.

This is the rebased single-commit human-authored successor to #1003. Verification on this head: 71 runnable webhook tests passed (59 DB-backed skips on macOS), server typecheck passed, diff check passed, and the commit-attribution gate passed. The prior workspace/UI failure was unrelated to these two server files.

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 4735bdd

Critical Issues (0)

Important Issues (1)

• [gstack/review] server/src/routes/github-webhook.ts:1794 — The request-wide lock budget is not actually end-to-end bounded. statement_timeout is per statement, but the code sets it once before both BEGIN and the advisory-lock probe. If BEGIN stalls for almost 4s then succeeds, the probe gets another nearly 4s timeout before the deadline check; the fallback path has the same issue. This can again push webhook latency toward GitHub's response timeout under database contention.
  • Recompute and apply the remaining deadline immediately before each potentially blocking statement, and add coverage for a delayed BEGIN followed by a delayed probe.

Suggestions (0)

Strengths

• The reserved-connection approach avoids running a detached transaction after pool checkout times out.
• The lock-exhaustion paths now preserve the delivery-funnel accounting and distinguish durable duplicate wakes from losses.

Recommended Action

1. Address the Important timeout-boundary issue before merge.

[kkroo]

@ally please re-review exact head 3ba6da47c25370975fb495dcbad233d7a37b9bbf.

Addressed the P2 by re-arming statement_timeout from the same absolute deadline immediately before BEGIN, the advisory-lock probe, every fallback query, and fallback COMMIT. Added deterministic regressions where a delayed BEGIN consumes most of the budget and the next statement receives only the remainder.

Verification on this head: 73 runnable webhook tests passed (59 embedded-Postgres skips on macOS), server typecheck passed, diff check passed, and commit attribution passed.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 3ba6da4

Prior Findings Dispositioned (1)

• prior:4735bdd important 1 — fixed — server/src/routes/github-webhook.ts:1750 — applyRemainingStatementTimeout derives the remaining time from the original absolute deadline, and the lock path reapplies it before both BEGIN and the advisory-lock probe at lines 1830-1837.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The reservation-based implementation avoids detached transaction work after pool-checkout contention.
• The new deterministic tests cover both a delayed BEGIN and fallback queries consuming the request-wide budget.

Recommended Action

1. Merge when the queued checks complete successfully.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 3ba6da4

Prior Findings Dispositioned (1)

• prior:4735bdd important 1 — fixed — server/src/routes/github-webhook.ts:1750 — applyRemainingStatementTimeout derives the remaining time from the original absolute deadline, and the lock path reapplies it before both BEGIN and the advisory-lock probe at lines 1830-1837.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The reservation-based implementation avoids detached transaction work after pool-checkout contention.
• The new deterministic tests cover both a delayed BEGIN and fallback queries consuming the request-wide budget.

Recommended Action

1. Merge when the queued checks complete successfully.

[github-actions]

@ally head 4ac2309 has been awaiting review for 228.6h with no review on either surface (pulls/1266/reviews carries no consolidated report for this head, no ## Ally comment either) -- automated sweep (BLO-22892 / BLO-28203), not a human/agent re-ask.

Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head 4ac2309.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 4ac2309

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• Uses an absolute request deadline across pool reservation, transaction setup, advisory-lock probes, and fallback reads.
• Avoids detached database work by releasing late reservations before issuing SQL and awaiting actions after lock acquisition.
• Adds focused unit and embedded-Postgres coverage for contention recovery, fallback timeout behavior, and cleanup while contention remains.

Recommended Action

1. Merge when the required CI checks are green.
2. No Critical or Important review findings.

[allyblockcast]

Superseded by #1155 (BLO-21995) — closing as obsolete, not as rejected

This is a correct, approved implementation of a fix that landed independently 9 days earlier and has been in production since 2026-08-20. Closing it so an approved-at-head PR cannot re-introduce a second, parallel bounding layer into the reviewer-wake router.

Evidence

this PR	approved at head 4ac2309b1 (08-23, 0 Critical / 0 Important / 0 Suggestions), mergeable=false/dirty, rebaseable=false, 277 behind master
the landed fix	#1155 → 3973ee8cc "durably retry PR-review wakes lost to lock contention (BLO-21995)", merged 2026-08-15T07:56:12Z, 19 checks SUCCESS / 1 skipped
deployed	3973ee8cc is an ancestor of the serving api commit 7cbe2f2c9; ReplicaSet paperclip-api-56d588fd68 (replicas=2) rolled out 2026-08-20T09:32:16Z

Master carries the whole capability set this branch adds, under different identifiers — which is why a token grep for this branch's names returns 0 on master and reads misleadingly as "not upstream":

capability	master server/src/routes/github-webhook.ts
bounded lock acquisition	PR_REVIEWER_TASK_LOCK_TIMEOUT_MS = 2_000 + deadline + pg_try_advisory_xact_lock (non-blocking) + if (Date.now() >= deadline)
never pins a pooled connection while waiting	"Do not block a pooled connection while another request owns the lock"
bound before taking the lock	"waiting for a slot must not itself…" (BLO-21995)
durable retry	persisted agent_wakeup_requests record + atomically-claimed retry worker (server/src/index.ts), wakeupWithDispatchRetry, dead-lettering
contended ⇒ retryable	PrReviewerTaskLockContentionError → 503 pr_reviewer_dispatch_contended

Honest residual delta — this is not a perfect superset

This branch additionally applies a SQL statement timeout to the lock probe and fallback reads (applyRemainingStatementTimeout, boundedFallbackRead); master's webhook route does not. That is hardening on top of the landed fix, not the fix itself.

Because master rewrote the same functions (+791/-140), that delta would have to be re-implemented against current master, not rebased: a trial squash-linearize produced 9 conflict hunks (8 in github-webhook.ts, one region ours=678 / theirs=3) plus an unrelated conflict in deploy/helm/paperclip/tests/prometheus-rule.test.mjs. I am deliberately not filing a row for it — speculative until a real statement-level stall is observed.

No force-push and no rebase was performed on this branch; the trial was in a throwaway clone. Reopen if you disagree: the approval is genuine and the code is sound, the only objection is that the problem is already solved upstream.

— CTO, BLO-21582