From fad3355368b15856f2031af73b4bb9b8877a7c90 Mon Sep 17 00:00:00 2001 From: Rolando Bosch Date: Sat, 28 Feb 2026 15:19:24 -0800 Subject: [PATCH] fix: prevent in-place schema mutation from breaking cache keys (#18784) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _remove_strict_from_schema() and _remove_additional_properties() mutate tool schemas in-place via `del schema[key]`. Because the cache key is computed from the same kwargs dict before (GET) and after (SET) the provider call, the mutation causes the SET key to differ from the GET key — making the disk cache permanently miss for any request with tools containing `strict: true` or `additionalProperties: false`. The fix wraps both functions to deep-copy before mutating. Internal `_in_place` helpers preserve the recursive logic for the already-copied data. All existing callers use the `x = func(x)` return pattern, so no caller changes are needed. Affected providers: hosted_vllm, watsonx, vertex_ai/gemini. Note: the Vertex AI team already works around this with a defensive deepcopy at their call site (vertex_and_google_ai_studio_gemini.py:648). This fix makes that defensive copy redundant but harmless. Prior art: PR #9693 attempted this identical fix 14 months ago but went stale due to CLA paperwork, not technical rejection. Co-Authored-By: Claude Opus 4.6 --- litellm/utils.py | 50 ++- .../code_coverage_tests/recursive_detector.py | 2 + .../test_schema_mutation_cache_key.py | 346 ++++++++++++++++++ 3 files changed, 380 insertions(+), 18 deletions(-) create mode 100644 tests/local_testing/test_schema_mutation_cache_key.py diff --git a/litellm/utils.py b/litellm/utils.py index 02fb346bba9..9c16d04d36e 100644 --- a/litellm/utils.py +++ b/litellm/utils.py @@ -3453,44 +3453,58 @@ def _remove_additional_properties(schema): """ clean out 'additionalProperties = False'. Causes vertexai/gemini OpenAI API Schema errors - https://github.com/langchain-ai/langchainjs/issues/5240 + Returns a deep copy with 'additionalProperties' removed — the original schema + is not mutated. This is critical for cache-key stability: the cache key is + computed from the caller's original tools dict, so in-place mutation would + cause the key at GET time to differ from the key at SET time. + Relevant Issues: https://github.com/BerriAI/litellm/issues/6136, https://github.com/BerriAI/litellm/issues/6088 + See also: https://github.com/BerriAI/litellm/issues/18784 """ + schema = copy.deepcopy(schema) + _remove_additional_properties_in_place(schema) + return schema + + +def _remove_additional_properties_in_place(schema): + """In-place recursive helper — only called on an already-copied schema.""" if isinstance(schema, dict): - # Remove the 'additionalProperties' key if it exists and is set to False if "additionalProperties" in schema and schema["additionalProperties"] is False: del schema["additionalProperties"] - - # Recursively process all dictionary values for key, value in schema.items(): - _remove_additional_properties(value) - + _remove_additional_properties_in_place(value) elif isinstance(schema, list): - # Recursively process all items in the list for item in schema: - _remove_additional_properties(item) - - return schema + _remove_additional_properties_in_place(item) def _remove_strict_from_schema(schema): """ + Remove 'strict' keys from a JSON schema structure. + + Returns a deep copy with 'strict' removed — the original schema is not + mutated. This is critical for cache-key stability: the cache key is + computed from the caller's original tools dict, so in-place mutation would + cause the key at GET time to differ from the key at SET time. + Relevant Issues: https://github.com/BerriAI/litellm/issues/6136, https://github.com/BerriAI/litellm/issues/6088 + See also: https://github.com/BerriAI/litellm/issues/18784 """ + schema = copy.deepcopy(schema) + _remove_strict_from_schema_in_place(schema) + return schema + + +def _remove_strict_from_schema_in_place(schema): + """In-place recursive helper — only called on an already-copied schema.""" if isinstance(schema, dict): - # Remove the 'additionalProperties' key if it exists and is set to False if "strict" in schema: del schema["strict"] - - # Recursively process all dictionary values for key, value in schema.items(): - _remove_strict_from_schema(value) - + _remove_strict_from_schema_in_place(value) elif isinstance(schema, list): - # Recursively process all items in the list for item in schema: - _remove_strict_from_schema(item) - - return schema + _remove_strict_from_schema_in_place(item) def _remove_json_schema_refs(schema, max_depth=10): diff --git a/tests/code_coverage_tests/recursive_detector.py b/tests/code_coverage_tests/recursive_detector.py index 3710971229b..650f1be835f 100644 --- a/tests/code_coverage_tests/recursive_detector.py +++ b/tests/code_coverage_tests/recursive_detector.py @@ -4,7 +4,9 @@ import os IGNORE_FUNCTIONS = [ "_format_type", "_remove_additional_properties", + "_remove_additional_properties_in_place", "_remove_strict_from_schema", + "_remove_strict_from_schema_in_place", "filter_schema_fields", "text_completion", "_check_for_os_environ_vars", diff --git a/tests/local_testing/test_schema_mutation_cache_key.py b/tests/local_testing/test_schema_mutation_cache_key.py new file mode 100644 index 00000000000..c59651f6005 --- /dev/null +++ b/tests/local_testing/test_schema_mutation_cache_key.py @@ -0,0 +1,346 @@ +""" +Tests for https://github.com/BerriAI/litellm/issues/18784 + +_remove_strict_from_schema and _remove_additional_properties must NOT mutate +their input, because the caller's original tools dict is used to compute cache +keys both before and after the LLM provider call. If the schema is mutated +in-place, the cache key at GET time differs from the key at SET time and the +disk cache is effectively broken. +""" + +import copy +import tempfile + +import pytest + +import litellm +from litellm.caching.caching import Cache, LiteLLMCacheType +from litellm.utils import ( + _remove_additional_properties, + _remove_additional_properties_in_place, + _remove_strict_from_schema, + _remove_strict_from_schema_in_place, +) + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + +def _make_tools_with_strict(): + """Return a realistic tools list with strict=True and additionalProperties.""" + return [ + { + "type": "function", + "function": { + "name": "get_weather", + "parameters": { + "type": "object", + "properties": { + "location": {"type": "string"}, + }, + "additionalProperties": False, + }, + "strict": True, + }, + } + ] + + +# --------------------------------------------------------------------------- +# Core: in-place mutation must not happen +# --------------------------------------------------------------------------- + +class TestRemoveStrictDoesNotMutate: + """_remove_strict_from_schema must return a new object, leaving the input untouched.""" + + def test_original_unchanged_after_remove_strict(self): + tools = _make_tools_with_strict() + original = copy.deepcopy(tools) + + _remove_strict_from_schema(tools) + + assert tools == original, ( + "_remove_strict_from_schema mutated the input in-place — " + "this breaks cache-key computation (issue #18784)" + ) + + def test_return_value_has_strict_removed(self): + tools = _make_tools_with_strict() + + result = _remove_strict_from_schema(tools) + + # 'strict' should be gone from the returned copy + assert "strict" not in result[0]["function"] + + def test_original_still_has_strict(self): + tools = _make_tools_with_strict() + + _remove_strict_from_schema(tools) + + # The original must still have 'strict' + assert tools[0]["function"]["strict"] is True + + +class TestRemoveAdditionalPropertiesDoesNotMutate: + """_remove_additional_properties must return a new object, leaving the input untouched.""" + + def test_original_unchanged_after_remove_additional_properties(self): + tools = _make_tools_with_strict() + original = copy.deepcopy(tools) + + _remove_additional_properties(tools) + + assert tools == original, ( + "_remove_additional_properties mutated the input in-place — " + "this breaks cache-key computation (issue #18784)" + ) + + def test_return_value_has_additional_properties_removed(self): + tools = _make_tools_with_strict() + + result = _remove_additional_properties(tools) + + assert "additionalProperties" not in result[0]["function"]["parameters"] + + def test_original_still_has_additional_properties(self): + tools = _make_tools_with_strict() + + _remove_additional_properties(tools) + + assert tools[0]["function"]["parameters"]["additionalProperties"] is False + + +# --------------------------------------------------------------------------- +# Integration: cache key must be stable across remove_strict calls +# --------------------------------------------------------------------------- + +class TestCacheKeyStability: + """The cache key must be identical before and after _remove_strict_from_schema + is applied to the tools, because the cache GET uses the original tools and + the cache SET uses whatever is in kwargs after the provider call.""" + + def test_cache_key_matches_before_and_after_remove_strict(self): + tools = _make_tools_with_strict() + + with tempfile.TemporaryDirectory() as cache_dir: + cache = Cache(type=LiteLLMCacheType.DISK, disk_cache_dir=cache_dir) + + key_before = cache.get_cache_key( + model="hosted_vllm/test", + messages=[{"role": "user", "content": "test"}], + tools=tools, + ) + + # Simulate what the provider does internally + _remove_strict_from_schema(tools) + + key_after = cache.get_cache_key( + model="hosted_vllm/test", + messages=[{"role": "user", "content": "test"}], + tools=tools, + ) + + assert key_before == key_after, ( + f"Cache key changed after _remove_strict_from_schema: " + f"{key_before!r} != {key_after!r} — disk cache is broken (issue #18784)" + ) + + def test_cache_key_matches_before_and_after_remove_additional_properties(self): + tools = _make_tools_with_strict() + + with tempfile.TemporaryDirectory() as cache_dir: + cache = Cache(type=LiteLLMCacheType.DISK, disk_cache_dir=cache_dir) + + key_before = cache.get_cache_key( + model="hosted_vllm/test", + messages=[{"role": "user", "content": "test"}], + tools=tools, + ) + + # Simulate what the provider does internally + _remove_additional_properties(tools) + + key_after = cache.get_cache_key( + model="hosted_vllm/test", + messages=[{"role": "user", "content": "test"}], + tools=tools, + ) + + assert key_before == key_after, ( + f"Cache key changed after _remove_additional_properties: " + f"{key_before!r} != {key_after!r} — disk cache is broken (issue #18784)" + ) + + +# --------------------------------------------------------------------------- +# Edge cases +# --------------------------------------------------------------------------- + +class TestEdgeCases: + """Edge cases for the deep-copy fix.""" + + def test_nested_strict_in_parameters(self): + """strict can appear at multiple nesting levels.""" + schema = { + "strict": True, + "properties": { + "inner": { + "strict": True, + "type": "object", + } + }, + } + original = copy.deepcopy(schema) + + result = _remove_strict_from_schema(schema) + + # Original preserved + assert schema == original + # Result cleaned + assert "strict" not in result + assert "strict" not in result["properties"]["inner"] + + def test_empty_schema(self): + assert _remove_strict_from_schema({}) == {} + assert _remove_additional_properties({}) == {} + + def test_none_passthrough(self): + """Non-dict/list inputs are returned as-is.""" + assert _remove_strict_from_schema(None) is None + assert _remove_strict_from_schema("hello") == "hello" + assert _remove_strict_from_schema(42) == 42 + + def test_list_of_tools(self): + """The top-level input is often a list of tool dicts.""" + tools = _make_tools_with_strict() + original = copy.deepcopy(tools) + + result = _remove_strict_from_schema(tools) + + assert tools == original + assert "strict" not in result[0]["function"] + + def test_deeply_nested_additional_properties(self): + """additionalProperties can appear at multiple nesting levels.""" + schema = { + "type": "object", + "additionalProperties": False, + "properties": { + "outer": { + "type": "object", + "additionalProperties": False, + "properties": { + "inner": { + "type": "object", + "additionalProperties": False, + } + }, + } + }, + } + original = copy.deepcopy(schema) + + result = _remove_additional_properties(schema) + + # Original preserved at all levels + assert schema == original + # Result cleaned at all levels + assert "additionalProperties" not in result + assert "additionalProperties" not in result["properties"]["outer"] + assert "additionalProperties" not in result["properties"]["outer"]["properties"]["inner"] + + +# --------------------------------------------------------------------------- +# Sequential calls: production pattern (hosted_vllm, watsonx call both) +# --------------------------------------------------------------------------- + +class TestSequentialCalls: + """In production, providers like hosted_vllm and watsonx call both + _remove_additional_properties and _remove_strict_from_schema in sequence + on the same tools list. The original must survive both calls.""" + + def test_original_preserved_after_both_removals(self): + tools = _make_tools_with_strict() + original = copy.deepcopy(tools) + + cleaned = _remove_additional_properties(tools) + cleaned = _remove_strict_from_schema(cleaned) + + assert tools == original, "Original tools mutated by sequential removal calls" + assert "additionalProperties" not in cleaned[0]["function"]["parameters"] + assert "strict" not in cleaned[0]["function"] + + def test_cache_key_stable_after_sequential_calls(self): + """Cache key must be identical before and after both functions run.""" + tools = _make_tools_with_strict() + + with tempfile.TemporaryDirectory() as cache_dir: + cache = Cache(type=LiteLLMCacheType.DISK, disk_cache_dir=cache_dir) + + key_before = cache.get_cache_key( + model="hosted_vllm/test", + messages=[{"role": "user", "content": "test"}], + tools=tools, + ) + + # Simulate the exact production call sequence + cleaned = _remove_additional_properties(tools) + cleaned = _remove_strict_from_schema(cleaned) + + key_after = cache.get_cache_key( + model="hosted_vllm/test", + messages=[{"role": "user", "content": "test"}], + tools=tools, + ) + + assert key_before == key_after, ( + f"Cache key changed after sequential removal calls: " + f"{key_before!r} != {key_after!r}" + ) + + +# --------------------------------------------------------------------------- +# In-place helpers: direct coverage +# --------------------------------------------------------------------------- + +class TestInPlaceHelpers: + """Direct tests for the _in_place internal helpers. + These are called only on already-copied data, but we verify they + actually perform the mutations they promise.""" + + def test_remove_strict_in_place_deletes_strict(self): + schema = {"strict": True, "properties": {"a": {"strict": True}}} + + _remove_strict_from_schema_in_place(schema) + + assert "strict" not in schema + assert "strict" not in schema["properties"]["a"] + + def test_remove_strict_in_place_on_list(self): + schemas = [{"strict": True}, {"strict": True, "nested": {"strict": True}}] + + _remove_strict_from_schema_in_place(schemas) + + assert "strict" not in schemas[0] + assert "strict" not in schemas[1] + assert "strict" not in schemas[1]["nested"] + + def test_remove_additional_properties_in_place_deletes(self): + schema = { + "additionalProperties": False, + "properties": {"a": {"additionalProperties": False}}, + } + + _remove_additional_properties_in_place(schema) + + assert "additionalProperties" not in schema + assert "additionalProperties" not in schema["properties"]["a"] + + def test_remove_additional_properties_in_place_keeps_true(self): + """Only additionalProperties: false is removed; true is kept.""" + schema = {"additionalProperties": True} + + _remove_additional_properties_in_place(schema) + + assert schema["additionalProperties"] is True