Merge branch '36114-stuck-mrs-job-follow-up' into 'master'

app/controllers/projects/merge_requests_controller.rb

@@ -318,14 +318,14 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
       elsif @merge_request.head_pipeline.success?
         # This can be triggered when a user clicks the auto merge button while
         # the tests finish at about the same time
-        MergeWorker.perform_async(@merge_request.id, current_user.id, params)
+        @merge_request.merge_async(current_user.id, params)
 
         :success
       else
         :failed
       end
     else
-      MergeWorker.perform_async(@merge_request.id, current_user.id, params)
+      @merge_request.merge_async(current_user.id, params)
 
       :success
     end

app/models/merge_request.rb

@@ -241,6 +241,14 @@ class MergeRequest < ActiveRecord::Base
     end
   end
 
+  # Calls `MergeWorker` to proceed with the merge process and
+  # updates `merge_jid` with the MergeWorker#jid.
+  # This helps tracking enqueued and ongoing merge jobs.
+  def merge_async(user_id, params)
+    jid = MergeWorker.perform_async(id, user_id, params)
+    update_column(:merge_jid, jid)
+  end
+
   def first_commit
     merge_request_diff ? merge_request_diff.first_commit : compare_commits.first
   end
@@ -384,9 +392,7 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def merge_ongoing?
-    return false unless merge_jid
-
-    Gitlab::SidekiqStatus.num_running([merge_jid]) > 0
+    !!merge_jid && !merged?
   end
 
   def closed_without_fork?
@@ -819,7 +825,7 @@ class MergeRequest < ActiveRecord::Base
       lock_mr
       yield
     ensure
-      unlock_mr if locked?
+      unlock_mr
     end
   end
 

app/services/merge_requests/merge_service.rb

@@ -26,10 +26,12 @@ 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)
     end
 
@@ -70,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

app/services/merge_requests/merge_when_pipeline_succeeds_service.rb

@@ -30,7 +30,7 @@ module MergeRequests
           next
         end
 
-        MergeWorker.perform_async(merge_request.id, merge_request.merge_user_id, merge_request.merge_params)
+        merge_request.merge_async(merge_request.merge_user_id, merge_request.merge_params)
       end
     end
 

app/services/merge_requests/update_service.rb

@@ -83,7 +83,7 @@ module MergeRequests
       if merge_request.head_pipeline && merge_request.head_pipeline.active?
         MergeRequests::MergeWhenPipelineSucceedsService.new(project, current_user).execute(merge_request)
       else
-        MergeWorker.perform_async(merge_request.id, current_user.id, {})
+        merge_request.merge_async(current_user.id, {})
       end
     end
 

app/workers/merge_worker.rb

@@ -7,8 +7,6 @@ class MergeWorker
     current_user = User.find(current_user_id)
     merge_request = MergeRequest.find(merge_request_id)
 
-    merge_request.update_column(:merge_jid, jid)
-
     MergeRequests::MergeService.new(merge_request.target_project, current_user, params)
       .execute(merge_request)
   end

changelogs/unreleased/36114-stuck-mrs-job-follow-up.yml

+---
+title: Present enqueued merge jobs as Merging as well
+merge_request:
+author:

spec/models/merge_request_spec.rb

@@ -931,6 +931,23 @@ describe MergeRequest do
     end
   end
 
+  describe '#merge_async' do
+    it 'enqueues MergeWorker job and updates merge_jid' do
+      merge_request = create(:merge_request)
+      user_id = double(:user_id)
+      params = double(:params)
+      merge_jid = 'hash-123'
+
+      expect(MergeWorker).to receive(:perform_async).with(merge_request.id, user_id, params) do
+        merge_jid
+      end
+
+      merge_request.merge_async(user_id, params)
+
+      expect(merge_request.reload.merge_jid).to eq(merge_jid)
+    end
+  end
+
   describe '#check_if_can_be_merged' do
     let(:project) { create(:project, only_allow_merge_if_pipeline_succeeds: true) }
 
@@ -1370,29 +1387,11 @@ describe MergeRequest do
   end
 
   describe '#merge_ongoing?' do
-    it 'returns true when merge process is ongoing for merge_jid' do
-      merge_request = create(:merge_request, merge_jid: 'foo')
-
-      allow(Gitlab::SidekiqStatus).to receive(:num_running).with(['foo']).and_return(1)
+    it 'returns true when merge_id is present and MR is not merged' do
+      merge_request = build_stubbed(:merge_request, state: :open, merge_jid: 'foo')
 
       expect(merge_request.merge_ongoing?).to be(true)
     end
-
-    it 'returns false when no merge process running for merge_jid' do
-      merge_request = build(:merge_request, merge_jid: 'foo')
-
-      allow(Gitlab::SidekiqStatus).to receive(:num_running).with(['foo']).and_return(0)
-
-      expect(merge_request.merge_ongoing?).to be(false)
-    end
-
-    it 'returns false when merge_jid is nil' do
-      merge_request = build(:merge_request, merge_jid: nil)
-
-      expect(Gitlab::SidekiqStatus).not_to receive(:num_running)
-
-      expect(merge_request.merge_ongoing?).to be(false)
-    end
   end
 
   describe "#closed_without_fork?" do

spec/services/merge_requests/merge_service_spec.rb

@@ -12,6 +12,38 @@ describe MergeRequests::MergeService do
   end
 
   describe '#execute' do
+    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
+
+      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)
+
+        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
       let(:service) { described_class.new(project, user, commit_message: 'Awesome message') }
 

spec/workers/merge_worker_spec.rb

@@ -27,15 +27,4 @@ describe MergeWorker do
       expect(source_project.repository.branch_names).not_to include('markdown')
     end
   end
-
-  it 'persists merge_jid' do
-    merge_request = create(:merge_request, merge_jid: nil)
-    user = create(:user)
-    worker = described_class.new
-
-    allow(worker).to receive(:jid) { '999' }
-
-    expect { worker.perform(merge_request.id, user.id, {}) }
-      .to change { merge_request.reload.merge_jid }.from(nil).to('999')
-  end
 end