From 4661972f97350106e88f8e3d7b354a22d436b23b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A0=20Casaj=C3=BAs?= Date: Thu, 10 Nov 2022 13:24:46 +0100 Subject: [PATCH] Fix: When re-sending emails if they trigger exceptions move out of failed dir (#1411) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Fix: When re-sending emails if they trigger exceptions move out of failed dir * Use proper timeout * Lint Co-authored-by: Adrià Casajús --- app/mail_sender.py | 51 +++++++++++++++++++++++++++++++-------- tests/test_mail_sender.py | 19 ++++++++++++--- 2 files changed, 57 insertions(+), 13 deletions(-) diff --git a/app/mail_sender.py b/app/mail_sender.py index faf3e1f9..a6737ed1 100644 --- a/app/mail_sender.py +++ b/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), ) diff --git a/tests/test_mail_sender.py b/tests/test_mail_sender.py index 8bc70064..065b6dfc 100644 --- a/tests/test_mail_sender.py +++ b/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