Merge branch '5049-fix-transaction-raised-error-in-mirroring' into 'master'

Resolve "`RepositoryRemoveRemoteWorker.perform_async` being called from transaction"

Closes #5049

See merge request gitlab-org/gitlab-ee!4747

config/initializers/forbid_sidekiq_in_transactions.rb

@@ -18,13 +18,26 @@ module Sidekiq
         %i(perform_async perform_at perform_in).each do |name|
           define_method(name) do |*args|
             if !Sidekiq::Worker.skip_transaction_check && AfterCommitQueue.inside_transaction?
-              raise Sidekiq::Worker::EnqueueFromTransactionError, <<~MSG
+              begin
+                raise Sidekiq::Worker::EnqueueFromTransactionError, <<~MSG
                 `#{self}.#{name}` cannot be called inside a transaction as this can lead to
                 race conditions when the worker runs before the transaction is committed and
                 tries to access a model that has not been saved yet.
 
                 Use an `after_commit` hook, or include `AfterCommitQueue` and use a `run_after_commit` block instead.
-              MSG
+                MSG
+              rescue Sidekiq::Worker::EnqueueFromTransactionError => e
+                if Rails.env.production?
+                  Rails.logger.error(e.message)
+
+                  if Gitlab::Sentry.enabled?
+                    Gitlab::Sentry.context
+                    Raven.capture_exception(e)
+                  end
+                else
+                  raise
+                end
+              end
             end
 
             super(*args)

ee/app/models/ee/project.rb

@@ -337,7 +337,9 @@ module EE
     end
 
     def remove_mirror_repository_reference
-      repository.async_remove_remote(::Repository::MIRROR_REMOTE)
+      run_after_commit do
+        repository.async_remove_remote(::Repository::MIRROR_REMOTE)
+      end
     end
 
     def username_only_import_url

ee/app/models/remote_mirror.rb

@@ -24,7 +24,8 @@ class RemoteMirror < ActiveRecord::Base
   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
+
+  after_commit :remove_remote, on: :destroy
 
   scope :enabled, -> { where(enabled: true) }
   scope :started, -> { with_update_status(:started) }

ee/changelogs/unreleased/5049-fix-transaction-raised-error-in-mirroring.yml

+---
+title: Supresses error being raised due to async remote removal being run outside
+  a transaction
+merge_request: 4747
+author:
+type: fixed

ee/spec/models/project_spec.rb

@@ -328,6 +328,16 @@ describe Project do
     end
   end
 
+  describe 'updating import_url' do
+    it 'removes previous remote' do
+      project = create(:project, :repository, :mirror)
+
+      expect(RepositoryRemoveRemoteWorker).to receive(:perform_async).with(project.id, ::Repository::MIRROR_REMOTE).and_call_original
+
+      project.update_attributes(import_url: "http://test.com")
+    end
+  end
+
   describe '#mirror_waiting_duration' do
     it 'returns in seconds the time spent in the queue' do
       project = create(:project, :mirror, :import_scheduled)

ee/spec/models/remote_mirror_spec.rb

@@ -70,6 +70,14 @@ describe RemoteMirror do
         config = repo.raw_repository.rugged.config
         expect(config["remote.#{mirror.remote_name}.url"]).to eq('http://foo:baz@test.com')
       end
+
+      it 'removes previous remote' do
+        mirror = create_mirror(url: 'http://foo:bar@test.com')
+
+        expect(RepositoryRemoveRemoteWorker).to receive(:perform_async).with(mirror.project.id, mirror.remote_name).and_call_original
+
+        mirror.update_attributes(url: 'http://test.com')
+      end
     end
   end
 
@@ -120,6 +128,16 @@ describe RemoteMirror do
     end
   end
 
+  context 'when remote mirror gets destroyed' do
+    it 'removes remote' do
+      mirror = create_mirror(url: 'http://foo:bar@test.com')
+
+      expect(RepositoryRemoveRemoteWorker).to receive(:perform_async).with(mirror.project.id, mirror.remote_name).and_call_original
+
+      mirror.destroy!
+    end
+  end
+
   context 'stuck mirrors' do
     it 'includes mirrors stuck in started with no last_update_at set' do
       mirror = create_mirror(url: 'http://cantbeblank',