Race conditions related to merges to target branches that are about to be deleted
Summary
It is possible to merge code into branches that are already "pending deletion"
Steps to reproduce
First, Create two merge requests:
-
MR 1: source
branch-1
, targetmain
; mark the source branch for deletion -
MR 2: source
branch-2
, targetbranch-1
Then, rapidly click merge on both of them.
Example Project
What is the current bug behavior?
Some of the time, MR 2 will get merged into branch-1
, and then gets branch-1
deleted. This can happen if MR 2 merges after this check is performed but before the branch is actually deleted.
What is the expected correct behavior?
There are two problems:
- It should not be possible to merge another MR into the source branch of an MR that is being merged (or already merged), if the source branch is set to be deleted
- If a change lands on the source branch anyway, then the source branch should not get deleted
Relevant logs and/or screenshots
Output of checks
Results of GitLab environment info
Expand for output related to GitLab environment info
(For installations with omnibus-gitlab package run and paste the output of: `sudo gitlab-rake gitlab:env:info`) (For installations from source run and paste the output of: `sudo -u git -H bundle exec rake gitlab:env:info RAILS_ENV=production`)
Results of GitLab application Check
Expand for output related to the GitLab application check
(For installations with omnibus-gitlab package run and paste the output of:
sudo gitlab-rake gitlab:check SANITIZE=true
)(For installations from source run and paste the output of:
sudo -u git -H bundle exec rake gitlab:check RAILS_ENV=production SANITIZE=true
)(we will only investigate if the tests are passing)
Possible fixes
- To prevent the merge of MRs like MR 2, we need to be able to efficiently check if its target branch is pending post-merge deletion. This can be stored in PostgreSQL or redis.
- To prevent the deletion of branches that have already changed, we need to pass down the expected commit SHA of the branch all the way to Gitaly. This is already how git ref deletions work under the hood and the Gitaly RPC already supports the [required parameter,
expected_old_oid
]. It's just a matter of passing it down the chain and being mindful of multi-version compatibility when changing the sidekiq worker arguments.
Edited by Hordur Freyr Yngvason