fix: resolve cache poisoning causing silent failures on container restart
Root Cause: Cache Poisoning - getNode() checked cache BEFORE any v3.37.6 logging (early return) - Cache likely contained null values from previous failed attempts - Early return skipped ALL diagnostic logging (100% silent failure) Direct GCS SDK Test Proved Files Exist: - Same files, credentials, paths work perfectly with direct @google-cloud/storage calls - Files exist in GCS with valid JSON (manually verified) - Brainy's getNode() returned null with NO logs - This confirmed the bug was in Brainy's caching logic, not GCS Fixes Applied to Both GCS and S3 Storage Adapters: 1. Cache Check Logging (Reveal Poisoning): - Log cache state BEFORE returning cached value - Show if cache contains null, undefined, or valid object - Helps diagnose what's being cached 2. Never Cache Null Values: - Only return cached value if it's valid (not null/undefined) - Only cache nodes after successful load with validation - Never cache null results from 404 errors - Explicit logging when NOT caching invalid nodes 3. Clear Cache on Init (Fresh Start): - Clear all cache entries during storage initialization - Ensures containers start with fresh cache (no stale nulls) - Prevents cache poisoning from persisting across restarts Expected Outcome: - Container restart will now successfully load entities from storage - Cache poisoning cannot cause silent failures - Comprehensive logging will reveal any remaining issues - Same fix benefits both GCS and S3 users
This commit is contained in:
parent
06069768ee
commit
02b4dcc211
2 changed files with 75 additions and 6 deletions
|
|
@ -350,6 +350,16 @@ export class S3CompatibleStorage extends BaseStorage {
|
|||
// Initialize counts from storage
|
||||
await this.initializeCounts()
|
||||
|
||||
// CRITICAL FIX (v3.37.7): Clear any stale cache entries from previous runs
|
||||
// This prevents cache poisoning from causing silent failures on container restart
|
||||
const nodeCacheSize = this.nodeCache?.size || 0
|
||||
if (nodeCacheSize > 0) {
|
||||
prodLog.info(`🧹 Clearing ${nodeCacheSize} cached node entries from previous run`)
|
||||
this.nodeCache.clear()
|
||||
} else {
|
||||
prodLog.info('🧹 Node cache is empty - starting fresh')
|
||||
}
|
||||
|
||||
this.isInitialized = true
|
||||
this.logger.info(`Initialized ${this.serviceType} storage with bucket ${this.bucketName}`)
|
||||
} catch (error) {
|
||||
|
|
@ -1073,6 +1083,28 @@ export class S3CompatibleStorage extends BaseStorage {
|
|||
protected async getNode(id: string): Promise<HNSWNode | null> {
|
||||
await this.ensureInitialized()
|
||||
|
||||
// Check cache first WITH LOGGING
|
||||
const cached = this.nodeCache.get(id)
|
||||
|
||||
// DIAGNOSTIC LOGGING: Reveal cache poisoning
|
||||
prodLog.info(`[getNode] 🔍 Cache check for ${id.substring(0, 8)}...:`, {
|
||||
hasCached: cached !== undefined,
|
||||
isNull: cached === null,
|
||||
isObject: cached !== null && typeof cached === 'object',
|
||||
type: typeof cached
|
||||
})
|
||||
|
||||
// CRITICAL FIX: Only return cached value if it's valid (not null/undefined)
|
||||
if (cached !== undefined && cached !== null) {
|
||||
prodLog.info(`[getNode] ✅ Cache HIT - returning cached node for ${id.substring(0, 8)}...`)
|
||||
this.logger.trace(`Cache hit for node ${id}`)
|
||||
return cached
|
||||
} else if (cached === null) {
|
||||
prodLog.warn(`[getNode] ⚠️ Cache contains NULL for ${id.substring(0, 8)}... - ignoring and loading from S3`)
|
||||
} else {
|
||||
prodLog.info(`[getNode] ❌ Cache MISS - loading from S3 for ${id.substring(0, 8)}...`)
|
||||
}
|
||||
|
||||
try {
|
||||
// Import the GetObjectCommand only when needed
|
||||
const { GetObjectCommand } = await import('@aws-sdk/client-s3')
|
||||
|
|
@ -1137,6 +1169,14 @@ export class S3CompatibleStorage extends BaseStorage {
|
|||
level: parsedNode.level || 0
|
||||
}
|
||||
|
||||
// CRITICAL FIX: Only cache valid nodes (never cache null)
|
||||
if (node && node.id && node.vector && Array.isArray(node.vector)) {
|
||||
this.nodeCache.set(id, node)
|
||||
prodLog.info(`[getNode] 💾 Cached node ${id.substring(0, 8)}... successfully`)
|
||||
} else {
|
||||
prodLog.warn(`[getNode] ⚠️ NOT caching invalid node for ${id.substring(0, 8)}...`)
|
||||
}
|
||||
|
||||
this.logger.trace(`Successfully retrieved node ${id}`)
|
||||
return node
|
||||
} catch (error: any) {
|
||||
|
|
@ -1155,7 +1195,8 @@ export class S3CompatibleStorage extends BaseStorage {
|
|||
|
||||
// Check if this is a "not found" error (S3 uses "NoSuchKey")
|
||||
if (error?.name === 'NoSuchKey' || error?.Code === 'NoSuchKey' || error?.$metadata?.httpStatusCode === 404) {
|
||||
prodLog.warn(`[getNode] Identified as 404/NoSuchKey error - returning null`)
|
||||
prodLog.warn(`[getNode] Identified as 404/NoSuchKey error - returning null WITHOUT caching`)
|
||||
// CRITICAL FIX: Do NOT cache null values
|
||||
return null
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue