Merge branch 'sh-destroy-hook-logs-in-batches' into 'master'

Fix Web hook deletion not working when many hook logs are present

See merge request gitlab-org/gitlab!43464

app/controllers/admin/hooks_controller.rb

@@ -36,7 +36,7 @@ class Admin::HooksController < Admin::ApplicationController
   end
 
   def destroy
-    hook.destroy
+    destroy_hook(hook)
 
     redirect_to admin_hooks_path, status: :found
   end

app/controllers/concerns/hooks_execution.rb

@@ -5,6 +5,21 @@ module HooksExecution
 
   private
 
+  def destroy_hook(hook)
+    result = WebHooks::DestroyService.new(current_user).execute(hook)
+
+    if result[:status] == :success
+      flash[:notice] =
+        if result[:async]
+          _("%{hook_type} was scheduled for deletion") % { hook_type: hook.model_name.human }
+        else
+          _("%{hook_type} was deleted") % { hook_type: hook.model_name.human }
+        end
+    else
+      flash[:alert] = result[:message]
+    end
+  end
+
   def set_hook_execution_notice(result)
     http_status = result[:http_status]
     message = result[:message]

app/controllers/projects/hooks_controller.rb

@@ -50,7 +50,7 @@ class Projects::HooksController < Projects::ApplicationController
   end
 
   def destroy
-    hook.destroy
+    destroy_hook(hook)
 
     redirect_to action: :index, status: :found
   end

app/services/web_hooks/destroy_service.rb

+# frozen_string_literal: true
+
+module WebHooks
+  class DestroyService
+    include BaseServiceUtility
+
+    BATCH_SIZE = 1000
+    LOG_COUNT_THRESHOLD = 10000
+
+    DestroyError = Class.new(StandardError)
+
+    attr_accessor :current_user, :web_hook
+
+    def initialize(current_user)
+      @current_user = current_user
+    end
+
+    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
+
+      delete_web_hook_logs
+      result = web_hook.destroy
+
+      if result
+        success({ async: false })
+      else
+        error("Unable to destroy #{web_hook.model_name.human}")
+      end
+    end
+
+    private
+
+    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
+
+    # 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
+    end
+    # rubocop: enable CodeReuse/ActiveRecord
+  end
+end

app/workers/all_queues.yml

@@ -1940,6 +1940,14 @@
   :weight: 1
   :idempotent:
   :tags: []
+- :name: web_hooks_destroy
+  :feature_category: :integrations
+  :has_external_dependencies:
+  :urgency: :low
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent: true
+  :tags: []
 - :name: x509_certificate_revoke
   :feature_category: :source_code_management
   :has_external_dependencies:

app/workers/web_hooks/destroy_worker.rb

+# frozen_string_literal: true
+
+module WebHooks
+  class DestroyWorker
+    include ApplicationWorker
+
+    feature_category :integrations
+    urgency :low
+    idempotent!
+
+    def perform(user_id, web_hook_id)
+      user = User.find_by_id(user_id)
+      hook = WebHook.find_by_id(web_hook_id)
+
+      return unless user && hook
+
+      result = ::WebHooks::DestroyService.new(user).sync_destroy(hook)
+
+      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

changelogs/unreleased/sh-destroy-hook-logs-in-batches.yml

+---
+title: Fix Web hook deletion not working when many hook logs are present
+merge_request: 43464
+author:
+type: fixed

config/application.rb

@@ -128,12 +128,12 @@ module Gitlab
       /^description$/,
       /^note$/,
       /^text$/,
