Change handling of handle conflicts to invalidate known-outdated account handle

app/models/account.rb

@@ -253,14 +253,28 @@ class Account < ApplicationRecord
     "acct:#{local_username_and_domain}"
   end
 
+  def invalidate_username!
+    return if invalidated_username?
+
+    # This uses some characters (`{`, `}`, `!`) that we are extremely unlikely to allow
+    # i nthe future, in order to ensure this will never match a webfinger handle.
+    # Using the local ID ensures we won't have any conflict.
+
+    update_attribute(:username, "{invalid!#{id}}")
+  end
+
+  def invalidated_username?
+    username.start_with?('{invalid!')
+  end
+
   def possibly_stale?
-    last_webfingered_at.nil? || last_webfingered_at <= STALE_THRESHOLD.ago
+    last_webfingered_at.nil? || last_webfingered_at <= STALE_THRESHOLD.ago || invalidated_username?
   end
 
   def needs_background_refresh?
     return false if local?
 
-    return true if last_webfingered_at.blank? || last_webfingered_at <= BACKGROUND_REFRESH_INTERVAL.ago
+    return true if last_webfingered_at.blank? || last_webfingered_at <= BACKGROUND_REFRESH_INTERVAL.ago || invalidated_username?
 
     # TODO: Remove some time after 4.6
     # This is temporary workaround to speed up account refreshs after

app/services/activitypub/process_account_service.rb

@@ -114,15 +114,23 @@ class ActivityPub::ProcessAccountService < BaseService
       @account.update!(username: @username, domain: @domain)
     rescue ActiveRecord::RecordInvalid, ActiveRecord::RecordNotUnique
       # This account (identified by ActivityPub `id`) is being renamed to a handle that was
-      # previously known by this Mastodon server as a different account… ignore the renaming for now
+      # previously known by this Mastodon server as a different account…
 
-      # TODO: better handle this scenario, by e.g. renaming the user to something unique
-      # and scheduling re-discovery
+      rename_conflicting_account!
 
-      @account.restore_attributes([:username, :domain])
+      retry
     end
   end
 
+  def rename_conflicting_account!
+    conflicting_account = Account.find_remote(@username, @domain)
+    return if conflicting_account.nil? || conflicting_account.local? || conflicting_account.uri == @account.uri
+
+    conflicting_account.invalidate_username!
+
+    AccountRefreshWorker.perform_async(conflicting_account.id, { 'request_id' => @request_id })
+  end
+
   def extract_username_and_domain!
     # FEP-2c59 defines a `webfinger` attribute that makes things more explicit and spares an extra request in some cases.
     # It supersedes `preferredUsername`.

app/services/resolve_account_service.rb

@@ -13,12 +13,15 @@ class ResolveAccountService < BaseService
   # @option options [Boolean] :skip_webfinger Do not attempt any webfinger query or refreshing account data
   # @option options [Boolean] :skip_cache Get the latest data from origin even if cache is not due to update yet
   # @option options [Boolean] :suppress_errors When failing, return nil instead of raising an error
+  # @option options [String]  :request_id Used to limit the number of HTTP requests issued from a single outside request
   # @return [Account]
   def call(uri, options = {})
     return if uri.blank?
 
     process_options!(uri, options)
 
+    return ActivityPub::FetchRemoteAccountService.new.call(@account.uri, suppress_errors: @options[:suppress_errors], request_id: options[:request_id]) if @account&.remote? && @account.invalidated_username?
+
     # First of all we want to check if we've got the account
     # record with the URI already, and if so, we can exit early
 

app/workers/account_refresh_worker.rb

@@ -5,10 +5,10 @@ class AccountRefreshWorker
 
   sidekiq_options queue: 'pull', retry: 3, dead: false, lock: :until_executed, lock_ttl: 1.day.to_i
 
-  def perform(account_id)
+  def perform(account_id, options = {})
     account = Account.find_by(id: account_id)
     return unless account&.needs_background_refresh?
 
-    ResolveAccountService.new.call(account)
+    ResolveAccountService.new.call(account, { request_id: options['request_id'] })
   end
 end

spec/services/activitypub/process_account_service_spec.rb

@@ -496,14 +496,17 @@ RSpec.describe ActivityPub::ProcessAccountService do
     context 'when the destination handle is already occupied' do
       let!(:conflicting_account) { Fabricate(:remote_account, username: 'alice', domain: 'example.com', uri: 'https://foo.test/original_alice', inbox_url: 'https://foo.test/original_alice/inbox') }
 
-      it 'updates the profile but does not touch the usernames or call AccountMergingWorker' do
+      it 'updates the profile without creating a new one or calling AccountMergingWorker, renames conflicting and schedules refresh' do
         expect { subject.call(payload) }
-          .to not_change { account.reload.username }
-          .and not_change { account.reload.domain }
+          .to change { account.reload.username }.from('bob').to('alice')
+          .and change { account.reload.domain }.from('foo.test').to('example.com')
           .and not_change { account.reload.uri }
-          .and not_change { conflicting_account.reload.acct }
+          .and change { conflicting_account.reload.username }.from('alice').to("{invalid!#{conflicting_account.id}}")
           .and(not_change { Account.count })
 
+        expect(AccountRefreshWorker)
+          .to have_enqueued_sidekiq_job(conflicting_account.id, { 'request_id' => /.*-.*@.*/ })
+
         expect(AccountMergingWorker)
           .to_not have_enqueued_sidekiq_job
 
@@ -575,14 +578,17 @@ RSpec.describe ActivityPub::ProcessAccountService do
     context 'when the destination handle is already occupied' do
       let!(:conflicting_account) { Fabricate(:remote_account, username: 'alice', domain: 'foo.test', uri: 'https://foo.test/original_alice', inbox_url: 'https://foo.test/original_alice/inbox') }
 
-      it 'updates the profile but does not touch the usernames or call AccountMergingWorker' do
+      it 'updates the profile without creating a new one or calling AccountMergingWorker, renames conflicting and schedules refresh' do
         expect { subject.call(payload) }
-          .to not_change { account.reload.username }
+          .to change { account.reload.username }.from('bob').to('alice')
           .and not_change { account.reload.domain }
           .and not_change { account.reload.uri }
-          .and not_change { conflicting_account.reload.acct }
+          .and change { conflicting_account.reload.username }.from('alice').to("{invalid!#{conflicting_account.id}}")
           .and(not_change { Account.count })
 
+        expect(AccountRefreshWorker)
+          .to have_enqueued_sidekiq_job(conflicting_account.id, { 'request_id' => /.*-.*@.*/ })
+
         expect(AccountMergingWorker)
           .to_not have_enqueued_sidekiq_job