-
Notifications
You must be signed in to change notification settings - Fork 241
fix: stop returning expired in-memory cache hits #423
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -230,28 +230,34 @@ func (c *InMemoryCache) FindSimilar(model string, query string) ([]byte, bool, e | |
} | ||
|
||
c.mu.RLock() | ||
|
||
// Check for expired entries during search | ||
c.cleanupExpiredEntriesReadOnly() | ||
|
||
var ( | ||
bestIndex = -1 | ||
bestEntry CacheEntry | ||
bestSimilarity float32 | ||
entriesChecked int | ||
expiredCount int | ||
) | ||
// Capture the lookup time after acquiring the read lock so TTL checks aren’t skewed by embedding work or lock wait | ||
now := time.Now() | ||
|
||
// Compare with completed entries for the same model, tracking only the best match | ||
for entryIndex, entry := range c.entries { | ||
// Skip incomplete entries | ||
if entry.ResponseBody == nil { | ||
continue // Skip incomplete entries | ||
continue | ||
} | ||
|
||
// Only consider entries for the same model | ||
if entry.Model != model { | ||
continue | ||
} | ||
|
||
// Skip entries that have expired before considering them | ||
if c.isExpired(entry, now) { | ||
expiredCount++ | ||
continue | ||
} | ||
|
||
// Compute semantic similarity using dot product | ||
var dotProduct float32 | ||
for i := 0; i < len(queryEmbedding) && i < len(entry.Embedding); i++ { | ||
|
@@ -272,6 +278,17 @@ func (c *InMemoryCache) FindSimilar(model string, query string) ([]byte, bool, e | |
// Unlock the read lock since we need the write lock to update the access info | ||
c.mu.RUnlock() | ||
|
||
// Log if any expired entries were skipped | ||
if expiredCount > 0 { | ||
observability.Debugf("InMemoryCache: excluded %d expired entries during search (TTL: %ds)", | ||
expiredCount, c.ttlSeconds) | ||
observability.LogEvent("cache_expired_entries_found", map[string]interface{}{ | ||
"backend": "memory", | ||
"expired_count": expiredCount, | ||
"ttl_seconds": c.ttlSeconds, | ||
}) | ||
} | ||
|
||
// Handle case where no suitable entries exist | ||
if bestIndex < 0 { | ||
atomic.AddInt64(&c.missCount, 1) | ||
|
@@ -371,7 +388,7 @@ func (c *InMemoryCache) cleanupExpiredEntries() { | |
|
||
for _, entry := range c.entries { | ||
// Retain entries that are still within their TTL based on last access | ||
if now.Sub(entry.LastAccessAt).Seconds() < float64(c.ttlSeconds) { | ||
if !c.isExpired(entry, now) { | ||
validEntries = append(validEntries, entry) | ||
} | ||
} | ||
|
@@ -397,31 +414,13 @@ func (c *InMemoryCache) cleanupExpiredEntries() { | |
metrics.UpdateCacheEntries("memory", len(c.entries)) | ||
} | ||
|
||
// cleanupExpiredEntriesReadOnly identifies expired entries without modifying the cache | ||
// Used during read operations with only a read lock held | ||
func (c *InMemoryCache) cleanupExpiredEntriesReadOnly() { | ||
// isExpired checks if a cache entry has expired based on its last access time | ||
func (c *InMemoryCache) isExpired(entry CacheEntry, now time.Time) bool { | ||
if c.ttlSeconds <= 0 { | ||
return | ||
} | ||
|
||
now := time.Now() | ||
expiredCount := 0 | ||
|
||
for _, entry := range c.entries { | ||
if now.Sub(entry.LastAccessAt).Seconds() >= float64(c.ttlSeconds) { | ||
expiredCount++ | ||
} | ||
return false | ||
} | ||
|
||
if expiredCount > 0 { | ||
observability.Debugf("InMemoryCache: found %d expired entries during read (TTL: %ds)", | ||
expiredCount, c.ttlSeconds) | ||
observability.LogEvent("cache_expired_entries_found", map[string]interface{}{ | ||
"backend": "memory", | ||
"expired_count": expiredCount, | ||
"ttl_seconds": c.ttlSeconds, | ||
}) | ||
} | ||
return now.Sub(entry.LastAccessAt) >= time.Duration(c.ttlSeconds)*time.Second | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The conversion Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||
} | ||
|
||
// updateAccessInfo updates the access information for the given entry index | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hardcoded sleep duration of 1100ms (1.1 seconds) relies on a magic number. Consider defining this as a constant like
ttlWaitDuration = 1100 * time.Millisecond
or calculating it astime.Duration(ttlSeconds)*time.Second + 100*time.Millisecond
to make the relationship to the TTL explicit.Copilot uses AI. Check for mistakes.