Rollback mutation of merge request on failed merge60720-wrap-merge-in-transaction

The second call to the Gitaly UserMergeBranch RPC method can raise an
exception. Previously, if this were to happen, the merge request's
`in_progress_merge_commit_sha` attribute would be set to a commit SHA
that didn't exist.

`Repository#merge` now uses a transaction block in order to rollback
the setting of this attribute if any error occurs.

https://gitlab.com/gitlab-org/gitlab-ce/issues/60720

2 files changed, 51 insertions, 8 deletions

diff --git a/app/models/repository.rb b/app/models/repository.rb
index 574ce12b309..c1631784450 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -844,9 +844,11 @@ class Repository
 
   def merge(user, source_sha, merge_request, message)
     with_cache_hooks do
-      raw_repository.merge(user, source_sha, merge_request.target_branch, message) do |commit_id|
-        merge_request.update(in_progress_merge_commit_sha: commit_id)
-        nil # Return value does not matter.
+      MergeRequest.transaction do
+        raw_repository.merge(user, source_sha, merge_request.target_branch, message) do |commit_id|
+          merge_request.update(in_progress_merge_commit_sha: commit_id)
+          nil # Return value does not matter.
+        end
       end
     end
   end
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index 3f5d285bc2c..290723e7936 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -1364,16 +1364,57 @@ describe Repository do
       expect(repository.blob_at(merge_commit.id, 'files/ruby/feature.rb')).to be_present
     end
 
-    it 'sets the `in_progress_merge_commit_sha` flag for the given merge request' do
+    it 'removes carriage returns from commit message' do
       merge_commit_id = merge(repository, user, merge_request, message)
 
-      expect(merge_request.in_progress_merge_commit_sha).to eq(merge_commit_id)
+      expect(repository.commit(merge_commit_id).message).to eq(message.delete("\r"))
     end
 
-    it 'removes carriage returns from commit message' do
-      merge_commit_id = merge(repository, user, merge_request, message)
+    describe 'setting the `in_progress_merge_commit_sha` for the given merge request' do
+      let(:new_sha) { Digest::SHA1.hexdigest('foo') }
 
-      expect(repository.commit(merge_commit_id).message).to eq(message.delete("\r"))
+      it 'sets the attribute when there are no errors' do
+        second_response = build_second_response(pre_receive_error: nil)
+        mock_gitaly(second_response)
+
+        merge(repository, user, merge_request, message)
+
+        expect(merge_request.reload.in_progress_merge_commit_sha).to eq(new_sha)
+      end
+
+      it 'rollsback when an error is encountered in the second step' do
+        second_response = build_second_response(pre_receive_error: 'my_error')
+        mock_gitaly(second_response)
+
+        expect do
+          merge(repository, user, merge_request, message)
+        end.to raise_error(Gitlab::Git::PreReceiveError)
+
+        expect(merge_request.reload.in_progress_merge_commit_sha).to be_nil
+      end
+
+      def build_second_response(pre_receive_error:)
+        double(
+          pre_receive_error: pre_receive_error,
+          git_error: nil,
+          branch_update: double(
+            commit_id: new_sha,
+            repo_created: false,
+            branch_created: false
+          )
+        )
+      end
+
+      def mock_gitaly(second_response)
+        responses = [
+          double(commit_id: new_sha, pre_receive_error: nil, git_error: nil),
+          second_response
+        ]
+
+        expect_any_instance_of(
+          Gitaly::OperationService::Stub
+        ).to receive(:user_merge_branch).and_return(responses.each)
+      end
     end
 
     def merge(repository, user, merge_request, message)