Commit 64c36629 authored by Sean McGivern's avatar Sean McGivern

Merge branch 'fix-merge-train-to-evaluate-in_progress_merge_commit_sha' into 'master'

Fix merge train unnecessarily retries pipeline by a race condition

Closes #196133

See merge request gitlab-org/gitlab!24566
parents ec6b44ae 6364a1d9
...@@ -79,6 +79,8 @@ module MergeRequests ...@@ -79,6 +79,8 @@ module MergeRequests
end end
merge_request.update!(merge_commit_sha: commit_id) merge_request.update!(merge_commit_sha: commit_id)
ensure
merge_request.update_column(:in_progress_merge_commit_sha, nil)
end end
def try_merge def try_merge
...@@ -89,8 +91,6 @@ module MergeRequests ...@@ -89,8 +91,6 @@ module MergeRequests
rescue => e rescue => e
handle_merge_error(log_message: e.message) handle_merge_error(log_message: e.message)
raise_error('Something went wrong during merge') raise_error('Something went wrong during merge')
ensure
merge_request.update!(in_progress_merge_commit_sha: nil)
end end
def after_merge def after_merge
......
---
title: Fix merge train unnecessarily retries pipeline by a race condition
merge_request: 24566
author:
type: fixed
...@@ -95,8 +95,9 @@ class MergeTrain < ApplicationRecord ...@@ -95,8 +95,9 @@ class MergeTrain < ApplicationRecord
end end
def sha_exists_in_history?(target_project_id, target_branch, newrev, limit: 20) def sha_exists_in_history?(target_project_id, target_branch, newrev, limit: 20)
MergeRequest.exists?(id: complete_merge_trains(target_project_id, target_branch, limit: limit), MergeRequest.where(id: complete_merge_trains(target_project_id, target_branch, limit: limit))
merge_commit_sha: newrev) .where('merge_commit_sha = ? OR in_progress_merge_commit_sha = ?', newrev, newrev)
.exists?
end end
def total_count_in_train(merge_request) def total_count_in_train(merge_request)
......
...@@ -263,7 +263,7 @@ describe MergeTrain do ...@@ -263,7 +263,7 @@ describe MergeTrain do
let!(:merge_request_1) { create_merge_request_on_train(status: :merging) } let!(:merge_request_1) { create_merge_request_on_train(status: :merging) }
before do before do
merge_request_1.update_column(:merge_commit_sha, merge_commit_sha_1) merge_request_1.update_column(:in_progress_merge_commit_sha, merge_commit_sha_1)
end end
it { is_expected.to eq(true) } it { is_expected.to eq(true) }
......
...@@ -31,6 +31,11 @@ describe MergeRequests::MergeService do ...@@ -31,6 +31,11 @@ describe MergeRequests::MergeService do
it { expect(merge_request).to be_valid } it { expect(merge_request).to be_valid }
it { expect(merge_request).to be_merged } it { expect(merge_request).to be_merged }
it 'persists merge_commit_sha and nullifies in_progress_merge_commit_sha' do
expect(merge_request.merge_commit_sha).not_to be_nil
expect(merge_request.in_progress_merge_commit_sha).to be_nil
end
it 'sends email to user2 about merge of new merge_request' do it 'sends email to user2 about merge of new merge_request' do
email = ActionMailer::Base.deliveries.last email = ActionMailer::Base.deliveries.last
expect(email.to.first).to eq(user2.email) expect(email.to.first).to eq(user2.email)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment