Merge branch '352245-increase-retry-lease-for-LogExecutionService' into 'master'

Conditionally raise when lease cannot be obtained in LogExecutionService

See merge request gitlab-org/gitlab!81770

app/services/web_hooks/log_execution_service.rb

@@ -42,10 +42,20 @@ module WebHooks
           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

spec/services/web_hooks/log_execution_service_spec.rb

@@ -4,6 +4,7 @@ require 'spec_helper'
 
 RSpec.describe WebHooks::LogExecutionService do
   include ExclusiveLeaseHelpers
+  using RSpec::Parameterized::TableSyntax
 
   describe '#execute' do
     around do |example|
@@ -34,11 +35,13 @@ RSpec.describe WebHooks::LogExecutionService do
       expect(WebHookLog.recent.first).to have_attributes(data)
     end
 
+    context 'obtaining an exclusive lease' do
+      let(:lease_key) { "web_hooks:update_hook_failure_state:#{project_hook.id}" }
+
       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)
 
-      lease_key = "web_hooks:update_hook_failure_state:#{project_hook.id}"
         lease = stub_exclusive_lease(lease_key, timeout: described_class::LOCK_TTL)
 
         expect(lease).to receive(:try_obtain)
@@ -46,6 +49,35 @@ RSpec.describe WebHooks::LogExecutionService do
         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
       it 'does not increment the failure count' do
         expect { service.execute }.not_to change(project_hook, :recent_failures)