Use advisory lock to prevent concurrent inserts in maven registry
Context
Some users reported a case of race condition when uploading packages to the Maven Repository. With recent versions of maven, uploading the artifacts can happen in parallel, and this could cause a race condition that results in an error and aborts the package upload.
The challenge with maven packages upload is that the Maven Repository doesn't receive the package as a whole, instead it receives it as multiple files. When it receives the 1st file, it creates a package and attaches this file to it. Starting the 2nd received file, the Maven Repository searches for the existing package that was created when it received the 1st file, and attaches the remaining files to it.
When those files are sent concurrently, then the backend can do a couple of things wrongly:
- it treats the files as if they are the 1st one (they are sent at the same time), and it tries to create a new package for each. So we could end up having duplicate packages.
- it could create a new package for one file. But for another racing file, it would fail to find the created package that this file should be attached to, and then tries to create a new package for it. This trial could fail because of this uniqueness model validation. In this case, an error would be returned (bad request - name is already taken), and the file wouldn't be uploaded.
To ultimately fix this race condition, we should have a database unique index on the name, version & project_id for the maven packages. Unfortunately, we don't have such an index. I tried to evaluate the possible solutions to the issue here: #424238 (comment 2182343756).
In Use exclusive lease in maven FindOrCreatePackag... (!170916 - merged), we tried to use an exclusive lease to prevent the concurrent uploads. But after enabling the feature flag, the error rate was a bit high. We then decided to disable the feature flag, and use a different approach: the advisory lock, as described in this guide.
What does this MR do and why?
- Add a transaction-level advisory lock to prevent creating duplicate records.
- Combine the lock with the check existence and rescue to satisfy any concurrent uploads.
- Put the advisory lock behind the same feature flag introduced in Use exclusive lease in maven FindOrCreatePackag... (!170916 - merged).
- Cover the changes with tests.
References
Please include cross links to any resources that are relevant to this MR This will give reviewers and future readers helpful context to give an efficient review of the changes introduced.
- Database race condition when uploading maven pa... (#424238 - closed)
- Use exclusive lease in maven FindOrCreatePackag... (!170916 - merged)
MR acceptance checklist
Please evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.
Screenshots or screen recordings
Screenshots are required for UI changes, and strongly recommended for all other merge requests.
| Before | After |
|---|---|
How to set up and validate locally
Call the method below in rails console. On this branch, only one thread will create one package, and all other racing threads will return that created package.
On master, calling this method will create duplicate maven packages in the database for some threads, while the others could get the name is already taken error, which means that the model validation was able to prevent the duplication, but also means that the race condition caused a failure in finding the already created package (created by another racing thread)
def simulate_mvn_race_condition
user = User.first
project = Project.first
params = {
path: 'foo/bar/mypkg/1.9.5',
file_name: 'my-app-1.9.5.jar'
}
wait_for_it = true
threads = Array.new(10) do
Thread.new do
true while wait_for_it
Packages::Maven::FindOrCreatePackageService.new(project, user, params).execute
end
end
wait_for_it = false
threads.map(&:join).map(&:value)
endRelated to #424238 (closed)