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 active worker detection by using correct keys #756

Merged
merged 1 commit into from
Feb 14, 2023
Merged

Fix active worker detection by using correct keys #756

merged 1 commit into from
Feb 14, 2023

Conversation

dodo121
Copy link
Contributor

@dodo121 dodo121 commented Feb 14, 2023

Hello,
We are using this gem for some of our products at marketer.tech.

The problem:
Recently we've spotted an issue with one our workers. The lock was not working and the same cron job was processed multiple times. After some investigation, we noticed that the reaper removes active locks which is incorrect behavior.

Root cause of the issue
It turns out that the active? method does not find any active workers because of the naming issue. Sidekiq sets the keys with "#{key}:work" instead of "#{key}:workers". The same happens to Lua reaper.

Solution:
Rename keys from :workers to :work since that's the correct name set in Redis:
Zrzut ekranu 2023-02-14 o 13 04 25

QA:
We tested those changes with our app and both reapers (Ruby & Lua) no longer remove active locks.

Rename keys from :workers to :work since that's correct name set in Redis
@mhenrixon
Copy link
Owner

Quick question: which Sidekiq and SidekiqUniqueJobs versions are you currently using? I want to make sure this gets backported if it concerns v7.

@dodo121
Copy link
Contributor Author

dodo121 commented Feb 14, 2023

Quick question: which Sidekiq and SidekiqUniqueJobs versions are you currently using? I want to make sure this gets backported if it concerns v7.

sidekiq-unique-jobs (8.0.0)
sidekiq (7.0.3)

@mhenrixon mhenrixon enabled auto-merge (squash) February 14, 2023 12:14
@mhenrixon mhenrixon disabled auto-merge February 14, 2023 12:15
@mhenrixon mhenrixon merged commit 75314c3 into mhenrixon:main Feb 14, 2023
@mhenrixon
Copy link
Owner

Releaased as 8.0.1 and should be available in a few minutes. I know rubygems are cached pretty hard so sometimes takes a minute.

@dsander
Copy link
Contributor

dsander commented Apr 12, 2023

We are running sidekiq-unique-jobs with Sidekiq 6.5.8, took me a bit to figure out why some jobs were running in a loop 😄. It would be awesome if we could get this backported to v7 of sidekiq-unique-jobs

dsander pushed a commit to projectivetech/sidekiq-unique-jobs that referenced this pull request Apr 12, 2023
Rename keys from :workers to :work since that's correct name set in Redis
@andrepiske andrepiske mentioned this pull request Jun 7, 2023
mhenrixon pushed a commit that referenced this pull request Jul 14, 2023
Rename keys from :workers to :work since that's correct name set in Redis
mhenrixon added a commit that referenced this pull request Jul 14, 2023
* Fix active worker detection by using correct keys (#756)

Rename keys from :workers to :work since that's correct name set in Redis

* chore(lint): fix linter issues

* fix(unlock): ensure callback and unlock (#771)

* chore(deps): update gems (solargraph is awesome)

* fix(unlock): ensure unlock and callback runs

* chore(lint): lint'em real good
# Conflicts:
#	.github/workflows/rspec.yml
#	myapp/.tool-versions

* fix: backport the fix for the return value of #deep_transform_keys (#750)

Backport fix the return value of #deep_transform_keys

* Hide lock info debug suggestion on lock page if it's already enabled. (#763)

only show lock info suggestion if value is not already on

* chore(v7): backport fixes from v8

* chore(ci): backport ci changes from v8

---------

Co-authored-by: Dominik Szromik <[email protected]>
Co-authored-by: Egor Romanov <[email protected]>
Co-authored-by: Jeremiah <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants