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

perform_limit and perform_throttle don't work both #1469

Closed
doits opened this issue Aug 16, 2024 · 5 comments · Fixed by #1470
Closed

perform_limit and perform_throttle don't work both #1469

doits opened this issue Aug 16, 2024 · 5 comments · Fixed by #1470

Comments

@doits
Copy link
Contributor

doits commented Aug 16, 2024

Can it be that there is a bug when perform_limit and perform_throttle are both specified so that perform_throttle is ignored? Isn't this "next" at line 153 skipping throttling if the limit isn't reached, which means you cannot have both?

if limit
allowed_active_job_ids = GoodJob::Job.unfinished.where(concurrency_key: key)
.advisory_locked
.order(Arel.sql("COALESCE(performed_at, scheduled_at, created_at) ASC"))
.limit(limit).pluck(:active_job_id)
# The current job has already been locked and will appear in the previous query
exceeded = :limit unless allowed_active_job_ids.include?(job.job_id)
next
end
if throttle

Shouldn't it only skip throttling if it is exceeded already?

   unless allowed_active_job_ids.include?(job.job_id)
     exceeded = :limit  
     next 
  end

Originally posted by @doits in #1353 (comment)

@doits
Copy link
Contributor Author

doits commented Aug 16, 2024

I think this bug was introduced by refactoring in #1439

@bensheldon
Copy link
Owner

Thanks for opening this issue. I'm staring at it for a couple minutes now and I think you're correct that the current logic is wrong. We want to next/jump only if the job should be rescheduled, and continue testing the following conditions otherwise.

Is this a PR you'd be comfortable making?

@doits
Copy link
Contributor Author

doits commented Aug 16, 2024

sure, I'll add a spec do a PR

doits added a commit to doits/good_job that referenced this issue Aug 16, 2024
@doits
Copy link
Contributor Author

doits commented Aug 16, 2024

Please see #1470 for the PR

@bensheldon
Copy link
Owner

Thank you! This fix has been released: https://github.com/bensheldon/good_job/releases/tag/v4.2.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging a pull request may close this issue.

2 participants