-      /^title$/
+      /^title$/,
+      /^hook$/
     ]
     config.filter_parameters += %i(
       certificate
       encrypted_key
-      hook
       import_url
       elasticsearch_url
       otp_attempt

config/sidekiq_queues.yml

@@ -310,5 +310,7 @@
   - 1
 - - web_hook
   - 1
+- - web_hooks_destroy
+  - 1
 - - x509_certificate_revoke
   - 1

ee/app/controllers/groups/hooks_controller.rb

@@ -60,7 +60,7 @@ class Groups::HooksController < Groups::ApplicationController
   end
 
   def destroy
-    @hook.destroy
+    destroy_hook(@hook)
 
     redirect_to group_hooks_path(@group), status: :found
   end

ee/lib/api/group_hooks.rb

@@ -101,7 +101,9 @@ module API
       delete ":id/hooks/:hook_id" do
         hook = user_group.hooks.find(params.delete(:hook_id))
 
-        destroy_conditionally!(hook)
+        destroy_conditionally!(hook) do
+          WebHooks::DestroyService.new(current_user).execute(hook)
+        end
       end
     end
   end

ee/spec/controllers/groups/hooks_controller_spec.rb

@@ -178,6 +178,14 @@ RSpec.describe Groups::HooksController do
     end
   end
 
+  describe 'DELETE #destroy' do
+    let(:hook) { create(:group_hook, group: group) }
+    let!(:log) { create(:web_hook_log, web_hook: hook) }
+    let(:params) { { group_id: group.to_param, id: hook } }
+
+    it_behaves_like 'Web hook destroyer'
+  end
+
   context 'with group_webhooks disabled' do
     before do
       stub_licensed_features(group_webhooks: false)

lib/api/project_hooks.rb

@@ -104,7 +104,9 @@ module API
       delete ":id/hooks/:hook_id" do
         hook = user_project.hooks.find(params.delete(:hook_id))
 
-        destroy_conditionally!(hook)
+        destroy_conditionally!(hook) do
+          WebHooks::DestroyService.new(current_user).execute(hook)
+        end
       end
     end
   end

lib/api/system_hooks.rb

@@ -70,7 +70,9 @@ module API
         hook = SystemHook.find_by(id: params[:id])
         not_found!('System hook') unless hook
 
-        destroy_conditionally!(hook)
+        destroy_conditionally!(hook) do
+          WebHooks::DestroyService.new(current_user).execute(hook)
+        end
       end
       # rubocop: enable CodeReuse/ActiveRecord
     end

lib/tasks/gitlab/web_hook.rake

@@ -37,7 +37,10 @@ namespace :gitlab do
       web_hooks.find_each do |hook|
         next unless hook.url == web_hook_url
 
-        hook.destroy!
+        result = WebHooks::DestroyService.new(nil).sync_destroy(hook)
+
+        raise "Unable to destroy Web hook" unless result[:status] == :success
+
         count += 1
       end
 

locale/gitlab.pot

@@ -498,6 +498,12 @@ msgstr ""
 msgid "%{group_name}&%{epic_iid} · opened %{epic_created} by %{author}"
 msgstr ""
 
+msgid "%{hook_type} was deleted"
+msgstr ""
+
+msgid "%{hook_type} was scheduled for deletion"
+msgstr ""
+
 msgid "%{host} sign-in from new location"
 msgstr ""
 

spec/controllers/admin/hooks_controller_spec.rb

@@ -29,4 +29,12 @@ RSpec.describe Admin::HooksController do
       expect(SystemHook.first).to have_attributes(hook_params)
     end
   end
+
+  describe 'DELETE #destroy' do
+    let!(:hook) { create(:system_hook) }
+    let!(:log) { create(:web_hook_log, web_hook: hook) }
+    let(:params) { { id: hook } }
+
+    it_behaves_like 'Web hook destroyer'
+  end
 end

spec/controllers/projects/hooks_controller_spec.rb

@@ -48,6 +48,14 @@ RSpec.describe Projects::HooksController do
     end
   end
 
+  describe 'DELETE #destroy' do
+    let!(:hook) { create(:project_hook, project: project) }
+    let!(:log) { create(:web_hook_log, web_hook: hook) }
+    let(:params) { { namespace_id: project.namespace, project_id: project, id: hook } }
+
+    it_behaves_like 'Web hook destroyer'
+  end
+
   describe '#test' do
     let(:hook) { create(:project_hook, project: project) }
 

spec/services/web_hooks/destroy_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe WebHooks::DestroyService do
+  let_it_be(:user) { create(:user) }
+
+  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
+
+      expect do
+        status = subject.execute(hook)
+        expect(status[:async]).to be false
+      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)
+
+      result = subject.sync_destroy(hook)
+
+      expect(result[:status]).to eq(:error)
+      expect(result[:message]).to eq("Unable to destroy #{hook.model_name.human}")
+    end
+
+    it 'schedules an async delete' do
+      stub_const('WebHooks::DestroyService::LOG_COUNT_THRESHOLD', 1)
+
+      expect(WebHooks::DestroyWorker).to receive(:perform_async).with(user.id, hook.id).and_call_original
+
+      status = subject.execute(hook)
+
+      expect(status[:async]).to be true
+    end
+  end
+
+  context 'with system hook' do
+    let_it_be(:hook) { create(:system_hook, url: "http://example.com") }
+    let_it_be(:log) { create_list(:web_hook_log, 3, web_hook: hook) }
+
+    it_behaves_like 'batched destroys'
+  end
+
+  context 'with project hook' do
+    let_it_be(:hook) { create(:project_hook) }
+    let_it_be(:log) { create_list(:web_hook_log, 3, web_hook: hook) }
+
+    it_behaves_like 'batched destroys'
+  end
+end

spec/support/shared_examples/controllers/destroy_hook_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'Web hook destroyer' do
+  it 'displays a message about synchronous delete', :aggregate_failures do
+    expect_next_instance_of(WebHooks::DestroyService) do |instance|
+      expect(instance).to receive(:execute).with(anything).and_call_original
+    end
+
+    delete :destroy, params: params
+
+    expect(response).to have_gitlab_http_status(:found)
+    expect(flash[:notice]).to eq("#{hook.model_name.human} was deleted")
+  end
+
+  it 'displays a message about async delete', :aggregate_failures do
+    expect_next_instance_of(WebHooks::DestroyService) do |instance|
+      expect(instance).to receive(:execute).with(anything).and_return({ status: :success, async: true } )
+    end
+
+    delete :destroy, params: params
+
+    expect(response).to have_gitlab_http_status(:found)
+    expect(flash[:notice]).to eq("#{hook.model_name.human} was scheduled for deletion")
+  end
+
+  it 'displays an error if deletion failed', :aggregate_failures do
+    expect_next_instance_of(WebHooks::DestroyService) do |instance|
+      expect(instance).to receive(:execute).with(anything).and_return({ status: :error, async: true, message: "failed" } )
+    end
+
+    delete :destroy, params: params
+
+    expect(response).to have_gitlab_http_status(:found)
+    expect(flash[:alert]).to eq("failed")
+  end
+end

spec/workers/web_hooks/destroy_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe WebHooks::DestroyWorker 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 Web hook and logs", :aggregate_failures do
+        expect { subject.perform(user.id, hook.id) }
+          .to change { WebHookLog.count }.from(2).to(1)
+          .and 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
+      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({ status: :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)
+        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)
+        end
+      end
+    end
+  end
+end