Fix: Do not re-re-deliver unsent mails on failure to re-deliver (#1397)

Co-authored-by: Adrià Casajús <adria.casajus@proton.ch>

app/mail_sender.py

@@ -98,7 +98,7 @@ class MailSender:
     def enable_background_pool(self, max_workers=10):
         self._pool = ThreadPoolExecutor(max_workers=max_workers)
 
-    def send(self, send_request: SendRequest, retries: int = 2):
+    def send(self, send_request: SendRequest, retries: int = 2) -> bool:
         """replace smtp.sendmail"""
         if self._store_emails:
             self._emails_sent.append(send_request)
@@ -109,20 +109,20 @@ class MailSender:
                 send_request.msg[headers.FROM],
                 send_request.msg[headers.TO],
             )
-            return
+            return True
         if not self._pool:
-            self._send_to_smtp(send_request, retries)
+            return self._send_to_smtp(send_request, retries)
         else:
             self._pool.submit(self._send_to_smtp, (send_request, retries))
+            return True
 
-    def _send_to_smtp(self, send_request: SendRequest, retries: int):
+    def _send_to_smtp(self, send_request: SendRequest, retries: int) -> bool:
+        if config.POSTFIX_SUBMISSION_TLS:
+            smtp_port = 587
+        else:
+            smtp_port = config.POSTFIX_PORT
         try:
             start = time.time()
-            if config.POSTFIX_SUBMISSION_TLS:
-                smtp_port = 587
-            else:
-                smtp_port = config.POSTFIX_PORT
-
             with SMTP(config.POSTFIX_SERVER, smtp_port) as smtp:
                 if config.POSTFIX_SUBMISSION_TLS:
                     smtp.starttls()
@@ -160,16 +160,17 @@ class MailSender:
             TimeoutError,
         ) as e:
             if retries > 0:
-                time.sleep(0.3 * send_request.retries)
-                self._send_to_smtp(send_request, retries - 1)
+                time.sleep(0.3 * retries)
+                return self._send_to_smtp(send_request, retries - 1)
             else:
                 if send_request.ignore_smtp_errors:
                     LOG.e(f"Ignore smtp error {e}")
-                    return
+                    return False
                 LOG.e(
                     f"Could not send message to smtp server {config.POSTFIX_SERVER}:{smtp_port}"
                 )
                 self._save_request_to_unsent_dir(send_request)
+                return False
 
     def _save_request_to_unsent_dir(self, send_request: SendRequest):
         file_name = f"DeliveryFail-{int(time.time())}-{uuid.uuid4()}.{SendRequest.SAVE_EXTENSION}"
@@ -201,7 +202,16 @@ def load_unsent_mails_from_fs_and_resend():
         except Exception as e:
             LOG.error(f"Could not load file {filename}: {e}")
             continue
-        mail_sender.send(send_request, 2)
+        send_request.ignore_smtp_errors = True
+        if mail_sender.send(send_request, 2):
+            newrelic.agent.record_custom_event(
+                "DeliverUnsentEmail", {"delivered": "true"}
+            )
+            os.unlink(full_file_path)
+        else:
+            newrelic.agent.record_custom_event(
+                "DeliverUnsentEmail", {"delivered": "false"}
+            )
 
 
 def sl_sendmail(

tests/test_mail_sender.py

@@ -135,12 +135,44 @@ def test_send_unsent_email_from_fs():
         try:
             config.SAVE_UNSENT_DIR = temp_dir
             send_request = create_dummy_send_request()
-            mail_sender.send(send_request, 0)
+            mail_sender.send(send_request, 1)
         finally:
             config.POSTFIX_SERVER = original_postfix_server
             config.NOT_SEND_EMAIL = True
+        saved_files = os.listdir(config.SAVE_UNSENT_DIR)
+        assert len(saved_files) == 1
         mail_sender.purge_stored_emails()
         load_unsent_mails_from_fs_and_resend()
         sent_emails = mail_sender.get_stored_emails()
         assert len(sent_emails) == 1
         compare_send_requests(send_request, sent_emails[0])
+        assert sent_emails[0].ignore_smtp_errors
+        assert not os.path.exists(os.path.join(config.SAVE_UNSENT_DIR, saved_files[0]))
+
+
+@mail_sender.store_emails_test_decorator
+def test_failed_resend_does_not_delete_file():
+    original_postfix_server = config.POSTFIX_SERVER
+    config.POSTFIX_SERVER = "localhost"
+    config.NOT_SEND_EMAIL = False
+    try:
+        with tempfile.TemporaryDirectory() as temp_dir:
+            config.SAVE_UNSENT_DIR = temp_dir
+            send_request = create_dummy_send_request()
+            # Send and store email in disk
+            mail_sender.send(send_request, 1)
+            saved_files = os.listdir(config.SAVE_UNSENT_DIR)
+            assert len(saved_files) == 1
+            mail_sender.purge_stored_emails()
+            # Send and keep email in disk
+            load_unsent_mails_from_fs_and_resend()
+            sent_emails = mail_sender.get_stored_emails()
+            assert len(sent_emails) == 1
+            compare_send_requests(send_request, sent_emails[0])
+            assert sent_emails[0].ignore_smtp_errors
+            assert os.path.exists(os.path.join(config.SAVE_UNSENT_DIR, saved_files[0]))
+            # No more emails are stored in disk
+            assert saved_files == os.listdir(config.SAVE_UNSENT_DIR)
+    finally:
+        config.POSTFIX_SERVER = original_postfix_server
+        config.NOT_SEND_EMAIL = True