From c128a0f4d995f2d2f5b3bf24d33d287f376fb491 Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Tue, 3 Nov 2026 00:07:57 +0000 Subject: [PATCH] fix(ratelimit): count Req per Interval, not per second The sustained gate compared requests per second with Req, which is a count per Interval. An interval of one minute with req 10 allowed 600 requests. The gate now compares the count in the interval with Req. One-second intervals behave as before. A burst at or above req has no effect, because the sustained gate always binds first. --- README.md | 2 +- ratelimit.go | 36 +++++++++---------------------- ratelimit_test.go | 47 ++++++----------------------------------- ratelimit_units_test.go | 44 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 62 insertions(+), 67 deletions(-) create mode 100644 ratelimit_units_test.go diff --git a/README.md b/README.md index ef57fc6..3f7de5a 100644 --- a/README.md +++ b/README.md @@ -821,7 +821,7 @@ Enable rate limiting based on user roles using the `ROLE_RATE_LIMIT` environment - `ROLE_RATE_LIMIT`: Enable role-based rate limiting (default: `false`) **Features:** -- **Burst control**: Optional burst limits for handling traffic spikes +- **Burst control**: Optional `burst` caps requests in a short window (one tenth of the interval). `req` counts requests per `interval`. A `burst` at or above `req` has no effect. `ratelimit.json` is loaded once at startup; changing the file requires a restart. diff --git a/ratelimit.go b/ratelimit.go index fb5b6e7..af4c49e 100644 --- a/ratelimit.go +++ b/ratelimit.go @@ -304,35 +304,19 @@ func rateLimitedRequest(userID, userRole string) bool { // checkRateLimit performs the actual rate limit check. // -// Two independent gates apply, both must pass: +// Two gates apply, both must pass: // -// 1. Sustained: tickerRate (requests measured over the full Interval) must -// stay at or below Req. This is the only gate when Burst <= 0, and its -// comparison is unchanged from before the burst fix. -// 2. Burst (only when roleConfig.burstCounter != nil, i.e. Burst > 0): the -// raw request count within a short trailing window (see -// burstWindowDuration), much shorter than Interval, must stay at or -// below Burst. +// 1. Sustained: the request count over the full Interval must stay at or +// below Req (Req is per Interval, not per second). +// 2. Burst (only when Burst > 0): the request count within a short trailing +// window (see burstWindowDuration) must stay at or below Burst. // -// Because the burst window is short relative to Interval, a client can -// legally land up to Burst requests in a tight cluster - more than a -// steady Req-paced stream would deliver in that same short span - as long -// as the sustained gate still holds over the full Interval. That is what -// makes burst > req a real spike allowance instead of a no-op. Burst is a -// short-window ceiling layered ON TOP of the sustained Req rate, not a -// replacement for it. burst < req is an unusual config: the burst gate -// becomes the tighter, binding constraint whenever requests cluster in -// time, but the sustained cap for evenly-spread traffic is still Req -// alone. The pre-fix code compared the SAME sustained-window tickerRate -// against both Burst and Req, so when burst < req its Burst check tripped -// first and Burst was effectively the sustained cap too. This gate's -// Req-only sustained check can therefore allow MORE through than the -// pre-fix code did for evenly-spread traffic under a burst < req config. -// In both cases the burst gate never allows more than Burst requests -// within its own window (fail-safe). +// The burst window lies inside the Interval, so its count never exceeds the +// sustained count. Burst >= Req therefore never rejects anything the sustained +// gate admits; Burst only takes effect when it is below Req. func checkRateLimit(userID, userRole string, roleConfig RateLimitConfig, endpoint string) bool { roleConfig.RateCounterTicker.Incr(1) - tickerRate := roleConfig.RateCounterTicker.GetRate() + tickerRate := roleConfig.RateCounterTicker.Get() logDetails := map[string]any{ "user_role": userRole, @@ -348,7 +332,7 @@ func checkRateLimit(userID, userRole string, roleConfig RateLimitConfig, endpoin Pairs: map[string]any{"log_details": logDetails}, }) - if tickerRate > float64(roleConfig.Req) { + if tickerRate > int64(roleConfig.Req) { cfg.Logger.Debug(&libpack_logger.LogMessage{ Message: "Rate limit exceeded", Pairs: map[string]any{"log_details": logDetails}, diff --git a/ratelimit_test.go b/ratelimit_test.go index 563a401..872bebd 100644 --- a/ratelimit_test.go +++ b/ratelimit_test.go @@ -372,59 +372,26 @@ func Test_checkRateLimit_BurstSemantics(t *testing.T) { } }) - t.Run("b_burst_above_req_allows_short_spike_sustained_still_capped", func(t *testing.T) { + t.Run("b_burst_above_req_is_noop_sustained_caps_at_req", func(t *testing.T) { const ( interval = 10 * time.Second req = 3 - burst = 6 // burst > req + burst = 6 // burst >= req: dominated by the sustained gate ) rc := newBurstTestRoleConfig(interval, req, burst) fc := newFakeClock(time.Now()) rc.burstCounter.now = fc.Now - // A rapid cluster of `burst` requests must all be allowed, even - // though burst (6) > req (3): the sustained gate only bounds the - // total over the full 10s Interval (tickerRate <= 3 needs count <= - // 30), so a small cluster like this never trips it. - for i := 1; i <= burst; i++ { + for i := 1; i <= req; i++ { if !checkRateLimit("u", "role-b", rc, "") { - t.Fatalf("request %d in first burst: got denied, want allowed (<= Burst=%d)", i, burst) + t.Fatalf("request %d: got denied, want allowed (<= Req=%d per Interval)", i, req) } } - // The (burst+1)th request in the SAME short window must be denied - // by the burst gate specifically - not by the sustained gate, - // which alone would still allow it (tickerRate = 7/10 = 0.7 <= 3). - // This is the proof burst > req is not a no-op: it is the gate - // that actually fires here. + // Clearing the burst window must not readmit: Req is per Interval. + fc.Advance(2 * burstWindowDuration(interval)) if checkRateLimit("u", "role-b", rc, "") { - t.Fatalf("request %d: got allowed, want denied by burst gate (> Burst=%d)", burst+1, burst) - } - - // Roll the burst window forward (fake clock only - the real - // sustained ticker has not aged, since no real time passed) and - // repeat clusters of `burst` requests. Each cluster alone passes - // burst, but the cumulative sustained count keeps climbing until - // it exceeds req*interval.Seconds() = 30, at which point the - // sustained gate must deny regardless of how the burst window - // resets. This proves "sustained still capped at Req". - sustainedTripped := false - for batch := 0; batch < 10 && !sustainedTripped; batch++ { - fc.Advance(2 * burstWindowDuration(interval)) // clears the burst window - for i := 1; i <= burst; i++ { - allowed := checkRateLimit("u", "role-b", rc, "") - rate := rc.RateCounterTicker.GetRate() - if rate > float64(req) { - if allowed { - t.Fatalf("batch %d req %d: allowed with sustained rate %.2f > Req=%d", batch, i, rate, req) - } - sustainedTripped = true - break - } - } - } - if !sustainedTripped { - t.Fatalf("sustained gate never tripped after repeated bursts; expected it to cap total requests at Req over the Interval") + t.Fatalf("request %d: got allowed, want denied by the sustained gate (Req=%d per Interval)", req+1, req) } }) diff --git a/ratelimit_units_test.go b/ratelimit_units_test.go new file mode 100644 index 0000000..8ed952e --- /dev/null +++ b/ratelimit_units_test.go @@ -0,0 +1,44 @@ +package main + +import ( + "testing" + "time" + + libpack_logger "github.com/lukaszraczylo/graphql-monitoring-proxy/logging" +) + +func setupRateLimitCfg() { + cfg = &config{} + parseConfig() + cfg.Logger = libpack_logger.New() +} + +// Pins theorems/Theorems/RateLimitUnits.lean: Req counts requests per Interval, not per second. +func TestCheckRateLimit_ReqIsPerInterval(t *testing.T) { + const req = 3 + setupRateLimitCfg() + rc := newBurstTestRoleConfig(time.Minute, req, 0) + for i := 1; i <= req; i++ { + if !checkRateLimit("u", "r", rc, "") { + t.Fatalf("request %d denied, want allowed (Req=%d per minute)", i, req) + } + } + if checkRateLimit("u", "r", rc, "") { + t.Fatalf("request %d allowed, want denied: Req=%d per minute", req+1, req) + } +} + +// With Burst >= Req the burst gate never rejects what the sustained gate admits. +func TestCheckRateLimit_BurstAboveReqIsNoOp(t *testing.T) { + const req, burst = 3, 6 + setupRateLimitCfg() + rc := newBurstTestRoleConfig(time.Minute, req, burst) + for i := 1; i <= req; i++ { + if !checkRateLimit("u", "r", rc, "") { + t.Fatalf("request %d denied", i) + } + } + if checkRateLimit("u", "r", rc, "") { + t.Fatalf("request %d allowed, want denied by the sustained gate", req+1) + } +}