diff --git a/app/celery/callback_tasks.py b/app/celery/callback_tasks.py index 3de4b746e..6f59a33f5 100644 --- a/app/celery/callback_tasks.py +++ b/app/celery/callback_tasks.py @@ -13,6 +13,6 @@ def process_ses_results(self, response): errors = process_ses_response(response) if errors: current_app.logger.error(errors) - except Exception: + except Exception as exc: current_app.logger.exception('Error processing SES results') - self.retry(queue=QueueNames.RETRY, exc="SES responses processed with error") + self.retry(queue=QueueNames.RETRY) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index e96d7e3d8..3a146cdea 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -18,6 +18,8 @@ from requests import ( RequestException ) from sqlalchemy.exc import SQLAlchemyError +from botocore.exceptions import ClientError as BotoClientError + from app import ( create_uuid, create_random_identifier, @@ -323,12 +325,13 @@ def build_dvla_file(self, job_id): ) dao_update_job_status(job_id, JOB_STATUS_READY_TO_SEND) else: - current_app.logger.info("All notifications for job {} are not persisted".format(job_id)) - self.retry(queue=QueueNames.RETRY, exc="All notifications for job {} are not persisted".format(job_id)) - except Exception as e: - # ? should this retry? + msg = "All notifications for job {} are not persisted".format(job_id) + current_app.logger.info(msg) + self.retry(queue=QueueNames.RETRY) + # specifically don't catch celery.retry errors + except (SQLAlchemyError, BotoClientError): current_app.logger.exception("build_dvla_file threw exception") - raise e + self.retry(queue=QueueNames.RETRY) @notify_celery.task(bind=True, name='update-letter-job-to-sent') @@ -520,9 +523,7 @@ def send_inbound_sms_to_service(self, inbound_sms_id, service_id): ) if not isinstance(e, HTTPError) or e.response.status_code >= 500: try: - self.retry(queue=QueueNames.RETRY, - exc='Unable to send_inbound_sms_to_service for service_id: {} and url: {}. \n{}'.format( - service_id, inbound_api.url, e)) + self.retry(queue=QueueNames.RETRY) except self.MaxRetriesExceededError: current_app.logger.exception('Retry: send_inbound_sms_to_service has retried the max number of times') diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index b3a046897..ad516a5e4 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -1,16 +1,17 @@ -import codecs import json import uuid from datetime import datetime, timedelta from unittest.mock import Mock + import pytest import requests_mock from flask import current_app from freezegun import freeze_time from requests import RequestException from sqlalchemy.exc import SQLAlchemyError -from notifications_utils.template import SMSMessageTemplate, WithSubjectTemplate, LetterDVLATemplate from celery.exceptions import Retry +from botocore.exceptions import ClientError +from notifications_utils.template import SMSMessageTemplate, WithSubjectTemplate, LetterDVLATemplate from app import (encryption, DATETIME_FORMAT) from app.celery import provider_tasks @@ -1024,9 +1025,7 @@ def test_build_dvla_file(sample_letter_template, mocker): create_notification(template=job.template, job=job) mocked_upload = mocker.patch("app.celery.tasks.s3upload") mocked_send_task = mocker.patch("app.celery.tasks.notify_celery.send_task") - mocked_letter_template = mocker.patch("app.celery.tasks.LetterDVLATemplate") - mocked_letter_template_instance = mocked_letter_template.return_value - mocked_letter_template_instance.__str__.return_value = "dvla|string" + mocker.patch("app.celery.tasks.LetterDVLATemplate", return_value='dvla|string') build_dvla_file(job.id) mocked_upload.assert_called_once_with( @@ -1049,12 +1048,25 @@ def test_build_dvla_file_retries_if_all_notifications_are_not_created(sample_let build_dvla_file(job.id) mocked.assert_not_called() - tasks.build_dvla_file.retry.assert_called_with(queue="retry-tasks", - exc="All notifications for job {} are not persisted".format(job.id)) + tasks.build_dvla_file.retry.assert_called_with(queue="retry-tasks") assert Job.query.get(job.id).job_status == 'in progress' mocked_send_task.assert_not_called() +def test_build_dvla_file_retries_if_s3_err(sample_letter_template, mocker): + job = create_job(sample_letter_template, notification_count=1) + create_notification(job.template, job=job) + + mocker.patch('app.celery.tasks.LetterDVLATemplate', return_value='dvla|string') + mocker.patch('app.celery.tasks.s3upload', side_effect=ClientError({}, 'operation_name')) + retry_mock = mocker.patch('app.celery.tasks.build_dvla_file.retry', side_effect=Retry) + + with pytest.raises(Retry): + build_dvla_file(job.id) + + retry_mock.assert_called_once_with(queue='retry-tasks') + + def test_create_dvla_file_contents(notify_db_session, mocker): service = create_service(service_permissions=SERVICE_PERMISSION_TYPES) create_letter_contact(service=service, contact_block='London,\nNW1A 1AA') @@ -1157,7 +1169,6 @@ def test_send_inbound_sms_to_service_retries_if_request_returns_500(notify_api, ) assert mocked.call_count == 1 assert mocked.call_args[1]['queue'] == 'retry-tasks' - assert exc_msg in mocked.call_args[1]['exc'] def test_send_inbound_sms_to_service_retries_if_request_throws_unknown(notify_api, sample_service, mocker): @@ -1177,7 +1188,6 @@ def test_send_inbound_sms_to_service_retries_if_request_throws_unknown(notify_ap ) assert mocked.call_count == 1 assert mocked.call_args[1]['queue'] == 'retry-tasks' - assert exc_msg in mocked.call_args[1]['exc'] def test_send_inbound_sms_to_service_does_not_retries_if_request_returns_404(notify_api, sample_service, mocker):