Merge pull request #798 from NHSDigital/DTOSS11769

Remove retries for failed but recoverable message batches

Author: dnimmo

infrastructure/modules/container-apps/jobs.tf

@@ -28,17 +28,6 @@ locals {
       job_short_name     = "smb"
       job_container_args = "send_message_batch"
     }
-    retry_failed_message_batch = {
-      # cron_expression = "0,30 9-12 * * 1-5"
-      cron_expression = null
-      environment_variables = {
-        API_OAUTH_TOKEN_URL              = var.api_oauth_token_url
-        NHS_NOTIFY_API_MESSAGE_BATCH_URL = var.nhs_notify_api_message_batch_url
-        RETRY_QUEUE_NAME                 = "notifications-message-batch-retries"
-      }
-      job_short_name     = "rmb"
-      job_container_args = "retry_failed_message_batch"
-    }
     save_message_status = {
       # cron_expression = "0,30 * * * *"
       cron_expression = null
@@ -69,7 +58,6 @@ locals {
     collect_metrics = {
       cron_expression = "*/5 * * * *"
       environment_variables = {
-        RETRY_QUEUE_NAME          = "notifications-message-batch-retries"
         STATUS_UPDATES_QUEUE_NAME = "notifications-message-status-updates"
         ENVIRONMENT               = var.environment
       }

manage_breast_screening/notifications/management/commands/collect_metrics.py

@@ -11,14 +11,13 @@
 class Command(BaseCommand):
     def handle(self, *args, **options):
         try:
-            # Set queue_size metrics
-            for queue in [Queue.RetryMessageBatches(), Queue.MessageStatusUpdates()]:
-                Metrics().set_gauge_value(
-                    f"queue_size_{queue.queue_name}",
-                    "messages",
-                    "Queue length",
-                    queue.get_message_count(),
-                )
+            queue = Queue.MessageStatusUpdates()
+            Metrics().set_gauge_value(
+                f"queue_size_{queue.queue_name}",
+                "messages",
+                "Queue length",
+                queue.get_message_count(),
+            )
 
         except Exception as e:
             logger.error(e, exc_info=True)

manage_breast_screening/notifications/management/commands/helpers/message_batch_helpers.py

@@ -1,4 +1,3 @@
-import json
 import logging
 import re
 from datetime import datetime
@@ -15,7 +14,6 @@
 from manage_breast_screening.notifications.services.application_insights_logging import (
     ApplicationInsightsLogging,
 )
-from manage_breast_screening.notifications.services.queue import Queue
 
 logger = logging.getLogger(__name__)
 
@@ -100,23 +98,14 @@ def process_validation_errors(message_batch: MessageBatch, retry_count: int = 0)
 
                 message.save()
 
-        message_batch.status = MessageBatchStatusChoices.FAILED_RECOVERABLE.value
+        message_batch.status = MessageBatchStatusChoices.FAILED_UNRECOVERABLE.value
         message_batch.save()
 
-        logger.info(
-            "Adding MessageBatch %s to retry queue after validation failure",
+        logger.error(
+            "MessageBatch %s failed to send.",
             message_batch.id,
         )
 
-        Queue.RetryMessageBatches().add(
-            json.dumps(
-                {
-                    "message_batch_id": str(message_batch.id),
-                    "retry_count": retry_count,
-                }
-            )
-        )
-
     @staticmethod
     def process_unrecoverable_batch(message_batch: MessageBatch):
         message_batch.status = MessageBatchStatusChoices.FAILED_UNRECOVERABLE.value
@@ -132,19 +121,15 @@ def process_unrecoverable_batch(message_batch: MessageBatch):
 
     @staticmethod
     def process_recoverable_batch(message_batch: MessageBatch, retry_count: int):
-        message_batch.status = MessageBatchStatusChoices.FAILED_RECOVERABLE.value
+        message_batch.status = MessageBatchStatusChoices.FAILED_UNRECOVERABLE.value
         message_batch.save()
 
-        logger.info(
-            "Adding MessageBatch %s to retry queue after recoverable failure",
-            message_batch.id,
-        )
+        for message in message_batch.messages.all():
+            message.status = MessageStatusChoices.FAILED.value
+            message.sent_at = datetime.now(tz=ZONE_INFO)
+            message.save()
 
-        Queue.RetryMessageBatches().add(
-            json.dumps(
-                {
-                    "message_batch_id": str(message_batch.id),
-                    "retry_count": retry_count,
-                }
-            )
+        logger.error(
+            "MessageBatch %s failed to send.",
+            message_batch.id,
         )

manage_breast_screening/notifications/management/commands/retry_failed_message_batch.py

@@ -69,7 +69,3 @@ def MessageStatusUpdates(cls):
                 "STATUS_UPDATES_QUEUE_NAME", "notifications-message-status-updates"
             )
         )
-
-    @classmethod
-    def RetryMessageBatches(cls):
-        return cls(os.getenv("RETRY_QUEUE_NAME", "notifications-message-batch-retries"))

manage_breast_screening/notifications/services/queue.py

@@ -14,9 +14,6 @@
 from manage_breast_screening.notifications.management.commands.create_appointments import (
     Command as CreateAppointments,
 )
-from manage_breast_screening.notifications.management.commands.retry_failed_message_batch import (
-    Command as RetryFailedMessageBatch,
-)
 from manage_breast_screening.notifications.management.commands.save_message_status import (
     Command as SaveMessageStatus,
 )
@@ -29,11 +26,9 @@
 from manage_breast_screening.notifications.models import (
     Appointment,
     ChannelStatus,
-    Message,
     MessageBatch,
     MessageBatchStatusChoices,
     MessageStatus,
-    MessageStatusChoices,
 )
 from manage_breast_screening.notifications.services.queue import Queue
 from manage_breast_screening.notifications.tests.integration.helpers import Helpers
@@ -67,7 +62,6 @@ def setup_environment(self):
             f"{os.path.dirname(os.path.realpath(__file__))}/ABC_20250302091221_APPT_100.dat"
         )
         Queue.MessageStatusUpdates().client.clear_messages()
-        Queue.RetryMessageBatches().client.clear_messages()
 
     @pytest.mark.django_db
     def test_all_commands_in_sequence(self, mock_jwt_encode):
@@ -88,55 +82,11 @@ def test_all_commands_in_sequence(self, mock_jwt_encode):
 
         assert appointments.count() == 3
 
-        # Respond with a 408 recoverable error
-        self.env.set(
-            "NHS_NOTIFY_API_MESSAGE_BATCH_URL",
-            "http://localhost:8888/message/batch/recoverable-error",
-        )
-
-        SendMessageBatch().handle()
-
-        failed_batches = MessageBatch.objects.filter(
-            status=MessageBatchStatusChoices.FAILED_RECOVERABLE.value
-        )
-
-        assert failed_batches.count() == 1
-
-        messages = Message.objects.filter(
-            batch=failed_batches.first(),
-        )
-
-        assert messages.count() == 3
-
-        # Respond with a 400 validation error
-        self.env.set(
-            "NHS_NOTIFY_API_MESSAGE_BATCH_URL",
-            "http://localhost:8888/message/batch/validation-error",
-        )
-
-        RetryFailedMessageBatch().handle()
-
-        failed_batches = MessageBatch.objects.filter(
-            status=MessageBatchStatusChoices.FAILED_RECOVERABLE.value
-        )
-
-        assert failed_batches.first().messages.count() == 2
-
-        invalid_messages = Message.objects.filter(
-            status=MessageStatusChoices.FAILED.value
-        ).all()
-        assert invalid_messages.count() == 1
-        assert (
-            invalid_messages.first().nhs_notify_errors[0]["code"]
-            == "CM_INVALID_NHS_NUMBER"
-        )
-
-        # Respond with 201 success
         self.env.set(
             "NHS_NOTIFY_API_MESSAGE_BATCH_URL", "http://localhost:8888/message/batch"
         )
 
-        RetryFailedMessageBatch().handle()
+        SendMessageBatch().handle()
 
         sent_batches = MessageBatch.objects.filter(
             status=MessageBatchStatusChoices.SENT.value
@@ -145,11 +95,10 @@ def test_all_commands_in_sequence(self, mock_jwt_encode):
         assert sent_batches.count() == 1
         sent_batch = sent_batches.first()
         sent_messages = sent_batch.messages.all()
-        assert sent_messages.count() == 2
+        assert sent_messages.count() == 3
         appointments_from_sent_messages = [m.appointment for m in sent_messages]
 
-        assert appointments_from_sent_messages == [appointments[1], appointments[2]]
-        assert len(Queue.RetryMessageBatches().peek()) == 0
+        assert appointments_from_sent_messages == list(appointments)
 
         self.make_callbacks(sent_messages)