From e0e39e475bbe5d314cc514b8be6f61583878bd65 Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Tue, 16 Sep 2025 02:30:08 -0700 Subject: [PATCH] Fix Redis caching always returning False due to unhandled string values (#7022) Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: ekzhu <320302+ekzhu@users.noreply.github.com> Co-authored-by: Eric Zhu --- .../models/cache/_chat_completion_cache.py | 25 ++++ .../models/test_chat_completion_cache.py | 123 ++++++++++++++++++ 2 files changed, 148 insertions(+) diff --git a/python/packages/autogen-ext/src/autogen_ext/models/cache/_chat_completion_cache.py b/python/packages/autogen-ext/src/autogen_ext/models/cache/_chat_completion_cache.py index 4bd324872..124a3ea76 100644 --- a/python/packages/autogen-ext/src/autogen_ext/models/cache/_chat_completion_cache.py +++ b/python/packages/autogen-ext/src/autogen_ext/models/cache/_chat_completion_cache.py @@ -223,6 +223,31 @@ class ChatCompletionCache(ChatCompletionClient, Component[ChatCompletionCacheCon except ValidationError: # If reconstruction fails, treat as cache miss return None, cache_key + elif isinstance(cached_result, str): + # Handle case where cache store returns a string (e.g., Redis with decode errors) + try: + # Try to parse the string as JSON and reconstruct CreateResult + parsed_data = json.loads(cached_result) + if isinstance(parsed_data, dict): + cached_result = CreateResult.model_validate(parsed_data) + elif isinstance(parsed_data, list): + # Handle streaming results stored as JSON string + reconstructed_list_2: list[CreateResult | str] = [] + for item in parsed_data: # type: ignore[reportUnknownVariableType] + if isinstance(item, dict): + reconstructed_list_2.append(CreateResult.model_validate(item)) + elif isinstance(item, str): + reconstructed_list_2.append(item) + else: + # If item is neither dict nor str, treat as cache miss + return None, cache_key + cached_result = reconstructed_list_2 + else: + # If parsed data is not dict or list, treat as cache miss + return None, cache_key + except (json.JSONDecodeError, ValidationError): + # If JSON parsing or validation fails, treat as cache miss + return None, cache_key # If it's already the right type (CreateResult or list), return as-is return cached_result, cache_key diff --git a/python/packages/autogen-ext/tests/models/test_chat_completion_cache.py b/python/packages/autogen-ext/tests/models/test_chat_completion_cache.py index c0730904e..8627cb9f6 100644 --- a/python/packages/autogen-ext/tests/models/test_chat_completion_cache.py +++ b/python/packages/autogen-ext/tests/models/test_chat_completion_cache.py @@ -1,4 +1,5 @@ import copy +import json from typing import Any, Dict, List, Optional, Tuple, Union, cast import pytest @@ -482,6 +483,128 @@ def test_check_cache_already_correct_type() -> None: assert cache_key is not None +def test_check_cache_string_json_deserialization_success() -> None: + """Test _check_cache when Redis cache returns a string containing valid JSON. + This tests the fix for the Redis string caching issue where Redis returns + string data instead of dict/CreateResult, causing cache misses. + """ + _, prompts, system_prompt, replay_client, _ = get_test_data() + + # Create a JSON string representing a valid CreateResult + create_result_json = json.dumps( + { + "content": "response from string json", + "usage": {"prompt_tokens": 12, "completion_tokens": 6}, + "cached": False, + "finish_reason": "stop", + "logprobs": None, + "thought": None, + } + ) + + # Mock cache store that returns the JSON string (simulating Redis behavior) + mock_store = MockCacheStore(return_value=cast(Any, create_result_json)) + cached_client = ChatCompletionCache(replay_client, mock_store) + + # Test _check_cache method directly + messages = [system_prompt, UserMessage(content=prompts[0], source="user")] + cached_result, cache_key = cached_client._check_cache(messages, [], None, {}) # type: ignore + + # Should successfully reconstruct the CreateResult from JSON string + assert cached_result is not None + assert isinstance(cached_result, CreateResult) + assert cached_result.content == "response from string json" + assert cached_result.usage.prompt_tokens == 12 + assert cached_result.usage.completion_tokens == 6 + assert cache_key is not None + + +def test_check_cache_string_json_list_deserialization_success() -> None: + """Test _check_cache when Redis cache returns a string containing valid JSON list. + This tests the fix for streaming results stored as JSON strings in Redis. + """ + _, prompts, system_prompt, replay_client, _ = get_test_data() + + # Create a JSON string representing a streaming result list + streaming_list_json = json.dumps( + [ + "streaming chunk 1", + { + "content": "streaming response from json", + "usage": {"prompt_tokens": 8, "completion_tokens": 4}, + "cached": False, + "finish_reason": "stop", + "logprobs": None, + "thought": None, + }, + "streaming chunk 2", + ] + ) + + # Mock cache store that returns the JSON string (simulating Redis streaming) + mock_store = MockCacheStore(return_value=cast(Any, streaming_list_json)) + cached_client = ChatCompletionCache(replay_client, mock_store) + + # Test _check_cache method directly + messages = [system_prompt, UserMessage(content=prompts[0], source="user")] + cached_result, cache_key = cached_client._check_cache(messages, [], None, {}) # type: ignore + + # Should successfully reconstruct the list from JSON string + assert cached_result is not None + assert isinstance(cached_result, list) + assert len(cached_result) == 3 + assert cached_result[0] == "streaming chunk 1" + assert isinstance(cached_result[1], CreateResult) + assert cached_result[1].content == "streaming response from json" + assert cached_result[2] == "streaming chunk 2" + assert cache_key is not None + + +def test_check_cache_string_invalid_json_failure() -> None: + """Test _check_cache gracefully handles invalid JSON strings. + This ensures the system degrades gracefully when Redis returns corrupted + string data that cannot be parsed as JSON. + """ + _, prompts, system_prompt, replay_client, _ = get_test_data() + + # Create an invalid JSON string + invalid_json_string = '{"content": "test", invalid json}' + + # Mock cache store that returns the invalid JSON string + mock_store = MockCacheStore(return_value=cast(Any, invalid_json_string)) + cached_client = ChatCompletionCache(replay_client, mock_store) + + # Test _check_cache method directly + messages = [system_prompt, UserMessage(content=prompts[0], source="user")] + cached_result, cache_key = cached_client._check_cache(messages, [], None, {}) # type: ignore + + # Should return None (cache miss) when JSON parsing fails + assert cached_result is None + assert cache_key is not None + + +def test_check_cache_string_invalid_data_failure() -> None: + """Test _check_cache gracefully handles JSON strings with invalid data structure. + This ensures the system handles JSON that parses but doesn't represent valid CreateResult data. + """ + _, prompts, system_prompt, replay_client, _ = get_test_data() + + # Create a JSON string that parses but has invalid structure + invalid_data_json = json.dumps({"invalid_structure": "not a CreateResult"}) + + # Mock cache store that returns the invalid data JSON string + mock_store = MockCacheStore(return_value=cast(Any, invalid_data_json)) + cached_client = ChatCompletionCache(replay_client, mock_store) + + # Test _check_cache method directly + messages = [system_prompt, UserMessage(content=prompts[0], source="user")] + cached_result, cache_key = cached_client._check_cache(messages, [], None, {}) # type: ignore + + # Should return None (cache miss) when validation fails + assert cached_result is None + assert cache_key is not None + + @pytest.mark.asyncio async def test_redis_streaming_cache_integration() -> None: """Integration test for Redis streaming cache scenario.