From 7ae0e5d9b965ccd900fa92ac03490b6a2b40b6e5 Mon Sep 17 00:00:00 2001 From: Adie Date: Sat, 13 Dec 2025 20:05:30 +0100 Subject: [PATCH 1/5] Move destroy entries worker to top level Move DestroyOrphanEntriesWorker to top level for reuse in container and maven EE: true --- ee/app/workers/all_queues.yml | 11 +++ .../cleanup_dependency_proxy_worker.rb | 2 +- .../cache/destroy_orphan_entries_worker.rb | 46 +--------- .../cache/destroy_orphan_entries_worker.rb | 70 +++++++++++++++ .../cleanup_dependency_proxy_worker_spec.rb | 4 +- .../destroy_orphan_entries_worker_spec.rb | 73 ++-------------- .../destroy_orphan_entries_worker_spec.rb | 86 +++++++++++++++++++ spec/workers/every_sidekiq_worker_spec.rb | 1 + 8 files changed, 180 insertions(+), 113 deletions(-) create mode 100644 ee/app/workers/virtual_registries/cache/destroy_orphan_entries_worker.rb create mode 100644 ee/spec/workers/virtual_registries/cache/destroy_orphan_entries_worker_spec.rb diff --git a/ee/app/workers/all_queues.yml b/ee/app/workers/all_queues.yml index 73f03bdec9f998..06a9c68588bde6 100644 --- a/ee/app/workers/all_queues.yml +++ b/ee/app/workers/all_queues.yml @@ -1033,6 +1033,17 @@ :idempotent: false :tags: [] :queue_namespace: :cronjob +- :name: dependency_proxy_blob:virtual_registries_cache_destroy_orphan_entries + :worker_name: VirtualRegistries::Cache::DestroyOrphanEntriesWorker + :feature_category: :virtual_registry + :has_external_dependencies: false + :urgency: :low + :resource_boundary: :unknown + :weight: 1 + :idempotent: true + :tags: + - :cronjob_child + :queue_namespace: :dependency_proxy_blob - :name: dependency_proxy_blob:virtual_registries_cache_mark_entries_for_destruction :worker_name: VirtualRegistries::Cache::MarkEntriesForDestructionWorker :feature_category: :virtual_registry diff --git a/ee/app/workers/ee/dependency_proxy/cleanup_dependency_proxy_worker.rb b/ee/app/workers/ee/dependency_proxy/cleanup_dependency_proxy_worker.rb index 638f73374f40fc..434794ae2696b9 100644 --- a/ee/app/workers/ee/dependency_proxy/cleanup_dependency_proxy_worker.rb +++ b/ee/app/workers/ee/dependency_proxy/cleanup_dependency_proxy_worker.rb @@ -16,7 +16,7 @@ def perform def enqueue_vreg_packages_cache_entry_cleanup_job [::VirtualRegistries::Packages::Maven::Cache::Entry].each do |klass| if klass.pending_destruction.any? - ::VirtualRegistries::Packages::Cache::DestroyOrphanEntriesWorker.perform_with_capacity(klass.name) + ::VirtualRegistries::Cache::DestroyOrphanEntriesWorker.perform_with_capacity(klass.name) end end end diff --git a/ee/app/workers/ee/virtual_registries/packages/cache/destroy_orphan_entries_worker.rb b/ee/app/workers/ee/virtual_registries/packages/cache/destroy_orphan_entries_worker.rb index 8b6488e6105818..966aa223635979 100644 --- a/ee/app/workers/ee/virtual_registries/packages/cache/destroy_orphan_entries_worker.rb +++ b/ee/app/workers/ee/virtual_registries/packages/cache/destroy_orphan_entries_worker.rb @@ -8,50 +8,8 @@ module DestroyOrphanEntriesWorker extend ::Gitlab::Utils::Override override :perform_work - def perform_work(model) - next_item = next_item(model.constantize) - - return unless next_item - - next_item.destroy! - log_metadata(next_item) - rescue StandardError => exception - next_item&.update_column(:status, :error) unless next_item&.destroyed? - - ::Gitlab::ErrorTracking.log_exception( - exception, - class: self.class.name - ) - end - - override :remaining_work_count - def remaining_work_count(model) - super - model.constantize.pending_destruction.limit(max_running_jobs + 1).count - end - - private - - def next_item(klass) - klass.transaction do - next_item = klass.next_pending_destruction - - if next_item - next_item.update_column(:status, :processing) - log_cleanup_item(next_item) - end - - next_item - end - end - - def log_metadata(cache_entry) - log_extra_metadata_on_done(:cache_entry_id, cache_entry.id) - log_extra_metadata_on_done(:group_id, cache_entry.group_id) - end - - def log_cleanup_item(cache_entry) - logger.info(structured_payload(cache_entry_id: cache_entry.id)) + def perform_work(_model) + # no-op end end end diff --git a/ee/app/workers/virtual_registries/cache/destroy_orphan_entries_worker.rb b/ee/app/workers/virtual_registries/cache/destroy_orphan_entries_worker.rb new file mode 100644 index 00000000000000..c517513574e643 --- /dev/null +++ b/ee/app/workers/virtual_registries/cache/destroy_orphan_entries_worker.rb @@ -0,0 +1,70 @@ +# frozen_string_literal: true + +module VirtualRegistries + module Cache + class DestroyOrphanEntriesWorker + include ApplicationWorker + include CronjobChildWorker + include LimitedCapacity::Worker + + MAX_CAPACITY = 2 + + data_consistency :sticky + urgency :low + idempotent! + + queue_namespace :dependency_proxy_blob + feature_category :virtual_registry + defer_on_database_health_signal :gitlab_main, + [:virtual_registries_cache_destroy_orphan_entries_worker], 5.minutes + + def perform_work(model) + next_item = next_item(model.constantize) + + return unless next_item + + next_item.destroy! + log_metadata(next_item) + rescue StandardError => exception + next_item&.update_column(:status, :error) unless next_item&.destroyed? + + ::Gitlab::ErrorTracking.log_exception( + exception, + class: self.class.name + ) + end + + def remaining_work_count(model) + model.constantize.pending_destruction.limit(max_running_jobs + 1).count + end + + def max_running_jobs + MAX_CAPACITY + end + + private + + def next_item(klass) + klass.transaction do + next_item = klass.next_pending_destruction + + if next_item + next_item.update_column(:status, :processing) + log_cleanup_item(next_item) + end + + next_item + end + end + + def log_metadata(cache_entry) + log_extra_metadata_on_done(:cache_entry_id, cache_entry.id) + log_extra_metadata_on_done(:group_id, cache_entry.group_id) + end + + def log_cleanup_item(cache_entry) + logger.info(structured_payload(cache_entry_id: cache_entry.id)) + end + end + end +end diff --git a/ee/spec/workers/ee/dependency_proxy/cleanup_dependency_proxy_worker_spec.rb b/ee/spec/workers/ee/dependency_proxy/cleanup_dependency_proxy_worker_spec.rb index 6e84d68ffdea39..d1e272c9d5d9d3 100644 --- a/ee/spec/workers/ee/dependency_proxy/cleanup_dependency_proxy_worker_spec.rb +++ b/ee/spec/workers/ee/dependency_proxy/cleanup_dependency_proxy_worker_spec.rb @@ -11,7 +11,7 @@ it 'queues the cleanup jobs', :aggregate_failures do create(:virtual_registries_packages_maven_cache_entry, :pending_destruction) - expect(::VirtualRegistries::Packages::Cache::DestroyOrphanEntriesWorker) + expect(::VirtualRegistries::Cache::DestroyOrphanEntriesWorker) .to receive(:perform_with_capacity).once worker_perform @@ -22,7 +22,7 @@ context 'when there are no records to be deleted' do it_behaves_like 'an idempotent worker' do it 'does not queue the cleanup jobs', :aggregate_failures do - expect(::VirtualRegistries::Packages::Cache::DestroyOrphanEntriesWorker) + expect(::VirtualRegistries::Cache::DestroyOrphanEntriesWorker) .not_to receive(:perform_with_capacity) worker_perform diff --git a/ee/spec/workers/ee/virtual_registries/packages/cache/destroy_orphan_entries_worker_spec.rb b/ee/spec/workers/ee/virtual_registries/packages/cache/destroy_orphan_entries_worker_spec.rb index d283adcbf23b55..4647956213dd8e 100644 --- a/ee/spec/workers/ee/virtual_registries/packages/cache/destroy_orphan_entries_worker_spec.rb +++ b/ee/spec/workers/ee/virtual_registries/packages/cache/destroy_orphan_entries_worker_spec.rb @@ -10,77 +10,18 @@ let(:job_args) { [model.name] } end - it_behaves_like 'worker with data consistency', described_class, data_consistency: :sticky - - it 'has a none deduplicate strategy' do - expect(described_class.get_deduplicate_strategy).to eq(:none) - end - describe '#perform_work' do - subject(:perform_work) { worker.perform_work(model.name) } - - context 'with no work to do' do - it { is_expected.to be_nil } - end - - context 'with work to do' do - let_it_be(:cache_entry) { create(:virtual_registries_packages_maven_cache_entry) } - let_it_be(:orphan_cache_entry) do - create(:virtual_registries_packages_maven_cache_entry, :pending_destruction) - end - - it 'destroys orphan cache entries' do - expect(worker).to receive(:log_extra_metadata_on_done).with(:cache_entry_id, - [orphan_cache_entry.upstream_id, orphan_cache_entry.relative_path, 'processing']) - expect(worker).to receive(:log_extra_metadata_on_done).with(:group_id, orphan_cache_entry.group_id) - expect(model).to receive(:next_pending_destruction).and_call_original - expect { perform_work }.to change { model.count }.by(-1) - expect { orphan_cache_entry.reset }.to raise_error(ActiveRecord::RecordNotFound) - end + subject(:perform_work) { worker.perform_work('') } - context 'with an error during deletion' do - before do - allow_next_found_instance_of(model) do |instance| - allow(instance).to receive(:destroy).and_raise(StandardError) - end - end - - it 'tracks the error' do - expect(Gitlab::ErrorTracking).to receive(:log_exception).with( - instance_of(StandardError), class: described_class.name - ) - - expect { perform_work }.to change { model.error.count }.by(1) - end - end - - context 'when trying to update a destroyed record' do - before do - allow_next_found_instance_of(model) do |instance| - destroy_method = instance.method(:destroy!) - - allow(instance).to receive(:destroy!) do - destroy_method.call - - raise StandardError - end - end - end - - it 'does not change the status to error' do - expect(Gitlab::ErrorTracking).to receive(:log_exception) - .with(instance_of(StandardError), class: described_class.name) - expect { perform_work }.not_to change { model.error.count } - end - end + it 'does not trigger any sql query' do + control = ActiveRecord::QueryRecorder.new { perform_work } + expect(control.count).to be_zero end end - describe '#max_running_jobs' do - let(:capacity) { described_class::MAX_CAPACITY } - - subject { worker.max_running_jobs } + describe '#remaining_work_count' do + subject { worker.remaining_work_count('') } - it { is_expected.to eq(capacity) } + it { is_expected.to be_zero } end end diff --git a/ee/spec/workers/virtual_registries/cache/destroy_orphan_entries_worker_spec.rb b/ee/spec/workers/virtual_registries/cache/destroy_orphan_entries_worker_spec.rb new file mode 100644 index 00000000000000..d7b2ebdcfbf74f --- /dev/null +++ b/ee/spec/workers/virtual_registries/cache/destroy_orphan_entries_worker_spec.rb @@ -0,0 +1,86 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe VirtualRegistries::Cache::DestroyOrphanEntriesWorker, feature_category: :virtual_registry do + let(:worker) { described_class.new } + let(:model) { ::VirtualRegistries::Packages::Maven::Cache::Entry } + + it_behaves_like 'an idempotent worker' do + let(:job_args) { [model.name] } + end + + it_behaves_like 'worker with data consistency', described_class, data_consistency: :sticky + + it 'has a none deduplicate strategy' do + expect(described_class.get_deduplicate_strategy).to eq(:none) + end + + describe '#perform_work' do + subject(:perform_work) { worker.perform_work(model.name) } + + context 'with no work to do' do + it { is_expected.to be_nil } + end + + context 'with work to do' do + let_it_be(:cache_entry) { create(:virtual_registries_packages_maven_cache_entry) } + let_it_be(:orphan_cache_entry) do + create(:virtual_registries_packages_maven_cache_entry, :pending_destruction) + end + + it 'destroys orphan cache entries' do + expect(worker).to receive(:log_extra_metadata_on_done).with(:cache_entry_id, + [orphan_cache_entry.upstream_id, orphan_cache_entry.relative_path, 'processing']) + expect(worker).to receive(:log_extra_metadata_on_done).with(:group_id, orphan_cache_entry.group_id) + expect(model).to receive(:next_pending_destruction).and_call_original + expect { perform_work }.to change { model.count }.by(-1) + expect { orphan_cache_entry.reset }.to raise_error(ActiveRecord::RecordNotFound) + end + + context 'with an error during deletion' do + before do + allow_next_found_instance_of(model) do |instance| + allow(instance).to receive(:destroy).and_raise(StandardError) + end + end + + it 'tracks the error' do + expect(Gitlab::ErrorTracking).to receive(:log_exception).with( + instance_of(StandardError), class: described_class.name + ) + + expect { perform_work }.to change { model.error.count }.by(1) + end + end + + context 'when trying to update a destroyed record' do + before do + allow_next_found_instance_of(model) do |instance| + destroy_method = instance.method(:destroy!) + + allow(instance).to receive(:destroy!) do + destroy_method.call + + raise StandardError + end + end + end + + it 'does not change the status to error' do + expect(Gitlab::ErrorTracking).to receive(:log_exception) + .with(instance_of(StandardError), class: described_class.name) + expect { perform_work }.not_to change { model.error.count } + end + end + end + end + + describe '#max_running_jobs' do + let(:capacity) { described_class::MAX_CAPACITY } + + subject { worker.max_running_jobs } + + it { is_expected.to eq(capacity) } + end +end diff --git a/spec/workers/every_sidekiq_worker_spec.rb b/spec/workers/every_sidekiq_worker_spec.rb index ebffbde2b61c05..e30c9cde590f7d 100644 --- a/spec/workers/every_sidekiq_worker_spec.rb +++ b/spec/workers/every_sidekiq_worker_spec.rb @@ -508,6 +508,7 @@ 'Vulnerabilities::UpdateArchivedAttributeOfVulnerabilityReadsWorker' => 2, 'VulnerabilityExports::ExportDeletionWorker' => 3, 'VulnerabilityExports::ExportWorker' => 3, + 'VirtualRegistries::Cache::DestroyOrphanEntriesWorker' => 0, 'VirtualRegistries::Packages::Cache::DestroyOrphanEntriesWorker' => 0, 'VirtualRegistries::Cleanup::ExecutePolicyWorker' => 0, 'WaitForClusterCreationWorker' => 3, -- GitLab From 742f4af6ba59713feab1e0a9a469cdac58d5cdea Mon Sep 17 00:00:00 2001 From: Adie Date: Mon, 15 Dec 2025 19:07:56 +0100 Subject: [PATCH 2/5] Apply feedback from backend review --- .../cache/destroy_orphan_entries_worker.rb | 2 ++ .../destroy_orphan_entries_worker_spec.rb | 24 +++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/ee/app/workers/ee/virtual_registries/packages/cache/destroy_orphan_entries_worker.rb b/ee/app/workers/ee/virtual_registries/packages/cache/destroy_orphan_entries_worker.rb index 966aa223635979..dcd7ff350614c5 100644 --- a/ee/app/workers/ee/virtual_registries/packages/cache/destroy_orphan_entries_worker.rb +++ b/ee/app/workers/ee/virtual_registries/packages/cache/destroy_orphan_entries_worker.rb @@ -7,6 +7,8 @@ module Cache module DestroyOrphanEntriesWorker extend ::Gitlab::Utils::Override + # a no-op worker that should be removed in 18.10 according to + # https://docs.gitlab.com/development/sidekiq/compatibility_across_updates/#removing-worker-classes override :perform_work def perform_work(_model) # no-op diff --git a/ee/spec/workers/virtual_registries/cache/destroy_orphan_entries_worker_spec.rb b/ee/spec/workers/virtual_registries/cache/destroy_orphan_entries_worker_spec.rb index d7b2ebdcfbf74f..3f1bacebf6e6b0 100644 --- a/ee/spec/workers/virtual_registries/cache/destroy_orphan_entries_worker_spec.rb +++ b/ee/spec/workers/virtual_registries/cache/destroy_orphan_entries_worker_spec.rb @@ -83,4 +83,28 @@ it { is_expected.to eq(capacity) } end + + describe '#remaining_work_count' do + subject { worker.remaining_work_count(model.name) } + + context 'with no orphan entries' do + it { is_expected.to be_zero } + end + + context 'with orphan entries' do + let_it_be(:orphan_entries) do + create_list(:virtual_registries_packages_maven_cache_entry, 3, :pending_destruction) + end + + it { is_expected.to eq(3) } + end + + context 'when count exceeds max_running_jobs' do + let_it_be(:orphan_entries) do + create_list(:virtual_registries_packages_maven_cache_entry, 5, :pending_destruction) + end + + it { is_expected.to eq(worker.max_running_jobs + 1) } + end + end end -- GitLab From 74ec25d6cbbdd3bd03b167828220c13d67463e8b Mon Sep 17 00:00:00 2001 From: Adie Date: Tue, 16 Dec 2025 09:04:28 +0100 Subject: [PATCH 3/5] Add test for error case --- .../destroy_orphan_entries_worker_spec.rb | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/ee/spec/workers/virtual_registries/cache/destroy_orphan_entries_worker_spec.rb b/ee/spec/workers/virtual_registries/cache/destroy_orphan_entries_worker_spec.rb index 3f1bacebf6e6b0..2af513aa2eed1a 100644 --- a/ee/spec/workers/virtual_registries/cache/destroy_orphan_entries_worker_spec.rb +++ b/ee/spec/workers/virtual_registries/cache/destroy_orphan_entries_worker_spec.rb @@ -73,6 +73,30 @@ expect { perform_work }.not_to change { model.error.count } end end + + context 'when an error occurs during destruction' do + before do + allow_next_found_instance_of(model) do |instance| + allow(instance).to receive(:destroy!).and_raise(StandardError, 'Test error') + end + end + + it 'logs the exception with correct metadata' do + expect(::Gitlab::ErrorTracking).to receive(:log_exception).with( + instance_of(StandardError), + class: described_class.name + ) + + perform_work + end + + it 'updates the status to error when record is not destroyed' do + expect(Gitlab::ErrorTracking).to receive(:log_exception) + .with(instance_of(StandardError), class: described_class.name) + + expect { perform_work }.to change { model.error.count }.by(1) + end + end end end -- GitLab From 6d65e9dcbeb2ac6fb8cda69c4c057afd6efc00f6 Mon Sep 17 00:00:00 2001 From: Adie Date: Tue, 16 Dec 2025 21:17:27 +0100 Subject: [PATCH 4/5] chore: retrigger CI for coverage check -- GitLab From 5ccef8f87a0bf2e60eb1c9ebf13fb541f1035eac Mon Sep 17 00:00:00 2001 From: Adie Date: Wed, 17 Dec 2025 13:48:09 +0100 Subject: [PATCH 5/5] Feedback from maintainer review Remove unused spec; fix defer_on_database_health_signal --- .../packages/cache/destroy_orphan_entries_worker.rb | 4 +--- .../cache/destroy_orphan_entries_worker.rb | 5 +++-- .../packages/cache/destroy_orphan_entries_worker_spec.rb | 4 ---- 3 files changed, 4 insertions(+), 9 deletions(-) diff --git a/ee/app/workers/ee/virtual_registries/packages/cache/destroy_orphan_entries_worker.rb b/ee/app/workers/ee/virtual_registries/packages/cache/destroy_orphan_entries_worker.rb index dcd7ff350614c5..dec31142e5ce6e 100644 --- a/ee/app/workers/ee/virtual_registries/packages/cache/destroy_orphan_entries_worker.rb +++ b/ee/app/workers/ee/virtual_registries/packages/cache/destroy_orphan_entries_worker.rb @@ -10,9 +10,7 @@ module DestroyOrphanEntriesWorker # a no-op worker that should be removed in 18.10 according to # https://docs.gitlab.com/development/sidekiq/compatibility_across_updates/#removing-worker-classes override :perform_work - def perform_work(_model) - # no-op - end + def perform_work(_model); end end end end diff --git a/ee/app/workers/virtual_registries/cache/destroy_orphan_entries_worker.rb b/ee/app/workers/virtual_registries/cache/destroy_orphan_entries_worker.rb index c517513574e643..a4886dcb145f64 100644 --- a/ee/app/workers/virtual_registries/cache/destroy_orphan_entries_worker.rb +++ b/ee/app/workers/virtual_registries/cache/destroy_orphan_entries_worker.rb @@ -15,8 +15,9 @@ class DestroyOrphanEntriesWorker queue_namespace :dependency_proxy_blob feature_category :virtual_registry - defer_on_database_health_signal :gitlab_main, - [:virtual_registries_cache_destroy_orphan_entries_worker], 5.minutes + defer_on_database_health_signal :gitlab_main, 5.minutes do + [::VirtualRegistries::Packages::Maven::Cache::Entry] + end def perform_work(model) next_item = next_item(model.constantize) diff --git a/ee/spec/workers/ee/virtual_registries/packages/cache/destroy_orphan_entries_worker_spec.rb b/ee/spec/workers/ee/virtual_registries/packages/cache/destroy_orphan_entries_worker_spec.rb index 4647956213dd8e..b9eb753a9fc5c6 100644 --- a/ee/spec/workers/ee/virtual_registries/packages/cache/destroy_orphan_entries_worker_spec.rb +++ b/ee/spec/workers/ee/virtual_registries/packages/cache/destroy_orphan_entries_worker_spec.rb @@ -6,10 +6,6 @@ let(:worker) { described_class.new } let(:model) { ::VirtualRegistries::Packages::Maven::Cache::Entry } - it_behaves_like 'an idempotent worker' do - let(:job_args) { [model.name] } - end - describe '#perform_work' do subject(:perform_work) { worker.perform_work('') } -- GitLab