Send failures retried forever

I think the main scenario to retry is if your entire transport has failed or is misconfigred, so there wasn't even an attempt to send the mail.

It's key here to clarify our understanding of the interface definitions MailManagerInterface and MailInterface. Both specify that the result indicates "the mail was successfully accepted for delivery".

1. If we interpret this literally, it means that bounced email addresses should actually return success, because they were accepted for delivery. In this view, success means don't bother to retry I am handling it (e.g. SMTP might be retrying).
2. If we follow a more "natural" approach we could return fail for any error. The interface description could be viewed as accidentally too specific to the default PHP mail implementation and it's OK for other implementations to differ.

In a way Drupal is mostly consistent with option 1. MailManager logs a fail with an error. Simplenews retries because it wasn't yet accepted for delivery. It is SwiftMailer module that fails to follow option 1 because it returns FALSE even though the message.

Concrete things we could do

1. Try to fix SwiftMailer module! It's tricky because the library doesn't distinguish different types of failure. You could test the transport with transport->ping, and if the transport is up but the message fails it suggests a specific fault in the message so not worth retrying, hence return success.
2. Even if we follow option 1, it still seems like a bad idea to retry forever. We could have a max retry limit, configurable in simplenews settings (and possibly also retry interval for sites with frequent hook_cron?). We could reuse the spool result which is currently described as boolean but it should hold up to 128 at least. This allows a policy of no retries by assigning 0.
3. Next question is what to do once any allowed retries are used up. Options I can see are discard, or keep in the spool, which implies a new SpoolStorageInterface::STATUS_XXX. If keeping presumably there ought to be a time limit.
4. The main reason for keeping is as you say to allow manual operations for retry. But as a site admin I really don't want to be doing manual operations. So maybe we shouldn't bother to keep them?
5. We could try to detect if there is a global failure and if so a) stop sending more b) grant those already failed an exemption from discarding.

OK that's a bit of a brainstorming, but I think I'll stop writing now and let you respond indicating which ideas are attractive to you.

OK please see updated IS and let me know what you think.

I wonder if some admins might prefer to turn off automatic expiry and clear them by hand and probably it's not that hard to add. However we can review progress as things go along and skip it if needs be. I changed 'discard' to 'clear'.

Yes it's useful to discuss this. One of our objectives is to start off with a minimal implementation and build up.

I see two essential aspects: prevent retries, and discard errors after a certain number of days. In normal cases these two together mean that a site will run unattended without creating major problems. The main hassle for the site admin is handling bounces - either via email (NDR) or a direct synchronous error (could be automated via #3053756: Automatically block bouncing subscriber).

I guess as a first step I can get this part working and then we can re-evaluate.

Normally the site-admin will not want to retry, but they would need to if there was a failure in the entire mail transport (although the "test email" idea could bypass that problem). The simplest to implement is a global retry button. Can you think of any circumstances where someone would want to control retries on a per newsletter basis?

So far we have not touched the per-newsletter UI at all. With the existing UI, the newsletter would be stuck on 47/51 which would imply 4 errors. The fixes to the newsletter UI will roughly double the time taken. Allowing retries per newsletter requires most or all of the per-newsletter UI.

Discarding errors is nice, but not vital

True.

So far we've not mentioned any way for the site-admin to see the failures (currently they would have to search the logs). This could be useful to block/delete a subscription that always fails. Combining this with the UI for retrying, perhaps what we want is a new view of all failed mail spool with retry/clear task buttons?

I had imagined this would be simple as the views code for mailspool exists, but I just tested and it totally fails with "Broken/missing handler". I'll raise an issue and take a quick look how hard it is to fix.

Good discussion, I think we are improving our ideas step by step.

NB the terminology is that a newsletter is a config entity and what are talking about here is a newsletter issue (a node) - I am trying to teach myself to use the right word:-)

So there are two categories of error:

• Global - the mails are not leaving our server. These should typically be retried once the problem is fixed. In fact to insist on trying to send 10k mails is a waste of time and instead the cron run should probably be aborted in case of global error.
• Specific mailbox - problem reported by destination MTA. These should not be retried. The subscriber should potentially be blocked from any future issue, but we ought to distinguish soft and hard bounces - e.g. mailbox full is a soft bounce that could later be resolved.

The focus of this issue is to stop retries of mailbox errors. The problem is that we don't easily know which category an error is. We could try and figure it out with:

