Implement the loopless mode for the cleanup policy worker

app/models/container_repository.rb

@@ -7,6 +7,7 @@ class ContainerRepository < ApplicationRecord
   include Sortable
 
   WAITING_CLEANUP_STATUSES = %i[cleanup_scheduled cleanup_unfinished].freeze
+  REQUIRING_CLEANUP_STATUSES = %i[cleanup_unscheduled cleanup_scheduled].freeze
 
   belongs_to :project
 
@@ -31,6 +32,7 @@ class ContainerRepository < ApplicationRecord
   scope :for_project_id, ->(project_id) { where(project_id: project_id) }
   scope :search_by_name, ->(query) { fuzzy_search(query, [:name], use_minimum_char_limit: false) }
   scope :waiting_for_cleanup, -> { where(expiration_policy_cleanup_status: WAITING_CLEANUP_STATUSES) }
+  scope :expiration_policy_started_at_nil_or_before, ->(timestamp) { where('expiration_policy_started_at < ? OR expiration_policy_started_at IS NULL', timestamp) }
 
   def self.exists_by_path?(path)
     where(
@@ -39,6 +41,23 @@ class ContainerRepository < ApplicationRecord
     ).exists?
   end
 
+  def self.with_enabled_policy
+    joins("INNER JOIN container_expiration_policies ON container_repositories.project_id = container_expiration_policies.project_id")
+      .where(container_expiration_policies: { enabled: true })
+  end
+
+  def self.requiring_cleanup
+    where(
+      container_repositories: { expiration_policy_cleanup_status: REQUIRING_CLEANUP_STATUSES },
+      project_id: ::ContainerExpirationPolicy.runnable_schedules
+                                             .select(:project_id)
+    )
+  end
+
+  def self.with_unfinished_cleanup
+    with_enabled_policy.cleanup_unfinished
+  end
+
   # rubocop: disable CodeReuse/ServiceClass
   def registry
     @registry ||= begin

app/services/container_expiration_policies/cleanup_service.rb

@@ -13,7 +13,14 @@ module ContainerExpirationPolicies
     def execute
       return ServiceResponse.error(message: 'no repository') unless repository
 
+      unless policy.valid?
+        disable_policy!
+
+        return ServiceResponse.error(message: 'invalid policy')
+      end
+
       repository.start_expiration_policy!
+      schedule_next_run_if_needed
 
       begin
         service_result = Projects::ContainerRepository::CleanupTagsService
@@ -28,7 +35,6 @@ module ContainerExpirationPolicies
       if service_result[:status] == :success
         repository.update!(
           expiration_policy_cleanup_status: :cleanup_unscheduled,
-          expiration_policy_started_at: nil,
           expiration_policy_completed_at: Time.zone.now
         )
 
@@ -42,6 +48,27 @@ module ContainerExpirationPolicies
 
     private
 
+    def schedule_next_run_if_needed
+      return unless Feature.enabled?(:container_registry_expiration_policies_loopless)
+      return if policy.next_run_at.future?
+
+      repos_before_next_run = ::ContainerRepository.for_project_id(policy.project_id)
+                                                   .expiration_policy_started_at_nil_or_before(policy.next_run_at)
+      return if repos_before_next_run.exists?
+
+      policy.schedule_next_run!
+    end
+
+    def disable_policy!
+      policy.disable!
+      repository.cleanup_unscheduled!
+
+      Gitlab::ErrorTracking.log_exception(
+        ::ContainerExpirationPolicyWorker::InvalidPolicyError.new,
+        container_expiration_policy_id: policy.id
+      )
+    end
+
     def success(cleanup_status, service_result)
       payload = {
         cleanup_status: cleanup_status,

app/workers/container_expiration_policies/cleanup_container_repository_worker.rb

@@ -24,16 +24,22 @@ module ContainerExpirationPolicies
       cleanup_tags_service_deleted_size
     ].freeze
 
-    delegate :perform_work, :remaining_work_count, to: :inner_instance
+    def perform_work
+      return unless throttling_enabled?
+      return unless container_repository
 
-    def inner_instance
-      strong_memoize(:inner_instance) do
-        if loopless_enabled?
-          Loopless.new(self)
-        else
-          Looping.new(self)
-        end
+      log_extra_metadata_on_done(:container_repository_id, container_repository.id)
+      log_extra_metadata_on_done(:project_id, project.id)
+
+      unless allowed_to_run?
+        container_repository.cleanup_unscheduled!
+        log_extra_metadata_on_done(:cleanup_status, :skipped)
+        return
       end
+
+      result = ContainerExpirationPolicies::CleanupService.new(container_repository)
+                                                          .execute
+      log_on_done(result)
     end
 
     def max_running_jobs
@@ -42,6 +48,97 @@ module ContainerExpirationPolicies
       ::Gitlab::CurrentSettings.container_registry_expiration_policies_worker_capacity
     end
 
