From ae613c8d9f53d179466f1f98750a1c2d66c90d86 Mon Sep 17 00:00:00 2001 From: epinault Date: Thu, 26 Mar 2026 15:11:21 -0700 Subject: [PATCH 1/3] Fix timestamp unit mismatch in TokenBucket and LeakyBucket clean/1 hit/5 stores timestamps in seconds (System.system_time(:second)), but clean/1 was using ETS.now() which returns milliseconds. This caused all entries to appear stale and get deleted every cleanup cycle, silently resetting rate limits. - Use System.system_time(:second) in clean/1 to match hit/5 - Convert key_older_than from ms to seconds with div/2 - Update tests to set key_older_than: 1000 so entries age out in tests Fixes #174 --- lib/hammer/ets/leaky_bucket.ex | 4 ++-- lib/hammer/ets/token_bucket.ex | 4 ++-- test/hammer/ets/clean_test.exs | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/hammer/ets/leaky_bucket.ex b/lib/hammer/ets/leaky_bucket.ex index 224533c..c8b0122 100644 --- a/lib/hammer/ets/leaky_bucket.ex +++ b/lib/hammer/ets/leaky_bucket.ex @@ -165,8 +165,8 @@ defmodule Hammer.ETS.LeakyBucket do """ @spec clean(config :: ETS.config()) :: non_neg_integer() def clean(config) do - now = ETS.now() - older_than = now - config.key_older_than + now = System.system_time(:second) + older_than = now - div(config.key_older_than, 1000) match_spec = [{{:_, :_, :"$1"}, [], [{:<, :"$1", {:const, older_than}}]}] :ets.select_delete(config.table, match_spec) diff --git a/lib/hammer/ets/token_bucket.ex b/lib/hammer/ets/token_bucket.ex index b2d1fa8..e06a5ee 100644 --- a/lib/hammer/ets/token_bucket.ex +++ b/lib/hammer/ets/token_bucket.ex @@ -165,8 +165,8 @@ defmodule Hammer.ETS.TokenBucket do """ @spec clean(config :: ETS.config()) :: non_neg_integer() def clean(config) do - now = ETS.now() - older_than = now - config.key_older_than + now = System.system_time(:second) + older_than = now - div(config.key_older_than, 1000) match_spec = [{{:_, :_, :"$1"}, [], [{:<, :"$1", {:const, older_than}}]}] :ets.select_delete(config.table, match_spec) diff --git a/test/hammer/ets/clean_test.exs b/test/hammer/ets/clean_test.exs index 0ff462f..e945ff9 100644 --- a/test/hammer/ets/clean_test.exs +++ b/test/hammer/ets/clean_test.exs @@ -72,7 +72,7 @@ defmodule Hammer.ETS.CleanTest do end test "cleaning works for token bucket" do - start_supervised!({RateLimitTokenBucket, clean_period: 100}) + start_supervised!({RateLimitTokenBucket, clean_period: 100, key_older_than: 1000}) key = "key" refill_rate = 1 @@ -89,7 +89,7 @@ defmodule Hammer.ETS.CleanTest do end test "cleaning works for leaky bucket" do - start_supervised!({RateLimitLeakyBucket, clean_period: 100}) + start_supervised!({RateLimitLeakyBucket, clean_period: 100, key_older_than: 1000}) key = "key" leak_rate = 1 From ff70a31936e2404764523477e23806be769095c0 Mon Sep 17 00:00:00 2001 From: epinault Date: Thu, 26 Mar 2026 15:32:51 -0700 Subject: [PATCH 2/3] Fix same timestamp unit mismatch in Atomic backend clean/1 The Atomic backend's clean/1 had the same bug: hit/5 stores timestamps in seconds for TokenBucket/LeakyBucket, but clean/1 compared using now() which returns milliseconds. Branch clean/1 by algorithm: FixWindow correctly uses milliseconds throughout, while bucket algorithms now use seconds with key_older_than converted from ms to seconds via div/2. Fixes #174 --- lib/hammer/atomic.ex | 54 ++++++++++++++++++++++--------- test/hammer/atomic/clean_test.exs | 4 +-- 2 files changed, 41 insertions(+), 17 deletions(-) diff --git a/lib/hammer/atomic.ex b/lib/hammer/atomic.ex index 0c67385..240b683 100644 --- a/lib/hammer/atomic.ex +++ b/lib/hammer/atomic.ex @@ -188,22 +188,46 @@ defmodule Hammer.Atomic do defp clean(config) do table = config.table - now = now() - - :ets.foldl( - fn {_key, atomic} = term, deleted -> - expires_at = :atomics.get(atomic, 2) + case config.algorithm_module do + Hammer.Atomic.FixWindow -> + # FixWindow stores expires_at in milliseconds in slot 2 + now = now() + + :ets.foldl( + fn {_key, atomic} = term, deleted -> + expires_at = :atomics.get(atomic, 2) + + if now - expires_at > config.key_older_than do + :ets.delete_object(table, term) + deleted + 1 + else + deleted + end + end, + 0, + table + ) - if now - expires_at > config.key_older_than do - :ets.delete_object(table, term) - deleted + 1 - else - deleted - end - end, - 0, - table - ) + _ -> + # TokenBucket and LeakyBucket store last_update in seconds in slot 2 + now = System.system_time(:second) + older_than = now - div(config.key_older_than, 1000) + + :ets.foldl( + fn {_key, atomic} = term, deleted -> + last_update = :atomics.get(atomic, 2) + + if last_update < older_than do + :ets.delete_object(table, term) + deleted + 1 + else + deleted + end + end, + 0, + table + ) + end end defp schedule(clean_period) do diff --git a/test/hammer/atomic/clean_test.exs b/test/hammer/atomic/clean_test.exs index 62c5f70..0ac3aef 100644 --- a/test/hammer/atomic/clean_test.exs +++ b/test/hammer/atomic/clean_test.exs @@ -51,7 +51,7 @@ defmodule Hammer.Atomic.CleanTest do end test "cleaning works for token bucket" do - start_supervised!({RateAtomicLimitTokenBucket, clean_period: 50, key_older_than: 10}) + start_supervised!({RateAtomicLimitTokenBucket, clean_period: 100, key_older_than: 1000}) key = "key" refill_rate = 1 @@ -68,7 +68,7 @@ defmodule Hammer.Atomic.CleanTest do end test "cleaning works for leaky bucket" do - start_supervised!({RateAtomicLimitLeakyBucket, clean_period: 50, key_older_than: 10}) + start_supervised!({RateAtomicLimitLeakyBucket, clean_period: 100, key_older_than: 1000}) key = "key" leak_rate = 1 From 59a3fabda427edb27ae9bc6619f851f4412a4a8a Mon Sep 17 00:00:00 2001 From: epinault Date: Thu, 26 Mar 2026 15:45:37 -0700 Subject: [PATCH 3/3] Refactor Atomic clean/1 to fix credo nesting depth warning Extract clean_fix_window/1 and clean_bucket/1 to reduce nesting depth. --- lib/hammer/atomic.ex | 83 +++++++++++++++++++++++--------------------- 1 file changed, 43 insertions(+), 40 deletions(-) diff --git a/lib/hammer/atomic.ex b/lib/hammer/atomic.ex index 240b683..780b41c 100644 --- a/lib/hammer/atomic.ex +++ b/lib/hammer/atomic.ex @@ -186,50 +186,53 @@ defmodule Hammer.Atomic do end defp clean(config) do - table = config.table - case config.algorithm_module do - Hammer.Atomic.FixWindow -> - # FixWindow stores expires_at in milliseconds in slot 2 - now = now() - - :ets.foldl( - fn {_key, atomic} = term, deleted -> - expires_at = :atomics.get(atomic, 2) - - if now - expires_at > config.key_older_than do - :ets.delete_object(table, term) - deleted + 1 - else - deleted - end - end, - 0, - table - ) - - _ -> - # TokenBucket and LeakyBucket store last_update in seconds in slot 2 - now = System.system_time(:second) - older_than = now - div(config.key_older_than, 1000) - - :ets.foldl( - fn {_key, atomic} = term, deleted -> - last_update = :atomics.get(atomic, 2) - - if last_update < older_than do - :ets.delete_object(table, term) - deleted + 1 - else - deleted - end - end, - 0, - table - ) + Hammer.Atomic.FixWindow -> clean_fix_window(config) + _ -> clean_bucket(config) end end + # FixWindow stores expires_at in milliseconds in slot 2 + defp clean_fix_window(config) do + now = now() + + :ets.foldl( + fn {_key, atomic} = term, deleted -> + expires_at = :atomics.get(atomic, 2) + + if now - expires_at > config.key_older_than do + :ets.delete_object(config.table, term) + deleted + 1 + else + deleted + end + end, + 0, + config.table + ) + end + + # TokenBucket and LeakyBucket store last_update in seconds in slot 2 + defp clean_bucket(config) do + now = System.system_time(:second) + older_than = now - div(config.key_older_than, 1000) + + :ets.foldl( + fn {_key, atomic} = term, deleted -> + last_update = :atomics.get(atomic, 2) + + if last_update < older_than do + :ets.delete_object(config.table, term) + deleted + 1 + else + deleted + end + end, + 0, + config.table + ) + end + defp schedule(clean_period) do Process.send_after(self(), :clean, clean_period) end