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! }