fix: atomic increment and other minor issues suggested by coderabbit

Author: max-programming

src/core/cache.test.ts

@@ -58,6 +58,50 @@ describe("MemoryCache", () => {
 		expect(cache.get("key1")).toBeNull();
 		expect(cache.get("key2")).toBe("value2");
 	});
+
+	it("should increment a non-existent key to 1", () => {
+		const result = cache.incr("counter", 60);
+		expect(result).toBe(1);
+		expect(cache.get("counter")).toBe("1");
+	});
+
+	it("should increment an existing key", () => {
+		cache.set("counter", "5", 60);
+		const result = cache.incr("counter", 60);
+		// biome-ignore lint/style/noMagicNumbers: 6 is expected incremented value
+		expect(result).toBe(6);
+		expect(cache.get("counter")).toBe("6");
+	});
+
+	it("should increment multiple times", () => {
+		expect(cache.incr("counter", 60)).toBe(1);
+		expect(cache.incr("counter", 60)).toBe(2);
+		// biome-ignore lint/style/noMagicNumbers: 3 is expected incremented value
+		expect(cache.incr("counter", 60)).toBe(3);
+		expect(cache.get("counter")).toBe("3");
+	});
+
+	it("should handle increment with TTL", async () => {
+		// biome-ignore lint/style/noMagicNumbers: 100ms
+		cache.incr("counter", 0.1);
+		expect(cache.get("counter")).toBe("1");
+
+		// biome-ignore lint/style/noMagicNumbers: 150ms
+		await new Promise((resolve) => setTimeout(resolve, 150));
+		expect(cache.get("counter")).toBeNull();
+	});
+
+	it("should not increment expired keys", async () => {
+		// biome-ignore lint/style/noMagicNumbers: 100ms
+		cache.set("counter", "5", 0.1);
+
+		// biome-ignore lint/style/noMagicNumbers: 150ms
+		await new Promise((resolve) => setTimeout(resolve, 150));
+
+		const result = cache.incr("counter", 60);
+		expect(result).toBe(1);
+		expect(cache.get("counter")).toBe("1");
+	});
 });
 
 describe("RedisCache", () => {
@@ -69,6 +113,7 @@ describe("RedisCache", () => {
 			get: vi.fn(),
 			setex: vi.fn(),
 			del: vi.fn(),
+			eval: vi.fn(),
 		};
 		cache = new RedisCache(mockRedisClient);
 	});
@@ -101,4 +146,30 @@ describe("RedisCache", () => {
 		const result = await cache.get("non-existent");
 		expect(result).toBeNull();
 	});
+
+	it("should call redis eval with Lua script for incr", async () => {
+		// biome-ignore lint/style/noMagicNumbers: 5 as the incremented value
+		mockRedisClient.eval.mockResolvedValue(5);
+
+		// biome-ignore lint/style/noMagicNumbers: 120 seconds
+		const result = await cache.incr("counter", 120);
+
+		// biome-ignore lint/style/noMagicNumbers: 5 as the incremented value
+		expect(result).toBe(5);
+		expect(mockRedisClient.eval).toHaveBeenCalledWith(
+			expect.stringContaining("INCR"),
+			1,
+			"counter",
+			// biome-ignore lint/style/noMagicNumbers: 120 seconds
+			120
+		);
+	});
+
+	it("should handle incr returning number", async () => {
+		mockRedisClient.eval.mockResolvedValue(1);
+
+		const result = await cache.incr("new-counter", 60);
+		expect(result).toBe(1);
+		expect(typeof result).toBe("number");
+	});
 });

src/core/cache.ts

