Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
📝 WalkthroughWalkthroughAdds a Changes
Sequence DiagramsequenceDiagram
participant Handler as Handler\n(v2_ratelimit_limit)
participant Validation as Validation\n(keys/validation.go)
participant Service as Ratelimit Service\n(service.go)
participant Bucket as Bucket Store\n(bucket.go)
Handler->>Validation: construct ratelimit request\n(Name = namespace.ID, Identifier)
Validation->>Service: send Ratelimit(s) with Name field
Service->>Service: validate req.Name != ""\nconstruct bucketKey {Name, Identifier, Limit, Duration}
Service->>Bucket: getOrCreateBucket(key)
Bucket->>Bucket: create/lookup bucket using Name as part of key
Bucket->>Service: return counters/result
Service->>Validation: map results back to configs
Validation->>Handler: return aggregated response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (4)📓 Common learnings📚 Learning: 2025-08-29T13:48:43.353ZApplied to files:
📚 Learning: 2025-08-21T15:54:45.198ZApplied to files:
📚 Learning: 2025-08-19T09:42:40.919ZApplied to files:
⏰ 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). (1)
🔇 Additional comments (2)
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 |
|
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 (1)
go/internal/services/ratelimit/service.go (1)
171-177: Add Name validation to RatelimitMany for consistency.The
Ratelimitmethod validates thatreq.Nameis not empty (line 277), butRatelimitManydoes not include this validation. Since both methods construct bucket keys usingreq.Name, this inconsistency could allow empty names inRatelimitMany, potentially causing incorrect bucket key generation.Apply this diff to add the missing validation:
err := assert.All( + assert.NotEmpty(reqs[i].Name, "ratelimit name must not be empty"), assert.NotEmpty(reqs[i].Identifier, "ratelimit identifier must not be empty"), assert.Greater(reqs[i].Limit, 0, "ratelimit limit must be greater than zero"), assert.GreaterOrEqual(reqs[i].Cost, 0, "ratelimit cost must not be negative"), assert.GreaterOrEqual(reqs[i].Duration.Milliseconds(), 1000, "ratelimit duration must be at least 1s"), assert.False(reqs[i].Time.IsZero(), "request time must not be zero"), )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
go/apps/api/routes/v2_keys_verify_key/ratelimit_response_test.go(1 hunks)go/apps/api/routes/v2_ratelimit_limit/handler.go(1 hunks)go/internal/services/keys/validation.go(1 hunks)go/internal/services/ratelimit/bucket.go(5 hunks)go/internal/services/ratelimit/interface.go(1 hunks)go/internal/services/ratelimit/replay.go(1 hunks)go/internal/services/ratelimit/service.go(3 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4190
File: go/internal/services/keys/verifier.go:51-53
Timestamp: 2025-10-30T15:10:52.743Z
Learning: PR #4190 for unkeyed/unkey is focused solely on database schema and query changes for identity-based credits. It adds IdentityCredits and KeyCredits fields to structs and queries, but does not implement the priority enforcement logic in the usagelimiter. The logic implementation is intentionally deferred to a later PR in the stack.
📚 Learning: 2025-08-29T13:48:43.353Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3884
File: go/apps/api/routes/v2_ratelimit_delete_override/handler.go:218-228
Timestamp: 2025-08-29T13:48:43.353Z
Learning: In the unkeyed/unkey codebase, when working with ratelimit namespace caching, req.Namespace parameter is either the namespace ID or the namespace name, so cache invalidation only needs to remove cache keys for namespace.ID and namespace.Name - no need to also remove req.Namespace as a separate key.
Applied to files:
go/internal/services/ratelimit/replay.gogo/apps/api/routes/v2_ratelimit_limit/handler.gogo/internal/services/keys/validation.gogo/internal/services/ratelimit/service.go
📚 Learning: 2025-08-21T15:54:45.198Z
Learnt from: chronark
Repo: unkeyed/unkey PR: 3825
File: go/internal/services/usagelimiter/limit.go:38-0
Timestamp: 2025-08-21T15:54:45.198Z
Learning: In go/internal/services/usagelimiter/limit.go, the UpdateKeyCreditsDecrement operation cannot be safely wrapped with db.WithRetry due to the lack of idempotency mechanisms in the current tech stack. Retrying this non-idempotent write operation risks double-charging users if the first attempt commits but the client sees a transient error.
Applied to files:
go/internal/services/ratelimit/replay.gogo/internal/services/keys/validation.gogo/internal/services/ratelimit/service.go
📚 Learning: 2025-10-21T09:45:47.560Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 4107
File: go/pkg/clickhouse/key_verifications_test.go:20-20
Timestamp: 2025-10-21T09:45:47.560Z
Learning: In ClickHouse tests (e.g., go/pkg/clickhouse/key_verifications_test.go), parallel execution with t.Parallel() is safe when tests use workspaceID-based isolation. Each test generates a unique workspaceID (via uid.New(uid.WorkspacePrefix)), and all database operations are scoped to that workspaceID, providing logical isolation even when tests share a single ClickHouse instance.
Applied to files:
go/apps/api/routes/v2_keys_verify_key/ratelimit_response_test.go
📚 Learning: 2025-08-19T09:42:40.919Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3800
File: go/internal/services/keys/validation.go:45-52
Timestamp: 2025-08-19T09:42:40.919Z
Learning: In go/internal/services/keys/validation.go, keys with unlimited usage (RemainingRequests.Valid = false) have an early return that bypasses the usage limiter entirely. The usage limiter is only called for keys with finite remaining request counts.
Applied to files:
go/internal/services/keys/validation.go
📚 Learning: 2024-10-20T07:05:55.471Z
Learnt from: chronark
Repo: unkeyed/unkey PR: 2294
File: apps/api/src/pkg/keys/service.ts:268-271
Timestamp: 2024-10-20T07:05:55.471Z
Learning: In `apps/api/src/pkg/keys/service.ts`, `ratelimitAsync` is a table relation, not a column selection. When querying, ensure that table relations are included appropriately, not as columns.
Applied to files:
go/internal/services/keys/validation.go
🧬 Code graph analysis (1)
go/apps/api/routes/v2_keys_verify_key/ratelimit_response_test.go (3)
go/pkg/testutil/seed/seed.go (2)
CreateIdentityRequest(368-373)CreateRatelimitRequest(310-318)go/apps/api/openapi/gen.go (1)
VerifyKeyRatelimitData(2047-2072)go/pkg/testutil/http.go (1)
CallRoute(385-419)
⏰ 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 (10)
go/internal/services/ratelimit/replay.go (1)
62-67: LGTM! Name field correctly propagated to bucket key.The addition of
name: req.Nameensures that replay requests use the same expanded bucket key structure as the main rate limiting paths, maintaining consistency across the cluster.go/apps/api/routes/v2_keys_verify_key/ratelimit_response_test.go (1)
253-335: LGTM! Comprehensive test validates name-based isolation.This test correctly verifies that rate limits with identical configurations but different names maintain independent counters. The test confirms:
- Both
api_requestsanddata_accesslimits are auto-applied and checked on each request- Each maintains its own counter (validates the bug fix)
- Both decrement independently through 5 requests
- Both exceed on the 6th request as expected
The test effectively validates the core objective of this PR.
go/internal/services/keys/validation.go (1)
188-197: LGTM! Name field properly propagated in validation layer.The addition of
Name: config.Nameensures that rate limit requests constructed during key validation include the rate limit name, enabling downstream services to create properly isolated buckets.go/internal/services/ratelimit/service.go (2)
186-192: LGTM! Bucket key construction includes Name.The bucket key now correctly includes
req.Nameas the first component, ensuring rate limits are properly isolated by name.
275-288: LGTM! Name validation and bucket key construction are correct.The validation ensures
req.Nameis not empty, and the bucket key construction properly includes it for isolation.go/internal/services/ratelimit/bucket.go (4)
34-35: LGTM! Name field added to bucket struct.The addition of the
namefield to the bucket enables proper isolation of rate limits by name.
57-64: LGTM! Bucket key includes name.The
key()method correctly includes the bucket's name when constructing thebucketKey.
83-98: LGTM! BucketKey structure and serialization updated correctly.The
bucketKeystruct now includes thenamefield, and thetoString()method properly includes it as the first component in the key string, ensuring unique identification.
120-128: LGTM! Bucket initialization includes name.The
getOrCreateBucketfunction correctly initializes the bucket'snamefield from the provided key.go/apps/api/routes/v2_ratelimit_limit/handler.go (1)
259-266: LGTM! Correct separation of Name and Identifier.The changes properly distinguish between:
Name: Set tonamespace.IDto identify the rate limit configurationIdentifier: Set toreq.Identifierto identify the entity being rate limitedThis ensures that rate limits are correctly isolated by namespace while still tracking per-identifier limits.
|
good catch |
|
|
Graphite Automations"Post a GIF when PR approved" took an action on this PR • (11/05/25)1 gif was posted to this PR based on Andreas Thomas's automation. |
Flo4604
deleted the
eng-2179-ensure-v2-rate-limits-are-isolated-by-name
branch
3 participants
What does this PR do?
Fixes #4201
Type of change
How should this be tested?
Run tests.
When a identity has the same limit applied twice to it that maps to a unique bucket (same identifier (identity-id) + duration + limit) then we would re-use the same bucket even tho thats not what should happen.
So now we just add the ratelimit name into the bucket key in order to make them unique to mirror the v1.
Checklist
Required
pnpm buildpnpm fmtmake fmton/godirectoryconsole.logsgit pull origin mainAppreciated