Merge branch 'ajk-prevent-overflow-in-hook-backoff-count' into 'master'

Prevent overflow in hook backoff count

See merge request gitlab-org/gitlab!62202

app/models/hooks/web_hook.rb

@@ -3,6 +3,7 @@
 class WebHook < ApplicationRecord
   include Sortable
 
+  MAX_FAILURES = 100
   FAILURE_THRESHOLD = 3 # three strikes
   INITIAL_BACKOFF = 10.minutes
   MAX_BACKOFF = 1.day
@@ -75,6 +76,14 @@ class WebHook < ApplicationRecord
     update!(recent_failures: 0, disabled_until: nil, backoff_count: 0)
   end
 
+  def backoff!
+    update!(disabled_until: next_backoff.from_now, backoff_count: backoff_count.succ.clamp(0, MAX_FAILURES))
+  end
+
+  def failed!
+    update!(recent_failures: recent_failures + 1) if recent_failures < MAX_FAILURES
+  end
+
   # Overridden in ProjectHook and GroupHook, other webhooks are not rate-limited.
   def rate_limit
     nil

app/services/web_hook_service.rb

@@ -27,7 +27,6 @@ class WebHookService
 
   REQUEST_BODY_SIZE_LIMIT = 25.megabytes
   GITLAB_EVENT_HEADER = 'X-Gitlab-Event'
-  MAX_FAILURES = 100
 
   attr_accessor :hook, :data, :hook_name, :request_options
 
@@ -144,10 +143,9 @@ class WebHookService
     if response.success? || response.redirection?
       hook.enable!
     elsif response.internal_server_error?
-      next_backoff = hook.next_backoff
-      hook.update!(disabled_until: next_backoff.from_now, backoff_count: hook.backoff_count + 1)
+      hook.backoff!
     else
-      hook.update!(recent_failures: hook.recent_failures + 1) if hook.recent_failures < MAX_FAILURES
+      hook.failed!
     end
   end
 

changelogs/unreleased/ajk-prevent-overflow-in-hook-backoff-count.yml

+---
+title: Prevent overflows in WebHook#backoff_count
+merge_request: 62202
+author:
+type: fixed

spec/models/hooks/web_hook_spec.rb

@@ -275,6 +275,34 @@ RSpec.describe WebHook do
     end
   end
 
+  describe 'backoff!' do
+    it 'sets disabled_until to the next backoff' do
+      expect { hook.backoff! }.to change(hook, :disabled_until).to(hook.next_backoff.from_now)
+    end
+
+    it 'increments the backoff count' do
+      expect { hook.backoff! }.to change(hook, :backoff_count).by(1)
+    end
+
+    it 'does not let the backoff count exceed the maximum failure count' do
+      hook.backoff_count = described_class::MAX_FAILURES
+
+      expect { hook.backoff! }.not_to change(hook, :backoff_count)
+    end
+  end
+
+  describe 'failed!' do
+    it 'increments the failure count' do
+      expect { hook.failed! }.to change(hook, :recent_failures).by(1)
+    end
+
+    it 'does not allow the failure count to exceed the maximum value' do
+      hook.recent_failures = described_class::MAX_FAILURES
+
+      expect { hook.failed! }.not_to change(hook, :recent_failures)
+    end
+  end
+
   describe '#disable!' do
     it 'disables a hook' do
       expect { hook.disable! }.to change(hook, :executable?).from(true).to(false)

spec/services/web_hook_service_spec.rb

@@ -278,9 +278,10 @@ RSpec.describe WebHookService do
           expect { service_instance.execute }.not_to change(project_hook, :recent_failures)
         end
 
-        it 'sets the disabled_until attribute' do
-          expect { service_instance.execute }
-            .to change(project_hook, :disabled_until).to(project_hook.next_backoff.from_now)
+        it 'backs off' do
+          expect(project_hook).to receive(:backoff!).and_call_original
+
+          expect { service_instance.execute }.to change(project_hook, :disabled_until)
         end
 
         it 'increases the backoff count' do
@@ -295,10 +296,6 @@ RSpec.describe WebHookService do
           it 'sets the disabled_until attribute' do
             expect { service_instance.execute }.to change(project_hook, :disabled_until).to(1.day.from_now)
           end
-
-          it 'sets the last_backoff attribute' do
-            expect { service_instance.execute }.to change(project_hook, :backoff_count).by(1)
-          end
         end
 
         context 'when we have backed-off many many times' do
@@ -309,10 +306,6 @@ RSpec.describe WebHookService do
           it 'sets the disabled_until attribute' do
             expect { service_instance.execute }.to change(project_hook, :disabled_until).to(1.day.from_now)
           end
-
-          it 'sets the last_backoff attribute' do
-            expect { service_instance.execute }.to change(project_hook, :backoff_count).by(1)
-          end
         end
       end