while_executing with on_conflict: :reschedule reschedules job when unlock fails

[dsander]

Hi there! 👋

I am not 100% sure if this really is a bug or if I am not understanding the use case.

Describe the bug

sidekiq-unique-jobs/lib/sidekiq_unique_jobs/lock/while_executing.rb

Lines 41 to 52 in 98a3e8e

	def execute(&block)
	with_logging_context do
	executed = locksmith.execute do
	yield
	callback_safely if locksmith.unlock
	ensure
	locksmith.unlock
	end
	unless executed
	reflect(:execution_failed, item)
	call_strategy(origin: :server, &block)

Because executed captures the return value of the locksmith.execute block it will be falsy when the unlock operation failed (and when callback_safely returns something fasly?).

We noticed the behaviour on v7 with a sidekiq version that changed the redis key that hold the busy workers (#756), in our case this lead to a job being run over and over because unlock failed (the reaper deleted the lock of the running job).

Expected behavior

The server server strategy should only be called when the job lock could not be acquired, not when the unlock failed (successfully 😉 )

Maybe something like this?

      def execute(&block)
        with_logging_context do
          executed = false
          locksmith.execute do
            yield
            executed = true
            callback_safely if locksmith.unlock
          ensure
            locksmith.unlock
          end

          unless executed
            reflect(:execution_failed, item)
            call_strategy(origin: :server, &block)
          end
        end
      end

Worker class

class MyWorker
  include Sidekiq::Worker

  sidekiq_options queue: :assets, retry: false, lock: :while_executing, on_conflict: :reschedule
  
  ....
end

[mhenrixon]

Is that only for blocks? For a rescue ensure scenario in a method, the ensure part would only return something with an explicit return. Are blocks different?

[mhenrixon]

Try this in the console:

****

[mhenrixon]

If the ensure part would indeed return something it would say "pokus" not "hokus".

[dsander]

In our case I believe this happened:

irb(main):015:0* block = lambda do
irb(main):016:0*   true
irb(main):017:0*   "i don't know the return value" if false
irb(main):018:0* ensure
irb(main):019:0*   false
irb(main):020:-> end
irb(main):021:-> pp block.call
nil
=> nil

If unlock succeeds everything works as you mentioned:

irb(main):022:0* block = lambda do
irb(main):023:0*   true
irb(main):024:0*   "i don't know the return value" if true
irb(main):025:0* ensure
irb(main):026:0*   false
irb(main):027:-> end
"i don't know the return value"
=> "i don't know the return value"

[dsander]

So maybe a different approach to solve this would be

      def execute(&block)
        with_logging_context do
          executed = locksmith.execute do
            yield
          ensure
            callback_safely if locksmith.unlock
          end

          unless executed
            reflect(:execution_failed, item)
            call_strategy(origin: :server, &block)
          end
        end
      end

? To be honest I wasn't aware that the result of the ensure block isn't the return value of the block 😉

[mhenrixon]

Ah now I see your problem doh 🙄

[dsander]

Thank you!