FIX: Remove expired subscription for push notifications

approved

#1

FIX: Remove expired subscription for push notifications

All other errors get logged, but do not stop the system from sending further push notifications.

diff --git a/app/services/push_notification_pusher.rb b/app/services/push_notification_pusher.rb
index d397e13..3a63378 100644
--- a/app/services/push_notification_pusher.rb
+++ b/app/services/push_notification_pusher.rb
@@ -2,25 +2,25 @@ require_dependency 'webpush'
 
 class PushNotificationPusher
   def self.push(user, payload)
+    message = {
+      title: I18n.t(
+        "discourse_push_notifications.popup.#{Notification.types[payload[:notification_type]]}",
+        site_title: SiteSetting.title,
+        topic: payload[:topic_title],
+        username: payload[:username]
+      ),
+      body: payload[:excerpt],
+      badge: get_badge,
+      icon: ActionController::Base.helpers.image_url("push-notifications/#{Notification.types[payload[:notification_type]]}.png"),
+      tag: "#{Discourse.current_hostname}-#{payload[:topic_id]}",
+      base_url: Discourse.base_url,
+      url: payload[:post_url],
+      hide_when_active: true
+    }
+
     subscriptions(user).each do |subscription|
       subscription = JSON.parse(subscription.data)
-
-      message = {
-        title: I18n.t(
-          "discourse_push_notifications.popup.#{Notification.types[payload[:notification_type]]}",
-          site_title: SiteSetting.title,
-          topic: payload[:topic_title],
-          username: payload[:username]
-        ),
-        body: payload[:excerpt],
-        badge: get_badge,
-        icon: ActionController::Base.helpers.image_url("push-notifications/#{Notification.types[payload[:notification_type]]}.png"),
-        tag: "#{Discourse.current_hostname}-#{payload[:topic_id]}",
-        base_url: Discourse.base_url,
-        url: payload[:post_url],
-        hide_when_active: true
-      }
-      send_notification user, subscription, message
+      send_notification(user, subscription, message)
     end
   end
 
@@ -54,7 +54,7 @@ class PushNotificationPusher
         tag: "#{Discourse.current_hostname}-subscription"
       }
 
-      send_notification user, subscription, message
+      send_notification(user, subscription, message)
     end
   end
 
@@ -74,7 +74,7 @@ class PushNotificationPusher
 
   def self.send_notification(user, subscription, message)
     begin
-      response = Webpush.payload_send(
+      Webpush.payload_send(
         endpoint: subscription["endpoint"],
         message: message.to_json,
         p256dh: subscription.dig("keys", "p256dh"),
@@ -85,8 +85,18 @@ class PushNotificationPusher
           private_key: SiteSetting.vapid_private_key
         }
       )
-    rescue Webpush::InvalidSubscription => e
-      unsubscribe user, subscription
+    rescue Webpush::ExpiredSubscription
+      unsubscribe(user, subscription)
+    rescue Webpush::ResponseError => e
+      Discourse.warn_exception(
+        e,
+        message: "Failed to send push notification",
+        env: {
+          user_id: user.id,
+          endpoint: subscription["endpoint"],
+          message: message.to_json
+        }
+      )
     end
   end
 end

GitHub sha: 978cc0cf


#2

So Webpush::InvalidSubscription is not a valid exception anymore?


#3

#4

InvalidSubscription is a ResponseError, so it will be logged. I don’t think we want to ignore that exception and silently remove the subscription anymore when an InvalidSubscription is raised. That was the reason why we didn’t pick up on the fact that something was wrong with push notifications right away.

I’m going to watch our logs closely for warnings about push notifications in the next few weeks just to make sure that we really don’t need to handle any of the other exceptions raised by webpush. But I think we are good here.