From 35b281ea5227501bd5431ff0d49735e08d5b5c10 Mon Sep 17 00:00:00 2001
From: Alex Kalderimis <akalderimis@gitlab.com>
Date: Thu, 19 May 2022 13:46:10 -0400
Subject: [PATCH 01/10] Always destroy webhooks synchronously

Previously we fell-back to asynchronous batch deletion if there were
many logs, but we no longer need to do that now that we do not have a
foreign key preventing deletion.

Now we can delete the hook in all cases, and then start a background
worker to delete the logs. This is actually unnecessary, since they will
be truncated over time anyway, but it is nice to minimize table bloat.

Changelog: fixed
---
 app/models/hooks/web_hook_log.rb              |  13 +++
 app/services/web_hooks/destroy_service.rb     |  68 +++---------
 app/workers/all_queues.yml                    |   9 ++
 app/workers/web_hooks/log_destroy_worker.rb   |  28 +++++
 config/sidekiq_queues.yml                     |   2 +
 spec/models/hooks/web_hook_log_spec.rb        |  14 +++
 .../services/projects/destroy_service_spec.rb |   1 -
 .../web_hooks/destroy_service_spec.rb         | 100 +++++++++++++-----
 spec/workers/web_hooks/destroy_worker_spec.rb |   9 +-
 .../web_hooks/log_destroy_worker_spec.rb      |  66 ++++++++++++
 10 files changed, 224 insertions(+), 86 deletions(-)
 create mode 100644 app/workers/web_hooks/log_destroy_worker.rb
 create mode 100644 spec/workers/web_hooks/log_destroy_worker_spec.rb

diff --git a/app/models/hooks/web_hook_log.rb b/app/models/hooks/web_hook_log.rb
index 8c0565e4a38497..aaf57c2092a638 100644
--- a/app/models/hooks/web_hook_log.rb
+++ b/app/models/hooks/web_hook_log.rb
@@ -7,6 +7,8 @@ class WebHookLog < ApplicationRecord
   include CreatedAtFilterable
   include PartitionedTable
 
+  BATCH_SIZE = 1000
+
   self.primary_key = :id
 
   partitioned_by :created_at, strategy: :monthly, retain_for: 3.months
@@ -26,6 +28,17 @@ def self.recent
       .order(created_at: :desc)
   end
 
+  def self.delete_batch_for(web_hook)
+    where(web_hook: web_hook).limit(BATCH_SIZE).delete_all
+  end
+
+  def self.delete_for(web_hook)
+    loop do
+      count = delete_batch_for(web_hook)
+      break if count < BATCH_SIZE
+    end
+  end
+
   def success?
     response_status =~ /^2/
   end
diff --git a/app/services/web_hooks/destroy_service.rb b/app/services/web_hooks/destroy_service.rb
index 58117985b56d08..2efbcb1d9e909e 100644
--- a/app/services/web_hooks/destroy_service.rb
+++ b/app/services/web_hooks/destroy_service.rb
@@ -4,75 +4,37 @@ module WebHooks
   class DestroyService
     include BaseServiceUtility
 
-    BATCH_SIZE = 1000
-    LOG_COUNT_THRESHOLD = 10000
-
     DestroyError = Class.new(StandardError)
 
-    attr_accessor :current_user, :web_hook
+    attr_accessor :current_user
 
     def initialize(current_user)
       @current_user = current_user
     end
 
+    # Destroy the hook immediately, schedule the logs for deletion
     def execute(web_hook)
-      @web_hook = web_hook
-
-      async = false
-      # For a better user experience, it's better if the Web hook is
-      # destroyed right away without waiting for Sidekiq. However, if
-      # there are a lot of Web hook logs, we will need more time to
-      # clean them up, so schedule a Sidekiq job to do this.
-      if needs_async_destroy?
-        Gitlab::AppLogger.info("User #{current_user&.id} scheduled a deletion of hook ID #{web_hook.id}")
-        async_destroy(web_hook)
-        async = true
-      else
-        sync_destroy(web_hook)
-      end
-
-      success({ async: async })
-    end
-
-    def sync_destroy(web_hook)
-      @web_hook = web_hook
+      hook_id = web_hook.id
 
-      delete_web_hook_logs
-      result = web_hook.destroy
+      if web_hook.destroy
+        WebHooks::LogDestroyWorker.perform_async(current_user&.id, hook_id)
+        Gitlab::AppLogger.info("User #{current_user&.id} scheduled a deletion of logs for hook ID #{hook_id}")
 
-      if result
-        success({ async: false })
+        ServiceResponse.success(payload: { async: false })
       else
-        error("Unable to destroy #{web_hook.model_name.human}")
+        ServiceResponse.error(message: "Unable to destroy #{web_hook.model_name.human}")
       end
     end
 
-    private
+    # Backwards compatibility with WebHooks::DestroyWorker
+    alias_method :sync_destroy, :execute
 
-    def async_destroy(web_hook)
-      WebHooks::DestroyWorker.perform_async(current_user.id, web_hook.id)
-    end
-
-    # rubocop: disable CodeReuse/ActiveRecord
-    def needs_async_destroy?
-      web_hook.web_hook_logs.limit(LOG_COUNT_THRESHOLD).count == LOG_COUNT_THRESHOLD
-    end
-    # rubocop: enable CodeReuse/ActiveRecord
-
-    def delete_web_hook_logs
-      loop do
-        count = delete_web_hook_logs_in_batches
-        break if count < BATCH_SIZE
-      end
-    end
+    def delete_web_hook_logs(hook_id)
+      WebHookLog.delete_for(hook_id)
 
-    # rubocop: disable CodeReuse/ActiveRecord
-    def delete_web_hook_logs_in_batches
-      # We can't use EachBatch because that does an ORDER BY id, which can
-      # easily time out. We don't actually care about ordering when
-      # we are deleting these rows.
-      web_hook.web_hook_logs.limit(BATCH_SIZE).delete_all
+      ServiceResponse.success
+    rescue StandardError => ex
+      ServiceResponse.error(message: ex.message)
     end
-    # rubocop: enable CodeReuse/ActiveRecord
   end
 end
diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml
index e7ea39c913e853..c8dfd2e4e6c0d8 100644
--- a/app/workers/all_queues.yml
+++ b/app/workers/all_queues.yml
@@ -3056,6 +3056,15 @@
   :weight: 1
   :idempotent: true
   :tags: []
+- :name: web_hooks_log_destroy
+  :worker_name: WebHooks::LogDestroyWorker
+  :feature_category: :integrations
+  :has_external_dependencies:
+  :urgency: :high
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent: true
+  :tags: []
 - :name: web_hooks_log_execution
   :worker_name: WebHooks::LogExecutionWorker
   :feature_category: :integrations
diff --git a/app/workers/web_hooks/log_destroy_worker.rb b/app/workers/web_hooks/log_destroy_worker.rb
new file mode 100644
index 00000000000000..f093df1964feff
--- /dev/null
+++ b/app/workers/web_hooks/log_destroy_worker.rb
@@ -0,0 +1,28 @@
+# frozen_string_literal: true
+
+module WebHooks
+  class LogDestroyWorker
+    include ApplicationWorker
+
+    data_consistency :always
+    feature_category :integrations
+    urgency :high
+
+    idempotent!
+
+    def perform(user_id, hook_id)
+      user = User.find_by_id(user_id)
+
+      return unless user
+
+      result = ::WebHooks::DestroyService.new(user).delete_web_hook_logs(hook_id)
+
+      return result if result[:status] == :success
+
+      e = ::WebHooks::DestroyService::DestroyError.new(result[:message])
+      Gitlab::ErrorTracking.track_exception(e, web_hook_id: hook_id)
+
+      raise e
+    end
+  end
+end
diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml
index f5d9708b55bc57..9f2eede1c47fb7 100644
--- a/config/sidekiq_queues.yml
+++ b/config/sidekiq_queues.yml
@@ -457,6 +457,8 @@
   - 1
 - - web_hooks_destroy
   - 1
+- - web_hooks_log_destroy
+  - 1
 - - web_hooks_log_execution
   - 1
 - - wikis_git_garbage_collect
diff --git a/spec/models/hooks/web_hook_log_spec.rb b/spec/models/hooks/web_hook_log_spec.rb
index 9cfbb14e0875a2..2e0c508eba1504 100644
--- a/spec/models/hooks/web_hook_log_spec.rb
+++ b/spec/models/hooks/web_hook_log_spec.rb
@@ -48,6 +48,20 @@
     end
   end
 
+  describe '.delete_for' do
+    it 'deletes records in batches' do
+      stub_const("#{described_class}::BATCH_SIZE", 2)
+
+      hook = create(:project_hook)
+      create_list(:web_hook_log, 5, web_hook: hook)
+
+      expect(described_class).to receive(:delete_batch_for).exactly(3).times.and_call_original
+
+      expect { described_class.delete_for(hook.id) }
+        .to change(described_class, :count).by(-5)
+    end
+  end
+
   describe '#success?' do
     let(:web_hook_log) { build(:web_hook_log, response_status: status) }
 
