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

Ruby reaper incorrectly checks active jobs — removes every active lock as result #517

Closed
markiz opened this issue Jun 11, 2020 · 6 comments · Fixed by #518
Closed

Ruby reaper incorrectly checks active jobs — removes every active lock as result #517

markiz opened this issue Jun 11, 2020 · 6 comments · Fixed by #518
Assignees

Comments

@markiz
Copy link

markiz commented Jun 11, 2020

Describe the bug
https://github.com/mhenrixon/sidekiq-unique-jobs/blob/master/lib/sidekiq_unique_jobs/orphans/ruby_reaper.rb#L120-L132

This method checks sidekiq process list, and not active jobs.

Process hash looks like this in our setup:

{"hostname"=>"dss-queue-single2", "started_at"=>1591895424.547636, "pid"=>13767, "tag"=>"sg_dss_portal", "concurrency"=>5, "queues"=>["heavy"], "labels"=>[], "identity"=>"dss-queue-single2:13767:db7bbfe1a54e"}

In sidekiq dashboard it looks like this:
image
Note: one the db7bbfe1a54e is not the job id, but rather process id.

This results in reaper removing any locks for the jobs being actively executed, so you might see however many concurrent heavy jobs at the same time.

@markiz
Copy link
Author

markiz commented Jun 11, 2020

https://github.com/mperham/sidekiq/blob/master/lib/sidekiq/api.rb#L925 I think what should have been used is workers instead of info, and iterate over all jobs. Or maybe use the Sidekiq API itself.

mhenrixon added a commit that referenced this issue Jun 12, 2020
mhenrixon added a commit that referenced this issue Jun 12, 2020
* Use helper methods

* Prevent reaping of active jobs

Close #517

* Use an existing method
@mhenrixon mhenrixon self-assigned this Jun 12, 2020
@mhenrixon
Copy link
Owner

Released as v7.0.0.beta22

@markiz
Copy link
Author

markiz commented Jun 12, 2020

@mhenrixon thank you 🙏

@mhenrixon
Copy link
Owner

@markiz Thank you for the wonderfully detailed bug reports. Made it a breeze to fix

@godric
Copy link

godric commented Oct 29, 2020

Hi @mhenrixon, not sure if I should have opened a separate issue..

This bug is mentioned as "fixed" in the changelog for v6.0.23, but there isn't anything about it in the corresponding diff

Am I correctly assuming that the Changelog is wrong and this bug still present on the 6.x branch?
If so, is there any plans to backport the fix in 6.x ? 🙏

@mhenrixon
Copy link
Owner

Hi @godric,

Unfortunately, there will be no reaping of old jobs in v6. I am having some trouble with the changelog generator.

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

Successfully merging a pull request may close this issue.

3 participants