fix(caching): forward metadata to valkey semantic cache sync embedding calls - #35441
Open
nickleodoen wants to merge 1 commit into
Open
Conversation
ValkeySemanticCache.set_cache/get_cache called _get_embedding(prompt) without metadata, so the sync path dropped the request metadata that _get_embedding forwards to build_router_embedding_metadata(). Router-served embedding deployments therefore lost per-deployment auth context (user_api_key, team id, Bedrock aws_role_name) on sync completion() calls, and because both methods swallow exceptions the resulting failures were a silent cache bypass. The async twins were fixed in BerriAI#32295 but the sync callers were missed. Both sibling backends already pass metadata on their sync paths (RedisSemanticCache 393/430, QdrantSemanticCache 246/285), so Valkey was the only backend diverging. Adds sync regression tests mirroring the existing async ones. Both fail on the unfixed code with metadata == None.
Contributor
Greptile SummaryThis PR aligns synchronous Valkey semantic-cache embedding calls with the existing Redis, Qdrant, and asynchronous Valkey paths
Confidence Score: 5/5The PR appears safe to merge; the implementation matches established sibling paths and the regression tests cover both corrected call sites The inherited helper accepts optional metadata, copies it before adding semantic-cache context, and the new calls now behave consistently with Redis, Qdrant, and asynchronous Valkey implementations
|
| Filename | Overview |
|---|---|
| litellm/caching/valkey_semantic_cache.py | The two synchronous embedding calls now forward metadata using the established semantic-cache backend pattern, with no identified regression |
| tests/test_litellm/caching/test_valkey_semantic_cache.py | Adds focused tests confirming metadata reaches both synchronous embedding call sites while preserving cache read and write behavior |
Reviews (1): Last reviewed commit: "fix(caching): forward metadata to valkey..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem this solves:
How it solves it:
metadata=kwargs.get("metadata")in syncset_cache/get_cacheRelevant issues
Follow-up to #32295, which fixed
_get_async_embeddingat lines 282/301 but left thetwo sync
_get_embeddingcallers at lines 243/262 untouched.Type
🐛 Bug Fix
Changes
ValkeySemanticCache.set_cacheandget_cachecalledself._get_embedding(prompt)with no
metadata. The inheritedRedisSemanticCache._get_embeddingfeeds thatargument to
build_router_embedding_metadata()beforerouter.embedding(...), and itsown docstring says it does so "mirroring
_get_async_embedding".Across the three semantic-cache backends there are six sync embedding call sites.
Valkey's two were the only ones dropping metadata:
qdrant_semantic_cache.pyredis_semantic_cache.pyvalkey_semantic_cache.pyImpact: on a sync
completion()againstcache_type: valkey-semanticwith aRouter-served embedding deployment, the embedding call loses
user_api_key,user_api_key_team_id, Bedrockaws_role_name, etc. Per-deployment auth can then fail,and since
set_cache/get_cacheswallow exceptions the failure surfaces only as apermanently cold cache. The async path was already correct, so this is sync-only.
Scope is limited to the two argument lists plus tests.
Pre-Submission checklist
Screenshots / Proof of Fix
A production e2e needs ElastiCache for Valkey 8.2+ running valkey-search, which I don't
have access to. Instead, below is a fully unmocked reproduction exercising the real
code path: a real
litellm.Router, a realValkeySemanticCache, real proxy globals, anda real litellm
CustomLoggerobserving what the Router actually received. Nothing ispatched or stubbed. Embeddings are served by a local HTTP server implementing the OpenAI
embeddings API, so the run costs $0. Happy to attach the script if useful.
Unit tests (
tests/test_litellm/caching, base 23de7a1):2 failed—assert None == {'user_api_key': 'sk-test'}263 passed(261 pre-existing + 2 new)Final Attestation