From f4ca116ea8f86057e91c99a1cd8e64e116c86746 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Fri, 29 Sep 2017 03:16:20 +0200 Subject: [PATCH] After 7 days of repeated delivery failures, give up on inbox (#5131) - A successful delivery cancels it out - An incoming delivery from account of the inbox cancels it out --- .../activitypub/inboxes_controller.rb | 1 + app/lib/delivery_failure_tracker.rb | 56 +++++++++++++++ app/models/account.rb | 3 +- app/workers/activitypub/delivery_worker.rb | 7 ++ spec/lib/delivery_failure_tracker_spec.rb | 71 +++++++++++++++++++ 5 files changed, 137 insertions(+), 1 deletion(-) create mode 100644 app/lib/delivery_failure_tracker.rb create mode 100644 spec/lib/delivery_failure_tracker_spec.rb diff --git a/app/controllers/activitypub/inboxes_controller.rb b/app/controllers/activitypub/inboxes_controller.rb index b37910b36..d0f8073ed 100644 --- a/app/controllers/activitypub/inboxes_controller.rb +++ b/app/controllers/activitypub/inboxes_controller.rb @@ -32,6 +32,7 @@ class ActivityPub::InboxesController < Api::BaseController end Pubsubhubbub::UnsubscribeWorker.perform_async(signed_request_account.id) if signed_request_account.subscribed? + DeliveryFailureTracker.track_inverse_success!(signed_request_account) end def process_payload diff --git a/app/lib/delivery_failure_tracker.rb b/app/lib/delivery_failure_tracker.rb new file mode 100644 index 000000000..8d3be35de --- /dev/null +++ b/app/lib/delivery_failure_tracker.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +class DeliveryFailureTracker + FAILURE_DAYS_THRESHOLD = 7 + + def initialize(inbox_url) + @inbox_url = inbox_url + end + + def track_failure! + Redis.current.sadd(exhausted_deliveries_key, today) + Redis.current.sadd('unavailable_inboxes', @inbox_url) if reached_failure_threshold? + end + + def track_success! + Redis.current.del(exhausted_deliveries_key) + Redis.current.srem('unavailable_inboxes', @inbox_url) + end + + def days + Redis.current.scard(exhausted_deliveries_key) || 0 + end + + class << self + def filter(arr) + arr.reject(&method(:unavailable?)) + end + + def unavailable?(url) + Redis.current.sismember('unavailable_inboxes', url) + end + + def available?(url) + !unavailable?(url) + end + + def track_inverse_success!(from_account) + new(from_account.inbox_url).track_success! if from_account.inbox_url.present? + new(from_account.shared_inbox_url).track_success! if from_account.shared_inbox_url.present? + end + end + + private + + def exhausted_deliveries_key + "exhausted_deliveries:#{@inbox_url}" + end + + def today + Time.now.utc.strftime('%Y%m%d') + end + + def reached_failure_threshold? + days >= FAILURE_DAYS_THRESHOLD + end +end diff --git a/app/models/account.rb b/app/models/account.rb index ce7773b4b..54035d94a 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -190,7 +190,8 @@ class Account < ApplicationRecord end def inboxes - reorder(nil).where(protocol: :activitypub).pluck("distinct coalesce(nullif(accounts.shared_inbox_url, ''), accounts.inbox_url)") + urls = reorder(nil).where(protocol: :activitypub).pluck("distinct coalesce(nullif(accounts.shared_inbox_url, ''), accounts.inbox_url)") + DeliveryFailureTracker.filter(urls) end def triadic_closures(account, limit: 5, offset: 0) diff --git a/app/workers/activitypub/delivery_worker.rb b/app/workers/activitypub/delivery_worker.rb index 059c32813..7510b1739 100644 --- a/app/workers/activitypub/delivery_worker.rb +++ b/app/workers/activitypub/delivery_worker.rb @@ -15,7 +15,10 @@ class ActivityPub::DeliveryWorker perform_request raise Mastodon::UnexpectedResponseError, @response unless response_successful? + + failure_tracker.track_success! rescue => e + failure_tracker.track_failure! raise e.class, "Delivery failed for #{inbox_url}: #{e.message}", e.backtrace[0] end @@ -34,4 +37,8 @@ class ActivityPub::DeliveryWorker def response_successful? @response.code > 199 && @response.code < 300 end + + def failure_tracker + @failure_tracker ||= DeliveryFailureTracker.new(@inbox_url) + end end diff --git a/spec/lib/delivery_failure_tracker_spec.rb b/spec/lib/delivery_failure_tracker_spec.rb new file mode 100644 index 000000000..39c8c7aaf --- /dev/null +++ b/spec/lib/delivery_failure_tracker_spec.rb @@ -0,0 +1,71 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe DeliveryFailureTracker do + subject { described_class.new('http://example.com/inbox') } + + describe '#track_success!' do + before do + subject.track_failure! + subject.track_success! + end + + it 'marks URL as available again' do + expect(described_class.available?('http://example.com/inbox')).to be true + end + + it 'resets days to 0' do + expect(subject.days).to be_zero + end + end + + describe '#track_failure!' do + it 'marks URL as unavailable after 7 days of being called' do + 6.times { |i| Redis.current.sadd('exhausted_deliveries:http://example.com/inbox', i) } + subject.track_failure! + + expect(subject.days).to eq 7 + expect(described_class.unavailable?('http://example.com/inbox')).to be true + end + + it 'repeated calls on the same day do not count' do + subject.track_failure! + subject.track_failure! + + expect(subject.days).to eq 1 + end + end + + describe '.filter' do + before do + Redis.current.sadd('unavailable_inboxes', 'http://example.com/unavailable/inbox') + end + + it 'removes URLs that are unavailable' do + result = described_class.filter(['http://example.com/good/inbox', 'http://example.com/unavailable/inbox']) + + expect(result).to include('http://example.com/good/inbox') + expect(result).to_not include('http://example.com/unavailable/inbox') + end + end + + describe '.track_inverse_success!' do + let(:from_account) { Fabricate(:account, inbox_url: 'http://example.com/inbox', shared_inbox_url: 'http://example.com/shared/inbox') } + + before do + Redis.current.sadd('unavailable_inboxes', 'http://example.com/inbox') + Redis.current.sadd('unavailable_inboxes', 'http://example.com/shared/inbox') + + described_class.track_inverse_success!(from_account) + end + + it 'marks inbox URL as available again' do + expect(described_class.available?('http://example.com/inbox')).to be true + end + + it 'marks shared inbox URL as available again' do + expect(described_class.available?('http://example.com/shared/inbox')).to be true + end + end +end