+    def remaining_work_count
+      total_count = cleanup_scheduled_count + cleanup_unfinished_count
+
+      log_info(
+        cleanup_scheduled_count: cleanup_scheduled_count,
+        cleanup_unfinished_count: cleanup_unfinished_count,
+        cleanup_total_count: total_count
+      )
+
+      total_count
+    end
+
+    private
+
+    def container_repository
+      strong_memoize(:container_repository) do
+        ContainerRepository.transaction do
+          # rubocop: disable CodeReuse/ActiveRecord
+          # We need a lock to prevent two workers from picking up the same row
+          container_repository = if loopless_enabled?
+                                   next_container_repository
+                                 else
+                                   ContainerRepository.waiting_for_cleanup
+                                                      .order(:expiration_policy_cleanup_status, :expiration_policy_started_at)
+                                                      .limit(1)
+                                                      .lock('FOR UPDATE SKIP LOCKED')
+                                                      .first
+                                 end
+
+          # rubocop: enable CodeReuse/ActiveRecord
+          container_repository&.tap(&:cleanup_ongoing!)
+        end
+      end
+    end
+
+    def next_container_repository
+      # rubocop: disable CodeReuse/ActiveRecord
+      next_one_requiring = ContainerRepository.requiring_cleanup
+                                              .order(:expiration_policy_cleanup_status, :expiration_policy_started_at)
+                                              .limit(1)
+                                              .lock('FOR UPDATE SKIP LOCKED')
+                                              .first
+      return next_one_requiring if next_one_requiring
+
+      ContainerRepository.with_unfinished_cleanup
+                         .order(:expiration_policy_started_at)
+                         .limit(1)
+                         .lock('FOR UPDATE SKIP LOCKED')
+                         .first
+      # rubocop: enable CodeReuse/ActiveRecord
+    end
+
+    def cleanup_scheduled_count
+      strong_memoize(:cleanup_scheduled_count) do
+        if loopless_enabled?
+          limit = max_running_jobs + 1
+          ContainerExpirationPolicy.with_container_repositories
+                                   .runnable_schedules
+                                   .limit(limit)
+                                   .count
+        else
+          ContainerRepository.cleanup_scheduled.count
+        end
+      end
+    end
+
+    def cleanup_unfinished_count
+      strong_memoize(:cleanup_unfinished_count) do
+        if loopless_enabled?
+          limit = max_running_jobs + 1
+          ContainerRepository.with_unfinished_cleanup
+                             .limit(limit)
+                             .count
+        else
+          ContainerRepository.cleanup_unfinished.count
+        end
+      end
+    end
+
+    def allowed_to_run?
+      return false unless policy&.enabled && policy&.next_run_at
+
+      now = Time.zone.now
+
+      if loopless_enabled?
+        policy.next_run_at < now || (now + max_cleanup_execution_time.seconds < policy.next_run_at)
+      else
+        now + max_cleanup_execution_time.seconds < policy.next_run_at
+      end
+    end
+
     def throttling_enabled?
       Feature.enabled?(:container_registry_expiration_policies_throttling)
     end
@@ -59,6 +156,11 @@ module ContainerExpirationPolicies
     end
 
     def log_on_done(result)
+      if result.error?
+        log_extra_metadata_on_done(:cleanup_status, :error)
+        log_extra_metadata_on_done(:cleanup_error_message, result.message)
+      end
+
       LOG_ON_DONE_FIELDS.each do |field|
         value = result.payload[field]
 
@@ -76,75 +178,6 @@ module ContainerExpirationPolicies
       log_extra_metadata_on_done(:running_jobs_count, running_jobs_count)
     end
 
-    # rubocop: disable Scalability/IdempotentWorker
-    # TODO: move the logic from this class to the parent one when container_registry_expiration_policies_loopless is removed
-    # Tracking issue: https://gitlab.com/gitlab-org/gitlab/-/issues/325273
-    class Loopless
-      # TODO fill the logic here with the approach documented in
-      # https://gitlab.com/gitlab-org/gitlab/-/issues/267546#limited-worker
-      def initialize(parent)
-        @parent = parent
-      end
-    end
-    # rubocop: enable Scalability/IdempotentWorker
-
-    # rubocop: disable Scalability/IdempotentWorker
-    # TODO remove this class when `container_registry_expiration_policies_loopless` is removed
-    # Tracking issue: https://gitlab.com/gitlab-org/gitlab/-/issues/325273
-    class Looping
-      include Gitlab::Utils::StrongMemoize
-
-      delegate :throttling_enabled?,
-               :log_extra_metadata_on_done,
-               :log_info,
-               :log_on_done,
-               :max_cleanup_execution_time,
-               to: :@parent
-
-      def initialize(parent)
-        @parent = parent
-      end
-
-      def perform_work
-        return unless throttling_enabled?
-        return unless container_repository
-
-        log_extra_metadata_on_done(:container_repository_id, container_repository.id)
-        log_extra_metadata_on_done(:project_id, project.id)
-
-        unless allowed_to_run?(container_repository)
-          container_repository.cleanup_unscheduled!
-          log_extra_metadata_on_done(:cleanup_status, :skipped)
-          return
-        end
-
-        result = ContainerExpirationPolicies::CleanupService.new(container_repository)
-                                                            .execute
-        log_on_done(result)
-      end
-
-      def remaining_work_count
-        cleanup_scheduled_count = ContainerRepository.cleanup_scheduled.count
-        cleanup_unfinished_count = ContainerRepository.cleanup_unfinished.count
-        total_count = cleanup_scheduled_count + cleanup_unfinished_count
-
-        log_info(
-          cleanup_scheduled_count: cleanup_scheduled_count,
-          cleanup_unfinished_count: cleanup_unfinished_count,
-          cleanup_total_count: total_count
-        )
-
-        total_count
-      end
-
-      private
-
-      def allowed_to_run?(container_repository)
-        return false unless policy&.enabled && policy&.next_run_at
-
-        Time.zone.now + max_cleanup_execution_time.seconds < policy.next_run_at
-      end
-
     def policy
       project.container_expiration_policy
     end
@@ -152,23 +185,5 @@ module ContainerExpirationPolicies
     def project
       container_repository.project
     end
-
-      def container_repository
-        strong_memoize(:container_repository) do
-          ContainerRepository.transaction do
-            # rubocop: disable CodeReuse/ActiveRecord
-            # We need a lock to prevent two workers from picking up the same row
-            container_repository = ContainerRepository.waiting_for_cleanup
-                                                      .order(:expiration_policy_cleanup_status, :expiration_policy_started_at)
-                                                      .limit(1)
-                                                      .lock('FOR UPDATE SKIP LOCKED')
-                                                      .first
-            # rubocop: enable CodeReuse/ActiveRecord
-            container_repository&.tap(&:cleanup_ongoing!)
-          end
-        end
-      end
-    end
-    # rubocop: enable Scalability/IdempotentWorker
   end
 end

changelogs/unreleased/267546-limited-worker-loopless-inner-class.yml

+---
+title: Add indexes for cleanup policies on container_repositories and container_expiration_policies
+merge_request: 58123
+author:
+type: added

db/migrate/20210422142647_add_project_id_next_run_at_index_to_container_expiration_policies.rb

+# frozen_string_literal: true
+
+class AddProjectIdNextRunAtIndexToContainerExpirationPolicies < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  disable_ddl_transaction!
+
+  INDEX_NAME = 'idx_container_exp_policies_on_project_id_next_run_at'
+
+  def up
+    add_concurrent_index :container_expiration_policies, [:project_id, :next_run_at], name: INDEX_NAME, where: 'enabled = true'
+  end
+
+  def down
+    remove_concurrent_index :container_expiration_policies, [:project_id, :next_run_at], name: INDEX_NAME
+  end
+end

db/schema_migrations/20210422142647

+eaefc2a0f08ce312b1ae3fb100e4a818eb3013b95c38d940371a25b605b09ca1
\ No newline at end of file

db/structure.sql

@@ -21991,6 +21991,8 @@ CREATE INDEX idx_award_emoji_on_user_emoji_name_awardable_type_awardable_id ON a
 
 CREATE INDEX idx_ci_pipelines_artifacts_locked ON ci_pipelines USING btree (ci_ref_id, id) WHERE (locked = 1);
 
+CREATE INDEX idx_container_exp_policies_on_project_id_next_run_at ON container_expiration_policies USING btree (project_id, next_run_at) WHERE (enabled = true);
+
 CREATE INDEX idx_container_exp_policies_on_project_id_next_run_at_enabled ON container_expiration_policies USING btree (project_id, next_run_at, enabled);
 
 CREATE INDEX idx_container_repositories_on_exp_cleanup_status_and_start_date ON container_repositories USING btree (expiration_policy_cleanup_status, expiration_policy_started_at);

spec/models/container_repository_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 RSpec.describe ContainerRepository do
+  using RSpec::Parameterized::TableSyntax
+
   let(:group) { create(:group, name: 'group') }
   let(:project) { create(:project, path: 'test', group: group) }
 
@@ -29,18 +31,6 @@ RSpec.describe ContainerRepository do
     end
   end
 
-  describe '.exists_by_path?' do
-    it 'returns true for known container repository paths' do
-      path = ContainerRegistry::Path.new("#{project.full_path}/#{repository.name}")
-      expect(described_class.exists_by_path?(path)).to be_truthy
-    end
-
-    it 'returns false for unknown container repository paths' do
-      path = ContainerRegistry::Path.new('you/dont/know/me')
-      expect(described_class.exists_by_path?(path)).to be_falsey
-    end
-  end
-
   describe '#tag' do
     it 'has a test tag' do
       expect(repository.tag('test')).not_to be_nil
@@ -359,6 +349,17 @@ RSpec.describe ContainerRepository do
     it { is_expected.to contain_exactly(repository) }
   end
 
+  describe '.expiration_policy_started_at_nil_or_before' do
+    let_it_be(:repository1) { create(:container_repository, expiration_policy_started_at: nil) }
+    let_it_be(:repository2) { create(:container_repository, expiration_policy_started_at: 1.day.ago) }
+    let_it_be(:repository3) { create(:container_repository, expiration_policy_started_at: 2.hours.ago) }
+    let_it_be(:repository4) { create(:container_repository, expiration_policy_started_at: 1.week.ago) }
+
+    subject { described_class.expiration_policy_started_at_nil_or_before(3.hours.ago) }
+
+    it { is_expected.to contain_exactly(repository1, repository2, repository4) }
+  end
+
   describe '.waiting_for_cleanup' do
     let_it_be(:repository_cleanup_scheduled) { create(:container_repository, :cleanup_scheduled) }
     let_it_be(:repository_cleanup_unfinished) { create(:container_repository, :cleanup_unfinished) }
@@ -368,4 +369,74 @@ RSpec.describe ContainerRepository do
 
     it { is_expected.to contain_exactly(repository_cleanup_scheduled, repository_cleanup_unfinished) }
   end
+
+  describe '.exists_by_path?' do
+    it 'returns true for known container repository paths' do
+      path = ContainerRegistry::Path.new("#{project.full_path}/#{repository.name}")
+      expect(described_class.exists_by_path?(path)).to be_truthy
+    end
+
+    it 'returns false for unknown container repository paths' do
+      path = ContainerRegistry::Path.new('you/dont/know/me')
+      expect(described_class.exists_by_path?(path)).to be_falsey
+    end
+  end
+
+  describe '.with_enabled_policy' do
+    let_it_be(:repository) { create(:container_repository) }
+    let_it_be(:repository2) { create(:container_repository) }
+
+    subject { described_class.with_enabled_policy }
+
+    before do
+      repository.project.container_expiration_policy.update!(enabled: true)
+    end
+
+    it { is_expected.to eq([repository]) }
+  end
+
+  context 'with repositories' do
+    let_it_be_with_reload(:repository) { create(:container_repository, :cleanup_unscheduled) }
+    let_it_be(:other_repository) { create(:container_repository, :cleanup_unscheduled) }
+
+    let(:policy) { repository.project.container_expiration_policy }
+
+    before do
+      ContainerExpirationPolicy.update_all(enabled: true)
+    end
+
+    describe '.requiring_cleanup' do
+      subject { described_class.requiring_cleanup }
+
+      context 'with next_run_at in the future' do
+        before do
+          policy.update_column(:next_run_at, 10.minutes.from_now)
+        end
+
+        it { is_expected.to eq([]) }
+      end
+
+      context 'with next_run_at in the past' do
+        before do
+          policy.update_column(:next_run_at, 10.minutes.ago)
+        end
+
+        it { is_expected.to eq([repository]) }
+      end
+    end
+
+    describe '.with_unfinished_cleanup' do
+      subject { described_class.with_unfinished_cleanup }
+
+      it { is_expected.to eq([]) }
+
+      context 'with an unfinished repository' do
+        before do
+          repository.cleanup_unfinished!
+        end
+
+        it { is_expected.to eq([repository]) }
+      end
+    end
+  end
 end

spec/services/container_expiration_policies/cleanup_service_spec.rb

@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 RSpec.describe ContainerExpirationPolicies::CleanupService do
-  let_it_be(:repository, reload: true) { create(:container_repository) }
+  let_it_be(:repository, reload: true) { create(:container_repository, expiration_policy_started_at: 30.minutes.ago) }
   let_it_be(:project) { repository.project }
 
   let(:service) { described_class.new(repository) }
@@ -11,6 +11,7 @@ RSpec.describe ContainerExpirationPolicies::CleanupService do
   describe '#execute' do
     subject { service.execute }
 
+    shared_examples 'cleaning up a container repository' do
       context 'with a successful cleanup tags service execution' do
         let(:cleanup_tags_service_params) { project.container_expiration_policy.policy_params.merge('container_expiration_policy' => true) }
         let(:cleanup_tags_service) { instance_double(Projects::ContainerRepository::CleanupTagsService) }
@@ -27,8 +28,8 @@ RSpec.describe ContainerExpirationPolicies::CleanupService do
             expect(response.payload).to include(cleanup_status: :finished, container_repository_id: repository.id)
             expect(ContainerRepository.waiting_for_cleanup.count).to eq(0)
             expect(repository.reload.cleanup_unscheduled?).to be_truthy
-          expect(repository.expiration_policy_started_at).to eq(nil)
             expect(repository.expiration_policy_completed_at).not_to eq(nil)
+            expect(repository.expiration_policy_started_at).not_to eq(nil)
           end
         end
       end
@@ -94,9 +95,25 @@ RSpec.describe ContainerExpirationPolicies::CleanupService do
         let(:service) { described_class.new(nil) }
 
         it 'returns an error response' do
-        response = subject
+          expect(subject.success?).to eq(false)
+          expect(subject.message).to eq('no repository')
+        end
+      end
 
-        expect(response.success?).to eq(false)
+      context 'with an invalid policy' do
+        let(:policy) { repository.project.container_expiration_policy }
+
+        before do
+          policy.name_regex = nil
+          policy.enabled = true
+          repository.expiration_policy_cleanup_status = :cleanup_ongoing
+        end
+
+        it 'returns an error response' do
+          expect { subject }.to change { repository.expiration_policy_cleanup_status }.from('cleanup_ongoing').to('cleanup_unscheduled')
+          expect(subject.success?).to eq(false)
+          expect(subject.message).to eq('invalid policy')
+          expect(policy).not_to be_enabled
         end
       end
 
@@ -116,4 +133,93 @@ RSpec.describe ContainerExpirationPolicies::CleanupService do
         end
       end
     end
+
+    context 'with loopless enabled' do
+      let(:policy) { repository.project.container_expiration_policy }
+
+      before do
+        policy.update!(enabled: true)
+        policy.update_column(:next_run_at, 5.minutes.ago)
+      end
+
+      it_behaves_like 'cleaning up a container repository'
+
+      context 'next run scheduling' do
+        let_it_be_with_reload(:repository2) { create(:container_repository, project: project) }
+        let_it_be_with_reload(:repository3) { create(:container_repository, project: project) }
+
+        before do
+          cleanup_tags_service = instance_double(Projects::ContainerRepository::CleanupTagsService)
+          allow(Projects::ContainerRepository::CleanupTagsService)
+            .to receive(:new).and_return(cleanup_tags_service)
+          allow(cleanup_tags_service).to receive(:execute).and_return(status: :success)
+        end
+
+        shared_examples 'not scheduling the next run' do
+          it 'does not scheduled the next run' do
+            expect(policy).not_to receive(:schedule_next_run!)
+
+            expect { subject }.not_to change { policy.reload.next_run_at }
+          end
+        end
+
+        shared_examples 'scheduling the next run' do
+          it 'schedules the next run' do
+            expect(policy).to receive(:schedule_next_run!).and_call_original
+
+            expect { subject }.to change { policy.reload.next_run_at }
+          end
+        end
+
+        context 'with cleanups started_at before policy next_run_at' do
+          before do
+            ContainerRepository.update_all(expiration_policy_started_at: 10.minutes.ago)
+          end
+
+          it_behaves_like 'not scheduling the next run'
+        end
+
+        context 'with cleanups started_at around policy next_run_at' do
+          before do
+            repository3.update!(expiration_policy_started_at: policy.next_run_at + 10.minutes.ago)
+          end
+
+          it_behaves_like 'not scheduling the next run'
+        end
+
+        context 'with only the current repository started_at before the policy next_run_at' do
+          before do
+            repository2.update!(expiration_policy_started_at: policy.next_run_at + 10.minutes)
+            repository3.update!(expiration_policy_started_at: policy.next_run_at + 12.minutes)
+          end
+
+          it_behaves_like 'scheduling the next run'
+        end
+
+        context 'with cleanups started_at after policy next_run_at' do
+          before do
+            ContainerRepository.update_all(expiration_policy_started_at: policy.next_run_at + 10.minutes)
+          end
+
+          it_behaves_like 'scheduling the next run'
+        end
+
+        context 'with a future policy next_run_at' do
+          before do
+            policy.update_column(:next_run_at, 5.minutes.from_now)
+          end
+
+          it_behaves_like 'not scheduling the next run'
+        end
+      end
+    end
+
+    context 'with loopless disabled' do
+      before do
+        stub_feature_flags(container_registry_expiration_policies_loopless: false)
+      end
+
+      it_behaves_like 'cleaning up a container repository'
+    end
+  end
 end

spec/workers/container_expiration_policies/cleanup_container_repository_worker_spec.rb

@@ -5,11 +5,11 @@ require 'spec_helper'
 RSpec.describe ContainerExpirationPolicies::CleanupContainerRepositoryWorker do
   using RSpec::Parameterized::TableSyntax
 
-  let_it_be(:repository, refind: true) { create(:container_repository, :cleanup_scheduled) }
-  let_it_be(:project) { repository.project }
-  let_it_be(:policy) { project.container_expiration_policy }
-  let_it_be(:other_repository) { create(:container_repository) }
+  let_it_be(:repository, refind: true) { create(:container_repository, :cleanup_scheduled, expiration_policy_started_at: 1.month.ago) }
+  let_it_be(:other_repository, refind: true) { create(:container_repository, expiration_policy_started_at: 15.days.ago) }
 
+  let(:project) { repository.project }
+  let(:policy) { project.container_expiration_policy }
   let(:worker) { described_class.new }
 
   describe '#perform_work' do
@@ -19,7 +19,7 @@ RSpec.describe ContainerExpirationPolicies::CleanupContainerRepositoryWorker do
       policy.update_column(:enabled, true)
     end
 
-    RSpec.shared_examples 'handling all repository conditions' do
+    shared_examples 'handling all repository conditions' do
       it 'sends the repository for cleaning' do
         service_response = cleanup_service_response(repository: repository)
         expect(ContainerExpirationPolicies::CleanupService)
@@ -72,11 +72,21 @@ RSpec.describe ContainerExpirationPolicies::CleanupContainerRepositoryWorker do
         end
       end
 
+      context 'with an erroneous cleanup' do
+        it 'logs an error' do
+          service_response = ServiceResponse.error(message: 'cleanup in an error')
+          expect(ContainerExpirationPolicies::CleanupService)
+            .to receive(:new).with(repository).and_return(double(execute: service_response))
+          expect_log_extra_metadata(service_response: service_response, cleanup_status: :error)
+
+          subject
+        end
+      end
+
       context 'with policy running shortly' do
         before do
-          repository.project
-                    .container_expiration_policy
-                    .update_column(:next_run_at, 1.minute.from_now)
+          repository.cleanup_unfinished! if loopless_enabled?
+          policy.update_column(:next_run_at, 1.minute.from_now)
         end
 
         it 'skips the repository' do
@@ -84,27 +94,288 @@ RSpec.describe ContainerExpirationPolicies::CleanupContainerRepositoryWorker do
           expect(worker).to receive(:log_extra_metadata_on_done).with(:container_repository_id, repository.id)
           expect(worker).to receive(:log_extra_metadata_on_done).with(:project_id, repository.project.id)
           expect(worker).to receive(:log_extra_metadata_on_done).with(:cleanup_status, :skipped)
-
           expect { subject }.to change { ContainerRepository.waiting_for_cleanup.count }.from(1).to(0)
+
           expect(repository.reload.cleanup_unscheduled?).to be_truthy
         end
       end
 
       context 'with disabled policy' do
         before do
-          repository.project
-                    .container_expiration_policy
-                    .disable!
+          policy.disable!
         end
 
         it 'skips the repository' do
           expect(ContainerExpirationPolicies::CleanupService).not_to receive(:new)
 
+          if loopless_enabled?
+            expect { subject }
+              .to not_change { ContainerRepository.waiting_for_cleanup.count }
+              .and not_change { repository.reload.expiration_policy_cleanup_status }
+          else
             expect { subject }.to change { ContainerRepository.waiting_for_cleanup.count }.from(1).to(0)
             expect(repository.reload.cleanup_unscheduled?).to be_truthy
           end
         end
       end
