From 4e05e629da0fce1fc484da514788cc0bad57372b Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 5 Feb 2026 01:08:50 +0000 Subject: [PATCH] fix(chats): prevent null cache poisoning in app state sync key lookup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CRITICAL FIX: Addresses Codex Bot and Copilot AI review comments from PR #81 PROBLEM IDENTIFIED (Codex Bot): When getCachedAppStateSyncKey() doesn't find a key in DB, it cached null with 1h TTL. Later, when APP_STATE_SYNC_KEY_SHARE arrives with that key, the cached null blocks the newly stored key for up to 1 hour, causing sync failures. RACE CONDITION SCENARIO: T=0s: decodeSyncdSnapshot() needs keyId_ABC → DB miss → cache null (TTL 1h) T=5s: APP_STATE_SYNC_KEY_SHARE stores keyId_ABC in DB T=10s: decodeSyncdSnapshot() needs keyId_ABC → cache hit → returns null ❌ SYNC FAILS even though key exists in DB! FIXES APPLIED: 1. CRITICAL (Codex Bot): Only cache non-null values - Prevents stale null from blocking newly arrived keys - Missing keys can now be found after APP_STATE_SYNC_KEY_SHARE 2. MEDIUM (Copilot AI Comment C): Fix race between has() and get() - Use get() directly instead of has() + get() - Prevents key from expiring/evicting between checks 3. LOW (Copilot AI Comment B): Use constants from Defaults - Changed max: 1000 → DEFAULT_CACHE_MAX_KEYS.SIGNAL_STORE (10,000) - Changed ttl: 60*60*1000 → DEFAULT_CACHE_TTLS.MSG_RETRY * 1000 - Maintains consistency with codebase patterns IMPACT: - Eliminates critical sync failure scenario - Maintains performance benefits (5x faster sync) - Increases cache size to 10k (better hit rate for large syncs) TESTING: - Verified null values are not cached - Verified APP_STATE_SYNC_KEY_SHARE can now update missing keys - Verified constants are correctly imported and used Review Comments Addressed: - Codex Bot: Cache invalidation ✅ FIXED - Copilot AI Comment B: Hardcoded constants ✅ FIXED - Copilot AI Comment C: Race condition ✅ FIXED https://claude.ai/code/session_01NTVq3RHgGpgKL289JGvw55 --- src/Socket/chats.ts | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/src/Socket/chats.ts b/src/Socket/chats.ts index 6c1f57af..82cd2626 100644 --- a/src/Socket/chats.ts +++ b/src/Socket/chats.ts @@ -2,7 +2,7 @@ import NodeCache from '@cacheable/node-cache' import { Boom } from '@hapi/boom' import { LRUCache } from 'lru-cache' import { proto } from '../../WAProto/index.js' -import { DEFAULT_CACHE_TTLS, PROCESSABLE_HISTORY_TYPES } from '../Defaults' +import { DEFAULT_CACHE_TTLS, DEFAULT_CACHE_MAX_KEYS, PROCESSABLE_HISTORY_TYPES } from '../Defaults' import type { BotListInfo, CacheStore, @@ -111,12 +111,12 @@ export const makeChatsSocket = (config: SocketConfig) => { * App State Sync Key Cache with LRU eviction policy * Prevents repeated database lookups for same keys during sync operations. * - * MEMORY SAFETY: Limited to 1000 entries with 1-hour TTL to prevent unbounded growth. + * MEMORY SAFETY: Limited by DEFAULT_CACHE_MAX_KEYS.SIGNAL_STORE with 1-hour TTL. * Auto-purges expired entries to maintain memory bounds. */ const appStateSyncKeyCache = new LRUCache({ - max: 1000, // Limit total number of cached keys - ttl: 60 * 60 * 1000, // 1 hour TTL per key + max: DEFAULT_CACHE_MAX_KEYS.SIGNAL_STORE, // Use constant from Defaults (10,000) + ttl: DEFAULT_CACHE_TTLS.MSG_RETRY * 1000, // 1 hour TTL (convert seconds to ms) ttlAutopurge: true, // Automatically remove expired entries updateAgeOnGet: true // LRU refresh on access }) @@ -127,18 +127,28 @@ export const makeChatsSocket = (config: SocketConfig) => { * * Performance: 5x faster sync operations by eliminating redundant key fetches. * Memory: Bounded by LRU policy (max 1000 keys, 1h TTL) + * + * CRITICAL FIX: Only cache successful lookups (non-null values) to prevent + * stale null values from blocking newly arrived keys via APP_STATE_SYNC_KEY_SHARE. */ const getCachedAppStateSyncKey = async (keyId: string) => { - // Check cache first - if (appStateSyncKeyCache.has(keyId)) { - return appStateSyncKeyCache.get(keyId) ?? undefined + // Use get() directly to avoid race between has() and get() (Fix: Copilot C) + const cached = appStateSyncKeyCache.get(keyId) + if (cached !== undefined) { + // Null in cache means we explicitly cached a null (which we don't do anymore) + // Undefined means not in cache + return cached ?? undefined } // Cache miss - fetch from database const key = await getAppStateSyncKey(keyId) - // Store in cache (null is cached to prevent repeated lookups for missing keys) - appStateSyncKeyCache.set(keyId, key ?? null) + // CRITICAL: Only cache non-null values + // Null/undefined means key doesn't exist YET, but may arrive via APP_STATE_SYNC_KEY_SHARE + // If we cache null, the cache (TTL 1h) will block newly arrived keys + if (key) { + appStateSyncKeyCache.set(keyId, key) + } return key }