From 76aeab24ce25b9a221dc6b7a5034e6e766467b96 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 13 Dec 2021 16:56:21 +0000 Subject: [PATCH] Rewrite DAO timeout method to take cutoff_time Previously we specified the period and calculated the cutoff time in the function. Passing it in means we can run the method multiple times and avoid getting "new" notifications to time out in the time it takes to process each batch. --- app/celery/nightly_tasks.py | 7 +++++-- app/dao/notifications_dao.py | 10 +++++----- tests/app/celery/test_nightly_tasks.py | 7 ++----- .../app/dao/notification_dao/test_notification_dao.py | 6 +++--- 4 files changed, 15 insertions(+), 15 deletions(-) diff --git a/app/celery/nightly_tasks.py b/app/celery/nightly_tasks.py index 92d401f46..54714922f 100644 --- a/app/celery/nightly_tasks.py +++ b/app/celery/nightly_tasks.py @@ -117,9 +117,12 @@ def timeout_notifications(): # so that we can cope with a high volume that need processing. We've changed # dao_timeout_notifications to return up to 100K notifications, so this task # will operate on up to 500K - normally we only get around 20K. + cutoff_time = datetime.utcnow() - timedelta( + seconds=current_app.config.get('SENDING_NOTIFICATIONS_TIMEOUT_PERIOD') + ) + for _ in range(0, 5): - notifications = \ - dao_timeout_notifications(current_app.config.get('SENDING_NOTIFICATIONS_TIMEOUT_PERIOD')) + notifications = dao_timeout_notifications(cutoff_time) for notification in notifications: check_and_queue_callback_task(notification) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 15daca241..acfa286e3 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -489,11 +489,11 @@ def dao_delete_notifications_by_id(notification_id): ).delete(synchronize_session='fetch') -def dao_timeout_notifications(timeout_period_in_seconds): +def dao_timeout_notifications(cutoff_time): """ - Set email and SMS notifications (only) to "temporary-failure" status. + Set email and SMS notifications (only) to "temporary-failure" status + if they're still sending from before the specified cutoff_time. """ - timeout_start = datetime.utcnow() - timedelta(seconds=timeout_period_in_seconds) updated_at = datetime.utcnow() current_statuses = [NOTIFICATION_SENDING, NOTIFICATION_PENDING] new_status = NOTIFICATION_TEMPORARY_FAILURE @@ -503,13 +503,13 @@ def dao_timeout_notifications(timeout_period_in_seconds): # - we believe this is a OOM error but there are no logs. From # experimentation we've found we can safely process up to 100K. notifications = Notification.query.filter( - Notification.created_at < timeout_start, + Notification.created_at < cutoff_time, Notification.status.in_(current_statuses), Notification.notification_type.in_([SMS_TYPE, EMAIL_TYPE]) ).limit(100000).all() Notification.query.filter( - Notification.created_at < timeout_start, + Notification.created_at < cutoff_time, Notification.status.in_(current_statuses), Notification.notification_type.in_([SMS_TYPE, EMAIL_TYPE]), Notification.id.in_([n.id for n in notifications]), diff --git a/tests/app/celery/test_nightly_tasks.py b/tests/app/celery/test_nightly_tasks.py index 7a6167aa2..847a27ce9 100644 --- a/tests/app/celery/test_nightly_tasks.py +++ b/tests/app/celery/test_nightly_tasks.py @@ -161,17 +161,14 @@ def test_delete_letter_notifications_older_than_retention_calls_child_task(notif mocked.assert_called_once_with('letter') +@freeze_time("2021-12-13T10:00") def test_timeout_notifications(mocker, sample_notification): mock_update = mocker.patch('app.celery.nightly_tasks.check_and_queue_callback_task') mock_dao = mocker.patch('app.celery.nightly_tasks.dao_timeout_notifications') mock_dao.return_value = [sample_notification] timeout_notifications() - - mock_dao.assert_called_once_with( - current_app.config.get('SENDING_NOTIFICATIONS_TIMEOUT_PERIOD') - ) - + mock_dao.assert_called_once_with(datetime.fromisoformat('2021-12-10T10:00')) mock_update.assert_called_once_with(sample_notification) diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index 8e5f495e2..e09bb26d8 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -671,7 +671,7 @@ def test_dao_timeout_notifications(sample_template): pending = create_notification(sample_template, status='pending') delivered = create_notification(sample_template, status='delivered') - temporary_failure_notifications = dao_timeout_notifications(1) + temporary_failure_notifications = dao_timeout_notifications(datetime.utcnow()) assert len(temporary_failure_notifications) == 2 assert Notification.query.get(created.id).status == 'created' @@ -687,7 +687,7 @@ def test_dao_timeout_notifications_only_updates_for_older_notifications(sample_t pending = create_notification(sample_template, status='pending') delivered = create_notification(sample_template, status='delivered') - temporary_failure_notifications = dao_timeout_notifications(1) + temporary_failure_notifications = dao_timeout_notifications(datetime.utcnow()) assert len(temporary_failure_notifications) == 0 assert Notification.query.get(created.id).status == 'created' @@ -703,7 +703,7 @@ def test_dao_timeout_notifications_doesnt_affect_letters(sample_letter_template) pending = create_notification(sample_letter_template, status='pending') delivered = create_notification(sample_letter_template, status='delivered') - temporary_failure_notifications = dao_timeout_notifications(1) + temporary_failure_notifications = dao_timeout_notifications(datetime.utcnow()) assert len(temporary_failure_notifications) == 0 assert Notification.query.get(created.id).status == 'created'