+    end
+
+    context 'with loopless enabled' do
+      before do
+        stub_feature_flags(container_registry_expiration_policies_loopless: true)
+      end
+
+      context 'with repository in cleanup unscheduled state' do
+        before do
+          policy.update_column(:next_run_at, 5.minutes.ago)
+        end
+
+        it_behaves_like 'handling all repository conditions'
+      end
+
+      context 'with repository in cleanup unfinished state' do
+        before do
+          repository.cleanup_unfinished!
+        end
+
+        it_behaves_like 'handling all repository conditions'
+      end
+
+      context 'container repository selection' do
+        where(:repository_cleanup_status, :repository_policy_status, :other_repository_cleanup_status, :other_repository_policy_status, :expected_selected_repository) do
+          :unscheduled | :disabled     | :unscheduled | :disabled     | :none
+          :unscheduled | :disabled     | :unscheduled | :runnable     | :other_repository
+          :unscheduled | :disabled     | :unscheduled | :not_runnable | :none
+
+          :unscheduled | :disabled     | :scheduled   | :disabled     | :none
+          :unscheduled | :disabled     | :scheduled   | :runnable     | :other_repository
+          :unscheduled | :disabled     | :scheduled   | :not_runnable | :none
+
+          :unscheduled | :disabled     | :unfinished  | :disabled     | :none
+          :unscheduled | :disabled     | :unfinished  | :runnable     | :other_repository
+          :unscheduled | :disabled     | :unfinished  | :not_runnable | :other_repository
+
+          :unscheduled | :disabled     | :ongoing     | :disabled     | :none
+          :unscheduled | :disabled     | :ongoing     | :runnable     | :none
+          :unscheduled | :disabled     | :ongoing     | :not_runnable | :none
+
+          :unscheduled | :runnable     | :unscheduled | :disabled     | :repository
+          :unscheduled | :runnable     | :unscheduled | :runnable     | :repository
+          :unscheduled | :runnable     | :unscheduled | :not_runnable | :repository
+
+          :unscheduled | :runnable     | :scheduled   | :disabled     | :repository
+          :unscheduled | :runnable     | :scheduled   | :runnable     | :repository
+          :unscheduled | :runnable     | :scheduled   | :not_runnable | :repository
+
+          :unscheduled | :runnable     | :unfinished  | :disabled     | :repository
+          :unscheduled | :runnable     | :unfinished  | :runnable     | :repository
+          :unscheduled | :runnable     | :unfinished  | :not_runnable | :repository
+
+          :unscheduled | :runnable     | :ongoing     | :disabled     | :repository
+          :unscheduled | :runnable     | :ongoing     | :runnable     | :repository
+          :unscheduled | :runnable     | :ongoing     | :not_runnable | :repository
+
+          :scheduled   | :disabled     | :unscheduled | :disabled     | :none
+          :scheduled   | :disabled     | :unscheduled | :runnable     | :other_repository
+          :scheduled   | :disabled     | :unscheduled | :not_runnable | :none
+
+          :scheduled   | :disabled     | :scheduled   | :disabled     | :none
+          :scheduled   | :disabled     | :scheduled   | :runnable     | :other_repository
+          :scheduled   | :disabled     | :scheduled   | :not_runnable | :none
+
+          :scheduled   | :disabled     | :unfinished  | :disabled     | :none
+          :scheduled   | :disabled     | :unfinished  | :runnable     | :other_repository
+          :scheduled   | :disabled     | :unfinished  | :not_runnable | :other_repository
+
+          :scheduled   | :disabled     | :ongoing     | :disabled     | :none
+          :scheduled   | :disabled     | :ongoing     | :runnable     | :none
+          :scheduled   | :disabled     | :ongoing     | :not_runnable | :none
+
+          :scheduled   | :runnable     | :unscheduled | :disabled     | :repository
+          :scheduled   | :runnable     | :unscheduled | :runnable     | :other_repository
+          :scheduled   | :runnable     | :unscheduled | :not_runnable | :repository
+
+          :scheduled   | :runnable     | :scheduled   | :disabled     | :repository
+          :scheduled   | :runnable     | :scheduled   | :runnable     | :repository
+          :scheduled   | :runnable     | :scheduled   | :not_runnable | :repository
+
+          :scheduled   | :runnable     | :unfinished  | :disabled     | :repository
+          :scheduled   | :runnable     | :unfinished  | :runnable     | :repository
+          :scheduled   | :runnable     | :unfinished  | :not_runnable | :repository
+
+          :scheduled   | :runnable     | :ongoing     | :disabled     | :repository
+          :scheduled   | :runnable     | :ongoing     | :runnable     | :repository
+          :scheduled   | :runnable     | :ongoing     | :not_runnable | :repository
+
+          :scheduled   | :not_runnable | :unscheduled | :disabled     | :none
+          :scheduled   | :not_runnable | :unscheduled | :runnable     | :other_repository
+          :scheduled   | :not_runnable | :unscheduled | :not_runnable | :none
+
+          :scheduled   | :not_runnable | :scheduled   | :disabled     | :none
+          :scheduled   | :not_runnable | :scheduled   | :runnable     | :other_repository
+          :scheduled   | :not_runnable | :scheduled   | :not_runnable | :none
+
+          :scheduled   | :not_runnable | :unfinished  | :disabled     | :none
+          :scheduled   | :not_runnable | :unfinished  | :runnable     | :other_repository
+          :scheduled   | :not_runnable | :unfinished  | :not_runnable | :other_repository
+
+          :scheduled   | :not_runnable | :ongoing     | :disabled     | :none
+          :scheduled   | :not_runnable | :ongoing     | :runnable     | :none
+          :scheduled   | :not_runnable | :ongoing     | :not_runnable | :none
+
+          :unfinished  | :disabled     | :unscheduled | :disabled     | :none
+          :unfinished  | :disabled     | :unscheduled | :runnable     | :other_repository
+          :unfinished  | :disabled     | :unscheduled | :not_runnable | :none
+
+          :unfinished  | :disabled     | :scheduled   | :disabled     | :none
+          :unfinished  | :disabled     | :scheduled   | :runnable     | :other_repository
+          :unfinished  | :disabled     | :scheduled   | :not_runnable | :none
+
+          :unfinished  | :disabled     | :unfinished  | :disabled     | :none
+          :unfinished  | :disabled     | :unfinished  | :runnable     | :other_repository
+          :unfinished  | :disabled     | :unfinished  | :not_runnable | :other_repository
+
+          :unfinished  | :disabled     | :ongoing     | :disabled     | :none
+          :unfinished  | :disabled     | :ongoing     | :runnable     | :none
+          :unfinished  | :disabled     | :ongoing     | :not_runnable | :none
+
+          :unfinished  | :runnable     | :unscheduled | :disabled     | :repository
+          :unfinished  | :runnable     | :unscheduled | :runnable     | :other_repository
+          :unfinished  | :runnable     | :unscheduled | :not_runnable | :repository
+
+          :unfinished  | :runnable     | :scheduled   | :disabled     | :repository
+          :unfinished  | :runnable     | :scheduled   | :runnable     | :other_repository
+          :unfinished  | :runnable     | :scheduled   | :not_runnable | :repository
+
+          :unfinished  | :runnable     | :unfinished  | :disabled     | :repository
+          :unfinished  | :runnable     | :unfinished  | :runnable     | :repository
+          :unfinished  | :runnable     | :unfinished  | :not_runnable | :repository
+
+          :unfinished  | :runnable     | :ongoing     | :disabled     | :repository
+          :unfinished  | :runnable     | :ongoing     | :runnable     | :repository
+          :unfinished  | :runnable     | :ongoing     | :not_runnable | :repository
+
+          :unfinished  | :not_runnable | :unscheduled | :disabled     | :repository
+          :unfinished  | :not_runnable | :unscheduled | :runnable     | :other_repository
+          :unfinished  | :not_runnable | :unscheduled | :not_runnable | :repository
+
+          :unfinished  | :not_runnable | :scheduled   | :disabled     | :repository
+          :unfinished  | :not_runnable | :scheduled   | :runnable     | :other_repository
+          :unfinished  | :not_runnable | :scheduled   | :not_runnable | :repository
+
+          :unfinished  | :not_runnable | :unfinished  | :disabled     | :repository
+          :unfinished  | :not_runnable | :unfinished  | :runnable     | :repository
+          :unfinished  | :not_runnable | :unfinished  | :not_runnable | :repository
+
+          :unfinished  | :not_runnable | :ongoing     | :disabled     | :repository
+          :unfinished  | :not_runnable | :ongoing     | :runnable     | :repository
+          :unfinished  | :not_runnable | :ongoing     | :not_runnable | :repository
+
+          :ongoing     | :disabled     | :unscheduled | :disabled     | :none
+          :ongoing     | :disabled     | :unscheduled | :runnable     | :other_repository
+          :ongoing     | :disabled     | :unscheduled | :not_runnable | :none
+
+          :ongoing     | :disabled     | :scheduled   | :disabled     | :none
+          :ongoing     | :disabled     | :scheduled   | :runnable     | :other_repository
+          :ongoing     | :disabled     | :scheduled   | :not_runnable | :none
+
+          :ongoing     | :disabled     | :unfinished  | :disabled     | :none
+          :ongoing     | :disabled     | :unfinished  | :runnable     | :other_repository
+          :ongoing     | :disabled     | :unfinished  | :not_runnable | :other_repository
+
+          :ongoing     | :disabled     | :ongoing     | :disabled     | :none
+          :ongoing     | :disabled     | :ongoing     | :runnable     | :none
+          :ongoing     | :disabled     | :ongoing     | :not_runnable | :none
+
+          :ongoing     | :runnable     | :unscheduled | :disabled     | :none
+          :ongoing     | :runnable     | :unscheduled | :runnable     | :other_repository
+          :ongoing     | :runnable     | :unscheduled | :not_runnable | :none
+
+          :ongoing     | :runnable     | :scheduled   | :disabled     | :none
+          :ongoing     | :runnable     | :scheduled   | :runnable     | :other_repository
+          :ongoing     | :runnable     | :scheduled   | :not_runnable | :none
+
+          :ongoing     | :runnable     | :unfinished  | :disabled     | :none
+          :ongoing     | :runnable     | :unfinished  | :runnable     | :other_repository
+          :ongoing     | :runnable     | :unfinished  | :not_runnable | :other_repository
+
+          :ongoing     | :runnable     | :ongoing     | :disabled     | :none
+          :ongoing     | :runnable     | :ongoing     | :runnable     | :none
+          :ongoing     | :runnable     | :ongoing     | :not_runnable | :none
+
+          :ongoing     | :not_runnable | :unscheduled | :disabled     | :none
+          :ongoing     | :not_runnable | :unscheduled | :runnable     | :other_repository
+          :ongoing     | :not_runnable | :unscheduled | :not_runnable | :none
+
+          :ongoing     | :not_runnable | :scheduled   | :disabled     | :none
+          :ongoing     | :not_runnable | :scheduled   | :runnable     | :other_repository
+          :ongoing     | :not_runnable | :scheduled   | :not_runnable | :none
+
+          :ongoing     | :not_runnable | :unfinished  | :disabled     | :none
+          :ongoing     | :not_runnable | :unfinished  | :runnable     | :other_repository
+          :ongoing     | :not_runnable | :unfinished  | :not_runnable | :other_repository
+
+          :ongoing     | :not_runnable | :ongoing     | :disabled     | :none
+          :ongoing     | :not_runnable | :ongoing     | :runnable     | :none
+          :ongoing     | :not_runnable | :ongoing     | :not_runnable | :none
+        end
+
+        with_them do
+          before do
+            update_container_repository(repository, repository_cleanup_status, repository_policy_status)
+            update_container_repository(other_repository, other_repository_cleanup_status, other_repository_policy_status)
+          end
+
+          subject { worker.send(:container_repository) }
+
+          if params[:expected_selected_repository] == :none
+            it 'does not select any repository' do
+              expect(subject).to eq(nil)
+            end
+          else
+            it 'does select a repository' do
+              selected_repository = expected_selected_repository == :repository ? repository : other_repository
+
+              expect(subject).to eq(selected_repository)
+            end
+          end
+
+          def update_container_repository(container_repository, cleanup_status, policy_status)
+            container_repository.update_column(:expiration_policy_cleanup_status, "cleanup_#{cleanup_status}")
+
+            policy = container_repository.project.container_expiration_policy
+
+            case policy_status
+            when :disabled
+              policy.update!(enabled: false)
+            when :runnable
+              policy.update!(enabled: true)
+              policy.update_column(:next_run_at, 5.minutes.ago)
+            when :not_runnable
+              policy.update!(enabled: true)
+              policy.update_column(:next_run_at, 5.minutes.from_now)
+            end
+          end
+        end
+      end
+
+      context 'with another repository in cleanup unfinished state' do
+        let_it_be(:another_repository) { create(:container_repository, :cleanup_unfinished) }
+
+        before do
+          policy.update_column(:next_run_at, 5.minutes.ago)
+        end
+
+        it 'process the cleanup scheduled repository first' do
+          service_response = cleanup_service_response(repository: repository)
+          expect(ContainerExpirationPolicies::CleanupService)
+            .to receive(:new).with(repository).and_return(double(execute: service_response))
+          expect_log_extra_metadata(service_response: service_response)
+
+          subject
+        end
+      end
+    end
 
     context 'with loopless disabled' do
       before do
@@ -230,17 +501,18 @@ RSpec.describe ContainerExpirationPolicies::CleanupContainerRepositoryWorker do
       end
       expect(worker).to receive(:log_extra_metadata_on_done).with(:cleanup_tags_service_truncated, truncated)
       expect(worker).to receive(:log_extra_metadata_on_done).with(:running_jobs_count, 0)
+
+      if service_response.error?
+        expect(worker).to receive(:log_extra_metadata_on_done).with(:cleanup_error_message, service_response.message)
+      end
     end
   end
 
   describe '#remaining_work_count' do
     subject { worker.remaining_work_count }
 
-    context 'with loopless disabled' do
-      before do
-        stub_feature_flags(container_registry_expiration_policies_loopless: false)
-      end
-      context 'with container repositoires waiting for cleanup' do
+    shared_examples 'handling all conditions' do
+      context 'with container repositories waiting for cleanup' do
         let_it_be(:unfinished_repositories) { create_list(:container_repository, 2, :cleanup_unfinished) }
 
         it { is_expected.to eq(3) }
@@ -259,6 +531,7 @@ RSpec.describe ContainerExpirationPolicies::CleanupContainerRepositoryWorker do
       context 'with no container repositories waiting for cleanup' do
         before do
           repository.cleanup_ongoing!
