feat: add rate limit reason to Allow/Wait #2
Conversation
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
- Allow() returns (allowed, reason) instead of bool - Wait() returns (err, reason) instead of error - Add Reason type: fake_bot, rate_limited - Update knownbots to v1.0.5
Reviewer's GuideAdds structured rate-limit reasons to Limiter.Allow and Limiter.Wait while updating callers/tests to handle the new return values and upgrading the knownbots dependency. Sequence diagram for Limiter.Allow with rate limit reasonssequenceDiagram
actor Client
participant Limiter
participant KnownBots as KnownBotsValidator
participant Analyzer
Client->>Limiter: Allow(ua, ip) returns (allowed, reason)
Limiter->>KnownBots: Validate(ua, ip)
KnownBots-->>Limiter: botResult
alt Bot verification enabled
alt botResult.Status == StatusVerified
Limiter-->>Client: true, ""
else botResult.Status == StatusPending
Limiter-->>Client: true, ""
else botResult.Status == StatusFailed or StatusUnknown
Limiter-->>Client: false, ReasonFakeBot
end
else Bot verification disabled
alt Analyzer.Blocked(ip) == true
Limiter->>Limiter: allowBlocked(ip)
alt allowBlocked returns true
Limiter-->>Client: true, ""
else allowBlocked returns false
Limiter-->>Client: false, ReasonRateLimited
end
else Analyzer.Blocked(ip) == false
Limiter->>Analyzer: Record(ip, ua)
Limiter-->>Client: true, ""
end
end
Sequence diagram for Limiter.Wait with rate limit reasonssequenceDiagram
actor Client
participant Limiter
participant KnownBots as KnownBotsValidator
participant Analyzer
Client->>Limiter: Wait(ctx, ua, ip) returns (err, reason)
Limiter->>KnownBots: Validate(ua, ip)
KnownBots-->>Limiter: botResult
alt Bot verification enabled
alt botResult.Status == StatusVerified
Limiter-->>Client: nil, ""
else botResult.Status == StatusPending
Limiter-->>Client: nil, ""
else botResult.Status == StatusFailed or StatusUnknown
Limiter-->>Client: ErrLimit, ReasonFakeBot
end
else Bot verification disabled
alt Analyzer.Blocked(ip) == true
Limiter->>Limiter: waitBlocked(ctx, ip)
Limiter-->>Limiter: err
alt err == nil
Limiter-->>Client: nil, ""
else err != nil
Limiter-->>Client: ErrLimit, ReasonRateLimited
end
else Analyzer.Blocked(ip) == false
Limiter->>Analyzer: Record(ip, ua)
Limiter-->>Client: nil, ""
end
end
Updated class diagram for Limiter with Reason typeclassDiagram
class Reason {
<<enumeration>>
+string value
}
class ReasonFakeBot {
}
class ReasonRateLimited {
}
ReasonFakeBot --|> Reason
ReasonRateLimited --|> Reason
class Limiter {
-Config cfg
-KnownBotsValidator kb
-Analyzer analyzer
+New(opts Option) Limiter
+Allow(ua string, ip string) (allowed bool, reason Reason)
+Wait(ctx context.Context, ua string, ip string) (err error, reason Reason)
-allowBlocked(ip string) bool
-waitBlocked(ctx context.Context, ip string) error
}
class KnownBotsValidator {
+Validate(ua string, ip string) BotResult
}
class Analyzer {
+Blocked(ip string) bool
+Record(ip string, ua string) void
}
class BotResult {
+Status status
}
class status {
}
Limiter --> KnownBotsValidator : uses
Limiter --> Analyzer : uses
KnownBotsValidator --> BotResult : returns
BotResult --> status : has
Limiter --> Reason : returns
ReasonFakeBot ..> Limiter : used_as_reason
ReasonRateLimited ..> Limiter : used_as_reason
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Benchmark Results |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
Limiter.Wait, the new(error, Reason)signature currently maps any non-nil error fromwaitBlockedtoErrLimit, which loses important context likecontext.Canceled/context.DeadlineExceeded; consider returning the original context error (with an empty reason) instead of always collapsing toErrLimitso callers can still distinguish cancellation from rate limiting. - For consistency and clarity, consider introducing an explicit
ReasonNone(or similar) constant instead of returning the empty string""for the success case inAllow/Wait, so callers don't have to rely on an implicit empty-value convention.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `Limiter.Wait`, the new `(error, Reason)` signature currently maps any non-nil error from `waitBlocked` to `ErrLimit`, which loses important context like `context.Canceled`/`context.DeadlineExceeded`; consider returning the original context error (with an empty reason) instead of always collapsing to `ErrLimit` so callers can still distinguish cancellation from rate limiting.
- For consistency and clarity, consider introducing an explicit `ReasonNone` (or similar) constant instead of returning the empty string `""` for the success case in `Allow`/`Wait`, so callers don't have to rely on an implicit empty-value convention.
## Individual Comments
### Comment 1
<location> `limiter.go:120` </location>
<code_context>
+// Returns:
+// - err: nil if allowed, ErrLimit if blocked, or context error if canceled
+// - reason: the reason for blocking when err is ErrLimit
+func (l *Limiter) Wait(ctx context.Context, ua, ip string) (err error, reason Reason) {
// Layer 1: Bot verification
botResult := l.kb.Validate(ua, ip)
</code_context>
<issue_to_address>
**issue (bug_risk):** Context errors from waitBlocked are being masked as ErrLimit, losing the actual cancellation/timeout cause.
The updated `Wait` always wraps any non-nil `waitBlocked` error as `ErrLimit`/`ReasonRateLimited`, which conflicts with the documented behavior (“ErrLimit if blocked, or context error if canceled”) and hides whether the wait ended due to `context.Canceled` / `DeadlineExceeded`.
If `waitBlocked` can return context errors, they should still be propagated directly. For example:
```go
if l.analyzer.Blocked(ip) {
err = l.waitBlocked(ctx, ip)
if err == nil {
return nil, ""
}
if errors.Is(err, ErrLimit) {
return ErrLimit, ReasonRateLimited
}
// For context cancellation / timeout, propagate original error
return err, ""
}
```
This keeps the previous context-error behavior while still tagging genuine rate-limit errors with a reason.
</issue_to_address>
### Comment 2
<location> `botrate_test.go:100-109` </location>
<code_context>
- result := l.Allow("Googlebot/2.1", "66.249.66.1")
- _ = result
+ allowed, reason := l.Allow("TestBot/1.0", "192.168.100.42")
+ if !allowed {
+ t.Error("verified bot should be allowed")
+ }
+ if reason != "" {
+ t.Errorf("reason should be empty for allowed request, got %s", reason)
+ }
+
+ allowed, reason = l.Allow("TestBot/1.0", "10.0.0.1")
+ if allowed {
+ t.Error("fake bot should be blocked")
+ }
+ if reason != ReasonFakeBot {
+ t.Errorf("expected reason %s, got %s", ReasonFakeBot, reason)
+ }
</code_context>
<issue_to_address>
**suggestion (testing):** Add complementary Wait() tests that assert `reason` for verified and fake bots
To fully exercise the new `reason` return, please add analogous `Wait()` tests:
- `TestLimiter_Wait_VerifiedBotReason`: call `Wait()` with a verified bot and assert `err == nil` and `reason == ""`.
- `TestLimiter_Wait_FakeBotReason`: call `Wait()` with a fake/invalid bot (as in your second `Allow()` case) and assert `err == ErrLimit` and `reason == ReasonFakeBot`.
Without these, `Wait()`'s new `reason` behavior isn’t covered and regressions there would go unnoticed.
Suggested implementation:
```golang
allowed, reason := l.Allow("TestBot/1.0", "192.168.100.42")
if !allowed {
t.Error("verified bot should be allowed")
}
if reason != "" {
t.Errorf("reason should be empty for allowed request, got %s", reason)
}
// Verify Wait() behavior for a verified bot
ctx := context.Background()
waitReason, err := l.Wait(ctx, "TestBot/1.0", "192.168.100.42")
if err != nil {
t.Errorf("verified bot should not be rate limited, got err: %v", err)
}
if waitReason != "" {
t.Errorf("reason should be empty for allowed Wait request, got %s", waitReason)
}
allowed, reason = l.Allow("TestBot/1.0", "10.0.0.1")
if allowed {
t.Error("fake bot should be blocked")
}
if reason != ReasonFakeBot {
t.Errorf("expected reason %s, got %s", ReasonFakeBot, reason)
}
// Verify Wait() behavior for a fake/invalid bot
waitReason, err = l.Wait(ctx, "TestBot/1.0", "10.0.0.1")
if !errors.Is(err, ErrLimit) {
t.Errorf("fake bot should be rate limited with ErrLimit, got: %v", err)
}
if waitReason != ReasonFakeBot {
t.Errorf("expected Wait reason %s, got %s", ReasonFakeBot, waitReason)
}
```
1. Ensure `botrate_test.go` imports the required packages if they are not already present:
- `context`
- `errors`
For example, in the import block, add:
```go
import (
"context"
"errors"
// existing imports...
)
```
2. If the actual signature of `Wait` differs (e.g., it returns `(error, string)` instead of `(string, error)`), adjust the assignment order accordingly, e.g. `err, waitReason := l.Wait(...)` and keep the same assertions on `err` and `waitReason`.
3. If you strictly need separate top-level tests named `TestLimiter_Wait_VerifiedBotReason` and `TestLimiter_Wait_FakeBotReason`, factor the shared setup (creating `kb`, `l`, and the custom bot config) into a helper and call that helper from two new `Test...` functions that each perform the corresponding `Wait()` assertion.
</issue_to_address>
### Comment 3
<location> `botrate_test.go:102-105` </location>
<code_context>
ctx, cancel := context.WithCancel(context.Background())
cancel()
- err = l.Wait(ctx, "Mozilla/5.0", "192.168.1.1")
+ err, _ = l.Wait(ctx, "Mozilla/5.0", "192.168.1.1")
if err != nil && err != context.Canceled && err != ErrLimit {
</code_context>
<issue_to_address>
**suggestion (testing):** Add a dedicated test to verify `reason` when Wait() blocks due to rate limiting
The new `ReasonRateLimited` path still isn’t covered via `Wait()`. Current tests either don’t check the reason or don’t cause the behavior-analysis layer to block. Please add a `Wait`-side test (e.g., `TestLimiter_Wait_RateLimitedReason`) that:
1. Forces the analyzer to block for a given IP.
2. Calls `Wait(ctx, ua, ip)` for that IP.
3. Asserts the blocking error (e.g., `ErrLimit`) and `reason == ReasonRateLimited`.
This will complement the `Allow`-side coverage (`TestLimiter_Allow_BlacklistedIP`) and verify both paths return the correct reason when rate limiting occurs.
Suggested implementation:
```golang
if !allowed {
t.Error("normal user should be allowed")
}
}
func TestLimiter_Wait_RateLimitedReason(t *testing.T) {
// Construct a limiter instance using the same helper / pattern as other tests.
l := newLimiterForTest(t) // adjust to match existing test setup
defer l.Close()
ua := "Mozilla/5.0"
ip := "203.0.113.10"
// Drive the limiter into a rate-limited state for this IP via Allow(),
// mirroring real behavior-analysis behavior rather than touching internals.
var (
allowed bool
reason Reason
)
for i := 0; i < 1000; i++ { // upper bound chosen to exceed any reasonable threshold
allowed, reason = l.Allow(ua, ip)
if !allowed {
break
}
}
if allowed {
t.Fatalf("expected IP %s to become rate limited via Allow(), but it never did", ip)
}
if reason != ReasonRateLimited {
t.Fatalf("expected ReasonRateLimited from Allow() when IP %s is blocked, got %v", ip, reason)
}
ctx := context.Background()
err, waitReason := l.Wait(ctx, ua, ip)
if err != ErrLimit {
t.Fatalf("expected ErrLimit from Wait() for rate-limited IP %s, got %v", ip, err)
}
if waitReason != ReasonRateLimited {
t.Fatalf("expected ReasonRateLimited from Wait(); got %v", waitReason)
}
```
1. Replace `newLimiterForTest(t)` with the actual limiter-construction pattern used in the rest of `botrate_test.go` (e.g., a helper or direct call such as `NewLimiter(testConfig)`).
2. Ensure that the `Reason` type and `ReasonRateLimited` constant are imported / referenced correctly in this file (if they are defined in another package, prefix them appropriately, e.g., `botrate.ReasonRateLimited`).
3. If the existing tests already use a different user agent or IP pattern for triggering rate limiting, you may want to reuse those values instead of `"Mozilla/5.0"` / `"203.0.113.10"` to keep tests consistent.
4. If the limiter has a very high threshold and the loop bound `1000` is insufficient to trigger rate limiting in your configuration, either lower the configured threshold for tests or increase the loop bound so that the IP is reliably driven into the rate-limited state.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2 +/- ##
==========================================
- Coverage 76.41% 75.74% -0.68%
==========================================
Files 6 6
Lines 229 235 +6
==========================================
+ Hits 175 178 +3
- Misses 50 53 +3
Partials 4 4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary by Sourcery
Expose rate limit decisions with machine-readable reasons for blocked requests while keeping existing behavior for allowed traffic.
New Features:
Enhancements:
Build: