fix(v2): isolate kwargs for retry to prevent cache key divergence (#2454)

[feiiiiii5]

Summary

Fixes #2454.

When cache= is used and the first LLM call fails validation, the reask loop appends a failed tool-call turn and a validation-feedback turn to messages. All 13 v2 providers do this with kwargs = kwargs.copy() (shallow) followed by in-place kwargs["messages"].extend/append(...).

Because patch.py passes new_kwargs by reference into the retry function and then computes the cache store key from new_kwargs["messages"] after the retry returns, the store key diverges from the lookup key: the store key now includes the failed tool-call turn + feedback turn, so no future call can ever produce that same key. The successful result is cached under an unreachable key — effectively a silent cache miss for every retried request.

Root cause

The data flow that produces the divergence:

1. patch.py computes the cache lookup key from new_kwargs.get("messages") before calling retry_sync_v2 / retry_async_v2.
2. It passes new_kwargs by reference into the retry function.
3. Inside the retry loop, reask handlers do kwargs = kwargs.copy() (shallow) and then kwargs["messages"].extend(reask_msgs) / .append(...) — mutating the shared messages list object in place.
4. After the retry returns, patch.py computes the cache store key from new_kwargs.get("messages") — which now includes the failed tool-call turn + validation-feedback turn.
5. The store key diverges from the lookup key. The successful result is cached under a key that no future identical call will ever produce, so every follow-up call goes back through the real LLM.

This affects all 13 v2 providers that have reask handlers with the same kwargs = kwargs.copy() + in-place messages.extend/append pattern: openai, anthropic, gemini, genai, cohere, mistral, vertexai, perplexity, openrouter, writer, xai, bedrock, groq.

Fix

Pass an isolated copy of new_kwargs into the retry function so reask-handler mutations to messages / contents / chat_history are not observable through the caller's new_kwargs. The store key is then computed from the same pristine new_kwargs the lookup key was computed from, so the two keys always agree.

This closes the door for any future handler making the same shallow-copy-then-mutate mistake, rather than fixing each of the 13 provider handlers individually (option a from the issue).

The new helper _isolate_kwargs_for_retry() only copies the list container for the three known message-bearing kwargs (messages, contents, chat_history); the individual message dicts are shared, which is sufficient because reask handlers only append/extend new dicts — they never mutate existing entries — and the cache key is computed from the JSON serialization of the list, which is invariant to whether the dicts are aliased or not.

Tests

Adds tests/v2/test_issue_2454.py with sync + async regression tests that:

• Force a validation failure on the first LLM call ({"name": "Ada"} — missing age) and a success on the second ({"name": "Ada", "age": 37}).
• Call the patched create_fn twice with identical inputs.
• Assert that the second call does not re-invoke the underlying LLM (call_count == 2, not 3).

Both tests fail on unmodified main (call_count == 3 instead of 2) and pass with this fix.

$ uv run pytest tests/v2/test_issue_2454.py -v
============================= test session starts ==============================
platform darwin -- Python 3.11.15, pytest-8.4.2, pluggy-1.6.0
collecting ... collected 2 items

tests/v2/test_issue_2454.py::test_cache_survives_reask_loop_sync PASSED [ 50%]
tests/v2/test_issue_2454.py::test_cache_survives_reask_loop_async PASSED [100%]

============================== 2 passed in 0.98s ===============================

No regressions in the existing v2 / cache suites (the 2 pre-existing mistralai import failures are unrelated — the mistralai package is an optional dependency not installed locally and the failures exist on main):

$ uv run pytest tests/v2/ tests/cache/ -q
2 failed, 1575 passed, 170 skipped
# failed: tests/v2/test_messages_not_mutated.py::test_prepare_request_does_not_alias_caller_messages[mistral-json_schema_mode]
# failed: tests/v2/test_messages_not_mutated.py::test_reask_does_not_mutate_caller_messages[mistral-json_schema_mode]
# both: ModuleNotFoundError: No module named 'mistralai'

Lint, format, and type checks all pass:

$ uv run ruff check instructor/v2/core/patch.py tests/v2/test_issue_2454.py
All checks passed!
$ uv run ruff format --check instructor/v2/core/patch.py tests/v2/test_issue_2454.py
2 files already formatted
$ uv run ty check instructor/v2/core/patch.py
All checks passed!

Checklist

• Fix is minimal and surgical — one new helper + two call-site changes.
• Regression test added that fails on main and passes with the fix.
• Sync + async paths both covered.
• No existing tests broken (the 2 mistralai failures pre-date this PR).
• CHANGELOG entry added under [Unreleased].
• ruff check, ruff format --check, ty check all clean.

Closes #2454.

[ErenAta16]

This fixes the same bug as #2455 (I filed #2454 and opened that PR the day before this one), so wanted to flag the overlap rather than let both sit in the queue unremarked.

The core fix is functionally the same: isolate the messages/contents/chat_history list before it goes into the retry loop so reask mutations can't leak back into the kwargs the cache store key gets computed from. One real difference worth mentioning: your _isolate_kwargs_for_retry copies every matching key it finds among the three, while my isolate_retry_kwargs originally returned after the first match, so a kwargs dict carrying more than one of those keys would leave the others aliased. No current provider actually does that, but your version handles it correctly and mine didn't, so I've updated #2455 to match (isolates all three independently now instead of stopping at the first).

Given that, functionally these are close to interchangeable at this point. Not going to push for one over the other, just wanted the maintainers to have the full picture instead of reviewing two PRs that look unrelated at a glance but touch the exact same bug.

[feiiiiii5]

@ErenAta16 thanks for the transparent heads-up — appreciate you flagging the overlap rather than letting both PRs sit in the queue looking unrelated. You're right that #2455 was first (opened 7/17 20:50 UTC, mine 7/18 10:38 UTC) and that the core fix is functionally the same: shallow-copy the messages/contents/chat_history list before it enters the retry loop so reask-handler extend/append mutations can't leak back into the kwargs the cache store key is computed from.

A few honest differences worth putting on the table for the maintainers:

1. Placement. Your isolate_retry_kwargs lives in instructor/v2/core/messages.py next to copy_messages_for_mutation — same neighborhood, same concept (one layer up the call stack). Mine is a private _isolate_kwargs_for_retry inside patch.py. Yours is the better placement; that's where the next reader will look for "messages-list mutation safety" utilities. If maintainers pick yours, no relocation needed.

2. CHANGELOG. Mine has a CHANGELOG entry under [Unreleased] / Fixed explaining the cache-key divergence symptom (referencing #2454). I didn't see one in #2455's diff — happy to drop the entry verbatim into your PR if maintainers want it, or it can ride along with whichever PR lands.

3. Tests. Both add regression tests; I haven't diffed them line-by-line but the angles look similar (cache key stability across a retry that touches messages). Worth a glance by the maintainer to see if there's coverage on either side worth merging.

4. The multi-key behavior you already adopted. Thanks for the explicit callout — your original early-return-after-first-match version would have left a hypothetical multi-key kwargs dict with one isolated list and two still-aliased ones. No current provider sends more than one of the three keys, but the "isolate all three independently" shape is strictly safer and costs nothing. Both PRs converge on that now.

Suggested resolution. I'd lean toward the maintainers taking #2455 as the canonical PR (it was first, has the better placement, and you've already updated it to handle the multi-key case). I'm happy to close #2457 once there's a steer — or, if @jxnl prefers the CHANGELOG entry + my test angle, I can fold those into #2455 and close mine either way. Not going to push either direction; just want to spare the maintainers from reviewing two near-identical diffs.

Tagging @jxnl for visibility — let me know which way you'd like to go and I'll act on it (close mine, or rebase the unique bits onto #2455).

[ErenAta16]

Really appreciate the thoroughness here, that's a fair and generous rundown. Added the CHANGELOG entry to #2455 (ec6e640), same wording you used since it's accurate and I didn't want to reinvent it. Agreed on your suggested resolution, happy to have the maintainers take #2455 given the placement point, but genuinely no attachment either way, whichever one they pick works fine for me. If they'd rather have your test angle folded in alongside, I'm open to that too.

Thanks for handling this the collaborative way instead of just racing it.

[feiiiiii5]

Thanks @ErenAta16 — appreciate the generous response and the CHANGELOG entry on #2455. Sounds like we're aligned either way: leaving it to @jxnl to pick, and I'll close #2457 if #2455 is chosen. No further action from my side until there's a decision.

[ErenAta16]

Sounds good, same here. Thanks for making this an easy one to sort out.

[feiiiiii5]

Closing in favor of #2455 as discussed with @ErenAta16 — the CHANGELOG entry was added there (ec6e640). Thanks for the smooth resolution!

[ErenAta16]

Sounds good, and thanks for moving the CHANGELOG entry over to #2455 rather than leaving it stranded here. One PR carrying the fix and its changelog line is cleaner than splitting them across two.