diff --git a/spec/services/projects/destroy_service_spec.rb b/spec/services/projects/destroy_service_spec.rb
index adea79509b3df1..86e9a176798d17 100644
--- a/spec/services/projects/destroy_service_spec.rb
+++ b/spec/services/projects/destroy_service_spec.rb
@@ -492,7 +492,6 @@
       expect do
         destroy_project(project, user)
       end.to change(WebHook, :count).by(-2)
-         .and change(WebHookLog, :count).by(-1)
     end
 
     context 'when an error is raised deleting webhooks' do
diff --git a/spec/services/web_hooks/destroy_service_spec.rb b/spec/services/web_hooks/destroy_service_spec.rb
index 5269fe08ac0ceb..72e533f3817a13 100644
--- a/spec/services/web_hooks/destroy_service_spec.rb
+++ b/spec/services/web_hooks/destroy_service_spec.rb
@@ -7,50 +7,92 @@
 
   subject { described_class.new(user) }
 
-  shared_examples 'batched destroys' do
-    it 'destroys all hooks in batches' do
-      stub_const("#{described_class}::BATCH_SIZE", 1)
-      expect(subject).to receive(:delete_web_hook_logs_in_batches).exactly(4).times.and_call_original
+  describe '#execute' do
+    shared_examples 'destroys hook and schedules log deletion' do
+      it 'is successful' do
+        expect(subject.execute(hook)).to be_success
+      end
 
-      expect do
-        status = subject.execute(hook)
-        expect(status[:async]).to be false
+      it 'destroys the hook' do
+        expect { subject.execute(hook) }.to change(WebHook, :count).from(1).to(0)
       end
-        .to change { WebHook.count }.from(1).to(0)
-        .and change { WebHookLog.count }.from(3).to(0)
-    end
 
-    it 'returns an error if sync destroy fails' do
-      expect(hook).to receive(:destroy).and_return(false)
+      it 'does not destroy logs' do
+        expect { subject.execute(hook) }.not_to change(WebHookLog, :count)
+      end
+
+      it 'schedules the destruction of logs' do
+        expect(WebHooks::LogDestroyWorker).to receive(:perform_async).with(user.id, hook.id)
+        expect(Gitlab::AppLogger).to receive(:info).with(match(/scheduled a deletion of logs/))
+
+        subject.execute(hook)
+      end
+
+      context 'when the hook fails to destroy' do
+        before do
+          allow(hook).to receive(:destroy).and_return(false)
+        end
 
-      result = subject.sync_destroy(hook)
+        it 'is not a success' do
+          r = subject.execute(hook)
 
-      expect(result[:status]).to eq(:error)
-      expect(result[:message]).to eq("Unable to destroy #{hook.model_name.human}")
+          expect(r).to be_error
+          expect(r[:message]).to match %r{Unable to destroy}
+        end
+      end
     end
 
-    it 'schedules an async delete' do
-      stub_const('WebHooks::DestroyService::LOG_COUNT_THRESHOLD', 1)
+    context 'with system hook' do
+      let!(:hook) { create(:system_hook, url: "http://example.com") }
+      let!(:log) { create_list(:web_hook_log, 3, web_hook: hook) }
 
-      expect(WebHooks::DestroyWorker).to receive(:perform_async).with(user.id, hook.id).and_call_original
+      it_behaves_like 'destroys hook and schedules log deletion'
+    end
 
-      status = subject.execute(hook)
+    context 'with project hook' do
+      let!(:hook) { create(:project_hook) }
+      let!(:log) { create_list(:web_hook_log, 3, web_hook: hook) }
 
-      expect(status[:async]).to be true
+      it_behaves_like 'destroys hook and schedules log deletion'
     end
   end
 
-  context 'with system hook' do
-    let!(:hook) { create(:system_hook, url: "http://example.com") }
-    let!(:log) { create_list(:web_hook_log, 3, web_hook: hook) }
+  describe '#delete_web_hook_logs' do
+    shared_examples 'deletes web hook logs for hook' do
+      before do
+        hook.destroy! # expected to be called _after_ hook destruction
+      end
+
+      it 'deletes the logs' do
+        expect { subject.delete_web_hook_logs(hook.id) }
+          .to change(WebHookLog, :count).from(3).to(0)
+      end
 
-    it_behaves_like 'batched destroys'
-  end
+      context 'when it encounters an error' do
+        before do
+          allow(WebHookLog).to receive(:delete_for).and_raise(StandardError.new('bang'))
+        end
+
+        it 'reports the error' do
+          expect(subject.delete_web_hook_logs(hook.id))
+            .to be_error
+            .and have_attributes(message: 'bang')
+        end
+      end
+    end
+
+    context 'with system hook' do
+      let!(:hook) { create(:system_hook, url: "http://example.com") }
+      let!(:log) { create_list(:web_hook_log, 3, web_hook: hook) }
 
-  context 'with project hook' do
-    let!(:hook) { create(:project_hook) }
-    let!(:log) { create_list(:web_hook_log, 3, web_hook: hook) }
+      it_behaves_like 'deletes web hook logs for hook'
+    end
 
-    it_behaves_like 'batched destroys'
+    context 'with project hook' do
+      let!(:hook) { create(:project_hook) }
+      let!(:log) { create_list(:web_hook_log, 3, web_hook: hook) }
+
+      it_behaves_like 'deletes web hook logs for hook'
+    end
   end
 end
diff --git a/spec/workers/web_hooks/destroy_worker_spec.rb b/spec/workers/web_hooks/destroy_worker_spec.rb
index fd26c8591ee6b1..5f3ae702bb8a17 100644
--- a/spec/workers/web_hooks/destroy_worker_spec.rb
+++ b/spec/workers/web_hooks/destroy_worker_spec.rb
@@ -20,9 +20,10 @@
       let!(:other_log) { create(:web_hook_log, web_hook: other_hook) }
 
       it "deletes the Web hook and logs", :aggregate_failures do
+        expect(WebHooks::LogDestroyWorker).to receive(:perform_async)
+
         expect { subject.perform(user.id, hook.id) }
-          .to change { WebHookLog.count }.from(2).to(1)
-          .and change { WebHook.count }.from(2).to(1)
+          .to change { WebHook.count }.from(2).to(1)
 
         expect(WebHook.find(other_hook.id)).to be_present
         expect(WebHookLog.find(other_log.id)).to be_present
@@ -30,7 +31,9 @@
 
       it "raises and tracks an error if destroy failed" do
         allow_next_instance_of(::WebHooks::DestroyService) do |instance|
-          expect(instance).to receive(:sync_destroy).with(anything).and_return({ status: :error, message: "failed" })
+          expect(instance).to receive(:sync_destroy)
+            .with(anything)
+            .and_return(ServiceResponse.error(message: "failed"))
         end
 
         expect(Gitlab::ErrorTracking).to receive(:track_exception)
diff --git a/spec/workers/web_hooks/log_destroy_worker_spec.rb b/spec/workers/web_hooks/log_destroy_worker_spec.rb
new file mode 100644
index 00000000000000..98d5ecdafc6354
--- /dev/null
+++ b/spec/workers/web_hooks/log_destroy_worker_spec.rb
@@ -0,0 +1,66 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe WebHooks::LogDestroyWorker do
+  let_it_be(:project) { create(:project) }
+  let_it_be(:user) { create(:user) }
+
+  before_all do
+    project.add_maintainer(user)
+  end
+
+  subject { described_class.new }
+
+  describe "#perform" do
+    context 'with a Web hook' do
+      let!(:hook) { create(:project_hook, project: project) }
+      let!(:other_hook) { create(:project_hook, project: project) }
+      let!(:log) { create(:web_hook_log, web_hook: hook) }
+      let!(:other_log) { create(:web_hook_log, web_hook: other_hook) }
+
+      it "deletes the relevant logs", :aggregate_failures do
+        hook.destroy! # It does not depend on the presence of the hook
+
+        expect { subject.perform(user.id, hook.id) }
+          .to change { WebHookLog.count }.by(-1)
+
+        expect(WebHook.find(other_hook.id)).to be_present
+        expect(WebHookLog.find(other_log.id)).to be_present
+      end
+
+      it "raises and tracks an error if destroy failed" do
+        allow_next_instance_of(::WebHooks::DestroyService) do |instance|
+          expect(instance).to receive(:delete_web_hook_logs)
+            .with(hook.id)
+            .and_return(ServiceResponse.error(message: "failed"))
+        end
+
+        expect(Gitlab::ErrorTracking)
+          .to receive(:track_exception)
+          .with(an_instance_of(::WebHooks::DestroyService::DestroyError), web_hook_id: hook.id)
+          .and_call_original
+
+        expect { subject.perform(user.id, hook.id) }.to raise_error(::WebHooks::DestroyService::DestroyError)
+      end
+
+      context 'with unknown hook' do
+        it 'does not raise an error' do
+          expect { subject.perform(user.id, non_existing_record_id) }.not_to raise_error
+
+          expect(WebHook.count).to eq(2)
+          expect(WebHookLog.count).to eq(2)
+        end
+      end
+
+      context 'with unknown user' do
+        it 'does not raise an error' do
+          expect { subject.perform(non_existing_record_id, hook.id) }.not_to raise_error
+
+          expect(WebHook.count).to eq(2)
+          expect(WebHookLog.count).to eq(2)
+        end
+      end
+    end
+  end
+end
-- 
GitLab


