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

Merged
merged 9 commits into from
May 31, 2025
Merged

Prevent multiple execution of aborted jobs #528

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 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
77feac5
Update CHANGELOG.md
luke- May 30, 2025
d76bdc6
Update src/Queue.php
luke- May 30, 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
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ Yii2 Queue Extension Change Log
-----------------------
- Enh #516: Ensure Redis driver messages are consumed at least once (soul11201)
- Bug #522: Fix SQS driver type error with custom value passed to `queue/listen` (flaviovs)

- Bug #528: Prevent multiple execution of aborted jobs

2.3.7 April 29, 2024
--------------------
Expand Down
10 changes: 10 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,16 @@ public function getWorkerPid()
protected function handleMessage($id, $message, $ttr, $attempt)
{
list($job, $error) = $this->unserializeMessage($message);

// Handle aborted jobs without thrown 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;
} elseif (!($job instanceof RetryableJobInterface) && $attempt > $this->attempts) {
return true;
}
}

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