FailOnException middleware doesn't work in conjunction with ThrottlesExceptions middleware

[michaeldzjap]

Laravel Version

12.19.3

PHP Version

8.4.8

Database Driver & Version

No response

Description

Not exactly sure if this should be categorised as a bug, but it is definitely unexpected / unwanted behaviour: the new FailOnException job middleware doesn't seem to work as expected in conjunction with the ThrottlesExceptions job middleware. When both are specified as middleware the job seems to be attempted multiple times regardless. I have only tested this out with the "redis" queue driver.

Steps To Reproduce

The job below may be used to reproduce the problem. When both middlewares are active the failing job will be re-attempted until throttling kicks in and finally fails on job timeout. When you remove / comment out the ThrottlesExceptions middleware things work like expected.

<?php

namespace App\Jobs;

use Carbon\CarbonImmutable;
use DateTimeImmutable;
use Illuminate\Contracts\Queue\ShouldQueue;
use Illuminate\Foundation\Queue\Queueable;
use Illuminate\Queue\Middleware\FailOnException;
use Illuminate\Queue\Middleware\ThrottlesExceptions;
use Throwable;

class FailingJob implements ShouldQueue
{
    use Queueable;

    /**
     * Create a new job instance.
     */
    public function __construct()
    {
        //
    }

    /**
     * Execute the job.
     */
    public function handle(): void
    {
        throw new \Exception('Hey now!');
    }

    /**
     * Get the middleware the job should pass through.
     *
     * @return array<int, object>
     */
    public function middleware(): array
    {
        return [
            new FailOnException([\Exception::class]),
            new ThrottlesExceptions(10, 60)->report(
                fn (Throwable $throwable): bool => $throwable instanceof \Exception,
            ),
        ];
    }

    /**
     * Determine the time at which the job should timeout.
     */
    public function retryUntil(): DateTimeImmutable
    {
        return CarbonImmutable::now()->addMinutes(2);
    }
}

[michaeldzjap]

Looking through the documentation for ThrottlesExceptions I see there is a public method deleteWhen that seems to be doing something similar to FailOnException though. So perhaps that is the solution to this problem: just use ThrottlesExceptions#deleteWhen instead of FailOnException when you want a job using the ThrottlesExceptions middleware to fail straight away for specific exceptions. It doesn't have exactly the same effect though, since deleting a job is not the same as failing a job (deletion is only a part of failing a job), especially in the context of a job chain.

[michaeldzjap]

Would a PR for adding a failWhen method to the ThrottlesExceptions middleware be considered? If so, I'd be happy to work on this.

[cosmastech]

I wonder if the solution isn't to add a check here to see if the job is already failed.

framework/src/Illuminate/Queue/Middleware/ThrottlesExceptions.php

Line 127 in 1831e26

return $job->release($this->retryAfterMinutes * 60);

+            if ($job->job?->isFailed()) {
+                return;
+            }
             return $job->release($this->retryAfterMinutes * 60);

I don't think I've used this middleware before, so that could be a breaking change from the userland perspective. Not entirely sure.

[michaeldzjap]

I wonder if the solution isn't to add a check here to see if the job is already failed.

framework/src/Illuminate/Queue/Middleware/ThrottlesExceptions.php

Line 127 in 1831e26

return $job->release($this->retryAfterMinutes * 60);

•        if ($job->job?->isFailed()) {
  

•            return;
  

•        }
         return $job->release($this->retryAfterMinutes * 60);
  

I don't think I've used this middleware before, so that could be a breaking change from the userland perspective. Not entirely sure.

I guess that's an option too, although, like you said, that might be a breaking change from a use perspective, as that would change how the ThrottlesExceptions middleware currently behaves. Of course you could argue if that makes sense or not. I would say the current behaviour of ignoring if the job failed doesn't make much sense, but that's me.

Personally, I would prefer a new failWhen method for ThrottlesExceptions as there already is a deleteWhen method and you can avoid creating any potential breaking changes as long as you do the shouldFail check after the shouldDelete check. You just add a new functionality to ThrottlesExceptions that would be quite desirable. Well, I think so anyway.

Of course even with a new failWhen method for ThrottlesExceptions the original issue I reported still persists. So from that point of view your solution would be favourable (if that indeed works as expected).