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

Version 6 Ignores Jobs Enqueued in Version 5 #345

Closed
chadrschroeder opened this issue Nov 13, 2018 · 4 comments
Closed

Version 6 Ignores Jobs Enqueued in Version 5 #345

chadrschroeder opened this issue Nov 13, 2018 · 4 comments
Assignees
Labels

Comments

@chadrschroeder
Copy link

When upgrading from version 5 to version 6, any jobs that were enqueued on version 5 are removed from their Sidekiq queues and marked as processed in the Sidekiq log but they aren't actually executed. I think this is because the GRABBED key doesn't exist on the jobs that were enqueued on version 5 and SidekiqUniqueJobs silently exits before yielding to the worker.

If possible, it would be nice if version 6 could just pick up and process these jobs. Otherwise, it might be worth documenting that the required upgrade steps are currently:

  1. Stop all processes that can enqueue new jobs that have a unique requirement.
  2. Let all the jobs that have been enqueued on version 5 finish processing.
  3. Deploy version 6.
  4. Restart the processes that enqueue jobs.
@mhenrixon
Copy link
Owner

I actually have tests saying this should work but it is pretty hard to to test this in an automated way. Have a look at: https://github.com/mhenrixon/sidekiq-unique-jobs/blob/master/spec/integration/sidekiq_unique_jobs/legacy_lock_spec.rb I even kept some of the old files to make sure the transition is smooth (and that I can create the same locks as v5 did). Any ideas what I am missing?

@mhenrixon mhenrixon self-assigned this Nov 14, 2018
@mhenrixon mhenrixon added the bug label Nov 14, 2018
@mhenrixon
Copy link
Owner

Are you guys using redis namespace by any chance?

@chadrschroeder
Copy link
Author

No, we aren't using redis namespace.

In version 6, when I call perform_async the client middleware asks the SidekiqUniqueJobs::Locksmith to lock the job. This runs touch_grabbed_token which sets the GRABBED key. The GRABBED key must be present in order for locked? to be true and for the job to run:

# SidekiqUniqueJobs::Lock::UntilExecuted
def execute
  return unless locked?  # Will exit without executing the job here if locked? is false
  with_cleanup { yield }
end

But version 5 didn't set this GRABBED key when perform_async was called. So jobs enqueued in version 5 won't run in version 6.

This is one way to examine the situation using legacy_lock_spec.rb:

context 'with a legacy lock' do
  before do
    result = SidekiqUniqueJobs::Scripts.call(
      :acquire_lock,
      redis_pool,
      keys: [unique_digest],
      argv: [lock_value, lock_expiration],
    )

    expect(result).to eq(1)
    expect(unique_keys).to include(unique_digest)
  end

  context 'test if locked' do
    let(:lock_value) { jid_one }

    it 'is locked' do
      puts "Redis keys: #{SidekiqUniqueJobs.redis(&:keys)}"
      puts "locked?: #{locksmith_one.locked?}"
      locksmith_one.send(:touch_grabbed_token, jid_one)  # Only happens in version 6 during perform_async
      puts "Redis keys: #{SidekiqUniqueJobs.redis(&:keys)}"
      puts "locked?: #{locksmith_one.locked?}"
    end
  end
  ...

This will result in:

Redis keys: ["uniquejobs:test_mutex_key"]
locked?: false
Redis keys: ["uniquejobs:test_mutex_key", "uniquejobs:test_mutex_key:GRABBED"]
locked?: true

@mhenrixon
Copy link
Owner

Sorry it took so long @chadrschroeder, not sure if you still need it for an upgrade but it is now fixed and will be released with the next version.

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

No branches or pull requests

2 participants