Repository navigation
Add two-tier conversation caching with race condition and duplicate fixes - #633
Conversation
…ixes Implements L1 (memory) + L2 (Redis) conversation caching to reduce database load for active AI chat sessions. Key fixes in this commit: - P1: Await appendMessage to prevent race condition where getConversation reads stale data before async Redis write completes - P2: Add upsert logic in appendMessage to deduplicate messages on retry, mirroring the DB's onConflictDoUpdate behavior Features: - ConversationCache service with automatic TTL and cleanup - Graceful Redis degradation (falls back to memory-only) - Cache invalidation on message edit/delete - Comprehensive test coverage (36 tests) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR introduces a comprehensive two-tier conversation caching system (in-memory L1 + optional Redis L2) for AI chat conversations. Cache invalidation is integrated across message update/delete operations, and cache population occurs on conversation reads with fallback to database. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ChatAPI as Chat API Route
participant Cache as ConversationCache<br/>(L1 Memory)
participant Redis as Redis<br/>(L2)
participant Database
Client->>ChatAPI: GET /chat (conversationId, pageId)
ChatAPI->>Cache: getConversation(pageId, conversationId)
alt Cache Hit
Cache-->>ChatAPI: return CachedConversation
ChatAPI-->>Client: 200 OK (from cache)
else Cache Miss
Cache->>Database: fetch conversation messages
Database-->>Cache: messages[]
Cache->>Cache: convert to CachedMessage[]
Cache->>Cache: store in L1 memory
Cache->>Redis: async setConversation (fire-and-forget)
Redis-->>Cache: ✓ (async)
Cache-->>ChatAPI: return messages
ChatAPI-->>Client: 200 OK (from database)
end
Client->>ChatAPI: PATCH /chat/messages/[messageId]
ChatAPI->>Database: update message content
Database-->>ChatAPI: ✓
ChatAPI->>Cache: invalidateConversation(pageId, conversationId)
Cache->>Cache: remove from L1
Cache->>Redis: async delete (fire-and-forget)
ChatAPI-->>Client: 200 OK
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 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 |
![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
🤖 Fix all issues with AI agents
In `@apps/web/src/app/api/ai/chat/route.ts`:
- Around line 649-722: The cache population step currently hardcodes messageType
to 'standard' inside the messagesToCache mapping, losing any actual
msg.messageType (e.g., 'todo_list'); update the mapping that builds
messagesToCache (used before calling conversationCache.setConversation) to read
and pass msg.messageType directly from the DB row (from dbMessages /
msg.messageType) so the cached CachedMessage.messageType preserves the original
enum value; ensure the mapping still falls back safely if msg.messageType is
null/undefined.
🧹 Nitpick comments (4)
packages/lib/src/services/conversation-cache.ts (4)
380-400: UseSCANinstead ofKEYSfor production Redis safety; fix Biome lint warning.Two items on this section:
Redis
KEYScommand (Line 386):KEYSis O(N) over the entire keyspace and blocks the Redis server. While the scoped pattern limits matches, this can still cause latency spikes if the Redis instance has many keys. Consider usingSCANwith a cursor-based iteration instead. The same concern applies toclearAllat Line 444.Biome lint (Line 380):
Map.delete()returns a boolean thatforEachdiscards. Use afor...ofloop instead.Suggested fixes
For the lint fix:
- keysToDelete.forEach(key => this.memoryCache.delete(key)); + for (const key of keysToDelete) { + this.memoryCache.delete(key); + }For the
KEYS→SCANmigration (if desired):// Helper to iterate with SCAN instead of KEYS private async scanKeys(pattern: string): Promise<string[]> { if (!this.redis) return []; const keys: string[] = []; let cursor = '0'; do { const [nextCursor, batch] = await this.redis.scan(cursor, 'MATCH', pattern, 'COUNT', 100); cursor = nextCursor; keys.push(...batch); } while (cursor !== '0'); return keys; }
136-170: Cleanup metrics conflate TTL expirations with size-limit evictions.Line 160 increments
ttlExpirationsfor both expired entries and entries evicted due to the max memory limit. This makes the metric less meaningful for monitoring. Consider tracking size-limit evictions separately.
85-93: SingletongetInstancesilently ignores config on subsequent calls.If
getInstanceis called with a config after the singleton already exists, the provided config is silently ignored. This is inherent to the singleton pattern, but consumers may not realize their config isn't applied. Consider logging a warning when config is passed but the instance already exists, similar to how this is handled in other cache services.
175-212: L2→L1 promotion doesn't validate TTL before promoting.When a Redis L2 hit is promoted to L1 (Line 198), the entry's
cachedAttimestamp from Redis is used as-is. If there's clock drift or if the entry was cached long ago but Redis hasn't expired it yet, the promoted L1 entry could immediately appear expired on the nextgetConversationcall (Line 181 check). In practice, this is unlikely since Redis'sSETEXTTL and the application-level TTL are set simultaneously, but it's worth noting as a potential edge case.
- Preserve messageType from DB when populating cache (was hardcoded to 'standard') - Use SCAN instead of KEYS for Redis operations (non-blocking) - Fix Biome lint: use for...of instead of forEach for Map.delete - Separate sizeEvictions metric from ttlExpirations for clearer monitoring - Warn when getInstance config is ignored due to existing singleton Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add conversationCache.invalidateConversation mock to test files that were missing it after the cache integration was added to the routes. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary
appendMessageto prevent stale reads when conversation is in Redis but not L1appendMessageto deduplicate on retry, mirroring DB'sonConflictDoUpdateChanges
ConversationCacheservice with automatic TTL and cleanupTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Tests