From 3532abfcc851b0a1105f863310e277b03e4e6dc8 Mon Sep 17 00:00:00 2001
From: Alex Kalderimis <akalderimis@gitlab.com>
Date: Thu, 19 May 2022 22:00:19 -0400
Subject: [PATCH 02/10] Support error propagation

---
 app/services/service_response.rb       | 11 +++++++
 spec/services/service_response_spec.rb | 42 ++++++++++++++++++++++++++
 2 files changed, 53 insertions(+)

diff --git a/app/services/service_response.rb b/app/services/service_response.rb
index 6bc394d2ae25f8..49274891ffe90f 100644
--- a/app/services/service_response.rb
+++ b/app/services/service_response.rb
@@ -18,6 +18,17 @@ def initialize(status:, message: nil, payload: {}, http_status: nil)
     self.http_status = http_status
   end
 
+  def track_exception(as: StandardError, track_only: false, **extra_data)
+    if error?
+      e = as.new(message)
+      Gitlab::ErrorTracking.track_exception(e, **extra_data)
+
+      raise e unless track_only
+    end
+
+    self
+  end
+
   def [](key)
     to_h[key]
   end
diff --git a/spec/services/service_response_spec.rb b/spec/services/service_response_spec.rb
index 082ee4ddc67399..223b6f6ae5a957 100644
--- a/spec/services/service_response_spec.rb
+++ b/spec/services/service_response_spec.rb
@@ -2,7 +2,10 @@
 
 require 'fast_spec_helper'
 
+require 're2'
+
 require_relative '../../app/services/service_response'
+require_relative '../../lib/gitlab/error_tracking'
 
 RSpec.describe ServiceResponse do
   describe '.success' do
@@ -94,4 +97,43 @@
       expect(described_class.error(message: 'error message').errors).to eq(['error message'])
     end
   end
+
+  describe '#track_exception' do
+    context 'when successful' do
+      let(:response) { described_class.success }
+
+      it 'returns self' do
+        expect(response.track_exception).to be response
+      end
+    end
+
+    context 'when an error' do
+      let(:response) { described_class.error(message: 'bang') }
+
+      it 'tracks and raises' do
+        expect(::Gitlab::ErrorTracking).to receive(:track_exception).with(StandardError.new('bang'))
+
+        expect { response.track_exception }.to raise_error('bang')
+      end
+
+      it 'allows specification of error class' do
+        error = Class.new(StandardError)
+        expect(::Gitlab::ErrorTracking).to receive(:track_exception).with(error.new('bang'))
+
+        expect { response.track_exception(as: error) }.to raise_error(an_instance_of(error))
+      end
+
+      it 'allows extra data for tracking' do
+        expect(::Gitlab::ErrorTracking).to receive(:track_exception).with(StandardError.new('bang'), foo: 1, bar: 2)
+
+        expect { response.track_exception(foo: 1, bar: 2) }.to raise_error(StandardError)
+      end
+
+      it 'allows the error to be tracked only' do
+        expect(::Gitlab::ErrorTracking).to receive(:track_exception).with(StandardError.new('bang'))
+
+        expect(response.track_exception(track_only: true)).to be response
+      end
+    end
+  end
 end
-- 
GitLab


From a92da26c4ddd8013a2a4162532aced682a58a082 Mon Sep 17 00:00:00 2001
From: Alex Kalderimis <akalderimis@gitlab.com>
Date: Fri, 20 May 2022 11:02:16 -0400
Subject: [PATCH 03/10] Update queue definitions

---
 app/workers/all_queues.yml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml
index c8dfd2e4e6c0d8..87a621980c7981 100644
--- a/app/workers/all_queues.yml
+++ b/app/workers/all_queues.yml
@@ -3060,7 +3060,7 @@
   :worker_name: WebHooks::LogDestroyWorker
   :feature_category: :integrations
   :has_external_dependencies:
-  :urgency: :high
+  :urgency: :low
   :resource_boundary: :unknown
   :weight: 1
   :idempotent: true
-- 
GitLab


From 14fcd37ed201ceacab92bdab3cc29cd715a6b9ca Mon Sep 17 00:00:00 2001
From: Alex Kalderimis <akalderimis@gitlab.com>
Date: Fri, 20 May 2022 13:08:33 -0400
Subject: [PATCH 04/10] Test idempotency

---
 spec/workers/web_hooks/log_destroy_worker_spec.rb | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/spec/workers/web_hooks/log_destroy_worker_spec.rb b/spec/workers/web_hooks/log_destroy_worker_spec.rb
index 98d5ecdafc6354..77c3c2901fb611 100644
--- a/spec/workers/web_hooks/log_destroy_worker_spec.rb
+++ b/spec/workers/web_hooks/log_destroy_worker_spec.rb
@@ -29,6 +29,13 @@
         expect(WebHookLog.find(other_log.id)).to be_present
       end
 
+      it 'is idempotent' do
+        subject.perform({ 'hook_id' => hook.id })
+        subject.perform({ 'hook_id' => hook.id })
+
+        expect(hook.web_hook_logs).to be_none
+      end
+
       it "raises and tracks an error if destroy failed" do
         allow_next_instance_of(::WebHooks::DestroyService) do |instance|
           expect(instance).to receive(:delete_web_hook_logs)
-- 
GitLab


From 91cfdff8a3f3ff419f5754e05f8180b472b8bea2 Mon Sep 17 00:00:00 2001
From: Alex Kalderimis <akalderimis@gitlab.com>
Date: Thu, 19 May 2022 22:00:55 -0400
Subject: [PATCH 05/10] Split log destroyer out from hook destroyer

This makes web hook log destruction its own service.

We also enforce the `delete_web_hook` policy in the hook
destroy service. Log deletion (async clean up after the
hook itself is gone) is not subject to permissions, since
it is just database tidying.

Batch deletion remains the responsibility of the `WebHookLog` model,
but the loop is moved to the service.

Ignores policy class violations of `Gitlab/NamespacedClass` - their
names are fixed.
---
 app/models/hooks/web_hook_log.rb              | 14 ++---
 app/services/web_hooks/destroy_service.rb     | 14 +----
 app/services/web_hooks/log_destroy_service.rb | 19 ++++++
 app/workers/web_hooks/destroy_worker.rb       |  4 +-
 app/workers/web_hooks/log_destroy_worker.rb   | 20 +++---
 spec/models/hooks/web_hook_log_spec.rb        | 58 ++++++++++++++---
 .../web_hooks/destroy_service_spec.rb         | 55 +---------------
 .../web_hooks/log_destroy_service_spec.rb     | 48 ++++++++++++++
 spec/workers/web_hooks/destroy_worker_spec.rb |  4 +-
 .../web_hooks/log_destroy_worker_spec.rb      | 63 +++++++++++--------
 10 files changed, 174 insertions(+), 125 deletions(-)
 create mode 100644 app/services/web_hooks/log_destroy_service.rb
 create mode 100644 spec/services/web_hooks/log_destroy_service_spec.rb

diff --git a/app/models/hooks/web_hook_log.rb b/app/models/hooks/web_hook_log.rb
index aaf57c2092a638..a95dd0473b69ed 100644
--- a/app/models/hooks/web_hook_log.rb
+++ b/app/models/hooks/web_hook_log.rb
@@ -7,8 +7,6 @@ class WebHookLog < ApplicationRecord
   include CreatedAtFilterable
   include PartitionedTable
 
-  BATCH_SIZE = 1000
-
   self.primary_key = :id
 
   partitioned_by :created_at, strategy: :monthly, retain_for: 3.months
@@ -28,15 +26,11 @@ def self.recent
       .order(created_at: :desc)
   end
 
-  def self.delete_batch_for(web_hook)
-    where(web_hook: web_hook).limit(BATCH_SIZE).delete_all
-  end
+  # Delete a batch of log records. Returns true if there may be more remaining.
+  def self.delete_batch_for(web_hook, batch_size:)
+    raise ArgumentError, 'batch_size is too small' if batch_size < 1
 
-  def self.delete_for(web_hook)
-    loop do
-      count = delete_batch_for(web_hook)
-      break if count < BATCH_SIZE
-    end
+    where(web_hook: web_hook).limit(batch_size).delete_all == batch_size
   end
 
   def success?
diff --git a/app/services/web_hooks/destroy_service.rb b/app/services/web_hooks/destroy_service.rb
index 2efbcb1d9e909e..ecb530f0d2a23b 100644
--- a/app/services/web_hooks/destroy_service.rb
+++ b/app/services/web_hooks/destroy_service.rb
@@ -2,10 +2,6 @@
 
 module WebHooks
   class DestroyService
-    include BaseServiceUtility
-
-    DestroyError = Class.new(StandardError)
-
     attr_accessor :current_user
 
     def initialize(current_user)
@@ -17,7 +13,7 @@ def execute(web_hook)
       hook_id = web_hook.id
 
       if web_hook.destroy
