Queue worker ignores job's maxTries setting if using retryUntil()

[trevorgehman]

• Laravel Version: 8.14.0
• PHP Version: 7.4.11
• Database Driver & Version: Mysql 8.0.21 / Redis 5.0.7

Description:

There is a discrepancy between how the worker treats $job->retryUntil() and $job->maxTries() depending on whether an exception is thrown. This only affects jobs where retryUntil() is used.

Essentially, if a job throws an exception, if either $job->retryUntil() is exceeded or $job->maxTries() is exceeded, the job fails. This seems to be the expected behavior.

However, if the job doesn't throw an exception, but is just released (such as when using a rate limit middleware), then only $job->retryUntil() is considered and $job->maxTries() doesn't matter.

Steps To Reproduce:

Job that throws an exception:

1. Create a job with $tries=5 and retryUntil() { return now()->addHour(); }.
2. Throw an exception in the job.
3. The job will be attempted 5 times, then fail.

Job that is just released:

1. Create a job with $tries=5 and retryUntil() { return now()->addHour(); }.
2. Use a middleware that just releases the job.
3. The job will be attempted repeatedly for 1 hour.

Note that in either case (whether throwing an exception or releasing the job), the number of attempts is incremented.

Details:

When picking up a job, the worker first checks the number of attempts:

framework/src/Illuminate/Queue/Worker.php

Lines 395 to 397 in 72ea7ba

	$this->markJobAsFailedIfAlreadyExceedsMaxAttempts(
	$connectionName, $job, (int) $options->maxTries
	);

This method prioritizes retryUntil(). That is, if you use $job->retryUntil(), it doesn't matter what the $tries property is on the job or the --tries parameter is on the worker. The $job->retryUntil() method essentially overrides them:

framework/src/Illuminate/Queue/Worker.php

Lines 474 to 480 in 72ea7ba

	if ($retryUntil && Carbon::now()->getTimestamp() <= $retryUntil) {
	return;
	}
	if (! $retryUntil && ($maxTries === 0 || $job->attempts() <= $maxTries)) {
	return;
	}

If the job throws an exception, the worker again checks the number of attempts:

framework/src/Illuminate/Queue/Worker.php

Lines 432 to 434 in 72ea7ba

	$this->markJobAsFailedIfWillExceedMaxAttempts(
	$connectionName, $job, (int) $options->maxTries, $e
	);

But this time, retryUntil() is not prioritized. Meaning, if either the time set in retryUntil() is passed or the max attempts (as specified on the job or worker) is exceeded, the job will be marked as failed:

framework/src/Illuminate/Queue/Worker.php

Lines 500 to 506 in 72ea7ba

	if ($job->retryUntil() && $job->retryUntil() <= Carbon::now()->getTimestamp()) {
	$this->failJob($job, $e);
	}
	if ($maxTries > 0 && $job->attempts() >= $maxTries) {
	$this->failJob($job, $e);
	}

[taylorotwell]

IMO retryUntil and maxTries are sort of mutually exclusive. Either you want the job to retried until a given time is reached or you want it to be retried a specific number of times. When using retryUntil I would use maxExceptions if you want to determine how many uncaught exceptions are allowed:

https://laravel.com/docs/8.x/queues#max-exceptions

[trevorgehman]

IMO retryUntil and maxTries are sort of mutually exclusive. Either you want the job to retried until a given time is reached or you want it to be retried a specific number of times. When using retryUntil I would use maxExceptions if you want to determine how many uncaught exceptions are allowed:

https://laravel.com/docs/8.x/queues#max-exceptions

That's how I thought it worked originally and I think that makes more sense. That is, I thought retryUntil() would override --tries on the worker. The documentation seems to indicate that all you need to do is use retryUntil(), but unless I'm missing something, you also need to set --tries=0 on the worker or $tries = 0 on the job, because by default it is set to 1:

framework/src/Illuminate/Queue/Console/WorkCommand.php

Line 37 in e9483c4

{--tries=1 : Number of times to attempt a job before logging it failed}';

And when an exception is thrown, it will check the $maxTries even if retryUntil() is set:

framework/src/Illuminate/Queue/Worker.php

Lines 504 to 506 in 72ea7ba

	if ($maxTries > 0 && $job->attempts() >= $maxTries) {
	$this->failJob($job, $e);
	}

[trevorgehman]

Either way, it seems like these two methods shouldn't contain different logic:

framework/src/Illuminate/Queue/Worker.php

Lines 468 to 485 in e9483c4

	protected function markJobAsFailedIfAlreadyExceedsMaxAttempts($connectionName, $job, $maxTries)
	{
	$maxTries = ! is_null($job->maxTries()) ? $job->maxTries() : $maxTries;
	$retryUntil = $job->retryUntil();
	if ($retryUntil && Carbon::now()->getTimestamp() <= $retryUntil) {
	return;
	}
	if (! $retryUntil && ($maxTries === 0 || $job->attempts() <= $maxTries)) {
	return;
	}
	$this->failJob($job, $e = $this->maxAttemptsExceededException($job));
	throw $e;
	}

framework/src/Illuminate/Queue/Worker.php

Lines 496 to 507 in e9483c4

	protected function markJobAsFailedIfWillExceedMaxAttempts($connectionName, $job, $maxTries, Throwable $e)
	{
	$maxTries = ! is_null($job->maxTries()) ? $job->maxTries() : $maxTries;
	if ($job->retryUntil() && $job->retryUntil() <= Carbon::now()->getTimestamp()) {
	$this->failJob($job, $e);
	}
	if ($maxTries > 0 && $job->attempts() >= $maxTries) {
	$this->failJob($job, $e);
	}
	}

[themsaid]

We need to decide which logic we want to keep and change the other one.

Should retryUntil and maxAttempts work together or maxAttempts should be ignored if retryUntil was set?

[trevorgehman]

I think what you did in the PR is the best approach: retryUntil() essentially overrides maxAttempts.

[kamyargerami]

I have the same issue.
In my situation, I need to run my queue 5 times before 5 minutes.
and if takes much longer than 5 minutes get fail.
also if it process too fast and I tried 5 times no more new jobs are needed.

I think maybe you can affect tries to queue run.
thanks.

[miken32]

IMO retryUntil and maxTries are sort of mutually exclusive. Either you want the job to retried until a given time is reached or you want it to be retried a specific number of times.

I found this issue during a search, thinking something was broken. The quoted logic makes sense but it should be more explicitly explained in the documentation that retryUntil() negates $tries and tries() completely.