Add minute resolution to DeliveryFailureTracker (#35625)

app/lib/delivery_failure_tracker.rb

@@ -3,14 +3,18 @@
 class DeliveryFailureTracker
   include Redisable
 
-  FAILURE_DAYS_THRESHOLD = 7
+  FAILURE_THRESHOLDS = {
+    days: 7,
+    minutes: 5,
+  }.freeze
 
-  def initialize(url_or_host)
+  def initialize(url_or_host, resolution: :days)
     @host = url_or_host.start_with?('https://', 'http://') ? Addressable::URI.parse(url_or_host).normalized_host : url_or_host
+    @resolution = resolution
   end
 
   def track_failure!
-    redis.sadd(exhausted_deliveries_key, today)
+    redis.sadd(exhausted_deliveries_key, failure_time)
     UnavailableDomain.create(domain: @host) if reached_failure_threshold?
   end
 
@@ -24,6 +28,12 @@ class DeliveryFailureTracker
   end
 
   def days
+    raise TypeError, 'resolution is not in days' unless @resolution == :days
+
+    failures
+  end
+
+  def failures
     redis.scard(exhausted_deliveries_key) || 0
   end
 
@@ -32,7 +42,7 @@ class DeliveryFailureTracker
   end
 
   def exhausted_deliveries_days
-    @exhausted_deliveries_days ||= redis.smembers(exhausted_deliveries_key).sort.map { |date| Date.new(date.slice(0, 4).to_i, date.slice(4, 2).to_i, date.slice(6, 2).to_i) }
+    @exhausted_deliveries_days ||= redis.smembers(exhausted_deliveries_key).sort.map { |date| Date.new(date.slice(0, 4).to_i, date.slice(4, 2).to_i, date.slice(6, 2).to_i) }.uniq
   end
 
   alias reset! track_success!
@@ -89,11 +99,16 @@ class DeliveryFailureTracker
     "exhausted_deliveries:#{@host}"
   end
 
-  def today
+  def failure_time
-    Time.now.utc.strftime('%Y%m%d')
+    case @resolution
+    when :days
+      Time.now.utc.strftime('%Y%m%d')
+    when :minutes
+      Time.now.utc.strftime('%Y%m%d%H%M')
+    end
   end
 
   def reached_failure_threshold?
-    days >= FAILURE_DAYS_THRESHOLD
+    failures >= FAILURE_THRESHOLDS[@resolution]
   end
 end

app/views/admin/instances/show.html.haml

@@ -78,7 +78,7 @@
   %h3= t('admin.instances.availability.title')
 
   %p
-    = t('admin.instances.availability.description_html', count: DeliveryFailureTracker::FAILURE_DAYS_THRESHOLD)
+    = t('admin.instances.availability.description_html', count: DeliveryFailureTracker::FAILURE_THRESHOLDS[:days])
 
   .availability-indicator
     %ul.availability-indicator__graphic

spec/lib/delivery_failure_tracker_spec.rb

@@ -3,37 +3,101 @@
 require 'rails_helper'
 
 RSpec.describe DeliveryFailureTracker do
-  subject { described_class.new('http://example.com/inbox') }
+  context 'with the default resolution of :days' do
+    subject { described_class.new('http://example.com/inbox') }
 
-  describe '#track_success!' do
+    describe '#track_success!' do
-    before do
+      before do
-      subject.track_failure!
+        track_failure(7, :days)
-      subject.track_success!
+        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
 
-    it 'marks URL as available again' do
+    describe '#track_failure!' do
-      expect(described_class.available?('http://example.com/inbox')).to be true
+      it 'marks URL as unavailable after 7 days of being called' do
+        track_failure(7, :days)
+
+        expect(subject.days).to eq 7
+        expect(described_class.available?('http://example.com/inbox')).to be false
+      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
 
-    it 'resets days to 0' do
+    describe '#exhausted_deliveries_days' do
-      expect(subject.days).to be_zero
+      it 'returns the days on which failures were recorded' do
+        track_failure(3, :days)
+
+        expect(subject.exhausted_deliveries_days).to contain_exactly(3.days.ago.to_date, 2.days.ago.to_date, Date.yesterday)
+      end
     end
   end
 
-  describe '#track_failure!' do
+  context 'with a resolution of :minutes' do
-    it 'marks URL as unavailable after 7 days of being called' do
+    subject { described_class.new('http://example.com/inbox', resolution: :minutes) }
-      6.times { |i| redis.sadd('exhausted_deliveries:example.com', i) }
-      subject.track_failure!
 
-      expect(subject.days).to eq 7
+    describe '#track_success!' do
-      expect(described_class.available?('http://example.com/inbox')).to be false
+      before do
+        track_failure(5, :minutes)
+        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 failures to 0' do
+        expect(subject.failures).to be_zero
+      end
     end
 
-    it 'repeated calls on the same day do not count' do
+    describe '#track_failure!' do
-      subject.track_failure!
+      it 'marks URL as unavailable after 5 minutes of being called' do
-      subject.track_failure!
+        track_failure(5, :minutes)
 
-      expect(subject.days).to eq 1
+        expect(subject.failures).to eq 5
+        expect(described_class.available?('http://example.com/inbox')).to be false
+      end
+
+      it 'repeated calls within the same minute do not count' do
+        freeze_time
+        subject.track_failure!
+        subject.track_failure!
+
+        expect(subject.failures).to eq 1
+      end
+    end
+
+    describe '#exhausted_deliveries_days' do
+      it 'returns the days on which failures were recorded' do
+        # Make sure this does not accidentally span two days when run
+        # around midnight
+        travel_to Time.zone.now.change(hour: 10)
+        track_failure(3, :minutes)
+
+        expect(subject.exhausted_deliveries_days).to contain_exactly(Time.zone.today)
+      end
+    end
+
+    describe '#days' do
+      it 'raises due to wrong resolution' do
+        assert_raises TypeError do
+          subject.days
+        end
+      end
     end
   end
 
@@ -60,4 +124,12 @@ RSpec.describe DeliveryFailureTracker do
       expect(described_class.available?('http://foo.bar/inbox')).to be true
     end
   end
+
+  def track_failure(times, unit)
+    times.times do
+      travel_to 1.send(unit).ago
+      subject.track_failure!
+    end
+    travel_back
+  end
 end