Sidekiq::Periodic:Loop Options serialization misalignment

[nohat]

Summary

The cronitor-sidekiq gem relies on the options method of Sidekiq::Periodic::Loop objects to return a hash. However, due to inconsistencies in Sidekiq's own handling of @options serialization and deserialization, the method returns a JSON string instead. This causes a NoMethodError in cronitor-sidekiq. See this related issue sidekiq/sidekiq#6092 I filed with Sidekiq.

---

Description

The core issue arises from the misalignment between how Sidekiq::Periodic::Loop handles @options serialization and how cronitor-sidekiq expects to consume it.

In Sidekiq::Periodic::Loop, @options is serialized to JSON when stored in Redis:

args.append("options", Sidekiq.dump_json(lop.options))

However, it is not deserialized back to a hash when read:

@options = opts.delete("options")

In cronitor-sidekiq, the assumption is made that @options will be a hash:

periodic_job.present? && periodic_job.options.fetch('cronitor_key', nil)

Example error

2023-10-31 19:20:00.112938 I [6:default/processor] Sidekiq -- fail
2023-10-31 19:20:00.113108 W [6:default/processor] Sidekiq -- {"context":"Job raised exception","job":{"retry":true,"queue":"default","class":"TestLoanProConnection","args":[],"cronitor_enabled":true,"jid":"50ce858a9af4a387a7277a26","created_at":1698780000.085093,"enqueued_at":1698780000.085868},"_config":"#<Sidekiq::Config:0x0000ffffb0e65810>"}
2023-10-31 19:20:00.114064 W [6:default/processor] Sidekiq -- NoMethodError: undefined method `fetch' for "{\"cronitor_enabled\":true}":String

      periodic_job.present? && periodic_job.options.fetch('cronitor_key', nil)
                                                   ^^^^^^
2023-10-31 19:20:00.114091 W [6:default/processor] Sidekiq -- /rails_terraform_docker/vendor/bundle/ruby/3.1.0/gems/sidekiq-cronitor-3.7.1/lib/sidekiq/cronitor.rb:55:in `periodic_job_key'
/rails_terraform_docker/vendor/bundle/ruby/3.1.0/gems/sidekiq-cronitor-3.7.1/lib/sidekiq/cronitor.rb:44:in `job_key'
/rails_terraform_docker/vendor/bundle/ruby/3.1.0/gems/sidekiq-cronitor-3.7.1/lib/sidekiq/cronitor.rb:76:in `rescue in ping'
/rails_terraform_docker/vendor/bundle/ruby/3.1.0/gems/sidekiq-cronitor-3.7.1/lib/sidekiq/cronitor.rb:67:in `ping'
/rails_terraform_docker/vendor/bundle/ruby/3.1.0/gems/sidekiq-cronitor-3.7.1/lib/sidekiq/cronitor.rb:21:in `rescue in call'
/rails_terraform_docker/vendor/bundle/ruby/3.1.0/gems/sidekiq-cronitor-3.7.1/lib/sidekiq/cronitor.rb:16:in `call'
/rails_terraform_docker/vendor/bundle/ruby/3.1.0/gems/sidekiq-7.0.3/lib/sidekiq/middleware/chain.rb:177:in `block in invoke'
/rails_terraform_docker/vendor/bundle/ruby/3.1.0/gems/sidekiq-pro-7.0.10/lib/sidekiq/batch/middleware.rb:58:in `call'
/rails_terraform_docker/vendor/bundle/ruby/3.1.0/gems/sidekiq-7.0.3/lib/sidekiq/middleware/chain.rb:177:in `block in invoke'
/rails_terraform_docker/vendor/bundle/ruby/3.1.0/gems/sidekiq-ent-7.0.8/lib/sidekiq-ent/limiter/middleware.rb:40:in `call'
/rails_terraform_docker/vendor/bundle/ruby/3.1.0/gems/sidekiq-7.0.3/lib/sidekiq/middleware/chain.rb:177:in `block in invoke'
/rails_terraform_docker/vendor/bundle/ruby/3.1.0/gems/sidekiq-7.0.3/lib/sidekiq/metrics/tracking.rb:24:in `track'
/rails_terraform_docker/vendor/bundle/ruby/3.1.0/gems/sidekiq-7.0.3/lib/sidekiq/metrics/tracking.rb:120:in `call'
/rails_terraform_docker/vendor/bundle/ruby/3.1.0/gems/sidekiq-7.0.3/lib/sidekiq/middleware/chain.rb:177:in `block in invoke'
/rails_terraform_docker/vendor/bundle/ruby/3.1.0/gems/sidekiq-7.0.3/lib/sidekiq/middleware/chain.rb:180:in `invoke'
/rails_terraform_docker/vendor/bundle/ruby/3.1.0/gems/sidekiq-7.0.3/lib/sidekiq/processor.rb:173:in `block in process'
/rails_terraform_docker/vendor/bundle/ruby/3.1.0/gems/sidekiq-7.0.3/lib/sidekiq/processor.rb:140:in `block (6 levels) in dispatch'
/rails_terraform_docker/vendor/bundle/ruby/3.1.0/gems/sidekiq-7.0.3/lib/sidekiq/job_retry.rb:113:in `local'
/rails_terraform_docker/vendor/bundle/ruby/3.1.0/gems/sidekiq-7.0.3/lib/sidekiq/processor.rb:139:in `block (5 levels) in dispatch'
/rails_terraform_docker/vendor/bundle/ruby/3.1.0/gems/sidekiq-7.0.3/lib/sidekiq/rails.rb:16:in `block in call'
/rails_terraform_docker/vendor/bundle/ruby/3.1.0/gems/activesupport-7.0.4.3/lib/active_support/execution_wrapper.rb:92:in `wrap'
/rails_terraform_docker/vendor/bundle/ruby/3.1.0/gems/activesupport-7.0.4.3/lib/active_support/reloader.rb:72:in `block in wrap'
/rails_terraform_docker/vendor/bundle/ruby/3.1.0/gems/activesupport-7.0.4.3/lib/active_support/execution_wrapper.rb:92:in `wrap'
/rails_terraform_docker/vendor/bundle/ruby/3.1.0/gems/activesupport-7.0.4.3/lib/active_support/reloader.rb:71:in `wrap'
/rails_terraform_docker/vendor/bundle/ruby/3.1.0/gems/sidekiq-7.0.3/lib/sidekiq/rails.rb:15:in `call'
/rails_terraform_docker/vendor/bundle/ruby/3.1.0/gems/sidekiq-7.0.3/lib/sidekiq/processor.rb:135:in `block (4 levels) in dispatch'
/rails_terraform_docker/vendor/bundle/ruby/3.1.0/gems/sidekiq-7.0.3/lib/sidekiq/processor.rb:267:in `stats'
/rails_terraform_docker/vendor/bundle/ruby/3.1.0/gems/sidekiq-7.0.3/lib/sidekiq/processor.rb:130:in `block (3 levels) in dispatch'
/rails_terraform_docker/vendor/bundle/ruby/3.1.0/gems/sidekiq-7.0.3/lib/sidekiq/job_logger.rb:13:in `call'
/rails_terraform_docker/vendor/bundle/ruby/3.1.0/gems/sidekiq-7.0.3/lib/sidekiq/processor.rb:129:in `block (2 levels) in dispatch'
/rails_terraform_docker/vendor/bundle/ruby/3.1.0/gems/sidekiq-7.0.3/lib/sidekiq/job_retry.rb:80:in `global'
/rails_terraform_docker/vendor/bundle/ruby/3.1.0/gems/sidekiq-7.0.3/lib/sidekiq/processor.rb:128:in `block in dispatch'
/rails_terraform_docker/vendor/bundle/ruby/3.1.0/gems/sidekiq-7.0.3/lib/sidekiq/job_logger.rb:39:in `prepare'
/rails_terraform_docker/vendor/bundle/ruby/3.1.0/gems/sidekiq-7.0.3/lib/sidekiq/processor.rb:127:in `dispatch'
/rails_terraform_docker/vendor/bundle/ruby/3.1.0/gems/sidekiq-7.0.3/lib/sidekiq/processor.rb:172:in `process'
/rails_terraform_docker/vendor/bundle/ruby/3.1.0/gems/sidekiq-7.0.3/lib/sidekiq/processor.rb:82:in `process_one'
/rails_terraform_docker/vendor/bundle/ruby/3.1.0/gems/sidekiq-7.0.3/lib/sidekiq/processor.rb:72:in `run'
/rails_terraform_docker/vendor/bundle/ruby/3.1.0/gems/sidekiq-7.0.3/lib/sidekiq/component.rb:8:in `watchdog'
/rails_terraform_docker/vendor/bundle/ruby/3.1.0/gems/sidekiq-7.0.3/lib/sidekiq/component.rb:17:in `block in safe_thread'

Initializer

require 'sidekiq'
require 'sidekiq/cronitor/periodic_jobs'

sidekiq_config = { url: ENV.fetch('DEDICATED_REDIS_URI', 'redis://localhost:6379/0') }

Sidekiq.configure_server do |config|
  config.server_middleware do |chain|
    chain.add Sidekiq::Cronitor::ServerMiddleware
  end
  LogstopGuard.guard!(config.logger)
  config.redis = sidekiq_config
  config.periodic do |mgr|
    mgr.register('*/5 * * * 1-5', 'TestLoanProConnection', cronitor_enabled: true)
  end
end

Suggested Fix

1. Option Parsing: To ensure compatibility with both the current and any fixed version of Sidekiq Enterprise, a version-agnostic approach could be to check the type of @options before accessing its keys.

if periodic_job.present?
  options = JSON.parse(periodic_job.options) if periodic_job.options.is_a?(String)
  options ||= periodic_job.options
  options.fetch('cronitor_key', nil)
end

Probably would want to rescue JSON::ParserError

[aflanagan]

Hi @nohat, thanks for taking the time to document this, and appreciate such a thorough write up! Would you be willing to open a PR that implements your suggested options parsing approach? That looks like a good way to solve it to me.

EDIT: I just read Mike's reply on the issue filed in the Sidekiq repo, and it sounds like the right approach is to just look at the value directly in the job hash.