[2.1]: Mail queue not emptying

[jdarwood007]

Basic Information

The mail queue was updated in 2.1.5 to address emails that can't be sent by adding a priority. I missed the code to have it delete emails after they reached the timeout (127). This should be a release blocker and a simple fix.

Steps to reproduce

Expected result

No response

Actual result

No response

Version/Git revision

2.1.5 GIT

Database Engine

All

Database Version

No response

PHP Version

No response

Logs

No response

Additional Information

No response

[Sesquipedalian]

It looks like you actually did add code to drop undeliverable email messages, @jdarwood007:

SMF/Sources/ScheduledTasks.php

Lines 808 to 811 in 938503e

	// Old emails should expire
	if (!$result && $email['priority'] >= $max_priority)
	$result = true;

If there is some other code that needs to be added elsewhere, please reopen this issue and add more details.

[jdarwood007]

I'm still testing another fix. Seems like it sometimes doesn't delete from the queue. In all this is just because I'm trying to fix this without a database change so I'm abusing the unused priority column to try to determine when we need to resend without dos remote mail servers.

[Sesquipedalian]

In that case, maybe it would be simpler and easier to base the decision to discard an email on the value of $email['time_sent'] rather than the value of $email['priority']. Keep doing what you are doing with the priority in order to avoid hammering remote mail servers, but let the decision to discard the email be something like this:

// Old emails should expire
if (!$result && time() - $email['time_sent'] > 86400 * 3) {
	$result = true;
}

That will give up after three days of unsuccessfully trying to send the email. Meanwhile, your code to adjust the priority will only have to deal with how often to retry and not with when to give up.

[jdarwood007]

The problem is, that our mail queue process will run as specified, let's say 60 seconds. Every 60 seconds if it's the only mail in the queue, it tries to send it. Which may upset some mail servers. Ideally, we would do something like slowly add more time. Such as every attempt we add something like a 30% increase to the time we next try it.

For 3.0 I was thinking we would track a "next try" stamp and "tries". We would slowly add time until we have tried as many times as we think is correct (like 20).

[Sesquipedalian]

Sure, but throttling the retry rate is a different task than setting an expiry date. The problem where old messages are never being discarded from the queue is about the latter, not the former.

[jdarwood007]

Any suggestions?

I'm running a test right now where I select (from our $number) 70% from the queue and then the remaining 30% is random, running those and processing them, adding a random check between 0 and 1. Increasing the priority by 1 every time it runs. Should ensure new mail gets selected but also tries randomly to grab older emails and process them. Ensuring that even if the queue gets clogged with bad emails, we still try fairly well to reprocess all of them and not dos.

[Sesquipedalian]

Here's a suggestion.

Find:

	// Send each email, yea!
	$failed_emails = array();
	$max_priority = 127;
	$smtp_expire = 259200;
	$priority_offset = 4;
	foreach ($emails as $email)
	{
		// This seems odd, but check the priority if we should try again so soon. Do this so we don't DOS some poor mail server.
		if ($email['priority'] > $priority_offset && (time() - $email['time_sent']) % $priority_offset != rand(0, $priority_offset))
		{
			$failed_emails[] = array($email['to'], $email['body'], $email['subject'], $email['headers'], $email['send_html'], $email['time_sent'], $email['private'], $email['priority']);
			continue;
		}

		if (empty($modSettings['mail_type']) || $modSettings['smtp_host'] == '')
		{
			$email['subject'] = strtr($email['subject'], array("\r" => '', "\n" => ''));
			if (!empty($modSettings['mail_strip_carriage']))
			{
				$email['body'] = strtr($email['body'], array("\r" => ''));
				$email['headers'] = strtr($email['headers'], array("\r" => ''));
			}

			// No point logging a specific error here, as we have no language. PHP error is helpful anyway...
			$result = mail(strtr($email['to'], array("\r" => '', "\n" => '')), $email['subject'], $email['body'], $email['headers']);

			// Try to stop a timeout, this would be bad...
			@set_time_limit(300);
			if (function_exists('apache_reset_timeout'))
				@apache_reset_timeout();
		}
		else
			$result = smtp_mail(array($email['to']), $email['subject'], $email['body'], $email['headers']);

		// Old emails should expire
		if (!$result && $email['priority'] >= $max_priority)
			$result = true;

		// Hopefully it sent?
		if (!$result)
		{
			// Determine the "priority" as a way to keep track of SMTP failures.
			$email['priority'] = max($priority_offset, $email['priority'], min(ceil((time() - $email['time_sent']) / $smtp_expire * ($max_priority - $priority_offset)) + $priority_offset, $max_priority));

			$failed_emails[] = array($email['to'], $email['body'], $email['subject'], $email['headers'], $email['send_html'], $email['time_sent'], $email['private'], $email['priority']);
		}
	}


	// Any emails that didn't send?
	if (!empty($failed_emails))
	{
		// Update the failed attempts check.
		$smcFunc['db_insert']('replace',
			'{db_prefix}settings',
			array('variable' => 'string', 'value' => 'string'),
			array('mail_failed_attempts', empty($modSettings['mail_failed_attempts']) ? 1 : ++$modSettings['mail_failed_attempts']),
			array('variable')
		);

		// If we have failed to many times, tell mail to wait a bit and try again.
		if ($modSettings['mail_failed_attempts'] > 5)
			$smcFunc['db_query']('', '
				UPDATE {db_prefix}settings
				SET value = {string:next_mail_send}
				WHERE variable = {literal:mail_next_send}
					AND value = {string:last_send}',
				array(
					'next_mail_send' => time() + 60,
					'last_send' => $modSettings['mail_next_send'],
				)
			);

		// Add our email back to the queue, manually.
		$smcFunc['db_insert']('insert',
			'{db_prefix}mail_queue',
			array('recipient' => 'string', 'body' => 'string', 'subject' => 'string', 'headers' => 'string', 'send_html' => 'string', 'time_sent' => 'string', 'private' => 'int', 'priority' => 'int'),
			$failed_emails,
			array('id_mail')
		);

		return false;
	}
	// We where unable to send the email, clear our failed attempts.
	elseif (!empty($modSettings['mail_failed_attempts']))
		$smcFunc['db_query']('', '
			UPDATE {db_prefix}settings
			SET value = {string:zero}
			WHERE variable = {string:mail_failed_attempts}',
			array(
				'zero' => '0',
				'mail_failed_attempts' => 'mail_failed_attempts',
			)
		);

	// Had something to send...
	return true;

