Merge branch '5348-slower-sync-when-backoff-time-is-being-respected-option3' into 'master'

Geo: Limit max backoff time by 1 hour, instead of 7 days

Closes #5348

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

ee/changelogs/unreleased/5348-slower-sync-when-backoff-time-is-being-respected-option3.yml

+---
+title: 'Geo: Limit max backoff time by 1 hour, instead of 7 days'
+merge_request: 9893
+author:
+type: changed

ee/lib/delay.rb

@@ -7,10 +7,10 @@ module Delay
   end
 
   # To prevent the retry time from storing invalid dates in the database,
-  # cap the max time to a week plus some random jitter value.
+  # cap the max time to a hour plus some random jitter value.
   def next_retry_time(retry_count)
     proposed_time = Time.now + delay(retry_count).seconds
-    max_future_time = Time.now + 7.days + delay(1).seconds
+    max_future_time = 1.hour.from_now + delay(1).seconds
 
     [proposed_time, max_future_time].min
   end

ee/spec/models/geo/project_registry_spec.rb

@@ -334,6 +334,17 @@ describe Geo::ProjectRegistry do
         expect(subject.last_repository_synced_at).to be_like_time(Time.now)
       end
 
+      it 'ensures repository_retry_at is capped at one hour' do
+        subject.update(repository_retry_count: 31)
+
+        subject.start_sync!(type)
+
+        expect(subject).to have_attributes(
+          repository_retry_at: be_within(100.seconds).of(1.hour.from_now),
+          repository_retry_count: 32
+        )
+      end
+
       shared_examples_for 'sets repository_retry_at to a future time' do
         it 'sets repository_retry_at to a future time' do
           subject.start_sync!(type)
@@ -390,6 +401,17 @@ describe Geo::ProjectRegistry do
         expect(subject.last_wiki_synced_at).to be_like_time(Time.now)
       end
 
+      it 'ensures wiki_retry_at is capped at one hour' do
+        subject.update(wiki_retry_count: 31)
+
+        subject.start_sync!(type)
+
+        expect(subject).to have_attributes(
+          wiki_retry_at: be_within(100.seconds).of(1.hour.from_now),
+          wiki_retry_count: 32
+        )
+      end
+
       shared_examples_for 'sets wiki_retry_at to a future time' do
         it 'sets wiki_retry_at to a future time' do
           subject.start_sync!(type)

ee/spec/services/geo/file_download_service_spec.rb

@@ -11,6 +11,45 @@ describe Geo::FileDownloadService do
     stub_current_geo_node(secondary)
   end
 
+  context 'retry time' do
+    before do
+      stub_transfer_result(bytes_downloaded: 0, success: false)
+    end
+
+    shared_examples_for 'a service that sets next retry time capped at some value' do
+      it 'ensures the next retry time is capped properly' do
+        download_service.execute
+
+        expect(registry_entry.reload).to have_attributes(
+          retry_at: be_within(100.seconds).of(1.hour.from_now),
+          retry_count: 32
+        )
+      end
+    end
+
+    context 'with job_artifacts' do
+      let!(:registry_entry) do
+        create(:geo_job_artifact_registry, success: false, retry_count: 31, artifact_id: file.id)
+      end
+
+      let(:file) { create(:ci_job_artifact) }
+      let(:download_service) { described_class.new('job_artifact', file.id) }
+
+      it_behaves_like 'a service that sets next retry time capped at some value'
+    end
+
+    context 'with uploads' do
+      let!(:registry_entry) do
+        create(:geo_file_registry, :avatar, success: false, file_id: file.id, retry_count: 31)
+      end
+
+      let(:file) { create(:upload) }
+      let(:download_service) { described_class.new('avatar', file.id) }
+
+      it_behaves_like 'a service that sets next retry time capped at some value'
+    end
+  end
+
   shared_examples_for 'a service that handles orphaned uploads' do |file_type|
     let(:download_service) { described_class.new(file_type, file.id) }
     let(:registry) { Geo::FileRegistry }
@@ -377,14 +416,14 @@ describe Geo::FileDownloadService do
         expect { described_class.new(:bad, 1).execute }.to raise_error(NameError)
       end
     end
+  end
 
-    def stub_transfer_result(bytes_downloaded:, success: false, primary_missing_file: false)
-      result = double(:transfer_result,
-                      bytes_downloaded: bytes_downloaded,
-                      success: success,
-                      primary_missing_file: primary_missing_file)
-      instance = double("(instance of Gitlab::Geo::Transfer)", download_from_primary: result)
-      allow(Gitlab::Geo::Transfer).to receive(:new).and_return(instance)
-    end
+  def stub_transfer_result(bytes_downloaded:, success: false, primary_missing_file: false)
+    result = double(:transfer_result,
+                    bytes_downloaded: bytes_downloaded,
+                    success: success,
+                    primary_missing_file: primary_missing_file)
+    instance = double("(instance of Gitlab::Geo::Transfer)", download_from_primary: result)
+    allow(Gitlab::Geo::Transfer).to receive(:new).and_return(instance)
   end
 end

ee/spec/services/geo/repository_verification_primary_service_spec.rb

@@ -223,9 +223,9 @@ describe Geo::RepositoryVerificationPrimaryService do
           wiki_verification_checksum: nil,
           last_wiki_verification_ran_at: be_present,
           last_wiki_verification_failure: 'Something went wrong with wiki',
-          repository_retry_at: be_within(100.seconds).of(Time.now + 7.days),
+          repository_retry_at: be_within(100.seconds).of(1.hour.from_now),
           repository_retry_count: 31,
-          wiki_retry_at: be_within(100.seconds).of(Time.now + 7.days),
+          wiki_retry_at: be_within(100.seconds).of(1.hour.from_now),
           wiki_retry_count: 31
         )
       end

ee/spec/services/geo/repository_verification_secondary_service_spec.rb

@@ -104,7 +104,7 @@ describe Geo::RepositoryVerificationSecondaryService, :geo do
 
         expect(registry).to have_attributes(
           "resync_#{type}" => true,
-          "#{type}_retry_at" => be_within(100.seconds).of(Time.now + 7.days),
+          "#{type}_retry_at" => be_within(100.seconds).of(1.hour.from_now),
           "#{type}_retry_count" => 31
         )
       end
@@ -137,7 +137,7 @@ describe Geo::RepositoryVerificationSecondaryService, :geo do
 
         expect(registry).to have_attributes(
           "resync_#{type}" => true,
-          "#{type}_retry_at" => be_within(100.seconds).of(Time.now + 7.days),
+          "#{type}_retry_at" => be_within(100.seconds).of(1.hour.from_now),
           "#{type}_retry_count" => 31
         )
       end