Fix too many forwards (#5854)

* Avoid sending explicit Undo->Announce when original deleted

* Do not forward a reply back to the server that sent it

* Deduplicate inboxes of rebloggers' followers for delete forwarding

* Adjust test

* Fix wrong class, bad SQL, wrong variable, outdated comment

app/lib/activitypub/activity/create.rb

@@ -210,7 +210,7 @@ class ActivityPub::Activity::Create < ActivityPub::Activity
 
   def forward_for_reply
     return unless @json['signature'].present? && reply_to_local?
-    ActivityPub::RawDistributionWorker.perform_async(Oj.dump(@json), replied_to_status.account_id)
+    ActivityPub::RawDistributionWorker.perform_async(Oj.dump(@json), replied_to_status.account_id, [@account.preferred_inbox_url])
   end
 
   def lock_options

app/lib/activitypub/activity/delete.rb

@@ -30,8 +30,11 @@ class ActivityPub::Activity::Delete < ActivityPub::Activity
   def forward_for_reblogs(status)
     return if @json['signature'].blank?
 
-    ActivityPub::RawDistributionWorker.push_bulk(status.reblogs.includes(:account).references(:account).merge(Account.local).pluck(:account_id)) do |account_id|
+    rebloggers_ids = status.reblogs.includes(:account).references(:account).merge(Account.local).pluck(:account_id)
-      [payload, account_id]
+    inboxes        = Account.where(id: ::Follow.where(target_account_id: rebloggers_ids).select(:account_id)).inboxes - [@account.preferred_inbox_url]
+
+    ActivityPub::DeliveryWorker.push_bulk(inboxes) do |inbox_url|
+      [payload, rebloggers_ids.first, inbox_url]
     end
   end
 

app/models/account.rb

@@ -214,6 +214,10 @@ class Account < ApplicationRecord
     Rails.cache.fetch("exclude_domains_for:#{id}") { domain_blocks.pluck(:domain) }
   end
 
+  def preferred_inbox_url
+    shared_inbox_url.presence || inbox_url
+  end
+
   class << self
     def readonly_attributes
       super - %w(statuses_count following_count followers_count)

app/services/remove_status_service.rb

@@ -3,7 +3,7 @@
 class RemoveStatusService < BaseService
   include StreamEntryRenderer
 
-  def call(status)
+  def call(status, options = {})
     @payload      = Oj.dump(event: :delete, payload: status.id.to_s)
     @status       = status
     @account      = status.account
@@ -11,6 +11,7 @@ class RemoveStatusService < BaseService
     @mentions     = status.mentions.includes(:account).to_a
     @reblogs      = status.reblogs.to_a
     @stream_entry = status.stream_entry
+    @options      = options
 
     remove_from_self if status.account.local?
     remove_from_followers
@@ -22,7 +23,12 @@ class RemoveStatusService < BaseService
 
     @status.destroy!
 
-    return unless @account.local?
+    # There is no reason to send out Undo activities when the
+    # cause is that the original object has been removed, since
+    # original object being removed implicitly removes reblogs
+    # of it. The Delete activity of the original is forwarded
+    # separately.
+    return if !@account.local? || @options[:original_removed]
 
     remove_from_remote_followers
     remove_from_remote_affected
@@ -104,7 +110,7 @@ class RemoveStatusService < BaseService
     # without us being able to do all the fancy stuff
 
     @reblogs.each do |reblog|
-      RemoveStatusService.new.call(reblog)
+      RemoveStatusService.new.call(reblog, original_removed: true)
     end
   end
 

app/workers/activitypub/raw_distribution_worker.rb

@@ -5,10 +5,10 @@ class ActivityPub::RawDistributionWorker
 
   sidekiq_options queue: 'push'
 
-  def perform(json, source_account_id)
+  def perform(json, source_account_id, exclude_inboxes = [])
     @account = Account.find(source_account_id)
 
-    ActivityPub::DeliveryWorker.push_bulk(inboxes) do |inbox_url|
+    ActivityPub::DeliveryWorker.push_bulk(inboxes - exclude_inboxes) do |inbox_url|
       [json, @account.id, inbox_url]
     end
   rescue ActiveRecord::RecordNotFound

spec/lib/activitypub/activity/delete_spec.rb

@@ -1,8 +1,8 @@
 require 'rails_helper'
 
 RSpec.describe ActivityPub::Activity::Delete do
-  let(:sender)    { Fabricate(:account, domain: 'example.com') }
+  let(:sender) { Fabricate(:account, domain: 'example.com') }
-  let(:status)    { Fabricate(:status, account: sender, uri: 'foobar') }
+  let(:status) { Fabricate(:status, account: sender, uri: 'foobar') }
 
   let(:json) do
     {
@@ -30,13 +30,13 @@ RSpec.describe ActivityPub::Activity::Delete do
   context 'when the status has been reblogged' do
     describe '#perform' do
       subject { described_class.new(json, sender) }
-      let(:reblogger) { Fabricate(:account) }
+      let!(:reblogger) { Fabricate(:account) }
-      let(:follower)   { Fabricate(:account, username: 'follower', protocol: :activitypub, domain: 'example.com', inbox_url: 'http://example.com/inbox') }
+      let!(:follower)  { Fabricate(:account, username: 'follower', protocol: :activitypub, domain: 'example.com', inbox_url: 'http://example.com/inbox') }
+      let!(:reblog)    { Fabricate(:status, account: reblogger, reblog: status) }
 
       before do
         stub_request(:post, 'http://example.com/inbox').to_return(status: 200)
         follower.follow!(reblogger)
-        Fabricate(:status, account: reblogger, reblog: status)
         subject.perform
       end
 
@@ -45,8 +45,7 @@ RSpec.describe ActivityPub::Activity::Delete do
       end
 
       it 'sends delete activity to followers of rebloggers' do
-        # one for Delete original post, and one for Undo reblog (normal delivery)
+        expect(a_request(:post, 'http://example.com/inbox')).to have_been_made.once
-        expect(a_request(:post, 'http://example.com/inbox')).to have_been_made.twice
       end
     end
   end