DEV: Handle SSL errors in push notification pusher (#22771)

We're seeing unhandled errors in production when web push notifications are failing with an SSL error. This is happening for a few users, but generating a large amount of log noise due to the sheer number of notifications.

This adds handling of SSL errors in two places:

1. In FinalDestination::HTTP, this is handled the same as a timeout error, and gives a chance to recover.
2. In PushNotificationPusher. This will cause the notification to retry a number of times, and if it keeps failing, disable push notifications for the user. (Existing behaviour.)

I wanted to wrap the SSL error in e.g. WebPush::RequestError, but the gem doesn't have request error handling, so didn't want to have the freedom patch diverge from the gem as well. Instead just propagating the raw SSL error.
This commit is contained in:
Ted Johansson 2023-07-25 15:01:02 +08:00 committed by GitHub
parent 752bb29415
commit f1a43f2319
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 3 additions and 1 deletions

View File

@ -157,6 +157,8 @@ class PushNotificationPusher
end end
rescue Timeout::Error => e rescue Timeout::Error => e
handle_generic_error(subscription, e, user, endpoint, message) handle_generic_error(subscription, e, user, endpoint, message)
rescue OpenSSL::SSL::SSLError => e
handle_generic_error(subscription, e, user, endpoint, message)
end end
end end

View File

@ -23,7 +23,7 @@ class FinalDestination::HTTP < Net::HTTP
@open_timeout = remaining_time @open_timeout = remaining_time
return super return super
rescue SystemCallError, Net::OpenTimeout => e rescue OpenSSL::SSL::SSLError, SystemCallError, Net::OpenTimeout => e
debug "[FinalDestination] Error connecting to #{ip}... #{e.message}" debug "[FinalDestination] Error connecting to #{ip}... #{e.message}"
was_last_attempt = index == ips.length - 1 was_last_attempt = index == ips.length - 1
raise if was_last_attempt raise if was_last_attempt