Merge pull request #3152 from alphagov/retry-bug-fix

Retry bug fix

app/__init__.py

@@ -381,7 +381,7 @@ def setup_sqlalchemy_events(app):
                         'host': current_app.config['NOTIFY_APP_NAME'],  # worker name
                         'url_rule': current_task.name,  # task name
                     }
-                # anything else. migrations possibly.
+                # anything else. migrations possibly, or flask cli commands.
                 else:
                     current_app.logger.warning('Checked out sqlalchemy connection from outside of request/task')
                     connection_record.info['request_data'] = {

app/celery/broadcast_message_tasks.py

@@ -52,11 +52,11 @@ def check_provider_message_should_send(broadcast_event, provider):
     """
     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:
+    if current_provider_message and current_provider_message.status != BroadcastProviderMessageStatus.SENDING:
         raise CBCProxyFatalException(
             f'Cannot send broadcast_event {broadcast_event.id} ' +
             f'to provider {provider}: ' +
-            'It is already in status technical-failure'
+            f'It is in status {current_provider_message.status}'
         )
 
     if broadcast_event.transmitted_finishes_at < datetime.utcnow():

app/clients/cbc_proxy.py

@@ -147,7 +147,7 @@ class CBCProxyClientBase(ABC):
 
         elif 'FunctionError' in result:
             current_app.logger.info(
-                f"Error calling lambda {lambda_name} with function error { result['Payload'] }"
+                f"Error calling lambda {lambda_name} with function error { result['Payload'].read() }"
             )
             success = False
 

tests/app/celery/test_broadcast_message_tasks.py

@@ -701,15 +701,20 @@ def test_check_provider_message_should_send_doesnt_raise_if_newer_event_not_acke
 
 @pytest.mark.parametrize('existing_message_status', [
     BroadcastProviderMessageStatus.SENDING,
-    BroadcastProviderMessageStatus.ACK,
-    BroadcastProviderMessageStatus.ERR,
-
+    pytest.param(
+        BroadcastProviderMessageStatus.ACK,
+        marks=pytest.mark.xfail(raises=CBCProxyFatalException)
+    ),
+    pytest.param(
+        BroadcastProviderMessageStatus.ERR,
+        marks=pytest.mark.xfail(raises=CBCProxyFatalException)
+    ),
     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(
+def test_check_provider_message_should_send_raises_if_current_event_already_has_provider_message_not_in_sending(
     sample_template,
     existing_message_status
 ):

tests/app/clients/test_cbc_proxy.py

@@ -1,3 +1,4 @@
+from io import BytesIO
 import json
 import uuid
 from collections import namedtuple
@@ -380,10 +381,7 @@ def test_cbc_proxy_will_failover_to_second_lambda_if_function_error(
         {
             'StatusCode': 200,
             'FunctionError': 'Handled',
-            'Payload': {
-                "errorMessage": "",
-                "errorType": "CBCNewConnectionError"
-            }
+            'Payload': BytesIO(json.dumps({"errorMessage": "", "errorType": "CBCNewConnectionError"}).encode('utf-8')),
         },
         {
             'StatusCode': 200
@@ -522,10 +520,7 @@ def test_cbc_proxy_create_and_send_tries_failover_lambda_on_function_error_and_r
     ld_client_mock.invoke.return_value = {
         'StatusCode': 200,
         'FunctionError': 'something',
-        'Payload': {
-            'errorMessage': 'some message',
-            'errorType': 'SomeErrorType'
-        }
+        'Payload': BytesIO(json.dumps({"errorMessage": "some message", "errorType": "SomeErrorType"}).encode('utf-8')),
     }
 
     with pytest.raises(CBCProxyRetryableException) as e: