Don't use `Redis#keys` in the circuitbreakerbvl-do-not-use-redis-keys

6 files changed, 33 insertions, 36 deletions

diff --git a/changelogs/unreleased/bvl-do-not-use-redis-keys.yml b/changelogs/unreleased/bvl-do-not-use-redis-keys.yml
new file mode 100644
index 00000000000..f703aad2065
--- /dev/null
+++ b/changelogs/unreleased/bvl-do-not-use-redis-keys.yml
@@ -0,0 +1,5 @@
+---
+title: Forbid the usage of `Redis#keys`
+merge_request: 14889
+author:
+type: fixed
diff --git a/lib/gitlab/git/storage/circuit_breaker.rb b/lib/gitlab/git/storage/circuit_breaker.rb
index 1eaa2d83fb6..ba56aa2baf7 100644
--- a/lib/gitlab/git/storage/circuit_breaker.rb
+++ b/lib/gitlab/git/storage/circuit_breaker.rb
@@ -18,7 +18,7 @@ module Gitlab
           pattern = "#{Gitlab::Git::Storage::REDIS_KEY_PREFIX}*"
 
           Gitlab::Git::Storage.redis.with do |redis|
-            all_storage_keys = redis.keys(pattern)
+            all_storage_keys = redis.scan_each(match: pattern).to_a
             redis.del(*all_storage_keys) unless all_storage_keys.empty?
           end
 
diff --git a/lib/gitlab/git/storage/health.rb b/lib/gitlab/git/storage/health.rb
index 1564e94b7f7..7049772fe3b 100644
--- a/lib/gitlab/git/storage/health.rb
+++ b/lib/gitlab/git/storage/health.rb
@@ -23,26 +23,36 @@ module Gitlab
           end
         end
 
-        def self.all_keys_for_storages(storage_names, redis)
+        private_class_method def self.all_keys_for_storages(storage_names, redis)
           keys_per_storage = {}
 
           redis.pipelined do
             storage_names.each do |storage_name|
               pattern = pattern_for_storage(storage_name)
+              matched_keys = redis.scan_each(match: pattern)
 
-              keys_per_storage[storage_name] = redis.keys(pattern)
+              keys_per_storage[storage_name] = matched_keys
             end
           end
 
-          keys_per_storage
+          # We need to make sure each lazy-loaded `Enumerator` for matched keys
+          # is loaded into an array.
+          #
+          # Otherwise it would be loaded in the second `Redis#pipelined` block
+          # within `.load_for_keys`. In this pipelined call, the active
+          # Redis-client changes again, so the values would not be available
+          # until the end of that pipelined-block.
+          keys_per_storage.each do |storage_name, key_future|
+            keys_per_storage[storage_name] = key_future.to_a
+          end
         end
 
-        def self.load_for_keys(keys_per_storage, redis)
+        private_class_method def self.load_for_keys(keys_per_storage, redis)
           info_for_keys = {}
 
           redis.pipelined do
             keys_per_storage.each do |storage_name, keys_future|
-              info_for_storage = keys_future.value.map do |key|
+              info_for_storage = keys_future.map do |key|
                 { name: key, failure_count: redis.hget(key, :failure_count) }
               end
 
diff --git a/spec/lib/gitlab/git/storage/circuit_breaker_spec.rb b/spec/lib/gitlab/git/storage/circuit_breaker_spec.rb
index 98cf7966dad..4edd360d11d 100644
--- a/spec/lib/gitlab/git/storage/circuit_breaker_spec.rb
+++ b/spec/lib/gitlab/git/storage/circuit_breaker_spec.rb
@@ -49,6 +49,10 @@ describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state:
 
       expect(key_exists).to be_falsey
     end
+
+    it 'does not break when there are no keys in redis' do
+      expect { described_class.reset_all! }.not_to raise_error
+    end
   end
 
   describe '.for_storage' do
diff --git a/spec/lib/gitlab/git/storage/health_spec.rb b/spec/lib/gitlab/git/storage/health_spec.rb
index 2d3af387971..4a14a5201d1 100644
--- a/spec/lib/gitlab/git/storage/health_spec.rb
+++ b/spec/lib/gitlab/git/storage/health_spec.rb
@@ -20,36 +20,6 @@ describe Gitlab::Git::Storage::Health, clean_gitlab_redis_shared_state: true, br
     end
   end
 
-  describe '.load_for_keys' do
-    let(:subject) do
-      results = Gitlab::Git::Storage.redis.with do |redis|
-        fake_future = double
-        allow(fake_future).to receive(:value).and_return([host1_key])
-        described_class.load_for_keys({ 'broken' => fake_future }, redis)
-      end
-
-      # Make sure the `Redis#future is loaded
-      results.inject({}) do |result, (name, info)|
-        info.each { |i| i[:failure_count] = i[:failure_count].value.to_i }
-
-        result[name] = info
-
-        result
-      end
-    end
-
-    it 'loads when there is no info in redis' do
-      expect(subject).to eq('broken' => [{ name: host1_key, failure_count: 0 }])
-    end
-
-    it 'reads the correct values for a storage from redis' do
-      set_in_redis(host1_key, 5)
-      set_in_redis(host2_key, 7)
-
-      expect(subject).to eq('broken' => [{ name: host1_key, failure_count: 5 }])
-    end
-  end
-
   describe '.for_all_storages' do
     it 'loads health status for all configured storages' do
       healths = described_class.for_all_storages
diff --git a/spec/support/redis_without_keys.rb b/spec/support/redis_without_keys.rb
new file mode 100644
index 00000000000..6220167dee6
--- /dev/null
+++ b/spec/support/redis_without_keys.rb
@@ -0,0 +1,8 @@
+class Redis
+  ForbiddenCommand = Class.new(StandardError)
+
+  def keys(*args)
+    raise ForbiddenCommand.new("Don't use `Redis#keys` as it iterates over all "\
+                               "keys in redis. Use `Redis#scan_each` instead.")
+  end
+end