FIX: automatically expire bad push channels (#13156)

Previously we would retry push notifications indefinitely for all errors
except for ExpiredSubscription

Under certain conditions other persistent errors may arise such as a persistent
rate limit.

If we track more than 3 errors in a period of time longer than a day we will
delete the subscription

Also performs a bit of internal cleanup to ensure protected methods really
are private.

app/models/push_subscription.rb

@@ -2,15 +2,21 @@
 
 class PushSubscription < ActiveRecord::Base
   belongs_to :user
+
+  def parsed_data
+    JSON.parse(data)
+  end
 end
 
 # == Schema Information
 #
 # Table name: push_subscriptions
 #
-#  id         :bigint           not null, primary key
-#  user_id    :integer          not null
-#  data       :string           not null
-#  created_at :datetime         not null
-#  updated_at :datetime         not null
+#  id            :bigint           not null, primary key
+#  user_id       :integer          not null
+#  data          :string           not null
+#  created_at    :datetime         not null
+#  updated_at    :datetime         not null
+#  error_count   :integer          default(0), not null
+#  first_error_at :datetime
 #

app/services/push_notification_pusher.rb

@@ -22,7 +22,6 @@ class PushNotificationPusher
       }
 
       subscriptions(user).each do |subscription|
-        subscription = JSON.parse(subscription.data)
         send_notification(user, subscription, message)
       end
     end
@@ -66,8 +65,6 @@ class PushNotificationPusher
     PushSubscription.find_by(user: user, data: subscription.to_json)&.destroy!
   end
 
-  protected
-
   def self.get_badge
     if (url = SiteSetting.site_push_notifications_icon_url).present?
       url
@@ -76,13 +73,30 @@ class PushNotificationPusher
     end
   end
 
+  MAX_ERRORS ||= 3
+  MIN_ERROR_DURATION ||= 86400 # 1 day
+
+  def self.handle_generic_error(subscription)
+    subscription.error_count += 1
+    subscription.first_error_at ||= Time.zone.now
+
+    delta = Time.zone.now - subscription.first_error_at
+    if subscription.error_count >= MAX_ERRORS && delta > MIN_ERROR_DURATION
+      subscription.destroy!
+    else
+      subscription.save!
+    end
+  end
+
   def self.send_notification(user, subscription, message)
-    endpoint = subscription["endpoint"]
-    p256dh = subscription.dig("keys", "p256dh")
-    auth = subscription.dig("keys", "auth")
+    parsed_data = subscription.parsed_data
+
+    endpoint = parsed_data["endpoint"]
+    p256dh = parsed_data.dig("keys", "p256dh")
+    auth = parsed_data.dig("keys", "auth")
 
     if (endpoint.blank? || p256dh.blank? || auth.blank?)
-      unsubscribe(user, subscription)
+      subscription.destroy!
       return
     end
 
@@ -99,22 +113,31 @@ class PushNotificationPusher
           expiration: TOKEN_VALID_FOR_SECONDS
         }
       )
+
+      if subscription.first_error_at || subscription.error_count != 0
+        subscription.update_columns(error_count: 0, first_error_at: nil)
+      end
     rescue Webpush::ExpiredSubscription
-      unsubscribe(user, subscription)
+      subscription.destroy!
     rescue Webpush::ResponseError => e
       if e.response.message == "MismatchSenderId"
-        unsubscribe(user, subscription)
+        subscription.destroy!
       else
+        handle_generic_error(subscription)
         Discourse.warn_exception(
           e,
           message: "Failed to send push notification",
           env: {
             user_id: user.id,
-            endpoint: subscription["endpoint"],
+            endpoint: endpoint,
             message: message.to_json
           }
         )
       end
     end
   end
+
+  private_class_method :send_notification
+  private_class_method :handle_generic_error
+
 end

db/migrate/20210526053611_add_error_count_to_push_subscriptions.rb

@@ -0,0 +1,8 @@
+# frozen_string_literal: true
+
+class AddErrorCountToPushSubscriptions < ActiveRecord::Migration[6.1]
+  def change
+    add_column :push_subscriptions, :error_count, :integer, null: false, default: 0
+    add_column :push_subscriptions, :first_error_at, :datetime
+  end
+end

spec/services/push_notification_pusher_spec.rb

@@ -19,18 +19,20 @@ RSpec.describe PushNotificationPusher do
   it "sends notification in user's locale" do
     SiteSetting.allow_user_locale = true
     user = Fabricate(:user, locale: 'pt_BR')
-    PushSubscription.create!(user_id: user.id, data: "{\"endpoint\": \"endpoint\"}")
+    data = <<~JSON
+      {
+        "endpoint": "endpoint",
+        "keys": {
+          "p256dh": "p256dh",
+          "auth": "auth"
+        }
+      }
+    JSON
+    PushSubscription.create!(user_id: user.id, data: data)
 
-    PushNotificationPusher.expects(:send_notification).with(user, { "endpoint" => "endpoint" }, {
-      title: "system mencionou você em \"Topic\" - Discourse",
-      body: "description",
-      badge: "/assets/push-notifications/discourse.png",
-      icon: "/assets/push-notifications/mentioned.png",
-      tag: "test.localhost-1",
-      base_url: "http://test.localhost",
-      url: "https://example.com/t/1/2",
-      hide_when_active: true
-    }).once
+    Webpush.expects(:payload_send).with do |*args|
+      args.to_s.include?("system mencionou")
+    end.once
 
     PushNotificationPusher.push(user, {
       topic_title: 'Topic',
@@ -42,6 +44,60 @@ RSpec.describe PushNotificationPusher do
     })
   end
 
+  it "deletes subscriptions which are erroring regularly" do
+    start = freeze_time
+
+    user = Fabricate(:user)
+
+    data = <<~JSON
+      {
+        "endpoint": "endpoint",
+        "keys": {
+          "p256dh": "p256dh",
+          "auth": "auth"
+        }
+      }
+    JSON
+
+    sub = PushSubscription.create!(user_id: user.id, data: data)
+
+    response = Struct.new(:body, :inspect, :message).new("test", "test", "failed")
+    error = Webpush::ResponseError.new(response, "localhost")
+
+    Webpush.expects(:payload_send).raises(error).times(4)
+
+    # 3 failures in more than 24 hours
+    3.times do
+      PushNotificationPusher.push(user, {
+        topic_title: 'Topic',
+        username: 'system',
+        excerpt: 'description',
+        topic_id: 1,
+        post_url: "https://example.com/t/1/2",
+        notification_type: 1
+      })
+
+      freeze_time 1.minute.from_now
+    end
+
+    sub.reload
+    expect(sub.error_count).to eq(3)
+    expect(sub.first_error_at).to eq_time(start)
+
+    freeze_time(2.days.from_now)
+
+    PushNotificationPusher.push(user, {
+      topic_title: 'Topic',
+      username: 'system',
+      excerpt: 'description',
+      topic_id: 1,
+      post_url: "https://example.com/t/1/2",
+      notification_type: 1
+    })
+
+    expect(PushSubscription.where(id: sub.id).exists?).to eq(false)
+  end
+
   it "deletes invalid subscriptions during send" do
     user = Fabricate(:walter_white)