Cleans merge_jid when possible on MergeService36114-stuck-mrs-job-follow-up

2 files changed, 34 insertions, 8 deletions

diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb
index aac1e7e19c3..b2b6c5627fb 100644
--- a/app/services/merge_requests/merge_service.rb
+++ b/app/services/merge_requests/merge_service.rb
@@ -26,14 +26,13 @@ module MergeRequests
       merge_request.in_locked_state do
         if commit
           after_merge
+          clean_merge_jid
           success
         end
       end
     rescue MergeError => e
+      clean_merge_jid
       log_merge_error(e.message, save_message_on_model: true)
-    ensure
-      # Make sure to clean up merge_jid in the end of the merge process.
-      merge_request.update_column(:merge_jid, nil)
     end
 
     private
@@ -73,6 +72,10 @@ module MergeRequests
       end
     end
 
+    def clean_merge_jid
+      merge_request.update_column(:merge_jid, nil)
+    end
+
     def branch_deletion_user
       @merge_request.force_remove_source_branch? ? @merge_request.author : current_user
     end
diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb
index 0a007bdfc3b..b60136064b7 100644
--- a/spec/services/merge_requests/merge_service_spec.rb
+++ b/spec/services/merge_requests/merge_service_spec.rb
@@ -12,13 +12,36 @@ describe MergeRequests::MergeService do
   end
 
   describe '#execute' do
-    it 'cleans up merge_jid from MergeRequest' do
-      merge_request.update_column(:merge_jid, 'hash-123')
-      service = described_class.new(project, user, commit_message: 'Awesome message')
+    context 'MergeRequest#merge_jid' do
+      before do
+        merge_request.update_column(:merge_jid, 'hash-123')
+      end
+
+      it 'is cleaned when no error is raised' do
+        service = described_class.new(project, user, commit_message: 'Awesome message')
+
+        service.execute(merge_request)
+
+        expect(merge_request.reload.merge_jid).to be_nil
+      end
 
-      service.execute(merge_request)
+      it 'is cleaned when expected error is raised' do
+        service = described_class.new(project, user, commit_message: 'Awesome message')
+        allow(service).to receive(:commit).and_raise(described_class::MergeError)
 
-      expect(merge_request.reload.merge_jid).to be_nil
+        service.execute(merge_request)
+
+        expect(merge_request.reload.merge_jid).to be_nil
+      end
+
+      it 'is not cleaned when unexpected error is raised' do
+        service = described_class.new(project, user, commit_message: 'Awesome message')
+        allow(service).to receive(:commit).and_raise(StandardError)
+
+        expect { service.execute(merge_request) }.to raise_error(StandardError)
+
+        expect(merge_request.reload.merge_jid).to be_present
+      end
     end
 
     context 'valid params' do