diff --git a/CHANGELOG.md b/CHANGELOG.md index 38465b87..81904255 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## Unreleased +### Changed + +- Do not retry `firebase.messaging.UnregisteredError` exceptions for FCM relay tasks by @joeyorlando ([#3637](https://github.com/grafana/oncall/pull/3637)) + ## v1.3.83 (2024-01-08) ### Changed diff --git a/engine/apps/mobile_app/tests/test_utils.py b/engine/apps/mobile_app/tests/test_utils.py index 4dc7d974..2db4d941 100644 --- a/engine/apps/mobile_app/tests/test_utils.py +++ b/engine/apps/mobile_app/tests/test_utils.py @@ -1,5 +1,6 @@ from unittest.mock import Mock, patch +import firebase_admin.messaging import pytest from firebase_admin.exceptions import FirebaseError from requests import HTTPError @@ -59,6 +60,40 @@ def test_send_push_notification_cloud_firebase_error( mock_send_message.assert_called_once_with(mock_message) +@patch.object(FCMDevice, "send_message") +@pytest.mark.parametrize( + "ExceptionClass,exception_kwargs", + [ + (firebase_admin.messaging.UnregisteredError, {"message": "test_error_message"}), + ], +) +@pytest.mark.django_db +def test_send_push_notification_cloud_ignores_certain_errors( + mock_send_message, + settings, + make_organization_and_user, + ExceptionClass, + exception_kwargs, +): + mock_send_message.return_value = ExceptionClass(**exception_kwargs) + + # create a user and connect a mobile device + _, user = make_organization_and_user() + device = FCMDevice.objects.create(user=user, registration_id="test_device_id") + mock_message = {"foo": "bar"} + + # check FCM is contacted directly when using the cloud license + settings.LICENSE = CLOUD_LICENSE_NAME + settings.IS_OPEN_SOURCE = False + + try: + utils.send_push_notification(device, mock_message) + except Exception: + pytest.fail(f"send_push_notification should not raise an exception for {ExceptionClass.__name__} errors") + + mock_send_message.assert_called_once_with(mock_message) + + @patch("apps.mobile_app.utils._send_push_notification_to_fcm_relay", return_value="ok") @pytest.mark.django_db def test_send_push_notification_oss( diff --git a/engine/apps/mobile_app/utils.py b/engine/apps/mobile_app/utils.py index a4637aa6..fbd4db3b 100644 --- a/engine/apps/mobile_app/utils.py +++ b/engine/apps/mobile_app/utils.py @@ -5,7 +5,7 @@ import typing import requests from django.conf import settings from firebase_admin.exceptions import FirebaseError -from firebase_admin.messaging import AndroidConfig, APNSConfig, APNSPayload, Message +from firebase_admin.messaging import AndroidConfig, APNSConfig, APNSPayload, Message, UnregisteredError from requests import HTTPError from rest_framework import status @@ -19,6 +19,13 @@ if typing.TYPE_CHECKING: MAX_RETRIES = 1 if settings.DEBUG else 10 + +# UnregisteredError +# App instance was unregistered from FCM. This usually means that the token used is no longer valid and a +# new one must be used. +# +# In other words, this error occurs outside of our control and retrying will never fix it, therefore we should skip +FIREBASE_ERRORS_TO_NOT_RETRY = (UnregisteredError,) logger = logging.getLogger(__name__) logger.setLevel(logging.DEBUG) @@ -46,13 +53,15 @@ def send_message_to_fcm_device(device: "FCMDevice", message: Message) -> None: if isinstance(response, FirebaseError): logger.exception( - f"FCM error occured in mobile_app.utils.send_message_to_fcm_device\n" - f"FCMDevice info: {device}\n" - f"FirebaseError code: {response._code}\n" - f"FirebaseError cause: {response._cause}\n" - f"FirebaseError http_response: {response._http_response}\n" + f"FCM error occured in mobile_app.utils.send_message_to_fcm_device fcm_device_info={device} " + f"firebase_error_code={response._code} firebase_error_cause={response._cause} " + f"firebase_error_http_response={response._http_response}" ) + if isinstance(response, FIREBASE_ERRORS_TO_NOT_RETRY): + logger.warning(f"FCM error {response} is not being retried as we explicitly do not want to retry it") + return + raise response