feat: add more cloudflare ratelimits #2636
Conversation
chronark
requested review from
perkinsjr,
mcstepp and
MichaelUnkey
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThis pull request introduces several modifications across multiple files, primarily focusing on enhancing rate limiting configurations in the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
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: 2
🧹 Outside diff range and nitpick comments (7)
apps/api/src/pkg/hono/app.ts (1)
60-60: Consider using a more generic example value.The current example "UNKEY_ROOT_KEY" might expose internal naming conventions. Consider using a more generic example that still conveys the format but doesn't reveal implementation details.
- "x-speakeasy-example": "UNKEY_ROOT_KEY", + "x-speakeasy-example": "YOUR_API_KEY",apps/api/src/pkg/env.ts (1)
51-51: LGTM! Rate limit configurations follow consistent patterns.The new rate limiters (RL_50_60s, RL_600_60s, RL_1_10s) are correctly implemented using the cloudflareRatelimiter schema and follow the established naming convention.
Consider documenting the intended use cases for each rate limit tier, especially:
- RL_600_60s (10 requests/second) which is quite permissive
- RL_1_10s (0.1 requests/second) which is very restrictive
This will help future maintainers understand when to apply each tier.Also applies to: 53-54
apps/api/src/pkg/ratelimit/client.ts (2)
89-95: Consider documenting the rate limiting architecture.The codebase implements a sophisticated hybrid rate limiting approach using both Cloudflare and agent-based solutions. Consider adding architecture documentation that covers:
- The difference between async and sync behaviors
- Fallback mechanisms and when they trigger
- Cache management strategy
- Retry policies
This would help maintainers understand the system's behavior and trade-offs.
89-95: Consider optimizing cache cleanup strategy.The current implementation cleans up expired entries only when the cache size exceeds 10,000 entries. Consider implementing a periodic cleanup or using a more efficient data structure like an LRU cache to prevent memory spikes.
Example libraries to consider:
lru-cachefor efficient LRU implementationquick-lrufor a simpler alternativepackages/api/src/openapi.d.ts (3)
1381-1382: Enhance documentation for refillDay in updateKey.The documentation for
refillDayin the updateKey operation is minimal compared to other locations. Consider adding:
- Default value information
- Valid value ranges
- Behavior when the specified day doesn't exist in a month
Apply this diff to improve documentation:
/** @description The day verifications will refill each month, when interval is set to 'monthly' */ refillDay?: number; + /** + * @description The day verifications will refill each month, when interval is set to 'monthly'. Value is not zero-indexed making 1 the first day of the month. If left blank it will default to the first day of the month. When 'daily' is set for 'interval' 'refillDay' will be set to null. + * @default 1 + * @example 15 + */ + refillDay?: number | null;
2783-2784: Standardize documentation across migration endpoints.The documentation for
refillDayin migration endpoints (createKeys and enqueueKeys) lacks consistency with the main schema. This could lead to confusion for API consumers.Apply this diff to both migration endpoints:
/** @description The day verifications will refill each month, when interval is set to 'monthly' */ refillDay?: number; + /** + * @description The day verifications will refill each month, when interval is set to 'monthly'. Value is not zero-indexed making 1 the first day of the month. If left blank it will default to the first day of the month. When 'daily' is set for 'interval' 'refillDay' will be set to null. + * @default 1 + * @example 15 + */ + refillDay?: number | null;Also applies to: 2997-2998
Line range hint
361-383: Consider adding validation constraints for refillDay.While the documentation is comprehensive, it would be beneficial to add validation constraints for the refillDay value to ensure it falls within a valid range (1-31).
Add validation constraints to the schema:
/** * @description The day verifications will refill each month, when interval is set to 'monthly'. Value is not zero-indexed making 1 the first day of the month. If left blank it will default to the first day of the month. When 'daily' is set for 'interval' 'refillDay' will be set to null. * @default 1 * @example 15 + * @minimum 1 + * @maximum 31 */ refillDay?: number | null;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
apps/api/src/pkg/env.ts(1 hunks)apps/api/src/pkg/hono/app.ts(1 hunks)apps/api/src/pkg/ratelimit/client.ts(1 hunks)apps/api/wrangler.toml(4 hunks)packages/api/src/openapi.d.ts(7 hunks)
🔇 Additional comments (4)
apps/api/src/pkg/hono/app.ts (1)
60-60: LGTM! Good documentation enhancement.
Adding an example value improves the API documentation and helps consumers understand the expected authentication format.
apps/api/src/pkg/env.ts (1)
51-51: Verify rate limit configuration usage.
Let's ensure these new rate limits are being used appropriately in the codebase.
Also applies to: 53-54
apps/api/wrangler.toml (1)
57-84: LGTM on the rate limit configurations!
The new rate limiters have been consistently added across all environments with appropriate limits:
RL_50_60s: 50 requests per minuteRL_600_60s: 600 requests per minuteRL_1_10s: 1 request per 10 seconds
These additions provide more granular control over API usage.
Let's verify the consistency of rate limiter configurations across all environments:
Also applies to: 145-170, 225-250, 308-334
✅ Verification successful
Rate limiter configurations are consistent across environments
The verification confirms that all rate limiters (RL_50_60s, RL_200_60s, RL_600_60s, RL_1_10s, and others) are consistently configured across development, preview, canary, and production environments with identical limits and periods. Each rate limiter maintains its specific namespace ID and configuration parameters uniformly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify rate limiter configurations are consistent across environments
# Test: Extract and compare rate limiter configurations
echo "Checking rate limiter consistency across environments..."
for env in "" ".preview" ".canary" ".production"; do
echo "Environment: ${env:-development}"
rg "^\[\[(?:env${env}\.)?unsafe\.bindings\]\]" -A 5 | grep -E "name|limit"
done
Length of output: 12110
apps/api/src/pkg/ratelimit/client.ts (1)
89-95: Document the async rate limit response structure.
The response structure uses special values (-1) for untracked fields in async mode. Consider adding a comment explaining this behavior and why these fields differ from the sync mode response.
+ // In async mode, we don't track exact counts and reset times
+ // Only the pass/fail status and trigger reason are available
return Ok({
pass: res.success,
reset: -1,
current: -1,
remaining: -1,
triggered: res.success ? null : req.name,
});Let's verify the response structure consistency across the codebase:
✅ Verification successful
Based on the search results, I can now generate the final response. The code shows consistent handling of rate limit responses across the codebase, with the triggered field being properly implemented in both sync and async paths.
The -1 values in async mode are intentional and documented.
The rate limit response structure is consistently implemented across the codebase:
- In sync mode: All fields (pass, reset, current, remaining, triggered) contain actual values
- In async mode: Only pass/fail status and trigger reason are tracked, with other numeric fields set to -1
- The
triggeredfield is consistently used to indicate which rate limit caused the rejection
The implementation aligns with the defined interfaces and is used consistently throughout the rate limiting system. The -1 values are a valid way to indicate untracked metrics in async mode.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other rate limit response structures to ensure consistency
rg -A 5 "return Ok\(\{" --type typescript
Length of output: 78
Script:
#!/bin/bash
# Search for rate limit response structures with ripgrep, without file type filter
rg -A 5 "return Ok\(\{"
# Also search for similar patterns to catch variations
rg -A 5 "return Ok\s*\("
# Use ast-grep to find return statements with object literals
ast-grep --pattern 'return Ok({$$$})'
Length of output: 37629
Script:
#!/bin/bash
# Search for rate limit response type definitions
rg -A 10 "type.*RatelimitResponse" --type typescript
# Search for rate limit interface or type declarations
rg -A 10 "interface.*Ratelimit" --type typescript
# Look for rate limit related tests
rg -A 5 "describe.*ratelimit" --type typescript
Length of output: 261
Script:
#!/bin/bash
# Search for rate limit response type definitions without file type filter
rg -A 10 "type.*RatelimitResponse"
# Search for rate limit interface declarations
rg -A 10 "interface.*Ratelimit"
# Look for rate limit related tests
rg -A 5 "describe.*ratelimit"
# Search for files containing ratelimit in their name
fd ratelimit
Length of output: 25612
| [[env.canary.unsafe.bindings]] | ||
| name = "RL_1_10s" | ||
| type = "ratelimit" | ||
| namespace_id = "9900110" | ||
| simple = { limit = 1, period = 10} | ||
|
|
There was a problem hiding this comment.
Move misplaced binding to correct environment section
The RL_1_10s binding is incorrectly placed under env.canary.unsafe.bindings within the preview environment section. It should be under env.preview.unsafe.bindings.
Move this binding to the preview environment section, just before the existing RL_500_10s binding.
| [[unsafe.bindings]] | ||
| name = "RL_1_10s" | ||
| type = "ratelimit" | ||
| namespace_id = "9900110" | ||
| simple = { limit = 1, period = 10} | ||
|
|
There was a problem hiding this comment.
Fix namespace_id format inconsistency
The namespace_id for RL_1_10s doesn't follow the established 9-digit format used by other rate limiters. It should be 99000110 (9 digits) instead of 9900110 (7 digits) to maintain consistency with the pattern 9900{limit}{duration}.
Apply this fix:
[[unsafe.bindings]]
name = "RL_1_10s"
type = "ratelimit"
- namespace_id = "9900110"
+ namespace_id = "99000110"
simple = { limit = 1, period = 10}📝 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.
| [[unsafe.bindings]] | |
| name = "RL_1_10s" | |
| type = "ratelimit" | |
| namespace_id = "9900110" | |
| simple = { limit = 1, period = 10} | |
| [[unsafe.bindings]] | |
| name = "RL_1_10s" | |
| type = "ratelimit" | |
| namespace_id = "99000110" | |
| simple = { limit = 1, period = 10} | |
Summary by CodeRabbit
Release Notes
New Features
RL_50_60s,RL_600_60s, andRL_1_10s, enhancing control over request limits.Improvements
refillDayfield for refill scheduling.Deprecations
deprecated.verifyKeyoperation as deprecated to streamline key verification processes.