Adds ExclusiveLeaseGuard to RepositoryRemoveRemoteWorker and restores...

Adds ExclusiveLeaseGuard to RepositoryRemoveRemoteWorker and restores RemoteMirror#refresh_remote to after_save callback

app/workers/repository_remove_remote_worker.rb

 class RepositoryRemoveRemoteWorker
   include ApplicationWorker
+  include ExclusiveLeaseGuard
 
   LEASE_TIMEOUT = 1.hour
 
-  def perform(project_id, remote_name)
-    lease_uuid = try_obtain_lease(project_id)
-    return unless lease_uuid
+  attr_reader :project, :remote_name
 
-    project = Project.find(project_id)
-    project.repository.remove_remote(remote_name)
+  def perform(project_id, remote_name)
+    @remote_name = remote_name
+    @project = Project.find(project_id)
 
-    cancel_lease(project_id, lease_uuid)
+    try_obtain_lease do
+      @project.repository.remove_remote(remote_name)
     end
-
-  private
-
-  def lease_key(project_id)
-    "remove_remote_#{project_id}"
   end
 
-  def try_obtain_lease(id)
-    key = lease_key(id)
-
-    Gitlab::ExclusiveLease.new(key, timeout: LEASE_TIMEOUT).try_obtain
+  def lease_timeout
+    LEASE_TIMEOUT
   end
 
-  def cancel_lease(id, uuid)
-    key = lease_key(id)
-
-    Gitlab::ExclusiveLease.cancel(key, uuid)
+  def lease_key
+    "remove_remote_#{project.id}_#{remote_name}"
   end
 end

ee/app/models/remote_mirror.rb

@@ -21,9 +21,10 @@ class RemoteMirror < ActiveRecord::Base
   validate  :url_availability, if: -> (mirror) { mirror.url_changed? || mirror.enabled? }
   validates :url, addressable_url: true, if: :url_changed?
 
-  before_save :refresh_remote, if: :mirror_url_changed?
+  before_save :set_new_remote_name, if: :mirror_url_changed?
 
   after_save :set_override_remote_mirror_available, unless: -> { Gitlab::CurrentSettings.current_application_settings.mirror_available }
+  after_save :refresh_remote, if: :mirror_url_changed?
   after_update :reset_fields, if: :mirror_url_changed?
   after_destroy :remove_remote
 
@@ -76,7 +77,7 @@ class RemoteMirror < ActiveRecord::Base
     return name if name
     return unless id
 
-    "remote_mirror_#{id}"
+    fallback_remote_name
   end
 
   def update_failed?
@@ -158,6 +159,10 @@ class RemoteMirror < ActiveRecord::Base
     @raw ||= Gitlab::Git::RemoteMirror.new(project.repository.raw, remote_name)
   end
 
+  def fallback_remote_name
+    "remote_mirror_#{id}"
+  end
+
   def recently_scheduled?
     return false unless self.last_update_started_at
 
@@ -195,7 +200,7 @@ class RemoteMirror < ActiveRecord::Base
     project.update(remote_mirror_available_overridden: enabled)
   end
 
-  def write_new_remote_name
+  def set_new_remote_name
     self.remote_name = "remote_mirror_#{SecureRandom.hex}"
   end
 
@@ -204,12 +209,11 @@ class RemoteMirror < ActiveRecord::Base
 
     # Before adding a new remote we have to delete the data from
     # the previous remote name
-    prev_remote_name = remote_name
+    prev_remote_name = remote_name_was || fallback_remote_name
     run_after_commit do
       project.repository.schedule_remove_remote(prev_remote_name)
     end
 
-    write_new_remote_name
     project.repository.add_remote(remote_name, url)
   end
 

spec/workers/repository_remove_remote_worker_spec.rb

 require 'rails_helper'
 
 describe RepositoryRemoveRemoteWorker do
+  subject(:worker) { described_class.new }
+
   describe '#perform' do
     let(:remote_name) { 'joe'}
     let!(:project) { create(:project, :repository) }
 
-    context 'when it does not get the lease' do
-      it 'does not execute' do
-        allow_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain).and_return(false)
+    context 'when it cannot obtain lease' do
+      it 'logs error' do
+        allow_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain) { nil }
 
         expect_any_instance_of(Repository).not_to receive(:remove_remote)
+        expect(worker).to receive(:log_error).with('Cannot obtain an exclusive lease. There must be another instance already in execution.')
 
-        subject.perform(project.id, remote_name)
+        worker.perform(project.id, remote_name)
       end
     end
 
@@ -22,7 +25,7 @@ describe RepositoryRemoveRemoteWorker do
 
       context 'when project does not exist' do
         it 'returns nil' do
-          expect { subject.perform(-1, 'remote_name') }.to raise_error(ActiveRecord::RecordNotFound)
+          expect { worker.perform(-1, 'remote_name') }.to raise_error(ActiveRecord::RecordNotFound)
         end
       end
 
@@ -34,7 +37,7 @@ describe RepositoryRemoveRemoteWorker do
 
           expect_any_instance_of(Repository).to receive(:remove_remote).with(remote_name).and_call_original
 
-          subject.perform(project.id, remote_name)
+          worker.perform(project.id, remote_name)
         end
       end
     end