ixti / sidekiq-throttled

Concurrency and rate-limit throttling for Sidekiq
MIT License
713 stars 77 forks source link

Unwrap ActiveJob arguments #184

Closed jlledom closed 8 months ago

jlledom commented 8 months ago

When using the concurrency strategy with dynamic suffix, I'm getting this error from sidekiq-throttle:

ArgumentError: wrong number of arguments (given 1, expected 2+)

This happens because the key_suffix I'm using is not receiving the proper arguments. Consider this dummy job:

class TestJob < ActiveJob::Base
  include Sidekiq::Throttled::Job

  queue_as :default

  sidekiq_throttle(
    concurrency: {
      limit: 1,
      key_suffix: ->(job_class, index, *) { "job_class:#{job_class}/index:#{index}" },
      ttl: 1.hour.to_i
    }
  )

  def perform(job_class, index)
    puts "Job Performed. job_class: #{job_class}, index: #{index}"
  end
end

This follows the documentation, which states that key_suffix must accept the same parameters as the job. However, sidekiq-throttle fails to unwrap the ActiveJob arguments and instead it just calls the :throttled? and :finalize! methods passing them the arguments in msg["args"], which in ActiveJob is an array of JSON objects, like this:

{
  "args": [
    {
      "job_class": "TestJob",
      "job_id": "96f6c127-1e0c-4d8c-bdaa-c934b5aaded7",
      "provider_job_id": null,
      "queue_name": "default",
      "priority": null,
      "arguments": ["TestJob", 9],
      "executions": 0,
      "exception_executions": {},
      "locale": "en",
      "timezone": null,
      "enqueued_at": "2024-03-15T11:31:11.464949297Z",
      "scheduled_at": null
    }
  ]
}

Some other methods in the Concurrency class like :count or :reset! also call the :key method, so they are vulnerable to this error too. However, I haven't found any place in the code where these methods are called with any parameter.

I wrote a very simple workaround that seems to work fine for my use case, and the test suite passes.

codecov[bot] commented 8 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 98.95%. Comparing base (1b71da2) to head (5dd5b5e).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #184 +/- ## ======================================= Coverage 98.95% 98.95% ======================================= Files 18 18 Lines 381 383 +2 Branches 53 55 +2 ======================================= + Hits 377 379 +2 Misses 4 4 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ixti commented 8 months ago

Thank you!