From a28844ea5683d6ae7ae7e37065feee9baa59aeaf Mon Sep 17 00:00:00 2001
From: Stan Hu <stanhu@gmail.com>
Date: Sat, 6 Jul 2019 00:58:13 -0700
Subject: [PATCH] Prevent amplification of ReactiveCachingWorker jobs upon
 failures

When `ReactiveCachingWorker` hits an SSL or other exception that occurs
quickly and reliably, automatically rescheduling a new worker could lead
to excessive number of jobs being scheduled. This happens because not
only does the failed job get rescheduled in a minute, but each Sidekiq
retry will also add even more rescheduled jobs.

In busy instances, this can become an issue because large numbers of
`ReactiveCachingWorker` running can cause high rates of `ExclusiveLease`
reads to occur and possibly saturate the Redis server with queries.

We now disable this automatic retry and rely on Sidekiq to perform its 3
retries with a backoff period.

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/64176
---
 app/models/concerns/reactive_caching.rb                      | 2 +-
 .../sh-disable-reactive-caching-automatic-retries.yml        | 5 +++++
 spec/models/concerns/reactive_caching_spec.rb                | 5 +++--
 3 files changed, 9 insertions(+), 3 deletions(-)
 create mode 100644 changelogs/unreleased/sh-disable-reactive-caching-automatic-retries.yml

diff --git a/app/models/concerns/reactive_caching.rb b/app/models/concerns/reactive_caching.rb
index 6c3962b4c4f..5b3880f94ba 100644
--- a/app/models/concerns/reactive_caching.rb
+++ b/app/models/concerns/reactive_caching.rb
@@ -178,7 +178,7 @@ module ReactiveCaching
 
     def enqueuing_update(*args)
       yield
-    ensure
+
       ReactiveCachingWorker.perform_in(self.class.reactive_cache_refresh_interval, self.class, id, *args)
     end
   end
diff --git a/changelogs/unreleased/sh-disable-reactive-caching-automatic-retries.yml b/changelogs/unreleased/sh-disable-reactive-caching-automatic-retries.yml
new file mode 100644
index 00000000000..a0db68adb78
--- /dev/null
+++ b/changelogs/unreleased/sh-disable-reactive-caching-automatic-retries.yml
@@ -0,0 +1,5 @@
+---
+title: Prevent amplification of ReactiveCachingWorker jobs upon failures
+merge_request: 30432
+author:
+type: performance
diff --git a/spec/models/concerns/reactive_caching_spec.rb b/spec/models/concerns/reactive_caching_spec.rb
index 7faa196623f..e2ab9ddd4a5 100644
--- a/spec/models/concerns/reactive_caching_spec.rb
+++ b/spec/models/concerns/reactive_caching_spec.rb
@@ -206,8 +206,9 @@ describe ReactiveCaching, :use_clean_rails_memory_store_caching do
           expect(read_reactive_cache(instance)).to eq("preexisting")
         end
 
-        it 'enqueues a repeat worker' do
-          expect_reactive_cache_update_queued(instance)
+        it 'does not enqueue a repeat worker' do
+          expect(ReactiveCachingWorker)
+            .not_to receive(:perform_in)
 
           expect { go! }.to raise_error("foo")
         end