Fix webfinger retries (#4275)

* Do not raise unretryable exceptions in ResolveRemoteAccountService

* Removed fatal exceptions from ResolveRemoteAccountService

Exceptions that cannot be retried should not be raised. New exception
class for those that can be retried (Mastodon::UnexpectedResponseError)

10 files changed, 33 insertions, 32 deletions

diff --git a/app/controllers/api/base_controller.rb b/app/controllers/api/base_controller.rb
index c1b2ec3cf..105a2859d 100644
--- a/app/controllers/api/base_controller.rb
+++ b/app/controllers/api/base_controller.rb
@@ -17,11 +17,7 @@ class Api::BaseController < ApplicationController
     render json: { error: 'Record not found' }, status: 404
   end
 
-  rescue_from Goldfinger::Error do
-    render json: { error: 'Remote account could not be resolved' }, status: 422
-  end
-
-  rescue_from HTTP::Error do
+  rescue_from HTTP::Error, Mastodon::UnexpectedResponseError do
     render json: { error: 'Remote data could not be fetched' }, status: 503
   end
 
diff --git a/app/lib/exceptions.rb b/app/lib/exceptions.rb
index 9bc802c12..34d84a34f 100644
--- a/app/lib/exceptions.rb
+++ b/app/lib/exceptions.rb
@@ -5,4 +5,14 @@ module Mastodon
   class NotPermittedError < Error; end
   class ValidationError < Error; end
   class RaceConditionError < Error; end
+
+  class UnexpectedResponseError < Error
+    def initialize(response = nil)
+      @response = response
+    end
+
+    def to_s
+      "#{@response.uri} returned code #{@response.code}"
+    end
+  end
 end
diff --git a/app/services/fetch_remote_account_service.rb b/app/services/fetch_remote_account_service.rb
index 1efac365b..8eed0d454 100644
--- a/app/services/fetch_remote_account_service.rb
+++ b/app/services/fetch_remote_account_service.rb
@@ -32,8 +32,5 @@ class FetchRemoteAccountService < BaseService
   rescue Nokogiri::XML::XPath::SyntaxError
     Rails.logger.debug 'Invalid XML or missing namespace'
     nil
-  rescue Goldfinger::NotFoundError, Goldfinger::Error
-    Rails.logger.debug 'Exceptions related to Goldfinger occurs'
-    nil
   end
 end
diff --git a/app/services/fetch_remote_status_service.rb b/app/services/fetch_remote_status_service.rb
index 6ac31e4d8..b9f5f97b1 100644
--- a/app/services/fetch_remote_status_service.rb
+++ b/app/services/fetch_remote_status_service.rb
@@ -33,9 +33,6 @@ class FetchRemoteStatusService < BaseService
   rescue Nokogiri::XML::XPath::SyntaxError
     Rails.logger.debug 'Invalid XML or missing namespace'
     nil
-  rescue Goldfinger::NotFoundError, Goldfinger::Error
-    Rails.logger.debug 'Exceptions related to Goldfinger occurs'
-    nil
   end
 
   def confirmed_domain?(domain, account)
diff --git a/app/services/process_interaction_service.rb b/app/services/process_interaction_service.rb
index 584a109ad..cc99cde03 100644
--- a/app/services/process_interaction_service.rb
+++ b/app/services/process_interaction_service.rb
@@ -47,7 +47,7 @@ class ProcessInteractionService < BaseService
         reflect_unblock!(account, target_account)
       end
     end
-  rescue Goldfinger::Error, HTTP::Error, OStatus2::BadSalmonError, Mastodon::NotPermittedError
+  rescue HTTP::Error, OStatus2::BadSalmonError, Mastodon::NotPermittedError
     nil
   end
 
diff --git a/app/services/resolve_remote_account_service.rb b/app/services/resolve_remote_account_service.rb
index c948243ec..e0e2ebc83 100644
--- a/app/services/resolve_remote_account_service.rb
+++ b/app/services/resolve_remote_account_service.rb
@@ -23,18 +23,19 @@ class ResolveRemoteAccountService < BaseService
 
     @webfinger = Goldfinger.finger("acct:#{uri}")
 
-    raise Goldfinger::Error, 'Missing resource links' if links_missing?
-
     confirmed_username, confirmed_domain = @webfinger.subject.gsub(/\Aacct:/, '').split('@')
 
     if confirmed_username.casecmp(@username).zero? && confirmed_domain.casecmp(@domain).zero?
       @username = confirmed_username
       @domain   = confirmed_domain
+    elsif redirected.nil?
+      return call("#{confirmed_username}@#{confirmed_domain}", update_profile, true)
     else
-      return call("#{confirmed_username}@#{confirmed_domain}", update_profile, true) if redirected.nil?
-      raise Goldfinger::Error, 'Requested and returned acct URIs do not match'
+      Rails.logger.debug 'Requested and returned acct URIs do not match'
+      return
     end
 
+    return if links_missing?
     return Account.find_local(@username) if TagManager.instance.local_domain?(@domain)
 
     RedisLock.acquire(lock_options) do |lock|
@@ -49,6 +50,9 @@ class ResolveRemoteAccountService < BaseService
     end
 
     @account
+  rescue Goldfinger::Error => e
+    Rails.logger.debug "Webfinger query for #{uri} unsuccessful: #{e}"
+    nil
   end
 
   private
@@ -57,7 +61,9 @@ class ResolveRemoteAccountService < BaseService
     @webfinger.link('http://schemas.google.com/g/2010#updates-from').nil? ||
       @webfinger.link('salmon').nil? ||
       @webfinger.link('http://webfinger.net/rel/profile-page').nil? ||
-      @webfinger.link('magic-public-key').nil?
+      @webfinger.link('magic-public-key').nil? ||
+      canonical_uri.nil? ||
+      hub_url.nil?
   end
 
   def webfinger_update_due?
@@ -123,19 +129,14 @@ class ResolveRemoteAccountService < BaseService
       author_uri = owner.at_xpath('./xmlns:uri') unless owner.nil?
     end
 
-    raise Goldfinger::Error, 'Author URI could not be found' if author_uri.nil?
-
-    @canonical_uri = author_uri.content
+    @canonical_uri = author_uri.nil? ? nil : author_uri.content
   end
 
   def hub_url
     return @hub_url if defined?(@hub_url)
 
-    hubs = atom.xpath('//xmlns:link[@rel="hub"]')
-
-    raise Goldfinger::Error, 'No PubSubHubbub hubs found' if hubs.empty? || hubs.first['href'].nil?
-
-    @hub_url = hubs.first['href']
+    hubs     = atom.xpath('//xmlns:link[@rel="hub"]')
+    @hub_url = hubs.empty? || hubs.first['href'].nil? ? nil : hubs.first['href']
   end
 
   def atom_body
@@ -143,7 +144,7 @@ class ResolveRemoteAccountService < BaseService
 
     response = Request.new(:get, atom_url).perform
 
-    raise Goldfinger::Error, "Feed attempt failed for #{atom_url}: HTTP #{response.code}" unless response.code == 200
+    raise Mastodon::UnexpectedResponseError, response unless response.code == 200
 
     @atom_body = response.to_s
   end
diff --git a/app/services/send_interaction_service.rb b/app/services/send_interaction_service.rb
index ab0d3aeed..c11813abc 100644
--- a/app/services/send_interaction_service.rb
+++ b/app/services/send_interaction_service.rb
@@ -14,7 +14,7 @@ class SendInteractionService < BaseService
 
     delivery = build_request.perform
 
-    raise "Delivery failed for #{target_account.salmon_url}: HTTP #{delivery.code}" unless delivery.code > 199 && delivery.code < 300
+    raise Mastodon::UnexpectedResponseError, delivery unless delivery.code > 199 && delivery.code < 300
   end
 
   private
diff --git a/app/services/subscribe_service.rb b/app/services/subscribe_service.rb
index c1c0a4c8b..d3e41e691 100644
--- a/app/services/subscribe_service.rb
+++ b/app/services/subscribe_service.rb
@@ -18,7 +18,7 @@ class SubscribeService < BaseService
     else
       # The response was either a 429 rate limit, or a 5xx error.
       # We need to retry at a later time. Fail loudly!
-      raise "Subscription attempt failed for #{@account.acct} (#{@account.hub_url}): HTTP #{@response.code}"
+      raise Mastodon::UnexpectedResponseError, @response
     end
   end
 
diff --git a/app/workers/import_worker.rb b/app/workers/import_worker.rb
index 90a226206..27cc6b365 100644
--- a/app/workers/import_worker.rb
+++ b/app/workers/import_worker.rb
@@ -44,7 +44,7 @@ class ImportWorker
         target_account = ResolveRemoteAccountService.new.call(row.first)
         next if target_account.nil?
         MuteService.new.call(from_account, target_account)
-      rescue Goldfinger::Error, HTTP::Error, OpenSSL::SSL::SSLError
+      rescue Mastodon::UnexpectedResponseError, HTTP::Error, OpenSSL::SSL::SSLError
         next
       end
     end
@@ -56,7 +56,7 @@ class ImportWorker
         target_account = ResolveRemoteAccountService.new.call(row.first)
         next if target_account.nil?
         BlockService.new.call(from_account, target_account)
-      rescue Goldfinger::Error, HTTP::Error, OpenSSL::SSL::SSLError
+      rescue Mastodon::UnexpectedResponseError, HTTP::Error, OpenSSL::SSL::SSLError
         next
       end
     end
@@ -66,7 +66,7 @@ class ImportWorker
     import_rows.each do |row|
       begin
         FollowService.new.call(from_account, row.first)
-      rescue Mastodon::NotPermittedError, ActiveRecord::RecordNotFound, Goldfinger::Error, HTTP::Error, OpenSSL::SSL::SSLError
+      rescue Mastodon::NotPermittedError, ActiveRecord::RecordNotFound, Mastodon::UnexpectedResponseError, HTTP::Error, OpenSSL::SSL::SSLError
         next
       end
     end
diff --git a/app/workers/pubsubhubbub/delivery_worker.rb b/app/workers/pubsubhubbub/delivery_worker.rb
index 2e1101b93..035a59048 100644
--- a/app/workers/pubsubhubbub/delivery_worker.rb
+++ b/app/workers/pubsubhubbub/delivery_worker.rb
@@ -23,7 +23,7 @@ class Pubsubhubbub::DeliveryWorker
   def process_delivery
     payload_delivery
 
-    raise "Delivery failed for #{subscription.callback_url}: HTTP #{payload_delivery.code}" unless response_successful?
+    raise Mastodon::UnexpectedResponseError, payload_delivery unless response_successful?
 
     subscription.touch(:last_successful_delivery_at)
   end