Fix build metadata race condition #9756
Merged
Conversation
This file contains 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
This shall be filled by the following SQL script:
```sql
update versions
set num_no_build = split_part(num, '+', 1);
with duplicates as (
-- find all versions that have the same `crate_id` and `unique_num`
select crate_id, num_no_build, array_agg(num ORDER BY id) as nums
from versions
group by crate_id, num_no_build
having count(*) > 1
),
duplicates_to_update as (
-- for each group of duplicates, update all versions except the one that
-- doesn't have "build metadata", or the first one that was published if
-- all versions have "build metadata"
select crate_id, num_no_build, unnest(case when array_position(nums, num_no_build) IS NOT NULL then array_remove(nums, num_no_build) else nums[2:] end) as num
from duplicates
)
update versions
set num_no_build = duplicates_to_update.num
from duplicates_to_update
where versions.crate_id = duplicates_to_update.crate_id
and versions.num = duplicates_to_update.num;
```
The script takes a few seconds to complete, so this should not be added to the database schema migration script, since it would block the API server from booting.
… already exists" response Previously, it was theoretically possible to publish two versions with the same base version number, but different build metadata, at the same time due to a race condition between the check and the insert. This commit fixes the issue by instead relying on the database uniqueness error to build the appropriate response.
…ller code The model layer is not supposed to know anything about HTTP API errors...
Turbo87
changed the title
versions.num_no_build column
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9756 +/- ##
=======================================
Coverage ? 88.81%
=======================================
Files ? 289
Lines ? 29825
Branches ? 0
=======================================
Hits ? 26488
Misses ? 3337
Partials ? 0 ☔ View full report in Codecov by Sentry. |
This was referenced Oct 27, 2024
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.
This PR fixes a race condition in our publish endpoint. When two versions,
1.0.0+fooand1.0.0+barare published concurrently we previously could get into a situation where the "unique version number excluding build metadata" check succeeds for both, and then both versions are added to the database, when only one such version should exist.This PR adds a new
uniqueconstraint on(crate_id, num_no_build)to prevent this from happening.We currently have around 650 such cases still in the database though. In those cases the
num_no_buildcolumn is adjusted to actually contain the build metadata too (see column comment).Finally, this PR needs to be merged/deployed in multiple stages. The initial migration adds the column in a nullable way, because the SQL script to backfill the data for the column takes about 30 sec (on my machine), and this shouldn't prevent the API server from booting up because of a running schema migration. It is recommended to merge/deploy each migration in this PR independently!