Skip to content

fix(caching): pass only metadata to valkey semantic async embedding - #32295

Merged
yassin-berriai merged 2 commits into
litellm_internal_stagingfrom
litellm_fix_valkey_semantic_cache_embedding_call
Jul 7, 2026
Merged

fix(caching): pass only metadata to valkey semantic async embedding#32295
yassin-berriai merged 2 commits into
litellm_internal_stagingfrom
litellm_fix_valkey_semantic_cache_embedding_call

Conversation

@shivamrawat1

@shivamrawat1 shivamrawat1 commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

Resolves LIT-4237

Issue
valkey-semantic caching never worked through the LiteLLM proxy async path. Every request returned x-litellm-semantic-similarity: 0.0, always hit the upstream LLM (cost + llm_provider-* headers), and Valkey's semantic index stayed empty (num_docs: 0) even for identical prompts.

This affected all embedding models configured for semantic cache, not just WatsonX. The bug was present since valkey-semantic was introduced in #30675

Cause
ValkeySemanticCache.async_set_cache and async_get_cache called:

await self._get_async_embedding(prompt, **kwargs)
The parent RedisSemanticCache._get_async_embedding() only accepts (prompt, metadata=None). Passing the full kwargs dict (e.g. cache_key, custom_llm_provider, messages) raised a TypeError before any embedding was generated. That exception was caught and logged at verbose level only, so cache writes and reads silently no-op'd

The existing unit tests masked this because they replace _get_async_embedding with AsyncMock, which accepts arbitrary kwargs. The sync path was unaffected since it calls _get_embedding(prompt) without extra kwargs; the proxy uses the async path exclusively

Fix
Match redis-semantic and pass only metadata:

await self._get_async_embedding(prompt, metadata=kwargs.get("metadata"))
Added a regression test (test_async_set_cache_passes_only_metadata_to_get_async_embedding) that uses a real spy function with the correct signature instead of AsyncMock, so extra kwargs would fail the test

Proof:

Before:
Screenshot 2026-07-06 at 5 46 01 PM

After:
Screenshot 2026-07-06 at 5 48 51 PM

ValkeySemanticCache async get/set passed **kwargs into _get_async_embedding,
which raised TypeError on cache_key and other fields and silently skipped
all cache writes. Match redis-semantic by forwarding metadata only.

Co-authored-by: Cursor <cursoragent@cursor.com>
@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Shivam Rawat seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@greptile-apps

greptile-apps Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes the async path of ValkeySemanticCache where **kwargs was passed directly to _get_async_embedding, which only accepts (prompt, metadata=None), causing a TypeError that silently no-op'd every cache write and read through the proxy.

  • Core fix: Both async_set_cache and async_get_cache now call _get_async_embedding(prompt, metadata=kwargs.get("metadata")), matching the parent RedisSemanticCache signature and eliminating the silent failure.
  • Regression tests: Two new spy-based tests (test_async_set_cache_passes_only_metadata_to_get_async_embedding and test_async_get_cache_passes_only_metadata_to_get_async_embedding) use a strict-signature coroutine that would fail if extra kwargs were forwarded, closing the gap left by the original AsyncMock-based tests.

Confidence Score: 5/5

The change is a minimal, targeted fix to two symmetrical call sites that corrects a type mismatch that had made the async cache completely non-functional.

The diff is exactly four lines of production code. Both changed call sites are corrected identically, the fix matches the parent-class method signature, and the new spy-based tests would catch any re-introduction of the extra-kwargs pattern on either path. No other logic is touched.

No files require special attention.

Important Files Changed

Filename Overview
litellm/caching/valkey_semantic_cache.py Two-line fix replacing **kwargs with metadata=kwargs.get("metadata") in both async_set_cache and async_get_cache calls to _get_async_embedding, matching the parent class signature.
tests/test_litellm/caching/test_valkey_semantic_cache.py Adds two spy-based regression tests — one for async_set_cache and one for async_get_cache — that use a strict-signature coroutine instead of AsyncMock, so passing extra kwargs would raise a TypeError and expose any regression.

Reviews (2): Last reviewed commit: "test(caching): add async_get_cache embed..." | Re-trigger Greptile

@codecov

codecov Bot commented Jul 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Mirror the async_set_cache spy test so async_get_cache passing **kwargs
into _get_async_embedding is caught by a real signature, not AsyncMock.

Co-authored-by: Cursor <cursoragent@cursor.com>
@shivamrawat1

Copy link
Copy Markdown
Collaborator Author

@greptile review again with new commit that resolves p2 issue

@shivamrawat1

Copy link
Copy Markdown
Collaborator Author

bugbot run

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit ee9eef7. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants