Fix SQL statement timeout while fetching inactive projects

- Use each_batch instead of find_each to traverse the table using
primary index and prevent statement timeouts.
- Monitor the elapsed time of inactive_projects_deletion_cron_worker
and exit before 5 minutes to avoid breaching the maximum allowed
execution latency for low urgency workers.
- Update inactive_projects_deletion_cron_worker to run at every 10th
minute instead of running daily.

Changelog: performance

app/workers/projects/inactive_projects_deletion_cron_worker.rb

@@ -9,34 +9,53 @@ class InactiveProjectsDeletionCronWorker
     idempotent!
     data_consistency :always
     feature_category :compliance_management
+    urgency :low
 
-    INTERVAL = 2.seconds.to_i
+    # This cron worker is executed at an interval of 10 minutes.
+    # Maximum run time is kept as 4 minutes to avoid breaching maximum allowed execution latency of 5 minutes.
+    MAX_RUN_TIME = 4.minutes
+    LAST_PROCESSED_INACTIVE_PROJECT_REDIS_KEY = 'last_processed_inactive_project_id'
+
+    TimeoutError = Class.new(StandardError)
 
     def perform
       return unless ::Gitlab::CurrentSettings.delete_inactive_projects?
 
+      @start_time = Time.current.utc
       admin_user = User.admins.active.first
 
       return unless admin_user
 
       notified_inactive_projects = Gitlab::InactiveProjectsDeletionWarningTracker.notified_projects
 
-      Project.inactive.without_deleted.find_each(batch_size: 100).with_index do |project, index| # rubocop: disable CodeReuse/ActiveRecord
-        next unless Feature.enabled?(:inactive_projects_deletion, project.root_namespace)
+      project_id = last_processed_project_id
+
+      Project.where('projects.id > ?', project_id).each_batch(of: 100) do |batch| # rubocop: disable CodeReuse/ActiveRecord
+        inactive_projects = batch.inactive.without_deleted
 
-        delay = index * INTERVAL
+        inactive_projects.each do |project|
+          next unless Feature.enabled?(:inactive_projects_deletion, project.root_namespace)
 
-        with_context(project: project, user: admin_user) do
-          deletion_warning_email_sent_on = notified_inactive_projects["project:#{project.id}"]
+          with_context(project: project, user: admin_user) do
+            deletion_warning_email_sent_on = notified_inactive_projects["project:#{project.id}"]
+
+            if send_deletion_warning_email?(deletion_warning_email_sent_on, project)
+              send_notification(project, admin_user)
+            elsif deletion_warning_email_sent_on && delete_due_to_inactivity?(deletion_warning_email_sent_on)
+              Gitlab::InactiveProjectsDeletionWarningTracker.new(project.id).reset
+              delete_project(project, admin_user)
+            end
+          end
 
-          if send_deletion_warning_email?(deletion_warning_email_sent_on, project)
-            send_notification(delay, project, admin_user)
-          elsif deletion_warning_email_sent_on && delete_due_to_inactivity?(deletion_warning_email_sent_on)
-            Gitlab::InactiveProjectsDeletionWarningTracker.new(project.id).reset
-            delete_project(project, admin_user)
+          if over_time?
+            save_last_processed_project_id(project.id)
+            raise TimeoutError
           end
         end
       end
+      reset_last_processed_project_id
+    rescue TimeoutError
+      # no-op
     end
 
     private
@@ -64,8 +83,30 @@ def delete_project(project, user)
       ::Projects::DestroyService.new(project, user, {}).async_execute
     end
 
-    def send_notification(delay, project, user)
-      ::Projects::InactiveProjectsDeletionNotificationWorker.perform_in(delay, project.id, deletion_date)
+    def send_notification(project, user)
+      ::Projects::InactiveProjectsDeletionNotificationWorker.perform_async(project.id, deletion_date)
+    end
+
+    def over_time?
+      (Time.current.utc - @start_time) > MAX_RUN_TIME
+    end
+
+    def save_last_processed_project_id(project_id)
+      Gitlab::Redis::Cache.with do |redis|
+        redis.set(LAST_PROCESSED_INACTIVE_PROJECT_REDIS_KEY, project_id)
+      end
+    end
+
+    def last_processed_project_id
+      Gitlab::Redis::Cache.with do |redis|
+        redis.get(LAST_PROCESSED_INACTIVE_PROJECT_REDIS_KEY).to_i
+      end
+    end
+
+    def reset_last_processed_project_id
+      Gitlab::Redis::Cache.with do |redis|
+        redis.del(LAST_PROCESSED_INACTIVE_PROJECT_REDIS_KEY)
+      end
     end
   end
 end

