diff --git a/CHANGELOG.md b/CHANGELOG.md index 0fcbd6f1b..31c6f9c31 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ Versioning: [Semantic Versioning](https://semver.org/spec/v2.0.0.html) ## [Unreleased] ### Fixed +- **v2 caching**: Isolate retry-bound `kwargs` so reask-handler mutations to `messages` cannot leak back into the caller's `new_kwargs` and cause the cache lookup/store keys to diverge after any retry. Without this fix, `cache=` silently stops caching any request that needed at least one reask. ([#2454](https://github.com/567-labs/instructor/issues/2454)) - **v2 message handling**: Preserve caller-owned message lists and nested content across request preparation and retries for OpenAI-compatible, Cohere, Mistral, OpenRouter, Writer, and xAI handlers. ([#2417](https://github.com/567-labs/instructor/issues/2417), [#2428](https://github.com/567-labs/instructor/issues/2428)) - **v2 JSON extraction**: Prefer the final complete top-level JSON value in text responses and retain every JSON object when multiple objects arrive in one streaming chunk. - **v2 schemas**: Treat fields with Pydantic `default_factory` values as optional in generated OpenAI tool schemas. diff --git a/instructor/v2/core/messages.py b/instructor/v2/core/messages.py index 9a24c81ea..b016f330f 100644 --- a/instructor/v2/core/messages.py +++ b/instructor/v2/core/messages.py @@ -38,6 +38,29 @@ def copy_messages_for_mutation(messages: list[dict[str, Any]]) -> list[dict[str, return copied +def isolate_retry_kwargs(kwargs: dict[str, Any]) -> dict[str, Any]: + """Return kwargs with a shallow copy of the messages/contents/chat_history list. + + Reask handlers append/extend that list in place during the retry loop (see + `copy_messages_for_mutation` above for the equivalent problem one layer up, in + `prepare_request`). Passing the caller-facing kwargs dict straight into the retry + loop means those mutations land on the exact same list object the caller passed + in, which corrupts any code that reads it again afterward, e.g. computing a cache + key from it both before and after the retry loop runs. + + Each of the three candidate keys is isolated independently rather than stopping + at the first match, so a kwargs dict carrying more than one of them (not used by + any current provider, but not precluded by the shape of kwargs either) doesn't + leave the others aliased to the caller's list. + """ + isolated = dict(kwargs) + for key_name in ("messages", "contents", "chat_history"): + value = isolated.get(key_name) + if isinstance(value, list): + isolated[key_name] = list(value) + return isolated + + def dump_message(message: ChatCompletionMessage) -> ChatCompletionMessageParam: ret: ChatCompletionMessageParam = { "role": message.role, diff --git a/instructor/v2/core/patch.py b/instructor/v2/core/patch.py index 26aacc6ba..ac436d93e 100644 --- a/instructor/v2/core/patch.py +++ b/instructor/v2/core/patch.py @@ -20,6 +20,7 @@ from instructor.v2.core.utils import is_async from instructor.v2.core.exceptions import RegistryValidationMixin from instructor.v2.core.registry import mode_registry +from instructor.v2.core.messages import isolate_retry_kwargs from instructor.v2.core.response_model import prepare_response_model from instructor.v2.core.retry import retry_async_v2, retry_sync_v2 @@ -247,7 +248,9 @@ def new_create_sync( if cached is not None: return cached # type: ignore[return-value] - # Use v2 retry logic with registry handlers + # Use v2 retry logic with registry handlers. Pass an isolated copy of the + # messages list so reask-handler mutations during the retry loop can't leak + # back into new_kwargs, which is read again below for the cache store key. response = retry_sync_v2( func=func, response_model=response_model, @@ -256,7 +259,7 @@ def new_create_sync( context=context, max_retries=max_retries, args=args, - kwargs=new_kwargs, + kwargs=isolate_retry_kwargs(new_kwargs), strict=strict, hooks=hooks, ) @@ -359,7 +362,9 @@ async def new_create_async( if cached is not None: return cached # type: ignore[return-value] - # Use v2 retry logic with registry handlers + # Use v2 retry logic with registry handlers. Pass an isolated copy of the + # messages list so reask-handler mutations during the retry loop can't leak + # back into new_kwargs, which is read again below for the cache store key. response = await retry_async_v2( func=func, response_model=response_model, @@ -368,7 +373,7 @@ async def new_create_async( context=context, max_retries=max_retries, args=args, - kwargs=new_kwargs, + kwargs=isolate_retry_kwargs(new_kwargs), strict=strict, hooks=hooks, ) diff --git a/tests/cache/test_cache_integration.py b/tests/cache/test_cache_integration.py index afb8687a8..158d2329b 100644 --- a/tests/cache/test_cache_integration.py +++ b/tests/cache/test_cache_integration.py @@ -3,7 +3,7 @@ import instructor from instructor.cache import AutoCache from openai.types.chat import ChatCompletionMessageParam -from pydantic import BaseModel, Field # type: ignore[import-not-found] +from pydantic import BaseModel, Field, field_validator # type: ignore[import-not-found] def test_auto_cache_prevents_duplicate_provider_calls(monkeypatch): @@ -43,3 +43,61 @@ def fake_completion(*_args, **_kwargs): # noqa: D401, ANN001 # Second call with identical inputs – should hit cache, no new provider call _ = client.create(messages=list(messages), response_model=User, cache=cache) assert call_counter["n"] == 1, "Cache miss – provider was called again" + + +def test_auto_cache_prevents_duplicate_calls_after_a_retry(monkeypatch): + _ = monkeypatch + """Regression test: a call that needed a retry must still be cacheable. + + Reask handlers append/extend the request's messages list in place. If that + list is the same object patch.py reads again to compute the cache store key, + the store key ends up different from the lookup key computed before the + retry, so a later, identical call never hits the cache. + """ + + class Answer(BaseModel): + value: int + + @field_validator("value") + @classmethod + def must_be_positive(cls, v: int) -> int: + if v < 0: + raise ValueError("value must be positive") + return v + + call_counter = {"n": 0} + + def fake_completion(*_args, **_kwargs): + call_counter["n"] += 1 + # First call ever returns an invalid value, forcing exactly one retry. + value = -5 if call_counter["n"] == 1 else 42 + content = Answer.model_construct(value=value).model_dump_json() + return types.SimpleNamespace( + choices=[ + types.SimpleNamespace( + message=types.SimpleNamespace(content=content), + finish_reason="stop", + ) + ], + usage={}, + ) + + cache = AutoCache(maxsize=10) + client = instructor.from_litellm(fake_completion, mode=instructor.Mode.JSON) + messages: list[ChatCompletionMessageParam] = [ + {"role": "user", "content": "what is 6 times 7?"} + ] + + result1 = client.create( + messages=list(messages), response_model=Answer, max_retries=2, cache=cache + ) + assert result1.value == 42 + assert call_counter["n"] == 2, "First call should need exactly one retry" + + result2 = client.create( + messages=list(messages), response_model=Answer, max_retries=2, cache=cache + ) + assert result2.value == 42 + assert call_counter["n"] == 2, ( + "Second, identical call should hit the cache instead of calling the provider again" + )