fix: absorb concurrent-writer insert races on unique-key models#187
Merged
Conversation
analyzer.rebuild_queue_windows_sweep crashed in production with a duplicate-key IntegrityError on prqwin_pr_ruleset_from_unique: the sweep and analyzer.process_pr rebuilt the same PR's queue windows concurrently, both snapshotted the existing rows before either inserted, and the loser's bulk_create hit the unique constraint — aborting the entire sweep run. An audit found the same check-then-create race at seven other write sites. All now converge on the winner's row instead of raising: - queue_windows: bulk_create upsert (update_conflicts) keyed on (pull_request, rule_set, from_ts); create/update/delete wrapped in transaction.atomic so readers never see a half-rebuilt window set. - queue_window_build_state: same upsert on (pull_request, rule_set). - rebuild_queue_windows_sweep: contain per-PR IntegrityError (logged and reported as prs_conflict_skipped) so one conflict cannot poison the run; the skipped PR stays stale and is retried next sweep. - dependencies: get_or_create on the unique edge triple (per-PR task vs sweep). - labels_sync (both functions): get_or_create with name__iexact lookup so the conflict-retry get covers the (repository, lower(name)) constraint. - core_entities_sync.upsert_user_from_github: savepoint + IntegrityError re-resolve on the case-insensitive login constraint (any two concurrent PR syncs ingesting the same actor). - pull_request_sync.upsert_pull_request: savepoint + re-fetch on (repository, number) (live sync vs archive importer vs admin enqueues). - registration_linking: savepoint + re-resolve, falling through to the linking path when the syncer creates the same GitHub user concurrently. - reviewer_opt_out_backfill: get_or_create on (repository, pr_number, reviewer_login). Every fix has a dedicated race-path regression test that simulates losing the insert race (stale snapshot or forced get-miss against a pre-committed winner row). The get_or_create tests specifically pin that the lookup kwargs cover the unique constraint — the property Django's conflict-retry get relies on, and the easiest one to break in a future refactor. qb_site/AGENTS.md gains a "Concurrent Writers and Unique Keys" section codifying the approved patterns (with in-repo reference files), and the analyzer/syncer AGENTS.md files point to it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A deployed
analyzer.rebuild_queue_windows_sweeprun failed with a duplicate-key IntegrityError onprqwin_pr_ruleset_from_unique. Root cause: the sweep andanalyzer.process_prcan rebuild the same PR's queue windows concurrently (the sweep preferentially picks freshly-updated PRs which are exactly the ones process_pr is handling after a sync). Both snapshot the existing rows before either inserts, both compute the same new window, and the loser'sbulk_createhits the unique constraint. Worse, the uncaught exception aborted the entire sweep run, not just that PR.An audit of every write site against a unique-key model found the same check-then-create race in seven more places:
All now converge on the winner's row instead of raising, using the pattern that fits each site:
bulk_create(update_conflicts=...)upserts for the snapshot-diff rebuilds (now also wrapped intransaction.atomic()so readers never see a half-rebuilt window set),get_or_createkeyed exactly on the unique constraint (__iexactlookups for theLower()functional constraints),The sweep additionally contains per-PR IntegrityError (logged and reported as
prs_conflict_skippedin the task result) so a residual conflict skips one PR (which stays stale and is retried next sweep) instead of poisoning the run.Every fix has a dedicated race-path regression test that simulates losing the insert race against a pre-committed winner row (stale snapshot reads, or a forced get-miss so
get_or_create's create genuinely conflicts and Django's conflict-retry get must recover). The get_or_create tests deliberately conflict across casings where the constraint is onLower(...), pinning that the lookup kwargs cover the constraint — the property the retry relies on and the easiest one to silently break in a refactor.qb_site/AGENTS.mdgains a "Concurrent Writers and Unique Keys" section codifying the rule and the approved patterns with in-repo reference files; the analyzer and syncer AGENTS.md files point to it.Performance impact is negligible: Postgres already probes the unique index on every insert (ON CONFLICT just tells it what to do on a hit), and the new savepoints only fire on the entity-creation paths. Rollout needs no flags, migrations, or backfills; the previously failing sweep self-heals on its next run.
🤖 Generated with Claude Code