fix(locations): tolerate concurrent creation in bulk_get_or_create - #15845
Open
Maffooch wants to merge 1 commit into
Open
fix(locations): tolerate concurrent creation in bulk_get_or_create#15845Maffooch wants to merge 1 commit into
Maffooch wants to merge 1 commit into
Conversation
Two scan imports that reference the same package or endpoint race to create the same location row. identity_hash is a global singleton with a unique constraint, so when one import commits the row between another import's existence check and its bulk INSERT, the second import's subtype bulk_create raised IntegrityError (duplicate key value violates unique constraint "<subtype>_identity_hash_key"). That exception propagated out of the whole persist() and aborted the entire scan import, failing the async import task. The previous code deliberately let the transaction roll back on this race. Instead, create the parent Location and subtype rows together in a savepoint; on a unique-constraint collision the savepoint rolls back (parents included, so no orphaned Location rows), then re-resolve the rows the concurrent writer committed, carry over their association data, and retry only the rows still missing. The enclosing transaction stays usable throughout, so a routine collision no longer aborts the import. Add a regression test that hides a pre-existing row from the first existence lookup so the code hits the real DB unique constraint, and asserts recovery: the raced row resolves to the original (no duplicate), the genuinely-new row is still created, and no orphaned parent Location rows are left behind. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014K826EST31JnjV4xGv6jRf
6 tasks
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
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.
Description
A scan import could abort with a database
IntegrityErrorwhen a second import created the same location row concurrently.AbstractLocation.bulk_get_or_createlooks up existing rows byidentity_hash, then bulk-inserts the ones it didn't find.identity_hashis a global singleton with a unique constraint, so when two imports reference the same package/endpoint, one import can commit the row in the window between the other's existence check and itsbulk_create. The second import's subtypebulk_createthen raised:That exception propagated out of the location manager's
persist()and failed the entire async scan-import task. The previous code deliberately let the transaction roll back on this race, which is exactly what turned a routine collision into a failed import.Fix
Create the parent
Locationrows and their subtype rows together inside a savepoint. On a unique-constraint collision the savepoint rolls back — parentLocationrows included, so nothing is orphaned — and the enclosing transaction stays usable. The code then re-resolves the rows the concurrent writer committed, carries over their association data, and retries only the rows still missing (bounded retry). A routine collision no longer aborts the import; it resolves to the existing row and continues.No schema change and no migration: the fix is purely in the create/recover logic, and the existing unique constraint is what makes the recovery correct.
This lives on
AbstractLocation, so every location subtype (URL and the ProDependency/CodeLocationcompanions that inherit it) gets the same race tolerance. Pro does not overridebulk_get_or_create, so no companion change is required for this path.Test results
Added a regression test in
unittests/test_bulk_locations.pythat reproduces the race deterministically: it hides a pre-existing row from the first existence lookup only, so the code attempts a duplicate INSERT and hits the real database unique constraint, then asserts recovery — the raced row resolves to the original (no duplicate created), the genuinely-new row in the same batch is still created, and no orphaned parentLocationrows are left behind. The test fails on the previous code (uncaughtIntegrityError) and passes with the fix.Note: this environment has no Docker daemon or Postgres, so the suite was validated through CI rather than locally.
Documentation
No user-facing behavior or documentation change; internal import-robustness fix only.
Checklist
bugfixbranch.🤖 Generated with Claude Code
https://claude.ai/code/session_014K826EST31JnjV4xGv6jRf
Generated by Claude Code