config/initializers/1_settings.rb

@@ -627,7 +627,7 @@
 Settings.cron_jobs['projects_schedule_refresh_build_artifacts_size_statistics_worker']['cron'] ||= '2/17 * * * *'
 Settings.cron_jobs['projects_schedule_refresh_build_artifacts_size_statistics_worker']['job_class'] = 'Projects::ScheduleRefreshBuildArtifactsSizeStatisticsWorker'
 Settings.cron_jobs['inactive_projects_deletion_cron_worker'] ||= Settingslogic.new({})
-Settings.cron_jobs['inactive_projects_deletion_cron_worker']['cron'] ||= '0 1 * * *'
+Settings.cron_jobs['inactive_projects_deletion_cron_worker']['cron'] ||= '*/10 * * * *'
 Settings.cron_jobs['inactive_projects_deletion_cron_worker']['job_class'] = 'Projects::InactiveProjectsDeletionCronWorker'
 Settings.cron_jobs['loose_foreign_keys_cleanup_worker'] ||= Settingslogic.new({})
 Settings.cron_jobs['loose_foreign_keys_cleanup_worker']['cron'] ||= '*/1 * * * *'

ee/app/workers/ee/projects/inactive_projects_deletion_cron_worker.rb

@@ -23,7 +23,7 @@ def send_deletion_warning_email?(deletion_warning_email_sent_on, project)
       end
 
       override :send_notification
-      def send_notification(delay, project, user)
+      def send_notification(project, user)
         super
 
         ::AuditEventService.new(

ee/spec/workers/ee/projects/inactive_projects_deletion_cron_worker_spec.rb

@@ -47,7 +47,7 @@
     it 'does not send deletion warning email for inactive projects that are already marked for deletion' do
       inactive_large_project.update!(marked_for_deletion_at: Date.current)
 
-      expect(::Projects::InactiveProjectsDeletionNotificationWorker).not_to receive(:perform_in)
+      expect(::Projects::InactiveProjectsDeletionNotificationWorker).not_to receive(:perform_async)
       expect(::Projects::DestroyService).not_to receive(:new)
       expect(::Projects::MarkForDeletionService).not_to receive(:perform_in)
 
@@ -64,8 +64,8 @@
         expect(redis).to receive(:hset).with('inactive_projects_deletion_warning_email_notified',
                                              "project:#{inactive_large_project.id}", Date.current)
       end
-      expect(::Projects::InactiveProjectsDeletionNotificationWorker).to receive(:perform_in).with(
-        delay, inactive_large_project.id, deletion_date).and_call_original
+      expect(::Projects::InactiveProjectsDeletionNotificationWorker).to receive(:perform_async).with(
+        inactive_large_project.id, deletion_date).and_call_original
       expect(::Projects::DestroyService).not_to receive(:new)
 
       expect { worker.perform }
@@ -86,7 +86,7 @@
                      15.months.ago.to_date.to_s)
         end
 
-        expect(::Projects::InactiveProjectsDeletionNotificationWorker).not_to receive(:perform_in)
+        expect(::Projects::InactiveProjectsDeletionNotificationWorker).not_to receive(:perform_async)
         expect(::Projects::MarkForDeletionService).not_to receive(:perform_in)
         expect(::Projects::DestroyService).to receive(:new).with(inactive_large_project, admin_user, {})
                                                            .at_least(:once).and_call_original
@@ -119,7 +119,7 @@
                        15.months.ago.to_date.to_s)
           end
 
-          expect(::Projects::InactiveProjectsDeletionNotificationWorker).not_to receive(:perform_in)
+          expect(::Projects::InactiveProjectsDeletionNotificationWorker).not_to receive(:perform_async)
           expect(::Projects::MarkForDeletionService).not_to receive(:perform_in)
           expect(::Projects::DestroyService).to receive(:new).with(inactive_large_project, admin_user, {})
                                                              .at_least(:once).and_call_original
@@ -147,7 +147,7 @@
                        15.months.ago.to_date.to_s)
           end
 
