Skip to content

Prevent multiple execution of aborted jobs #528

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
+9 −1
Open

Prevent multiple execution of aborted jobs #528

Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
95c21de
Fix aborted retryable jobs
luke- May 15, 2025
83fac4c
Added comments
luke- May 19, 2025
2c5d0ff
Reduce attempt count before checking canRetry
luke- May 19, 2025
18e342f
Handle use queue default attempts
luke- May 21, 2025
14bfe70
Added to CHANGELOG
luke- May 21, 2025
1cbc278
Fix condition for non retryable jobs
luke- May 21, 2025
99d9f89
Combined Conditions
luke- May 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions src/Queue.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,17 @@ public function getWorkerPid()
protected function handleMessage($id, $message, $ttr, $attempt)
{
list($job, $error) = $this->unserializeMessage($message);

// Handle aborted jobs without thrown error
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Handle aborted jobs without thrown error
// Handle aborted jobs without throwing an error.

if ($attempt > 1) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that all this can be combined into one condition

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@s1lver Shall I summarize that? I'd be happy to. I personally prefer the breakdown for quick readability.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about this solution? Will return true if any of the conditions evaluate to true

return
    ($job instanceof RetryableJobInterface && !$job->canRetry($attempt - 1, $error)) 
    || (!($job instanceof RetryableJobInterface) && $attempt > $this->attempts)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@s1lver Thanks for the example.

I've summarized the condition here:
99d9f89

I'm not sure about the return in your example, since we only return true if the condition matches.

if ($job instanceof RetryableJobInterface && !$job->canRetry($attempt - 1, $error)) {
return true;
} else {
// Non RetryableJobs can have a maximum of one attempt
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assumption is incorrect. RetryableJobInterface means that job can decide whether it should be retried. In other case it is controlled by attempts property at queue level - job can still have multiple retries even if it doesn't implement RetryableJobInterface.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rob006 Thanks for pointing that out. I've adjusted the condition.
However, I still need to check the tests.

return true;
}
}

$event = new ExecEvent([
'id' => $id,
'job' => $job,
Expand Down
Loading