1. "test send" - if that works then following errors are mailbox
2. your option (B) which is observing a pattern so that if all mails on an issue fail it is likely (but not guaranteed!) to be global; more practical instead to say if all mails on a given cron run fail; slightly tricky to code because you only find out the answer at the end of the cron run
3. enhancing the error reporting so the transport optionally can indicate the category - simplenews patch to set $message['error_category'] (core doesn't have to be patched although it would be desirable to do so to document the setting)

Proposal

So perhaps we need a new config setting for configuring detection of mailbox errors. By default it is disabled (all global) then there could be multiple different strategies as above. We can start with just one strategy.

Replies to your comments

stop retries. No setting, just stop it. Retrying every failure is never the right solution.

As above, global failure retires seem fine. Each failed retry will not contribute to blacklist because no message is sent.

In any case from a maintainer perspective, I think I would need to require that we keep an option to preserve the existing behaviour. If a site uses the default PHP mailer then all errors are global, in which case the current behaviour of always retrying is basically fine.

B) If all mails in a newsletter fail, put the newsletter into a failed state where the send action flushes the failures from the spool and respools the newsletter.

As above, the existing simplenews behaviour seems reasonable - automatically send the mails once the transport has been fixed.

Once again I'm not sure why this needs to be manually done per issue. It's true that if the global failure was unnoticed for a long time then some issues might no longer be relevant. However that case could be handled by cancelling them.

C) if only some mails in a newsletter failed throw an event for each individual mail failure.

Yes definitely - don't retry; delete the spool entry and allow the issue to be marked as completed with statistics such as 45/48.

Thanks.

The IS proposal seems fine to me. It would be helpful to distinguish must-have (this issue) from related follow-ups.

Bed pardon I don't understand what you are asking. With the new proposed resolution I am not proposing any follow-ups. Provided that the site has a "Test email address" it should work well. There is no need for manual retry, and no complicated new status on the newsletter issue - it will simply be 47/51 i.e. 4 failed and you find the failures in the log. Does that answer your question?

I guess there is one follow up I could raise. We could try to get the hook into core as it would be useful beyond just simplenews. However core patches tend to use up a lot of time:-)

For example, I wondered if the test address hook implementation should be a stage 2.

I guess the problem is how do I test it without at least one implementation. I think we've reduced the complexity to the point that it's sufficiently simple to be hard to divide into stages. I can report back with a patch without tests as the next step for review.

Also, the proposal makes it relatively hard for sites to have the current behavior of retries of failures.

Yes this is important and was fixed with a recent IS edit. If they leave the test address unset, which is the default, then they get the current behaviour. So it's fully BC.

In order to disable retries, provide a "test email address"

Or implement the hook or install a different module that implements it. It doesn't really feel twisty to me - give the module a way to know whether to retry and it will retry intelligently. We could put a warning on the status page if there are no hook implementations.

Wouldn't it be much kinder to future souls to:

I'm a bit surprised because this seems to be heading back to problems we had just avoided.

Only retry failures if the setting is true and the failure is specific.

Surely we never want to retry specific failures - that's when we risk blacklisting? It's global ones that we need to retry.

"Retry failed emails" and default if to false; use an update hook to set it to true for existing installs.

So I agree this gives BC in the sense that existing sites aren't changed. However it changes the behaviour for newly installed sites in what seems a bad way. The default behaviour becomes not to retry but discard. However this seems wrong for the default PHP transport where retries are fairly harmless. Discarding might be hard to detect and difficult to recover from (newsletters don't allow resending). I can't see that people want mails to be discarded when they have some global config error.

As an optimisation of your suggestion, I think it would be fine to wait until the first error before sending a test mail. So I think that what you are describing is effectively the same as what I described with the addition of caching of the result of sending a test mail. Yes I had in mind that we might want to do that.

The caching could be done within the hook as a static variable. We might want to be slightly more subtle and for example retry the test after every N failures. We could also reset the failure count for every successful send - in which case we'd better also call the hook for success.

I think these are all details that would be fairly easy to adjust later on. Let me send you a basic patch that's working and it should be easier to discuss it with that context.

Let's try to get this in before 2.x beta.

Here is a partial patch that adds the new hook but without providing the implementation.

I ended up doing some refactoring of Mailer::sendSpool as it was a bit baffling and contorted from the history. I imagine that at first it was coded to send all the mails and then process results. Later it was discovered that this caused duplicate mails in the cases of PHP timeouts. In the checked-in version, the block of code commented "Update spool status" is duplicated (very nearly). I need to alter that code around block. This block of code is a loop and at first it is not obvious why, or even more odd, why it need to be there a second time. The answer is that SpoolList::nextMail can fail to load some subscribers before returning one. With hindsight now I have grasped everything I probably could have done it without refactoring, but the new code seems much clearer so I propose to keep it.

What is actually going on is:

1. Must run $this->spoolStorage->updateMails immediately for each mail else we get duplicates in case of timeout. Let's have a new function SpoolList::setResult that does this cleanly and clearly one mail at a time.
2. Update subscriber count is done once at the end. Therefore no point in doing "Update subscriber count." before the end either.

