diff --git a/app/models/hooks/web_hook_log.rb b/app/models/hooks/web_hook_log.rb index 8c0565e4a38497127ab33073ecc52eb23fa48f0e..a95dd0473b69ed0ab5023811b8b342f71d5e399b 100644 --- a/app/models/hooks/web_hook_log.rb +++ b/app/models/hooks/web_hook_log.rb @@ -26,6 +26,13 @@ def self.recent .order(created_at: :desc) 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 + + where(web_hook: web_hook).limit(batch_size).delete_all == batch_size + end + def success? response_status =~ /^2/ end diff --git a/app/services/service_response.rb b/app/services/service_response.rb index 6bc394d2ae25f87d653aab5ed557e80297ba45eb..c7ab75a44266c0adebcdce5f38d3ada39df49672 100644 --- a/app/services/service_response.rb +++ b/app/services/service_response.rb @@ -18,6 +18,24 @@ def initialize(status:, message: nil, payload: {}, http_status: nil) self.http_status = http_status end + def track_exception(as: StandardError, **extra_data) + if error? + e = as.new(message) + Gitlab::ErrorTracking.track_exception(e, extra_data) + end + + self + end + + def track_and_raise_exception(as: StandardError, **extra_data) + if error? + e = as.new(message) + Gitlab::ErrorTracking.track_and_raise_exception(e, extra_data) + end + + self + end + def [](key) to_h[key] end diff --git a/app/services/web_hooks/destroy_service.rb b/app/services/web_hooks/destroy_service.rb index 58117985b56d0803df25f1bb6b3e5896a224974e..ecb530f0d2a23b32d935936ae126dc3e73551434 100644 --- a/app/services/web_hooks/destroy_service.rb +++ b/app/services/web_hooks/destroy_service.rb @@ -2,77 +2,27 @@ 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({ 'hook_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 - - 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 + # Backwards compatibility with WebHooks::DestroyWorker + alias_method :sync_destroy, :execute 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 0000000000000000000000000000000000000000..8a5d214a95e567029793a6d569d185483e6de14e --- /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 + next 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/all_queues.yml b/app/workers/all_queues.yml index e7ea39c913e85357b21f8d625bc26b6014e7a9d0..87a621980c79814f2c1408ca116b472c12115582 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: :low + :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/destroy_worker.rb b/app/workers/web_hooks/destroy_worker.rb index 822a1e770d7bcb54593ea03f94d7697da1780c68..8f9b194f88acdaf63759a9f21fb9e4b2d33f79f3 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 @@ -19,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 = ::WebHooks::DestroyService::DestroyError.new(result[:message]) - Gitlab::ErrorTracking.track_exception(e, web_hook_id: hook.id) - - raise e + 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 new file mode 100644 index 0000000000000000000000000000000000000000..9ea5c70e4166390c9cf5abb928e7613779a0b46a --- /dev/null +++ b/app/workers/web_hooks/log_destroy_worker.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +module WebHooks + class LogDestroyWorker + include ApplicationWorker + + DestroyError = Class.new(StandardError) + + data_consistency :always + feature_category :integrations + urgency :low + + idempotent! + + def perform(params = {}) + hook_id = params['hook_id'] + return unless hook_id + + result = ::WebHooks::LogDestroyService.new(hook_id).execute + + result.track_and_raise_exception(as: DestroyError, web_hook_id: hook_id) + end + end +end diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index f5d9708b55bc57ca522b35acbe4d11e68a85aec6..9f2eede1c47fb7920c35da30af976ff6f086e575 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 9cfbb14e0875a272f31402d967065566dee12481..e1fea3318f64b532ed68c59f1e8451afb7c7d752 100644 --- a/spec/models/hooks/web_hook_log_spec.rb +++ b/spec/models/hooks/web_hook_log_spec.rb @@ -48,6 +48,62 @@ end end + 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 + + 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 + + it 'does not loop forever' do + batches = 0 + batches += 1 while described_class.delete_batch_for(hook, batch_size: 1) + + 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 + 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 adea79509b3df1051721de421746d39e19b50241..86e9a176798d1751ce89ac6ebde1cdc8e78c6caf 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/service_response_spec.rb b/spec/services/service_response_spec.rb index 082ee4ddc673999e7527d38d20a4b4a533be0c5d..3ede90fbc44cb052fd9d7cd5bfeedb916fe6e029 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,76 @@ expect(described_class.error(message: 'error message').errors).to eq(['error message']) 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 + 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' do + expect(::Gitlab::ErrorTracking).to receive(:track_exception) + .with(StandardError.new('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(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(response.track_exception(foo: 1, bar: 2)).to be response + 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 5269fe08ac0cebd3c085c0da2a094ab232b49d08..4d9bb18e5402a046f96491e0608b87948924423a 100644 --- a/spec/services/web_hooks/destroy_service_spec.rb +++ b/spec/services/web_hooks/destroy_service_spec.rb @@ -7,50 +7,46 @@ 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) + describe '#execute' do + %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) } - 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 'is successful' do + expect(subject.execute(hook)).to be_success + end - it 'schedules an async delete' do - stub_const('WebHooks::DestroyService::LOG_COUNT_THRESHOLD', 1) + it 'destroys the hook' do + expect { subject.execute(hook) }.to change(WebHook, :count).from(1).to(0) + end - expect(WebHooks::DestroyWorker).to receive(:perform_async).with(user.id, hook.id).and_call_original + it 'does not destroy logs' do + expect { subject.execute(hook) }.not_to change(WebHookLog, :count) + end - status = subject.execute(hook) + 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/)) - expect(status[:async]).to be true - end - end + subject.execute(hook) + 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 'when the hook fails to destroy' do + before do + allow(hook).to receive(:destroy).and_return(false) + end - it_behaves_like 'batched destroys' - end + it 'is not a success' do + expect(WebHooks::LogDestroyWorker).not_to receive(:perform_async) - context 'with project hook' do - let!(:hook) { create(:project_hook) } - let!(:log) { create_list(:web_hook_log, 3, web_hook: hook) } + r = subject.execute(hook) - it_behaves_like 'batched destroys' + expect(r).to be_error + expect(r[:message]).to match %r{Unable to destroy} + end + end + 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 new file mode 100644 index 0000000000000000000000000000000000000000..7634726e5a4e97eb994a5419ca40e7f647bf86ab --- /dev/null +++ b/spec/services/web_hooks/log_destroy_service_spec.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe WebHooks::LogDestroyService do + subject(:service) { 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! # The LogDestroyService is expected to be called _after_ hook destruction + end + + it 'deletes the logs' do + 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(service.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") } + + it_behaves_like 'deletes web hook logs for hook' + end + + context 'with project hook' do + let!(:hook) { create(:project_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 fd26c8591ee6b19a146128bac0cd325961f7826e..8e75610a031bfeb21bf6d59393ac09aeaa20b184 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) } @@ -20,23 +22,26 @@ 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 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_next(::WebHooks::DestroyService) + .to receive(:sync_destroy).with(anything) + .and_return(ServiceResponse.error(message: "failed")) + + expect(Gitlab::ErrorTracking) + .to receive(:track_and_raise_exception) + .with(an_instance_of(described_class::DestroyError), { web_hook_id: hook.id }) + .and_call_original - 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) + 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 new file mode 100644 index 0000000000000000000000000000000000000000..0c107c05360c0f4726c16ed2c5a24b7cd835a965 --- /dev/null +++ b/spec/workers/web_hooks/log_destroy_worker_spec.rb @@ -0,0 +1,86 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe WebHooks::LogDestroyWorker do + include AfterNextHelpers + + let_it_be(:project) { create(:project) } + + subject { described_class.new } + + describe "#perform" 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) } + + 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({ 'hook_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 '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 + expect_next(::WebHooks::LogDestroyService) + .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), { web_hook_id: hook.id }) + .and_call_original + + expect { subject.perform({ 'hook_id' => hook.id }) } + .to raise_error(described_class::DestroyError) + end + + context 'with extra arguments' do + it 'does not raise an error' do + expect { subject.perform({ 'hook_id' => hook.id, 'extra' => true }) }.not_to raise_error + + expect(WebHook.count).to eq(2) + expect(WebHookLog.count).to eq(1) + end + end + end + + 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 + 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 +end