From d9e45f2fa94449fe367a92b34f12775a0c85a8ee Mon Sep 17 00:00:00 2001
From: Claire <claire.github-309c@sitedethib.com>
Date: Sun, 23 Apr 2023 22:25:40 +0200
Subject: [PATCH] Fix AccountsStatusesCleanupScheduler not spreading deletes
across accounts correctly (#24607)
---
.../accounts_statuses_cleanup_scheduler.rb | 8 ++++----
...ccounts_statuses_cleanup_scheduler_spec.rb | 20 +++++++++++++++++--
2 files changed, 22 insertions(+), 6 deletions(-)
diff --git a/app/workers/scheduler/accounts_statuses_cleanup_scheduler.rb b/app/workers/scheduler/accounts_statuses_cleanup_scheduler.rb
index 26dd79e52..c4fc53794 100644
--- a/app/workers/scheduler/accounts_statuses_cleanup_scheduler.rb
+++ b/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
diff --git a/spec/workers/scheduler/accounts_statuses_cleanup_scheduler_spec.rb b/spec/workers/scheduler/accounts_statuses_cleanup_scheduler_spec.rb
index d953cc39d..bd17b2abf 100644
--- a/spec/workers/scheduler/accounts_statuses_cleanup_scheduler_spec.rb
+++ b/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