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?

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.

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)
end

Related to #424238 (closed)

Edited by Moaz Khalifa

Merge request reports

Loading
Loading