feat: add rate limiting interceptor with pluggable limiter#39
Merged
Conversation
Built-in token bucket rate limiter using go-grpc-middleware v2 ratelimit and golang.org/x/time/rate. Disabled by default (rate.Inf). Features: - SetDefaultRateLimit(rps, burst) for per-pod token bucket - SetRateLimiter() for custom implementations (Redis, per-tenant, etc.) - SetDisableRateLimit() to disable entirely - Both unary and stream interceptors - Positioned after timeout, before logging in the chain This is a per-pod in-memory limit. With N pods, effective cluster-wide limit is N × configured rate.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds server-side gRPC rate limiting: new Changes
Sequence DiagramsequenceDiagram
autonumber
actor Client
participant Server as gRPC Server
participant Interceptor as RateLimit<br/>Interceptor
participant Limiter as TokenBucket<br/>Limiter
participant Handler as Request<br/>Handler
Client->>Server: gRPC request
Server->>Interceptor: invoke interceptor chain
Interceptor->>Limiter: Allow()
alt Tokens available
Limiter-->>Interceptor: allowed
Interceptor->>Handler: proceed to handler
Handler-->>Client: response
else Rate limit exceeded
Limiter-->>Interceptor: error "rate limit exceeded"
Interceptor-->>Client: ResourceExhausted error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
3 tasks
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an opt-in gRPC rate limiting interceptor to the ColdBrew default interceptor chains, with support for either a built-in token-bucket limiter or a user-provided limiter implementation.
Changes:
- Introduce rate-limit configuration globals and initialization-time setters (
SetDefaultRateLimit,SetRateLimiter,SetDisableRateLimit). - Insert unary + stream rate limiting into the default server interceptor chains (unary placed after timeout, before logging).
- Add unit tests for unary rate limiting behavior and update module deps for
golang.org/x/time/rate.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| interceptors.go | Adds limiter config API, token-bucket limiter, and inserts rate limit interceptors into default unary/stream chains. |
| interceptors_test.go | Adds unary rate limit tests and resets new globals in test setup. |
| go.mod | Adds direct dependency on golang.org/x/time. |
| go.sum | Adds checksums for golang.org/x/time. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
interceptors_test.go (1)
1320-1437: Add a stream-path regression test.These cases validate the unary limiter behavior, but the PR also wires rate limiting into
DefaultStreamInterceptors(). A small stream allow/exceed test would cover the new production branch and catch wiring/order regressions that this suite currently misses.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interceptors_test.go` around lines 1320 - 1437, Add a stream-path regression test that mirrors the unary tests: create TestRateLimitInterceptor_StreamAllowedAndExceeded which calls SetDefaultRateLimit(...) and obtains limiter via getRateLimiter(), then builds a grpc.StreamServerInfo{FullMethod: "/test/RateLimitStream"} and a minimal fakeServerStream implementing grpc.ServerStream (Context, RecvMsg, SendMsg) to use as the stream argument; get the interceptor via ratelimit_middleware.StreamServerInterceptor(limiter) and invoke it once expecting success and a second time expecting a ResourceExhausted error. Also add a small TestRateLimitInterceptor_StreamDisabled that calls SetDisableRateLimit(true) and runs DefaultStreamInterceptors() over the fakeServerStream to assert no ResourceExhausted is returned. Ensure tests reference getRateLimiter, SetDefaultRateLimit, SetDisableRateLimit, ratelimit_middleware.StreamServerInterceptor, and DefaultStreamInterceptors so the stream branch is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@interceptors.go`:
- Around line 900-903: SetDefaultRateLimit currently allows a finite rps with
burst < 1 which results in a deny-all limiter; add input validation in
SetDefaultRateLimit to detect finite rps (use math.IsInf/math.IsNaN on the rps
value) and if rps is finite and burst < 1, clamp burst to 1 (or set
defaultRateBurst = 1) before assigning defaultRateLimit = rate.Limit(rps) and
defaultRateBurst; reference SetDefaultRateLimit, defaultRateLimit, and
defaultRateBurst when making the change so the limiter is never configured with
a zero burst for finite rates.
---
Nitpick comments:
In `@interceptors_test.go`:
- Around line 1320-1437: Add a stream-path regression test that mirrors the
unary tests: create TestRateLimitInterceptor_StreamAllowedAndExceeded which
calls SetDefaultRateLimit(...) and obtains limiter via getRateLimiter(), then
builds a grpc.StreamServerInfo{FullMethod: "/test/RateLimitStream"} and a
minimal fakeServerStream implementing grpc.ServerStream (Context, RecvMsg,
SendMsg) to use as the stream argument; get the interceptor via
ratelimit_middleware.StreamServerInterceptor(limiter) and invoke it once
expecting success and a second time expecting a ResourceExhausted error. Also
add a small TestRateLimitInterceptor_StreamDisabled that calls
SetDisableRateLimit(true) and runs DefaultStreamInterceptors() over the
fakeServerStream to assert no ResourceExhausted is returned. Ensure tests
reference getRateLimiter, SetDefaultRateLimit, SetDisableRateLimit,
ratelimit_middleware.StreamServerInterceptor, and DefaultStreamInterceptors so
the stream branch is exercised.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 59ba9693-3b28-416b-bc79-0b75a8123c29
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.modinterceptors.gointerceptors_test.go
Address review: - Fix gofumpt import ordering (golang.org/x after github.com) - Bump golang.org/x/time v0.3.0 → v0.15.0 - Clamp burst to min 1 in SetDefaultRateLimit (burst=0 rejects all) - Remove dead var _ = rate.Inf from tests
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This was referenced Apr 11, 2026
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
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
ratelimitinterceptor (already a dependency) +golang.org/x/time/raterate.Inf) — no breaking changes, opt-in onlySetRateLimiter()with a custom implementationConfig API:
SetDefaultRateLimit(rps, burst)— configure token bucketSetRateLimiter(limiter)— plug in custom implementationSetDisableRateLimit(true)— disable entirelyTest plan
TestRateLimitInterceptor_DefaultInf— default returns nil limiterTestRateLimitInterceptor_Allowed— 1000 rps, request passesTestRateLimitInterceptor_Exceeded— 1 rps burst 1, second request getsResourceExhaustedTestRateLimitInterceptor_CustomLimiter— custom always-reject limiter worksTestRateLimitInterceptor_Disabled— disabled via config, no rate limitingmake testandmake lintcleanSummary by CodeRabbit
New Features
Tests
Chores