Rate limiter: independent config defaults, ASN-sharing fix, bounded maps, contention cut - #65
Closed
Tumult1337 wants to merge 3 commits into
Closed
Rate limiter: independent config defaults, ASN-sharing fix, bounded maps, contention cut#65Tumult1337 wants to merge 3 commits into
Tumult1337 wants to merge 3 commits into
Conversation
NewLimiter only substituted defaults when IPRate was zero, so a partial config (IPRate set, CleanupInterval unset) reached cleanupLoop with a zero interval and panicked in time.NewTicker on a background goroutine, taking down the process. Clamp every field to its default when unset or non-positive.
- AllowASN no longer buckets on the "Unknown"/empty ASN sentinel that pkg/geoip returns for unresolved lookups. Previously every client with an unresolved ASN shared one TokenBucket, so an attacker could drain it and collaterally block unrelated victims (W5). - ipLimiters/asnLimiters now enforce a hard entry cap (MaxIPEntries/ MaxASNEntries, default 200000/50000), evicting the least-recently- used bucket on overflow instead of growing unbounded between the 5-minute idle cleanup passes (W13). - TokenBucket gained a setRate method, and SetIPRate/SetASNRate now apply the new rate/capacity to every already-tracked bucket, not just buckets created after the call (W27).
Two hot-path costs, one per signal from every collector stream: - AllowIP/AllowASN took the exclusive limiter lock even when the token bucket already existed (the overwhelmingly common case), serializing all signal processing. Use an RLock fast path with double-checked insert under the write lock. - checkRateLimit called GetStats (another lock round-trip) plus two Prometheus gauge writes per signal just to publish activity gauges. Publish them from a 10s ticker next to the tracking-map cleanup instead; enforcement behavior is unchanged. benchstat (10 runs, i7-12700K), parallel Allow hit path: 361.8ns -> 107.1ns (-70.4%) Also adds the package's first unit tests (burst exhaustion deny paths, nil-input allows, concurrent access under -race).
awlx
added a commit
that referenced
this pull request
Aug 8, 2026
Owner
|
Superseded by and merged through #77, which preserves this contribution and includes the follow-up fixes. Thanks for the contribution! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rate limiter correctness + hot-path perf.
Commits
Allowfast path (perf).Reviewer callout (low)
EnforceMaxSizeBatch) runs under the exclusive limiter lock; under a distinct-source-IP flood at the map cap it takes each bucket lock + an O(n log n) sort per batch. Amortized/bounded, but it is on the attack path — worth a benchmark if fleet-wideAllowlatency matters under load.Validation:
go build ./...clean;go test ./pkg/ratelimitpasses.