Setting default_job_options with a symbol key has unpredictable results with duplicate keys

[kmcphillips]

Ruby version: any
Rails version: any
Sidekiq / Pro / Enterprise version(s): 7.2.2 (but appears to be the same on main)

Hi Mike! Long time user first time commenter.

I've hit a bug that I'm happy to PR a fix, but I'm not really sure what kind of fix would be appropriate. None of the solutions I can come up with feel right, so I want feedback before I try to implement it.

Problem

In a Rails initializer in my app I set my job retry:

Sidekiq.default_job_options[:retry] = 2

This worked fine and has for a long time. But recently in a job I did not want to retry I set the retry to zero:

class ExampleJob < ApplicationJob
  sidekiq_options retry: 0

This had no effect on retries. After some debugging the reason turned out to be:

ExampleJob.sidekiq_options
{"retry"=>0, "queue"=>"default", :retry=>2}

The default_job_options is expected to be a hash with string keys and is transformed on assignment:

sidekiq/lib/sidekiq.rb

Lines 80 to 82 in 94b0750

	def self.default_job_options=(hash)
	@default_job_options = default_job_options.merge(hash.transform_keys(&:to_s))
	end

When being read the keys are stringified. But since this is just base hash assignment, nothing stops from both string and symbol keys from being set. The behaviour becomes unpredictable if there are duplicate symbol and string keys when they are later read for use.

Using this assignment method or assigning the key by string solves the issue. But since default_job_options is both memoized and mutable, assigning the hash keys in place does work and is possible, even if it only sorta works:

sidekiq/lib/sidekiq.rb

Lines 84 to 86 in 94b0750

	def self.default_job_options
	@default_job_options ||= {"retry" => true, "queue" => "default"}
	end

I try to go for "make the right thing easy to do" and in this case it was easy to do the wrong thing. I'm hoping to improve that.

Solutions

I have a few ideas but I don't like any of them...

The problem is with the reader

Since def self.default_job_options returns the mutable memoized hash it can be mutated. This could be changed to return a .freeze copy, and guide people to use the correct API which is to call self.default_job_options=(hash) which does the string/symbol management. This would be a breaking change though as users doing what I would doing would now raise FrozenError: can't modify frozen Hash on initialization. I know breaking changes are undesirable.

The problem is with the hash

What I would personally reach for is implementing the options hash as either some kind of configuration object or a hash with indifferent access. There is no use case for the config hash to have both string and symbol keys, but the hash object being used here allows it. You keep Sidekiq lean though and it doesn't pull in ActiveSupport. I could go down the path of maybe building a scaled down HashWithIndifferentAccess or vendoring part of the Rails version, but all that feels messy and prone to bugs and not at all clean.

The problem is data validation

Sidekiq could at some level validate the contents of the default options hash. As simple as.

raise if default_job_options.count != default_job_options.transform_keys(&:to_s).count

But I'm not really sure at what level makes sense. This can be changed at any point in initialization or even runtime, so there's no obvious hook where to validate config. It could validate when read, but that has its own set of issues including performance. It would also be a breaking change that would raise for users assigning the config as a symbol, which works even if it has a latent bug.

The problem is with the user

I'm using the API wrong. Don't use the API wrong. Close this issue. I'm good with that too!

The problem is somewhere else

I'm sure there's a better option I didn't think of. WDYT?

[mperham]

Yeah, that's a tricky one. I'm not sure how to improve the situation.

I wish Hash had a transform_proc option which can be called on every k/v pair to normalize items as they are added to the hash. Or Hash had an option to make String == Symbol for keys, like HWIA.

Should we add an API for mutating the default values and deprecate direct Hash access?

[kmcphillips]

That could work. Define and/or use a non-hash API for setting the defaults, and rewrite the existing hash access to use that API, deprecate it, and remove it on a major version.

I agree that hash with indifferent access feels like a pretty core behavior that is often the desired default, especially when dealing with args or opts. TBH I end up selectively including ActiveSupport and that extension in projects for just this reason.

Could be something closer to:

  def self.default_job_options
    @default_job_options ||= Sidekiq::JobOptions.new
  end

And implement that class with proper error handling and named methods for the options. It wouldn't be hard to maintain backwards compatibility with the existing hash API I don't think.

Is default_job_options a bounded set of valid options? Or can the keys be arbitrary depending on the project? I don't know this without digging.

[mperham]

default_job_options allows arbitrary String -> Any k/v pairs. You could have a k/v pair which is relevant to a Sidekiq extension, for instance.

[kmcphillips]

Makes sense.