+          policy.update_column(:next_run_at, 5.minutes.from_now)
         end
 
         it { is_expected.to eq(0) }
@@ -274,15 +547,23 @@ RSpec.describe ContainerExpirationPolicies::CleanupContainerRepositoryWorker do
         end
       end
     end
-  end
 
-  describe '#max_running_jobs' do
-    let(:capacity) { 50 }
+    context 'with loopless enabled' do
+      let_it_be(:disabled_repository) { create(:container_repository, :cleanup_scheduled) }
 
-    subject { worker.max_running_jobs }
+      let(:capacity) { 10 }
 
       before do
+        stub_feature_flags(container_registry_expiration_policies_loopless: true)
         stub_application_setting(container_registry_expiration_policies_worker_capacity: capacity)
+
+        # loopless mode is more accurate that non loopless: policies need to be enabled
+        ContainerExpirationPolicy.update_all(enabled: true)
+        repository.project.container_expiration_policy.update_column(:next_run_at, 5.minutes.ago)
+        disabled_repository.project.container_expiration_policy.update_column(:enabled, false)
+      end
+
+      it_behaves_like 'handling all conditions'
     end
 
     context 'with loopless disabled' do
@@ -290,6 +571,19 @@ RSpec.describe ContainerExpirationPolicies::CleanupContainerRepositoryWorker do
         stub_feature_flags(container_registry_expiration_policies_loopless: false)
       end
 
+      it_behaves_like 'handling all conditions'
+    end
+  end
+
+  describe '#max_running_jobs' do
+    let(:capacity) { 50 }
+
+    subject { worker.max_running_jobs }
+
+    before do
+      stub_application_setting(container_registry_expiration_policies_worker_capacity: capacity)
+    end
+
     it { is_expected.to eq(capacity) }
 
     context 'with feature flag disabled' do
@@ -300,10 +594,13 @@ RSpec.describe ContainerExpirationPolicies::CleanupContainerRepositoryWorker do
       it { is_expected.to eq(0) }
     end
   end
-  end
 
   def expect_log_info(structure)
     expect(worker.logger)
       .to receive(:info).with(worker.structured_payload(structure))
   end
+
+  def loopless_enabled?
+    Feature.enabled?(:container_registry_expiration_policies_loopless)
+  end
 end