Do not cancel PuSH subscriptions after encountering "permanent" error… (#3046)

* Do not cancel PuSH subscriptions after encountering "permanent" error response

After talking with MMN about it, turns out some servers/php setups do
return 4xx errors while rebooting, so this anti-feature that was meant
to take load off of the hub is doing more harm than good in terms of
breaking subscriptions

* Update delivery_worker.rb

diff --git a/app/workers/pubsubhubbub/delivery_worker.rb b/app/workers/pubsubhubbub/delivery_worker.rb
index 708a5fb9fc30eff887e584a8f62db20a204978c0..981838f3314e3df3e7b36de820fc1aa1c0003430 100644 (file)
--- a/app/workers/pubsubhubbub/delivery_worker.rb
+++ b/app/workers/pubsubhubbub/delivery_worker.rb
@@ -23,13 +23,9 @@ class Pubsubhubbub::DeliveryWorker
   def process_delivery
     payload_delivery
 
-    if response_successful?
-      subscription.touch(:last_successful_delivery_at)
-    elsif response_failed_permanently?
-      subscription.destroy!
-    else
-      raise "Delivery failed for #{subscription.callback_url}: HTTP #{payload_delivery.code}"
-    end
+    raise "Delivery failed for #{subscription.callback_url}: HTTP #{payload_delivery.code}" unless response_successful?
+
+    subscription.touch(:last_successful_delivery_at)
   end
 
   def payload_delivery
@@ -82,10 +78,6 @@ class Pubsubhubbub::DeliveryWorker
     OpenSSL::HMAC.hexdigest(OpenSSL::Digest.new('sha1'), subscription.secret, payload)
   end
 
-  def response_failed_permanently?
-    payload_delivery.code > 299 && payload_delivery.code < 500 && payload_delivery.code != 429
-  end
-
   def response_successful?
     payload_delivery.code > 199 && payload_delivery.code < 300
   end

diff --git a/spec/workers/pubsubhubbub/delivery_worker_spec.rb b/spec/workers/pubsubhubbub/delivery_worker_spec.rb
index ec1e319d55390c8056a7c2b4593269070cf454da..081dfa41c0e1c839e01ca838d432e117ea541ad2 100644 (file)
--- a/spec/workers/pubsubhubbub/delivery_worker_spec.rb
+++ b/spec/workers/pubsubhubbub/delivery_worker_spec.rb
@@ -22,15 +22,6 @@ describe Pubsubhubbub::DeliveryWorker do
       expect(subscription.reload.last_successful_delivery_at).to be_within(2).of(2.days.ago)
     end
 
-    it 'destroys subscription when request fails permanently' do
-      subscription = Fabricate(:subscription)
-
-      stub_request_to_respond_with(subscription, 404)
-      subject.perform(subscription.id, payload)
-
-      expect { subscription.reload }.to raise_error(ActiveRecord::RecordNotFound)
-    end
-
     it 'raises when request fails' do
       subscription = Fabricate(:subscription)