fix(cache): isolate retry-loop kwargs so cache store key matches lookup key - #2455
Closed
ErenAta16 wants to merge 3 commits into
Closed
fix(cache): isolate retry-loop kwargs so cache store key matches lookup key#2455ErenAta16 wants to merge 3 commits into
ErenAta16 wants to merge 3 commits into
Conversation
patch.py computes a cache lookup key from new_kwargs["messages"] before retry_sync_v2/retry_async_v2 run, and recomputes the store key from the same new_kwargs["messages"] after they return, but passes new_kwargs into the retry call by reference. Reask handlers (reask_tools, reask_md_json, reask_default, reask_responses_tools, Anthropic's handle_reask) each do kwargs = kwargs.copy() then kwargs["messages"].append(...)/.extend(...): the shallow dict copy doesn't copy the messages list, so those handlers mutate the same list object patch.py still holds a reference to. Any request that needed at least one retry ends up with a store key computed from the post-retry, reask-polluted messages list, different from the lookup key computed from the pristine list, so the result gets cached under a key nothing will ever look up again. Add isolate_retry_kwargs() in messages.py, mirroring the existing copy_messages_for_mutation helper (added for 567-labs#2417/567-labs#2428), and use it at both call sites where new_kwargs is handed to the retry layer, so mutations during the retry loop can never leak back into the kwargs dict used for the cache store key regardless of what an individual reask handler does. Adds a regression test, test_auto_cache_prevents_duplicate_calls_after_a_retry in tests/cache/test_cache_integration.py, next to the existing test_auto_cache_prevents_duplicate_provider_calls. Verified red (fails on the unfixed code) and green (passes on the fixed code). Fixes 567-labs#2454.
isolate_retry_kwargs stopped after copying the first of messages/contents/chat_history it found, so a kwargs dict carrying more than one would leave the others aliased to the caller's list. No current provider does this, but nothing in the kwargs shape rules it out either. Isolate each candidate independently instead.
6 tasks
Collaborator
|
Consolidated and shipped in #2495. Closing this focused patch as superseded; thank you for the contribution. |
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.
Fixes #2454.
What's broken
_create_sync_wrapper/_create_async_wrapper(instructor/v2/core/patch.py) compute a cache lookup key fromnew_kwargs["messages"]before callingretry_sync_v2/retry_async_v2, and recompute the store key from the samenew_kwargs["messages"]after the retry call returns, but passnew_kwargsinto the retry call by reference.Each reask handler (
reask_tools,reask_md_json,reask_default,reask_responses_toolsinproviders/openai/handlers.py, and Anthropic'shandle_reask) doeskwargs = kwargs.copy()thenkwargs["messages"].append(...)/.extend(...). The shallow dict copy doesn't copy themessageslist, so this mutates the exact list objectpatch.py'snew_kwargsstill points to.Any request that needed at least one retry ends up with a store key computed from the post-retry, reask-polluted
messageslist, different from the lookup key computed from the pristine one before the retry loop ran. The result is correct, but it gets cached under a key nothing will ever look up again, so an identical follow-up request always misses and re-triggers a real LLM call.Fix
Added
isolate_retry_kwargs()inmessages.py, right next to the existingcopy_messages_for_mutationhelper (added for #2417/#2428, which fixed the analogous problem one layer up inprepare_request). It returnskwargswith a shallow copy of whichever ofmessages/contents/chat_historyis present, so mutations during the retry loop land on a private list and can never leak back into thekwargsdictpatch.pyreads again for the cache store key, regardless of what an individual reask handler does internally.Used it at both call sites where
new_kwargsis handed to the retry layer (sync and async).Testing
Added
test_auto_cache_prevents_duplicate_calls_after_a_retryintests/cache/test_cache_integration.py, next to the existingtest_auto_cache_prevents_duplicate_provider_calls. It forces exactly one retry (first fake response fails a Pydantic validator, second succeeds), then makes an identical second call and asserts the provider function was only invoked twice total (once for the first call's retry, zero more for the second).Verified both directions:
Also ran the broader cache/retry suites to check for regressions, all green:
Issue ticket number and link
Fixes #2454
Checklist before requesting a review