[5.3] Catch errors when handling a failed job

[brianmclachlin]

More explanation in my comment on this issue

TLDR;
If there is any problems with the payload of a job then the failed job event will not fire and the job is lost.

[taylorotwell]

Thanks for tracking down this issue. I wonder if there is a more specific way to catch this error. It seems like could also be catching unrelated errors in this try / catch block and immediately marking things as failed for other reasons? Maybe that's what we want to happen? Do you have any thoughts there?

[brianmclachlin]

@taylorotwell I believe that we would want to catch all errors. We need the failed job event to get fired; otherwise, jobs will be lost. The smallest change I can think of is using a try catch to ensure that the failed job event gets fired. I can't think of any other solutions without doing larger changes to the Worker class.

After looking at this issue some more I realized that failJob also gets called from markJobAsFailedIfAlreadyExceedsMaxAttempts before the job is fired. This would also cause the job to skip the failed_jobs table.

Instead of putting a catch in handleJobException I thought it'd be better to wrap the logic in failJob with a try catch of it's own. This way the use case is handled from both spots where failJob is called. It will catch the exception and always run $this->raiseFailedJobEvent($connectionName, $job, $e); no matter what.

[taylorotwell]

And this still fixes the issue on your end?

[brianmclachlin]

Yup this still fixes the issue on our end, I ran through the test case again to be sure.

[jesseleite]

It seems like could also be catching unrelated errors in this try / catch block and immediately marking things as failed for other reasons? Maybe that's what we want to happen? Do you have any thoughts there?

@taylorotwell I believe that we would want to catch all errors. We need the failed job event to get fired; otherwise, jobs will be lost.

100% agree with @brianmclachlin. It doesn't matter where the exception comes from, I'm under the opinion that the failing job should be sent to failed jobs table with the recorded exception in all cases. A failed job should only be removed from the failed jobs table if: A) A developer runs artisan queue:retry and the job is successful, or B) A developer runs artisan queue:flush. The recorded payload and exception are valuable for debugging purposes. My 2 cents anyway.

[brianmclachlin]

@taylorotwell Not sure why travis is failing. The job log doesn't give me any output.

[brianmclachlin]

@taylorotwell Travis for some reason had output this morning. Figured out what broke the tests and fixed it.

[jesseleite]

Thank you @taylorotwell 👌