hammer
hammer copied to clipboard
Fix bug that allows check_rate to succeed 2x in sequence even if the bucket limit is 1 attempt per hour
Problem
When check_rate
is called for the first time, it's expected that ms_to_next_bucket
would be qual to scale_ms
, but that's not the case at the moment due to:
-
Utils.stamp_key/2
is currently creating a variable bucket key for the same ID andscale_ms
. The bucket key changes everyscale_ms
milliseconds. - Redis/ETS backends are not expiring the bucket key after
scale_ms
, instead they rely that a new key is provided (1)
This bug allows check_rate to be called 2x in sequence even if the bucket limit is 1 attempt per hour. Both calls could receive :allow
if timed when the bucket is about to expire (see how to reproduce the bug below).
How to reproduce the bug
# Install hammer
Application.put_env(
:hammer,
:backend,
{Hammer.Backend.ETS,
[
ets_table_name: :hammer_backend_ets_buckets,
expiry_ms: :timer.hours(24),
cleanup_interval_ms: :timer.hours(24)
]}
)
Mix.install([:hammer])
# Reproduce the bug
bucket_key = "bucket_key:#{:rand.uniform()}"
scale_ms = :timer.seconds(20)
max_attempts = 1
ms_about_to_expire = 100
wait_for_timing_fn = fn ->
{:ok, {0, 1, ms_to_next_bucket, nil, nil}} =
Hammer.inspect_bucket(bucket_key, scale_ms, max_attempts)
if ms_to_next_bucket >= ms_about_to_expire do
:timer.sleep(ms_to_next_bucket - ms_about_to_expire)
end
:ready
end
:ready = wait_for_timing_fn.()
{:allow, 1} = Hammer.check_rate(bucket_key, scale_ms, max_attempts)
:timer.sleep(ms_about_to_expire)
{:allow, 1} = Hammer.check_rate(bucket_key, scale_ms, max_attempts)
Proposed solution
This PR fixes this issue by:
- Returning a fixed key on
Utils.stamp_key/2
for the same bucket ID andscale_ms
- Requiring the Redis/ETS backends to expire the bucket after
scale_ms
. I will open a PR onhammer-backend-redis
if this PR is accepted: https://github.com/nash-io/hammer-backend-redis/pull/1