Hotfix AutoRetry For Rate Limiting SMS Deliveries (#1426)

This PR adds autoretry_for  to deliver_sms_with_rate_limiting and gives it up to a 2 second initial delay.

app/celery/provider_tasks.py

@@ -72,7 +72,10 @@ def deliver_sms(self, notification_id, sms_sender_id=None):
 
 
 # Including sms_sender_id is necessary in case it's passed in when being called
-@notify_celery.task(bind=True, name='deliver_sms_with_rate_limiting', max_retries=None)
+@notify_celery.task(bind=True, name='deliver_sms_with_rate_limiting',
+                    throws=(AutoRetryException, ),
+                    autoretry_for=(AutoRetryException, ),
+                    max_retries=2886, retry_backoff=2, retry_backoff_max=60)
 @statsd(namespace='tasks')
 def deliver_sms_with_rate_limiting(self, notification_id, sms_sender_id=None):
     from app.notifications.validators import check_sms_sender_over_rate_limit
@@ -88,7 +91,7 @@ def deliver_sms_with_rate_limiting(self, notification_id, sms_sender_id=None):
             )
         sms_sender = dao_get_service_sms_sender_by_service_id_and_number(notification.service_id,
                                                                          notification.reply_to_text)
-        check_sms_sender_over_rate_limit(notification.service_id, sms_sender.id)
+        check_sms_sender_over_rate_limit(notification.service_id, sms_sender)
         send_to_providers.send_sms_to_provider(notification, sms_sender_id)
         current_app.logger.info('Successfully sent sms with rate limiting for notification id: %s', notification_id)
     except InvalidProviderException as e:

app/notifications/validators.py

@@ -58,15 +58,15 @@ def check_service_over_daily_message_limit(key_type, service):
             raise TooManyRequestsError(service.message_limit)
 
 
-def check_sms_sender_over_rate_limit(service_id, sms_sender_id):
+def check_sms_sender_over_rate_limit(service_id, sms_sender):
     if (
         not is_feature_enabled(FeatureFlag.SMS_SENDER_RATE_LIMIT_ENABLED)
-        or sms_sender_id is None
+        or sms_sender is None
     ):
         current_app.logger.info('Skipping sms sender rate limit check')
         return
 
-    sms_sender = dao_get_service_sms_sender_by_id(service_id, sms_sender_id)
+    sms_sender = dao_get_service_sms_sender_by_id(service_id, sms_sender.id)
     if current_app.config['REDIS_ENABLED']:
         current_app.logger.info('Checking sms sender rate limit')
         cache_key = sms_sender.sms_sender

tests/app/celery/test_provider_tasks.py

@@ -199,3 +199,26 @@ def test_deliver_sms_with_rate_limiting_should_retry_if_rate_limit_exceeded(samp
         queue=QueueNames.RATE_LIMIT_RETRY, max_retries=None,
         countdown=sms_sender.rate_limit_interval / sms_sender.rate_limit
     )
+
+
+def test_deliver_sms_with_rate_limiting_should_retry_generic_exceptions(sample_notification, mocker):
+    mocker.patch('app.celery.provider_tasks.send_to_providers.send_sms_to_provider', side_effect=Exception)
+    mocker.patch.dict(os.environ, {'NOTIFICATION_FAILURE_REASON_ENABLED': 'True'})
+
+    with pytest.raises(AutoRetryException) as exc_info:
+        deliver_sms_with_rate_limiting(sample_notification.id)
+
+    assert exc_info.type is AutoRetryException
+
+
+def test_deliver_sms_with_rate_limiting_max_retries_exceeded(sample_notification, mocker):
+    mocker.patch('app.celery.provider_tasks.send_to_providers.send_sms_to_provider', side_effect=Exception)
+    mocker.patch('app.celery.provider_tasks.can_retry', return_value=False)
+    mocker.patch.dict(os.environ, {'NOTIFICATION_FAILURE_REASON_ENABLED': 'True'})
+
+    with pytest.raises(NotificationTechnicalFailureException) as exc_info:
+        deliver_sms_with_rate_limiting(sample_notification.id)
+
+    assert exc_info.type is NotificationTechnicalFailureException
+    assert sample_notification.status == 'technical-failure'
+    assert sample_notification.status_reason == RETRIES_EXCEEDED

tests/app/notifications/test_validators.py

@@ -510,7 +510,7 @@ def test_that_when_sms_sender_rate_exceed_rate_limit_request_fails(
             sample_service.restricted = True
 
             with pytest.raises(RateLimitError) as e:
-                check_sms_sender_over_rate_limit(sample_service, sms_sender.id)
+                check_sms_sender_over_rate_limit(sample_service, sms_sender)
 
             should_throttle.assert_called_once_with(
                 sms_sender.sms_sender, sample_service.rate_limit, 60
@@ -540,7 +540,7 @@ def test_that_when_not_exceeded_sms_sender_rate_limit_request_succeeds(self, sam
 
             sample_service.restricted = True
 
-            check_sms_sender_over_rate_limit(sample_service, sms_sender.id)
+            check_sms_sender_over_rate_limit(sample_service, sms_sender)
             should_throttle.assert_called_once_with(
                 str(sms_sender.sms_sender), 10, 60
             )