Skip to content

Fix race condition of `refs/merge` competing overwrite

Shinya Maeda requested to merge run-pipeline-for-merge-train-at-train-ref into master

What does this MR do?

Close https://gitlab.com/gitlab-org/gitlab-ee/issues/12562

While we were QAing on staging server, we found that there is a race condition on merge trains with https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/29569 change.

Given MR1 and MR2 are just pushed to the merge train, it will be processed as

  1. MR1 generates refs/merge-requests/1/merge and create a pipeline via MergeTrains::CreatePipelineService(i.e. MergeRequests::MergeToRefService)
  2. Pipeline on MR1 finished. MR1 is merged into master.
  3. MR2 generates refs/merge-requests/2/merge and create a pipeline via MergeTrains::CreatePipelineService(i.e. MergeRequests::MergeToRefService)
  4. Since MR2's merge status will be reset. MergeRequest::MergeabilityCheckService will regenerate merge ref to refs/merge-requests/2/merge. This SHA is different from the above SHA which was used for the pipeline.sha.
  5. Pipeline fails because it cannot find the SHA in the ref.

To combat this problem, Merge Train should generate a merge commit at refs/merge-trains/:iid/merge. This is already on our roadmap https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/14296, but since we cannot wait until it has been deployed to prd, we're just implementing a part of the functionality earlier as patch.

After this fix, the above flow will be:

  1. MR1 generates refs/merge-trains/1/merge and create a pipeline via MergeTrains::CreatePipelineService(i.e. MergeRequests::MergeToRefService)
  2. Pipeline on MR1 finished. MR1 is merged into master.
  3. MR2 generates refs/merge-trains/2/merge and create a pipeline via MergeTrains::CreatePipelineService(i.e. MergeRequests::MergeToRefService)
  4. Since MR2's merge status will be reset. MergeRequest::MergeabilityCheckService will regenerate merge ref to refs/merge-requests/2/merge.
  5. Pipeline succeeds as it find the intact SHA in the train ref.

Related: https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/14296 gitlab-org/release/tasks#840 (comment 187264936)

Does this MR meet the acceptance criteria?

Conformity

Performance and testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • [-] Label as security and @ mention @gitlab-com/gl-security/appsec
  • [-] The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • [-] Security reports checked/validated by a reviewer from the AppSec team
Edited by Shinya Maeda

Merge request reports