push_notifications: Fix handling of 500s from bouncer.

The comments explain the context, but we shouldn't mark our access to
push notifications as disabled incorrectly here.
This commit is contained in:
Tim Abbott 2023-12-07 12:02:35 -08:00
parent c94c194ea7
commit 19ac558d5f
3 changed files with 15 additions and 3 deletions

View File

@ -44,6 +44,7 @@ from zerver.lib.exceptions import ErrorCode, JsonableError
from zerver.lib.message import access_message, huddle_users from zerver.lib.message import access_message, huddle_users
from zerver.lib.outgoing_http import OutgoingSession from zerver.lib.outgoing_http import OutgoingSession
from zerver.lib.remote_server import ( from zerver.lib.remote_server import (
PushNotificationBouncerServerError,
send_json_to_push_bouncer, send_json_to_push_bouncer,
send_realms_only_to_push_bouncer, send_realms_only_to_push_bouncer,
send_to_push_bouncer, send_to_push_bouncer,
@ -786,6 +787,12 @@ def initialize_push_notifications() -> None:
try: try:
realms = send_realms_only_to_push_bouncer() realms = send_realms_only_to_push_bouncer()
except PushNotificationBouncerServerError: # nocoverage
# 50x errors from the bouncer cannot be addressed by the
# administrator of this server, and may be localized to
# this endpoint; don't rashly mark push notifications as
# disabled when they are likely working perfectly fine.
pass
except Exception: except Exception:
# An exception was thrown trying to ask the bouncer service whether we can send # An exception was thrown trying to ask the bouncer service whether we can send
# push notifications or not. There may be certain transient failures that we could # push notifications or not. There may be certain transient failures that we could

View File

@ -32,6 +32,10 @@ class PushNotificationBouncerRetryLaterError(JsonableError):
http_status_code = 502 http_status_code = 502
class PushNotificationBouncerServerError(PushNotificationBouncerRetryLaterError):
http_status_code = 502
class RealmDataForAnalytics(BaseModel): class RealmDataForAnalytics(BaseModel):
model_config = ConfigDict(extra="forbid") model_config = ConfigDict(extra="forbid")
@ -114,7 +118,7 @@ def send_to_push_bouncer(
# to the callers that the attempt failed and they can retry. # to the callers that the attempt failed and they can retry.
error_msg = "Received 500 from push notification bouncer" error_msg = "Received 500 from push notification bouncer"
logging.warning(error_msg) logging.warning(error_msg)
raise PushNotificationBouncerRetryLaterError(error_msg) raise PushNotificationBouncerServerError(error_msg)
elif res.status_code >= 400: elif res.status_code >= 400:
# If JSON parsing errors, just let that exception happen # If JSON parsing errors, just let that exception happen
result_dict = orjson.loads(res.content) result_dict = orjson.loads(res.content)

View File

@ -62,6 +62,7 @@ from zerver.lib.push_notifications import (
from zerver.lib.remote_server import ( from zerver.lib.remote_server import (
PushNotificationBouncerError, PushNotificationBouncerError,
PushNotificationBouncerRetryLaterError, PushNotificationBouncerRetryLaterError,
PushNotificationBouncerServerError,
build_analytics_data, build_analytics_data,
get_realms_info_for_push_bouncer, get_realms_info_for_push_bouncer,
send_analytics_to_push_bouncer, send_analytics_to_push_bouncer,
@ -906,7 +907,7 @@ class PushBouncerNotificationTest(BouncerTestCase):
with responses.RequestsMock() as resp, self.assertLogs(level="WARNING") as warn_log: with responses.RequestsMock() as resp, self.assertLogs(level="WARNING") as warn_log:
resp.add(responses.POST, URL, body=orjson.dumps({"msg": "error"}), status=500) resp.add(responses.POST, URL, body=orjson.dumps({"msg": "error"}), status=500)
with self.assertRaisesRegex( with self.assertRaisesRegex(
PushNotificationBouncerRetryLaterError, PushNotificationBouncerServerError,
r"Received 500 from push notification bouncer$", r"Received 500 from push notification bouncer$",
): ):
self.client_post(endpoint, {"token": token, **appid}, subdomain="zulip") self.client_post(endpoint, {"token": token, **appid}, subdomain="zulip")
@ -3492,7 +3493,7 @@ class TestSendToPushBouncer(ZulipTestCase):
def test_500_error(self) -> None: def test_500_error(self) -> None:
self.add_mock_response(status=500) self.add_mock_response(status=500)
with self.assertLogs(level="WARNING") as m: with self.assertLogs(level="WARNING") as m:
with self.assertRaises(PushNotificationBouncerRetryLaterError): with self.assertRaises(PushNotificationBouncerServerError):
send_to_push_bouncer("POST", "register", {"data": "true"}) send_to_push_bouncer("POST", "register", {"data": "true"})
self.assertEqual(m.output, ["WARNING:root:Received 500 from push notification bouncer"]) self.assertEqual(m.output, ["WARNING:root:Received 500 from push notification bouncer"])