-        WebHooks::LogDestroyWorker.perform_async(current_user&.id, hook_id)
+        WebHooks::LogDestroyWorker.perform_async({ 'hook_id' => hook_id })
         Gitlab::AppLogger.info("User #{current_user&.id} scheduled a deletion of logs for hook ID #{hook_id}")
 
         ServiceResponse.success(payload: { async: false })
@@ -28,13 +24,5 @@ def execute(web_hook)
 
     # Backwards compatibility with WebHooks::DestroyWorker
     alias_method :sync_destroy, :execute
-
-    def delete_web_hook_logs(hook_id)
-      WebHookLog.delete_for(hook_id)
-
-      ServiceResponse.success
-    rescue StandardError => ex
-      ServiceResponse.error(message: ex.message)
-    end
   end
 end
diff --git a/app/services/web_hooks/log_destroy_service.rb b/app/services/web_hooks/log_destroy_service.rb
new file mode 100644
index 00000000000000..fa14b818e04869
--- /dev/null
+++ b/app/services/web_hooks/log_destroy_service.rb
@@ -0,0 +1,19 @@
+# frozen_string_literal: true
+
+module WebHooks
+  class LogDestroyService
+    BATCH_SIZE = 1000
+
+    def initialize(web_hook_id)
+      @web_hook_id = web_hook_id
+    end
+
+    def execute
+      continue while WebHookLog.delete_batch_for(@web_hook_id, batch_size: BATCH_SIZE)
+
+      ServiceResponse.success
+    rescue StandardError => ex
+      ServiceResponse.error(message: ex.message)
+    end
+  end
+end
diff --git a/app/workers/web_hooks/destroy_worker.rb b/app/workers/web_hooks/destroy_worker.rb
index 822a1e770d7bcb..7cccb0b4b7b831 100644
--- a/app/workers/web_hooks/destroy_worker.rb
+++ b/app/workers/web_hooks/destroy_worker.rb
@@ -4,6 +4,8 @@ module WebHooks
   class DestroyWorker
     include ApplicationWorker
 
+    DestroyError = Class.new(StandardError)
+
     data_consistency :always
     sidekiq_options retry: 3
     feature_category :integrations
@@ -21,7 +23,7 @@ def perform(user_id, web_hook_id)
 
       return result if result[:status] == :success
 
-      e = ::WebHooks::DestroyService::DestroyError.new(result[:message])
+      e = DestroyError.new(result[:message])
       Gitlab::ErrorTracking.track_exception(e, web_hook_id: hook.id)
 
       raise e
diff --git a/app/workers/web_hooks/log_destroy_worker.rb b/app/workers/web_hooks/log_destroy_worker.rb
index f093df1964feff..557ca9fa4ccb0b 100644
--- a/app/workers/web_hooks/log_destroy_worker.rb
+++ b/app/workers/web_hooks/log_destroy_worker.rb
@@ -4,25 +4,21 @@ module WebHooks
   class LogDestroyWorker
     include ApplicationWorker
 
+    DestroyError = Class.new(StandardError)
+
     data_consistency :always
     feature_category :integrations
-    urgency :high
+    urgency :low
 
     idempotent!
 
-    def perform(user_id, hook_id)
-      user = User.find_by_id(user_id)
-
-      return unless user
-
-      result = ::WebHooks::DestroyService.new(user).delete_web_hook_logs(hook_id)
-
-      return result if result[:status] == :success
+    def perform(params = {})
+      hook_id = params['hook_id']
+      return unless hook_id
 
-      e = ::WebHooks::DestroyService::DestroyError.new(result[:message])
-      Gitlab::ErrorTracking.track_exception(e, web_hook_id: hook_id)
+      result = ::WebHooks::LogDestroyService.new(hook_id).execute
 
-      raise e
+      result.track_exception(as: DestroyError, web_hook_id: hook_id)
     end
   end
 end
diff --git a/spec/models/hooks/web_hook_log_spec.rb b/spec/models/hooks/web_hook_log_spec.rb
index 2e0c508eba1504..e1fea3318f64b5 100644
--- a/spec/models/hooks/web_hook_log_spec.rb
+++ b/spec/models/hooks/web_hook_log_spec.rb
@@ -48,17 +48,59 @@
     end
   end
 
-  describe '.delete_for' do
-    it 'deletes records in batches' do
-      stub_const("#{described_class}::BATCH_SIZE", 2)
+  describe '.delete_batch_for' do
+    let(:hook) { create(:project_hook) }
+
+    before do
+      create_list(:web_hook_log, 3, web_hook: hook)
+      create_list(:web_hook_log, 3)
+    end
+
+    subject { described_class.delete_batch_for(hook, batch_size: batch_size) }
+
+    shared_examples 'deletes batch of web hook logs' do
+      it { is_expected.to be(batch_size <= 3) }
+
+      it 'deletes min(batch_size, total) records' do
+        deleted = [batch_size, 3].min
 
-      hook = create(:project_hook)
-      create_list(:web_hook_log, 5, web_hook: hook)
+        expect { subject }.to change(described_class, :count).by(-deleted)
+      end
+    end
+
+    context 'when the batch size is less than one' do
+      let(:batch_size) { 0 }
+
+      it 'raises an argument error' do
+        expect { subject }.to raise_error(ArgumentError)
+      end
+    end
+
+    context 'when the batch size is smaller than the total' do
+      let(:batch_size) { 2 }
+
+      include_examples 'deletes batch of web hook logs'
+    end
+
+    context 'when the batch size is equal to the total' do
+      let(:batch_size) { 3 }
+
+      include_examples 'deletes batch of web hook logs'
+    end
+
+    context 'when the batch size is greater than the total' do
+      let(:batch_size) { 1000 }
+
+      include_examples 'deletes batch of web hook logs'
+    end
 
-      expect(described_class).to receive(:delete_batch_for).exactly(3).times.and_call_original
+    it 'does not loop forever' do
+      batches = 0
+      batches += 1 while described_class.delete_batch_for(hook, batch_size: 1)
 
-      expect { described_class.delete_for(hook.id) }
-        .to change(described_class, :count).by(-5)
+      expect(hook.web_hook_logs).to be_none
+      expect(described_class.count).to eq 3
+      expect(batches).to eq 3 # true three times, stops at first false
     end
   end
 
diff --git a/spec/services/web_hooks/destroy_service_spec.rb b/spec/services/web_hooks/destroy_service_spec.rb
index 72e533f3817a13..269e52e860ebec 100644
--- a/spec/services/web_hooks/destroy_service_spec.rb
+++ b/spec/services/web_hooks/destroy_service_spec.rb
@@ -22,7 +22,7 @@
       end
 
       it 'schedules the destruction of logs' do
-        expect(WebHooks::LogDestroyWorker).to receive(:perform_async).with(user.id, hook.id)
+        expect(WebHooks::LogDestroyWorker).to receive(:perform_async).with({ 'hook_id' => hook.id })
         expect(Gitlab::AppLogger).to receive(:info).with(match(/scheduled a deletion of logs/))
 
         subject.execute(hook)
@@ -41,58 +41,5 @@
         end
       end
     end
-
-    context 'with system hook' do
-      let!(:hook) { create(:system_hook, url: "http://example.com") }
-      let!(:log) { create_list(:web_hook_log, 3, web_hook: hook) }
-
-      it_behaves_like 'destroys hook and schedules log deletion'
-    end
-
-    context 'with project hook' do
-      let!(:hook) { create(:project_hook) }
-      let!(:log) { create_list(:web_hook_log, 3, web_hook: hook) }
-
-      it_behaves_like 'destroys hook and schedules log deletion'
-    end
-  end
-
-  describe '#delete_web_hook_logs' do
-    shared_examples 'deletes web hook logs for hook' do
-      before do
-        hook.destroy! # expected to be called _after_ hook destruction
-      end
-
-      it 'deletes the logs' do
-        expect { subject.delete_web_hook_logs(hook.id) }
-          .to change(WebHookLog, :count).from(3).to(0)
-      end
-
-      context 'when it encounters an error' do
-        before do
-          allow(WebHookLog).to receive(:delete_for).and_raise(StandardError.new('bang'))
-        end
-
-        it 'reports the error' do
-          expect(subject.delete_web_hook_logs(hook.id))
-            .to be_error
-            .and have_attributes(message: 'bang')
-        end
-      end
-    end
-
-    context 'with system hook' do
-      let!(:hook) { create(:system_hook, url: "http://example.com") }
-      let!(:log) { create_list(:web_hook_log, 3, web_hook: hook) }
-
-      it_behaves_like 'deletes web hook logs for hook'
-    end
-
-    context 'with project hook' do
-      let!(:hook) { create(:project_hook) }
-      let!(:log) { create_list(:web_hook_log, 3, web_hook: hook) }
-
-      it_behaves_like 'deletes web hook logs for hook'
-    end
   end
 end