This is a minor BC break, but 8.2 is still an alpha. SpoolStorage::getMails has a hard-coded call to create SpoolList. However someone could have overriden that with their own function that creates a different class. It's probably not a very likely thing to have done and the required action to fix is pretty simple.

I would welcome any review comments at this stage, plus I'd like to check the tests still all pass.

OK this is ready for preliminary review please. Please see the updated IS for explanation and status (see list of remaining tasks).

I am wondering about going a step further on the SpoolList enhancements. We could move the code for counting error/fail/skip into SpoolListInterface and pass SpoolListInterface though to the the new hook. This would allow a decision to be made on the basis of cumulative results.

Another point is that so far there is nowhere in the UI that indicates the number of errors for sending an issue - possibly we should add that?

Thanks @jonathanshaw. The major point for discussion is 3.

1. Yup (depending on answer to 3 if we still keep this hook). Probably better if we put the logging and cleanup into the core code. I will likely write an implementation in any case which can serve as an example.

2. Moving the exception is not BC and would require code updates from anyone who has written code to throw it. True we can make non-BC changes now, but we should try to minimise them and I don't feel this one is worth it.

3. Good point. However the current idea of using a test email address cannot be the default because it requires the site admin to configure an address. We could switch to one of the other ideas and I think the most likely one is this:

If a fail is followed later in the same spool run by a success then count it as a specific error.

It's definitely easier to use if there isn't a new config setting. We could get stuck with a set of spool entries that all repeatedly fail and no way to tell what type of error it is - but that feels like an edge case. So my feeling is that we start off by keeping it simple. There's nothing stopping us from later adding the test address in addition.

4. Will do.

5. Pending answer to 3.

OK so now we don't have to be BC we can simplify matters.

The result array with an int and a bool really isn't easy to code with or to understand. A single int is sufficient and much clearer. I have added a new constant SpoolStorageInterface::STATUS_FAILED which removes the need for the extra boolean.

Retrying forever is not really desirable for any site. The code now stores pending "unclassified" errors (STATUS_PENDING). If one of the following mails succeeds, pending errors are converted specific failures (STATUS_FAILED). If there are too many pending errors, then the spool run is aborted (AbortSendingException).

If the reviewers are in favour of this idea then I will tidy up further as listed in "remaining tasks".

NB #36.5 - there is no further information available. There is no caught exception. The result of $mail is already passed as the $message parameter.

OK this is ready for another review please.

The latest patch is a bigger hit to BC, but it's a win for the future by simplifying a lot of unnecessary complications. The "double return parameter passed as array" was ugly to code with and tended to be buggy (e.g. the old statistics counting in Mailer::sendSpool() could count some rows twice and some not at all).

Thanks @jonathanshaw. Done except where noted:

2. This code hasn't really changed from before in semantics. The preceding code has covered DONE and SKIPPED. If it's set to any other state, it seems it can only be because there was an error. The likely cases are STATUS_FAILED or STATUS_PENDING (= retrying failure), which I think can both reasonably be counted as failed. I added a comment. STATUS_HOLD and STATUS_IN_PROGRESS are not really expected at this point, but even if they do occur from the hook they tend to indicate that there will eventually be a retry so our best guess is that the first try didn't work.

5. The original code has a single efficient database write setting all the keys to the same value. Your proposal would make it more complex and less efficient by passing an array of values.

8. I have improved the comment. Note that the keys listed are extra ones in addition to the mail spool row keys.

10. I think our main duty here is to write tests for the new code paths. Once that's done we can reasonably expect future developers to keep our new code paths working.

We are fixing a valid issue causing real problems. It was difficult to fix the issue without some refactoring of code (which we could say was surprising/confusing/unhelpful in places). I have done my best to intuit what was in the minds of the original coders, and keep that working but sometimes it's hard to tell. If previous developers had scenarios that they considered important to generate a particular result, then obviously they had the option to write a test.

However if you are keen that this work includes back-filling missing tests for existing function then let's discuss that off-line.

New patch counting and displaying of errors.

Also I have altered the heuristic to classify unknown errors. If any single mail in the list succeeds, then we now assume the mail transport is good and all errors are specific failures. The previous approach seemed too likely to leave a few mails stuck retrying.

Changes in this patch:

• Fix test failures and coding standards
• Add example code in the hook
• Update hook to remove the simplenews_mail_spool error property

OK I've done everything on my list and the patch is green so I'm ready to commit. I will wait 1 week in case anyone has any comments.

I didn't upload an "only tests" patch as it seems pretty obvious that the tests can't possibly pass on their own.