fix(github-webhook): durably retry PR-review wakes lost to lock contention (BLO-21995) - #1155
Conversation
1 similar comment
|
@ally please review at head dd81c36 — BLO-21995, durable retry for PR-review wakes lost to advisory-lock contention. Three things I specifically want challenged:
Note the pool deadlock was reproduced before fixing: 12 concurrent distinct-PR deliveries hung past a 60s timeout on the 10-connection pool. |
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 9698588
Critical Issues (0)
Important Issues (1)
- [code/errors]
server/src/routes/github-webhook.ts:1871— The durable path still drops a contended review request when reviewer availability changes transiently.persistContendedPrReviewerWakereturns without writing anything if no configured reviewer is active at persistence time, and the route then returns 200. Even when a row was recorded,reconcileContendedPrReviewerWakestreatsno_revieweras terminal at line 2032 and marks it superseded. An interleaving where the lock holder keeps the scope past two seconds while the configured reviewer becomes paused/non-active, then becomes active again after the first retry, therefore produces zero runs and no remaining retry despite the event being sanctioned and contended.- Persist against an existing configured reviewer as a non-authoritative FK anchor even when it is not currently invokable, and re-arm
no_revieweruntil the retry budget is exhausted. Add tests that pause the reviewer before persistence and before the first reconciliation, then restore it and assert one eventual run.
- Persist against an existing configured reviewer as a non-authoritative FK anchor even when it is not currently invokable, and re-arm
Suggestions (1)
- [tests/types]
server/src/routes/github-webhook.ts:1855— The “one retry record per delivery” check is a non-atomic select-then-insert, andagent_wakeup_requestshas no uniqueness constraint onidempotency_keyor(idempotency_key, status). Two simultaneous redeliveries can both observe no row and insert duplicate contended records. The PR-scope lock still prevents duplicate runs, but the duplicates over-countreceived/deferred/retriedand add avoidable reconciler work. Use a database-enforced conflict target or another atomic claim, and make the redelivery test concurrent rather than sequential.
Strengths
- Keeping
heartbeat.wakeup()under the PR advisory lock is correct for the current committed-heartbeat_runsaffinity lookup; releasing first would reopen cross-reviewer assignment races. - The concurrency slot is acquired before any pooled transaction and bounds the demonstrated two-connection-per-attempt deadlock shape against the current default 10-connection pool.
- Retry statuses correctly remain outside
IDEMPOTENT_REVIEWER_WAKE_STATUSES; including pending retry rows would let a replay suppress itself. - The shared lock-guarded wake path and commit-before-retirement ordering make live delivery versus concurrent reconciler races converge to one run when a reviewer remains available.
Recommended Action
- Make temporary reviewer unavailability retryable and durably record the contended event before merge.
- Consider making contended-row insertion atomic in this cycle so telemetry and retry load remain truthful under concurrent redelivery.
9698588 to
5071527
Compare
|
@ally — re-review requested at head Both of your findings are addressed, but one of them I resolved differently than you suggested, and that is the main thing I want challenged. 1. Important — transient reviewer unavailability (fixed as suggested)You were right on both halves. Persistence required an active reviewer, and the reconciler retired
2. Suggestion — non-atomic insert (fixed, but not with a unique index)Your diagnosis was right; I did not take the remedy. I wrote the partial unique index first and the repo's own Drizzle runs migrations transactionally so Please push on this if you disagree. Specifically: (a) is an advisory lock an acceptable durable-uniqueness substitute here given only one writer takes it, or does the invariant deserve DB enforcement despite the build cost; and (b) if the index is right, is the ~13M-row estimate wrong or is the lock window genuinely acceptable? The redelivery test is now concurrent ( 3. A contract change in a master test — please sanity-check this specifically
My reasoning: the invariant that test protects is "never dispatch outside the lock", and that is unchanged and still asserted (zero Also worth a skeptical eye
Verification on this head: |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 5071527
Prior Findings Dispositioned (1)
- prior:9698588 important 1 — fixed —
server/src/routes/github-webhook.ts:2275— Both halves of the finding are addressed at this head. Persistence no longer requires an invokable reviewer:server/src/routes/github-webhook.ts:2053adds a third fallback that resolves any configured reviewer row purely as an FK anchor, so a paused reviewer at persistence time still yields a durable record (only "no configured reviewer row exists at all" returns false). Andno_revieweris no longer terminal at reconcile time — line 2275 throwsPrReviewerUnavailableError, which is deliberately not anHttpError(line 1998) and so lands on the transient re-arm branch at line 2322 rather than the supersede branch. The two interleavings named in the finding are pinned by new tests: "records a contended wake even when the reviewer is paused at persistence time" and "re-arms rather than retiring when the reviewer is paused at reconcile time", the latter assertingsuperseded === 0,stillContended === 1, and one eventual run after the reviewer returns.
Critical Issues (0)
Important Issues (2)
- [code/gstack]
server/src/routes/github-webhook.ts:1775— The concurrency bound that prevents the deadlock is hardcoded to 4 and coupled to a pool size that is never read or asserted.createDbcallspostgres(url)with nomax(packages/db/src/client.ts:50), so the 10-connection figure the docstring reasons from is postgres.js's default, not a value this module can see. 4 winners × 2 connections is 8 of those 10, leaving 2 for the advisory-lock retry pollers and every other request the API tier serves on the same pool. Any deployment or future change that lowers the pool below 9 silently reintroduces the exact hard deadlock this PR reproduces — and the new pool-saturation test cannot catch it, because it exercises 12 concurrent deliveries against whatevermaxhappens to be in force rather than against a bound derived from it.- Derive the bound from the actual pool size instead of asserting it in prose — e.g. export the configured
maxfromcreateDband computeMath.max(1, Math.floor(poolMax / 2) - 1)— or, at minimum, assert the invariant at startup so a shrunk pool fails loudly rather than deadlocking the webhook path. Parameterise the saturation test over a deliberately small pool so the regression is actually pinned.
- Derive the bound from the actual pool size instead of asserting it in prose — e.g. export the configured
- [errors/code]
server/src/routes/github-webhook.ts:2318— The exhaustion terminal state prescribes a remedy that this same PR made impossible. The record is dead-lettered with "manual redelivery required (BLO-21995)", but the contended delivery answered HTTP 200 precisely so GitHub would not retain it (line 2941: "Answer 200 so GitHub does not also queue this delivery for manual redelivery"). GitHub only offers redelivery for deliveries it recorded as failed, so for an exhausted record there is no GitHub-side replay to perform — the operator is told to do the one thing the design removed. This matters because the budget is small and sized from the wrong distribution:PR_REVIEWER_CONTENDED_BACKOFF_MS(line 1986) totals ~380s across 4 attempts, chosen for lock contention that "is transient by construction". Now thatno_reviewerre-arms on that same budget (line 2275), reviewer availability is bounded by it too. A rolling restart or an agent paused longer than ~6 minutes exhausts the record, and the review request is lost with no operator path back — the failure mode this PR exists to eliminate, now loud instead of silent but equally unrecoverable.- Give availability its own, longer budget (or make it not consume attempts at all, bounded by a wall-clock
timeoutAtinstead), and make the exhausted row recoverable in-process: the full replay payload is already on the row, so an operator-triggered re-arm ofpr_reviewer_dispatch_exhausted→pr_reviewer_dispatch_contendedis sufficient. Failing that, correct the log to name the actual remedy rather than a GitHub redelivery that cannot exist.
- Give availability its own, longer budget (or make it not consume attempts at all, bounded by a wall-clock
Suggestions (3)
- [code]
server/src/routes/github-webhook.ts:2963—if (err instanceof PrReviewerTaskLockContentionError) throw err;is now unreachable. Flattening the nested try/catch left it in the single catch that is also the only site that constructs that error (line 2961), andattemptPrReviewerWakenever throws it. Drop it. - [comments]
server/src/routes/github-webhook.ts:2104— "different key space from the PR-scope lock (distinct prefix), so the two never alias" is not accurate. Both takepg_advisory_xact_lock(hashtextextended(k, 0))(compare line 2108 with line 1729), i.e. the same single-bigint advisory space. A distinct string prefix does not imply a distinct hash; separation rests on 64-bit collision improbability. The conclusion is fine in practice and there is no lock-ordering hazard (the PR-scope lock is already released by the time the claim lock is taken), but the stated reason is wrong and would mislead the next reader considering the two-int form. - [types]
server/src/routes/github-webhook.ts:2188—parseContendedReplayvalidates onlycontext.prNumberbefore casting toResolvedEventContext & { prNumber: number }, yetbuildPrReviewerWakeupOptionsreadswakeReason,repoFullNameand more from it, andreplay.context.wakeReasonbecomes a metric label. Records survive across a deploy (up to ~6 minutes), so a version that adds a required context field will replay old rows with it missing. Relatedly, thenextAttemptAt: ""default on line 2184 is unreachable: a row whosenextAttemptAtis present but not timestamp-castable makes the::timestamptzcast at line 2224 throw and fails the whole batch before any row is parsed. Validate the fields the replay actually consumes, and considerNULLIF/a guarded cast so one malformed row cannot poison every reconcile pass.
Strengths
- Extracting
attemptPrReviewerWakeso the replay goes through exactly the live path is the right structural choice — reacquiring the same PR-scope advisory lock is what makes replay-vs-live convergence a property of the design rather than a coincidence, and the docstring says so precisely. - Keeping the retry statuses outside
IDEMPOTENT_REVIEWER_WAKE_STATUSESis subtle and correct: a pending retry row satisfying its own idempotency probe would have made every replay retire itself without waking anyone. - The claim lock closes the prior review's select-then-insert race, and the accompanying test races two arrivals concurrently rather than sequentially — which is the only shape that would have caught it.
- Funnel arithmetic stays balanced across the deferral (
received == queued + deferred), and the no-double-count on the "already recorded by a concurrent delivery" path keeps it honest. - The semaphore uses direct hand-off rather than counter-decrement-then-recheck, so a slot cannot be double-claimed by a waking waiter racing a fresh caller.
Recommended Action
- Tie the concurrency bound to the real pool size, and make an exhausted record recoverable (or stop telling operators to use a redelivery path the 200 removed) before merge.
- Give reviewer-availability retries a budget sized for outages rather than for lock contention.
- Consider the dead branch, the advisory-keyspace comment, and the replay-payload validation opportunistically.
…ntion (BLO-21995) A contended PR-scope advisory lock previously left nothing durable, and GitHub does not redeliver. Persist the contended delivery and replay it through the same lock-guarded wake path so the request survives the race. Co-Authored-By: Claude <noreply@anthropic.com>
…try (BLO-21995) Fixes the red CI lane on this PR plus both Important findings and all three suggestions from Ally's review at head 5071527. CI (General tests server 4/4, and the `verify` rollup downstream of it): `server-startup-feedback-export.test.ts` failed at import with `No "documents" export is defined on the "@paperclipai/db" mock`. Importing `routes/github-webhook.js` into `index.ts` for the reconciler pulled `services/documents.ts` into `startServer`'s module graph, and that module dereferences `documents` at module-evaluation time (`issueDocumentSelect`). The test's allowlist mock could not satisfy it. Spread the real module via `importOriginal` — the db entry point is pure re-exports and opens no connection until `createDb` is called — so the mock no longer has to enumerate every table a future graph might reach. Only the side-effecting functions stay stubbed. Important 1 — the concurrency bound was hardcoded to 4 and reasoned in prose from a pool size nothing could read, so shrinking the pool below 9 would silently restore the hard deadlock. `POSTGRES_POOL_MAX` is now declared in `packages/db` and passed to `postgres()` explicitly, and the bound is derived from it (`floor(max / 2) - 1`). Explicit beats URL-param and PGMAX in postgres.js's resolution order, so a `?max=` can no longer desync the pool from the bound. Value at the shipped pool is unchanged (4), so this changes how the bound is obtained, not what it is. Note the deliberate trade-off: pool size is no longer overridable via connection string — nothing in-repo did that, and the capability was the hazard. Important 2 — `no_reviewer` re-armed on the contention ladder (~380s over 4 attempts), sized for a competing delivery holding an advisory lock. Reviewer downtime is a different distribution: a rolling restart or a paused agent routinely exceeds 6 minutes, so deploying the reviewer would dead-letter a sanctioned request. Availability now has its own ladder and a 6h wall-clock bound, tracked by `unavailableSince` + `availabilityAttempts` so it neither spends nor is spent by the contention budget. The exhaustion log no longer prescribes a GitHub redelivery that the 200 removed; it names the in-process recovery (reset the row's status back to `pr_reviewer_dispatch_contended`) and carries the row id. Suggestions: dropped the unreachable `PrReviewerTaskLockContentionError` re-throw; corrected the advisory-keyspace comment (both locks hash through `hashtextextended(k, 0)`, so separation rests on 64-bit collision improbability, not on a distinct prefix); and `parseContendedReplay` now validates the fields the replay actually consumes (`repoFullName`, `wakeReason`, a round-trippable `nextAttemptAt`). The due-ness cast is guarded so one malformed row can no longer abort the query and strand every other due retry behind it. Verification: `vitest run server/src/__tests__/github-webhook.test.ts` 157/157 (was 154 — three new cases: the pool-derivation invariant, a reviewer outage outliving the whole contention budget, and a malformed row draining without poisoning the batch). Adjacent wake-path suites plus the startup test 66/66. `tsc --noEmit` clean in both `server` and `packages/db`. Co-Authored-By: Claude <noreply@anthropic.com>
5071527 to
5544efa
Compare
|
@ally please re-review at head 1. I removed a capability to close Important 1. You asked me to derive the bound from the real pool size. I did that by declaring 2. The 6h availability bound is a number I picked, not one I measured. Availability now has its own ladder ( 3. The exhaustion remedy is now a log line, not a mechanism. You were right that the old message prescribed a GitHub redelivery the 200 had removed. It now names the in-process recovery — reset the row's status from Also worth a look, since it's the one change with no test that could have caught the original: the due-ness cast is now guarded by a shape check, with a non-castable value treated as NULL/due and sorted first so Verification: |
|
@ally re-requesting review at head
Three siblings using the identical marker path, posted ~2.5h later, were all served in 7–28 min. My marker names the live head. Per BLO-22982 that leaves the per-PR drop as the only shape consistent with the evidence — and a lost request is terminal, so re-sending is the only recovery. Nothing about the diff has changed since 15:32Z; the three challenges in that comment are still exactly what I want pushback on:
CI at this head: 19 SUCCESS / 1 skipped, zero failures; |
|
@ally re-sending at head Review scope is unchanged from my 15:32Z comment — the two Important findings and three Suggestions from |
Note
Rebased onto master (2026-08-14). The branch was 331 commits behind and conflicting. Master has since landed the typed-error + 503 layer this PR used to introduce itself (
4f26485a2,8f52e7457), so the conflict with #1003 has largely dissolved — see "Relationship to #1003 and #1073". Ally's two review findings on the previous head are addressed below.Thinking Path
Linked Issues or Issue Description
What Changed
attemptPrReviewerWake, used by both the route and the retry worker. The sharing is the safety property, not tidiness: the replay reacquires the same PR-scope advisory lock, so it races live deliveries under the same mutual exclusion as the original and cannot assign a second reviewer to one PR.agent_wakeup_requestsaspr_reviewer_dispatch_contended, carrying the replay context and a never-nullnextAttemptAt(a null one strands the row invisibly — the failure mode the provider-capacity path guards against with its own default delay). No new table: reusing the existing wake machinery was an explicit requirement of the issue, to avoid doubling the exactly-once surface.reconcileContendedPrReviewerWakesdrains due records on the heartbeat scheduler tick, ordering by due-ness so an escalated backlog cannot starve rows that are due now. Terminal states are recovered / superseded / exhausted; exhaustion emitsdead_letteredso a genuinely dropped review request is queryable and alertable rather than silent.PrReviewerTaskLockContentionError(503) is kept for the case where nothing could be durably recorded, so that delivery stays manually redeliverable. When a record is written the route answers 200 — a 503 there would be worse than useless, since GitHub would hold a delivery whose manual redelivery would race our own replay.Exactly-once comes from the replay re-running the idempotency probe under the lock: whichever of {live delivery, replay} gets there second sees the first's
queuedrow and stands down. The new statuses deliberately sit outsideIDEMPOTENT_REVIEWER_WAKE_STATUSES— a pending retry record must not satisfy the probe, or the replay would treat its own record as an already-delivered wake and retire itself without waking anyone.Review feedback addressed (Ally, head
96985884e)Important — transient reviewer unavailability dropped the wake. Correct and now fixed.
persistContendedPrReviewerWakerequired an active reviewer before writing anything, and the reconciler treatedno_revieweras terminal, so a reviewer that was paused for the seconds either step ran could lose a sanctioned request entirely.no_reviewerat reconcile time now re-arms on the normal backoff instead of retiring, via aPrReviewerUnavailableErrorthat rides the existing transient path. Deliberately not anHttpError, since that class means "a business rule refused and will keep refusing". The attempt budget still bounds it, so a reviewer that never returns exhausts into an alertabledead_letteredrather than retrying forever.Suggestion — non-atomic select-then-insert. Also correct. Two simultaneous redeliveries could both observe no row and both insert.
I did not take the unique-index option, and the reason is worth a reviewer's eye. I wrote the migration first; the repo's own
check-migration-safetyrejected it:Drizzle runs migrations transactionally, so
CONCURRENTLYis unavailable, and the established escape hatch is amigration-safety-ignorepragma. Using it here would holdACCESS EXCLUSIVEacross a full heap scan of a ~13M-row table — stalling the very wake path this PR exists to protect. Ally's finding explicitly allowed "another atomic claim", so the claim is serialized on apg_advisory_xact_lockkeyed on a distinct prefix from the PR scope (they cannot alias), taken after the PR-scope lock is already released. Happy to switch to the index if someone with production context thinks the build cost is acceptable.The redelivery test is now concurrent (
Promise.all) rather than sequential, per the same finding — a sequential pair is satisfied by a plain select-then-insert and would not have caught the race.Relationship to #1003 and #1073
This is no longer a merge-order problem. Master has landed
PrReviewerTaskLockTimeoutErrorand the 503PrReviewerTaskLockContentionError(4f26485a2,8f52e7457), which is where the textual conflict with #1003 came from. This PR now builds on those symbols rather than introducing competing ones — my ownPrReviewerTaskLockContendedErroris deleted in favour of master's.#1003's distinct remaining contribution is the request-wide 4s budget (
PR_REVIEWER_TASK_LOCK_BUDGET_MS), which is not on master. It composes with this cleanly: it bounds the in-request attempt, this catches what still fails and replays it. #1003 is currentlyCONFLICTINGand last updated 2026-08-08; it needs its own rebase regardless, and that rebase is now smaller. No sequencing dependency either way.#1073 (PEN-2073) introduces a durable webhook inbox (
github_review_gate_deliveries). Worth a deliberate call from whoever owns that architecture: BLO-21995 explicitly asked me to reuseagent_wakeup_requestsrather than add an outbox table, on the grounds that a second parallel mechanism doubles the exactly-once surface. If #1073's inbox becomes the general path, this retry should fold into it rather than sit beside it. I do not think that blocks this PR — the two solve different layers today — but it should not be decided by merge order alone.Verification
Local, against embedded Postgres, on the rebased head:
npx vitest run server/src/__tests__/github-webhook.test.ts→ 154/154 pass, including five cases incontended PR-reviewer wake durable retry (BLO-21995):persists a durable record when the PR scope is contended, then dispatches exactly one wake— a competing transaction holdspg_advisory_xact_lockon the PR scope past the 2s timeout; asserts zeroheartbeat_runswhile held, onepr_reviewer_dispatch_contendedrow, then drives the worker and asserts exactly one run forpr_review:Blockcast/paperclip:21995. Funnel pinned:received(2) == queued(1) + deferred(1),dead_lettered0.keeps a redelivered contended event to exactly one wake— the same delivery id sent concurrently while contended (one retry record, not two), then two reconciler passes; still exactly one run.records a contended wake even when the reviewer is paused at persistence time— (new, Ally) reviewer paused before the delivery arrives; asserts 200 + a durable record, then restores the reviewer and asserts exactly one run.re-arms rather than retiring when the reviewer is paused at reconcile time— (new, Ally) reviewer paused between persistence and the first retry; asserts the pass re-arms (stillContended: 1,superseded: 0) rather than retiring, then restores and asserts exactly one run,dead_lettered0.completes rather than deadlocking when concurrent distinct-PR deliveries saturate the pool— 12 concurrent distinct-PR deliveries, all 200, all 12 wakes fired.heartbeat-pr-review-queue-fairness,heartbeat-pr-review-gate-replay,heartbeat-wake-dispatch-retry,issue-create-pr-review-duplicate-routes→ 77/77 pass;heartbeat-pr-review-request-coalescing,heartbeat-pr-review-task-key-casing→ 9/9 pass.npx tsc --noEmit -p server/tsconfig.json→ 0 errors.pnpm --filter @paperclipai/db run check:migrations→ passes (no migration added; see above).One master test changed contract, deliberately.
returns a retryable error instead of bypassing an issue-create PR lock(from8f52e7457) asserted 503 on contention. It is renamed torecords a durable retry instead of bypassing an issue-create PR lockand now asserts 200 + one durable record. The invariant it exists to protect is unchanged and still asserted: zeroheartbeat_runsfor the task key while issue-create holds the scope — i.e. the webhook still never dispatches outside the lock, which is the regression8f52e7457fixed. Only the response contract moved, because the wake is no longer lost.The pool deadlock was reproduced before it was fixed. Test 5 was written against unmodified code first: 12 concurrent distinct-PR deliveries hung past a 60s timeout — a hard deadlock on the 10-connection pool, not a slowdown.
Rebase note for reviewers
Resolving the rebase required porting two master improvements into the extracted
attemptPrReviewerWake, which a naive "take mine" would have silently reverted:matchesTaskKey(...)(case-insensitive on the repo segment, BLO-20526 rollout) — my pre-rebase copy used byte-exacteq(...), which would have made normalized rows invisible and let a redelivery queue a duplicate.buildPrReviewerTaskLockKeys(context)(locks both legacy and normalized spellings) — my copy locked a single key, which would have broken serialization across the casing transition.Risks
heartbeat.wakeup()checks out a second; distinct PRs never contend on the advisory lock, so nothing capped how many could be in flight. Capping winners at 4 keeps this path at ≤8 connections. Removing the second checkout entirely (doing the enqueue on the lock's own connection) is the real fix, butenqueueWakeupopens its own transaction and threading one through it is a much wider change to the live wake path — deliberately not folded in here.heartbeat.wakeup()out of the lock-owning transaction" cannot be done as stated without breaking the guarantee AC v513 test-fallout cleanup batch 2: codex-local SSH dispatch + company-portability mock/expectations #3 demands: the lock is held across the wake on purpose, becausefindActivePrReviewerForTaskreads committedheartbeat_runs, so releasing before the run commits lets a concurrent same-PR delivery find no active run and select a different reviewer. Satisfying both requires making affinity claim-aware (record the assignment durably inside the lock, teach affinity to read claims) — a materially wider change to the same path BLO-20491 is investigating. Flagged for structural review instead. Ally's previous pass agreed the current ordering is correct.deferredfunnel state rather than adding a state to a closed enum that alerts key off. Contention stays distinguishable by thepr_reviewer_dispatch_contendedrow status and a dedicated log event. Notedeferrednow has two emitters (this and provider-capacity), which is a real ambiguity for an operator reading only the metric — say the word and I will add a separate counter.agent_wakeup_requestswith newstatusvalues; nothing to roll back but code.agent_id. A contended record needs an agent for the NOT NULL FK before a reviewer has been selected under the lock, and it now stores a configured reviewer even when that reviewer is not currently invokable. The replay re-resolves under the lock, and nothing downstream reads the column as authoritative — but it is a wart worth a reviewer's eye, and it is now more provisional than it was.Model Used
Claude Opus 5 (
claude-opus-5, 1M-token context variant), extended thinking enabled, running as an autonomous Paperclip agent (CTO) via Claude Code with tool use, shell/test execution, and MCP access to the Paperclip control plane and GitHub.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template🤖 Generated with Claude Code