Fix: When re-sending emails if they trigger exceptions move out of failed dir (#1411)

* Fix: When re-sending emails if they trigger exceptions move out of failed dir

* Use proper timeout

* Lint

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

app/mail_sender.py

@@ -123,7 +123,9 @@ class MailSender:
             smtp_port = config.POSTFIX_PORT
         try:
             start = time.time()
-            with SMTP(config.POSTFIX_SERVER, smtp_port, config.POSTFIX_TIMEOUT) as smtp:
+            with SMTP(
+                config.POSTFIX_SERVER, smtp_port, timeout=config.POSTFIX_TIMEOUT
+            ) as smtp:
                 if config.POSTFIX_SUBMISSION_TLS:
                     smtp.starttls()
 
@@ -154,6 +156,7 @@ class MailSender:
                 newrelic.agent.record_custom_metric(
                     "Custom/smtp_sending_time", time.time() - start
                 )
+                return True
         except (
             SMTPException,
             ConnectionRefusedError,
@@ -184,6 +187,20 @@ class MailSender:
 mail_sender = MailSender()
 
 
+def save_request_to_failed_dir(exception_name: str, send_request: SendRequest):
+    file_name = f"{exception_name}-{int(time.time())}-{uuid.uuid4()}.{SendRequest.SAVE_EXTENSION}"
+    failed_file_dir = os.path.join(config.SAVE_UNSENT_DIR, "failed")
+    try:
+        os.makedirs(failed_file_dir)
+    except FileExistsError:
+        pass
+    file_path = os.path.join(failed_file_dir, file_name)
+    file_contents = send_request.to_bytes()
+    with open(file_path, "wb") as fd:
+        fd.write(file_contents)
+    return file_path
+
+
 def load_unsent_mails_from_fs_and_resend():
     if not config.SAVE_UNSENT_DIR:
         return
@@ -200,17 +217,31 @@ def load_unsent_mails_from_fs_and_resend():
         try:
             send_request = SendRequest.load_from_file(full_file_path)
         except Exception as e:
-            LOG.error(f"Could not load file {filename}: {e}")
+            LOG.e(f"Cannot load {filename}. Error {e}")
             continue
-        send_request.ignore_smtp_errors = True
-        if mail_sender.send(send_request, 2):
-            newrelic.agent.record_custom_event(
-                "DeliverUnsentEmail", {"delivered": "true"}
-            )
+        try:
+            send_request.ignore_smtp_errors = True
+            if mail_sender.send(send_request, 2):
+                os.unlink(full_file_path)
+                newrelic.agent.record_custom_event(
+                    "DeliverUnsentEmail", {"delivered": "true"}
+                )
+            else:
+                newrelic.agent.record_custom_event(
+                    "DeliverUnsentEmail", {"delivered": "false"}
+                )
+        except Exception as e:
+            # Unlink original file to avoid re-doing the same
             os.unlink(full_file_path)
-        else:
-            newrelic.agent.record_custom_event(
-                "DeliverUnsentEmail", {"delivered": "false"}
+            LOG.e(
+                "email sending failed with error:%s "
+                "envelope %s -> %s, mail %s -> %s saved to %s",
+                e,
+                send_request.envelope_from,
+                send_request.envelope_to,
+                send_request.msg[headers.FROM],
+                send_request.msg[headers.TO],
+                save_request_to_failed_dir(e.__class__.__name__, send_request),
             )
 
 

tests/test_mail_sender.py

@@ -114,7 +114,7 @@ def test_mail_sender_save_unsent_to_disk(server_fn):
         with tempfile.TemporaryDirectory() as temp_dir:
             config.SAVE_UNSENT_DIR = temp_dir
             send_request = create_dummy_send_request()
-            mail_sender.send(send_request, 0)
+            assert not mail_sender.send(send_request, 0)
             found_files = os.listdir(temp_dir)
             assert len(found_files) == 1
             loaded_send_request = SendRequest.load_from_file(
@@ -135,7 +135,7 @@ 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, 1)
+            assert not mail_sender.send(send_request, 1)
         finally:
             config.POSTFIX_SERVER = original_postfix_server
             config.NOT_SEND_EMAIL = True
@@ -148,6 +148,8 @@ def test_send_unsent_email_from_fs():
         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]))
+        saved_files = os.listdir(config.SAVE_UNSENT_DIR)
+        assert len(saved_files) == 0
 
 
 @mail_sender.store_emails_test_decorator
@@ -160,7 +162,7 @@ def test_failed_resend_does_not_delete_file():
             config.SAVE_UNSENT_DIR = temp_dir
             send_request = create_dummy_send_request()
             # Send and store email in disk
-            mail_sender.send(send_request, 1)
+            assert not mail_sender.send(send_request, 1)
             saved_files = os.listdir(config.SAVE_UNSENT_DIR)
             assert len(saved_files) == 1
             mail_sender.purge_stored_emails()
@@ -176,3 +178,14 @@ def test_failed_resend_does_not_delete_file():
     finally:
         config.POSTFIX_SERVER = original_postfix_server
         config.NOT_SEND_EMAIL = True
+
+
+@mail_sender.store_emails_test_decorator
+def test_ok_mail_does_not_generate_unsent_file():
+    with tempfile.TemporaryDirectory() as temp_dir:
+        config.SAVE_UNSENT_DIR = temp_dir
+        send_request = create_dummy_send_request()
+        # Send and store email in disk
+        assert mail_sender.send(send_request, 1)
+        saved_files = os.listdir(config.SAVE_UNSENT_DIR)
+        assert len(saved_files) == 0