From 3c06ed866bdbf2ca74b7cba0d89ec39ba1d5b4c7 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 | 144 ++++++------------ 2 files changed, 49 insertions(+), 103 deletions(-) diff --git a/app/workers/scheduler/accounts_statuses_cleanup_scheduler.rb b/app/workers/scheduler/accounts_statuses_cleanup_scheduler.rb index d245f6bbdc..ff45c3aa1c 100644 --- a/app/workers/scheduler/accounts_statuses_cleanup_scheduler.rb +++ b/app/workers/scheduler/accounts_statuses_cleanup_scheduler.rb @@ -42,7 +42,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? @@ -77,14 +77,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 4d9185093a..d953cc39d4 100644 --- a/spec/workers/scheduler/accounts_statuses_cleanup_scheduler_spec.rb +++ b/spec/workers/scheduler/accounts_statuses_cleanup_scheduler_spec.rb @@ -1,16 +1,17 @@ -# frozen_string_literal: true - require 'rails_helper' describe Scheduler::AccountsStatusesCleanupScheduler do subject { described_class.new } - let!(:account_alice) { Fabricate(:account, domain: nil, username: 'alice') } - let!(:account_bob) { Fabricate(:account, domain: nil, username: 'bob') } - let!(:account_chris) { Fabricate(:account, domain: nil, username: 'chris') } - let!(:account_dave) { Fabricate(:account, domain: nil, username: 'dave') } - let!(:account_erin) { Fabricate(:account, domain: nil, username: 'erin') } - let!(:remote) { Fabricate(:account) } + let!(:account1) { Fabricate(:account, domain: nil) } + let!(:account2) { Fabricate(:account, domain: nil) } + let!(:account3) { Fabricate(:account, domain: nil) } + let!(:account4) { 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(:queue_size) { 0 } let(:queue_latency) { 0 } @@ -18,18 +19,38 @@ describe Scheduler::AccountsStatusesCleanupScheduler do [ { 'concurrency' => 2, - 'queues' => %w(push default), + 'queues' => ['push', 'default'], }, ] end before do - queue_stub = instance_double(Sidekiq::Queue, size: queue_size, latency: queue_latency) + queue_stub = double + allow(queue_stub).to receive(:size).and_return(queue_size) + allow(queue_stub).to receive(:latency).and_return(queue_latency) allow(Sidekiq::Queue).to receive(:new).and_return(queue_stub) allow(Sidekiq::ProcessSet).to receive(:new).and_return(process_set_stub) - sidekiq_stats_stub = instance_double(Sidekiq::Stats) + sidekiq_stats_stub = double allow(Sidekiq::Stats).to receive(:new).and_return(sidekiq_stats_stub) + + # Create a bunch of old statuses + 10.times do + Fabricate(:status, account: account1, created_at: 3.years.ago) + 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: remote, created_at: 3.years.ago) + end + + # Create a bunch of newer statuses + 5.times do + Fabricate(:status, account: account1, created_at: 3.minutes.ago) + Fabricate(:status, account: account2, created_at: 3.minutes.ago) + Fabricate(:status, account: account3, created_at: 3.minutes.ago) + Fabricate(:status, account: account4, created_at: 3.minutes.ago) + Fabricate(:status, account: remote, created_at: 3.minutes.ago) + end end describe '#under_load?' do @@ -49,19 +70,19 @@ describe Scheduler::AccountsStatusesCleanupScheduler do end end - describe '#compute_budget' do - context 'with a single thread' do - let(:process_set_stub) { [{ 'concurrency' => 1, 'queues' => %w(push default) }] } + describe '#get_budget' do + context 'on a single thread' do + let(:process_set_stub) { [ { 'concurrency' => 1, 'queues' => ['push', 'default'] } ] } it 'returns a low value' do expect(subject.compute_budget).to be < 10 end end - context 'with a lot of threads' do + context 'on a lot of threads' do let(:process_set_stub) do [ - { 'concurrency' => 2, 'queues' => %w(push default) }, + { 'concurrency' => 2, 'queues' => ['push', 'default'] }, { 'concurrency' => 2, 'queues' => ['push'] }, { 'concurrency' => 2, 'queues' => ['push'] }, { 'concurrency' => 2, 'queues' => ['push'] }, @@ -75,96 +96,21 @@ describe Scheduler::AccountsStatusesCleanupScheduler do end describe '#perform' do - around do |example| - Timeout.timeout(30) do - example.run - end - end - - before do - # Policies for the accounts - Fabricate(:account_statuses_cleanup_policy, account: account_alice) - Fabricate(:account_statuses_cleanup_policy, account: account_chris) - Fabricate(:account_statuses_cleanup_policy, account: account_dave, enabled: false) - Fabricate(:account_statuses_cleanup_policy, account: account_erin) - - # Create a bunch of old statuses - 4.times do - Fabricate(:status, account: account_alice, created_at: 3.years.ago) - Fabricate(:status, account: account_bob, created_at: 3.years.ago) - Fabricate(:status, account: account_chris, created_at: 3.years.ago) - Fabricate(:status, account: account_dave, created_at: 3.years.ago) - Fabricate(:status, account: account_erin, created_at: 3.years.ago) - Fabricate(:status, account: remote, created_at: 3.years.ago) - end - - # Create a bunch of newer statuses - Fabricate(:status, account: account_alice, created_at: 3.minutes.ago) - Fabricate(:status, account: account_bob, created_at: 3.minutes.ago) - Fabricate(:status, account: account_chris, created_at: 3.minutes.ago) - Fabricate(:status, account: account_dave, created_at: 3.minutes.ago) - Fabricate(:status, account: remote, created_at: 3.minutes.ago) - end - context 'when the budget is lower than the number of toots to delete' do - it 'deletes the appropriate statuses' do - expect(Status.count).to be > (subject.compute_budget) # Data check - - expect { subject.perform } - .to change(Status, :count).by(-subject.compute_budget) # Cleanable statuses - .and (not_change { account_bob.statuses.count }) # No cleanup policy for account - .and(not_change { account_dave.statuses.count }) # Disabled cleanup policy + it 'deletes as many statuses as the given budget' do + expect { subject.perform }.to change { Status.count }.by(-subject.compute_budget) end - it 'eventually deletes every deletable toot given enough runs' do - stub_const 'Scheduler::AccountsStatusesCleanupScheduler::MAX_BUDGET', 4 - - expect { 3.times { subject.perform } }.to change(Status, :count).by(-cleanable_statuses_count) + it 'does not delete from accounts with no cleanup policy' do + expect { subject.perform }.to_not change { account2.statuses.count } 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 { account_alice.statuses.count } - .and change { account_chris.statuses.count } - .and(change { account_erin.statuses.count }) + it 'does not delete from accounts with disabled cleanup policies' do + expect { subject.perform }.to_not change { account4.statuses.count } end - context 'when given a big budget' do - let(:process_set_stub) { [{ 'concurrency' => 400, 'queues' => %w(push default) }] } - - before do - stub_const 'Scheduler::AccountsStatusesCleanupScheduler::MAX_BUDGET', 400 - end - - it 'correctly handles looping in a single run' do - expect(subject.compute_budget).to eq(400) - expect { subject.perform }.to change(Status, :count).by(-cleanable_statuses_count) - end - end - - context 'when there is no work to be done' do - let(:process_set_stub) { [{ 'concurrency' => 400, 'queues' => %w(push default) }] } - - before do - stub_const 'Scheduler::AccountsStatusesCleanupScheduler::MAX_BUDGET', 400 - subject.perform - end - - it 'does not get stuck' do - expect(subject.compute_budget).to eq(400) - expect { subject.perform }.to_not change(Status, :count) - end - end - - def cleanable_statuses_count - Status - .where(account_id: [account_alice, account_chris, account_erin]) # Accounts with enabled policies - .where('created_at < ?', 2.weeks.ago) # Policy defaults is 2.weeks - .count + it 'eventually deletes every deletable toot' do + expect { subject.perform; subject.perform; subject.perform; subject.perform }.to change { Status.count }.by(-20) end end end