ratelimit: eviction path untested and comment/policy mismatch #529

New issue

Open

opened 2026-08-04 19:36:59 +00:00 by the.auditor · 0 comments

the.auditor commented

2026-08-04 19:36:59 +00:00

Owner

TestEvictionBoundsMap generates IPs via fmt.Sprintf("10.0.0.%d", i%256) — only 256 distinct keys against maxIPs = 1024, so len(l.limiters) >= maxIPs is never reached and evictOneLocked is never exercised; the test passes trivially. The eviction path (the trickiest logic) is untested. Also, forIP's comment says "oldest-inserted bucket is dropped" but the implementation evicts the bucket with fewest remaining tokens.

Context: PR #526 (internal/ratelimit/ratelimit_test.go, internal/ratelimit/ratelimit.go:80).

Suggested fix: generate more than maxIPs unique IPs so eviction actually triggers, and align the comment with the fewest-tokens policy.

`TestEvictionBoundsMap` generates IPs via `fmt.Sprintf("10.0.0.%d", i%256)` — only 256 distinct keys against `maxIPs = 1024`, so `len(l.limiters) >= maxIPs` is never reached and `evictOneLocked` is never exercised; the test passes trivially. The eviction path (the trickiest logic) is untested. Also, `forIP`'s comment says "oldest-inserted bucket is dropped" but the implementation evicts the bucket with fewest remaining tokens. Context: PR #526 (`internal/ratelimit/ratelimit_test.go`, `internal/ratelimit/ratelimit.go:80`). Suggested fix: generate more than `maxIPs` unique IPs so eviction actually triggers, and align the comment with the fewest-tokens policy.

the.auditor added the

Kind/Testing

Priority

Low

labels

2026-08-04 19:37:03 +00:00