Merge ref can get outdated due to race condition with RefreshService
As pointed out at https://gitlab.slack.com/archives/CHLKE258E/p1559810531030900, the MergeabilityCheckService
can return success
even with an outdated merge ref. That's because the MergeRequests::RefreshService
is called async and it has the responsibility of flipping the MRs merge_status
to unchecked
when they receive updates, or its target branches receives an update.
This, for instance, cause problems to create pipelines with the latest state:
- MR-1 merged
- MR-2 has an existing merge ref (but
recheck_state?(merge_status)
returnsfalse
- race condition) - MR-2 creates a pipeline for merged results on old target sha (given
MergeabilityCheckService
returnedsuccess
with an outdated merge ref)
Possible solutions:
- Make the
MergeService
update all related MRs merge status tounchecked
upfront. It doesn't solve all the problems (i.e. pushes) - When
MergeabilityCheckService
gets called, make sure we're not returningsuccess
with an outdated merge-ref
For the second, we can make the following change to MergeabilityCheckService
:
def update_merge_status
+ ensure_unchecked_merge_status
+
return unless merge_request.recheck_merge_status?
if can_git_merge?
@@ -72,6 +72,15 @@ module MergeRequests
end
end
+ # Makes sure the MergeRequest#merge_status is "unchecked" if the merge-ref got out of
+ # sync with the target branch state.
+ #
+ # Given MergeRequests::RefreshService is called async, it might happen that the target
+ # branch gets updated, but the MergeRequest#merge_status lags behind.
+ def ensure_unchecked_merge_status
+ merge_request.mark_as_unchecked if merge_request.can_be_merged? && outdated_merge_ref?
+ end
cc @dosuken123
Edited by Oswaldo Ferreira