From 9db26db4958e47bb49f3724bdc550a72684079fe Mon Sep 17 00:00:00 2001 From: Claire Date: Wed, 12 Mar 2025 11:28:06 +0100 Subject: [PATCH] Refactor reply-fetching code and disable it by default (#34147) --- .env.production.sample | 4 ++-- app/helpers/json_ld_helper.rb | 14 +++++++------- .../concerns/status/fetch_replies_concern.rb | 12 +----------- .../activitypub/fetch_remote_status_service.rb | 8 ++++---- .../concerns/status/fetch_replies_concern_spec.rb | 5 ----- .../fetch_remote_status_service_spec.rb | 6 ------ .../activitypub/fetch_all_replies_worker_spec.rb | 1 + 7 files changed, 15 insertions(+), 35 deletions(-) diff --git a/.env.production.sample b/.env.production.sample index 61bad7609c1..12ab2b6dcb8 100644 --- a/.env.production.sample +++ b/.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) -FETCH_REPLIES_ENABLED=true +# (default: false) +FETCH_REPLIES_ENABLED=false # Period to wait between fetching replies (in minutes) FETCH_REPLIES_COOLDOWN_MINUTES=15 diff --git a/app/helpers/json_ld_helper.rb b/app/helpers/json_ld_helper.rb index ccdefb35d81..65daaa53020 100644 --- a/app/helpers/json_ld_helper.rb +++ b/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 - def fetch_resource(uri, id_is_known, on_behalf_of = nil, raise_on_error: false, request_options: {}) + # @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: :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>] - # - +true+, +:all+ - raise if response code is not in the 2** range + # @param raise_on_error [Symbol<:all, :temporary, :none>] + # - +: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+ - def fetch_resource_without_id_validation(uri, on_behalf_of = nil, raise_on_error: false, request_options: {}) + # - +:none+ - do not raise, return +nil+ + 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) ) diff --git a/app/models/concerns/status/fetch_replies_concern.rb b/app/models/concerns/status/fetch_replies_concern.rb index f34bce59b4e..fd9929aba49 100644 --- a/app/models/concerns/status/fetch_replies_concern.rb +++ b/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 diff --git a/app/services/activitypub/fetch_remote_status_service.rb b/app/services/activitypub/fetch_remote_status_service.rb index dc74de32f1a..7173746f2de 100644 --- a/app/services/activitypub/fetch_remote_status_service.rb +++ b/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 - existing_status = Status.find_by(uri: uri) - if !existing_status.nil? && existing_status.unsubscribed? && existing_status.distributable? + # If this is a 404 from a public status from a remote account, delete it + existing_status = Status.remote.find_by(uri: uri) + 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) diff --git a/spec/models/concerns/status/fetch_replies_concern_spec.rb b/spec/models/concerns/status/fetch_replies_concern_spec.rb index f152cf234a8..e9c81d43b10 100644 --- a/spec/models/concerns/status/fetch_replies_concern_spec.rb +++ b/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 diff --git a/spec/services/activitypub/fetch_remote_status_service_spec.rb b/spec/services/activitypub/fetch_remote_status_service_spec.rb index 847affd3071..2503a58ac20 100644 --- a/spec/services/activitypub/fetch_remote_status_service_spec.rb +++ b/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 diff --git a/spec/workers/activitypub/fetch_all_replies_worker_spec.rb b/spec/workers/activitypub/fetch_all_replies_worker_spec.rb index 2b291e9624f..4746d742d09 100644 --- a/spec/workers/activitypub/fetch_all_replies_worker_spec.rb +++ b/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