Replace:

	// Send each email, yea!
	$failed_emails = array();
	$max_priority = 127;
	$smtp_expire = 259200;
	$priority_offset = 8;

	foreach ($emails as $email)
	{
		// First, figure out when the next send attempt should happen based on the current priority.
		$next_send_time = $email['time_sent'];

		for ($i = 0; $i < $email['priority']; $i++)
		{
			$next_send_time += 20 * max(0, $email['priority'] - $priority_offset);
		}

		// If the email is too old, discard it.
		if ($next_send_time > $email['time_sent'] + $smtp_expire)
		{
			continue;
		}

		// Don't send if it's too soon. Also, if we've already failed a few times, only send on every fourth attempt so that we don't DOS some poor mail server.
		if (time() < $next_send_time || ($email['priority'] >= $priority_offset && $email['priority'] % 4 !== 0))
		{
			if ($email['priority'] < $max_priority)
			{
				$failed_emails[] = array($email['to'], $email['body'], $email['subject'], $email['headers'], $email['send_html'], $email['time_sent'], $email['private'], $email['priority']++);
			}

			continue;
		}

		// Try to send.
		if (empty($modSettings['mail_type']) || $modSettings['smtp_host'] == '')
		{
			$email['subject'] = strtr($email['subject'], array("\r" => '', "\n" => ''));

			if (!empty($modSettings['mail_strip_carriage']))
			{
				$email['body'] = strtr($email['body'], array("\r" => ''));
				$email['headers'] = strtr($email['headers'], array("\r" => ''));
			}

			// No point logging a specific error here, as we have no language. PHP error is helpful anyway...
			$result = mail(strtr($email['to'], array("\r" => '', "\n" => '')), $email['subject'], $email['body'], $email['headers']);

			// Try to stop a timeout, this would be bad...
			@set_time_limit(300);

			if (function_exists('apache_reset_timeout'))
			{
				@apache_reset_timeout();
			}
		}
		else
		{
			$result = smtp_mail(array($email['to']), $email['subject'], $email['body'], $email['headers']);
		}

		// If we failed, and we haven't already hit the limit, schedule this for another attempt.
		if (empty($result) && $email['priority'] < $max_priority)
		{
			$failed_emails[] = array($email['to'], $email['body'], $email['subject'], $email['headers'], $email['send_html'], $email['time_sent'], $email['private'], $email['priority']++);
		}
	}

	// Any emails that didn't send? Add them back to the queue.
	if (!empty($failed_emails))
	{
		$smcFunc['db_insert']('insert',
			'{db_prefix}mail_queue',
			array('recipient' => 'string', 'body' => 'string', 'subject' => 'string', 'headers' => 'string', 'send_html' => 'string', 'time_sent' => 'string', 'private' => 'int', 'priority' => 'int'),
			$failed_emails,
			array('id_mail')
		);

		return false;
	}


	// Had something to send...
	return true;

Note that I have not tested this on a live server.

The bit under the comment // If the email is too old, discard it. is a fail-safe that forces us to discard emails after a certain period of time (currently 3 days, as determined by the $smtp_expire variable). However, it isn't strictly necessary. If it were removed, we would keep trying until the priority reached the maximum, but then we would still discard the failed email.

The part that figures out $next_send_time is fairly straightforward. It simply adds a number of seconds to the original value of $email['time_sent'] in order to determine when the send attempt should happen. The trick is that as the value of $email['priority'] increases, the amount of time added increases exponentially (meaning, both the number of time increments and the size of the time increments increases).

As explained by the // Don't send if it's too soon. Also, if we've already failed a few times, only send on every fourth attempt so that we don't DOS some poor mail server comment, the ($email['priority'] >= $priority_offset && $email['priority'] % 4 !== 0) bit serves to further throttle our attempts by only trying to send when $email['priority'] is greater than 8 and is a multiple of 4. In other words, once we've failed a few times, we cut down the retry rate to one fourth of what it would have been otherwise.

As I said, I haven't actually tested this on a live server, but it seemed to go pretty well in my mock-ups.

[jdarwood007]

I'm testing your code, it falls into the same problem I am having with my code. When the queue fills up and more sits in the queue than can be processed in a run, the old stuff never gets ran. If you have your queue setup to process 20 emails a pass and you have 25 emails all sitting in the queue that won't sent, the last 5 are not touched until the others that it keeps trying to send have expired. If you get a new batch, they have a lower priority and even if they get stuck, they are the only ones being attempted, the others stuck in the queue don't get touched.

I think it falls into my idea I started to test which is 70% of the $number (in my example here 20*.7) would be the ones it grabs normally and then the last (20*.3) would be random ones it didn't grab. To sort of selectively grab out ones that may be stuck at a higher priority.

[Sesquipedalian]

Oh, I see. Then why not just add an extra query at the end that simply deletes queued emails that have expired?