A blocked URL for a push mirror is a hard failure

Push mirrors have a lot of retry logic. When the destination is blocked
by Gitlab::UrlBlocker, that logic was being skipped entirely, which
meant that the mirror would be rescheduled indefinitely. By turning it
into a hard error instead, we can prevent a large buildup of
RepositoryUpdateRemoteMirrorWorker sidekiq jobs.

app/models/remote_mirror.rb

@@ -84,13 +84,7 @@ class RemoteMirror < ApplicationRecord
     end
 
     after_transition started: :failed do |remote_mirror|
-      Gitlab::Metrics.add_event(:remote_mirrors_failed)
-
-      remote_mirror.update(last_update_at: Time.current)
-
-      remote_mirror.run_after_commit do
-        RemoteMirrorNotificationWorker.perform_async(remote_mirror.id)
-      end
+      remote_mirror.send_failure_notifications
     end
   end
 
@@ -188,6 +182,24 @@ class RemoteMirror < ApplicationRecord
     update_fail!
   end
 
+  # Force the mrror into the retry state
+  def hard_retry!(error_message)
+    update_error_message(error_message)
+    self.update_status = :to_retry
+
+    save!(validate: false)
+  end
+
+  # Force the mirror into the failed state
+  def hard_fail!(error_message)
+    update_error_message(error_message)
+    self.update_status = :failed
+
+    save!(validate: false)
+
+    send_failure_notifications
+  end
+
   def url=(value)
     super(value) && return unless Gitlab::UrlSanitizer.valid?(value)
 
@@ -239,6 +251,17 @@ class RemoteMirror < ApplicationRecord
     last_update_at.present? ? MAX_INCREMENTAL_RUNTIME : MAX_FIRST_RUNTIME
   end
 
+  def send_failure_notifications
+    Gitlab::Metrics.add_event(:remote_mirrors_failed)
+
+    run_after_commit do
+      RemoteMirrorNotificationWorker.perform_async(id)
+    end
+
+    self.last_update_at = Time.current
+    save!(validate: false)
+  end
+
   private
 
   def store_credentials

app/services/projects/update_remote_mirror_service.rb

@@ -9,8 +9,10 @@ module Projects
     def execute(remote_mirror, tries)
       return success unless remote_mirror.enabled?
 
+      # Blocked URLs are a hard failure, no need to attempt to retry
       if Gitlab::UrlBlocker.blocked_url?(normalized_url(remote_mirror.url))
-        return error("The remote mirror URL is invalid.")
+        hard_retry_or_fail(remote_mirror, _('The remote mirror URL is invalid.'), tries)
+        return error(remote_mirror.last_error)
       end
 
       update_mirror(remote_mirror)
@@ -19,11 +21,11 @@ module Projects
     rescue Gitlab::Git::CommandError => e
       # This happens if one of the gitaly calls above fail, for example when
       # branches have diverged, or the pre-receive hook fails.
-      retry_or_fail(remote_mirror, e.message, tries)
+      hard_retry_or_fail(remote_mirror, e.message, tries)
 
       error(e.message)
     rescue => e
-      remote_mirror.mark_as_failed!(e.message)
+      remote_mirror.hard_fail!(e.message)
       raise e
     end
 
@@ -70,15 +72,15 @@ module Projects
       ).execute
     end
 
-    def retry_or_fail(mirror, message, tries)
+    def hard_retry_or_fail(mirror, message, tries)
       if tries < MAX_TRIES
-        mirror.mark_for_retry!(message)
+        mirror.hard_retry!(message)
       else
         # It's not likely we'll be able to recover from this ourselves, so we'll
         # notify the users of the problem, and don't trigger any sidekiq retries
         # Instead, we'll wait for the next change to try the push again, or until
         # a user manually retries.
-        mirror.mark_as_failed!(message)
+        mirror.hard_fail!(message)
       end
     end
   end

changelogs/unreleased/300884-fix-push-mirror-failures.yml

+---
+title: A blocked URL for a push mirror is a hard failure
+merge_request: 57392
+author:
+type: fixed

locale/gitlab.pot

@@ -30420,6 +30420,9 @@ msgstr ""
 msgid "The regular expression used to find test coverage output in the job log. For example, use %{regex} for Simplecov (Ruby). Leave blank to disable."
 msgstr ""
 
+msgid "The remote mirror URL is invalid."
+msgstr ""
+
 msgid "The remote mirror took to long to complete."
 msgstr ""
 

spec/models/remote_mirror_spec.rb

@@ -263,6 +263,30 @@ RSpec.describe RemoteMirror, :mailer do
     end
   end
 
+  describe '#hard_retry!' do
+    let(:remote_mirror) { create(:remote_mirror).tap {|mirror| mirror.update_column(:url, 'invalid') } }
+
+    it 'transitions an invalid mirror to the to_retry state' do
+      remote_mirror.hard_retry!('Invalid')
+
+      expect(remote_mirror.update_status).to eq('to_retry')
+      expect(remote_mirror.last_error).to eq('Invalid')
+    end
+  end
+
+  describe '#hard_fail!' do
+    let(:remote_mirror) { create(:remote_mirror).tap {|mirror| mirror.update_column(:url, 'invalid') } }
+
+    it 'transitions an invalid mirror to the failed state' do
+      remote_mirror.hard_fail!('Invalid')
+
+      expect(remote_mirror.update_status).to eq('failed')
+      expect(remote_mirror.last_error).to eq('Invalid')
+      expect(remote_mirror.last_update_at).not_to be_nil
+      expect(RemoteMirrorNotificationWorker.jobs).not_to be_empty
+    end
+  end
+
   context 'when remote mirror gets destroyed' do
     it 'removes remote' do
       mirror = create_mirror(url: 'http://foo:bar@test.com')

spec/services/projects/update_remote_mirror_service_spec.rb

@@ -12,7 +12,9 @@ RSpec.describe Projects::UpdateRemoteMirrorService do
   subject(:service) { described_class.new(project, project.creator) }
 
   describe '#execute' do
-    subject(:execute!) { service.execute(remote_mirror, 0) }
+    let(:retries) { 0 }
+
+    subject(:execute!) { service.execute(remote_mirror, retries) }
 
     before do
       project.repository.add_branch(project.owner, 'existing-branch', 'master')
@@ -62,8 +64,18 @@ RSpec.describe Projects::UpdateRemoteMirrorService do
         allow(Gitlab::UrlBlocker).to receive(:blocked_url?).and_return(true)
       end
 
-      it 'fails and returns error status' do
+      it 'hard retries and returns error status' do
         expect(execute!).to eq(status: :error, message: 'The remote mirror URL is invalid.')
+        expect(remote_mirror).to be_to_retry
+      end
+
+      context 'when retries are exceeded' do
+        let(:retries) { 4 }
+
+        it 'hard fails and returns error status' do
+          expect(execute!).to eq(status: :error, message: 'The remote mirror URL is invalid.')
+          expect(remote_mirror).to be_failed
+        end
       end
     end