diff --git a/spec/services/web_hooks/log_destroy_service_spec.rb b/spec/services/web_hooks/log_destroy_service_spec.rb
new file mode 100644
index 00000000000000..6283564b7120c0
--- /dev/null
+++ b/spec/services/web_hooks/log_destroy_service_spec.rb
@@ -0,0 +1,48 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe WebHooks::LogDestroyService do
+  let_it_be(:user) { create(:user) }
+
+  subject { described_class.new(hook.id) }
+
+  describe '#execute' do
+    shared_examples 'deletes web hook logs for hook' do
+      before do
+        hook.destroy! # expected to be called _after_ hook destruction
+      end
+
+      it 'deletes the logs' do
+        expect { subject.execute }
+          .to change(WebHookLog, :count).from(3).to(0)
+      end
+
+      context 'when it encounters an error' do
+        before do
+          allow(WebHookLog).to receive(:delete_batch_for).and_raise(StandardError.new('bang'))
+        end
+
+        it 'reports the error' do
+          expect(subject.execute)
+            .to be_error
+            .and have_attributes(message: 'bang')
+        end
+      end
+    end
+
+    context 'with system hook' do
+      let!(:hook) { create(:system_hook, url: "http://example.com") }
+      let!(:log) { create_list(:web_hook_log, 3, web_hook: hook) }
+
+      it_behaves_like 'deletes web hook logs for hook'
+    end
+
+    context 'with project hook' do
+      let!(:hook) { create(:project_hook) }
+      let!(:log) { create_list(:web_hook_log, 3, web_hook: hook) }
+
+      it_behaves_like 'deletes web hook logs for hook'
+    end
+  end
+end
diff --git a/spec/workers/web_hooks/destroy_worker_spec.rb b/spec/workers/web_hooks/destroy_worker_spec.rb
index 5f3ae702bb8a17..c34d83b6cf7ffa 100644
--- a/spec/workers/web_hooks/destroy_worker_spec.rb
+++ b/spec/workers/web_hooks/destroy_worker_spec.rb
@@ -37,9 +37,9 @@
         end
 
         expect(Gitlab::ErrorTracking).to receive(:track_exception)
-                                       .with(an_instance_of(::WebHooks::DestroyService::DestroyError), web_hook_id: hook.id)
+                                       .with(an_instance_of(described_class::DestroyError), web_hook_id: hook.id)
                                        .and_call_original
-        expect { subject.perform(user.id, hook.id) }.to raise_error(::WebHooks::DestroyService::DestroyError)
+        expect { subject.perform(user.id, hook.id) }.to raise_error(described_class::DestroyError)
       end
 
       context 'with unknown hook' do
diff --git a/spec/workers/web_hooks/log_destroy_worker_spec.rb b/spec/workers/web_hooks/log_destroy_worker_spec.rb
index 77c3c2901fb611..df4f4b6cb6d6ae 100644
--- a/spec/workers/web_hooks/log_destroy_worker_spec.rb
+++ b/spec/workers/web_hooks/log_destroy_worker_spec.rb
@@ -4,25 +4,20 @@
 
 RSpec.describe WebHooks::LogDestroyWorker do
   let_it_be(:project) { create(:project) }
-  let_it_be(:user) { create(:user) }
-
-  before_all do
-    project.add_maintainer(user)
-  end
 
   subject { described_class.new }
 
   describe "#perform" do
-    context 'with a Web hook' do
-      let!(:hook) { create(:project_hook, project: project) }
-      let!(:other_hook) { create(:project_hook, project: project) }
-      let!(:log) { create(:web_hook_log, web_hook: hook) }
-      let!(:other_log) { create(:web_hook_log, web_hook: other_hook) }
+    let!(:hook) { create(:project_hook, project: project) }
+    let!(:other_hook) { create(:project_hook, project: project) }
+    let!(:log) { create(:web_hook_log, web_hook: hook) }
+    let!(:other_log) { create(:web_hook_log, web_hook: other_hook) }
 
+    context 'with a Web hook' do
       it "deletes the relevant logs", :aggregate_failures do
         hook.destroy! # It does not depend on the presence of the hook
 
-        expect { subject.perform(user.id, hook.id) }
+        expect { subject.perform({ 'hook_id' => hook.id }) }
           .to change { WebHookLog.count }.by(-1)
 
         expect(WebHook.find(other_hook.id)).to be_present
@@ -37,36 +32,54 @@
       end
 
       it "raises and tracks an error if destroy failed" do
-        allow_next_instance_of(::WebHooks::DestroyService) do |instance|
-          expect(instance).to receive(:delete_web_hook_logs)
-            .with(hook.id)
+        allow_next_instance_of(::WebHooks::LogDestroyService) do |instance|
+          expect(instance).to receive(:execute)
             .and_return(ServiceResponse.error(message: "failed"))
         end
 
         expect(Gitlab::ErrorTracking)
           .to receive(:track_exception)
-          .with(an_instance_of(::WebHooks::DestroyService::DestroyError), web_hook_id: hook.id)
+          .with(an_instance_of(described_class::DestroyError), web_hook_id: hook.id)
           .and_call_original
 
-        expect { subject.perform(user.id, hook.id) }.to raise_error(::WebHooks::DestroyService::DestroyError)
+        expect { subject.perform({ 'hook_id' => hook.id }) }
+          .to raise_error(described_class::DestroyError)
       end
 
-      context 'with unknown hook' do
+      context 'with extra arguments' do
         it 'does not raise an error' do
-          expect { subject.perform(user.id, non_existing_record_id) }.not_to raise_error
+          expect { subject.perform({ 'hook_id' => hook.id, 'extra' => true }) }.not_to raise_error
 
           expect(WebHook.count).to eq(2)
-          expect(WebHookLog.count).to eq(2)
+          expect(WebHookLog.count).to eq(1)
         end
       end
+    end
 
-      context 'with unknown user' do
-        it 'does not raise an error' do
-          expect { subject.perform(non_existing_record_id, hook.id) }.not_to raise_error
+    context 'with no arguments' do
+      it 'does not raise an error' do
+        expect { subject.perform }.not_to raise_error
 
-          expect(WebHook.count).to eq(2)
-          expect(WebHookLog.count).to eq(2)
-        end
+        expect(WebHook.count).to eq(2)
+        expect(WebHookLog.count).to eq(2)
+      end
+    end
+
+    context 'with empty arguments' do
+      it 'does not raise an error' do
+        expect { subject.perform({}) }.not_to raise_error
+
+        expect(WebHook.count).to eq(2)
+        expect(WebHookLog.count).to eq(2)
+      end
+    end
+
+    context 'with unknown hook' do
+      it 'does not raise an error' do
+        expect { subject.perform({ 'hook_id' => non_existing_record_id }) }.not_to raise_error
+
+        expect(WebHook.count).to eq(2)
+        expect(WebHookLog.count).to eq(2)
       end
     end
   end
-- 
GitLab


From d4e918829c325424be6a08d2199e524d72773ccd Mon Sep 17 00:00:00 2001
From: Alex Kalderimis <akalderimis@gitlab.com>
Date: Sun, 22 May 2022 19:36:35 -0400
Subject: [PATCH 06/10] Split tracking and raising behavior

---
 app/services/service_response.rb       | 13 +++--
 spec/services/service_response_spec.rb | 75 ++++++++++++++++++++++----
 2 files changed, 75 insertions(+), 13 deletions(-)

diff --git a/app/services/service_response.rb b/app/services/service_response.rb
index 49274891ffe90f..126956e0094103 100644
--- a/app/services/service_response.rb
+++ b/app/services/service_response.rb
@@ -18,12 +18,19 @@ def initialize(status:, message: nil, payload: {}, http_status: nil)
     self.http_status = http_status
   end
 
-  def track_exception(as: StandardError, track_only: false, **extra_data)
+  def track_exception(as: StandardError, **extra_data)
     if error?
       e = as.new(message)
-      Gitlab::ErrorTracking.track_exception(e, **extra_data)
+      Gitlab::ErrorTracking.track_exception(e, payload.merge(extra_data))
+    end
+
+    self
+  end
 
-      raise e unless track_only
+  def track_and_raise_exception(as: StandardError, **extra_data)
+    if error?
+      e = as.new(message)
+      Gitlab::ErrorTracking.track_and_raise_exception(e, payload.merge(extra_data))
     end
 
     self
diff --git a/spec/services/service_response_spec.rb b/spec/services/service_response_spec.rb
index 223b6f6ae5a957..5823bea4423292 100644
--- a/spec/services/service_response_spec.rb
+++ b/spec/services/service_response_spec.rb
@@ -98,6 +98,53 @@
     end
   end
 
+  describe '#track_and_raise_exception' do
+    context 'when successful' do
+      let(:response) { described_class.success }
+
+      it 'returns self' do
+        expect(response.track_and_raise_exception).to be response
+      end
+    end
+
+    context 'when an error' do
+      let(:response) { described_class.error(message: 'bang') }
+
+      it 'tracks and raises' do
+        expect(::Gitlab::ErrorTracking).to receive(:track_and_raise_exception)
+          .with(StandardError.new('bang'), {})
+
+        response.track_and_raise_exception
+      end
+
+      it 'allows specification of error class' do
+        error = Class.new(StandardError)
+        expect(::Gitlab::ErrorTracking).to receive(:track_and_raise_exception)
+          .with(error.new('bang'), {})
+
+        response.track_and_raise_exception(as: error)
+      end
+
+      it 'allows extra data for tracking' do
+        expect(::Gitlab::ErrorTracking).to receive(:track_and_raise_exception)
+          .with(StandardError.new('bang'), { foo: 1, bar: 2 })
+
+        response.track_and_raise_exception(foo: 1, bar: 2)
+      end
+
+      context 'when the response has a payload' do
+        let(:response) { described_class.error(message: 'bang', payload: { id: 123 }) }
+
+        it 'incorporates the payload extra data for tracking' do
+          expect(::Gitlab::ErrorTracking).to receive(:track_and_raise_exception)
+            .with(StandardError.new('bang'), { id: 123, foo: 1, bar: 2 })
+
+          response.track_and_raise_exception(foo: 1, bar: 2)
+        end
+      end
+    end
+  end
+
   describe '#track_exception' do
     context 'when successful' do
       let(:response) { described_class.success }
