[19.0][FIX] mail_tracking: do not swallow exceptions in send_email - #266
Open
skanndar wants to merge 1 commit into
Open
[19.0][FIX] mail_tracking: do not swallow exceptions in send_email#266skanndar wants to merge 1 commit into
skanndar wants to merge 1 commit into
Conversation
`MailMail._send()` tells a successful delivery from a failed one by the
exception raised by `send_email()`, not by its return value. When a
`mail.tracking.email` record exists -- which is the case for every outgoing
HTML mail, since `mail_tracking` creates one in `_prepare_outgoing_list()` --
the override recorded the error on the tracking record and returned normally,
so core believed the mail had been delivered:
* the recipient was appended to `success_pids` and
`_postprocess_sent_message()` wrote `notification_status = 'sent'` on a
notification for a mail that never left;
* core's `except AssertionError` never ran, so a `NO_VALID_RECIPIENT` was
never classified as `mail_email_invalid`;
* `res` stayed falsy, so the mail kept the placeholder failure reason set
before the sending loop ("Error without exception. Probably due to sending
an email without computed recipients.") with `failure_type = 'unknown'`,
even when the other recipients had been delivered.
Re-raise unconditionally. The error is still recorded on the tracking record
by the `smtp_error()` call just above, so nothing is lost; only core's
contract is restored. The previous `else: raise` (added in OCA#212 / OCA#215) is
kept as behaviour for the case with no tracking record.
Note for adopters: delivery failures that used to be silent now surface as
`exception` states on `mail.mail` and `mail.notification`.
Fixes OCA#253
pedrobaeza
reviewed
Sep 10, 2026
pedrobaeza
left a comment
Member
There was a problem hiding this comment.
The tests generated by AI are synthetic and are not bringing real flows, so better to remove them.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #253
The problem
MailMail._send()tells a successful delivery from a failed one by the exception raised bysend_email(), not by its return value.mail_trackingcatches every exception and, when amail.tracking.emailrecord exists, records the error on the tracking record and returnsnormally, so core believes the mail was delivered:
success_pidsand_postprocess_sent_message()writesnotification_status = 'sent'on a notification for a mail that never left — an email thatdid not arrive is displayed to users as delivered, with no trace anywhere;
except AssertionErrornever runs, so aNO_VALID_RECIPIENTis never classified asmail_email_invalid;resstays falsy, so the mail keeps the placeholder failure reason core sets before thesending loop ("Error without exception. Probably due to sending an email without computed
recipients.") with
failure_type = 'unknown', even when the other recipients were delivered.A tracking record is created for every outgoing email
(
mail_tracking/models/mail_mail.py::_prepare_outgoing_list), so this is not an edge case: withmail_trackinginstalled, core's whole failure-handling path is disabled for the database.Not a duplicate of #215 / #212
#215 added
else: raise, i.e. it re-raises only when there is no tracking record — thebranch that is effectively dead in practice. The case reported in #253 is the other one and is
still live on
19.0(checked atee6a0e79) and on18.0.The change
Re-raise unconditionally. The error is still recorded on the tracking record by the
smtp_error()call just above, so nothing is lost; only core's contract is restored.Tests
Two regression tests build the
mail.message+mail.mail+mail.notificationtrio core usesto decide whether a recipient was reached:
test_smtp_error_is_not_reported_as_sent— fails without the patch withAssertionError: 'sent' == 'sent'(the notification of a mail that never left is marked asdelivered) and, further down,
mail.failure_reasonis the placeholder instead of the real one;test_smtp_error_still_tracks_the_error— checks the tracking record is still annotated andthat core can now classify the failure (
failure_type == 'mail_email_invalid'), which failswithout the patch with
AssertionError: 'mail_email_invalid' != 'unknown'.Module suite on
odoo:19(19.0-20260817), fresh database: 46 tests / 0 failures before,48 tests / 0 failures after.
This is a behaviour change with a visible consequence: delivery failures that used to be silent
now surface as
exceptionstates onmail.mailandmail.notification. On a productiondatabase with this patch, a backlog of previously hidden failures became visible at once. That is
the correct outcome — the tracking records had been in
errorstate all along, the state wassimply never propagated — but it will look like a regression to anyone upgrading without warning.