refactor: use cache store from tinyauth#29
Conversation
dosubot
Bot
added
the
size:L
📝 WalkthroughWalkthroughThe PR replaces a non-generic ChangesCache subsystem and rate limiter migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
rate_limiter.go (1)
32-39: 💤 Low valueBackground goroutine has no shutdown mechanism.
The ticker goroutine runs indefinitely with no way to stop it. In the current application (single
RateLimiterliving for the process lifetime), this is acceptable. However, for testability and potential reuse, consider adding a shutdown path via a context or stop channel.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rate_limiter.go` around lines 32 - 39, The background ticker goroutine in rate_limiter.go that calls rl.caches.ratelimit.Sweep() needs a shutdown path; add a stop mechanism on the RateLimiter (e.g., a stopCh chan struct{} field or accept a context) and initialize it when creating the RateLimiter (e.g., in NewRateLimiter), change the anonymous goroutine to select between ticker.C and the stop channel (or ctx.Done()) so it breaks and defers ticker.Stop(), and expose a Stop() (or Close(ctx)) method on RateLimiter that signals the channel (or cancels the context) to terminate the goroutine cleanly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rate_limiter.go`:
- Around line 96-103: The parsed X-Forwarded-For value can include spaces, so
trim whitespace from the extracted IP to avoid inconsistent keys: after
computing firstIp := strings.SplitN(xForwardedFor, ",", 2)[0] call firstIp =
strings.TrimSpace(firstIp) (or equivalent) before checking if firstIp != "" and
returning it; update references to xForwardedFor/firstIp in that block
accordingly.
- Around line 52-66: The rate counter is not persisted when a request pushes
current past rl.config.RateLimitCount so x-ratelimit-used becomes stale; inside
the rl.caches.ratelimit.WithLock block (the code that reads/sets current for
clientIP and assigns used), always call actions.Update(clientIP, current, 0)
after incrementing current — even when current > rl.config.RateLimitCount —
before returning, so the cache reflects the actual attempted count; keep the
same TTL behavior used for the allowed path.
---
Nitpick comments:
In `@rate_limiter.go`:
- Around line 32-39: The background ticker goroutine in rate_limiter.go that
calls rl.caches.ratelimit.Sweep() needs a shutdown path; add a stop mechanism on
the RateLimiter (e.g., a stopCh chan struct{} field or accept a context) and
initialize it when creating the RateLimiter (e.g., in NewRateLimiter), change
the anonymous goroutine to select between ticker.C and the stop channel (or
ctx.Done()) so it breaks and defers ticker.Stop(), and expose a Stop() (or
Close(ctx)) method on RateLimiter that signals the channel (or cancels the
context) to terminate the goroutine cleanly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dbccdee6-4c76-4d7b-8eda-77e381700982
📒 Files selected for processing (4)
cache.gocache_store.gomain.gorate_limiter.go
💤 Files with no reviewable changes (1)
- cache.go
| var used int | ||
| rl.caches.ratelimit.WithLock(func(actions CacheStoreActions[int]) { | ||
| current, exists := actions.Get(clientIP) | ||
| if !exists { | ||
| actions.Set(clientIP, 1, 12*time.Hour) | ||
| used = 1 | ||
| return | ||
| } | ||
| current++ | ||
| used = current | ||
| if current > rl.config.RateLimitCount { | ||
| return | ||
| } | ||
| actions.Update(clientIP, current, 0) | ||
| }) |
There was a problem hiding this comment.
Counter not updated when rate limit exceeded, causing inaccurate x-ratelimit-used header.
When current > RateLimitCount (line 62-64), the function returns without calling Update, so the cache remains at RateLimitCount. All subsequent blocked requests will show used = RateLimitCount + 1 instead of the actual attempt count.
Example with RateLimitCount=3:
- Requests 1-3: cache updated to 1, 2, 3 (allowed)
- Request 4: cache=3, increments to 4, blocked, no update → cache stays at 3
- Request 5+: cache still 3, increments to 4, blocked →
usedheader always shows 4
If accurate usage tracking matters, update the counter even when blocked:
🔧 Suggested fix
current++
used = current
+ actions.Update(clientIP, current, 0)
if current > rl.config.RateLimitCount {
return
}
- actions.Update(clientIP, current, 0)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var used int | |
| rl.caches.ratelimit.WithLock(func(actions CacheStoreActions[int]) { | |
| current, exists := actions.Get(clientIP) | |
| if !exists { | |
| actions.Set(clientIP, 1, 12*time.Hour) | |
| used = 1 | |
| return | |
| } | |
| current++ | |
| used = current | |
| if current > rl.config.RateLimitCount { | |
| return | |
| } | |
| actions.Update(clientIP, current, 0) | |
| }) | |
| var used int | |
| rl.caches.ratelimit.WithLock(func(actions CacheStoreActions[int]) { | |
| current, exists := actions.Get(clientIP) | |
| if !exists { | |
| actions.Set(clientIP, 1, 12*time.Hour) | |
| used = 1 | |
| return | |
| } | |
| current++ | |
| used = current | |
| actions.Update(clientIP, current, 0) | |
| if current > rl.config.RateLimitCount { | |
| return | |
| } | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rate_limiter.go` around lines 52 - 66, The rate counter is not persisted when
a request pushes current past rl.config.RateLimitCount so x-ratelimit-used
becomes stale; inside the rl.caches.ratelimit.WithLock block (the code that
reads/sets current for clientIP and assigns used), always call
actions.Update(clientIP, current, 0) after incrementing current — even when
current > rl.config.RateLimitCount — before returning, so the cache reflects the
actual attempted count; keep the same TTL behavior used for the allowed path.
| xForwardedFor := r.Header.Get("x-forwarded-for") | ||
|
|
||
| if len(xForwardedFor) > 0 { | ||
| return xForwardedFor[0] | ||
| if xForwardedFor != "" { | ||
| firstIp := strings.SplitN(xForwardedFor, ",", 2)[0] | ||
| if firstIp != "" { | ||
| return firstIp | ||
| } | ||
| } |
There was a problem hiding this comment.
Trim whitespace from parsed x-forwarded-for IP.
The X-Forwarded-For header value may contain spaces around commas (e.g., "1.2.3.4, 5.6.7.8"). The SplitN result could have trailing whitespace, which would create inconsistent cache keys for the same client IP.
🔧 Suggested fix
if xForwardedFor != "" {
firstIp := strings.SplitN(xForwardedFor, ",", 2)[0]
+ firstIp = strings.TrimSpace(firstIp)
if firstIp != "" {
return firstIp
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| xForwardedFor := r.Header.Get("x-forwarded-for") | |
| if len(xForwardedFor) > 0 { | |
| return xForwardedFor[0] | |
| if xForwardedFor != "" { | |
| firstIp := strings.SplitN(xForwardedFor, ",", 2)[0] | |
| if firstIp != "" { | |
| return firstIp | |
| } | |
| } | |
| xForwardedFor := r.Header.Get("x-forwarded-for") | |
| if xForwardedFor != "" { | |
| firstIp := strings.SplitN(xForwardedFor, ",", 2)[0] | |
| firstIp = strings.TrimSpace(firstIp) | |
| if firstIp != "" { | |
| return firstIp | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rate_limiter.go` around lines 96 - 103, The parsed X-Forwarded-For value can
include spaces, so trim whitespace from the extracted IP to avoid inconsistent
keys: after computing firstIp := strings.SplitN(xForwardedFor, ",", 2)[0] call
firstIp = strings.TrimSpace(firstIp) (or equivalent) before checking if firstIp
!= "" and returning it; update references to xForwardedFor/firstIp in that block
accordingly.
1 participant
Summary by CodeRabbit
x-ratelimit-limit,x-ratelimit-used, andx-ratelimit-remainingcounts