feat: add TTL cache for API response caching - #5
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR adds a well-structured TTL cache class and Cache-Control header parser, but contains several correctness defects that could cause confusing behavior in production. The most impactful issues are: (1) has() reports expired entries as present while get() returns undefined for the same key — an inconsistent API contract; (2) ttlMs || this.defaultTtlMs uses logical-OR instead of nullish coalescing, so an explicit ttlMs=0 (intended immediate expiry) silently falls back to the default TTL; (3) parseCacheControl propagates NaN when max-age is non-numeric. There is also no test file for the new module. Risk level: medium.
5 finding(s): 0 error, 4 warning, 1 info
[WARNING] correctness
File: src/utils/cache.ts:36
has() delegates directly to Map.has() without checking expiry. This means has(key) returns true for expired entries that haven't been lazy-deleted yet, while get(key) returns undefined for the same key. This breaks the API contract — callers who guard on has() before calling get() will get unexpected undefined.
Suggestion: Reuse the expiry-check logic from get(): const entry = this.store.get(key); if (!entry) return false; if (Date.now() > entry.expiresAt) { this.store.delete(key); return false; } return true;
[WARNING] correctness
File: src/utils/cache.ts:32
ttlMs || this.defaultTtlMs uses logical-OR, which is falsy-coalescing. If a caller explicitly passes ttlMs: 0 (intending immediate or near-immediate expiry), the 0 is falsy and falls through to defaultTtlMs, silently caching for the default duration instead.
Suggestion: Use nullish coalescing: ttlMs ?? this.defaultTtlMs. This only falls back when ttlMs is null or undefined, preserving 0 as a valid value.
[WARNING] correctness
File: src/utils/cache.ts:76
Number(trimmed.split("=")[1]) produces NaN for malformed directives like max-age=abc. This NaN (multiplied by 1000) propagates into result.maxAge, and if used downstream as a TTL, Date.now() + NaN = NaN, making the entry effectively inaccessible via get() (since Date.now() > NaN is false).
Suggestion: Validate the parsed value before assignment: const parsed = Number(trimmed.split("=")[1]); if (Number.isFinite(parsed)) result.maxAge = parsed * 1000;
[WARNING] testing
File: src/utils/cache.ts
No test file accompanies this new utility module. The cache has non-trivial logic (lazy expiry, size-based eviction, header parsing with edge cases) that should be covered.
Suggestion: Add a test file (e.g., src/utils/cache.test.ts) covering: TTL expiry on get/has, eviction at max size, ttlMs=0 behavior, parseCacheControl with valid/invalid inputs, and the no-cache/no-store directives.
[INFO] correctness
File: src/utils/cache.ts:48
The size getter returns this.store.size, which includes expired entries that haven't been lazily cleaned up yet. Callers may see a size that doesn't reflect the number of actually-valid entries.
Suggestion: Document that size includes expired-but-not-yet-reaped entries, or consider filtering expired entries (though that would be O(n)).
Reviewed by pi-relay · View full run
| }); | ||
| } | ||
|
|
||
| has(key: string): boolean { |
There was a problem hiding this comment.
[WARNING] correctness
has() delegates directly to Map.has() without checking expiry. This means has(key) returns true for expired entries that haven't been lazy-deleted yet, while get(key) returns undefined for the same key. This breaks the API contract — callers who guard on has() before calling get() will get unexpected undefined.
Suggestion: Reuse the expiry-check logic from get(): const entry = this.store.get(key); if (!entry) return false; if (Date.now() > entry.expiresAt) { this.store.delete(key); return false; } return true;
| } | ||
| this.store.set(key, { | ||
| value, | ||
| expiresAt: Date.now() + (ttlMs || this.defaultTtlMs), |
There was a problem hiding this comment.
[WARNING] correctness
ttlMs || this.defaultTtlMs uses logical-OR, which is falsy-coalescing. If a caller explicitly passes ttlMs: 0 (intending immediate or near-immediate expiry), the 0 is falsy and falls through to defaultTtlMs, silently caching for the default duration instead.
Suggestion: Use nullish coalescing: ttlMs ?? this.defaultTtlMs. This only falls back when ttlMs is null or undefined, preserving 0 as a valid value.
| result.noCache = true; | ||
| } | ||
| if (trimmed.startsWith("max-age=")) { | ||
| result.maxAge = Number(trimmed.split("=")[1]) * 1000; |
There was a problem hiding this comment.
[WARNING] correctness
Number(trimmed.split("=")[1]) produces NaN for malformed directives like max-age=abc. This NaN (multiplied by 1000) propagates into result.maxAge, and if used downstream as a TTL, Date.now() + NaN = NaN, making the entry effectively inaccessible via get() (since Date.now() > NaN is false).
Suggestion: Validate the parsed value before assignment: const parsed = Number(trimmed.split("=")[1]); if (Number.isFinite(parsed)) result.maxAge = parsed * 1000;
| this.store.clear(); | ||
| } | ||
|
|
||
| get size(): number { |
There was a problem hiding this comment.
[INFO] correctness
The size getter returns this.store.size, which includes expired entries that haven't been lazily cleaned up yet. Callers may see a size that doesn't reflect the number of actually-valid entries.
Suggestion: Document that size includes expired-but-not-yet-reaped entries, or consider filtering expired entries (though that would be O(n)).
![pi-relay[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
AI Review: Fixes AppliedReviewed and found 5 issue(s). All addressed. Changes MadeFixed all 4 warning-severity findings in src/utils/cache.ts:
The info-severity finding about Verification passed. |
|
Test PR — verified end-to-end flow with thinking=high. Closing. |
1 participant
Adds a generic TTL cache with size-based eviction and a Cache-Control header parser.