diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index 7878461b9..67a8f10d1 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -5,6 +5,7 @@ from sqlalchemy.orm.exc import NoResultFound from app import notify_celery from app.config import QueueNames +from app.clients.email.aws_ses import AwsSesClientThrottlingSendRateException from app.dao import notifications_dao from app.dao.notifications_dao import update_notification_status_by_id from app.delivery import send_to_providers @@ -49,11 +50,17 @@ def deliver_email(self, notification_id): except InvalidEmailError as e: current_app.logger.exception(e) update_notification_status_by_id(notification_id, 'technical-failure') - except Exception: + except Exception as e: try: - current_app.logger.exception( - "RETRY: Email notification {} failed".format(notification_id) - ) + if isinstance(e, AwsSesClientThrottlingSendRateException): + current_app.logger.warning( + f"RETRY: Email notification {notification_id} was rate limited by SES" + ) + else: + current_app.logger.exception( + f"RETRY: Email notification {notification_id} failed" + ) + self.retry(queue=QueueNames.RETRY) except self.MaxRetriesExceededError: message = "RETRY FAILED: Max retries reached. " \ diff --git a/app/clients/email/aws_ses.py b/app/clients/email/aws_ses.py index 3b4289a97..aba39eed3 100644 --- a/app/clients/email/aws_ses.py +++ b/app/clients/email/aws_ses.py @@ -43,6 +43,10 @@ class AwsSesClientException(EmailClientException): pass +class AwsSesClientThrottlingSendRateException(AwsSesClientException): + pass + + class AwsSesClient(EmailClient): ''' Amazon SES email client. @@ -122,6 +126,11 @@ class AwsSesClient(EmailClient): to_addresses[0], e.response['Error']['Message'] )) + elif ( + e.response['Error']['Code'] == 'Throttling' + and e.response['Error']['Message'] == 'Maximum sending rate exceeded.' + ): + raise AwsSesClientThrottlingSendRateException(str(e)) else: self.statsd_client.incr("clients.ses.error") raise AwsSesClientException(str(e)) diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index 9886c348b..10764e74f 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/tests/app/celery/test_provider_tasks.py @@ -6,7 +6,7 @@ from notifications_utils.recipients import InvalidEmailError import app from app.celery import provider_tasks from app.celery.provider_tasks import deliver_sms, deliver_email -from app.clients.email.aws_ses import AwsSesClientException +from app.clients.email.aws_ses import AwsSesClientException, AwsSesClientThrottlingSendRateException from app.exceptions import NotificationTechnicalFailureException @@ -72,8 +72,17 @@ def test_should_go_into_technical_error_if_exceeds_retries_on_deliver_sms_task(s assert sample_notification.status == 'technical-failure' -def test_should_go_into_technical_error_if_exceeds_retries_on_deliver_email_task(sample_notification, mocker): - mocker.patch('app.delivery.send_to_providers.send_email_to_provider', side_effect=Exception("EXPECTED")) +@pytest.mark.parametrize( + 'exception_class', [ + Exception(), + AwsSesClientException(), + AwsSesClientThrottlingSendRateException(), + ] +) +def test_should_go_into_technical_error_if_exceeds_retries_on_deliver_email_task( + sample_notification, mocker, exception_class +): + mocker.patch('app.delivery.send_to_providers.send_email_to_provider', side_effect=exception_class) mocker.patch('app.celery.provider_tasks.deliver_email.retry', side_effect=MaxRetriesExceededError()) with pytest.raises(NotificationTechnicalFailureException) as e: @@ -105,11 +114,38 @@ def test_should_retry_and_log_exception(sample_notification, mocker): ex = ClientError(error_response=error_response, operation_name='opname') mocker.patch('app.delivery.send_to_providers.send_email_to_provider', side_effect=AwsSesClientException(str(ex))) mocker.patch('app.celery.provider_tasks.deliver_email.retry') + mock_logger_exception = mocker.patch('app.celery.tasks.current_app.logger.exception') deliver_email(sample_notification.id) assert provider_tasks.deliver_email.retry.called is True assert sample_notification.status == 'created' + assert mock_logger_exception.called + + +def test_if_ses_send_rate_throttle_then_should_retry_and_log_warning(sample_notification, mocker): + error_response = { + 'Error': { + 'Code': 'Throttling', + 'Message': 'Maximum sending rate exceeded.', + 'Type': 'Sender' + } + } + ex = ClientError(error_response=error_response, operation_name='opname') + mocker.patch( + 'app.delivery.send_to_providers.send_email_to_provider', + side_effect=AwsSesClientThrottlingSendRateException(str(ex)) + ) + mocker.patch('app.celery.provider_tasks.deliver_email.retry') + mock_logger_warning = mocker.patch('app.celery.tasks.current_app.logger.warning') + mock_logger_exception = mocker.patch('app.celery.tasks.current_app.logger.exception') + + deliver_email(sample_notification.id) + + assert provider_tasks.deliver_email.retry.called is True + assert sample_notification.status == 'created' + assert not mock_logger_exception.called + assert mock_logger_warning.called def test_send_sms_should_not_switch_providers_on_non_provider_failure( diff --git a/tests/app/clients/test_aws_ses.py b/tests/app/clients/test_aws_ses.py index 4842bbb63..17e76aed0 100644 --- a/tests/app/clients/test_aws_ses.py +++ b/tests/app/clients/test_aws_ses.py @@ -4,7 +4,7 @@ from unittest.mock import Mock, ANY from notifications_utils.recipients import InvalidEmailError from app import aws_ses_client -from app.clients.email.aws_ses import get_aws_responses, AwsSesClientException +from app.clients.email.aws_ses import get_aws_responses, AwsSesClientException, AwsSesClientThrottlingSendRateException def test_should_return_correct_details_for_delivery(): @@ -116,6 +116,48 @@ def test_send_email_raises_bad_email_as_InvalidEmailError(mocker): assert 'definitely@invalid_email.com' in str(excinfo.value) +def test_send_email_raises_send_rate_throttling_as_AwsSesClientThrottlingSendRateException(mocker): + boto_mock = mocker.patch.object(aws_ses_client, '_client', create=True) + mocker.patch.object(aws_ses_client, 'statsd_client', create=True) + error_response = { + 'Error': { + 'Code': 'Throttling', + 'Message': 'Maximum sending rate exceeded.', + 'Type': 'Sender' + } + } + boto_mock.send_email.side_effect = botocore.exceptions.ClientError(error_response, 'opname') + + with pytest.raises(AwsSesClientThrottlingSendRateException): + aws_ses_client.send_email( + source=Mock(), + to_addresses='foo@bar.com', + subject=Mock(), + body=Mock() + ) + + +def test_send_email_does_not_raise_AwsSesClientThrottlingSendRateException_if_non_send_rate_throttling(mocker): + boto_mock = mocker.patch.object(aws_ses_client, '_client', create=True) + mocker.patch.object(aws_ses_client, 'statsd_client', create=True) + error_response = { + 'Error': { + 'Code': 'Throttling', + 'Message': 'Daily message quota exceeded', + 'Type': 'Sender' + } + } + boto_mock.send_email.side_effect = botocore.exceptions.ClientError(error_response, 'opname') + + with pytest.raises(AwsSesClientException): + aws_ses_client.send_email( + source=Mock(), + to_addresses='foo@bar.com', + subject=Mock(), + body=Mock() + ) + + def test_send_email_raises_other_errs_as_AwsSesClientException(mocker): boto_mock = mocker.patch.object(aws_ses_client, '_client', create=True) mocker.patch.object(aws_ses_client, 'statsd_client', create=True)