From 7bf3c6e57b52cd9390f2140a1cc17292c281aacf Mon Sep 17 00:00:00 2001 From: ThibG Date: Sun, 20 Dec 2020 18:25:00 +0100 Subject: [PATCH] Fix AccountDeletionWorker crashing and clogging sidekiq queues (#15380) * Fix account deletion workers being queued multiple times for a single account * Fix poll votes being unnecessarily instantiated on poll deletion * Fix favourites being unnecessarily instantiated on status deletion * Remove inaccurate comments * Delete polls instead of destroying them Co-authored-by: Claire --- app/models/poll.rb | 2 +- app/models/status.rb | 2 +- app/services/batched_remove_status_service.rb | 3 --- app/services/delete_account_service.rb | 4 +++- app/workers/account_deletion_worker.rb | 2 +- 5 files changed, 6 insertions(+), 7 deletions(-) diff --git a/app/models/poll.rb b/app/models/poll.rb index b5deafcc21f..e1ca5525219 100644 --- a/app/models/poll.rb +++ b/app/models/poll.rb @@ -25,7 +25,7 @@ class Poll < ApplicationRecord belongs_to :account belongs_to :status - has_many :votes, class_name: 'PollVote', inverse_of: :poll, dependent: :destroy + has_many :votes, class_name: 'PollVote', inverse_of: :poll, dependent: :delete_all has_many :notifications, as: :activity, dependent: :destroy diff --git a/app/models/status.rb b/app/models/status.rb index 96d90e1c27e..f1b3b75ce8e 100644 --- a/app/models/status.rb +++ b/app/models/status.rb @@ -56,7 +56,7 @@ class Status < ApplicationRecord belongs_to :thread, foreign_key: 'in_reply_to_id', class_name: 'Status', inverse_of: :replies, optional: true belongs_to :reblog, foreign_key: 'reblog_of_id', class_name: 'Status', inverse_of: :reblogs, optional: true - has_many :favourites, inverse_of: :status, dependent: :destroy + has_many :favourites, inverse_of: :status, dependent: :delete_all has_many :bookmarks, inverse_of: :status, dependent: :destroy has_many :reblogs, foreign_key: 'reblog_of_id', class_name: 'Status', inverse_of: :reblog, dependent: :destroy has_many :replies, foreign_key: 'in_reply_to_id', class_name: 'Status', inverse_of: :thread diff --git a/app/services/batched_remove_status_service.rb b/app/services/batched_remove_status_service.rb index 2295a01dc3d..28e5468b389 100644 --- a/app/services/batched_remove_status_service.rb +++ b/app/services/batched_remove_status_service.rb @@ -4,8 +4,6 @@ class BatchedRemoveStatusService < BaseService include Redisable # Delete given statuses and reblogs of them - # Dispatch PuSH updates of the deleted statuses, but only local ones - # Dispatch Salmon deletes, unique per domain, of the deleted statuses, but only local ones # Remove statuses from home feeds # Push delete events to streaming API for home feeds and public feeds # @param [Enumerable] statuses A preferably batched array of statuses @@ -19,7 +17,6 @@ class BatchedRemoveStatusService < BaseService @json_payloads = statuses.each_with_object({}) { |s, h| h[s.id] = Oj.dump(event: :delete, payload: s.id.to_s) } - # Ensure that rendered XML reflects destroyed state statuses.each do |status| status.mark_for_mass_destruction! status.destroy diff --git a/app/services/delete_account_service.rb b/app/services/delete_account_service.rb index 9cb80c95aea..fe9b30b1746 100644 --- a/app/services/delete_account_service.rb +++ b/app/services/delete_account_service.rb @@ -122,7 +122,9 @@ class DeleteAccountService < BaseService @account.polls.reorder(nil).find_each do |poll| next if @options[:reserve_username] && reported_status_ids.include?(poll.status_id) - poll.destroy + # We can safely delete the poll rather than destroy it, as any non-reported + # status should have been deleted already + poll.delete end associations_for_destruction.each do |association_name| diff --git a/app/workers/account_deletion_worker.rb b/app/workers/account_deletion_worker.rb index 98b67419d01..fdf013e0104 100644 --- a/app/workers/account_deletion_worker.rb +++ b/app/workers/account_deletion_worker.rb @@ -3,7 +3,7 @@ class AccountDeletionWorker include Sidekiq::Worker - sidekiq_options queue: 'pull' + sidekiq_options queue: 'pull', lock: :until_executed def perform(account_id, options = {}) reserve_username = options.with_indifferent_access.fetch(:reserve_username, true)