Add a token-bucket rate limiter - #4
Conversation
Allows up to burst events, refilling one token per interval.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
|
@coderabbitai review |
|
@macroscopeapp review |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| if l.tokens > l.burst { | ||
| l.tokens = l.burst | ||
| } | ||
| l.last = time.Now() |
There was a problem hiding this comment.
🟡 Medium rate/limiter.go:28
Allow loses the elapsed remainder after a refill, so an empty limiter checked 15 seconds into a 10-second interval refills once but cannot refill again until 25 seconds instead of 20 seconds. Advance last by refill * l.interval rather than resetting it to the current time to preserve the configured cadence.
| l.last = time.Now() | |
| l.last = l.last.Add(time.Duration(refill) * l.interval) |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @internal/rate/limiter.go around line 28:
`Allow` loses the elapsed remainder after a refill, so an empty limiter checked 15 seconds into a 10-second interval refills once but cannot refill again until 25 seconds instead of 20 seconds. Advance `last` by `refill * l.interval` rather than resetting it to the current time to preserve the configured cadence.
| l.last = time.Now() | ||
| } | ||
|
|
||
| if l.tokens == 0 { |
There was a problem hiding this comment.
🟠 High rate/limiter.go:31
Concurrent calls to Allow can both return true when only one token remains, exceeding the configured limit and causing a Go data race. The tokens check/decrement and last update are unsynchronized; protect the limiter state with a mutex around the refill and consume operation.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @internal/rate/limiter.go around line 31:
Concurrent calls to `Allow` can both return `true` when only one token remains, exceeding the configured limit and causing a Go data race. The `tokens` check/decrement and `last` update are unsynchronized; protect the limiter state with a mutex around the refill and consume operation.
| refill := int(elapsed / l.interval) | ||
| if refill > 0 { | ||
| l.tokens += refill | ||
| if l.tokens > l.burst { | ||
| l.tokens = l.burst | ||
| } |
There was a problem hiding this comment.
🟡 Medium rate/limiter.go:22
On 32-bit Go targets, an exhausted limiter remains empty after more than about 2.15 seconds of idle time when interval is 1 ns, even though tokens should have refilled. int(elapsed / l.interval) overflows to a negative value before refill > 0 is checked, so the refill is skipped; keep the quotient in time.Duration and cap it at the remaining burst capacity before converting to int.
-\trefill := int(elapsed / l.interval)
+\trefill := elapsed / l.interval
\tif refill > 0 {
-\t\tl.tokens += refill
-\t\tif l.tokens > l.burst {
+\t\tif refill >= time.Duration(l.burst-l.tokens) {
\t\t\tl.tokens = l.burst
+\t\t} else {
+\t\t\tl.tokens += int(refill)
\t\t}
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @internal/rate/limiter.go around lines 22-27:
On 32-bit Go targets, an exhausted limiter remains empty after more than about 2.15 seconds of idle time when `interval` is 1 ns, even though tokens should have refilled. `int(elapsed / l.interval)` overflows to a negative value before `refill > 0` is checked, so the refill is skipped; keep the quotient in `time.Duration` and cap it at the remaining `burst` capacity before converting to `int`.
| if refill > 0 { | ||
| l.tokens += refill | ||
| if l.tokens > l.burst { | ||
| l.tokens = l.burst | ||
| } |
There was a problem hiding this comment.
🟠 High rate/limiter.go:23
With burst == math.MaxInt, a refill overflows l.tokens before the capacity clamp: math.MaxInt + 1 becomes negative, bypasses the l.tokens == 0 check, and the subsequent decrement wraps it back to a positive value. The bucket therefore never depletes correctly; clamp the refill against the remaining capacity before adding it.
if refill > 0 {
- l.tokens += refill
- if l.tokens > l.burst {
- l.tokens = l.burst
- }
+ if refill >= l.burst-l.tokens {
+ l.tokens = l.burst
+ } else {
+ l.tokens += refill
+ }🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @internal/rate/limiter.go around lines 23-27:
With `burst == math.MaxInt`, a refill overflows `l.tokens` before the capacity clamp: `math.MaxInt + 1` becomes negative, bypasses the `l.tokens == 0` check, and the subsequent decrement wraps it back to a positive value. The bucket therefore never depletes correctly; clamp the refill against the remaining capacity before adding it.
| // Allow reports whether an event may proceed, consuming a token if so. | ||
| func (l *Limiter) Allow() bool { | ||
| elapsed := time.Since(l.last) | ||
| refill := int(elapsed / l.interval) |
There was a problem hiding this comment.
🟠 High rate/limiter.go:22
Invalid inputs to New produce incorrect limiter behavior: New(0, burst).Allow() panics on elapsed / l.interval, New(time.Hour, -1).Allow() returns true indefinitely, and a negative interval permanently rejects events after the initial tokens are spent. Validate that interval > 0 and burst >= 0 in New before constructing the limiter.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @internal/rate/limiter.go around line 22:
Invalid inputs to `New` produce incorrect limiter behavior: `New(0, burst).Allow()` panics on `elapsed / l.interval`, `New(time.Hour, -1).Allow()` returns `true` indefinitely, and a negative interval permanently rejects events after the initial tokens are spent. Validate that `interval > 0` and `burst >= 0` in `New` before constructing the limiter.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee1c38e671
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| // Allow reports whether an event may proceed, consuming a token if so. | ||
| func (l *Limiter) Allow() bool { | ||
| elapsed := time.Since(l.last) |
There was a problem hiding this comment.
When a limiter is shared by concurrent request handlers, Allow reads and mutates last and tokens without synchronization. Two callers can both observe the final token and proceed; an interleaving can also decrement tokens below zero, after which the tokens == 0 check continues allowing requests. Protect the state transition with a mutex or another atomic design.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Merged into Allow mutates limiter state without synchronization (internal/rate/limiter.go:31), also reported by macroscopeapp, chatgpt-codex-connector.
Resolving this thread as a duplicate — the finding itself is not dismissed, and reopening this thread is the right move if the merge is wrong.
See the consolidated review: #4 (comment)
| if l.tokens > l.burst { | ||
| l.tokens = l.burst | ||
| } | ||
| l.last = time.Now() |
There was a problem hiding this comment.
When the bucket is still below capacity after a refill, resetting last to the current time discards the fractional interval already elapsed. For example, after exhausting a burst of two, a call at 1.5 intervals consumes the first refill, but the next token is delayed until 2.5 intervals rather than arriving at 2 intervals. Advance last by refill * interval instead, except where time above the full-bucket cap is intentionally discarded.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Merged into Resetting last to now discards the elapsed remainder, slowing refill (internal/rate/limiter.go:28), also reported by macroscopeapp, chatgpt-codex-connector, copilot-pull-request-reviewer.
Resolving this thread as a duplicate — the finding itself is not dismissed, and reopening this thread is the right move if the merge is wrong.
See the consolidated review: #4 (comment)
| func New(interval time.Duration, burst int) *Limiter { | ||
| return &Limiter{interval: interval, burst: burst, tokens: burst, last: time.Now()} |
There was a problem hiding this comment.
If interval is zero—such as from an omitted duration in caller configuration—New succeeds but the first call to Allow panics on elapsed / l.interval. Validate that the interval is positive in the constructor, or return an error, so invalid input cannot create a limiter that crashes on use.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Merged into New accepts a non-positive interval; Allow then panics or never refills (internal/rate/limiter.go:16), also reported by macroscopeapp, copilot-pull-request-reviewer, chatgpt-codex-connector.
Resolving this thread as a duplicate — the finding itself is not dismissed, and reopening this thread is the right move if the merge is wrong.
See the consolidated review: #4 (comment)
| // Allow reports whether an event may proceed, consuming a token if so. | ||
| func (l *Limiter) Allow() bool { | ||
| elapsed := time.Since(l.last) | ||
| refill := int(elapsed / l.interval) |
There was a problem hiding this comment.
On 32-bit targets, converting the elapsed-interval count to int can overflow during ordinary uptime for sufficiently short intervals—for example, a 1µs interval exceeds MaxInt32 after about 36 minutes. The wrapped value can be nonpositive, causing an exhausted limiter to skip refilling even though tokens are due. Keep the quotient as int64 and clamp it to the available bucket capacity before converting or adding it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Merged into Narrowing the refill quotient to int can skip refills on 32-bit targets (internal/rate/limiter.go:22), also reported by macroscopeapp, chatgpt-codex-connector.
Resolving this thread as a duplicate — the finding itself is not dismissed, and reopening this thread is the right move if the merge is wrong.
See the consolidated review: #4 (comment)
| func New(interval time.Duration, burst int) *Limiter { | ||
| return &Limiter{interval: interval, burst: burst, tokens: burst, last: time.Now()} |
There was a problem hiding this comment.
When a negative burst value reaches New, tokens starts below zero and the equality-only exhaustion check never rejects a request; every call instead decrements the value further and returns true, disabling the rate limit entirely. Validate that burst is nonnegative in the constructor, or otherwise ensure nonpositive token counts are refused.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Merged into A negative burst disables rate limiting entirely (internal/rate/limiter.go:31), also reported by macroscopeapp, chatgpt-codex-connector.
Resolving this thread as a duplicate — the finding itself is not dismissed, and reopening this thread is the right move if the merge is wrong.
See the consolidated review: #4 (comment)
There was a problem hiding this comment.
Pull request overview
Adds a new internal rate.Limiter implementing a token-bucket rate limiter (burst capacity with periodic refills) and basic tests validating burst allowance and exhaustion.
Changes:
- Introduces
internal/rate.LimiterwithNew(interval, burst)and(*Limiter).Allow(). - Implements time-based token refilling and token consumption logic.
- Adds initial unit tests for burst acceptance and refusal after exhaustion.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| internal/rate/limiter.go | Adds the limiter implementation (constructor + Allow refill/consume logic). |
| internal/rate/limiter_test.go | Adds unit tests for burst allowance and exhaustion behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // New builds a limiter that refills one token per interval, up to burst. | ||
| func New(interval time.Duration, burst int) *Limiter { | ||
| return &Limiter{interval: interval, burst: burst, tokens: burst, last: time.Now()} | ||
| } |
There was a problem hiding this comment.
Merged into New accepts a non-positive interval; Allow then panics or never refills (internal/rate/limiter.go:16), also reported by macroscopeapp, copilot-pull-request-reviewer, chatgpt-codex-connector.
Resolving this thread as a duplicate — the finding itself is not dismissed, and reopening this thread is the right move if the merge is wrong.
See the consolidated review: #4 (comment)
| if l.tokens > l.burst { | ||
| l.tokens = l.burst | ||
| } | ||
| l.last = time.Now() |
There was a problem hiding this comment.
Merged into Resetting last to now discards the elapsed remainder, slowing refill (internal/rate/limiter.go:28), also reported by macroscopeapp, chatgpt-codex-connector, copilot-pull-request-reviewer.
Resolving this thread as a duplicate — the finding itself is not dismissed, and reopening this thread is the right move if the merge is wrong.
See the consolidated review: #4 (comment)
| func TestExhaustedBurstIsRefused(t *testing.T) { | ||
| l := New(time.Hour, 1) | ||
| l.Allow() | ||
|
|
||
| if l.Allow() { | ||
| t.Error("a second event should be refused") | ||
| } | ||
| } |
Review reconciled — 7 findings from 3 reviewersFindingsMAJOR — Allow mutates limiter state without synchronization · MAJOR — New accepts a non-positive interval; Allow then panics or never refills · MAJOR — A negative burst disables rate limiting entirely · MINOR — Resetting last to now discards the elapsed remainder, slowing refill · MINOR — Narrowing the refill quotient to int can skip refills on 32-bit targets · MINOR — l.tokens += refill can overflow before the burst clamp · MINOR — No test exercises the refill path · CoverageAbsent: coderabbitai, baz-reviewer — this verdict is incomplete, not clean. Verdict: incomplete |
User description
Allows up to
burstevents, refilling one token perinterval.Allowconsumes a token when one is available and reports whether the caller may proceed.Generated description
Below is a concise technical summary of the changes proposed in this PR:
Add a token-bucket
Limiterthat allows an initial burst and refills one token per interval. ExposeNewandAllowto control event admission, with tests covering burst acceptance and exhaustion.NewandAllow.Modified files (1)
Latest Contributors(1)
Modified files (1)
Latest Contributors(1)
Summary by cubic
Adds a token-bucket rate limiter in
internal/ratethat allows up toburstevents, refilling one token perinterval.Written for commit ee1c38e. Summary will update on new commits.
Note
Add 'rate.Limiter' token-bucket type with 'New' and 'Allow'
rate.Limiterstruct implementing a token-bucket algorithm in limiter.go.rate.Newconstructor initializes the limiter with a refill interval and burst capacity, starting prefilled with tokens.rate.Limiter.Allowmethod refills tokens based on elapsed time, caps at burst, and consumes one token to allow events or denies when empty.📊 Macroscope summarized ee1c38e. 1 file reviewed, 7 issues evaluated, 0 issues filtered, 5 comments posted
🗂️ Filtered Issues