diff --git a/app/services/web_hooks/log_execution_service.rb b/app/services/web_hooks/log_execution_service.rb index 8a6ba1d23803d8ea98bcba5d446e836d3c250fb2..04b9ff593825c2c365e282e60b54505091d26e40 100644 --- a/app/services/web_hooks/log_execution_service.rb +++ b/app/services/web_hooks/log_execution_service.rb @@ -42,10 +42,20 @@ def update_hook_failure_state hook.failed! end end + rescue Gitlab::ExclusiveLeaseHelpers::FailedToObtainLockError + raise if raise_lock_error? end def lock_name "web_hooks:update_hook_failure_state:#{hook.id}" end + + # Allow an error to be raised after failing to obtain a lease only if the hook + # is not already in the correct failure state. + def raise_lock_error? + hook.reset # Reload so properties are guaranteed to be current. + + hook.executable? != (response_category == :ok) + end end end diff --git a/spec/services/web_hooks/log_execution_service_spec.rb b/spec/services/web_hooks/log_execution_service_spec.rb index 8dceba164326d11195cafab3751a3babc21ea31f..7e9a8de2dee667f8d28a4b810113717e91ccb562 100644 --- a/spec/services/web_hooks/log_execution_service_spec.rb +++ b/spec/services/web_hooks/log_execution_service_spec.rb @@ -4,6 +4,7 @@ RSpec.describe WebHooks::LogExecutionService do include ExclusiveLeaseHelpers + using RSpec::Parameterized::TableSyntax describe '#execute' do around do |example| @@ -34,16 +35,47 @@ expect(WebHookLog.recent.first).to have_attributes(data) end - it 'updates failure state using a lease that ensures fresh state is written' do - service = described_class.new(hook: project_hook, log_data: data, response_category: :error) - WebHook.find(project_hook.id).update!(backoff_count: 1) + context 'obtaining an exclusive lease' do + let(:lease_key) { "web_hooks:update_hook_failure_state:#{project_hook.id}" } - lease_key = "web_hooks:update_hook_failure_state:#{project_hook.id}" - lease = stub_exclusive_lease(lease_key, timeout: described_class::LOCK_TTL) + it 'updates failure state using a lease that ensures fresh state is written' do + service = described_class.new(hook: project_hook, log_data: data, response_category: :error) + WebHook.find(project_hook.id).update!(backoff_count: 1) - expect(lease).to receive(:try_obtain) - expect(lease).to receive(:cancel) - expect { service.execute }.to change { WebHook.find(project_hook.id).backoff_count }.to(2) + lease = stub_exclusive_lease(lease_key, timeout: described_class::LOCK_TTL) + + expect(lease).to receive(:try_obtain) + expect(lease).to receive(:cancel) + expect { service.execute }.to change { WebHook.find(project_hook.id).backoff_count }.to(2) + end + + context 'when a lease cannot be obtained' do + where(:response_category, :executable, :needs_updating) do + :ok | true | false + :ok | false | true + :failed | true | true + :failed | false | false + :error | true | true + :error | false | false + end + + with_them do + subject(:service) { described_class.new(hook: project_hook, log_data: data, response_category: response_category) } + + before do + stub_exclusive_lease_taken(lease_key, timeout: described_class::LOCK_TTL) + allow(project_hook).to receive(:executable?).and_return(executable) + end + + it 'raises an error if the hook needs to be updated' do + if needs_updating + expect { service.execute }.to raise_error(Gitlab::ExclusiveLeaseHelpers::FailedToObtainLockError) + else + expect { service.execute }.not_to raise_error + end + end + end + end end context 'when response_category is :ok' do