fix: make standalone ratelimiting namespace aware#3819
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
📝 WalkthroughWalkthroughHandler now prefixes the rate-limit lookup key with the namespace ID by concatenating namespace.ID and the incoming request identifier; no other fields, control flow, or exported signatures were changed. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler
participant NamespaceSvc as Namespace
participant RateLimiter
Client->>Handler: v2 rate-limit request (req.Identifier)
Handler->>Namespace: resolve namespace (namespace.ID)
Namespace-->>Handler: namespace.ID
Note right of Handler: compose key = namespace.ID + ":" + req.Identifier
Handler->>RateLimiter: lookup/check using composite key
RateLimiter-->>Handler: limit result
Handler-->>Client: response
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Flo4604
changed the title
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
go/apps/api/routes/v2_ratelimit_limit/handler.go (3)
184-189: Nit: prefer simple concatenation over fmt.Sprintf and document rationale.Sprintf allocates; in a hot path like ratelimiting, a simple concat is cheaper and clearer. Also, add a short AIDEV-RATIONALE comment to prevent regressions.
Apply this diff:
- Identifier: fmt.Sprintf("%s:%s", namespace.ID, req.Identifier), + // AIDEV-RATIONALE: Include namespace ID to ensure per-namespace isolation of standalone buckets. + Identifier: namespace.ID + ":" + req.Identifier,
241-259: Overrides remain keyed by raw identifier — add tests to lock intent.Because overrides are stored per-namespace and matched against the unscoped identifier, behavior remains correct. Add tests asserting:
- Same identifier hits different limits in two namespaces with different overrides.
- Wildcard override in one namespace does not affect another.
I can help scaffold these if useful.
183-189: Rollout note: key format change can cause split-buckets during rolling deploys.Mixed-version nodes will write to different bucket keys (“identifier-…” vs “nsID:identifier-…”), temporarily doubling capacity. If this endpoint is served by multiple instances, consider:
- Fast/atomic deploy or brief maintenance window, or
- A short-lived feature flag (e.g., UNKEY_API_RATELIMIT_NAMESPACE_SCOPED) to flip on simultaneously, or
- Accepting transient leniency given bucket TTL equals duration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
go/apps/api/routes/v2_ratelimit_limit/handler.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Follow comprehensive documentation guidelines for Go code as described in go/GO_DOCUMENTATION_GUIDELINES.md
Every public function/type in Go code must be documented
Prefer interfaces for testability in Go code
Use AIDEV-* comments for complex/important code in Go services
Files:
go/apps/api/routes/v2_ratelimit_limit/handler.go
**/*.{env,js,ts,go}
📄 CodeRabbit inference engine (CLAUDE.md)
All environment variables must follow the format: UNKEY_<SERVICE_NAME>_VARNAME
Files:
go/apps/api/routes/v2_ratelimit_limit/handler.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build / Build
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test API / API Test Local
🔇 Additional comments (2)
go/apps/api/routes/v2_ratelimit_limit/handler.go (2)
184-189: Namespace-scoped key eliminates cross-namespace bucket reuse (correct).Scoping the ratelimit Identifier with namespace.ID is the right fix and uses an immutable identifier rather than the namespace name. This directly prevents bucket collisions when identifier/limit/duration tuples are identical across namespaces and carries no API surface change.
210-219: Verify namespace-aware grouping for ratelimit metricsI’ve confirmed that the
RatelimitRequestV1struct and buffer call include bothNamespaceIDandIdentifier, and the raw ClickHouse table has corresponding columns, but you’ll still need to audit every downstream query and dashboard for proper grouping:
- In
go/pkg/clickhouse/schema/requests.go, the struct has both fields defined:type RatelimitRequestV1 struct { … NamespaceID string `ch:"namespace_id" json:"namespace_id"` Identifier string `ch:"identifier" json:"identifier"` … }- The handler (
handler.go:210–219) buffers with bothNamespaceID: namespace.IDandIdentifier: req.Identifier.- Tests in
v2_ratelimit_limit/200_test.goexplicitly select withWHERE workspace_id = … AND namespace_id = …, ensuring namespace filtering in smoke tests.- The ClickHouse client writes straight to
ratelimits.raw_ratelimits_v1with no transformation beyond buffering those fields.Next steps:
- Manually audit any ETL pipelines, materialized views, or SQL dashboards to ensure they always include both
namespace_idandidentifier(or otherwise partition by namespace) when aggregating.- Verify that no reporting queries group solely on
identifier, which would cause cross-namespace metric contamination.
ogzhanolguncu
left a comment
ogzhanolguncu
left a comment
There was a problem hiding this comment.
I tested it against main first and it actually worked as you described, different namespaces used the same quota. Then, switched to your branch everything worked as expected. LFG!
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
go/apps/api/routes/v2_ratelimit_limit/handler.go (1)
71-73: Optional: fail fast on missing namespace input.Currently an empty
req.Namespacefalls through to a DB lookup and later returns NotFound. Consider an explicit validation error (400) for better UX and cheaper path.I can draft a small patch with your preferred fault code for invalid arguments if you share the canonical code constant.
♻️ Duplicate comments (1)
go/apps/api/routes/v2_ratelimit_limit/handler.go (1)
3-25: Previous nit resolved: no unused fmt import.The earlier comment about removing an unused
fmtimport no longer applies; the import is absent now.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
go/apps/api/routes/v2_ratelimit_limit/handler.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Follow comprehensive documentation guidelines for Go code as described in go/GO_DOCUMENTATION_GUIDELINES.md
Every public function/type in Go code must be documented
Prefer interfaces for testability in Go code
Use AIDEV-* comments for complex/important code in Go services
Files:
go/apps/api/routes/v2_ratelimit_limit/handler.go
**/*.{env,js,ts,go}
📄 CodeRabbit inference engine (CLAUDE.md)
All environment variables must follow the format: UNKEY_<SERVICE_NAME>_VARNAME
Files:
go/apps/api/routes/v2_ratelimit_limit/handler.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
- GitHub Check: Test Go API Local / Test
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
go/apps/api/routes/v2_ratelimit_limit/handler.go (3)
183-183: LGTM: Namespacing the identifier fixes cross-namespace bucket collisions.Prefixing the identifier with
namespace.IDaligns the bucket key with namespace scope and matches the PR objective. No regressions apparent in overrides, permissions, or metrics emission.
209-218: Confirm intended metrics semantics: raw identifier vs namespaced identifier.Telemetry buffers
Identifier: req.Identifierwhile scoping is added only when calling the ratelimiter. That’s likely fine sinceNamespaceIDis recorded separately, but double-check any consumers that group by “Identifier” alone—those dashboards could mix namespaces.If grouping-by-identifier alone exists, consider emitting both fields or updating consumers to group by
(NamespaceID, Identifier).
181-188: Confirm namespace prefix on all RatelimitRequest constructionsPlease review the following call sites and ensure that their
Identifiervalues include the propernamespace.IDprefix (or are otherwise guaranteed to be namespaced upstream):• go/internal/services/keys/validation.go:196
response, err := k.rateLimiter.Ratelimit(ctx, ratelimit.RatelimitRequest{ Identifier: config.Identifier, // verify whether config.Identifier already includes namespace.ID })• apps/agent/pkg/api/routes/v1_ratelimit_multiRatelimit/handler.go:29
ratelimits[i] = &ratelimitv1.RatelimitRequest{ Identifier: r.Identifier, // needs namespace.ID + ":" + r.Identifier? }• apps/agent/pkg/api/routes/v1_ratelimit_ratelimit/handler.go:38
res, err := svc.Ratelimit.Ratelimit(ctx, &ratelimitv1.RatelimitRequest{ Identifier: req.Identifier, // should this be prefixed? })Also confirm that internal bucket-key generation (
bucketKey{req.Identifier,…}and itstoString()) only uses the identifier as supplied, to avoid double-prefixing.
chronark
left a comment
chronark
left a comment
There was a problem hiding this comment.
A different duration would also result in a new counter, cause the duration is used to determine the window sequence.
So in a way it is redundant anyways
I don't have a good reason for having the limit in the key. My thinking was that if I edit it, a reset would be fine.
|
|
Graphite Automations"Post a GIF when PR approved" took an action on this PR • (08/21/25)1 gif was posted to this PR based on Andreas Thomas's automation. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
3 participants
What does this PR do?
This fixes a case where the same rate-limit bucket could be used, even though it shouldn't.
A bucket key is a "unique" key of identifier-limit-duration
If I have namespace
Aand namespaceB, and I rate-limit the identifiertest-123, this works fine because in 99% of all cases, I will have different durations/limits set for them. Obviously, this is not 100% guaranteed.This becomes more likely with rate-limit overrides.
If you have two namespaces with different rate limits but you override the same identifier to have a new limit and duration that are the same in both namespaces, it will be the same limit, even though it's a different action.
EDIT:
After thinking about it, why do we consider the duration and limit for a unique bucket?
If I change the rate limit, wouldn't it make sense to keep whatever usage we have already recorded?
For example, I have a "duration": 300000 and a limit of 15.
Now, if I change the limit to 10, this is a completely new limit, yet I still have 9 remaining, even though I could have exceeded the limit before.
Just as food for thought.
Example:
{ "data": { "limit": 15, "remaining": 14, "reset": 1755762600000, "success": true }, "meta": { "requestId": "req_4dze2QH3xbY3G6qB" } }{ "data": { "limit": 15, "remaining": 13, "reset": 1755762600000, "success": true }, "meta": { "requestId": "req_4dze6HdXWTurR3Gx" } }As you can see in this example the second request started of at remaining 13 instead of 14, which it should have been instead.
Type of change
How should this be tested?
Use the standalone ratelimiting API with different namespaces to see that the same identifier-limit-duration combination is treated as a different limit per namespace and not one globally.
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit