diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index f613e6619..57d01889c 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -1,8 +1,5 @@ import json -from celery.exceptions import MaxRetriesExceededError - -from sqlalchemy.exc import SQLAlchemyError from datetime import datetime from monotonic import monotonic from flask import current_app @@ -11,10 +8,10 @@ from app.clients.sms import SmsClientException from app.dao.notifications_dao import ( update_provider_stats, get_notification_by_id, - dao_update_notification) + dao_update_notification, update_notification_status_by_id) from app.dao.provider_details_dao import get_provider_details_by_notification_type from app.dao.services_dao import dao_fetch_service_by_id -from app.celery.research_mode_tasks import send_email_response, send_sms_response +from app.celery.research_mode_tasks import send_sms_response from notifications_utils.recipients import ( validate_and_format_phone_number @@ -26,13 +23,20 @@ from notifications_utils.template import ( unlink_govuk_escaped ) -retry_iteration_to_delay = { - 0: 5, # 5 seconds - 1: 30, # 30 seconds - 2: 60 * 5, # 5 minutes - 3: 60 * 15, # 15 minutes - 4: 60 * 30 # 30 minutes -} + +def retry_iteration_to_delay(retry): + """ + Given current retry calculate some delay before retrying + Delay calculated as (1 + retry number) squared * 60 + 0: 60 seconds (1 minute) + 1: 240 seconds (4 minutes) + 2: 540 seconds (9 minutes) + 3: 960 seconds (16 minutes) + 4: 1500 seconds (25 minutes) + :param retry (zero indexed): + :return length to retry in seconds: + """ + return ((retry + 1) ** 2) * 60 @notify_celery.task(bind=True, name="send-sms-to-provider", max_retries=5, default_retry_delay=5) @@ -73,23 +77,22 @@ def send_sms_to_provider(self, service_id, notification_id, encrypted_notificati notification.sent_by = provider.get_name(), notification.content_char_count = template.replaced_content_count dao_update_notification(notification) - except SmsClientException as e: try: current_app.logger.error( "SMS notification {} failed".format(notification_id) ) current_app.logger.exception(e) - raise self.retry(queue="retry", countdown=retry_iteration_to_delay[self.request.retries]) + self.retry(queue="retry", countdown=retry_iteration_to_delay(self.request.retries)) except self.MaxRetriesExceededError: - notification.status = 'technical-failure' + update_notification_status_by_id(notification.id, 'technical-failure', 'failure') current_app.logger.info( "SMS {} created at {} sent at {}".format(notification_id, notification.created_at, notification.sent_at) ) statsd_client.incr("notifications.tasks.send-sms-to-provider") statsd_client.timing("notifications.tasks.send-sms-to-provider.task-time", monotonic() - task_start) - statsd_client.timing("notifications.sms.total-time", notification.sent_at - notification.created_at) + statsd_client.timing("notifications.sms.total-time", datetime.utcnow() - notification.created_at) def provider_to_use(notification_type, notification_id): diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 5fbb7fb0f..fd0446444 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -8,7 +8,6 @@ from sqlalchemy.exc import SQLAlchemyError from app import clients, statsd_client from app.clients import STATISTICS_FAILURE from app.clients.email import EmailClientException -from app.clients.sms import SmsClientException from app.dao.services_dao import dao_fetch_service_by_id from app.dao.templates_dao import dao_get_template_by_id from app.dao.provider_details_dao import get_provider_details_by_notification_type diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index a0ae9cec1..44d2440fa 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/tests/app/celery/test_provider_tasks.py @@ -1,16 +1,19 @@ -import uuid +from celery.exceptions import MaxRetriesExceededError from mock import ANY, call from app import statsd_client, mmg_client +from app.celery import provider_tasks from app.celery.provider_tasks import send_sms_to_provider from app.celery.tasks import provider_to_use +from app.clients.sms import SmsClientException from app.dao import provider_statistics_dao from datetime import datetime from freezegun import freeze_time from app.dao import notifications_dao, provider_details_dao from notifications_utils.recipients import validate_phone_number, format_phone_number from app.celery.research_mode_tasks import send_sms_response +from app.models import Notification, NotificationStatistics, Job from tests.app.conftest import ( sample_notification @@ -280,7 +283,6 @@ def test_statsd_updates(notify_db, notify_db_session, sample_service, sample_not mocker.patch('app.statsd_client.incr') mocker.patch('app.statsd_client.timing') mocker.patch('app.encryption.decrypt', return_value=notification) - mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.mmg_client.send_sms') mocker.patch('app.mmg_client.get_name', return_value="mmg") mocker.patch('app.celery.research_mode_tasks.send_sms_response.apply_async') @@ -298,6 +300,44 @@ def test_statsd_updates(notify_db, notify_db_session, sample_service, sample_not ]) +def test_should_go_into_technical_error_if_exceeds_retries( + notify_db, + notify_db_session, + sample_service, + sample_notification, + mocker): + + notification = _notification_json( + sample_notification.template, + to=sample_notification.to + ) + + mocker.patch('app.statsd_client.incr') + mocker.patch('app.statsd_client.timing') + mocker.patch('app.encryption.decrypt', return_value=notification) + mocker.patch('app.mmg_client.send_sms', side_effect=SmsClientException("EXPECTED")) + mocker.patch('app.celery.provider_tasks.send_sms_to_provider.retry', side_effect=MaxRetriesExceededError()) + + send_sms_to_provider( + sample_notification.service_id, + sample_notification.id, + "encrypted-in-reality" + ) + + provider_tasks.send_sms_to_provider.retry.assert_called_with(queue='retry', countdown=60) + assert statsd_client.incr.assert_not_called + assert statsd_client.timing.assert_not_called + + db_notification = Notification.query.get(sample_notification.id) + assert db_notification.status == 'technical-failure' + notification_stats = NotificationStatistics.query.filter_by(service_id=sample_notification.service.id).first() + assert notification_stats.sms_requested == 1 + assert notification_stats.sms_failed == 1 + job = Job.query.get(sample_notification.job.id) + assert job.notification_count == 1 + assert job.notifications_failed == 1 + + def _notification_json(template, to, personalisation=None, job_id=None, row_number=None): notification = { "template": template.id, diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 954b53db4..fd0833c29 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -2,7 +2,6 @@ import uuid import pytest from flask import current_app from mock import ANY -from notifications_utils.recipients import validate_phone_number, format_phone_number from app.celery import provider_tasks from app.celery.tasks import ( @@ -18,13 +17,9 @@ from app.celery.tasks import ( provider_to_use, timeout_notifications ) -from app.celery.research_mode_tasks import ( - send_email_response, - send_sms_response -) -from app import (aws_ses_client, encryption, DATETIME_FORMAT, mmg_client, statsd_client) +from app.celery.research_mode_tasks import send_email_response +from app import (aws_ses_client, encryption, DATETIME_FORMAT, statsd_client) from app.clients.email.aws_ses import AwsSesClientException -from app.clients.sms.mmg import MMGClientException from app.dao import notifications_dao, jobs_dao, provider_details_dao from sqlalchemy.exc import SQLAlchemyError from sqlalchemy.orm.exc import NoResultFound