Refactor reply-fetching code and disable it by default (#34147)

.env.production.sample

@@ -90,8 +90,8 @@ SESSION_RETENTION_PERIOD=31556952
 # Fetch All Replies Behavior
 # --------------------------
 # When a user expands a post (DetailedStatus view), fetch all of its replies
-# (default: true if unset, set explicitly to ``false`` to disable)
+# (default: false)
-FETCH_REPLIES_ENABLED=true
+FETCH_REPLIES_ENABLED=false
 
 # Period to wait between fetching replies (in minutes)
 FETCH_REPLIES_COOLDOWN_MINUTES=15

app/helpers/json_ld_helper.rb

@@ -159,8 +159,8 @@ module JsonLdHelper
   # @param uri [String]
   # @param id_is_known [Boolean]
   # @param on_behalf_of [nil, Account]
-  # @param raise_on_error [Boolean, Symbol<:all, :temporary>] See {#fetch_resource_without_id_validation} for possible values
+  # @param raise_on_error [Symbol<:all, :temporary, :none>] See {#fetch_resource_without_id_validation} for possible values
-  def fetch_resource(uri, id_is_known, on_behalf_of = nil, raise_on_error: false, request_options: {})
+  def fetch_resource(uri, id_is_known, on_behalf_of = nil, raise_on_error: :none, request_options: {})
     unless id_is_known
       json = fetch_resource_without_id_validation(uri, on_behalf_of, raise_on_error: raise_on_error)
 
@@ -185,17 +185,17 @@ module JsonLdHelper
   #
   # @param uri [String]
   # @param on_behalf_of [nil, Account]
-  # @param raise_on_error [Boolean, Symbol<:all, :temporary>]
+  # @param raise_on_error [Symbol<:all, :temporary, :none>]
-  #   - +true+, +:all+ - raise if response code is not in the 2** range
+  #   - +:all+ - raise if response code is not in the 2xx range
   #   - +:temporary+ - raise if the response code is not an "unsalvageable error" like a 404
   #     (see {#response_error_unsalvageable} )
-  #   - +false+ - do not raise, return +nil+
+  #   - +:none+ - do not raise, return +nil+
-  def fetch_resource_without_id_validation(uri, on_behalf_of = nil, raise_on_error: false, request_options: {})
+  def fetch_resource_without_id_validation(uri, on_behalf_of = nil, raise_on_error: :none, request_options: {})
     on_behalf_of ||= Account.representative
 
     build_request(uri, on_behalf_of, options: request_options).perform do |response|
       raise Mastodon::UnexpectedResponseError, response if !response_successful?(response) && (
-        [true, :all].include?(raise_on_error) ||
+        raise_on_error == :all ||
         (!response_error_unsalvageable?(response) && raise_on_error == :temporary)
       )
 

app/models/concerns/status/fetch_replies_concern.rb

@@ -4,7 +4,7 @@ module Status::FetchRepliesConcern
   extend ActiveSupport::Concern
 
   # enable/disable fetching all replies
-  FETCH_REPLIES_ENABLED = ENV.key?('FETCH_REPLIES_ENABLED') ? ENV['FETCH_REPLIES_ENABLED'] == 'true' : true
+  FETCH_REPLIES_ENABLED = ENV['FETCH_REPLIES_ENABLED'] == 'true'
 
   # debounce fetching all replies to minimize DoS
   FETCH_REPLIES_COOLDOWN_MINUTES = (ENV['FETCH_REPLIES_COOLDOWN_MINUTES'] || 15).to_i.minutes
@@ -40,14 +40,4 @@ module Status::FetchRepliesConcern
       fetched_replies_at.nil? || fetched_replies_at <= FETCH_REPLIES_COOLDOWN_MINUTES.ago
     )
   end
-
-  def unsubscribed?
-    return false if local?
-
-    !Follow.joins(:account).exists?(
-      target_account: account.id,
-      account: { domain: nil },
-      created_at: ..updated_at
-    )
-  end
 end

app/services/activitypub/fetch_remote_status_service.rb

@@ -83,13 +83,13 @@ class ActivityPub::FetchRemoteStatusService < BaseService
 
   def fetch_status(uri, id_is_known, on_behalf_of = nil)
     begin
-      fetch_resource(uri, id_is_known, on_behalf_of, raise_on_error: true)
+      fetch_resource(uri, id_is_known, on_behalf_of, raise_on_error: :all)
     rescue Mastodon::UnexpectedResponseError => e
       return unless e.response.code == 404
 
-      # If this is a 404 from a status from an account that has no local followers, delete it
+      # If this is a 404 from a public status from a remote account, delete it
-      existing_status = Status.find_by(uri: uri)
+      existing_status = Status.remote.find_by(uri: uri)
-      if !existing_status.nil? && existing_status.unsubscribed? && existing_status.distributable?
+      if existing_status&.distributable?
         Rails.logger.debug { "FetchRemoteStatusService - Got 404 for orphaned status with URI #{uri}, deleting" }
         Tombstone.find_or_create_by(uri: uri, account: existing_status.account)
         RemoveStatusService.new.call(existing_status, redraft: false)

spec/models/concerns/status/fetch_replies_concern_spec.rb

@@ -85,7 +85,6 @@ RSpec.describe Status::FetchRepliesConcern do
 
       it 'shows the status as unsubscribed' do
         expect(Status.unsubscribed).to eq([status])
-        expect(status.unsubscribed?).to be(true)
       end
     end
 
@@ -96,7 +95,6 @@ RSpec.describe Status::FetchRepliesConcern do
 
       it 'shows the status as unsubscribed' do
         expect(Status.unsubscribed).to eq([status])
-        expect(status.unsubscribed?).to be(true)
       end
     end
 
@@ -107,7 +105,6 @@ RSpec.describe Status::FetchRepliesConcern do
 
       it 'shows the status as unsubscribed' do
         expect(Status.unsubscribed).to eq([status])
-        expect(status.unsubscribed?).to be(true)
       end
     end
 
@@ -118,14 +115,12 @@ RSpec.describe Status::FetchRepliesConcern do
 
       it 'does not show the status as unsubscribed' do
         expect(Status.unsubscribed).to eq([])
-        expect(status.unsubscribed?).to be(false)
       end
     end
 
     context 'when the status has no followers' do
       it 'shows the status as unsubscribed' do
         expect(Status.unsubscribed).to eq([status])
-        expect(status.unsubscribed?).to be(true)
       end
     end
   end

spec/services/activitypub/fetch_remote_status_service_spec.rb

@@ -296,12 +296,6 @@ RSpec.describe ActivityPub::FetchRemoteStatusService do
               it_behaves_like 'no delete'
             end
           end
-
-          context 'when the status is from an account with local followers' do
-            let(:follow) { Fabricate(:follow, account: follower, target_account: sender, created_at: 2.days.ago) }
-
-            it_behaves_like 'no delete'
-          end
         end
       end
 

spec/workers/activitypub/fetch_all_replies_worker_spec.rb

@@ -123,6 +123,7 @@ RSpec.describe ActivityPub::FetchAllRepliesWorker do
   end
 
   before do
+    stub_const('Status::FetchRepliesConcern::FETCH_REPLIES_ENABLED', true)
     allow(FetchReplyWorker).to receive(:push_bulk)
     all_items.each do |item|
       next if [top_note_uri, reply_note_uri].include? item