diff --git a/app/mail_sender.py b/app/mail_sender.py index a6737ed17..c3acc2bd0 100644 --- a/app/mail_sender.py +++ b/app/mail_sender.py @@ -1,6 +1,7 @@ from __future__ import annotations import base64 import email +import enum import json import os import time @@ -8,7 +9,7 @@ from concurrent.futures import ThreadPoolExecutor from email.message import Message from functools import wraps -from smtplib import SMTP, SMTPException +from smtplib import SMTP, SMTPException, SMTPDataError from typing import Optional, Dict, List, Callable import newrelic.agent @@ -68,6 +69,12 @@ def load_from_bytes(data: bytes) -> SendRequest: ) +class SendResult(enum.Enum): + OK = 1 + Failed = 2 + Bounced = 3 + + class MailSender: def __init__(self): self._pool: Optional[ThreadPoolExecutor] = None @@ -98,7 +105,7 @@ def wrapper(*args, **kwargs): def enable_background_pool(self, max_workers=10): self._pool = ThreadPoolExecutor(max_workers=max_workers) - def send(self, send_request: SendRequest, retries: int = 2) -> bool: + def send(self, send_request: SendRequest, retries: int = 2) -> SendResult: """replace smtp.sendmail""" if self._store_emails: self._emails_sent.append(send_request) @@ -109,14 +116,30 @@ def send(self, send_request: SendRequest, retries: int = 2) -> bool: send_request.msg[headers.FROM], send_request.msg[headers.TO], ) - return True + return SendResult.OK if not self._pool: - return self._send_to_smtp(send_request, retries) + return self._send_with_retries_if_needed(send_request, retries) else: - self._pool.submit(self._send_to_smtp, (send_request, retries)) - return True + self._pool.submit( + self._send_with_retries_if_needed, (send_request, retries) + ) + return SendResult.OK + + def _send_with_retries_if_needed( + self, send_request: SendRequest, retries: int + ) -> SendResult: + result = self._send_to_smtp_server(send_request) + if result != SendResult.Failed: + return result + if retries > 0: + time.sleep(0.3 * retries) + return self._send_with_retries_if_needed(send_request, retries - 1) + else: + if not send_request.ignore_smtp_errors: + self._save_request_to_unsent_dir(send_request) + return SendResult.Failed - def _send_to_smtp(self, send_request: SendRequest, retries: int) -> bool: + def _send_to_smtp_server(self, send_request: SendRequest) -> SendResult: if config.POSTFIX_SUBMISSION_TLS and config.POSTFIX_PORT == 25: smtp_port = 587 else: @@ -156,24 +179,25 @@ def _send_to_smtp(self, send_request: SendRequest, retries: int) -> bool: newrelic.agent.record_custom_metric( "Custom/smtp_sending_time", time.time() - start ) - return True + return SendResult.OK + except SMTPDataError as e: + if not send_request.ignore_smtp_errors: + LOG.e( + f"Could not send message to smtp server {config.POSTFIX_SERVER}:{smtp_port}" + ) + if e.smtp_code >= 500: + return SendResult.Bounced + return SendResult.Failed except ( SMTPException, ConnectionRefusedError, TimeoutError, - ) as e: - if retries > 0: - 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 False + ): + if not send_request.ignore_smtp_errors: 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 + return SendResult.Failed def _save_request_to_unsent_dir(self, send_request: SendRequest): file_name = f"DeliveryFail-{int(time.time())}-{uuid.uuid4()}.{SendRequest.SAVE_EXTENSION}" @@ -221,15 +245,12 @@ def load_unsent_mails_from_fs_and_resend(): continue try: send_request.ignore_smtp_errors = True - if mail_sender.send(send_request, 2): + response = mail_sender.send(send_request, 2) + newrelic.agent.record_custom_event( + "DeliverUnsentEmail", {"delivered": f"{response.name}"} + ) + if response != SendResult.Failed: 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) @@ -254,7 +275,7 @@ def sl_sendmail( is_forward: bool = False, retries=2, ignore_smtp_error=False, -): +) -> SendResult: send_request = SendRequest( envelope_from, envelope_to, diff --git a/email_handler.py b/email_handler.py index 689dd5661..8f8eba4fc 100644 --- a/email_handler.py +++ b/email_handler.py @@ -146,7 +146,7 @@ from app.handler.unsubscribe_generator import UnsubscribeGenerator from app.handler.unsubscribe_handler import UnsubscribeHandler from app.log import LOG, set_message_id -from app.mail_sender import sl_sendmail +from app.mail_sender import sl_sendmail, SendResult from app.message_utils import message_to_bytes from app.models import ( Alias, @@ -1249,7 +1249,7 @@ def handle_reply(envelope, msg: Message, rcpt_to: str) -> (bool, str): add_dkim_signature(msg, alias_domain) try: - sl_sendmail( + send_result = sl_sendmail( generate_verp_email(VerpType.bounce_reply, email_log.id, alias_domain), contact.website_email, msg, @@ -1257,6 +1257,8 @@ def handle_reply(envelope, msg: Message, rcpt_to: str) -> (bool, str): envelope.rcpt_options, is_forward=False, ) + if send_result == SendResult.Bounced: + return False, status.E506 # if alias belongs to several mailboxes, notify other mailboxes about this email other_mailboxes = [mb for mb in alias.mailboxes if mb.email != mailbox.email] diff --git a/tests/test_mail_sender.py b/tests/test_mail_sender.py index 065b6dfc4..7bab8c17f 100644 --- a/tests/test_mail_sender.py +++ b/tests/test_mail_sender.py @@ -14,6 +14,7 @@ mail_sender, SendRequest, load_unsent_mails_from_fs_and_resend, + SendResult, ) from app import config @@ -96,16 +97,19 @@ def compare_send_requests(expected: SendRequest, request: SendRequest): @pytest.mark.parametrize( - "server_fn", + "server_fn, should_save", [ - close_on_connect_dummy_server, - closed_dummy_server, - smtp_response_server("421 Retry"), - smtp_response_server("500 error"), + (close_on_connect_dummy_server, True), + (closed_dummy_server, True), + (smtp_response_server("421 Retry"), True), + (smtp_response_server("500 error"), False), ], ) -def test_mail_sender_save_unsent_to_disk(server_fn): +def test_mail_sender_save_unsent_to_disk_if_failed( + server_fn: Callable, should_save: bool +): original_postfix_server = config.POSTFIX_SERVER + original_postfix_port = config.POSTFIX_PORT config.POSTFIX_SERVER = "localhost" config.NOT_SEND_EMAIL = False config.POSTFIX_SUBMISSION_TLS = False @@ -114,16 +118,20 @@ 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() - assert not mail_sender.send(send_request, 0) + assert mail_sender.send(send_request, 0) != SendResult.OK found_files = os.listdir(temp_dir) - assert len(found_files) == 1 - loaded_send_request = SendRequest.load_from_file( - os.path.join(temp_dir, found_files[0]) - ) - compare_send_requests(loaded_send_request, send_request) + if should_save: + assert len(found_files) == 1 + loaded_send_request = SendRequest.load_from_file( + os.path.join(temp_dir, found_files[0]) + ) + compare_send_requests(loaded_send_request, send_request) + else: + assert len(found_files) == 0 finally: config.POSTFIX_SERVER = original_postfix_server config.NOT_SEND_EMAIL = True + config.POSTFIX_PORT = original_postfix_port @mail_sender.store_emails_test_decorator @@ -135,7 +143,7 @@ def test_send_unsent_email_from_fs(): try: config.SAVE_UNSENT_DIR = temp_dir send_request = create_dummy_send_request() - assert not mail_sender.send(send_request, 1) + assert mail_sender.send(send_request, 1) == SendResult.Failed finally: config.POSTFIX_SERVER = original_postfix_server config.NOT_SEND_EMAIL = True @@ -162,7 +170,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 - assert not mail_sender.send(send_request, 1) + assert mail_sender.send(send_request, 1) == SendResult.Failed saved_files = os.listdir(config.SAVE_UNSENT_DIR) assert len(saved_files) == 1 mail_sender.purge_stored_emails() @@ -186,6 +194,6 @@ def test_ok_mail_does_not_generate_unsent_file(): 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) + assert mail_sender.send(send_request, 1) == SendResult.OK saved_files = os.listdir(config.SAVE_UNSENT_DIR) assert len(saved_files) == 0