-
Notifications
You must be signed in to change notification settings - Fork 7.7k
Fix Redis caching always returning False due to unhandled string values #7022
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
Conversation
@ekzhu 👋 This repository doesn't have Copilot instructions. With Copilot instructions, I can understand the repository better, work faster and produce higher quality PRs. I can generate a .github/copilot-instructions.md file for you automatically. Click here to open a pre-filled issue and assign it to me. I'll write the instructions, and then tag you for review. |
Co-authored-by: ekzhu <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7022 +/- ##
=======================================
Coverage 80.93% 80.93%
=======================================
Files 237 237
Lines 18240 18258 +18
=======================================
+ Hits 14762 14778 +16
- Misses 3478 3480 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The Redis caching implementation was experiencing cache misses even when data existed in Redis, causing unnecessary LLM API calls and always returning
cached=False
. This occurred because the_check_cache
method inChatCompletionCache
didn't handle string values returned by the Redis store.Problem
When using Redis with
decode_responses=True
or when Redis returns string data due to JSON decode errors, theRedisStore.get()
method would return string values instead of dictionaries orCreateResult
objects. The_check_cache
method only handled:dict
objects (converted toCreateResult
)list
objects (processed for streaming scenarios)CreateResult
orlist
)When a string was returned, the method would fall through and return
None, cache_key
, causing a cache miss even though valid data existed in Redis.Reproduction
Solution
Enhanced the
_check_cache
method to handle string values by:isinstance(cached_result, str)
checkjson.loads()
CreateResult
objectsThe fix ensures that when Redis returns string representations of cached data, they are properly deserialized back into
CreateResult
objects, allowing cache hits to work correctly.Testing
Added comprehensive test coverage including:
CreateResult
objectsAfter the fix, the reproduction case now correctly shows:
cached=False
(LLM called)cached=True
(cache used)Fixes #6984.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.