From 87cf4d44804314d84c81e2f09f1ae10b083358a2 Mon Sep 17 00:00:00 2001 From: Kyle MacMillan <16893311+k-macmillan@users.noreply.github.com> Date: Mon, 21 Aug 2023 17:05:24 -0400 Subject: [PATCH] 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 | 7 +++++-- app/notifications/validators.py | 6 +++--- tests/app/celery/test_provider_tasks.py | 23 ++++++++++++++++++++++ tests/app/notifications/test_validators.py | 4 ++-- 4 files changed, 33 insertions(+), 7 deletions(-) diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index cbb5e1de47..30ebc9ecdf 100644 --- a/app/celery/provider_tasks.py +++ b/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: diff --git a/app/notifications/validators.py b/app/notifications/validators.py index 52640d241d..172aaff019 100644 --- a/app/notifications/validators.py +++ b/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 diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index 6800774d3e..85007d36ec 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/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 diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index d70fdfc3c5..f36efbcd4d 100644 --- a/tests/app/notifications/test_validators.py +++ b/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 )