Only track accessibility once

2 files changed, 46 insertions, 11 deletions

diff --git a/lib/gitlab/git/storage/circuit_breaker.rb b/lib/gitlab/git/storage/circuit_breaker.rb
index 237b4598730..c3ef82d4e98 100644
--- a/lib/gitlab/git/storage/circuit_breaker.rb
+++ b/lib/gitlab/git/storage/circuit_breaker.rb
@@ -46,10 +46,6 @@ module Gitlab
         def perform
           return yield unless Feature.enabled?('git_storage_circuit_breaker')
 
-          if circuit_broken?
-            raise Gitlab::Git::Storage::CircuitOpen.new("Circuit for #{storage} open", failure_wait_time)
-          end
-
           check_storage_accessible!
 
           yield
@@ -70,14 +66,24 @@ module Gitlab
         # When the storage appears not available, and the memoized value is `false`
         # we might want to try again.
         def storage_available?
-          @storage_available ||= Gitlab::Git::Storage::ForkedStorageCheck.storage_available?(storage_path, storage_timeout)
-        end
+          return @storage_available if @storage_available
 
-        def check_storage_accessible!
-          if storage_available?
+          if @storage_available = Gitlab::Git::Storage::ForkedStorageCheck
+                                    .storage_available?(storage_path, storage_timeout)
             track_storage_accessible
           else
             track_storage_inaccessible
+          end
+
+          @storage_available
+        end
+
+        def check_storage_accessible!
+          if circuit_broken?
+            raise Gitlab::Git::Storage::CircuitOpen.new("Circuit for #{storage} is broken", failure_wait_time)
+          end
+
+          unless storage_available?
             raise Gitlab::Git::Storage::Inaccessible.new("#{storage} not accessible", failure_wait_time)
           end
         end
diff --git a/spec/lib/gitlab/git/storage/circuit_breaker_spec.rb b/spec/lib/gitlab/git/storage/circuit_breaker_spec.rb
index 6fab224ad0d..b2886628601 100644
--- a/spec/lib/gitlab/git/storage/circuit_breaker_spec.rb
+++ b/spec/lib/gitlab/git/storage/circuit_breaker_spec.rb
@@ -120,19 +120,48 @@ describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state:
     end
   end
 
-  describe '#check_storage_accessible!' do
+  describe "storage_available?" do
     context 'when the storage is available' do
       it 'tracks that the storage was accessible an raises the error' do
         expect(circuit_breaker).to receive(:track_storage_accessible)
 
-        circuit_breaker.check_storage_accessible!
+        circuit_breaker.storage_available?
+      end
+
+      it 'only performs the check once' do
+        expect(Gitlab::Git::Storage::ForkedStorageCheck)
+          .to receive(:storage_available?).once.and_call_original
+
+        2.times { circuit_breaker.storage_available? }
+      end
+    end
+
+    context 'when storage is not available' do
+      let(:circuit_breaker) { described_class.new('broken') }
+
+      it 'tracks that the storage was inaccessible' do
+        expect(circuit_breaker).to receive(:track_storage_inaccessible)
+
+        circuit_breaker.storage_available?
+      end
+    end
+  end
+
+  describe '#check_storage_accessible!' do
+    it 'raises an exception with retry time when the circuit is open' do
+      allow(circuit_breaker).to receive(:circuit_broken?).and_return(true)
+
+      expect { circuit_breaker.check_storage_accessible! }
+        .to raise_error do |exception|
+        expect(exception).to be_kind_of(Gitlab::Git::Storage::CircuitOpen)
+        expect(exception.retry_after).to eq(30)
       end
     end
 
     context 'when the storage is not available' do
       let(:circuit_breaker) { described_class.new('broken') }
 
-      it 'tracks that the storage was unavailable and raises an error with retry time' do
+      it 'raises an error' do
         expect(circuit_breaker).to receive(:track_storage_inaccessible)
 
         expect { circuit_breaker.check_storage_accessible! }