@@ -110,29 +157,37 @@
     context 'when an error' do
       let(:response) { described_class.error(message: 'bang') }
 
-      it 'tracks and raises' do
-        expect(::Gitlab::ErrorTracking).to receive(:track_exception).with(StandardError.new('bang'))
+      it 'tracks' do
+        expect(::Gitlab::ErrorTracking).to receive(:track_exception)
+          .with(StandardError.new('bang'), {})
 
-        expect { response.track_exception }.to raise_error('bang')
+        expect(response.track_exception).to be response
       end
 
       it 'allows specification of error class' do
         error = Class.new(StandardError)
-        expect(::Gitlab::ErrorTracking).to receive(:track_exception).with(error.new('bang'))
+        expect(::Gitlab::ErrorTracking).to receive(:track_exception)
+          .with(error.new('bang'), {})
 
-        expect { response.track_exception(as: error) }.to raise_error(an_instance_of(error))
+        expect(response.track_exception(as: error)).to be response
       end
 
       it 'allows extra data for tracking' do
-        expect(::Gitlab::ErrorTracking).to receive(:track_exception).with(StandardError.new('bang'), foo: 1, bar: 2)
+        expect(::Gitlab::ErrorTracking).to receive(:track_exception)
+          .with(StandardError.new('bang'), { foo: 1, bar: 2 })
 
-        expect { response.track_exception(foo: 1, bar: 2) }.to raise_error(StandardError)
+        expect(response.track_exception(foo: 1, bar: 2)).to be response
       end
 
-      it 'allows the error to be tracked only' do
-        expect(::Gitlab::ErrorTracking).to receive(:track_exception).with(StandardError.new('bang'))
+      context 'when the response has a payload' do
+        let(:response) { described_class.error(message: 'bang', payload: { id: 123 }) }
+
+        it 'incorporates the payload extra data for tracking' do
+          expect(::Gitlab::ErrorTracking).to receive(:track_exception)
+            .with(StandardError.new('bang'), { id: 123, foo: 1, bar: 2 })
 
-        expect(response.track_exception(track_only: true)).to be response
+          expect(response.track_exception(foo: 1, bar: 2)).to be response
+        end
       end
     end
   end
-- 
GitLab


From 58f2cdd80dec12a594760e2def156200aff3d6cd Mon Sep 17 00:00:00 2001
From: Alex Kalderimis <akalderimis@gitlab.com>
Date: Sun, 22 May 2022 20:02:42 -0400
Subject: [PATCH 07/10] Reviewer feedback and small fixes

Fixes rubocop violations and minor other matters.
---
 app/services/web_hooks/destroy_service.rb     |  3 +-
 app/services/web_hooks/log_destroy_service.rb |  2 +-
 app/workers/web_hooks/destroy_worker.rb       |  7 +--
 app/workers/web_hooks/log_destroy_worker.rb   |  2 +-
 .../web_hooks/destroy_service_spec.rb         | 50 +++++++++++--------
 .../web_hooks/log_destroy_service_spec.rb     |  7 +--
 spec/workers/web_hooks/destroy_worker_spec.rb | 18 ++++---
 .../web_hooks/log_destroy_worker_spec.rb      | 12 ++---
 8 files changed, 51 insertions(+), 50 deletions(-)

diff --git a/app/services/web_hooks/destroy_service.rb b/app/services/web_hooks/destroy_service.rb
index ecb530f0d2a23b..db753b256ea3e2 100644
--- a/app/services/web_hooks/destroy_service.rb
+++ b/app/services/web_hooks/destroy_service.rb
@@ -18,7 +18,8 @@ def execute(web_hook)
 
         ServiceResponse.success(payload: { async: false })
       else
-        ServiceResponse.error(message: "Unable to destroy #{web_hook.model_name.human}")
+        ServiceResponse.error(message: "Unable to destroy #{web_hook.model_name.human}",
+                              payload: { web_hook_id: hook_id })
       end
     end
 
diff --git a/app/services/web_hooks/log_destroy_service.rb b/app/services/web_hooks/log_destroy_service.rb
index fa14b818e04869..43eb6fc14a217c 100644
--- a/app/services/web_hooks/log_destroy_service.rb
+++ b/app/services/web_hooks/log_destroy_service.rb
@@ -13,7 +13,7 @@ def execute
 
       ServiceResponse.success
     rescue StandardError => ex
-      ServiceResponse.error(message: ex.message)
+      ServiceResponse.error(message: ex.message, payload: { web_hook_id: @web_hook_id })
     end
   end
 end
diff --git a/app/workers/web_hooks/destroy_worker.rb b/app/workers/web_hooks/destroy_worker.rb
index 7cccb0b4b7b831..5e99c86256c135 100644
--- a/app/workers/web_hooks/destroy_worker.rb
+++ b/app/workers/web_hooks/destroy_worker.rb
@@ -21,12 +21,7 @@ def perform(user_id, web_hook_id)
 
       result = ::WebHooks::DestroyService.new(user).sync_destroy(hook)
 
-      return result if result[:status] == :success
-
-      e = DestroyError.new(result[:message])
-      Gitlab::ErrorTracking.track_exception(e, web_hook_id: hook.id)
-
-      raise e
+      result.track_and_raise_exception(as: DestroyError)
     end
   end
 end
diff --git a/app/workers/web_hooks/log_destroy_worker.rb b/app/workers/web_hooks/log_destroy_worker.rb
index 557ca9fa4ccb0b..839717e4193b88 100644
--- a/app/workers/web_hooks/log_destroy_worker.rb
+++ b/app/workers/web_hooks/log_destroy_worker.rb
@@ -18,7 +18,7 @@ def perform(params = {})
 
       result = ::WebHooks::LogDestroyService.new(hook_id).execute
 
-      result.track_exception(as: DestroyError, web_hook_id: hook_id)
+      result.track_and_raise_exception(as: DestroyError)
     end
   end
 end
diff --git a/spec/services/web_hooks/destroy_service_spec.rb b/spec/services/web_hooks/destroy_service_spec.rb
index 269e52e860ebec..1dfedb352bf489 100644
--- a/spec/services/web_hooks/destroy_service_spec.rb
+++ b/spec/services/web_hooks/destroy_service_spec.rb
@@ -8,36 +8,42 @@
   subject { described_class.new(user) }
 
   describe '#execute' do
-    shared_examples 'destroys hook and schedules log deletion' do
-      it 'is successful' do
-        expect(subject.execute(hook)).to be_success
-      end
+    %i[system_hook project_hook].each do |factory|
+      context "deleting a #{factory}" do
+        let!(:hook) { create(factory) } # rubocop: disable Rails/SaveBang (false-positive!)
+        let!(:log) { create_list(:web_hook_log, 3, web_hook: hook) }
 
-      it 'destroys the hook' do
-        expect { subject.execute(hook) }.to change(WebHook, :count).from(1).to(0)
-      end
+        it 'is successful' do
+          expect(subject.execute(hook)).to be_success
+        end
 
-      it 'does not destroy logs' do
-        expect { subject.execute(hook) }.not_to change(WebHookLog, :count)
-      end
+        it 'destroys the hook' do
+          expect { subject.execute(hook) }.to change(WebHook, :count).from(1).to(0)
+        end
 
-      it 'schedules the destruction of logs' do
-        expect(WebHooks::LogDestroyWorker).to receive(:perform_async).with({ 'hook_id' => hook.id })
-        expect(Gitlab::AppLogger).to receive(:info).with(match(/scheduled a deletion of logs/))
+        it 'does not destroy logs' do
+          expect { subject.execute(hook) }.not_to change(WebHookLog, :count)
+        end
 
-        subject.execute(hook)
-      end
+        it 'schedules the destruction of logs' do
+          expect(WebHooks::LogDestroyWorker).to receive(:perform_async).with({ 'hook_id' => hook.id })
+          expect(Gitlab::AppLogger).to receive(:info).with(match(/scheduled a deletion of logs/))
 
-      context 'when the hook fails to destroy' do
-        before do
-          allow(hook).to receive(:destroy).and_return(false)
+          subject.execute(hook)
         end
 
-        it 'is not a success' do
-          r = subject.execute(hook)
+        context 'when the hook fails to destroy' do
+          before do
+            allow(hook).to receive(:destroy).and_return(false)
+          end
+
+          it 'is not a success' do
+            r = subject.execute(hook)
 
-          expect(r).to be_error
-          expect(r[:message]).to match %r{Unable to destroy}
+            expect(r).to be_error
+            expect(r[:message]).to match %r{Unable to destroy}
+            expect(r.payload).to eq(web_hook_id: hook.id)
+          end
         end
       end
     end
diff --git a/spec/services/web_hooks/log_destroy_service_spec.rb b/spec/services/web_hooks/log_destroy_service_spec.rb
index 6283564b7120c0..8e1d6008bfd8f7 100644
--- a/spec/services/web_hooks/log_destroy_service_spec.rb
+++ b/spec/services/web_hooks/log_destroy_service_spec.rb
@@ -3,13 +3,12 @@
 require 'spec_helper'
 
 RSpec.describe WebHooks::LogDestroyService do
-  let_it_be(:user) { create(:user) }
-
   subject { described_class.new(hook.id) }
 
   describe '#execute' do
     shared_examples 'deletes web hook logs for hook' do
       before do
+        create_list(:web_hook_log, 3, web_hook: hook)
         hook.destroy! # expected to be called _after_ hook destruction
       end
 
@@ -26,21 +25,19 @@
         it 'reports the error' do
           expect(subject.execute)
             .to be_error
-            .and have_attributes(message: 'bang')
+            .and have_attributes(message: 'bang', payload: { web_hook_id: hook.id })
         end
       end
     end
 
     context 'with system hook' do
       let!(:hook) { create(:system_hook, url: "http://example.com") }
-      let!(:log) { create_list(:web_hook_log, 3, web_hook: hook) }
 
       it_behaves_like 'deletes web hook logs for hook'
     end
 
     context 'with project hook' do
       let!(:hook) { create(:project_hook) }
-      let!(:log) { create_list(:web_hook_log, 3, web_hook: hook) }
 
       it_behaves_like 'deletes web hook logs for hook'
     end
diff --git a/spec/workers/web_hooks/destroy_worker_spec.rb b/spec/workers/web_hooks/destroy_worker_spec.rb
index c34d83b6cf7ffa..2460e9f8550820 100644
--- a/spec/workers/web_hooks/destroy_worker_spec.rb
+++ b/spec/workers/web_hooks/destroy_worker_spec.rb
@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 RSpec.describe WebHooks::DestroyWorker do
+  include AfterNextHelpers
+
   let_it_be(:project) { create(:project) }
   let_it_be(:user) { create(:user) }
 
@@ -30,15 +32,15 @@
       end
 
       it "raises and tracks an error if destroy failed" do
-        allow_next_instance_of(::WebHooks::DestroyService) do |instance|
-          expect(instance).to receive(:sync_destroy)
-            .with(anything)
-            .and_return(ServiceResponse.error(message: "failed"))
-        end
+        expect_next(::WebHooks::DestroyService)
+          .to receive(:sync_destroy).with(anything)
+          .and_return(ServiceResponse.error(message: "failed", payload: { x: 1 }))
+
+        expect(Gitlab::ErrorTracking)
+          .to receive(:track_and_raise_exception)
+          .with(an_instance_of(described_class::DestroyError), { x: 1 })
+          .and_call_original
 
-        expect(Gitlab::ErrorTracking).to receive(:track_exception)
-                                       .with(an_instance_of(described_class::DestroyError), web_hook_id: hook.id)
-                                       .and_call_original
         expect { subject.perform(user.id, hook.id) }.to raise_error(described_class::DestroyError)
       end
 
diff --git a/spec/workers/web_hooks/log_destroy_worker_spec.rb b/spec/workers/web_hooks/log_destroy_worker_spec.rb
index df4f4b6cb6d6ae..372dcabcbd61d3 100644
--- a/spec/workers/web_hooks/log_destroy_worker_spec.rb
+++ b/spec/workers/web_hooks/log_destroy_worker_spec.rb
@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 RSpec.describe WebHooks::LogDestroyWorker do
+  include AfterNextHelpers
+
   let_it_be(:project) { create(:project) }
 
   subject { described_class.new }
@@ -32,14 +34,12 @@
       end
 
       it "raises and tracks an error if destroy failed" do
-        allow_next_instance_of(::WebHooks::LogDestroyService) do |instance|
-          expect(instance).to receive(:execute)
-            .and_return(ServiceResponse.error(message: "failed"))
-        end
+        expect_next(::WebHooks::LogDestroyService)
+          .to receive(:execute).and_return(ServiceResponse.error(message: "failed", payload: { x: 1 }))
 
         expect(Gitlab::ErrorTracking)
-          .to receive(:track_exception)
-          .with(an_instance_of(described_class::DestroyError), web_hook_id: hook.id)
+          .to receive(:track_and_raise_exception)
+          .with(an_instance_of(described_class::DestroyError), { x: 1 })
           .and_call_original
 
         expect { subject.perform({ 'hook_id' => hook.id }) }
-- 
GitLab


From 1791b8ac1a413d98719ef626ec0dfabd706f5175 Mon Sep 17 00:00:00 2001
From: Alex Kalderimis <akalderimis@gitlab.com>
Date: Mon, 23 May 2022 17:47:06 -0400
Subject: [PATCH 08/10] Do not send response payload to error tracking by
 default

---
 app/services/service_response.rb              |  4 ++--
 app/services/web_hooks/destroy_service.rb     |  3 +--
 app/services/web_hooks/log_destroy_service.rb |  2 +-
 app/workers/web_hooks/destroy_worker.rb       |  2 +-
 app/workers/web_hooks/log_destroy_worker.rb   |  2 +-
 spec/services/service_response_spec.rb        | 22 -------------------
 .../web_hooks/destroy_service_spec.rb         |  1 -
 .../web_hooks/log_destroy_service_spec.rb     |  2 +-
 spec/workers/web_hooks/destroy_worker_spec.rb |  4 ++--
 .../web_hooks/log_destroy_worker_spec.rb      |  4 ++--
 10 files changed, 11 insertions(+), 35 deletions(-)

diff --git a/app/services/service_response.rb b/app/services/service_response.rb
index 126956e0094103..c7ab75a44266c0 100644
--- a/app/services/service_response.rb
+++ b/app/services/service_response.rb
@@ -21,7 +21,7 @@ def initialize(status:, message: nil, payload: {}, http_status: nil)
   def track_exception(as: StandardError, **extra_data)
     if error?
       e = as.new(message)
-      Gitlab::ErrorTracking.track_exception(e, payload.merge(extra_data))
+      Gitlab::ErrorTracking.track_exception(e, extra_data)
     end
 
     self
@@ -30,7 +30,7 @@ def track_exception(as: StandardError, **extra_data)
   def track_and_raise_exception(as: StandardError, **extra_data)
     if error?
       e = as.new(message)
-      Gitlab::ErrorTracking.track_and_raise_exception(e, payload.merge(extra_data))
+      Gitlab::ErrorTracking.track_and_raise_exception(e, extra_data)
     end
 
     self
diff --git a/app/services/web_hooks/destroy_service.rb b/app/services/web_hooks/destroy_service.rb
index db753b256ea3e2..ecb530f0d2a23b 100644
--- a/app/services/web_hooks/destroy_service.rb
+++ b/app/services/web_hooks/destroy_service.rb
@@ -18,8 +18,7 @@ def execute(web_hook)
 
         ServiceResponse.success(payload: { async: false })
       else
-        ServiceResponse.error(message: "Unable to destroy #{web_hook.model_name.human}",
-                              payload: { web_hook_id: hook_id })
+        ServiceResponse.error(message: "Unable to destroy #{web_hook.model_name.human}")
       end
     end
 
diff --git a/app/services/web_hooks/log_destroy_service.rb b/app/services/web_hooks/log_destroy_service.rb
index 43eb6fc14a217c..fa14b818e04869 100644
--- a/app/services/web_hooks/log_destroy_service.rb
+++ b/app/services/web_hooks/log_destroy_service.rb
@@ -13,7 +13,7 @@ def execute
 
       ServiceResponse.success
     rescue StandardError => ex
-      ServiceResponse.error(message: ex.message, payload: { web_hook_id: @web_hook_id })
+      ServiceResponse.error(message: ex.message)
     end
   end
 end
diff --git a/app/workers/web_hooks/destroy_worker.rb b/app/workers/web_hooks/destroy_worker.rb
index 5e99c86256c135..8f9b194f88acda 100644
--- a/app/workers/web_hooks/destroy_worker.rb
+++ b/app/workers/web_hooks/destroy_worker.rb
@@ -21,7 +21,7 @@ def perform(user_id, web_hook_id)
 
       result = ::WebHooks::DestroyService.new(user).sync_destroy(hook)
 
-      result.track_and_raise_exception(as: DestroyError)
+      result.track_and_raise_exception(as: DestroyError, web_hook_id: hook.id)
     end
   end
 end
diff --git a/app/workers/web_hooks/log_destroy_worker.rb b/app/workers/web_hooks/log_destroy_worker.rb
index 839717e4193b88..9ea5c70e416639 100644
--- a/app/workers/web_hooks/log_destroy_worker.rb
+++ b/app/workers/web_hooks/log_destroy_worker.rb
@@ -18,7 +18,7 @@ def perform(params = {})
 
       result = ::WebHooks::LogDestroyService.new(hook_id).execute
 
