Batch job with a chained job that fails does not trigger a finally call with allowFailures-option

[ehamrin]

• Laravel Version: 9.29.0

Description:

The following should only be considered when ->allowFailures() is applied, else the batch is cancelled correctly.

If there's a batch containing one or more chains, and one chain fails, the batch will not receive a call to ->finally() callbacks.
If more than one chain fail, ->catch() is only called once.

Steps To Reproduce:

\Bus::batch(
[
    function() {
        dump('Job 1');
    },
    [
        function() {
            dump('Job 2');
            throw new Exception('Failure');
        },
        function() {
            dump('Job 3');
        },
    ],
    [
        function() {
            dump('Job 4');
            throw new Exception('Failure');
        },
        function() {
            dump('Job 5');
        },
    ],
    function() {
        dump('Job 6');
    },
]
)
->allowFailures()
->catch(function () {
    dump("Job failed"); //Called only once
})
->then(function () {
    dump("Jobs then"); //Never called
})
->finally(function () {
    dump("Jobs finally"); //Never called
})
->dispatch();

Will produce a batch entry with:

Total jobs: 6
Failed jobs: 2
Pending jobs: 4

Expected output:

A call to finally callbacks, where the batch can be inspected for errors and taken care of accordingly.

The catch callback should be invoked once per error if allowFailures is set, not just once.

The failed jobs should increment by the number of chained jobs left in the chain, including itself

Total jobs: 6
Failed jobs: 4
Pending jobs: 4

[driesvints]

Please see Taylor's answer here: #39243

[ehamrin]

@driesvints I'm well aware of how it's handled internally according to Taylor's response in #39243. What I don't understand is WHY that is the correct way of handling chains in batches, and not a bug.

There is no way to catch the failed jobs and deal with them as the catch-block is only called once. The pruning of batches will also never deal with the batch in question as it not completed nor cancelled.

In similarity to the pending jobs being incremented by the chain, the failed jobs should also increment by the remaining chain. A different approach would be a new column "unhandled_jobs". It would increment by unhandled chain jobs and combined with a new callback called "always" that is dispatched when "pending jobs" - "failed jobs" - " unhandled jobs" amounts to 0.

IMO that seems unneccessary as it is what the finally callback should do.

A silly comparison perhaps, but an important one:
At language level, it you have a try/catch/finally-block and one line throws an exception - the finally block will always run, not depending on how many lines after the exception didn't.

[driesvints]

I'm not entirely sure myself. You can always try a PR to change the behavior if you want.

[jonnylink]

I'm confused why this would just be closed. It's clearly not the proper behavior for a finally block. The docs describe then, catch, and finally as as they are understood in general, which is to say that finally is always called. I understand the complication, but that's not relevant to a finally block only being called when there are not any exceptions.

What's more @taylorotwell explained why the code was function as it does, but not that this is intentional or even correct.

At the very least the docs should call out this very unusual behavior.

[driesvints]

Welcoming PR's to the docs.