Repository navigation
fix(code-reviews): serialize webhook create and retrigger races - #6180
pandemicsyn wants to merge 1 commit into
Conversation
Webhook review creation and retrigger could both violate the single-active-provider-publisher partial unique index (UQ_cloud_agent_code_reviews_active_provider_publisher) with Postgres 23505. - GitHub webhook create supersedes and inserts inside a per-change advisory lock with ON CONFLICT DO NOTHING and a winner fallback, so overlapping deliveries cannot both insert an active review. - resetCodeReviewForRetry refuses to revive a terminal review while another active provider review holds the same integration/repo/PR slot, and maps a residual unique violation to a conflict instead of throwing. - Check run attachment is status-aware (attachCheckRunIdIfActive, plus a merge-commit repoint CAS): when the review has already left the active set the newly created external check is cancelled instead of being attached to a cancelled row. Cancel queries report the post-update check id so a supersede that blocks behind an in-flight attach still tears the check down. Scoped to the GitHub webhook and review lifecycle path. GitLab/Bitbucket create paths and the retrigger gate re-check remain unchanged follow-ups. Tests: focused code-review DB and GitHub pull-request handler suites, plus typecheck, lint, and formatting.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| const winner = | ||
| (await findExistingReviewWithDatabase(tx, scope, params.headSha)) ?? | ||
| (params.platformIntegrationId | ||
| ? await findActiveProviderPublishingReviewWithDatabase(tx, { |
There was a problem hiding this comment.
WARNING: The surviving-conflict assumption does not hold under READ COMMITTED, so a newer push can be silently dropped
The comment above asserts a surviving conflict must be a manual provider review, but cancelReviewsForPR is a separate statement with its own snapshot. A concurrent resetCodeReviewForRetry — which does not take codeReviewWebhookLockKey — can commit between that cancel and this ON CONFLICT DO NOTHING insert, reviving an existing failed/cancelled review for an older SHA to pending. The insert then skips, findExistingReviewWithDatabase misses (different head_sha), and this lookup returns the just-revived older-SHA review as the winner. The caller's !creation.created branch then returns 200 A code review is already active for this pull request, so GitHub treats the delivery as successful and does not redeliver, while the new head SHA is never reviewed. The pre-insert cancel would have superseded that review had the reset committed before it. Consider re-running the cancel and retrying the insert when the winner's head SHA differs from params.headSha, or serializing the reset path on the same advisory lock.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| prNumber: params.prNumber, | ||
| }) | ||
| : null); | ||
| if (!winner) throw new Error('Code review webhook conflict winner not found'); |
There was a problem hiding this comment.
WARNING: A resolvable conflict can still surface here as an unhandled 500
findExistingReviewWithDatabase is owner- and platform-scoped (reviewScopeConditions), and findActiveProviderPublishingReviewWithDatabase only matches active, provider-publishing rows. A conflict on UQ_cloud_agent_code_reviews_webhook_integration_repo_pr_sha can satisfy neither lookup: e.g. a terminal same-integration/repo/PR/SHA row whose owner no longer matches (owner is derived from integration.owned_by_organization_id / owned_by_user_id) is invisible to both, so winner is null and this throws. That yields a webhook 500 plus a Sentry event on every redelivery for that SHA — the failure class this PR set out to remove. The sibling createCodeReviewIfAbsentInTransaction (lines 1754-1756) already handles exactly this with the owner/platform-agnostic findReviewByLegacyUniqueKeyWithDatabase; adding it as a third fallback keeps the conflict resolvable.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| // The review was cancelled or superseded after the new gate was created; | ||
| // tear it down rather than leaving a queued check on the merge commit. | ||
| await cancelMergeCommitCheckRun(newCheckRunId); | ||
| logExceptInTest( |
There was a problem hiding this comment.
SUGGESTION: This log reports a cancellation even when the cancel failed
cancelMergeCommitCheckRun (line 652) swallows updateCheckRun failures, so execution reaches this line unconditionally and logs cancelled check run … for a gate that may still be queued on the merge commit — the exact branch-protection block this branch is trying to avoid. Return a success flag from the helper, or move the log next to the updateCheckRun call inside the try.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 3 Issues Found | Recommendation: Address before merge Overview
Fix these issues in Kilo Cloud Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (4 files)
Reviewed by deepseek-v4.1-flash · Input: 0 · Output: 0 · Cached: 0 Review guidance: REVIEW.md from base branch |
1 participant
Summary
Fixes a class of Postgres
23505unique-violation errors on theUQ_cloud_agent_code_reviews_active_provider_publisherpartial index (single-active-provider-publisher invariant), reported as Sentry issue KILOCODE-WEB-279B. Scoped to the GitHub webhook/lifecycle path.Two paths treated a legitimate invariant conflict as an unhandled exception: the webhook create path ran supersede, same-SHA lookup, and a plain
INSERTas separate autocommit statements with no serialization; andresetCodeReviewForRetryset a terminal review back topendingwithout checking whether another active provider-publishing review already held the same integration/repo/PR slot.createCodeReviewForWebhookperforms the same-SHA re-check, supersede, andON CONFLICT DO NOTHINGinsert inside a per-changepg_advisory_xact_lock, resolving the winner instead of raising. The unique index remains the source of truth.resetCodeReviewForRetrymirrors the index predicate and refuses the reset while another active provider review holds the slot; a residual unique violation is mapped to a conflict result.attachCheckRunIdIfActive(plus a merge-commit repoint CAS) attaches the GitHub check id only while the review is still active. If the review was superseded, the newly created external check is cancelled and the handler returns without reacting or dispatching. Raw cancel queries project the post-updatecheck_run_idso a supersede that blocks behind an in-flight attach still tears the check down.Verification
No manual validation was performed. The DB concurrency behavior cannot be exercised by hand reliably; it is covered by automated tests that reproduce competing webhook creation, concurrent retrigger resets, and the check-run attach/cancel ordering (exact commands and results are in the completion report).
Visual Changes
N/A
Reviewer Notes
code-reviews.ts. This PR's review create/retry race fix is not supplied by that stack: when combining, keep fix(github): fix connect/disconnect lifecycle bugs found in live org testing #6100's owner-aware, transaction-owned disconnect/uninstall cancellation, reservation clearing, and post-commit ledger settlement together with this PR's post-updatecheck_run_idRETURNING and guarded check attachment.