From 5dc3173ef8a3bc3ed05c5ebf7c6c2c3840b803fb Mon Sep 17 00:00:00 2001 From: Claire 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 5203d4c25..a7737622d 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? @@ -81,14 +81,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 9ed8da1ba..436f2d93f 100644 --- a/spec/workers/scheduler/accounts_statuses_cleanup_scheduler_spec.rb +++ b/spec/workers/scheduler/accounts_statuses_cleanup_scheduler_spec.rb @@ -9,11 +9,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 } @@ -42,6 +44,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 @@ -111,8 +114,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