Fix race condition with UpdateMirrorWorker Lease

CHANGELOG-EE

@@ -11,6 +11,9 @@ v 8.11.0 (unreleased)
   - [Elastic] Significant improvement of global search performance
   - [Fix] Push rules check existing commits in some cases
 
+v 8.10.6
+  - Fix race condition with UpdateMirrorWorker Lease
+
 v 8.10.5
   - Used cached value of project count in `Elastic::RepositoriesSearch` to reduce DB load. !637
 

app/models/project.rb

@@ -603,7 +603,7 @@ class Project < ActiveRecord::Base
     mirror_updated? && self.mirror_last_successful_update_at
   end
 
-  def update_mirror(delay: 0)
+  def update_mirror
     return unless mirror? && repository_exists?
 
     return if import_in_progress?
@@ -614,7 +614,7 @@ class Project < ActiveRecord::Base
       import_start
     end
 
-    RepositoryUpdateMirrorWorker.perform_in(delay, self.id)
+    RepositoryUpdateMirrorWorker.perform_async(self.id)
   end
 
   def has_remote_mirror?

app/workers/repository_update_mirror_dispatch_worker.rb

+class RepositoryUpdateMirrorDispatchWorker
+  include Sidekiq::Worker
+
+  LEASE_TIMEOUT = 5.minutes
+
+  sidekiq_options queue: :gitlab_shell
+
+  attr_accessor :project, :repository, :current_user
+
+  def perform(project_id)
+    return unless try_obtain_lease(project_id)
+
+    @project = Project.find_by_id(project_id)
+    return unless project
+
+    project.update_mirror
+  end
+
+  private
+
+  def try_obtain_lease(project_id)
+    # Using 5 minutes timeout based on the 95th percent of timings (currently max of 25 minutes)
+    lease = ::Gitlab::ExclusiveLease.new("repository_update_mirror_dispatcher:#{project_id}", timeout: LEASE_TIMEOUT)
+    lease.try_obtain
+  end
+end

app/workers/repository_update_mirror_worker.rb

@@ -4,16 +4,12 @@ class RepositoryUpdateMirrorWorker
   include Sidekiq::Worker
   include Gitlab::ShellAdapter
 
-  LEASE_TIMEOUT = 300
-
   sidekiq_options queue: :gitlab_shell
 
   attr_accessor :project, :repository, :current_user
 
   def perform(project_id)
     begin
-      return unless try_obtain_lease(project_id)
-
       @project = Project.find(project_id)
       @current_user = @project.mirror_user || @project.creator
 
@@ -30,12 +26,4 @@ class RepositoryUpdateMirrorWorker
       raise UpdateMirrorError, "#{ex.class}: #{Gitlab::UrlSanitizer.sanitize(ex.message)}"
     end
   end
-
-  private
-
-  def try_obtain_lease(project_id)
-    # Using 5 minutes timeout based on the 95th percent of timings (currently max of 25 seconds)
-    lease = ::Gitlab::ExclusiveLease.new("repository_update_mirror:#{project_id}", timeout: LEASE_TIMEOUT)
-    lease.try_obtain
-  end
 end

app/workers/update_all_mirrors_worker.rb

@@ -9,7 +9,7 @@ class UpdateAllMirrorsWorker
     fail_stuck_mirrors!
 
     Project.mirror.find_each(batch_size: 200) do |project|
-      project.update_mirror(delay: rand(30.minutes))
+      RepositoryUpdateMirrorDispatchWorker.perform_in(rand(30.minutes), project.id)
     end
   end
 

spec/features/projects/mirror_spec.rb

+require 'spec_helper'
+
+feature 'Project mirror', feature: true do
+  let(:project) { create(:project, :mirror, :import_finished, creator: user, name: 'Victorialand') }
+  let(:user) { create(:user) }
+
+  describe 'On a project', js: true do
+    before do
+      project.team << [user, :master]
+      login_as user
+      visit namespace_project_mirror_path(project.namespace, project)
+    end
+
+    describe 'pressing "Update now"' do
+      it 'returns with the project updating (job enqueued)' do
+        Sidekiq::Testing.fake! { click_link('Update Now') }
+
+        expect(page).to have_content('Updating')
+      end
+    end
+  end
+end

