Skip to content
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.

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

Fix jobs being retried forever #15

Merged
merged 3 commits into from
Feb 13, 2025
Merged

Conversation

wfltaylor
Copy link

Currently, if a job is dispatched with a non-zero maxRetryCount, the job is retried forever. This is because the JobData was initialised with the default value (0) for attempts.

I’ve added a test for this, however it is currently flakey (especially when run with SQLite) since delayUntil is set for retried jobs, even if the retry delay is zero. This can be fixed by changing the behaviour in the queues package to set a nil delayUntil when the retry delay is zero, but I’m not sure if this is the best solution.

@wfltaylor wfltaylor requested a review from gwynne as a code owner February 13, 2025 02:57
@gwynne
Copy link
Member

gwynne commented Feb 13, 2025

Nice catch, thanks for this! As regards the flaky test, if I'm reading the code correctly (such that it's failing because the retry attempt comes so quickly that the check for the job delay doesn't pass?) you could make it reliable by .run()-ing the worker multiple times (in this case, 6). You're right about the real solution being to fix the behavior in queues itself, though, and your suggested fix looks correct to me - if you're up for opening a PR to do that, I'd gladly accept it along with this one 🙂.

@wfltaylor
Copy link
Author

Nice catch, thanks for this! As regards the flaky test, if I'm reading the code correctly (such that it's failing because the retry attempt comes so quickly that the check for the job delay doesn't pass?) you could make it reliable by .run()-ing the worker multiple times (in this case, 6). You're right about the real solution being to fix the behavior in queues itself, though, and your suggested fix looks correct to me - if you're up for opening a PR to do that, I'd gladly accept it along with this one 🙂.

The reason the SQLite test was so “flakey” was because jobs with a delay_until value are never executed 🙂. I’ve created another PR which should address this issue: #16

I also created a PR to change the behaviour for setting delayUntil in Queues as well, since I do think this makes sense: vapor/queues#140

Copy link

codecov bot commented Feb 13, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.18%. Comparing base (f302c45) to head (b42a2c6).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #15   +/-   ##
=======================================
  Coverage   98.18%   98.18%           
=======================================
  Files           7        7           
  Lines         275      275           
=======================================
  Hits          270      270           
  Misses          5        5           
Files with missing lines Coverage Δ
Sources/QueuesFluentDriver/FluentQueue.swift 96.93% <100.00%> (ø)

@gwynne
Copy link
Member

gwynne commented Feb 13, 2025

Holding off on merging temporarily until we get the release automation working (won't take long).

@gwynne gwynne merged commit 61b2fa9 into vapor-community:main Feb 13, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch Internal changes only
Development

Successfully merging this pull request may close these issues.

2 participants