Merge pull request #1754 from alphagov/revert-service-callback-eventlets

Revert service callback eventlets

app/celery/service_callback_tasks.py

@@ -24,28 +24,29 @@ from app.config import QueueNames
 @notify_celery.task(bind=True, name="send-delivery-status", max_retries=5, default_retry_delay=300)
 @statsd(namespace="tasks")
 def send_delivery_status_to_service(self, notification_id):
-    # TODO: do we need to do rate limit this?
-    notification = get_notification_by_id(notification_id)
-    service_callback_api = get_service_callback_api_for_service(service_id=notification.service_id)
-    if not service_callback_api:
-        # No delivery receipt API info set
-        return
-
-    # Release DB connection before performing an external HTTP request
-    db.session.close()
-
-    data = {
-        "id": str(notification_id),
-        "reference": str(notification.client_reference),
-        "to": notification.to,
-        "status": notification.status,
-        "created_at": notification.created_at.strftime(DATETIME_FORMAT),     # the time service sent the request
-        "completed_at": notification.updated_at.strftime(DATETIME_FORMAT),   # the last time the status was updated
-        "sent_at": notification.sent_at.strftime(DATETIME_FORMAT),           # the time the email was sent
-        "notification_type": notification.notification_type
-    }
-
+    retry = False
     try:
+        # TODO: do we need to do rate limit this?
+        notification = get_notification_by_id(notification_id)
+        service_callback_api = get_service_callback_api_for_service(service_id=notification.service_id)
+        if not service_callback_api:
+            # No delivery receipt API info set
+            return
+
+        # Release DB connection before performing an external HTTP request
+        db.session.close()
+
+        data = {
+            "id": str(notification_id),
+            "reference": str(notification.client_reference),
+            "to": notification.to,
+            "status": notification.status,
+            "created_at": notification.created_at.strftime(DATETIME_FORMAT),     # the time service sent the request
+            "completed_at": notification.updated_at.strftime(DATETIME_FORMAT),   # the last time the status was updated
+            "sent_at": notification.sent_at.strftime(DATETIME_FORMAT),           # the time the email was sent
+            "notification_type": notification.notification_type
+        }
+
         response = request(
             method="POST",
             url=service_callback_api.url,
@@ -71,7 +72,15 @@ def send_delivery_status_to_service(self, notification_id):
             )
         )
         if not isinstance(e, HTTPError) or e.response.status_code >= 500:
-            try:
-                self.retry(queue=QueueNames.RETRY)
-            except self.MaxRetriesExceededError:
-                current_app.logger.exception('Retry: send_delivery_status_to_service has retried the max num of times')
+            retry = True
+    except Exception as e:
+        current_app.logger.exception(
+            'Unhandled exception when sending callback for notification {}'.format(notification_id)
+        )
+        retry = True
+
+    if retry:
+        try:
+            self.retry(queue=QueueNames.RETRY)
+        except self.MaxRetriesExceededError:
+            current_app.logger.exception('Retry: send_delivery_status_to_service has retried the max num of times')

manifest-delivery-base.yml

@@ -95,6 +95,6 @@ applications:
       NOTIFY_APP_NAME: delivery-worker-receipts
 
   - name: notify-delivery-worker-service-callbacks
-    command: scripts/run_app_paas.sh celery -A run_celery.notify_celery worker --loglevel=INFO -P eventlet -c 1000 -Q service-callbacks
+    command: scripts/run_app_paas.sh celery -A run_celery.notify_celery worker --loglevel=INFO --concurrency=11 -Q service-callbacks
     env:
-      NOTIFY_APP_NAME: delivery-worker-service-callbacks
+      NOTIFY_APP_NAME: delivery-worker-service-callbacks

tests/app/celery/test_service_callback_tasks.py

@@ -1,10 +1,11 @@
+import uuid
 import json
 from datetime import datetime
 
+from requests import RequestException
 import pytest
 import requests_mock
-
-from requests import RequestException
+from sqlalchemy.exc import SQLAlchemyError
 
 from app import (DATETIME_FORMAT)
 
@@ -18,6 +19,7 @@ from tests.app.db import (
     create_service_callback_api
 )
 from app.celery.service_callback_tasks import send_delivery_status_to_service
+from app.config import QueueNames
 
 
 @pytest.mark.parametrize("notification_type",
@@ -88,7 +90,7 @@ def test_send_delivery_status_to_service_does_not_sent_request_when_service_call
     mocked = mocker.patch("requests.request")
     send_delivery_status_to_service(notification.id)
 
-    mocked.call_count == 0
+    assert mocked.call_count == 0
 
 
 @pytest.mark.parametrize("notification_type",
@@ -182,4 +184,15 @@ def test_send_delivery_status_to_service_does_not_retries_if_request_returns_404
                           status_code=404)
         send_delivery_status_to_service(notification.id)
 
-    mocked.call_count == 0
+    assert mocked.call_count == 0
+
+
+def test_send_delivery_status_to_service_retries_if_database_error(client, mocker):
+    notification_id = uuid.uuid4()
+    db_call = mocker.patch('app.celery.service_callback_tasks.get_notification_by_id', side_effect=SQLAlchemyError)
+    retry = mocker.patch('app.celery.service_callback_tasks.send_delivery_status_to_service.retry')
+
+    send_delivery_status_to_service(notification_id)
+
+    db_call.assert_called_once_with(notification_id)
+    retry.assert_called_once_with(queue=QueueNames.RETRY)