Merge branch '31207-clean-locked-merge-requests' into 'master'

Resolve "Store MergeWorker JID on merge request, and clean up stuck merges"

Closes #31207

See merge request !13207

10 files changed, 57 insertions, 32 deletions

diff --git a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_locked.js b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_merging.js
index a12f418e1af..f6d1a4feeb2 100644
--- a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_locked.js
+++ b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_merging.js
@@ -1,7 +1,7 @@
 import statusIcon from '../mr_widget_status_icon';
 
 export default {
-  name: 'MRWidgetLocked',
+  name: 'MRWidgetMerging',
   props: {
     mr: { type: Object, required: true },
   },
@@ -13,7 +13,7 @@ export default {
       <status-icon status="loading" />
       <div class="media-body">
         <h4>
-          This merge request is in the process of being merged, during which time it is locked and cannot be closed
+          This merge request is in the process of being merged
         </h4>
         <section class="mr-info-list">
           <p>
diff --git a/app/assets/javascripts/vue_merge_request_widget/dependencies.js b/app/assets/javascripts/vue_merge_request_widget/dependencies.js
index 546a3f625c7..49340c232c8 100644
--- a/app/assets/javascripts/vue_merge_request_widget/dependencies.js
+++ b/app/assets/javascripts/vue_merge_request_widget/dependencies.js
@@ -19,7 +19,7 @@ export { default as WidgetRelatedLinks } from './components/mr_widget_related_li
 export { default as MergedState } from './components/states/mr_widget_merged';
 export { default as FailedToMerge } from './components/states/mr_widget_failed_to_merge';
 export { default as ClosedState } from './components/states/mr_widget_closed';
-export { default as LockedState } from './components/states/mr_widget_locked';
+export { default as MergingState } from './components/states/mr_widget_merging';
 export { default as WipState } from './components/states/mr_widget_wip';
 export { default as ArchivedState } from './components/states/mr_widget_archived';
 export { default as ConflictsState } from './components/states/mr_widget_conflicts';
diff --git a/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js b/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js
index 577d77f09a6..0042c48816f 100644
--- a/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js
+++ b/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js
@@ -8,7 +8,7 @@ import {
   WidgetRelatedLinks,
   MergedState,
   ClosedState,
-  LockedState,
+  MergingState,
   WipState,
   ArchivedState,
   ConflictsState,
@@ -212,7 +212,7 @@ export default {
     'mr-widget-related-links': WidgetRelatedLinks,
     'mr-widget-merged': MergedState,
     'mr-widget-closed': ClosedState,
-    'mr-widget-locked': LockedState,
+    'mr-widget-merging': MergingState,
     'mr-widget-failed-to-merge': FailedToMerge,
     'mr-widget-wip': WipState,
     'mr-widget-archived': ArchivedState,
diff --git a/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js b/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js
index fddafb0ddfa..fbea764b739 100644
--- a/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js
+++ b/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js
@@ -73,6 +73,7 @@ export default class MergeRequestStore {
     this.canCancelAutomaticMerge = !!data.cancel_merge_when_pipeline_succeeds_path;
     this.hasSHAChanged = this.sha !== data.diff_head_sha;
     this.canBeMerged = data.can_be_merged || false;
+    this.mergeOngoing = data.merge_ongoing;
 
     // Cherry-pick and Revert actions related
     this.canCherryPickInCurrentMR = currentUser.can_cherry_pick_on_current_merge_request || false;
@@ -94,6 +95,11 @@ export default class MergeRequestStore {
   }
 
   setState(data) {
+    if (this.mergeOngoing) {
+      this.state = 'merging';
+      return;
+    }
+
     if (this.isOpen) {
       this.state = getStateKey.call(this, data);
     } else {
@@ -104,9 +110,6 @@ export default class MergeRequestStore {
         case 'closed':
           this.state = 'closed';
           break;
-        case 'locked':
-          this.state = 'locked';
-          break;
         default:
           this.state = null;
       }
diff --git a/app/assets/javascripts/vue_merge_request_widget/stores/state_maps.js b/app/assets/javascripts/vue_merge_request_widget/stores/state_maps.js
index 605dd3a1ff4..9074a064a6d 100644
--- a/app/assets/javascripts/vue_merge_request_widget/stores/state_maps.js
+++ b/app/assets/javascripts/vue_merge_request_widget/stores/state_maps.js
@@ -1,7 +1,7 @@
 const stateToComponentMap = {
   merged: 'mr-widget-merged',
   closed: 'mr-widget-closed',
-  locked: 'mr-widget-locked',
+  merging: 'mr-widget-merging',
   conflicts: 'mr-widget-conflicts',
   missingBranch: 'mr-widget-missing-branch',
   workInProgress: 'mr-widget-wip',
@@ -20,7 +20,7 @@ const stateToComponentMap = {
 };
 
 const statesToShowHelpWidget = [
-  'locked',
+  'merging',
   'conflicts',
   'workInProgress',
   'readyToMerge',
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index d361e661d0e..4de814d0ca8 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -67,11 +67,6 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
         @noteable = @merge_request
         @commits_count = @merge_request.commits_count
 
-        if @merge_request.locked_long_ago?
-          @merge_request.unlock_mr
-          @merge_request.close
-        end
-
         labels
 
         set_pipeline_variables
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 8ca850b6d96..e83b11f7668 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -8,6 +8,7 @@ class MergeRequest < ActiveRecord::Base
   include CreatedAtFilterable
 
   ignore_column :position
+  ignore_column :locked_at
 
   belongs_to :target_project, class_name: "Project"
   belongs_to :source_project, class_name: "Project"
@@ -61,16 +62,6 @@ class MergeRequest < ActiveRecord::Base
       transition locked: :opened
     end
 
-    after_transition any => :locked do |merge_request, transition|
-      merge_request.locked_at = Time.now
-      merge_request.save
-    end
-
-    after_transition locked: (any - :locked) do |merge_request, transition|
-      merge_request.locked_at = nil
-      merge_request.save
-    end
-
     state :opened
     state :closed
     state :merged
@@ -392,6 +383,12 @@ class MergeRequest < ActiveRecord::Base
                'Source project is not a fork of the target project'
   end
 
+  def merge_ongoing?
+    return false unless merge_jid
+
+    Gitlab::SidekiqStatus.num_running([merge_jid]) > 0
+  end
+
   def closed_without_fork?
     closed? && source_project_missing?
   end
@@ -725,12 +722,6 @@ class MergeRequest < ActiveRecord::Base
     end
   end
 
-  def locked_long_ago?
-    return false unless locked?
-
-    locked_at.nil? || locked_at < (Time.now - 1.day)
-  end
-
   def has_ci?
     has_ci_integration = source_project.try(:ci_service)
     uses_gitlab_ci = all_pipelines.any?
diff --git a/app/serializers/merge_request_entity.rb b/app/serializers/merge_request_entity.rb
index 7f17f2bf604..07650ce6f20 100644
--- a/app/serializers/merge_request_entity.rb
+++ b/app/serializers/merge_request_entity.rb
@@ -2,7 +2,6 @@ class MergeRequestEntity < IssuableEntity
   include RequestAwareEntity
 
   expose :in_progress_merge_commit_sha
-  expose :locked_at
   expose :merge_commit_sha
   expose :merge_error
   expose :merge_params
@@ -32,6 +31,7 @@ class MergeRequestEntity < IssuableEntity
   expose :head_pipeline, with: PipelineDetailsEntity, as: :pipeline
 
   # Booleans
+  expose :merge_ongoing?, as: :merge_ongoing
   expose :work_in_progress?, as: :work_in_progress
   expose :source_branch_exists?, as: :source_branch_exists
   expose :mergeable_discussions_state?, as: :mergeable_discussions_state
diff --git a/app/workers/merge_worker.rb b/app/workers/merge_worker.rb
index 48e2da338f6..c3b58df92c1 100644
--- a/app/workers/merge_worker.rb
+++ b/app/workers/merge_worker.rb
@@ -7,6 +7,8 @@ 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
diff --git a/app/workers/stuck_merge_jobs_worker.rb b/app/workers/stuck_merge_jobs_worker.rb
new file mode 100644
index 00000000000..7843179d77c
--- /dev/null
+++ b/app/workers/stuck_merge_jobs_worker.rb
@@ -0,0 +1,34 @@
+class StuckMergeJobsWorker
+  include Sidekiq::Worker
+  include CronjobQueue
+
+  def perform
+    stuck_merge_requests.find_in_batches(batch_size: 100) do |group|
+      jids = group.map(&:merge_jid)
+
+      # Find the jobs that aren't currently running or that exceeded the threshold.
+      completed_jids = Gitlab::SidekiqStatus.completed_jids(jids)
+
+      if completed_jids.any?
+        completed_ids = group.select { |merge_request| completed_jids.include?(merge_request.merge_jid) }.map(&:id)
+
+        apply_current_state!(completed_jids, completed_ids)
+      end
+    end
+  end
+
+  private
+
+  def apply_current_state!(completed_jids, completed_ids)
+    merge_requests = MergeRequest.where(id: completed_ids)
+
+    merge_requests.where.not(merge_commit_sha: nil).update_all(state: :merged)
+    merge_requests.where(merge_commit_sha: nil).update_all(state: :opened)
+
+    Rails.logger.info("Updated state of locked merge jobs. JIDs: #{completed_jids.join(', ')}")
+  end
+
+  def stuck_merge_requests
+    MergeRequest.select('id, merge_jid').with_state(:locked).where.not(merge_jid: nil).reorder(nil)
+  end
+end