Fix AccountsStatusesCleanupScheduler not spreading deletes across accounts correctly (#24607)

app/workers/scheduler/accounts_statuses_cleanup_scheduler.rb

@@ -44,7 +44,7 @@ class Scheduler::AccountsStatusesCleanupScheduler
       num_processed_accounts = 0
 
       scope = AccountStatusesCleanupPolicy.where(enabled: true)
-      scope.where(Account.arel_table[:id].gt(first_policy_id)) if first_policy_id.present?
+      scope = scope.where(id: first_policy_id...) if first_policy_id.present?
       scope.find_each(order: :asc) do |policy|
         num_deleted = AccountStatusesCleanupService.new.call(policy, [budget, PER_ACCOUNT_BUDGET].min)
         num_processed_accounts += 1 unless num_deleted.zero?
@@ -80,14 +80,14 @@ class Scheduler::AccountsStatusesCleanupScheduler
   end
 
   def last_processed_id
-    redis.get('account_statuses_cleanup_scheduler:last_account_id')
+    redis.get('account_statuses_cleanup_scheduler:last_policy_id')
   end
 
   def save_last_processed_id(id)
     if id.nil?
-      redis.del('account_statuses_cleanup_scheduler:last_account_id')
+      redis.del('account_statuses_cleanup_scheduler:last_policy_id')
     else
-      redis.set('account_statuses_cleanup_scheduler:last_account_id', id, ex: 1.hour.seconds)
+      redis.set('account_statuses_cleanup_scheduler:last_policy_id', id, ex: 1.hour.seconds)
     end
   end
 end

spec/workers/scheduler/accounts_statuses_cleanup_scheduler_spec.rb

@@ -7,11 +7,13 @@ describe Scheduler::AccountsStatusesCleanupScheduler do
   let!(:account2)  { Fabricate(:account, domain: nil) }
   let!(:account3)  { Fabricate(:account, domain: nil) }
   let!(:account4)  { Fabricate(:account, domain: nil) }
+  let!(:account5)  { Fabricate(:account, domain: nil) }
   let!(:remote)    { Fabricate(:account) }
 
   let!(:policy1)   { Fabricate(:account_statuses_cleanup_policy, account: account1) }
   let!(:policy2)   { Fabricate(:account_statuses_cleanup_policy, account: account3) }
   let!(:policy3)   { Fabricate(:account_statuses_cleanup_policy, account: account4, enabled: false) }
+  let!(:policy4)   { Fabricate(:account_statuses_cleanup_policy, account: account5) }
 
   let(:queue_size)       { 0 }
   let(:queue_latency)    { 0 }
@@ -40,6 +42,7 @@ describe Scheduler::AccountsStatusesCleanupScheduler do
       Fabricate(:status, account: account2, created_at: 3.years.ago)
       Fabricate(:status, account: account3, created_at: 3.years.ago)
       Fabricate(:status, account: account4, created_at: 3.years.ago)
+      Fabricate(:status, account: account5, created_at: 3.years.ago)
       Fabricate(:status, account: remote, created_at: 3.years.ago)
     end
 
@@ -109,8 +112,21 @@ describe Scheduler::AccountsStatusesCleanupScheduler do
         expect { subject.perform }.to_not change { account4.statuses.count }
       end
 
-      it 'eventually deletes every deletable toot' do
-        expect { subject.perform; subject.perform; subject.perform; subject.perform }.to change { Status.count }.by(-20)
+      it 'eventually deletes every deletable toot given enough runs' do
+        stub_const 'Scheduler::AccountsStatusesCleanupScheduler::MAX_BUDGET', 4
+
+        expect { 10.times { subject.perform } }.to change { Status.count }.by(-30)
+      end
+
+      it 'correctly round-trips between users across several runs' do
+        stub_const 'Scheduler::AccountsStatusesCleanupScheduler::MAX_BUDGET', 3
+        stub_const 'Scheduler::AccountsStatusesCleanupScheduler::PER_ACCOUNT_BUDGET', 2
+
+        expect { 3.times { subject.perform } }
+          .to change { Status.count }.by(-3 * 3)
+          .and change { account1.statuses.count }
+          .and change { account3.statuses.count }
+          .and change { account5.statuses.count }
       end
     end
   end