EmailSendingWorker: Fix retry for sending emails.

If an exception was thrown inside `send_email` resulting in a retry,
we would include the `failed_tries` data in the event, which turned
out to thrown an exception itself.

This fixes that flow, including deepening the test so that it would
fail if we didn't have the new logic.

zerver/tests/test_queue_worker.py

@@ -10,6 +10,7 @@ from django.test import TestCase
 from mock import patch, MagicMock
 from typing import Any, Callable, Dict, List, Mapping, Tuple
 
+from zerver.lib.send_email import FromAddress
 from zerver.lib.test_helpers import simulated_queue_client
 from zerver.lib.test_classes import ZulipTestCase
 from zerver.models import get_client, UserActivity, PreregistrationUser
@@ -171,7 +172,13 @@ class WorkerTest(ZulipTestCase):
         retries sending the email 3 times and then gives up."""
         fake_client = self.FakeClient()
 
-        data = {'test': 'test', 'id': 'test_missed'}
+        data = {
+            'template_prefix': 'zerver/emails/confirm_new_email',
+            'to_email': self.example_email("hamlet"),
+            'from_name': 'Zulip Account Security',
+            'from_address': FromAddress.NOREPLY,
+            'context': {}
+        }
         fake_client.queue.append(('email_senders', data))
 
         def fake_publish(queue_name: str,
@@ -182,7 +189,7 @@ class WorkerTest(ZulipTestCase):
         with simulated_queue_client(lambda: fake_client):
             worker = queue_processors.EmailSendingWorker()
             worker.setup()
-            with patch('zerver.worker.queue_processors.send_email_from_dict',
+            with patch('zerver.lib.send_email.build_email',
                        side_effect=smtplib.SMTPServerDisconnected), \
                     patch('zerver.lib.queue.queue_json_publish',
                           side_effect=fake_publish), \

zerver/worker/queue_processors.py

@@ -1,6 +1,7 @@
 # Documented in https://zulip.readthedocs.io/en/latest/subsystems/queuing.html
 from typing import Any, Callable, Dict, List, Mapping, Optional, cast
 
+import copy
 import signal
 from functools import wraps
 
@@ -112,11 +113,10 @@ def retry_send_email_failures(func):
         # type: (QueueProcessingWorker, Dict[str, Any]) -> None
         try:
             func(worker, data)
-        except (smtplib.SMTPServerDisconnected, socket.gaierror):
+        except (smtplib.SMTPServerDisconnected, socket.gaierror, EmailNotDeliveredException):
-
             def on_failure(event):
                 # type: (Dict[str, Any]) -> None
-                logging.exception("Event {} failed".format(event['id']))
+                logging.exception("Event {} failed".format(event))
 
             retry_event(worker.queue_name, data, on_failure)
 
@@ -299,13 +299,14 @@ class MissedMessageWorker(LoopQueueProcessingWorker):
 @assign_queue('email_senders')
 class EmailSendingWorker(QueueProcessingWorker):
     @retry_send_email_failures
-    def consume(self, data):
+    def consume(self, event: Dict[str, Any]) -> None:
-        # type: (Dict[str, Any]) -> None
+        # Copy the event, so that we don't pass the `failed_tries'
-        try:
+        # data to send_email_from_dict (which neither takes that
-            send_email_from_dict(data)
+        # argument nor needs that data).
-        except EmailNotDeliveredException:
+        copied_event = copy.deepcopy(event)
-            # TODO: Do something smarter here ..
+        if 'failed_tries' in copied_event:
-            pass
+            del copied_event['failed_tries']
+        send_email_from_dict(copied_event)
 
 @assign_queue('missedmessage_email_senders')
 class MissedMessageSendingWorker(EmailSendingWorker):