Skip to content

Commit a8df4ee

Browse files
committed
Fix lock leak in multiple concurrency strategies
1 parent c799ca3 commit a8df4ee

2 files changed

Lines changed: 34 additions & 1 deletion

File tree

lib/sidekiq/throttled/strategy.rb

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,9 +67,11 @@ def dynamic?
6767
end
6868

6969
# @return [Boolean] whenever job is throttled or not.
70-
def throttled?(jid, *job_args)
70+
def throttled?(jid, *job_args) # rubocop:disable Metrics/MethodLength
7171
if @concurrency&.throttled?(jid, *job_args)
7272
@observer&.call(:concurrency, *job_args)
73+
74+
finalize!(jid, *job_args)
7375
return true
7476
end
7577

spec/lib/sidekiq/throttled/strategy_spec.rb

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -205,6 +205,37 @@
205205
end
206206
end
207207

208+
context "when throttled by a later concurrency strategy in the collection" do
209+
let(:options) do
210+
{
211+
concurrency: [
212+
{ limit: 5, key_suffix: ->(*) { "global" } },
213+
{ limit: 1, key_suffix: ->(*) { "per_tenant" } }
214+
]
215+
}
216+
end
217+
218+
before do
219+
# Saturate the second concurrency strategy (per_tenant limit = 1)
220+
# by running a different job through it first
221+
strategy.throttled?("seed-jid")
222+
end
223+
224+
it "releases the first strategy's lock when the second strategy throttles" do
225+
first_strategy = strategy.concurrency.strategies.first
226+
227+
# Confirm the second strategy is at capacity before our call
228+
expect(strategy.concurrency.strategies.last.count).to eq(1)
229+
230+
# This call should be throttled by the second strategy (per_tenant is full)
231+
expect(strategy.throttled?(jid)).to be true
232+
233+
# The first strategy must NOT have a leaked lock for jid.
234+
# Count should remain 1 (from seed-jid only), not 2.
235+
expect(first_strategy.count).to eq(1)
236+
end
237+
end
238+
208239
context "when both concurrency and threshold given" do
209240
let(:options) { threshold.merge concurrency }
210241

0 commit comments

Comments
 (0)