The order of config.super_fetch! in the initializer can lead to the wrong choice of fetching algorithm

[seckenrode]

Ruby version: 2.5.6
Rails version: 5.2.4.3
Sidekiq / Pro / Enterprise version(s): sidekiq 6.1.0 / sidekiq-pro 5.1.1

Hello! In our sidekiq setup, we use a fairly complex initializer which sets up which queues we use and the weights for each of them based on environment variables and other application parameters. At a high level, our configure_server block looks like this:

Sidekiq.configure_server do |config|
  # Enable super fetch to mitigate against jobs that get lost due to bad
  # behavior on shutdown or worker death.
  # https://github.com/mperham/sidekiq/wiki/Pro-Reliability-Server#super_fetch
  config.super_fetch! if Rosie.enable_super_fetch?(worker_type)

  # ...
  
  config.options[:queues] = sidekiq_queues.flat_map(&:weighted_sidekiq_names)
  config.options[:strict] = false
end

In the fetcher refactor that was included in the sidekiq-pro 5.1.0 release (#4602), the algorithm for SuperFetch changed from being calculated at fetch time to being set in the initializer based on the options that were configured. The result of this refactor means that when we call config.super_fetch! at the start of our configuration block, the SuperFetch fetcher gets initialized with the default options, not the options that we configure later in that block. Importantly, the algorithm for fetching is set at that time, which means the strict option is ignored and our queue list isn't considered as part of that decision.

We managed to not notice this when we first upgraded because the updated list of queues is considered after the first server fetch. We didn't notice the algorithm change from weighted to strict until we had a significant volume of jobs happen and noticed job starvation.

The resolution to our issue was to move config.super_fetch! to the end of our configuration block, which correctly uses the updated options to initialize the fetcher. I wanted to raise this issue because the docs don't indicate that the order of where the config.super_fetch! call is located is not mentioned and it might be an unintended side effect that could subtly break other's applications as well!

Thanks!

[mperham]

It sounds like you got bit by an implementation detail. Note that I don't document setting up the queues that way, queues are supposed to be set up as CLI args or within the config YAML, both case are handled before the initializer runs so super_fetch will work correctly if you use either one.

[seckenrode]

Good to know, thanks! We're looking towards moving this implementation to a more standard approach, so it seems like that's the right course of action.

[chadrschroeder]

I just hit this when upgrading sidekiq-pro. I understand that this isn't the preferred way to set up queues. It might be worth documenting this as a breaking change in 5.1.0. Maybe the text could be something like:

BREAKING CHANGE If you call config.super_fetch!, make sure this is called after your queues have been configured. Otherwise, only jobs in the default queue will be processed.