diff --git a/api/libs/smtp.py b/api/libs/smtp.py index 53906d1769f..7175bd2159e 100644 --- a/api/libs/smtp.py +++ b/api/libs/smtp.py @@ -60,4 +60,12 @@ class SMTPClient: raise finally: if smtp: - smtp.quit() + # Cleanup must not replace a send failure or turn an accepted send into a failure. + try: + smtp.quit() + except Exception: + logger.warning("Failed to quit SMTP connection", exc_info=True) + try: + smtp.close() + except Exception: + logger.warning("Failed to close SMTP connection", exc_info=True) diff --git a/api/tests/unit_tests/libs/test_smtp_client.py b/api/tests/unit_tests/libs/test_smtp_client.py index 96d62de2d66..481f12b487f 100644 --- a/api/tests/unit_tests/libs/test_smtp_client.py +++ b/api/tests/unit_tests/libs/test_smtp_client.py @@ -1,3 +1,4 @@ +import smtplib from typing import Any from unittest.mock import ANY, MagicMock, patch @@ -95,3 +96,57 @@ def test_smtp_smtplib_exception_in_login(mock_smtp_cls: MagicMock): with pytest.raises(smtplib.SMTPException): client.send(_mail()) mock_smtp.quit.assert_called_once() + + +@pytest.mark.parametrize("use_tls", [False, True]) +@pytest.mark.parametrize( + "send_error_type", + [ + None, + smtplib.SMTPDataError, + TimeoutError, + RuntimeError, + ], +) +@pytest.mark.parametrize("close_fails", [False, True]) +def test_smtp_cleanup_preserves_send_result( + use_tls: bool, + send_error_type: type[Exception] | None, + close_fails: bool, + caplog: pytest.LogCaptureFixture, +): + send_error: Exception | None = None + if send_error_type is smtplib.SMTPDataError: + send_error = smtplib.SMTPDataError(451, b"temporary send failure") + elif send_error_type is not None: + send_error = send_error_type("send failed") + + smtp_class = "libs.smtp.smtplib.SMTP_SSL" if use_tls else "libs.smtp.smtplib.SMTP" + with patch(smtp_class, autospec=True) as mock_smtp_cls: + mock_smtp = mock_smtp_cls.return_value + mock_smtp.sendmail.side_effect = send_error + mock_smtp.quit.side_effect = smtplib.SMTPServerDisconnected("connection lost during QUIT") + if close_fails: + mock_smtp.close.side_effect = OSError("close failed") + client = SMTPClient( + server="smtp.example.com", + port=465 if use_tls else 25, + username="", + password="", + _from="noreply@example.com", + use_tls=use_tls, + ) + + if send_error is None: + assert client.send(_mail()) is None + else: + with pytest.raises(type(send_error)) as exc_info: + client.send(_mail()) + assert exc_info.value is send_error + + mock_smtp.sendmail.assert_called_once() + mock_smtp.quit.assert_called_once() + mock_smtp.close.assert_called_once() + assert "Failed to quit SMTP connection" in caplog.text + if close_fails: + assert "Failed to close SMTP connection" in caplog.text