From 4bd9ea2558b4be52d5926f78b058f109f1e89433 Mon Sep 17 00:00:00 2001 From: moaz-khalifa Date: Mon, 1 Dec 2025 15:08:44 +0100 Subject: [PATCH 1/3] Refactor cache purging logic to use a common worker --- .../virtual_registries/container/registry.rb | 4 +- .../virtual_registries/container/upstream.rb | 2 +- .../packages/maven/registry.rb | 4 +- .../packages/maven/upstream.rb | 2 +- ee/app/workers/all_queues.yml | 10 ++++ .../mark_entries_for_destruction_worker.rb | 41 +++++++++++++ .../mark_entries_for_destruction_worker.rb | 16 +---- .../mark_entries_for_destruction_worker.rb | 16 +---- .../container/registry_spec.rb | 2 +- .../container/upstream_spec.rb | 8 ++- .../packages/maven/registry_spec.rb | 2 +- .../packages/maven/upstream_spec.rb | 8 ++- ...ark_entries_for_destruction_worker_spec.rb | 58 +++++++++++++++++++ ...ark_entries_for_destruction_worker_spec.rb | 31 +--------- ...ark_entries_for_destruction_worker_spec.rb | 31 +--------- 15 files changed, 137 insertions(+), 98 deletions(-) create mode 100644 ee/app/workers/virtual_registries/cache/mark_entries_for_destruction_worker.rb create mode 100644 ee/spec/workers/virtual_registries/cache/mark_entries_for_destruction_worker_spec.rb diff --git a/ee/app/models/virtual_registries/container/registry.rb b/ee/app/models/virtual_registries/container/registry.rb index 1665f25e727f5d..f151b77e90dc14 100644 --- a/ee/app/models/virtual_registries/container/registry.rb +++ b/ee/app/models/virtual_registries/container/registry.rb @@ -14,9 +14,9 @@ class Registry < ::VirtualRegistries::Registry through: :registry_upstreams def purge_cache! - ::VirtualRegistries::Container::Cache::MarkEntriesForDestructionWorker.bulk_perform_async_with_contexts( + ::VirtualRegistries::Cache::MarkEntriesForDestructionWorker.bulk_perform_async_with_contexts( exclusive_upstreams, - arguments_proc: ->(upstream) { [upstream.id] }, + arguments_proc: ->(upstream) { [upstream.to_global_id.to_s] }, context_proc: ->(upstream) { { namespace: upstream.group } } ) end diff --git a/ee/app/models/virtual_registries/container/upstream.rb b/ee/app/models/virtual_registries/container/upstream.rb index f8baae5859a466..78052e4b3d8011 100644 --- a/ee/app/models/virtual_registries/container/upstream.rb +++ b/ee/app/models/virtual_registries/container/upstream.rb @@ -67,7 +67,7 @@ def default_cache_entries end def purge_cache! - ::VirtualRegistries::Container::Cache::MarkEntriesForDestructionWorker.perform_async(id) + ::VirtualRegistries::Cache::MarkEntriesForDestructionWorker.perform_async(to_global_id.to_s) end private diff --git a/ee/app/models/virtual_registries/packages/maven/registry.rb b/ee/app/models/virtual_registries/packages/maven/registry.rb index b973aaa3bc831e..dcd567997fa9c4 100644 --- a/ee/app/models/virtual_registries/packages/maven/registry.rb +++ b/ee/app/models/virtual_registries/packages/maven/registry.rb @@ -15,9 +15,9 @@ class Registry < ::VirtualRegistries::Registry through: :registry_upstreams def purge_cache! - ::VirtualRegistries::Packages::Cache::MarkEntriesForDestructionWorker.bulk_perform_async_with_contexts( + ::VirtualRegistries::Cache::MarkEntriesForDestructionWorker.bulk_perform_async_with_contexts( exclusive_upstreams, - arguments_proc: ->(upstream) { [upstream.id] }, + arguments_proc: ->(upstream) { [upstream.to_global_id.to_s] }, context_proc: ->(upstream) { { namespace: upstream.group } } ) end diff --git a/ee/app/models/virtual_registries/packages/maven/upstream.rb b/ee/app/models/virtual_registries/packages/maven/upstream.rb index bae7e7eab3f151..a49e8eaec012ff 100644 --- a/ee/app/models/virtual_registries/packages/maven/upstream.rb +++ b/ee/app/models/virtual_registries/packages/maven/upstream.rb @@ -126,7 +126,7 @@ def object_storage_key end def purge_cache! - ::VirtualRegistries::Packages::Cache::MarkEntriesForDestructionWorker.perform_async(id) + ::VirtualRegistries::Cache::MarkEntriesForDestructionWorker.perform_async(to_global_id.to_s) end def test diff --git a/ee/app/workers/all_queues.yml b/ee/app/workers/all_queues.yml index bd6897644b0a84..1f5702b60f0615 100644 --- a/ee/app/workers/all_queues.yml +++ b/ee/app/workers/all_queues.yml @@ -1033,6 +1033,16 @@ :idempotent: false :tags: [] :queue_namespace: :cronjob +- :name: dependency_proxy_blob:virtual_registries_cache_mark_entries_for_destruction + :worker_name: VirtualRegistries::Cache::MarkEntriesForDestructionWorker + :feature_category: :virtual_registry + :has_external_dependencies: false + :urgency: :low + :resource_boundary: :unknown + :weight: 1 + :idempotent: true + :tags: [] + :queue_namespace: :dependency_proxy_blob - :name: dependency_proxy_blob:virtual_registries_container_cache_mark_entries_for_destruction :worker_name: VirtualRegistries::Container::Cache::MarkEntriesForDestructionWorker :feature_category: :virtual_registry diff --git a/ee/app/workers/virtual_registries/cache/mark_entries_for_destruction_worker.rb b/ee/app/workers/virtual_registries/cache/mark_entries_for_destruction_worker.rb new file mode 100644 index 00000000000000..dbd75132337bbd --- /dev/null +++ b/ee/app/workers/virtual_registries/cache/mark_entries_for_destruction_worker.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +module VirtualRegistries + module Cache + class MarkEntriesForDestructionWorker + include ApplicationWorker + + BATCH_SIZE = 500 + ALLOWED_UPSTREAM_CLASSES = [ + ::VirtualRegistries::Packages::Maven::Upstream, + ::VirtualRegistries::Container::Upstream + ].freeze + + data_consistency :sticky + queue_namespace :dependency_proxy_blob + feature_category :virtual_registry + urgency :low + defer_on_database_health_signal :gitlab_main, + %i[virtual_registries_packages_maven_cache_entries virtual_registries_container_cache_entries], 5.minutes + deduplicate :until_executed + idempotent! + + def perform(upstream_gid) + upstream = begin + GlobalID::Locator.locate(upstream_gid, only: ALLOWED_UPSTREAM_CLASSES) + rescue ActiveRecord::RecordNotFound + end + + return unless upstream + + upstream.default_cache_entries.each_batch(of: BATCH_SIZE, column: :relative_path) do |batch| + batch.update_all( + status: :pending_destruction, + relative_path: Arel.sql("relative_path || '/deleted/' || gen_random_uuid()"), + updated_at: Time.current + ) + end + end + end + end +end diff --git a/ee/app/workers/virtual_registries/container/cache/mark_entries_for_destruction_worker.rb b/ee/app/workers/virtual_registries/container/cache/mark_entries_for_destruction_worker.rb index 85d0855818085f..5b12cbc180a23c 100644 --- a/ee/app/workers/virtual_registries/container/cache/mark_entries_for_destruction_worker.rb +++ b/ee/app/workers/virtual_registries/container/cache/mark_entries_for_destruction_worker.rb @@ -6,8 +6,6 @@ module Cache class MarkEntriesForDestructionWorker include ApplicationWorker - BATCH_SIZE = 500 - data_consistency :sticky queue_namespace :dependency_proxy_blob feature_category :virtual_registry @@ -16,19 +14,7 @@ class MarkEntriesForDestructionWorker deduplicate :until_executed idempotent! - def perform(upstream_id) - upstream = ::VirtualRegistries::Container::Upstream.find_by_id(upstream_id) - - return unless upstream - - upstream.default_cache_entries.each_batch(of: BATCH_SIZE, column: :relative_path) do |batch| - batch.update_all( - status: :pending_destruction, - relative_path: Arel.sql("relative_path || '/deleted/' || gen_random_uuid()"), - updated_at: Time.current - ) - end - end + def perform(upstream_id); end end end end diff --git a/ee/app/workers/virtual_registries/packages/cache/mark_entries_for_destruction_worker.rb b/ee/app/workers/virtual_registries/packages/cache/mark_entries_for_destruction_worker.rb index a0dd3d33edba50..b938c5ae79fa08 100644 --- a/ee/app/workers/virtual_registries/packages/cache/mark_entries_for_destruction_worker.rb +++ b/ee/app/workers/virtual_registries/packages/cache/mark_entries_for_destruction_worker.rb @@ -6,8 +6,6 @@ module Cache class MarkEntriesForDestructionWorker include ApplicationWorker - BATCH_SIZE = 500 - data_consistency :sticky queue_namespace :dependency_proxy_blob feature_category :virtual_registry @@ -16,19 +14,7 @@ class MarkEntriesForDestructionWorker deduplicate :until_executed idempotent! - def perform(upstream_id) - upstream = ::VirtualRegistries::Packages::Maven::Upstream.find_by_id(upstream_id) - - return unless upstream - - upstream.default_cache_entries.each_batch(of: BATCH_SIZE, column: :relative_path) do |batch| - batch.update_all( - status: :pending_destruction, - relative_path: Arel.sql("relative_path || '/deleted/' || gen_random_uuid()"), - updated_at: Time.current - ) - end - end + def perform(upstream_id); end end end end diff --git a/ee/spec/models/virtual_registries/container/registry_spec.rb b/ee/spec/models/virtual_registries/container/registry_spec.rb index b352dba636632c..c3117301cc156e 100644 --- a/ee/spec/models/virtual_registries/container/registry_spec.rb +++ b/ee/spec/models/virtual_registries/container/registry_spec.rb @@ -46,7 +46,7 @@ let_it_be(:upstream2) { create(:virtual_registries_container_upstream, registries: [registry1]) } it 'bulk enqueues the MarkEntriesForDestructionWorker' do - expect(::VirtualRegistries::Container::Cache::MarkEntriesForDestructionWorker) + expect(::VirtualRegistries::Cache::MarkEntriesForDestructionWorker) .to receive(:bulk_perform_async_with_contexts) .with([upstream2], arguments_proc: kind_of(Proc), context_proc: kind_of(Proc)) diff --git a/ee/spec/models/virtual_registries/container/upstream_spec.rb b/ee/spec/models/virtual_registries/container/upstream_spec.rb index 2b24dfd9777bde..7f5efddbc68071 100644 --- a/ee/spec/models/virtual_registries/container/upstream_spec.rb +++ b/ee/spec/models/virtual_registries/container/upstream_spec.rb @@ -723,9 +723,13 @@ def with_accept_headers(headers) end describe '#purge_cache!' do + before do + upstream.save! + end + it 'enqueues the MarkEntriesForDestructionWorker' do - expect(::VirtualRegistries::Container::Cache::MarkEntriesForDestructionWorker) - .to receive(:perform_async).with(upstream.id) + expect(::VirtualRegistries::Cache::MarkEntriesForDestructionWorker) + .to receive(:perform_async).with(upstream.to_global_id.to_s) upstream.purge_cache! end diff --git a/ee/spec/models/virtual_registries/packages/maven/registry_spec.rb b/ee/spec/models/virtual_registries/packages/maven/registry_spec.rb index 60f71a1152f0f0..c65083ebc78c42 100644 --- a/ee/spec/models/virtual_registries/packages/maven/registry_spec.rb +++ b/ee/spec/models/virtual_registries/packages/maven/registry_spec.rb @@ -64,7 +64,7 @@ let_it_be(:upstream2) { create(:virtual_registries_packages_maven_upstream, registries: [registry1]) } it 'bulk enqueues the MarkEntriesForDestructionWorker' do - expect(::VirtualRegistries::Packages::Cache::MarkEntriesForDestructionWorker) + expect(::VirtualRegistries::Cache::MarkEntriesForDestructionWorker) .to receive(:bulk_perform_async_with_contexts) .with([upstream2], arguments_proc: kind_of(Proc), context_proc: kind_of(Proc)) diff --git a/ee/spec/models/virtual_registries/packages/maven/upstream_spec.rb b/ee/spec/models/virtual_registries/packages/maven/upstream_spec.rb index da8acd75e18abc..cc897836747129 100644 --- a/ee/spec/models/virtual_registries/packages/maven/upstream_spec.rb +++ b/ee/spec/models/virtual_registries/packages/maven/upstream_spec.rb @@ -647,9 +647,13 @@ end describe '#purge_cache!' do + before do + upstream.save! + end + it 'enqueues the MarkEntriesForDestructionWorker' do - expect(::VirtualRegistries::Packages::Cache::MarkEntriesForDestructionWorker) - .to receive(:perform_async).with(upstream.id) + expect(::VirtualRegistries::Cache::MarkEntriesForDestructionWorker) + .to receive(:perform_async).with(upstream.to_global_id.to_s) upstream.purge_cache! end diff --git a/ee/spec/workers/virtual_registries/cache/mark_entries_for_destruction_worker_spec.rb b/ee/spec/workers/virtual_registries/cache/mark_entries_for_destruction_worker_spec.rb new file mode 100644 index 00000000000000..77ebf82bd08558 --- /dev/null +++ b/ee/spec/workers/virtual_registries/cache/mark_entries_for_destruction_worker_spec.rb @@ -0,0 +1,58 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe VirtualRegistries::Cache::MarkEntriesForDestructionWorker, feature_category: :virtual_registry do + let_it_be(:upstream) { create(:virtual_registries_container_upstream) } + + let(:worker) { described_class.new } + + subject(:perform) { worker.perform(upstream_gid) } + + it_behaves_like 'worker with data consistency', described_class, data_consistency: :sticky + it_behaves_like 'an idempotent worker' do + let(:job_args) { [upstream.to_global_id.to_s] } + end + + it 'has an until_executed deduplicate strategy' do + expect(described_class.get_deduplicate_strategy).to eq(:until_executed) + end + + describe '#perform' do + context 'when the upstream is found' do + let(:upstream_gid) { upstream.to_global_id.to_s } + + before do + create_list(:virtual_registries_container_cache_entry, 3, upstream:) # 3 default + create(:virtual_registries_container_cache_entry, :pending_destruction, upstream:) # 1 pending destruction + create(:virtual_registries_container_cache_entry) # 1 default in another upstream + end + + it 'marks default cache entries for destruction' do + expect { perform }.to change { + ::VirtualRegistries::Container::Cache::Entry.pending_destruction.size + }.by(3) + end + end + + context 'when the upstream is not found' do + let(:upstream_gid) { "gid://gitlab/VirtualRegistries::Container::Upstream/#{non_existing_record_id}" } + + it 'does not mark any cache entries for destruction' do + expect { perform }.not_to change { ::VirtualRegistries::Container::Cache::Entry.count } + + is_expected.to be_nil + end + end + + context 'for unsupported class' do + let(:upstream_gid) { upstream.group.to_global_id.to_s } + + it 'does not mark any cache entries for destruction' do + expect { perform }.not_to change { ::VirtualRegistries::Container::Cache::Entry.count } + + is_expected.to be_nil + end + end + end +end diff --git a/ee/spec/workers/virtual_registries/container/cache/mark_entries_for_destruction_worker_spec.rb b/ee/spec/workers/virtual_registries/container/cache/mark_entries_for_destruction_worker_spec.rb index 512a9fd735fa90..5babd3103fe94f 100644 --- a/ee/spec/workers/virtual_registries/container/cache/mark_entries_for_destruction_worker_spec.rb +++ b/ee/spec/workers/virtual_registries/container/cache/mark_entries_for_destruction_worker_spec.rb @@ -3,11 +3,10 @@ require 'spec_helper' RSpec.describe VirtualRegistries::Container::Cache::MarkEntriesForDestructionWorker, feature_category: :virtual_registry do - let_it_be(:upstream) { create(:virtual_registries_container_upstream) } - + let(:upstream) { build_stubbed(:virtual_registries_container_upstream) } let(:worker) { described_class.new } - subject(:perform) { worker.perform(upstream_id) } + subject { worker.perform(upstream.id) } it_behaves_like 'worker with data consistency', described_class, data_consistency: :sticky it_behaves_like 'an idempotent worker' do @@ -19,30 +18,6 @@ end describe '#perform' do - context 'when the upstream is found' do - let(:upstream_id) { upstream.id } - - before do - create_list(:virtual_registries_container_cache_entry, 3, upstream:) # 3 default - create(:virtual_registries_container_cache_entry, :pending_destruction, upstream:) # 1 pending destruction - create(:virtual_registries_container_cache_entry) # 1 default in another upstream - end - - it 'marks default cache entries for destruction' do - expect { perform }.to change { - ::VirtualRegistries::Container::Cache::Entry.pending_destruction.size - }.by(3) - end - end - - context 'when the upstream is not found' do - let(:upstream_id) { non_existing_record_id } - - it 'does not mark any cache entries for destruction' do - expect { perform }.not_to change { ::VirtualRegistries::Container::Cache::Entry.count } - - is_expected.to be_nil - end - end + it { is_expected.to be_nil } end end diff --git a/ee/spec/workers/virtual_registries/packages/cache/mark_entries_for_destruction_worker_spec.rb b/ee/spec/workers/virtual_registries/packages/cache/mark_entries_for_destruction_worker_spec.rb index 1db4e4b638fd62..310f9dd4a5cfe1 100644 --- a/ee/spec/workers/virtual_registries/packages/cache/mark_entries_for_destruction_worker_spec.rb +++ b/ee/spec/workers/virtual_registries/packages/cache/mark_entries_for_destruction_worker_spec.rb @@ -3,11 +3,10 @@ require 'spec_helper' RSpec.describe VirtualRegistries::Packages::Cache::MarkEntriesForDestructionWorker, type: :worker, feature_category: :virtual_registry do - let_it_be(:upstream) { create(:virtual_registries_packages_maven_upstream) } - + let(:upstream) { build_stubbed(:virtual_registries_packages_maven_upstream) } let(:worker) { described_class.new } - subject(:perform) { worker.perform(upstream_id) } + subject { worker.perform(upstream.id) } it_behaves_like 'worker with data consistency', described_class, data_consistency: :sticky it_behaves_like 'an idempotent worker' do @@ -19,30 +18,6 @@ end describe '#perform' do - context 'when the upstream is found' do - let(:upstream_id) { upstream.id } - - before do - create_list(:virtual_registries_packages_maven_cache_entry, 3, upstream:) # 3 default - create(:virtual_registries_packages_maven_cache_entry, :pending_destruction, upstream:) # 1 pending destruction - create(:virtual_registries_packages_maven_cache_entry) # 1 default in another upstream - end - - it 'marks default cache entries for destruction' do - expect { perform }.to change { - ::VirtualRegistries::Packages::Maven::Cache::Entry.pending_destruction.size - }.by(3) - end - end - - context 'when the upstream is not found' do - let(:upstream_id) { non_existing_record_id } - - it 'does not mark any cache entries for destruction' do - expect { perform }.not_to change { ::VirtualRegistries::Packages::Maven::Cache::Entry.count } - - is_expected.to be_nil - end - end + it { is_expected.to be_nil } end end -- GitLab From 0c55a400cdce79d7c434b713e7db272fae0520e8 Mon Sep 17 00:00:00 2001 From: moaz-khalifa Date: Tue, 2 Dec 2025 13:15:04 +0100 Subject: [PATCH 2/3] Address reviewer feedback --- ee/app/models/virtual_registries/container/registry.rb | 8 -------- ee/app/models/virtual_registries/container/upstream.rb | 4 ---- .../models/virtual_registries/packages/maven/registry.rb | 8 -------- ee/app/models/virtual_registries/registry.rb | 8 ++++++++ ee/app/models/virtual_registries/upstream.rb | 4 ++++ 5 files changed, 12 insertions(+), 20 deletions(-) diff --git a/ee/app/models/virtual_registries/container/registry.rb b/ee/app/models/virtual_registries/container/registry.rb index f151b77e90dc14..7d9cf3add299a9 100644 --- a/ee/app/models/virtual_registries/container/registry.rb +++ b/ee/app/models/virtual_registries/container/registry.rb @@ -12,14 +12,6 @@ class Registry < ::VirtualRegistries::Registry has_many :upstreams, class_name: '::VirtualRegistries::Container::Upstream', through: :registry_upstreams - - def purge_cache! - ::VirtualRegistries::Cache::MarkEntriesForDestructionWorker.bulk_perform_async_with_contexts( - exclusive_upstreams, - arguments_proc: ->(upstream) { [upstream.to_global_id.to_s] }, - context_proc: ->(upstream) { { namespace: upstream.group } } - ) - end end end end diff --git a/ee/app/models/virtual_registries/container/upstream.rb b/ee/app/models/virtual_registries/container/upstream.rb index 78052e4b3d8011..6e76cb2a870717 100644 --- a/ee/app/models/virtual_registries/container/upstream.rb +++ b/ee/app/models/virtual_registries/container/upstream.rb @@ -66,10 +66,6 @@ def default_cache_entries cache_entries.default end - def purge_cache! - ::VirtualRegistries::Cache::MarkEntriesForDestructionWorker.perform_async(to_global_id.to_s) - end - private def get_bearer_token(path) diff --git a/ee/app/models/virtual_registries/packages/maven/registry.rb b/ee/app/models/virtual_registries/packages/maven/registry.rb index dcd567997fa9c4..5e70cdd09369da 100644 --- a/ee/app/models/virtual_registries/packages/maven/registry.rb +++ b/ee/app/models/virtual_registries/packages/maven/registry.rb @@ -13,14 +13,6 @@ class Registry < ::VirtualRegistries::Registry has_many :upstreams, class_name: 'VirtualRegistries::Packages::Maven::Upstream', through: :registry_upstreams - - def purge_cache! - ::VirtualRegistries::Cache::MarkEntriesForDestructionWorker.bulk_perform_async_with_contexts( - exclusive_upstreams, - arguments_proc: ->(upstream) { [upstream.to_global_id.to_s] }, - context_proc: ->(upstream) { { namespace: upstream.group } } - ) - end end end end diff --git a/ee/app/models/virtual_registries/registry.rb b/ee/app/models/virtual_registries/registry.rb index 2cba46c6522cee..da8431bbcdba52 100644 --- a/ee/app/models/virtual_registries/registry.rb +++ b/ee/app/models/virtual_registries/registry.rb @@ -29,6 +29,14 @@ def exclusive_upstreams .where_not_exists(subquery) end + def purge_cache! + ::VirtualRegistries::Cache::MarkEntriesForDestructionWorker.bulk_perform_async_with_contexts( + exclusive_upstreams, + arguments_proc: ->(upstream) { [upstream.to_global_id.to_s] }, + context_proc: ->(upstream) { { namespace: upstream.group } } + ) + end + private def delete_upstreams diff --git a/ee/app/models/virtual_registries/upstream.rb b/ee/app/models/virtual_registries/upstream.rb index 9652f9e1e43c26..26da32728a3494 100644 --- a/ee/app/models/virtual_registries/upstream.rb +++ b/ee/app/models/virtual_registries/upstream.rb @@ -68,5 +68,9 @@ def local? def remote? true end + + def purge_cache! + ::VirtualRegistries::Cache::MarkEntriesForDestructionWorker.perform_async(to_global_id.to_s) + end end end -- GitLab From 0f552d499210526803d3eaef5c3d63f07fbd837e Mon Sep 17 00:00:00 2001 From: moaz-khalifa Date: Thu, 4 Dec 2025 14:35:05 +0100 Subject: [PATCH 3/3] Address maintainer feedback --- .../mark_entries_for_destruction_worker.rb | 14 ++- .../mark_entries_for_destruction_worker.rb | 2 + .../mark_entries_for_destruction_worker.rb | 2 + .../container/upstream_spec.rb | 4 +- .../packages/maven/upstream_spec.rb | 4 +- ...ark_entries_for_destruction_worker_spec.rb | 93 +++++++++++++------ 6 files changed, 80 insertions(+), 39 deletions(-) diff --git a/ee/app/workers/virtual_registries/cache/mark_entries_for_destruction_worker.rb b/ee/app/workers/virtual_registries/cache/mark_entries_for_destruction_worker.rb index dbd75132337bbd..3ebb7fd2a6aefe 100644 --- a/ee/app/workers/virtual_registries/cache/mark_entries_for_destruction_worker.rb +++ b/ee/app/workers/virtual_registries/cache/mark_entries_for_destruction_worker.rb @@ -21,10 +21,7 @@ class MarkEntriesForDestructionWorker idempotent! def perform(upstream_gid) - upstream = begin - GlobalID::Locator.locate(upstream_gid, only: ALLOWED_UPSTREAM_CLASSES) - rescue ActiveRecord::RecordNotFound - end + upstream = safe_locate(upstream_gid) return unless upstream @@ -36,6 +33,15 @@ def perform(upstream_gid) ) end end + + private + + def safe_locate(gid) + GlobalID::Locator.locate(gid, only: ALLOWED_UPSTREAM_CLASSES) + rescue StandardError => e + Gitlab::ErrorTracking.track_exception(e, gid: gid, worker: self.class.name) + nil + end end end end diff --git a/ee/app/workers/virtual_registries/container/cache/mark_entries_for_destruction_worker.rb b/ee/app/workers/virtual_registries/container/cache/mark_entries_for_destruction_worker.rb index 5b12cbc180a23c..bb3b775b456659 100644 --- a/ee/app/workers/virtual_registries/container/cache/mark_entries_for_destruction_worker.rb +++ b/ee/app/workers/virtual_registries/container/cache/mark_entries_for_destruction_worker.rb @@ -14,6 +14,8 @@ class MarkEntriesForDestructionWorker deduplicate :until_executed idempotent! + # a no-op worker that should be removed in 18.9 according to + # https://docs.gitlab.com/development/sidekiq/compatibility_across_updates/#removing-worker-classes def perform(upstream_id); end end end diff --git a/ee/app/workers/virtual_registries/packages/cache/mark_entries_for_destruction_worker.rb b/ee/app/workers/virtual_registries/packages/cache/mark_entries_for_destruction_worker.rb index b938c5ae79fa08..b061b9abbcece5 100644 --- a/ee/app/workers/virtual_registries/packages/cache/mark_entries_for_destruction_worker.rb +++ b/ee/app/workers/virtual_registries/packages/cache/mark_entries_for_destruction_worker.rb @@ -14,6 +14,8 @@ class MarkEntriesForDestructionWorker deduplicate :until_executed idempotent! + # a no-op worker that should be removed in 18.9 according to + # https://docs.gitlab.com/development/sidekiq/compatibility_across_updates/#removing-worker-classes def perform(upstream_id); end end end diff --git a/ee/spec/models/virtual_registries/container/upstream_spec.rb b/ee/spec/models/virtual_registries/container/upstream_spec.rb index 7f5efddbc68071..62cc7f8459bdd7 100644 --- a/ee/spec/models/virtual_registries/container/upstream_spec.rb +++ b/ee/spec/models/virtual_registries/container/upstream_spec.rb @@ -723,9 +723,7 @@ def with_accept_headers(headers) end describe '#purge_cache!' do - before do - upstream.save! - end + let(:upstream) { build_stubbed(:virtual_registries_container_upstream) } it 'enqueues the MarkEntriesForDestructionWorker' do expect(::VirtualRegistries::Cache::MarkEntriesForDestructionWorker) diff --git a/ee/spec/models/virtual_registries/packages/maven/upstream_spec.rb b/ee/spec/models/virtual_registries/packages/maven/upstream_spec.rb index cc897836747129..734d050eb92404 100644 --- a/ee/spec/models/virtual_registries/packages/maven/upstream_spec.rb +++ b/ee/spec/models/virtual_registries/packages/maven/upstream_spec.rb @@ -647,9 +647,7 @@ end describe '#purge_cache!' do - before do - upstream.save! - end + let(:upstream) { build_stubbed(:virtual_registries_packages_maven_upstream) } it 'enqueues the MarkEntriesForDestructionWorker' do expect(::VirtualRegistries::Cache::MarkEntriesForDestructionWorker) diff --git a/ee/spec/workers/virtual_registries/cache/mark_entries_for_destruction_worker_spec.rb b/ee/spec/workers/virtual_registries/cache/mark_entries_for_destruction_worker_spec.rb index 77ebf82bd08558..6db49966a3a1db 100644 --- a/ee/spec/workers/virtual_registries/cache/mark_entries_for_destruction_worker_spec.rb +++ b/ee/spec/workers/virtual_registries/cache/mark_entries_for_destruction_worker_spec.rb @@ -2,57 +2,92 @@ require 'spec_helper' -RSpec.describe VirtualRegistries::Cache::MarkEntriesForDestructionWorker, feature_category: :virtual_registry do - let_it_be(:upstream) { create(:virtual_registries_container_upstream) } - +RSpec.describe VirtualRegistries::Cache::MarkEntriesForDestructionWorker, :aggregate_failures, feature_category: :virtual_registry do let(:worker) { described_class.new } subject(:perform) { worker.perform(upstream_gid) } it_behaves_like 'worker with data consistency', described_class, data_consistency: :sticky - it_behaves_like 'an idempotent worker' do - let(:job_args) { [upstream.to_global_id.to_s] } - end it 'has an until_executed deduplicate strategy' do expect(described_class.get_deduplicate_strategy).to eq(:until_executed) end - describe '#perform' do - context 'when the upstream is found' do - let(:upstream_gid) { upstream.to_global_id.to_s } + shared_examples 'marking entries for destruction' do + it_behaves_like 'an idempotent worker' do + let(:job_args) { [upstream.to_global_id.to_s] } + end + + describe '#perform' do + context 'when the upstream is found' do + let(:upstream_gid) { upstream.to_global_id.to_s } + + before do + create_list(cache_entry_factory, 3, upstream:) # 3 default + create(cache_entry_factory, :pending_destruction, upstream:) # 1 pending destruction + # rubocop:disable Rails/SaveBang -- this is a FactoryBot method + create(cache_entry_factory) # 1 default in another upstream + # rubocop:enable Rails/SaveBang + end - before do - create_list(:virtual_registries_container_cache_entry, 3, upstream:) # 3 default - create(:virtual_registries_container_cache_entry, :pending_destruction, upstream:) # 1 pending destruction - create(:virtual_registries_container_cache_entry) # 1 default in another upstream + it 'marks default cache entries for destruction' do + expect { perform }.to change { cache_entry_class.pending_destruction.size }.by(3) + end end - it 'marks default cache entries for destruction' do - expect { perform }.to change { - ::VirtualRegistries::Container::Cache::Entry.pending_destruction.size - }.by(3) + context 'for unsupported class' do + let(:upstream_gid) { upstream.group.to_global_id.to_s } + + it 'does not mark any cache entries for destruction' do + expect { perform }.not_to change { cache_entry_class.pending_destruction.size } + + is_expected.to be_nil + end end - end - context 'when the upstream is not found' do - let(:upstream_gid) { "gid://gitlab/VirtualRegistries::Container::Upstream/#{non_existing_record_id}" } + context 'when the upstream is not found' do + let(:upstream_gid) { "gid://gitlab/#{upstream_class_name}/#{non_existing_record_id}" } + + it 'logs the error and does not mark any cache entries for destruction' do + expect(Gitlab::ErrorTracking).to receive(:track_exception) + .with(instance_of(ActiveRecord::RecordNotFound), gid: upstream_gid, worker: described_class.name) - it 'does not mark any cache entries for destruction' do - expect { perform }.not_to change { ::VirtualRegistries::Container::Cache::Entry.count } + expect { perform }.not_to change { cache_entry_class.pending_destruction.size } - is_expected.to be_nil + is_expected.to be_nil + end end - end - context 'for unsupported class' do - let(:upstream_gid) { upstream.group.to_global_id.to_s } + context 'when the global ID references a non-existent class' do + let(:upstream_gid) { "gid://gitlab/VirtualRegistries::Packages::Maven::NonExistent::Upstream/#{upstream.id}" } + + it 'logs the error and does not mark any cache entries for destruction' do + expect(Gitlab::ErrorTracking).to receive(:track_exception) + .with(instance_of(NameError), gid: upstream_gid, worker: described_class.name) - it 'does not mark any cache entries for destruction' do - expect { perform }.not_to change { ::VirtualRegistries::Container::Cache::Entry.count } + expect { perform }.not_to change { cache_entry_class.pending_destruction.size } - is_expected.to be_nil + is_expected.to be_nil + end end end end + + context 'with a container upstream' do + let_it_be(:upstream) { create(:virtual_registries_container_upstream) } + let(:cache_entry_factory) { :virtual_registries_container_cache_entry } + let(:cache_entry_class) { ::VirtualRegistries::Container::Cache::Entry } + let(:upstream_class_name) { 'VirtualRegistries::Container::Upstream' } + + it_behaves_like 'marking entries for destruction' + end + + context 'with a maven upstream' do + let_it_be(:upstream) { create(:virtual_registries_packages_maven_upstream) } + let(:cache_entry_factory) { :virtual_registries_packages_maven_cache_entry } + let(:cache_entry_class) { ::VirtualRegistries::Packages::Maven::Cache::Entry } + let(:upstream_class_name) { 'VirtualRegistries::Packages::Maven::Upstream' } + + it_behaves_like 'marking entries for destruction' + end end -- GitLab