@@ -4,6 +4,14 @@ export type Cache = {
 	get(key: string): Promise<string | null> | string | null;
 	set(key: string, value: string, ttl?: number): Promise<void> | void;
 	del(key: string): Promise<void> | void;
+	/**
+	 * Atomically increments the value at key by 1 and returns the new value.
+	 * If the key doesn't exist, it's set to 1.
+	 * @param key - The cache key
+	 * @param ttl - Time to live in seconds
+	 * @returns The new value after incrementing
+	 */
+	incr(key: string, ttl?: number): Promise<number> | number;
 };
 
 export class MemoryCache implements Cache {
@@ -38,6 +46,21 @@ export class MemoryCache implements Cache {
 		this.cache.delete(key);
 	}
 
+	incr(key: string, ttl = 60): number {
+		const item = this.cache.get(key);
+		const currentValue =
+			item && item.expires >= Date.now() ? Number.parseInt(item.value, 10) : 0;
+		const newValue = currentValue + 1;
+
+		this.cache.set(key, {
+			value: String(newValue),
+			// biome-ignore lint/style/noMagicNumbers: 1000ms to seconds
+			expires: Date.now() + ttl * 1000,
+		});
+
+		return newValue;
+	}
+
 	clear(): void {
 		this.cache.clear();
 	}
@@ -61,4 +84,17 @@ export class RedisCache implements Cache {
 	async del(key: string): Promise<void> {
 		await this.client.del(key);
 	}
+
+	async incr(key: string, ttl = 60): Promise<number> {
+		// Use Redis INCR which is atomic, then set TTL
+		// Using a Lua script ensures both operations are atomic
+		const script = `
+			local count = redis.call('INCR', KEYS[1])
+			redis.call('EXPIRE', KEYS[1], ARGV[1])
+			return count
+		`;
+
+		const result = await this.client.eval(script, 1, key, ttl);
+		return Number(result);
+	}
 }

src/core/rate-limiter.test.ts

@@ -52,7 +52,9 @@ describe("RateLimiter", () => {
 		const result = await rateLimiter.check(apiKeyRecord);
 
 		expect(result.allowed).toBe(false);
-		expect(result.current).toBe(REQUEST_COUNT);
+		// With atomic increment, the counter increments before checking,
+		// so the 6th request will show current: 6
+		expect(result.current).toBe(REQUEST_COUNT + 1);
 		expect(result.remaining).toBe(0);
 	});
 
@@ -176,11 +178,11 @@ describe("RateLimiter", () => {
 		try {
 			await redis.connect();
 			await redis.ping();
-		} catch (error) {
-			console.warn(
+		} catch {
+			it.skip(
 				"Redis not available. Skipping Redis tests. Start with: bun run redis:up"
 			);
-			throw error;
+			return;
 		}
 
 		const keyManager = createKeys({

src/core/rate-limiter.ts

@@ -13,6 +13,13 @@ export class RateLimiter {
 	private readonly keyPrefix: string;
 
 	constructor(cache: Cache, config: RateLimitConfig) {
+		if (config.windowMs <= 0) {
+			throw new Error("windowMs must be a positive number");
+		}
+		if (config.maxRequests <= 0) {
+			throw new Error("maxRequests must be a positive number");
+		}
+
 		this.cache = cache;
 		this.config = config;
 		this.keyPrefix = config.keyPrefix ?? "ratelimit";
@@ -46,32 +53,42 @@ export class RateLimiter {
 			this.config.windowMs / MILLISECONDS_PER_SECOND
 		);
 
-		const currentValue = await this.cache.get(key);
-		const current = currentValue ? Number.parseInt(currentValue, 10) : 0;
+		if (increment) {
+			// Use atomic increment to prevent race conditions
+			const newCount = await this.cache.incr(key, ttlSeconds);
+
+			if (newCount > this.config.maxRequests) {
+				return {
+					allowed: false,
+					current: newCount,
+					limit: this.config.maxRequests,
+					resetMs,
+					resetAt: new Date(resetAt).toISOString(),
+					remaining: 0,
+				};
+			}
 
-		if (current >= this.config.maxRequests) {
 			return {
-				allowed: false,
-				current,
+				allowed: true,
+				current: newCount,
 				limit: this.config.maxRequests,
 				resetMs,
 				resetAt: new Date(resetAt).toISOString(),
-				remaining: 0,
+				remaining: this.config.maxRequests - newCount,
 			};
 		}
 
-		const newCount = increment ? current + 1 : current;
-		if (increment) {
-			await this.cache.set(key, String(newCount), ttlSeconds);
-		}
+		// When not incrementing, just check the current value
+		const currentValue = await this.cache.get(key);
+		const current = currentValue ? Number.parseInt(currentValue, 10) : 0;
 
 		return {
-			allowed: true,
-			current: newCount,
+			allowed: current < this.config.maxRequests,
+			current,
 			limit: this.config.maxRequests,
 			resetMs,
 			resetAt: new Date(resetAt).toISOString(),
-			remaining: this.config.maxRequests - newCount,
+			remaining: Math.max(0, this.config.maxRequests - current),
 		};
 	}
 

src/types/rate-limit-types.ts

@@ -22,7 +22,7 @@ export type RateLimitResult = {
 	limit: number;
 	/** Time in milliseconds until the window resets */
 	resetMs: number;
-	/** ISO timestampt when the window resets */
+	/** ISO timestamp when the window resets */
 	resetAt: string;
 	/** Number of requests remaining within the window */
 	remaining: number;