-          expect(::Projects::InactiveProjectsDeletionNotificationWorker).not_to receive(:perform_in)
+          expect(::Projects::InactiveProjectsDeletionNotificationWorker).not_to receive(:perform_async)
           expect(::Projects::MarkForDeletionService).to receive(:new).with(inactive_large_project, admin_user, {})
                                                                      .and_call_original
 

spec/workers/projects/inactive_projects_deletion_cron_worker_spec.rb

@@ -44,7 +44,7 @@
       end
 
       it 'does not invoke Projects::InactiveProjectsDeletionNotificationWorker' do
-        expect(::Projects::InactiveProjectsDeletionNotificationWorker).not_to receive(:perform_in)
+        expect(::Projects::InactiveProjectsDeletionNotificationWorker).not_to receive(:perform_async)
         expect(::Projects::DestroyService).not_to receive(:new)
 
         worker.perform
@@ -68,7 +68,7 @@
         end
 
         it 'does not invoke Projects::InactiveProjectsDeletionNotificationWorker' do
-          expect(::Projects::InactiveProjectsDeletionNotificationWorker).not_to receive(:perform_in)
+          expect(::Projects::InactiveProjectsDeletionNotificationWorker).not_to receive(:perform_async)
           expect(::Projects::DestroyService).not_to receive(:new)
 
           worker.perform
@@ -82,8 +82,6 @@
       end
 
       context 'when feature flag is enabled', :clean_gitlab_redis_shared_state, :sidekiq_inline do
-        let_it_be(:delay) { anything }
-
         before do
           stub_feature_flags(inactive_projects_deletion: true)
         end
@@ -93,8 +91,8 @@
             expect(redis).to receive(:hset).with('inactive_projects_deletion_warning_email_notified',
                                                  "project:#{inactive_large_project.id}", Date.current)
           end
-          expect(::Projects::InactiveProjectsDeletionNotificationWorker).to receive(:perform_in).with(
-            delay, inactive_large_project.id, deletion_date).and_call_original
+          expect(::Projects::InactiveProjectsDeletionNotificationWorker).to receive(:perform_async).with(
+            inactive_large_project.id, deletion_date).and_call_original
           expect(::Projects::DestroyService).not_to receive(:new)
 
           worker.perform
@@ -106,7 +104,7 @@
                        Date.current.to_s)
           end
 
-          expect(::Projects::InactiveProjectsDeletionNotificationWorker).not_to receive(:perform_in)
+          expect(::Projects::InactiveProjectsDeletionNotificationWorker).not_to receive(:perform_async)
           expect(::Projects::DestroyService).not_to receive(:new)
 
           worker.perform
@@ -118,7 +116,7 @@
                        15.months.ago.to_date.to_s)
           end
 
-          expect(::Projects::InactiveProjectsDeletionNotificationWorker).not_to receive(:perform_in)
+          expect(::Projects::InactiveProjectsDeletionNotificationWorker).not_to receive(:perform_async)
           expect(::Projects::DestroyService).to receive(:new).with(inactive_large_project, admin_user, {})
                                                              .at_least(:once).and_call_original
 
@@ -131,6 +129,35 @@
                               "project:#{inactive_large_project.id}")).to be_nil
           end
         end
+
+        context 'when the worker is running for more than 4 minutes' do
+          before do
+            # only mock the current time once
+            allow(Time).to receive(:current).and_return(5.minutes.ago, Time.current)
+          end
+
+          it 'stores the last processed inactive project_id in redis cache' do
+            Gitlab::Redis::Cache.with do |redis|
+              expect { worker.perform }
+                .to change { redis.get('last_processed_inactive_project_id') }.to(inactive_large_project.id.to_s)
+            end
+          end
+        end
+
+        context 'when the worker finishes processing in less than 4 minutes' do
+          before do
+            Gitlab::Redis::Cache.with do |redis|
+              redis.set('last_processed_inactive_project_id', inactive_large_project.id)
+            end
+          end
+
+          it 'clears the last processed inactive project_id from redis cache' do
+            Gitlab::Redis::Cache.with do |redis|
+              expect { worker.perform }
+                .to change { redis.get('last_processed_inactive_project_id') }.to(nil)
+            end
+          end
+        end
       end
 
       it_behaves_like 'an idempotent worker'