Merge pull request #3137 from alphagov/revert-revert-revert

Bring back retry logic

app/broadcast_message/rest.py

@@ -56,7 +56,7 @@ def _update_broadcast_message(broadcast_message, new_status, updating_user):
                 f'User {updating_user.id} cannot approve their own broadcast_message {broadcast_message.id}',
                 status_code=400
             )
-        elif not broadcast_message.areas:
+        elif len(broadcast_message.areas['simple_polygons']) == 0:
             raise InvalidRequest(
                 f'broadcast_message {broadcast_message.id} has no selected areas and so cannot be broadcasted.',
                 status_code=400
@@ -209,7 +209,7 @@ def _create_broadcast_event(broadcast_message):
 
     dao_save_object(event)
 
-    if not broadcast_message.stubbed or current_app.config['NOTIFY_ENVIRONMENT'] == 'preview':
+    if not broadcast_message.stubbed or current_app.config['NOTIFY_ENVIRONMENT'] in ['preview', 'development']:
         send_broadcast_event.apply_async(
             kwargs={'broadcast_event_id': str(event.id)},
             queue=QueueNames.BROADCASTS

app/celery/broadcast_message_tasks.py

@@ -1,17 +1,103 @@
 import uuid
+from datetime import datetime
 
 from flask import current_app
 from notifications_utils.statsd_decorators import statsd
 from sqlalchemy.schema import Sequence
+from celery.exceptions import MaxRetriesExceededError
 
 from app import cbc_proxy_client, db, notify_celery
+from app.clients.cbc_proxy import CBCProxyFatalException, CBCProxyRetryableException
 from app.config import QueueNames
-from app.models import BroadcastEventMessageType, BroadcastProvider
+from app.models import BroadcastEventMessageType, BroadcastProvider, BroadcastProviderMessageStatus
-from app.dao.broadcast_message_dao import dao_get_broadcast_event_by_id, create_broadcast_provider_message
+from app.dao.broadcast_message_dao import (
+    dao_get_broadcast_event_by_id,
+    create_broadcast_provider_message,
+    update_broadcast_provider_message_status
+)
 
 from app.utils import format_sequential_number
 
 
+def get_retry_delay(retry_count):
+    """
+    Given a count of retries so far, return a delay for the next one.
+    `retry_count` should be 0 the first time a task fails.
+    """
+    # TODO: replace with celery's built in exponential backoff
+
+    # 2 to the power of x. 1, 2, 4, 8, 16, 32, ...
+    delay = 2**retry_count
+    # never wait longer than 4 minutes
+    return min(delay, 240)
+
+
+def check_provider_message_should_send(broadcast_event, provider):
+    """
+    If any previous event hasn't sent yet for that provider, then we shouldn't send the current event. Instead, fail and
+    raise a P1 - so that a notify team member can assess the state of the previous messages, and if necessary, can
+    replay the `send_broadcast_provider_message` task if the previous message has now been sent.
+
+    Note: This is called before the new broadcast_provider_message is created.
+
+    # Help, I've come across this code following a pagerduty alert, what should I do?
+
+    1. Find the failing broadcast_provider_message associated with the previous event that caused this to trip.
+    2. If that provider message is still failing to send, fix the issue causing that. The task to send that previous
+       message might still be retrying in the background - look for logs related to that task.
+    3. If that provider message has sent succesfully, you might need to send this task off depending on context. This
+       might not always be true though, for example, it may not be necessary to send a cancel if the original alert has
+       already expired.
+    4. If you need to re-send this task off again, you'll need to run the following command on paas:
+       `send_broadcast_provider_message.apply_async(args=(broadcast_event_id, provider), queue=QueueNames.BROADCASTS)`
+    """
+    current_provider_message = broadcast_event.get_provider_message(provider)
+    # if this is the first time a task is being executed, it won't have a provider message yet
+    if current_provider_message and current_provider_message.status == BroadcastProviderMessageStatus.TECHNICAL_FAILURE:
+        raise CBCProxyFatalException(
+            f'Cannot send broadcast_event {broadcast_event.id} ' +
+            f'to provider {provider}: ' +
+            f'It is already in status technical-failure'
+        )
+
+    if broadcast_event.transmitted_finishes_at < datetime.utcnow():
+        # TODO: This should be a different kind of exception to distinguish "We should know something went wrong, but
+        # no immediate action" from "We need to fix this immediately"
+        raise CBCProxyFatalException(
+            f'Cannot send broadcast_event {broadcast_event.id} ' +
+            f'to provider {provider}: ' +
+            f'The expiry time of {broadcast_event.transmitted_finishes_at} has already passed'
+        )
+
+    # get events sorted from earliest to latest
+    events = sorted(broadcast_event.broadcast_message.events, key=lambda x: x.sent_at)
+
+    for prev_event in events:
+        if prev_event.id != broadcast_event.id and prev_event.sent_at < broadcast_event.sent_at:
+            # get the record from when that event was sent to the same provider
+            prev_provider_message = prev_event.get_provider_message(provider)
+
+            # the previous message hasn't even got round to running `send_broadcast_provider_message` yet.
+            if not prev_provider_message:
+                raise CBCProxyFatalException(
+                    f'Cannot send {broadcast_event.id}. Previous event {prev_event.id} ' +
+                    f'(type {prev_event.message_type}) has no provider_message for provider {provider} yet.\n' +
+                    f'You must ensure that the other event sends succesfully, then manually kick off this event ' +
+                    f'again by re-running send_broadcast_provider_message for this event and provider.'
+                )
+
+            # if there's a previous message that has started but not finished sending (whether it fatally errored or is
+            # currently retrying)
+            if prev_provider_message.status != BroadcastProviderMessageStatus.ACK:
+                raise CBCProxyFatalException(
+                    f'Cannot send {broadcast_event.id}. Previous event {prev_event.id} ' +
+                    f'(type {prev_event.message_type}) has not finished sending to provider {provider} yet.\n' +
+                    f'It is currently in status "{prev_provider_message.status}".\n' +
+                    f'You must ensure that the other event sends succesfully, then manually kick off this event ' +
+                    f'again by re-running send_broadcast_provider_message for this event and provider.'
+                )
+
+
 @notify_celery.task(name="send-broadcast-event")
 @statsd(namespace="tasks")
 def send_broadcast_event(broadcast_event_id):
@@ -27,12 +113,19 @@ def send_broadcast_event(broadcast_event_id):
         )
 
 
-@notify_celery.task(name="send-broadcast-provider-message")
+# max_retries=None: retry forever
+@notify_celery.task(bind=True, name="send-broadcast-provider-message", max_retries=None)
 @statsd(namespace="tasks")
-def send_broadcast_provider_message(broadcast_event_id, provider):
+def send_broadcast_provider_message(self, broadcast_event_id, provider):
     broadcast_event = dao_get_broadcast_event_by_id(broadcast_event_id)
 
-    broadcast_provider_message = create_broadcast_provider_message(broadcast_event, provider)
+    check_provider_message_should_send(broadcast_event, provider)
+
+    # the broadcast_provider_message may already exist if we retried previously
+    broadcast_provider_message = broadcast_event.get_provider_message(provider)
+    if broadcast_provider_message is None:
+        broadcast_provider_message = create_broadcast_provider_message(broadcast_event, provider)
+
     formatted_message_number = None
     if provider == BroadcastProvider.VODAFONE:
         formatted_message_number = format_sequential_number(broadcast_provider_message.message_number)
@@ -54,43 +147,58 @@ def send_broadcast_provider_message(broadcast_event_id, provider):
 
     cbc_proxy_provider_client = cbc_proxy_client.get_proxy(provider)
 
-    if broadcast_event.message_type == BroadcastEventMessageType.ALERT:
+    try:
-        cbc_proxy_provider_client.create_and_send_broadcast(
+        if broadcast_event.message_type == BroadcastEventMessageType.ALERT:
-            identifier=str(broadcast_provider_message.id),
+            cbc_proxy_provider_client.create_and_send_broadcast(
-            message_number=formatted_message_number,
+                identifier=str(broadcast_provider_message.id),
-            headline="GOV.UK Notify Broadcast",
+                message_number=formatted_message_number,
-            description=broadcast_event.transmitted_content['body'],
+                headline="GOV.UK Notify Broadcast",
-            areas=areas,
+                description=broadcast_event.transmitted_content['body'],
-            sent=broadcast_event.sent_at_as_cap_datetime_string,
+                areas=areas,
-            expires=broadcast_event.transmitted_finishes_at_as_cap_datetime_string,
+                sent=broadcast_event.sent_at_as_cap_datetime_string,
-            channel=channel
+                expires=broadcast_event.transmitted_finishes_at_as_cap_datetime_string,
+                channel=channel
+            )
+        elif broadcast_event.message_type == BroadcastEventMessageType.UPDATE:
+            cbc_proxy_provider_client.update_and_send_broadcast(
+                identifier=str(broadcast_provider_message.id),
+                message_number=formatted_message_number,
+                headline="GOV.UK Notify Broadcast",
+                description=broadcast_event.transmitted_content['body'],
+                areas=areas,
+                previous_provider_messages=broadcast_event.get_earlier_provider_messages(provider),
+                sent=broadcast_event.sent_at_as_cap_datetime_string,
+                expires=broadcast_event.transmitted_finishes_at_as_cap_datetime_string,
+                # We think an alert update should always go out on the same channel that created the alert
+                # We recognise there is a small risk with this code here that if the services channel was
+                # changed between an alert being sent out and then updated, then something might go wrong
+                # but we are relying on service channels changing almost never, and not mid incident
+                # We may consider in the future, changing this such that we store the channel a broadcast was
+                # sent on on the broadcast message itself and pick the value from there instead of the service
+                channel=channel
+            )
+        elif broadcast_event.message_type == BroadcastEventMessageType.CANCEL:
+            cbc_proxy_provider_client.cancel_broadcast(
+                identifier=str(broadcast_provider_message.id),
+                message_number=formatted_message_number,
+                previous_provider_messages=broadcast_event.get_earlier_provider_messages(provider),
+                sent=broadcast_event.sent_at_as_cap_datetime_string,
+            )
+    except CBCProxyRetryableException as exc:
+        delay = get_retry_delay(self.request.retries)
+        current_app.logger.exception(
+            f'Retrying send_broadcast_provider_message for broadcast_event {broadcast_event_id} and ' +
+            f'provider {provider} in {delay} seconds'
         )
-    elif broadcast_event.message_type == BroadcastEventMessageType.UPDATE:
+
-        cbc_proxy_provider_client.update_and_send_broadcast(
+        self.retry(
-            identifier=str(broadcast_provider_message.id),
+            exc=exc,
-            message_number=formatted_message_number,
+            countdown=delay,
-            headline="GOV.UK Notify Broadcast",
+            queue=QueueNames.BROADCASTS,
-            description=broadcast_event.transmitted_content['body'],
-            areas=areas,
-            previous_provider_messages=broadcast_event.get_earlier_provider_messages(provider),
-            sent=broadcast_event.sent_at_as_cap_datetime_string,
-            expires=broadcast_event.transmitted_finishes_at_as_cap_datetime_string,
-            # We think an alert update should always go out on the same channel that created the alert
-            # We recognise there is a small risk with this code here that if the services channel was
-            # changed between an alert being sent out and then updated, then something might go wrong
-            # but we are relying on service channels changing almost never, and not mid incident
-            # We may consider in the future, changing this such that we store the channel a broadcast was
-            # sent on on the broadcast message itself and pick the value from there instead of the service
-            channel=channel
-        )
-    elif broadcast_event.message_type == BroadcastEventMessageType.CANCEL:
-        cbc_proxy_provider_client.cancel_broadcast(
-            identifier=str(broadcast_provider_message.id),
-            message_number=formatted_message_number,
-            previous_provider_messages=broadcast_event.get_earlier_provider_messages(provider),
-            sent=broadcast_event.sent_at_as_cap_datetime_string,
         )
 
+    update_broadcast_provider_message_status(broadcast_provider_message, status=BroadcastProviderMessageStatus.ACK)
+
 
 @notify_celery.task(name='trigger-link-test')
 def trigger_link_test(provider):

app/clients/cbc_proxy.py

@@ -2,6 +2,7 @@ import json
 from abc import ABC, abstractmethod
 
 import boto3
+import botocore
 from flask import current_app
 from notifications_utils.template import non_gsm_characters
 
@@ -25,7 +26,11 @@ from app.utils import DATETIME_FORMAT, format_sequential_number
 #    the preceeding Alert message in the previous_provider_messages field
 
 
-class CBCProxyException(Exception):
+class CBCProxyFatalException(Exception):
+    pass
+
+
+class CBCProxyRetryableException(Exception):
     pass
 
 
@@ -115,19 +120,24 @@ class CBCProxyClientBase(ABC):
         if not result:
             failover_result = self._invoke_lambda(self.failover_lambda_name, payload)
             if not failover_result:
-                raise CBCProxyException(f'Lambda failed for both {self.lambda_name} and {self.failover_lambda_name}')
+                raise CBCProxyRetryableException(
+                    f'Lambda failed for both {self.lambda_name} and {self.failover_lambda_name}'
+                )
 
         return result
 
     def _invoke_lambda(self, lambda_name, payload):
         payload_bytes = bytes(json.dumps(payload), encoding='utf8')
-        current_app.logger.info(f"Calling lambda {lambda_name}")
+        try:
-        result = self._lambda_client.invoke(
+            result = self._lambda_client.invoke(
-            FunctionName=lambda_name,
+                FunctionName=lambda_name,
-            InvocationType='RequestResponse',
+                InvocationType='RequestResponse',
-            Payload=payload_bytes,
+                Payload=payload_bytes,
-        )
+            )
-        current_app.logger.info(f"Finished calling lambda {lambda_name}")
+        except botocore.exceptions.ClientError as exc:
+            current_app.logger.exception(f'Boto ClientError calling lambda {lambda_name}')
+            success = False
+            return success
 
         if result['StatusCode'] > 299:
             current_app.logger.info(

app/dao/broadcast_message_dao.py

@@ -61,3 +61,8 @@ def create_broadcast_provider_message(broadcast_event, provider):
         db.session.add(provider_message_number)
         db.session.commit()
     return provider_message
+
+
+@transactional
+def update_broadcast_provider_message_status(broadcast_provider_message, *, status):
+    broadcast_provider_message.status = status

app/models.py

@@ -2423,7 +2423,7 @@ class BroadcastEvent(db.Model):
 
     def get_earlier_provider_messages(self, provider):
         """
-        Get the previous message for a provider. These are differentper provider, as the identifiers are different.
+        Get the previous message for a provider. These are different per provider, as the identifiers are different.
         Return the full provider_message object rather than just an identifier, since the different providers expect
         reference to contain different things - let the cbc_proxy work out what information is relevant.
         """

manifest.yml.j2

@@ -64,7 +64,9 @@
   'notify-delivery-worker-letters': {'memory': '2G'},
   'notify-delivery-worker-retry-tasks': {},
   'notify-delivery-worker-internal': {},
-  'notify-delivery-worker-broadcasts': {},
+  'notify-delivery-worker-broadcasts': {
+      'CELERYD_PREFETCH_MULTIPLIER': 1,
+  },
   'notify-delivery-worker-receipts': {},
   'notify-delivery-worker-service-callbacks': {'disk_quota': '2G'},
   'notify-delivery-worker-save-api-notifications': {'disk_quota': '2G'},

tests/app/celery/test_broadcast_message_tasks.py

@@ -1,7 +1,9 @@
 import uuid
+from datetime import datetime
 from unittest.mock import call, ANY
 
 from freezegun import freeze_time
+from celery.exceptions import Retry
 import pytest
 
 from app.models import (
@@ -11,7 +13,14 @@ from app.models import (
     BroadcastProviderMessageStatus,
     ServiceBroadcastSettings,
 )
-from app.celery.broadcast_message_tasks import send_broadcast_event, send_broadcast_provider_message, trigger_link_test
+from app.clients.cbc_proxy import CBCProxyRetryableException, CBCProxyFatalException
+from app.celery.broadcast_message_tasks import (
+    check_provider_message_should_send,
+    get_retry_delay,
+    send_broadcast_event,
+    send_broadcast_provider_message,
+    trigger_link_test,
+)
 
 from tests.app.db import (
     create_template,
@@ -133,7 +142,7 @@ def test_send_broadcast_provider_message_sends_data_correctly(
     send_broadcast_provider_message(provider=provider, broadcast_event_id=str(event.id))
 
     broadcast_provider_message = event.get_provider_message(provider)
-    assert broadcast_provider_message.status == BroadcastProviderMessageStatus.SENDING
+    assert broadcast_provider_message.status == BroadcastProviderMessageStatus.ACK
 
     mock_create_broadcast.assert_called_once_with(
         identifier=str(broadcast_provider_message.id),
@@ -240,6 +249,48 @@ def test_send_broadcast_provider_message_defaults_to_test_channel_if_no_service_
     )
 
 
+def test_send_broadcast_provider_message_works_if_we_retried_previously(mocker, sample_service):
+    template = create_template(sample_service, BROADCAST_TYPE)
+    broadcast_message = create_broadcast_message(
+        template,
+        areas={'areas': [], 'simple_polygons': [],},
+        status=BroadcastStatusType.BROADCASTING
+    )
+    event = create_broadcast_event(broadcast_message)
+
+    # an existing provider message already exists, and previously failed
+    existing_provider_message = create_broadcast_provider_message(
+        broadcast_event=event,
+        provider='ee',
+        status=BroadcastProviderMessageStatus.SENDING
+    )
+
+    mock_create_broadcast = mocker.patch(
+        f'app.clients.cbc_proxy.CBCProxyEE.create_and_send_broadcast',
+    )
+
+    send_broadcast_provider_message(provider='ee', broadcast_event_id=str(event.id))
+
+    # make sure we haven't completed a duplicate event - we shouldn't record the failure
+    assert len(event.provider_messages) == 1
+
+    broadcast_provider_message = event.get_provider_message('ee')
+
+    assert broadcast_provider_message.status == BroadcastProviderMessageStatus.ACK
+    assert broadcast_provider_message.updated_at is not None
+
+    mock_create_broadcast.assert_called_once_with(
+        identifier=str(broadcast_provider_message.id),
+        message_number=mocker.ANY,
+        headline='GOV.UK Notify Broadcast',
+        description='this is an emergency broadcast message',
+        areas=[],
+        sent=event.sent_at_as_cap_datetime_string,
+        expires=event.transmitted_finishes_at_as_cap_datetime_string,
+        channel='test',
+    )
+
+
 @freeze_time('2020-08-01 12:00')
 @pytest.mark.parametrize('provider,provider_capitalised', [
     ['ee', 'EE'],
@@ -308,7 +359,7 @@ def test_send_broadcast_provider_message_sends_update_with_references(
     )
 
     alert_event = create_broadcast_event(broadcast_message, message_type=BroadcastEventMessageType.ALERT)
-    create_broadcast_provider_message(alert_event, provider)
+    create_broadcast_provider_message(alert_event, provider, status=BroadcastProviderMessageStatus.ACK)
     update_event = create_broadcast_event(broadcast_message, message_type=BroadcastEventMessageType.UPDATE)
 
     mock_update_broadcast = mocker.patch(
@@ -318,7 +369,7 @@ def test_send_broadcast_provider_message_sends_update_with_references(
     send_broadcast_provider_message(provider=provider, broadcast_event_id=str(update_event.id))
 
     broadcast_provider_message = update_event.get_provider_message(provider)
-    assert broadcast_provider_message.status == BroadcastProviderMessageStatus.SENDING
+    assert broadcast_provider_message.status == BroadcastProviderMessageStatus.ACK
 
     mock_update_broadcast.assert_called_once_with(
         identifier=str(broadcast_provider_message.id),
@@ -363,8 +414,8 @@ def test_send_broadcast_provider_message_sends_cancel_with_references(
     update_event = create_broadcast_event(broadcast_message, message_type=BroadcastEventMessageType.UPDATE)
     cancel_event = create_broadcast_event(broadcast_message, message_type=BroadcastEventMessageType.CANCEL)
 
-    create_broadcast_provider_message(alert_event, provider)
+    create_broadcast_provider_message(alert_event, provider, status=BroadcastProviderMessageStatus.ACK)
-    create_broadcast_provider_message(update_event, provider)
+    create_broadcast_provider_message(update_event, provider, status=BroadcastProviderMessageStatus.ACK)
 
     mock_cancel_broadcast = mocker.patch(
         f'app.clients.cbc_proxy.CBCProxy{provider_capitalised}.cancel_broadcast',
@@ -373,7 +424,7 @@ def test_send_broadcast_provider_message_sends_cancel_with_references(
     send_broadcast_provider_message(provider=provider, broadcast_event_id=str(cancel_event.id))
 
     broadcast_provider_message = cancel_event.get_provider_message(provider)
-    assert broadcast_provider_message.status == BroadcastProviderMessageStatus.SENDING
+    assert broadcast_provider_message.status == BroadcastProviderMessageStatus.ACK
 
     mock_cancel_broadcast.assert_called_once_with(
         identifier=str(broadcast_provider_message.id),
@@ -410,14 +461,16 @@ def test_send_broadcast_provider_message_errors(mocker, sample_service, provider
 
     mock_create_broadcast = mocker.patch(
         f'app.clients.cbc_proxy.CBCProxy{provider_capitalised}.create_and_send_broadcast',
-        side_effect=Exception('oh no'),
+        side_effect=CBCProxyRetryableException('oh no'),
+    )
+    mock_retry = mocker.patch(
+        'app.celery.broadcast_message_tasks.send_broadcast_provider_message.retry',
+        side_effect=Retry
     )
 
-    with pytest.raises(Exception) as ex:
+    with pytest.raises(Retry):
         send_broadcast_provider_message(provider=provider, broadcast_event_id=str(event.id))
 
-    assert ex.match('oh no')
-
     mock_create_broadcast.assert_called_once_with(
         identifier=ANY,
         message_number=mocker.ANY,
@@ -434,6 +487,63 @@ def test_send_broadcast_provider_message_errors(mocker, sample_service, provider
         expires=event.transmitted_finishes_at_as_cap_datetime_string,
         channel="test"
     )
+    mock_retry.assert_called_once_with(
+        countdown=1,
+        exc=mock_create_broadcast.side_effect,
+        queue='broadcast-tasks'
+    )
+    broadcast_provider_message = event.get_provider_message(provider)
+    assert broadcast_provider_message.status == BroadcastProviderMessageStatus.SENDING
+
+
+
+@pytest.mark.parametrize('num_retries, expected_countdown', [
+    (0, 1),
+    (5, 32),
+    (20, 240),
+])
+def test_send_broadcast_provider_message_delays_retry_exponentially(
+    mocker,
+    sample_service,
+    num_retries,
+    expected_countdown
+):
+    template = create_template(sample_service, BROADCAST_TYPE)
+
+    broadcast_message = create_broadcast_message(template,  status=BroadcastStatusType.BROADCASTING)
+    event = create_broadcast_event(broadcast_message)
+
+    mock_create_broadcast = mocker.patch(
+        'app.clients.cbc_proxy.CBCProxyEE.create_and_send_broadcast',
+        side_effect=CBCProxyRetryableException('oh no'),
+    )
+    mock_retry = mocker.patch(
+        'app.celery.broadcast_message_tasks.send_broadcast_provider_message.retry',
+        side_effect=Retry
+    )
+
+    # patch celery request context as shown here: https://stackoverflow.com/a/59870468
+    mock_celery_task_request_context = mocker.patch("celery.app.task.Task.request")
+    mock_celery_task_request_context.retries = num_retries
+
+    with pytest.raises(Retry):
+        send_broadcast_provider_message(provider='ee', broadcast_event_id=str(event.id))
+
+    mock_create_broadcast.assert_called_once_with(
+        identifier=ANY,
+        message_number=mocker.ANY,
+        headline="GOV.UK Notify Broadcast",
+        description='this is an emergency broadcast message',
+        areas=[],
+        sent=event.sent_at_as_cap_datetime_string,
+        expires=event.transmitted_finishes_at_as_cap_datetime_string,
+        channel='test',
+    )
+    mock_retry.assert_called_once_with(
+        countdown=expected_countdown,
+        exc=mock_create_broadcast.side_effect,
+        queue='broadcast-tasks'
+    )
 
 
 @pytest.mark.parametrize("provider,provider_capitalised", [
@@ -466,3 +576,146 @@ def test_trigger_link_tests_invokes_cbc_proxy_client(
         assert len(mock_send_link_test.mock_calls[0][1][1]) == 8
     else:
         assert not mock_send_link_test.mock_calls[0][1][1]
+
+
+@pytest.mark.parametrize('retry_count, expected_delay', [
+    (0, 1),
+    (1, 2),
+    (2, 4),
+    (7, 128),
+    (8, 240),
+    (9, 240),
+    (1000, 240),
+])
+def test_get_retry_delay_has_capped_backoff(retry_count, expected_delay):
+    assert get_retry_delay(retry_count) == expected_delay
+
+
+@freeze_time('2021-01-01 12:00')
+def test_check_provider_message_should_send_doesnt_raise_if_event_hasnt_expired_yet(sample_template):
+    broadcast_message = create_broadcast_message(sample_template)
+    current_event = create_broadcast_event(
+        broadcast_message,
+        transmitted_starts_at=datetime(2021, 1, 1, 0, 0),
+        transmitted_finishes_at=datetime(2021, 1, 1, 12, 1),
+    )
+    check_provider_message_should_send(current_event, 'ee')
+
+
+@freeze_time('2021-01-01 12:00')
+def test_check_provider_message_should_send_raises_if_event_has_expired(sample_template):
+    broadcast_message = create_broadcast_message(sample_template)
+    current_event = create_broadcast_event(
+        broadcast_message,
+        transmitted_starts_at=datetime(2021, 1, 1, 0, 0),
+        transmitted_finishes_at=datetime(2021, 1, 1, 11, 59),
+    )
+    with pytest.raises(CBCProxyFatalException) as exc:
+        check_provider_message_should_send(current_event, 'ee')
+    assert 'The expiry time of 2021-01-01 11:59:00 has already passed' in str(exc.value)
+
+
+@freeze_time('2021-01-01 12:00')
+def test_check_provider_message_should_send_raises_if_older_event_still_sending(sample_template):
+    broadcast_message = create_broadcast_message(sample_template)
+    # event approved at midnight
+    past_succesful_event = create_broadcast_event(
+        broadcast_message,
+        message_type='alert',
+        sent_at=datetime(2021, 1, 1, 0, 0),
+    )
+    # event updated at 5am (this event is still sending)
+    past_still_sending_event = create_broadcast_event(
+        broadcast_message,
+        message_type='update',
+        sent_at=datetime(2021, 1, 1, 5, 0),
+    )
+    # event updated again at 7am
+    current_event = create_broadcast_event(
+        broadcast_message,
+        message_type='update',
+        sent_at=datetime(2021, 1, 1, 7, 0),
+    )
+
+    create_broadcast_provider_message(past_succesful_event, provider='ee', status=BroadcastProviderMessageStatus.ACK)
+    create_broadcast_provider_message(past_still_sending_event, provider='ee', status=BroadcastProviderMessageStatus.SENDING)  # noqa
+
+    # we havent sent the previous update yet - it's still in sending - so don't try and send this one.
+    with pytest.raises(CBCProxyFatalException) as exc:
+        check_provider_message_should_send(current_event, 'ee')
+
+    assert f'Previous event {past_still_sending_event.id} (type update) has not finished sending to provider ee' in str(exc.value)  # noqa
+
+
+@freeze_time('2021-01-01 12:00')
+def test_check_provider_message_should_send_raises_if_older_event_hasnt_started_sending_yet(sample_template):
+    broadcast_message = create_broadcast_message(sample_template)
+    # event approved at midnight
+    past_succesful_event = create_broadcast_event(
+        broadcast_message,
+        message_type='alert',
+        sent_at=datetime(2021, 1, 1, 0, 0),
+    )
+    # event updated at 5am
+    past_still_sending_event = create_broadcast_event(
+        broadcast_message,
+        message_type='update',
+        sent_at=datetime(2021, 1, 1, 5, 0),
+    )
+    # event updated at 7am
+    current_event = create_broadcast_event(
+        broadcast_message,
+        message_type='update',
+        sent_at=datetime(2021, 1, 1, 7, 0),
+    )
+
+    # no provider message for past_still_sending_event
+    create_broadcast_provider_message(past_succesful_event, provider='ee', status=BroadcastProviderMessageStatus.ACK)
+
+    # we shouldn't send the update now, because a previous event is still stuck in sending
+    with pytest.raises(CBCProxyFatalException) as exc:
+        check_provider_message_should_send(current_event, 'ee')
+
+    assert f'Previous event {past_still_sending_event.id} (type update) has no provider_message for provider ee' in str(exc.value)  # noqa
+
+
+@freeze_time('2021-01-01 12:00')
+def test_check_provider_message_should_send_doesnt_raise_if_newer_event_not_acked_yet(sample_template):
+    broadcast_message = create_broadcast_message(sample_template)
+    # event approved at midnight
+    current_event = create_broadcast_event(
+        broadcast_message,
+        message_type='alert',
+        sent_at=datetime(2021, 1, 1, 0, 0),
+    )
+    future_event = create_broadcast_event(
+        broadcast_message,
+        message_type='cancel',
+        sent_at=datetime(2021, 1, 1, 10, 0),
+    )
+
+    # this doesn't raise, because the alert event got an ack. The cancel doesn't have an event yet
+    # but this task is only interested in the current task (the update) so doesn't worry about that
+    check_provider_message_should_send(current_event, 'ee')
+
+
+@pytest.mark.parametrize('existing_message_status', [
+    BroadcastProviderMessageStatus.SENDING,
+    BroadcastProviderMessageStatus.ACK,
+    BroadcastProviderMessageStatus.ERR,
+
+    pytest.param(
+        BroadcastProviderMessageStatus.TECHNICAL_FAILURE,
+        marks=pytest.mark.xfail(raises=CBCProxyFatalException)
+    ),
+])
+def test_check_provider_message_should_send_doesnt_raise_if_current_event_already_has_provider_message(
+    sample_template,
+    existing_message_status
+):
+    broadcast_message = create_broadcast_message(sample_template)
+    current_event = create_broadcast_event(broadcast_message, message_type='alert')
+
+    create_broadcast_provider_message(current_event, provider='ee', status=existing_message_status)
+
+    check_provider_message_should_send(current_event, 'ee')

tests/app/clients/test_cbc_proxy.py

@@ -4,10 +4,11 @@ from collections import namedtuple
 from datetime import datetime
 from unittest.mock import Mock, call
 
+from botocore.exceptions import ClientError as BotoClientError
 import pytest
 
 from app.clients.cbc_proxy import (
-    CBCProxyClient, CBCProxyException, CBCProxyEE, CBCProxyCanary, CBCProxyVodafone, CBCProxyThree, CBCProxyO2
+    CBCProxyClient, CBCProxyRetryableException, CBCProxyEE, CBCProxyCanary, CBCProxyVodafone, CBCProxyThree, CBCProxyO2
 )
 from app.utils import DATETIME_FORMAT
 
@@ -316,6 +317,50 @@ def test_cbc_proxy_vodafone_cancel_invokes_function(mocker, cbc_proxy_vodafone):
     assert payload['sent'] == sent
 
 
+@pytest.mark.parametrize('cbc', ['ee', 'vodafone', 'three', 'o2'])
+def test_cbc_proxy_will_failover_to_second_lambda_if_boto_client_error(
+    mocker,
+    cbc_proxy_client,
+    cbc
+):
+    cbc_proxy = cbc_proxy_client.get_proxy(cbc)
+
+    ld_client_mock = mocker.patch.object(
+        cbc_proxy,
+        '_lambda_client',
+        create=True,
+    )
+
+    ld_client_mock.invoke.side_effect = BotoClientError({}, 'error')
+
+    with pytest.raises(CBCProxyRetryableException) as e:
+        cbc_proxy.create_and_send_broadcast(
+            identifier='my-identifier',
+            message_number='0000007b',
+            headline='my-headline',
+            description='test-description',
+            areas=EXAMPLE_AREAS,
+            sent='a-passed-through-sent-value',
+            expires='a-passed-through-expires-value',
+            channel="severe",
+        )
+
+    assert e.match(f'Lambda failed for both {cbc}-1-proxy and {cbc}-2-proxy')
+
+    assert ld_client_mock.invoke.call_args_list == [
+        call(
+            FunctionName=f'{cbc}-1-proxy',
+            InvocationType='RequestResponse',
+            Payload=mocker.ANY,
+        ),
+        call(
+            FunctionName=f'{cbc}-2-proxy',
+            InvocationType='RequestResponse',
+            Payload=mocker.ANY,
+        )
+    ]
+
+
 @pytest.mark.parametrize('cbc', ['ee', 'vodafone', 'three', 'o2'])
 def test_cbc_proxy_will_failover_to_second_lambda_if_function_error(
     mocker,
@@ -433,7 +478,7 @@ def test_cbc_proxy_create_and_send_tries_failover_lambda_on_invoke_error_and_rai
         'StatusCode': 400,
     }
 
-    with pytest.raises(CBCProxyException) as e:
+    with pytest.raises(CBCProxyRetryableException) as e:
         cbc_proxy.create_and_send_broadcast(
             identifier='my-identifier',
             message_number='0000007b',
@@ -482,7 +527,7 @@ def test_cbc_proxy_create_and_send_tries_failover_lambda_on_function_error_and_r
         }
     }
 
-    with pytest.raises(CBCProxyException) as e:
+    with pytest.raises(CBCProxyRetryableException) as e:
         cbc_proxy.create_and_send_broadcast(
             identifier='my-identifier',
             message_number='0000007b',

tests/app/db.py

@@ -1049,7 +1049,7 @@ def create_broadcast_message(
         starts_at=starts_at,
         finishes_at=finishes_at,
         created_by_id=created_by.id if created_by else service.created_by_id,
-        areas=areas or {},
+        areas=areas or {'areas': [], 'simple_polygons': []},
         content=content,
         stubbed=stubbed
     )
@@ -1077,7 +1077,7 @@ def create_broadcast_event(
         transmitted_areas=transmitted_areas or broadcast_message.areas,
         transmitted_sender=transmitted_sender or 'www.notifications.service.gov.uk',
         transmitted_starts_at=transmitted_starts_at,
-        transmitted_finishes_at=transmitted_finishes_at or datetime.utcnow(),
+        transmitted_finishes_at=transmitted_finishes_at or datetime.utcnow() + timedelta(hours=24),
     )
     db.session.add(b_e)
     db.session.commit()
@@ -1105,4 +1105,4 @@ def create_broadcast_provider_message(
             broadcast_provider_message_id=broadcast_provider_message_id)
         db.session.add(provider_message_number)
         db.session.commit()
-    return provider_message, provider_message_number
+    return provider_message