spec/workers/repository_update_mirror_dispatch_worker_spec.rb

+require 'rails_helper'
+
+describe RepositoryUpdateMirrorDispatchWorker do
+  describe '#perform' do
+    it 'executes project#update_mirror if can obtain a lease' do
+      allow_any_instance_of(Gitlab::ExclusiveLease)
+        .to receive(:try_obtain).and_return(true)
+      expect_any_instance_of(Project).to receive(:update_mirror)
+
+      project = create(:empty_project, :mirror)
+      described_class.new.perform(project.id)
+    end
+
+    it 'just returns if cannot obtain a lease' do
+      allow_any_instance_of(Gitlab::ExclusiveLease)
+        .to receive(:try_obtain).and_return(false)
+      expect_any_instance_of(Project).not_to receive(:update_mirror)
+
+      project = create(:empty_project, :mirror)
+      described_class.new.perform(project.id)
+    end
+  end
+end

spec/workers/repository_update_mirror_worker_spec.rb

@@ -2,43 +2,22 @@ require 'rails_helper'
 
 describe RepositoryUpdateMirrorWorker do
   describe '#perform' do
-    it "just returns if cannot obtain a lease" do
-      worker = described_class.new
-      project_id = 15
+    it 'sets import as finished when update mirror service executes successfully' do
+      project = create(:empty_project, :mirror)
 
-      allow_any_instance_of(Gitlab::ExclusiveLease)
-        .to receive(:try_obtain).and_return(false)
+      expect_any_instance_of(Projects::UpdateMirrorService).to receive(:execute).and_return(status: :success)
 
-      expect(Projects::UpdateMirrorService).not_to receive(:execute)
-
-      worker.perform(project_id)
+      expect { described_class.new.perform(project.id) }
+        .to change { project.reload.import_status }.to('finished')
     end
 
-    context "when obtain the lease" do
-      before do
-        allow_any_instance_of(Gitlab::ExclusiveLease)
-          .to receive(:try_obtain).and_return(true)
-      end
-
-      it "sets import as finished when update mirror service executes successfully" do
-        project = create(:empty_project, :mirror)
-
-        expect_any_instance_of(Projects::UpdateMirrorService).to receive(:execute).and_return(status: :success)
-
-        expect do
-          described_class.new.perform(project.id)
-        end.to change { project.reload.import_status }.to("finished")
-      end
-
-      it "sets import as failed when update mirror service executes with errors" do
-        project = create(:empty_project, :mirror)
+    it 'sets import as failed when update mirror service executes with errors' do
+      project = create(:empty_project, :mirror)
 
-        expect_any_instance_of(Projects::UpdateMirrorService).to receive(:execute).and_return(status: :error, message: 'fail!')
+      expect_any_instance_of(Projects::UpdateMirrorService).to receive(:execute).and_return(status: :error, message: 'fail!')
 
-        expect do
-          described_class.new.perform(project.id)
-        end.to change { project.reload.import_status }.to("failed")
-      end
+      expect { described_class.new.perform(project.id) }
+        .to change { project.reload.import_status }.to('failed')
     end
   end
 end

spec/workers/update_all_mirrors_worker_spec.rb

@@ -15,14 +15,14 @@ describe UpdateAllMirrorsWorker do
       worker.perform
     end
 
-    it 'updates all mirrored Projects' do
+    it 'enqueue a job on all mirrored Projects' do
       worker = described_class.new
 
-      create(:empty_project, :mirror)
+      mirror = create(:empty_project, :mirror)
       create(:empty_project)
 
       expect(worker).to receive(:rand).with(30.minutes).and_return(10)
-      expect_any_instance_of(Project).to receive(:update_mirror).with(delay: 10).once
+      expect(RepositoryUpdateMirrorDispatchWorker).to receive(:perform_in).with(10, mirror.id)
 
       worker.perform
     end