Improve account suspension speed and completeness (mastodon#9290)

- Some associations were missing from the clean-up - Some attributes were not reset on suspension - Skip federation and streaming deletes when purging a dead domain - Move account association definitions to concern
hachyderm · Dec 3, 2018 · 6ddf043 · 6ddf043
1 parent 2df5ef1
commit 6ddf043
Show file tree

Hide file tree

Showing 6 changed files with 153 additions and 76 deletions.
diff --git a/app/models/account.rb b/app/models/account.rb
@@ -49,6 +49,7 @@ class Account < ApplicationRecord
   USERNAME_RE = /[a-z0-9_]+([a-z0-9_\.-]+[a-z0-9_]+)?/i
   MENTION_RE  = /(?<=^|[^\/[:word:]])@((#{USERNAME_RE})(?:@[a-z0-9\.\-]+[a-z0-9]+)?)/i
 
+  include AccountAssociations
   include AccountAvatar
   include AccountFinderConcern
   include AccountHeader
@@ -59,9 +60,6 @@ class Account < ApplicationRecord
 
   enum protocol: [:ostatus, :activitypub]
 
-  # Local users
-  has_one :user, inverse_of: :account
-
   validates :username, presence: true
 
   # Remote user validations
@@ -76,45 +74,6 @@ class Account < ApplicationRecord
   validates :note, length: { maximum: 160 }, if: -> { local? && will_save_change_to_note? }
   validates :fields, length: { maximum: 4 }, if: -> { local? && will_save_change_to_fields? }
 
-  # Timelines
-  has_many :stream_entries, inverse_of: :account, dependent: :destroy
-  has_many :statuses, inverse_of: :account, dependent: :destroy
-  has_many :favourites, inverse_of: :account, dependent: :destroy
-  has_many :mentions, inverse_of: :account, dependent: :destroy
-  has_many :notifications, inverse_of: :account, dependent: :destroy
-
-  # Pinned statuses
-  has_many :status_pins, inverse_of: :account, dependent: :destroy
-  has_many :pinned_statuses, -> { reorder('status_pins.created_at DESC') }, through: :status_pins, class_name: 'Status', source: :status
-
-  # Endorsements
-  has_many :account_pins, inverse_of: :account, dependent: :destroy
-  has_many :endorsed_accounts, through: :account_pins, class_name: 'Account', source: :target_account
-
-  # Media
-  has_many :media_attachments, dependent: :destroy
-
-  # PuSH subscriptions
-  has_many :subscriptions, dependent: :destroy
-
-  # Report relationships
-  has_many :reports
-  has_many :targeted_reports, class_name: 'Report', foreign_key: :target_account_id
-
-  has_many :report_notes, dependent: :destroy
-  has_many :custom_filters, inverse_of: :account, dependent: :destroy
-
-  # Moderation notes
-  has_many :account_moderation_notes, dependent: :destroy
-  has_many :targeted_moderation_notes, class_name: 'AccountModerationNote', foreign_key: :target_account_id, dependent: :destroy
-
-  # Lists
-  has_many :list_accounts, inverse_of: :account, dependent: :destroy
-  has_many :lists, through: :list_accounts
-
-  # Account migrations
-  belongs_to :moved_to_account, class_name: 'Account', optional: true
-
   scope :remote, -> { where.not(domain: nil) }
   scope :local, -> { where(domain: nil) }
   scope :expiring, ->(time) { remote.where.not(subscription_expires_at: nil).where('subscription_expires_at < ?', time) }
@@ -452,6 +411,7 @@ def emojis
   before_create :generate_keys
   before_validation :normalize_domain
   before_validation :prepare_contents, if: :local?
+  before_destroy :clean_feed_manager
 
   private
 
@@ -477,4 +437,19 @@ def normalize_domain
   def emojifiable_text
     [note, display_name, fields.map(&:value)].join(' ')
   end
+
+  def clean_feed_manager
+    reblog_key       = FeedManager.instance.key(:home, id, 'reblogs')
+    reblogged_id_set = Redis.current.zrange(reblog_key, 0, -1)
+
+    Redis.current.pipelined do
+      Redis.current.del(FeedManager.instance.key(:home, id))
+      Redis.current.del(reblog_key)
+
+      reblogged_id_set.each do |reblogged_id|
+        reblog_set_key = FeedManager.instance.key(:home, id, "reblogs:#{reblogged_id}")
+        Redis.current.del(reblog_set_key)
+      end
+    end
+  end
 end
diff --git a/app/models/concerns/account_associations.rb b/app/models/concerns/account_associations.rb
@@ -0,0 +1,53 @@
+# frozen_string_literal: true
+
+module AccountAssociations
+  extend ActiveSupport::Concern
+
+  included do
+    # Local users
+    has_one :user, inverse_of: :account, dependent: :destroy
+
+    # Timelines
+    has_many :stream_entries, inverse_of: :account, dependent: :destroy
+    has_many :statuses, inverse_of: :account, dependent: :destroy
+    has_many :favourites, inverse_of: :account, dependent: :destroy
+    has_many :mentions, inverse_of: :account, dependent: :destroy
+    has_many :notifications, inverse_of: :account, dependent: :destroy
+    has_many :conversations, class_name: 'AccountConversation', dependent: :destroy, inverse_of: :account
+
+    # Pinned statuses
+    has_many :status_pins, inverse_of: :account, dependent: :destroy
+    has_many :pinned_statuses, -> { reorder('status_pins.created_at DESC') }, through: :status_pins, class_name: 'Status', source: :status
+
+    # Endorsements
+    has_many :account_pins, inverse_of: :account, dependent: :destroy
+    has_many :endorsed_accounts, through: :account_pins, class_name: 'Account', source: :target_account
+
+    # Media
+    has_many :media_attachments, dependent: :destroy
+
+    # PuSH subscriptions
+    has_many :subscriptions, dependent: :destroy
+
+    # Report relationships
+    has_many :reports, dependent: :destroy, inverse_of: :account
+    has_many :targeted_reports, class_name: 'Report', foreign_key: :target_account_id, dependent: :destroy, inverse_of: :target_account
+
+    has_many :report_notes, dependent: :destroy
+    has_many :custom_filters, inverse_of: :account, dependent: :destroy
+
+    # Moderation notes
+    has_many :account_moderation_notes, dependent: :destroy, inverse_of: :account
+    has_many :targeted_moderation_notes, class_name: 'AccountModerationNote', foreign_key: :target_account_id, dependent: :destroy, inverse_of: :target_account
+
+    # Lists (that the account is on, not owned by the account)
+    has_many :list_accounts, inverse_of: :account, dependent: :destroy
+    has_many :lists, through: :list_accounts
+
+    # Lists (owned by the account)
+    has_many :owned_lists, class_name: 'List', dependent: :destroy, inverse_of: :account
+
+    # Account migrations
+    belongs_to :moved_to_account, class_name: 'Account', optional: true
+  end
+end
diff --git a/app/services/batched_remove_status_service.rb b/app/services/batched_remove_status_service.rb
@@ -9,7 +9,9 @@ class BatchedRemoveStatusService < BaseService
   # Remove statuses from home feeds
   # Push delete events to streaming API for home feeds and public feeds
   # @param [Status] statuses A preferably batched array of statuses
-  def call(statuses)
+  # @param [Hash] options
+  # @option [Boolean] :skip_side_effects
+  def call(statuses, **options)
     statuses = Status.where(id: statuses.map(&:id)).includes(:account, :stream_entry).flat_map { |status| [status] + status.reblogs.includes(:account, :stream_entry).to_a }
 
     @mentions = statuses.each_with_object({}) { |s, h| h[s.id] = s.active_mentions.includes(:account).to_a }
@@ -26,6 +28,8 @@ def call(statuses)
       status.destroy
     end
 
+    return if options[:skip_side_effects]
+
     # Batch by source account
     statuses.group_by(&:account_id).each_value do |account_statuses|
       account = account_statuses.first.account

diff --git a/app/services/suspend_account_service.rb b/app/services/suspend_account_service.rb
@@ -1,67 +1,108 @@
 # frozen_string_literal: true
 
 class SuspendAccountService < BaseService
+  ASSOCIATIONS_ON_SUSPEND = %w(
+    account_pins
+    active_relationships
+    block_relationships
+    blocked_by_relationships
+    conversation_mutes
+    conversations
+    custom_filters
+    domain_blocks
+    favourites
+    follow_requests
+    list_accounts
+    media_attachments
+    mute_relationships
+    muted_by_relationships
+    notifications
+    owned_lists
+    passive_relationships
+    report_notes
+    status_pins
+    stream_entries
+    subscriptions
+  ).freeze
+
+  ASSOCIATIONS_ON_DESTROY = %w(
+    reports
+    targeted_moderation_notes
+    targeted_reports
+  ).freeze
+
+  # Suspend an account and remove as much of its data as possible
+  # @param [Account]
+  # @param [Hash] options
+  # @option [Boolean] :including_user Remove the user record as well
+  # @option [Boolean] :destroy Remove the account record instead of suspending
   def call(account, **options)
     @account = account
     @options = options
 
     purge_user!
     purge_profile!
     purge_content!
-    unsubscribe_push_subscribers!
   end
 
   private
 
   def purge_user!
-    if @options[:remove_user]
-      @account.user&.destroy
+    return if !@account.local? || @account.user.nil?
+
+    if @options[:including_user]
+      @account.user.destroy
     else
-      @account.user&.disable!
+      @account.user.disable!
     end
   end
 
   def purge_content!
-    if @account.local?
-      ActivityPub::DeliveryWorker.push_bulk(delivery_inboxes) do |inbox_url|
-        [delete_actor_json, @account.id, inbox_url]
-      end
-    end
+    distribute_delete_actor! if @account.local?
 
     @account.statuses.reorder(nil).find_in_batches do |statuses|
-      BatchedRemoveStatusService.new.call(statuses)
+      BatchedRemoveStatusService.new.call(statuses, skip_side_effects: @options[:destroy])
     end
 
-    [
-      @account.media_attachments,
-      @account.stream_entries,
-      @account.notifications,
-      @account.favourites,
-      @account.active_relationships,
-      @account.passive_relationships,
-    ].each do |association|
-      destroy_all(association)
+    associations_for_destruction.each do |association_name|
+      destroy_all(@account.public_send(association_name))
     end
+
+    @account.destroy if @options[:destroy]
   end
 
   def purge_profile!
-    @account.suspended      = true
-    @account.display_name   = ''
-    @account.note           = ''
-    @account.statuses_count = 0
+    # If the account is going to be destroyed
+    # there is no point wasting time updating
+    # its values first
+
+    return if @options[:destroy]
+
+    @account.silenced         = false
+    @account.suspended        = true
+    @account.locked           = false
+    @account.display_name     = ''
+    @account.note             = ''
+    @account.fields           = {}
+    @account.statuses_count   = 0
+    @account.followers_count  = 0
+    @account.following_count  = 0
+    @account.moved_to_account = nil
     @account.avatar.destroy
     @account.header.destroy
     @account.save!
   end
 
-  def unsubscribe_push_subscribers!
-    destroy_all(@account.subscriptions)
-  end
-
   def destroy_all(association)
     association.in_batches.destroy_all
   end
 
+  def distribute_delete_actor!
+    ActivityPub::DeliveryWorker.push_bulk(delivery_inboxes) do |inbox_url|
+      [delete_actor_json, @account.id, inbox_url]
+    end
+  end
+
   def delete_actor_json
     return @delete_actor_json if defined?(@delete_actor_json)
 
@@ -77,4 +118,12 @@ def delete_actor_json
   def delivery_inboxes
     Account.inboxes + Relay.enabled.pluck(:inbox_url)
   end
+
+  def associations_for_destruction
+    if @options[:destroy]
+      ASSOCIATIONS_ON_SUSPEND + ASSOCIATIONS_ON_DESTROY
+    else
+      ASSOCIATIONS_ON_SUSPEND
+    end
+  end
 end
diff --git a/app/workers/admin/suspension_worker.rb b/app/workers/admin/suspension_worker.rb
@@ -6,6 +6,6 @@ class Admin::SuspensionWorker
   sidekiq_options queue: 'pull'
 
   def perform(account_id, remove_user = false)
-    SuspendAccountService.new.call(Account.find(account_id), remove_user: remove_user)
+    SuspendAccountService.new.call(Account.find(account_id), including_user: remove_user)
   end
 end
diff --git a/lib/mastodon/domains_cli.rb b/lib/mastodon/domains_cli.rb
@@ -22,11 +22,7 @@ def purge(domain)
       dry_run = options[:dry_run] ? ' (DRY RUN)' : ''
 
       Account.where(domain: domain).find_each do |account|
-        unless options[:dry_run]
-          SuspendAccountService.new.call(account)
-          account.destroy
-        end
-
+        SuspendAccountService.new.call(account, destroy: true) unless options[:dry_run]
         removed += 1
         say('.', :green, false)
       end