-      result.track_and_raise_exception(as: DestroyError)
+      result.track_and_raise_exception(as: DestroyError, web_hook_id: hook_id)
     end
   end
 end
diff --git a/spec/services/service_response_spec.rb b/spec/services/service_response_spec.rb
index 5823bea4423292..3ede90fbc44cb0 100644
--- a/spec/services/service_response_spec.rb
+++ b/spec/services/service_response_spec.rb
@@ -131,17 +131,6 @@
 
         response.track_and_raise_exception(foo: 1, bar: 2)
       end
-
-      context 'when the response has a payload' do
-        let(:response) { described_class.error(message: 'bang', payload: { id: 123 }) }
-
-        it 'incorporates the payload extra data for tracking' do
-          expect(::Gitlab::ErrorTracking).to receive(:track_and_raise_exception)
-            .with(StandardError.new('bang'), { id: 123, foo: 1, bar: 2 })
-
-          response.track_and_raise_exception(foo: 1, bar: 2)
-        end
-      end
     end
   end
 
@@ -178,17 +167,6 @@
 
         expect(response.track_exception(foo: 1, bar: 2)).to be response
       end
-
-      context 'when the response has a payload' do
-        let(:response) { described_class.error(message: 'bang', payload: { id: 123 }) }
-
-        it 'incorporates the payload extra data for tracking' do
-          expect(::Gitlab::ErrorTracking).to receive(:track_exception)
-            .with(StandardError.new('bang'), { id: 123, foo: 1, bar: 2 })
-
-          expect(response.track_exception(foo: 1, bar: 2)).to be response
-        end
-      end
     end
   end
 end
diff --git a/spec/services/web_hooks/destroy_service_spec.rb b/spec/services/web_hooks/destroy_service_spec.rb
index 1dfedb352bf489..bdfb804648fa28 100644
--- a/spec/services/web_hooks/destroy_service_spec.rb
+++ b/spec/services/web_hooks/destroy_service_spec.rb
@@ -42,7 +42,6 @@
 
             expect(r).to be_error
             expect(r[:message]).to match %r{Unable to destroy}
-            expect(r.payload).to eq(web_hook_id: hook.id)
           end
         end
       end
diff --git a/spec/services/web_hooks/log_destroy_service_spec.rb b/spec/services/web_hooks/log_destroy_service_spec.rb
index 8e1d6008bfd8f7..8a575b94f4c5de 100644
--- a/spec/services/web_hooks/log_destroy_service_spec.rb
+++ b/spec/services/web_hooks/log_destroy_service_spec.rb
@@ -25,7 +25,7 @@
         it 'reports the error' do
           expect(subject.execute)
             .to be_error
-            .and have_attributes(message: 'bang', payload: { web_hook_id: hook.id })
+            .and have_attributes(message: 'bang')
         end
       end
     end
diff --git a/spec/workers/web_hooks/destroy_worker_spec.rb b/spec/workers/web_hooks/destroy_worker_spec.rb
index 2460e9f8550820..8e75610a031bfe 100644
--- a/spec/workers/web_hooks/destroy_worker_spec.rb
+++ b/spec/workers/web_hooks/destroy_worker_spec.rb
@@ -34,11 +34,11 @@
       it "raises and tracks an error if destroy failed" do
         expect_next(::WebHooks::DestroyService)
           .to receive(:sync_destroy).with(anything)
-          .and_return(ServiceResponse.error(message: "failed", payload: { x: 1 }))
+          .and_return(ServiceResponse.error(message: "failed"))
 
         expect(Gitlab::ErrorTracking)
           .to receive(:track_and_raise_exception)
-          .with(an_instance_of(described_class::DestroyError), { x: 1 })
+          .with(an_instance_of(described_class::DestroyError), { web_hook_id: hook.id })
           .and_call_original
 
         expect { subject.perform(user.id, hook.id) }.to raise_error(described_class::DestroyError)
diff --git a/spec/workers/web_hooks/log_destroy_worker_spec.rb b/spec/workers/web_hooks/log_destroy_worker_spec.rb
index 372dcabcbd61d3..0c107c05360c0f 100644
--- a/spec/workers/web_hooks/log_destroy_worker_spec.rb
+++ b/spec/workers/web_hooks/log_destroy_worker_spec.rb
@@ -35,11 +35,11 @@
 
       it "raises and tracks an error if destroy failed" do
         expect_next(::WebHooks::LogDestroyService)
-          .to receive(:execute).and_return(ServiceResponse.error(message: "failed", payload: { x: 1 }))
+          .to receive(:execute).and_return(ServiceResponse.error(message: "failed"))
 
         expect(Gitlab::ErrorTracking)
           .to receive(:track_and_raise_exception)
-          .with(an_instance_of(described_class::DestroyError), { x: 1 })
+          .with(an_instance_of(described_class::DestroyError), { web_hook_id: hook.id })
           .and_call_original
 
         expect { subject.perform({ 'hook_id' => hook.id }) }
-- 
GitLab


From 6c979b74cf09c6e4ef55e0b0f2f4da89b9a1ac12 Mon Sep 17 00:00:00 2001
From: Alex Kalderimis <akalderimis@gitlab.com>
Date: Mon, 23 May 2022 18:06:32 -0400
Subject: [PATCH 09/10] Apply reviewer feedback

---
 spec/services/web_hooks/destroy_service_spec.rb     | 2 ++
 spec/services/web_hooks/log_destroy_service_spec.rb | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/spec/services/web_hooks/destroy_service_spec.rb b/spec/services/web_hooks/destroy_service_spec.rb
index bdfb804648fa28..4d9bb18e5402a0 100644
--- a/spec/services/web_hooks/destroy_service_spec.rb
+++ b/spec/services/web_hooks/destroy_service_spec.rb
@@ -38,6 +38,8 @@
           end
 
           it 'is not a success' do
+            expect(WebHooks::LogDestroyWorker).not_to receive(:perform_async)
+
             r = subject.execute(hook)
 
             expect(r).to be_error
diff --git a/spec/services/web_hooks/log_destroy_service_spec.rb b/spec/services/web_hooks/log_destroy_service_spec.rb
index 8a575b94f4c5de..1ae231ac262737 100644
--- a/spec/services/web_hooks/log_destroy_service_spec.rb
+++ b/spec/services/web_hooks/log_destroy_service_spec.rb
@@ -9,7 +9,7 @@
     shared_examples 'deletes web hook logs for hook' do
       before do
         create_list(:web_hook_log, 3, web_hook: hook)
-        hook.destroy! # expected to be called _after_ hook destruction
+        hook.destroy! # The LogDestroyService is expected to be called _after_ hook destruction
       end
 
       it 'deletes the logs' do
-- 
GitLab


From 2b837d75c8b816ebc63ec158606c532fd94a862b Mon Sep 17 00:00:00 2001
From: Alex Kalderimis <akalderimis@gitlab.com>
Date: Tue, 31 May 2022 14:29:14 -0400
Subject: [PATCH 10/10] Fix deletion when multiple batches are needed

---
 app/services/web_hooks/log_destroy_service.rb   |  2 +-
 .../web_hooks/log_destroy_service_spec.rb       | 17 ++++++++++++++---
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/app/services/web_hooks/log_destroy_service.rb b/app/services/web_hooks/log_destroy_service.rb
index fa14b818e04869..8a5d214a95e567 100644
--- a/app/services/web_hooks/log_destroy_service.rb
+++ b/app/services/web_hooks/log_destroy_service.rb
@@ -9,7 +9,7 @@ def initialize(web_hook_id)
     end
 
     def execute
-      continue while WebHookLog.delete_batch_for(@web_hook_id, batch_size: BATCH_SIZE)
+      next while WebHookLog.delete_batch_for(@web_hook_id, batch_size: BATCH_SIZE)
 
       ServiceResponse.success
     rescue StandardError => ex
diff --git a/spec/services/web_hooks/log_destroy_service_spec.rb b/spec/services/web_hooks/log_destroy_service_spec.rb
index 1ae231ac262737..7634726e5a4e97 100644
--- a/spec/services/web_hooks/log_destroy_service_spec.rb
+++ b/spec/services/web_hooks/log_destroy_service_spec.rb
@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 RSpec.describe WebHooks::LogDestroyService do
-  subject { described_class.new(hook.id) }
+  subject(:service) { described_class.new(hook.id) }
 
   describe '#execute' do
     shared_examples 'deletes web hook logs for hook' do
@@ -13,17 +13,28 @@
       end
 
       it 'deletes the logs' do
-        expect { subject.execute }
+        expect { service.execute }
           .to change(WebHookLog, :count).from(3).to(0)
       end
 
+      context 'when the data-set exceeds the batch size' do
+        before do
+          stub_const("#{described_class}::BATCH_SIZE", 2)
+        end
+
+        it 'deletes the logs' do
+          expect { service.execute }
+            .to change(WebHookLog, :count).from(3).to(0)
+        end
+      end
+
       context 'when it encounters an error' do
         before do
           allow(WebHookLog).to receive(:delete_batch_for).and_raise(StandardError.new('bang'))
         end
 
         it 'reports the error' do
-          expect(subject.execute)
+          expect(service.execute)
             .to be_error
             .and have_attributes(message: 'bang')
         end
-- 
GitLab