Failed() method get called on every chained jobs that were successful

nuxwin commented 8 months ago

Laravel Version

10.39

PHP Version

8.1

Database Driver & Version

Sync queue

Description

Good morning,

I've jobs, each defining the failed() method to handle job failure. I ran those jobs through Bus::chain() (facade) (using synchronous queue) as follows :

...
return Bus::chain([
    new CreateMailboxJob($user, $mailboxDomainName, $mailboxName, encrypt($mailboxPassword)),
    new CreateRemoteMailboxJob($mailboxDomainName, $mailboxName),
    new CreateMailboxAliasJob($mailboxDomainName, $mailboxName, $mailboxAliasName),
    new CreateRemoteMailboxAliasJob($mailboxDomainName, $mailboxName, $mailboxAliasName)
])->dispatch();
...

Expected behavior:

If the third job is failing, I expect the failed() method on that third job to be called. The failed() method on other jobs shouldn't be called.

Current behavior:

When the third job is failing, the failed() method is well called on that failed job, but also on previous jobs which were successful.

Is that the expected behavior ? If yes, could you provide a way to not "bubble" the failure ?

Logs :

[2024-01-01 08:21:09] testing.INFO: Executing CreateMailboxJob {"mailboxDomainName":"resmail.biz","mailboxName":"nf99999_1234567"} 
[2024-01-01 08:21:09] testing.INFO: Executing CreateRemoteMailboxJob {"mailboxDomainName":"resmail.biz","mailboxName":"nf99999_1234567"} 
[2024-01-01 08:21:09] testing.INFO: Executing CreateMailboxJobAlias {"mailboxDomainName":"resmail.biz","mailboxName":"nf99999_1234567","mailboxAliasName":"nf99999"} 
[2024-01-01 08:21:09] testing.DEBUG: Failed method from CreateMailboxAliasJob should be called {"exceptionMessage":"The mailbox is not synced."} 
[2024-01-01 08:21:09] testing.DEBUG: Failed method from CreateRemoteMailboxJob should not be called {"exceptionMessage":"The mailbox is not synced."} 
[2024-01-01 08:21:09] testing.DEBUG: Failed method from CreateMailboxJob should not be called {"exceptionMessage":"The mailbox is not synced."}

Thank you.

BTW: It is my first report here. I'm not sure to provide you will all relevant information.

Steps To Reproduce

Create three jobs, each implementing the failed() method. Executing the jobs in a chain, using sync queue, making the last job failing volontary.

nuxwin commented 8 months ago

More info: The problem occurs only when using the synchronous queue. When processing through database queue, I get the expected behavior.

driesvints commented 8 months ago

Heya, thanks for reporting.

We'll need more info and/or code to debug this further. Can you please create a repository with the command below, commit the code that reproduces the issue as one separate commit on the main/master branch and share the repository here? Please make sure that you have the latest version of the Laravel installer in order to run this command. Please also make sure you have both Git & the GitHub CLI tool properly set up.

laravel new bug-report --github="--public"

Please do not amend and create a separate commit with your custom changes. After you've posted the repository, we'll try to reproduce the issue.

Thanks!

rodrigopedra commented 8 months ago

If the third job is failing, I expect the failed() method on that third job to be called. The failed() method on other jobs shouldn't be called.

Why not?

According to the docs:

When a particular job fails, you may want to send an alert to your users or revert any actions that were partially completed by the job. To accomplish this, you may define a failed method on your job class.

reference: https://laravel.com/docs/10.x/queues#cleaning-up-after-failed-jobs

Also:

Job chaining allows you to specify a list of queued jobs that should be run in sequence after the primary job has executed successfully. If one job in the sequence fails, the rest of the jobs will not be run.

reference: https://laravel.com/docs/10.x/queues#job-chaining

So, one of the intended behaviors of the failed method being called is to allow to "revert any actions". And a job chain is a "a list of queued jobs that should be run in sequence".

Therefore, I would expect to have any reversion logic on each job, and that each job is notified of a failure if a chain/sequence fails.

If I have a chain of jobs where the first job performs a withdrawal from a savings account from bank A, and the second performs a deposit to an account on bank B through an API, if the second job fails, where the logic to revert the first job action should be placed?

reference: https://martinfowler.com/eaaDev/EventSourcing.html#ReversingEvents

Of course, this example is a bit extreme, but there are many cases such as a logistic chain where each job on a chain performs a single operation, such as inventory check > address check > delivery availability check > ...

If a job fails further in the sequence, I would expect the logic to revert the previous jobs actions to be defined on each job.

Is that the expected behavior ? If yes, could you provide a way to not "bubble" the failure ?

I can see the benefit of having a cancelling "event" that would contain the thrown exception, or maybe returning a boolean from the failed method would suffice (true to indicate the exception is already handled, or false to short-circuit its handling, there are both approaches on some parts of the framework).

This could be a nice PR in time for the next release.

driesvints commented 8 months ago

Thanks @rodrigopedra

@nuxwin seems this is indeed expected.

nuxwin commented 8 months ago

@driesvints So why the behavior is not identical when using a database queue ?

synchronous queue : For all jobs in a chain, the failed() method is called on the job that failed, and all previous jobs that were succesful.
asynchronous queue : For all jobs in a chain, the failed() method is called on the job that failed only.

I can understand that when a job is failing in a chain, the failed() method on the previous jobs that were successful need to be called, giving them a chance to cancel any change but in that case, the behavior should be clearly described in the documentation, and be the identical accross different queue implementations (synchronous queue as asynchonous queue).

So, even if I can understand that the failed() method need to be called on every jobs that were successful, It could be great to be able to know which job was responsable of the failure in other jobs. The approach suggested by @rodrigopedra sound great.

In my case, I've a chain of jobs, for instance:

CreateMailboxJob job which is responsible of the creation of a mailbox locally
CreateRemoteMailboxJob job which is responsible of the creation of a mailbox remotely
CreateMailboxAliasJob job which is responsible of the creation of a mailbox alias locally
CreateRemoteMailboxAliasJob job which is responsible of the creation of a mailbox alias remotely

If the CreateRemoteMailboxJob job is failing, I want just update both the is_synced and sync_error fields (database field) of the mailbox_aliases table through the failed() method of the CreateRemoteMailboxAliasJob job, nothing more.

I'm chaining those jobs because here, the creation of the mailbox alias depend on the existence of the mailbox, but, if the two first jobs for the creation of the mailbox succeeded, there is no need to revert them. The problem is that actuallly, due to the fact that every failed() method of the jobs that were successful is being called, the the fields of the mailbox are also updated.

Having a way to known which job is responsible of the failure would solve that problem because I could choice to act or not in each failed() method.

I hope you did understand my point of view.

BTW: I'm french. I'm sorry for my bad English.

laravel / framework