🔒 Fix In-Memory Rate Limiting in Serverless Environment#303
Conversation
Co-authored-by: is0692vs <135803462+is0692vs@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
📝 WalkthroughSummary by CodeRabbitリリースノート
Walkthroughこのプルリクエストは、RateLimiter を Upstash Redis ベースの非同期実装に移行し、フォールバック機能を保持しながら API ハンドラを対応させ、包括的なテスト・設定を追加しています。 ChangesUpstash レート制限統合
Sequence DiagramsequenceDiagram
participant Handler as API Handler
participant RateLimiter
participant Upstash as Upstash Redis
participant Cache as In-memory Cache
Handler->>RateLimiter: await check(ip)
alt Upstash 設定あり
RateLimiter->>Upstash: limit(key)
Upstash-->>RateLimiter: {success, reset}
else Upstash 設定なし
RateLimiter->>Cache: 内メモリ キャッシュ確認
Cache-->>RateLimiter: {success, reset}
end
RateLimiter-->>Handler: Promise<{success, reset}>
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces Upstash Redis-based rate limiting to the application, updating the RateLimiter class to support distributed limiting while maintaining an in-memory fallback. The check method has been converted to an asynchronous function, necessitating updates across API routes and test suites. Feedback highlights the need for robust error handling around the Upstash network request; specifically, it is recommended to wrap the call in a try...catch block to prevent API failures and ensure a graceful fallback to in-memory limiting if the Redis service is unavailable.
| if (this.upstashRatelimit) { | ||
| const { success, reset } = await this.upstashRatelimit.limit(key); | ||
| return { success, reset }; | ||
| } |
There was a problem hiding this comment.
The call to this.upstashRatelimit.limit(key) performs a network request to the Upstash Redis service. If the service is unreachable or encounters a timeout, this call will throw an exception, causing the entire API route to fail with a 500 error. It is recommended to wrap this call in a try...catch block and fall back to the in-memory limiter to ensure the application remains functional. When implementing this fallback, ensure the error is logged to facilitate debugging, as per repository guidelines for handling suppressed rejections.
References
- When suppressing unhandled promise rejections, log the error instead of silently ignoring it to facilitate debugging.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| async check(key: string): Promise<{ success: boolean; reset: number }> { | ||
| if (this.upstashRatelimit) { | ||
| const { success, reset } = await this.upstashRatelimit.limit(key); | ||
| return { success, reset }; | ||
| } |
There was a problem hiding this comment.
upstashRatelimit.limit(key) がネットワーク障害や接続エラーでthrowした場合、例外がそのまま呼び出し元まで伝播し、APIが500エラーを返します。Redisが一時的に利用できない状況では、レート制限が機能しなくなる代わりにAPIそのものがダウンするため、インメモリのフォールバックに切り替える方が安全です。
| async check(key: string): Promise<{ success: boolean; reset: number }> { | |
| if (this.upstashRatelimit) { | |
| const { success, reset } = await this.upstashRatelimit.limit(key); | |
| return { success, reset }; | |
| } | |
| async check(key: string): Promise<{ success: boolean; reset: number }> { | |
| if (this.upstashRatelimit) { | |
| try { | |
| const { success, reset } = await this.upstashRatelimit.limit(key); | |
| return { success, reset }; | |
| } catch { | |
| // Redis が一時的に利用不可の場合はインメモリフォールバックで継続 | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/rateLimit.ts
Line: 29-33
Comment:
Redisエラー時のフォールバック処理が不足しています。`upstashRatelimit.limit(key)` がネットワーク障害や接続エラーでthrowした場合、例外がそのまま呼び出し元まで伝播し、APIが500エラーを返します。Redisが一時的に利用できない状況では、レート制限が機能しなくなる代わりにAPIそのものがダウンするため、インメモリのフォールバックに切り替える方が安全です。
```suggestion
async check(key: string): Promise<{ success: boolean; reset: number }> {
if (this.upstashRatelimit) {
try {
const { success, reset } = await this.upstashRatelimit.limit(key);
return { success, reset };
} catch {
// Redis が一時的に利用不可の場合はインメモリフォールバックで継続
}
}
```
How can I resolve this? If you propose a fix, please make it concise.Co-authored-by: is0692vs <135803462+is0692vs@users.noreply.github.com>
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/rateLimit.ts`:
- Around line 29-33: The call to this.upstashRatelimit.limit(key) in the check()
method can reject and bubble up a 500; wrap that call in a try/catch inside
check(), log the error, and on failure return the result from the in-memory
fallback rate limiter (i.e., call the class's in-memory limiter method/field
instead of letting the exception propagate). Specifically, modify check() so it
tries await this.upstashRatelimit.limit(key) but on catch returns the equivalent
response from your local/in-memory limiter (and include a brief error log),
ensuring callers like route handlers calling rateLimiter.check(ip) never get an
unhandled exception.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fa290fa4-e0db-4cc4-9815-7d95c9f542b9
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
package.jsonsrc/app/api/card/[username]/route.tssrc/app/api/og/[username]/route.tsxsrc/lib/__tests__/rateLimit.test.tssrc/lib/rateLimit.tsvitest.config.ts
| async check(key: string): Promise<{ success: boolean; reset: number }> { | ||
| if (this.upstashRatelimit) { | ||
| const { success, reset } = await this.upstashRatelimit.limit(key); | ||
| return { success, reset }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant section of src/lib/rateLimit.ts
if [ -f "src/lib/rateLimit.ts" ]; then
nl -ba src/lib/rateLimit.ts | sed -n '1,220p'
else
echo "Missing file: src/lib/rateLimit.ts"
fi
# Find where RateLimiter.check is called
rg -n "rateLimit\.check\(|\.check\(\s*key|new RateLimiter|class RateLimiter" -S src || true
# Specifically search for ".check(" usage on rate limiter instances
rg -n "\.check\(" -S src/lib src || true
# Search for "upstashRatelimit.limit" usage
rg -n "upstashRatelimit\.limit|upstashRatelimit" -S src || trueRepository: Hiroki-org/github-user-summary
Length of output: 117
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show relevant portion of src/lib/rateLimit.ts
if [ -f "src/lib/rateLimit.ts" ]; then
echo "=== src/lib/rateLimit.ts (1-220) ==="
sed -n '1,220p' src/lib/rateLimit.ts | cat -n
else
echo "Missing file: src/lib/rateLimit.ts"
fi
echo "=== search: RateLimiter.check call sites ==="
rg -n "rateLimit\.check\(|new RateLimiter|class RateLimiter" -S src || true
echo "=== search: .check( usage ==="
rg -n "\.check\(" -S src/lib src || true
echo "=== search: upstashRatelimit.limit / upstashRatelimit ==="
rg -n "upstashRatelimit\.limit|upstashRatelimit" -S src || trueRepository: Hiroki-org/github-user-summary
Length of output: 6004
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== src/app/api/og/[username]/route.tsx (1-140) ==="
sed -n '1,160p' src/app/api/og/[username]/route.tsx | cat -n
echo "=== src/app/api/card/[username]/route.ts (1-140) ==="
sed -n '1,180p' src/app/api/card/[username]/route.ts | cat -n
echo "=== src/lib/__tests__/rateLimit.test.ts (1-200) ==="
sed -n '1,240p' src/lib/__tests__/rateLimit.test.ts | cat -nRepository: Hiroki-org/github-user-summary
Length of output: 12427
Upstash 側 limit() の reject がそのまま例外伝播して 500 になり得るため、check() で try/catch して in-memory にフォールバックすべきです
src/lib/rateLimit.ts (29-33) で this.upstashRatelimit.limit(key) が reject した場合に例外を握りつぶさず、そのまま src/app/api/og/[username]/route.tsx / src/app/api/card/[username]/route.ts の rateLimiter.check(ip) 呼び出しへ伝播し、両 route とも check() 周りを try/catch していないためです。
💡 提案差分
async check(key: string): Promise<{ success: boolean; reset: number }> {
if (this.upstashRatelimit) {
- const { success, reset } = await this.upstashRatelimit.limit(key);
- return { success, reset };
+ try {
+ const { success, reset } = await this.upstashRatelimit.limit(key);
+ return { success, reset };
+ } catch {
+ // Upstash到達不可時は in-memory fallback に委譲
+ }
}
// Fallback to in-memory caching
const now = Date.now();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/rateLimit.ts` around lines 29 - 33, The call to
this.upstashRatelimit.limit(key) in the check() method can reject and bubble up
a 500; wrap that call in a try/catch inside check(), log the error, and on
failure return the result from the in-memory fallback rate limiter (i.e., call
the class's in-memory limiter method/field instead of letting the exception
propagate). Specifically, modify check() so it tries await
this.upstashRatelimit.limit(key) but on catch returns the equivalent response
from your local/in-memory limiter (and include a brief error log), ensuring
callers like route handlers calling rateLimiter.check(ip) never get an unhandled
exception.
1 participant
🎯 What: Replaced the purely in-memory rate limiter with a distributed Upstash Redis-based rate limiter using
⚠️ Risk: Purely in-memory rate limiting is ineffective in a serverless environment (like Vercel Edge functions) because state isn't shared across instances, allowing attackers to easily bypass limits by hitting different instances.
@upstash/ratelimitand@upstash/redis.🛡️ Solution: Integrated Upstash Redis for shared rate limit state across serverless instances, with a graceful fallback to in-memory caching if environment variables are missing (for local dev).
PR created automatically by Jules for task 457086307398069304 started by @is0692vs
Greptile Summary
インメモリのレート制限をUpstash Redisベースの分散レート制限に置き換えることで、Vercel Edgeのようなサーバーレス環境での複数インスタンス間での状態共有を実現しています。環境変数が未設定の場合にはインメモリへのフォールバックを行うことでローカル開発との互換性も保たれています。
RateLimiterクラスを拡張し、Upstash Redisが設定されている場合は@upstash/ratelimitのslidingWindowアルゴリズムで分散カウントを行い、そうでない場合は既存のインメモリキャッシュを使用するフォールバックを実装。/api/card/:usernameおよび/api/og/:usernameのルートハンドラーでcheck()呼び出しにawaitを追加し、テストも非同期対応に更新。Confidence Score: 3/5
Redisが一時的に利用できない場合にAPIが500エラーとなる未処理の例外パスがあるため、マージ前に修正が推奨されます。
Redisクライアントのエラーハンドリングが
check()メソッドに存在しないため、Upstash接続の問題がそのままAPIレスポンスの失敗に直結します。インメモリのフォールバックは実装済みですが、Redis例外をキャッチしてそのパスに切り替えるコードがないため、本来のフォールバックが機能しません。src/lib/rateLimit.ts の
check()メソッドにおけるエラーハンドリングを重点的に確認してください。Important Files Changed
rateLimiter.check()の呼び出しをawait付きに更新。それ以外の変更なし。rateLimiter.check()の呼び出しをawait付きに更新。それ以外の変更なし。@upstash/ratelimitと@upstash/redisの依存関係を追加。Sequence Diagram
sequenceDiagram participant Client participant EdgeFunction as Vercel Edge Function participant RateLimiter as RateLimiter participant Upstash as Upstash Redis participant InMemory as In-Memory Cache Client->>EdgeFunction: GET /api/card/:username (or /api/og/:username) EdgeFunction->>RateLimiter: await check(ip) alt Upstash 環境変数あり RateLimiter->>Upstash: upstashRatelimit.limit(key) Upstash-->>RateLimiter: "{ success, reset }" Note over RateLimiter,Upstash: エラー時は現状500エラーが伝播 else Upstash 環境変数なし (ローカル開発) RateLimiter->>InMemory: キャッシュ参照・更新 InMemory-->>RateLimiter: "{ success, reset }" end RateLimiter-->>EdgeFunction: "{ success, reset }" alt "success = false" EdgeFunction-->>Client: 429 Too Many Requests else "success = true" EdgeFunction->>EdgeFunction: ビジネスロジック実行 EdgeFunction-->>Client: 200 OK endComments Outside Diff (1)
src/lib/__tests__/rateLimit.test.ts, line 1-41 (link)テストスイートはインメモリのフォールバック動作のみを検証しており、
UPSTASH_REDIS_REST_URL/UPSTASH_REDIS_REST_TOKENが設定された場合のUpstash経由のパスが全くテストされていません。Ratelimitをモックして、Upstashがsuccess: falseを返す場合や例外をスローする場合の挙動を確認するテストケースを追加することを検討してください。Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "🔒 Fix In-Memory Rate Limiting in Server..." | Re-trigger Greptile