From 99252b36025275cb3f0c8e8cc2930d3263bd61ad Mon Sep 17 00:00:00 2001
From: "Ben Sheldon [he/him]" <bensheldon@gmail.com>
Date: Wed, 19 Apr 2023 08:07:16 -0700
Subject: [PATCH] Use batched queries in `GoodJob::self.cleanup_preserved_jobs`
 (#934)

* Use batch queries in `GoodJob::self.cleanup_preserved_jobs`

* Use a `loop` instead of `in_batches`
---
 lib/good_job.rb           | 29 ++++++++++++++++++++---------
 spec/lib/good_job_spec.rb |  2 +-
 2 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/lib/good_job.rb b/lib/good_job.rb
index 1995888c3..14be87d2c 100644
--- a/lib/good_job.rb
+++ b/lib/good_job.rb
@@ -162,22 +162,33 @@ def self._shutdown_all(executables, method_name = :shutdown, timeout: -1)
   # destroy old records and preserve space in your database.
   # @param older_than [nil,Numeric,ActiveSupport::Duration] Jobs older than this will be destroyed (default: +86400+).
   # @return [Integer] Number of job execution records and batches that were destroyed.
-  def self.cleanup_preserved_jobs(older_than: nil)
+  def self.cleanup_preserved_jobs(older_than: nil, in_batches_of: 1_000)
     older_than ||= GoodJob.configuration.cleanup_preserved_jobs_before_seconds_ago
     timestamp = Time.current - older_than
     include_discarded = GoodJob.configuration.cleanup_discarded_jobs?
 
     ActiveSupport::Notifications.instrument("cleanup_preserved_jobs.good_job", { older_than: older_than, timestamp: timestamp }) do |payload|
-      old_jobs = GoodJob::Job.where('finished_at <= ?', timestamp)
-      old_jobs = old_jobs.succeeded unless include_discarded
-      deleted_executions_count = GoodJob::Execution.where(job: old_jobs).delete_all
+      deleted_executions_count = 0
+      deleted_batches_count = 0
+
+      jobs_query = GoodJob::Job.where('finished_at <= ?', timestamp).order(finished_at: :asc).limit(in_batches_of)
+      jobs_query = jobs_query.succeeded unless include_discarded
+      loop do
+        deleted = GoodJob::Execution.where(job: jobs_query).delete_all
+        break if deleted.zero?
+
+        deleted_executions_count += deleted
+      end
 
       if GoodJob::BatchRecord.migrated?
-        old_batches = GoodJob::BatchRecord.where('finished_at <= ?', timestamp)
-        old_batches = old_batches.succeeded unless include_discarded
-        deleted_batches_count = old_batches.delete_all
-      else
-        deleted_batches_count = 0
+        batches_query = GoodJob::BatchRecord.where('finished_at <= ?', timestamp).limit(in_batches_of)
+        batches_query = batches_query.succeeded unless include_discarded
+        loop do
+          deleted = batches_query.delete_all
+          break if deleted.zero?
+
+          deleted_batches_count += deleted
+        end
       end
 
       payload[:destroyed_executions_count] = deleted_executions_count
diff --git a/spec/lib/good_job_spec.rb b/spec/lib/good_job_spec.rb
index 65bc2f191..d423acaea 100644
--- a/spec/lib/good_job_spec.rb
+++ b/spec/lib/good_job_spec.rb
@@ -46,7 +46,7 @@
     let!(:old_batch) { GoodJob::BatchRecord.create!(finished_at: 15.days.ago) }
 
     it 'deletes finished jobs' do
-      destroyed_records_count = described_class.cleanup_preserved_jobs
+      destroyed_records_count = described_class.cleanup_preserved_jobs(in_batches_of: 1)
 
       expect(destroyed_records_count).to eq 4