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

After sidekiq shutdown (with running job) lock digest is not pushned back #812

Open
kirsha2 opened this issue Oct 9, 2023 · 8 comments
Open

Comments

@kirsha2
Copy link

kirsha2 commented Oct 9, 2023

Describe the bug
Rails: 7.0.4.3
Ruby: 3.2.2
Sidekiq: 6.2.1

When trying upgrading sidekiq and unique jobs our tests began to fail. Same behavior in manual testing.

  1. Enqueue until_executed job
  2. start processing
  3. shutdown worker (job is still not finished)
  4. job is pushed back into queue but not the digest

The bug occurs after 7.1.29 version. Same behavior in 8.
Used lock: until_executed

Expected behavior
After sidekiq shutdown job and digest are pushed back.

Current behavior
After sidekiq shutdown only job is pushed back.

I've looked into diff between version 29 and 30 but I cant see anything suspicious there. Is this expected behavior ?

Thanks :) for all your hard work - love the gem!

@kirsha2
Copy link
Author

kirsha2 commented Oct 11, 2023

I've tried to look into the changes more and found out it was due to the ensure in until_executed.rb

https://my.diffend.io/gems/sidekiq-unique-jobs/7.1.29/7.1.30#d2h-630681

So I don't know if this change/ consequence was intentional or not

@mhenrixon
Copy link
Owner

mhenrixon commented Nov 11, 2023

I've tried to look into the changes more and found out it was due to the ensure in until_executed.rb

https://my.diffend.io/gems/sidekiq-unique-jobs/7.1.29/7.1.30#d2h-630681

So I don't know if this change/ consequence was intentional or not

It sounds like a bug; I'll see about fixing these issues in the next few days. I should use an else rather than an ensure.

@joshuacronemeyer
Copy link

what behavior does this cause? Currently i'm facing an issue where my until_executed jobs can't get enqueued anymore and this sounds like it could be related to my problem. if so Is there something i can run from the console that will fix this for me when this happens?

@kirsha2
Copy link
Author

kirsha2 commented Jan 29, 2024

@joshuacronemeyer sounds like your locks/jobs are not correctly unlocked.
Behavior that I reproduced only affects re-enqued jobs, so if the job is not finished but pushed back into to queue.

This is issue is about missing lock and yours is probably still there "stuck".
I'd recommend to check your Unique Digests.

@pboling
Copy link
Contributor

pboling commented Feb 13, 2024

@kirsha2 The ensure-based bug was fixed in v8.0.5 and v7.1.31 I think. thread: #800 (comment)

@kirsha2
Copy link
Author

kirsha2 commented Mar 8, 2024

@pboling thank you for the info

yes it works now

@kirsha2 kirsha2 closed this as completed Mar 8, 2024
@kirsha2
Copy link
Author

kirsha2 commented Mar 12, 2024

Unfortunally I closed it prematurely, I've forgotten to remove my monkey patch in the code (too many changes in MR :D )

The main issue for me is still the ensure part in

My expected behavior, when job is in retry set the lock is still kept.

When this job after retries goes to dead, death_handlers takes care of unlocking
https://github.com/mhenrixon/sidekiq-unique-jobs/blob/main/lib/sidekiq_unique_jobs/server.rb#L13

I patched it like this

# lib/sidekiq_unique_jobs/lock/until_executed.rb
def execute
  executed = locksmith.execute do
    yield
    unlock_and_callback # NOTE: override ensure due to lock not pushing back after sidekiq shutdown
  end

  reflect(:execution_failed, item) unless executed

  nil
end

@kirsha2 kirsha2 reopened this Mar 12, 2024
@pboling
Copy link
Contributor

pboling commented Mar 12, 2024

@kirsha2 thanks for the update. I'll need to pull in your monkey patch also, as my production system can't afford to have this issue either, but I hadn't had time to evaluate it closely yet.

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

No branches or pull requests

4 participants