-
Notifications
You must be signed in to change notification settings - Fork 0
🔒 Fix In-Memory Rate Limiting in Serverless Environment #303
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,41 +1,98 @@ | ||
| import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; | ||
| import { RateLimiter } from "../rateLimit"; | ||
| import { Ratelimit } from "@upstash/ratelimit"; | ||
|
|
||
| vi.mock("@upstash/redis", () => { | ||
| return { | ||
| Redis: class { | ||
| constructor() {} | ||
| } | ||
| }; | ||
| }); | ||
|
|
||
| vi.mock("@upstash/ratelimit", () => { | ||
| const mockLimit = vi.fn().mockResolvedValue({ success: true, reset: 12345 }); | ||
| return { | ||
| Ratelimit: class { | ||
| static slidingWindow = vi.fn().mockReturnValue("sliding-window-algo"); | ||
| limit = mockLimit; | ||
| } | ||
| }; | ||
| }); | ||
|
|
||
| describe("RateLimiter", () => { | ||
| const originalEnv = process.env; | ||
|
|
||
| beforeEach(() => { | ||
| vi.useFakeTimers(); | ||
| process.env = { ...originalEnv }; | ||
| delete process.env.UPSTASH_REDIS_REST_URL; | ||
| delete process.env.UPSTASH_REDIS_REST_TOKEN; | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| vi.restoreAllMocks(); | ||
| process.env = originalEnv; | ||
| }); | ||
|
|
||
| it("allows requests below the limit", () => { | ||
| const limiter = new RateLimiter(2, 1000); | ||
| const key = "test-key"; | ||
| describe("In-memory Fallback", () => { | ||
| it("allows requests below the limit", async () => { | ||
| const limiter = new RateLimiter(2, 1000); | ||
| const key = "test-key"; | ||
|
|
||
| expect(limiter.check(key).success).toBe(true); | ||
| expect(limiter.check(key).success).toBe(true); | ||
| }); | ||
| expect((await limiter.check(key)).success).toBe(true); | ||
| expect((await limiter.check(key)).success).toBe(true); | ||
| }); | ||
|
|
||
| it("blocks requests above the limit", async () => { | ||
| const limiter = new RateLimiter(2, 1000); | ||
| const key = "test-key"; | ||
|
|
||
| await limiter.check(key); | ||
| await limiter.check(key); | ||
| expect((await limiter.check(key)).success).toBe(false); | ||
| }); | ||
|
|
||
| it("resets after the window has passed", async () => { | ||
| const limiter = new RateLimiter(1, 1000); | ||
| const key = "test-key"; | ||
|
|
||
| expect((await limiter.check(key)).success).toBe(true); | ||
| expect((await limiter.check(key)).success).toBe(false); | ||
|
|
||
| vi.advanceTimersByTime(1001); | ||
|
|
||
| expect((await limiter.check(key)).success).toBe(true); | ||
| }); | ||
|
|
||
| it("lazy cleans up properly", async () => { | ||
| const limiter = new RateLimiter(1, 1000); | ||
| const key1 = "test-key-1"; | ||
| const key2 = "test-key-2"; | ||
|
|
||
| it("blocks requests above the limit", () => { | ||
| const limiter = new RateLimiter(2, 1000); | ||
| const key = "test-key"; | ||
| await limiter.check(key1); | ||
| vi.advanceTimersByTime(1001); | ||
| await limiter.check(key2); // triggers cleanup for key1 | ||
|
|
||
| limiter.check(key); | ||
| limiter.check(key); | ||
| expect(limiter.check(key).success).toBe(false); | ||
| // internal check would be needed, but essentially the fact it works implies cleanup did not crash | ||
| expect((await limiter.check(key2)).success).toBe(false); | ||
| }); | ||
| }); | ||
|
|
||
| it("resets after the window has passed", () => { | ||
| const limiter = new RateLimiter(1, 1000); | ||
| const key = "test-key"; | ||
| describe("Upstash Redis", () => { | ||
| beforeEach(() => { | ||
| process.env.UPSTASH_REDIS_REST_URL = "https://fake-url"; | ||
| process.env.UPSTASH_REDIS_REST_TOKEN = "fake-token"; | ||
| }); | ||
|
|
||
| expect(limiter.check(key).success).toBe(true); | ||
| expect(limiter.check(key).success).toBe(false); | ||
| it("uses Upstash Ratelimit when env vars are present", async () => { | ||
| const limiter = new RateLimiter(2, 1000); | ||
| const key = "upstash-key"; | ||
|
|
||
| vi.advanceTimersByTime(1001); | ||
| const result = await limiter.check(key); | ||
|
|
||
| expect(limiter.check(key).success).toBe(true); | ||
| expect(result.success).toBe(true); | ||
| expect(result.reset).toBe(12345); | ||
| }); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,8 +1,22 @@ | ||||||||||||||||||||||||||||||
| import { Redis } from "@upstash/redis"; | ||||||||||||||||||||||||||||||
| import { Ratelimit } from "@upstash/ratelimit"; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| export class RateLimiter { | ||||||||||||||||||||||||||||||
| private cache = new Map<string, { count: number; resetTime: number }>(); | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| constructor(private limit: number, private windowMs: number) {} | ||||||||||||||||||||||||||||||
| private upstashRatelimit: Ratelimit | null = null; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| constructor(private limit: number, private windowMs: number) { | ||||||||||||||||||||||||||||||
| if (process.env.UPSTASH_REDIS_REST_URL && process.env.UPSTASH_REDIS_REST_TOKEN) { | ||||||||||||||||||||||||||||||
| const redis = new Redis({ | ||||||||||||||||||||||||||||||
| url: process.env.UPSTASH_REDIS_REST_URL, | ||||||||||||||||||||||||||||||
| token: process.env.UPSTASH_REDIS_REST_TOKEN, | ||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||
| this.upstashRatelimit = new Ratelimit({ | ||||||||||||||||||||||||||||||
| redis: redis, | ||||||||||||||||||||||||||||||
| limiter: Ratelimit.slidingWindow(this.limit, `${this.windowMs} ms`), | ||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| private cleanup(now: number) { | ||||||||||||||||||||||||||||||
| for (const [key, record] of this.cache.entries()) { | ||||||||||||||||||||||||||||||
|
|
@@ -12,7 +26,13 @@ export class RateLimiter { | |||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| check(key: string): { success: boolean; reset: number } { | ||||||||||||||||||||||||||||||
| async check(key: string): Promise<{ success: boolean; reset: number }> { | ||||||||||||||||||||||||||||||
| if (this.upstashRatelimit) { | ||||||||||||||||||||||||||||||
| const { success, reset } = await this.upstashRatelimit.limit(key); | ||||||||||||||||||||||||||||||
| return { success, reset }; | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
Comment on lines
+29
to
+33
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Prompt To Fix With AIThis 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.
Comment on lines
+29
to
+33
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 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 側
💡 提案差分 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 |
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // Fallback to in-memory caching | ||||||||||||||||||||||||||||||
| const now = Date.now(); | ||||||||||||||||||||||||||||||
| this.cleanup(now); // Lazy cleanup | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 atry...catchblock 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