Skip to content

Include tool_choice in ChatCompletionCache's cache key#7969

Open
ErenAta16 wants to merge 2 commits into
microsoft:mainfrom
ErenAta16:fix/chat-completion-cache-tool-choice-7968
Open

Include tool_choice in ChatCompletionCache's cache key#7969
ErenAta16 wants to merge 2 commits into
microsoft:mainfrom
ErenAta16:fix/chat-completion-cache-tool-choice-7968

Conversation

@ErenAta16

Copy link
Copy Markdown

Fixes #7968.

What's wrong

ChatCompletionCache._check_cache builds its cache key from messages, tools, json_output, and extra_create_args only, tool_choice was never included, even though it's an explicit parameter of create()/create_stream() that changes what the underlying client is asked to do (let the model decide, force a tool call, forbid tool calls, or force one specific tool) and materially changes the shape of the response. Two calls with identical messages/tools but different tool_choice collided on the same cache key, so the second call was silently served the first call's cached response.

This is exactly the scenario the codebase itself exercises: AssistantAgent._reflect_on_tool_use_flow passes tool_choice="none" to force a plain-text reflection after tool execution, while the normal tool-call path uses tool_choice="auto". Reusing one ChatCompletionCache-wrapped client across both would be affected.

Fix

Thread tool_choice through to _check_cache from both create() and create_stream(), and include it in the hashed data (serializing a forced Tool's .schema, the same way tools is already serialized, or the literal string otherwise).

Testing

Added test_cache_miss_on_tool_choice_change to tests/models/test_chat_completion_cache.py, next to the existing test_cache_basic_with_args which already tests "cache miss if args change" for json_output.

Verified red against the original code:

AssertionError: assert not True
 +  where True = CreateResult(..., cached=True).cached

(the second call, same messages but tool_choice="none", incorrectly reported cached=True)

Verified green with the fix, and ran the full suite:

tests/models/test_chat_completion_cache.py: 27 passed

No regressions in any of the existing 26 tests.

I also reproduced the original bug with a small fake ChatCompletionClient whose response actually depends on tool_choice (to mirror what a real provider does, since ReplayChatCompletionClient alone can't distinguish "wrong response shape", only "which response index"), confirming the underlying client was only ever invoked once across both calls before the fix, and correctly invoked twice after.

_check_cache built its hash from messages, tools, json_output, and
extra_create_args only. tool_choice is an explicit, documented parameter of
create()/create_stream() that materially changes the shape of the response
(auto vs required vs none vs a specific forced Tool), but was silently
dropped from the cache key. Two calls with identical messages/tools but
different tool_choice collided on the same cache key, so the second call
was served a stale, wrong-shaped cached response instead of invoking the
underlying client.

Thread tool_choice through to _check_cache from both create() and
create_stream(), and include it in the hashed data (serializing a forced
Tool's .schema, matching how `tools` is already serialized).

Adds test_cache_miss_on_tool_choice_change to
tests/models/test_chat_completion_cache.py, verified red (fails on the
original code, the second call incorrectly reports cached=True) and green
(passes with the fix). Ran the full test_chat_completion_cache.py suite
after the fix: 27 passed, no regressions.

Fixes microsoft#7968.
@ErenAta16

Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree

@ErenAta16

Copy link
Copy Markdown
Author

Flagging for whoever picks this up for review: #7948 (HumphreySun98) fixes the exact same bug, same file, same function, opened about a week before this one. Looks like neither of us saw the other while working on it.

Comparing the two diffs honestly: #7948's cache-key representation for a forced Tool uses just tool.schema["name"], this one uses the full tool.schema. Using just the name is fine in the common case and arguably simpler, the full-schema version only matters if two tools happen to share a name but differ in parameters/description, a real but narrow edge case. #7948's test is also more thorough at the unit level, it exercises _check_cache directly across all four tool_choice states (auto/required/none/forced-tool) plus a default-matches-auto check. This PR's test goes through the public create() API end-to-end instead (cache miss then cache hit), which is closer to how the bug actually manifests for a caller, but only covers two of the four states.

No strong preference here on which one lands, they're both correct fixes for the same real bug. Happy to close this one in favor of #7948 if that's simpler, or if it's useful I can add the missing tool_choice state coverage to this PR's test to match #7948's thoroughness. Whatever's less work on your end.

…itted-vs-auto normalization

Two follow-up regression tests, addressing feedback from 0xbrainkid on microsoft#7968:

1. test_cache_miss_on_forced_tool_choice_then_none - the specific scenario
   raised in review: an agent step that forces a specific tool call
   followed by a step that forbids tool calls entirely. These are
   different authority boundaries for the model, so the fix needs to
   treat them as different cache keys, not just different string values
   of a generic parameter. Confirms tool_choice=<Tool> and
   tool_choice="none" don't collide.

2. test_cache_hit_omitted_tool_choice_matches_explicit_auto - locks in
   that omitting tool_choice entirely and passing tool_choice="auto"
   explicitly hash identically, since create()'s own default is "auto".
   This holds today because _check_cache's default matches create()'s
   default, but nothing enforced that the two stay in sync short of
   reading both signatures, so making it an explicit test means a future
   refactor that lets them drift apart fails loudly instead of silently
   splitting the cache for every caller who omits tool_choice.

Ran the full test_chat_completion_cache.py file: 29 passed.
@ErenAta16

Copy link
Copy Markdown
Author

Pushed c1296e8 with the two follow-up cases from the discussion on #7968:

  • test_cache_miss_on_forced_tool_choice_then_none: the exact scenario 0xbrainkid raised, a forced-specific-tool step followed by a no-tools step must not share a cache entry, since those are different authority boundaries for the model, not just different formatting.
  • test_cache_hit_omitted_tool_choice_matches_explicit_auto: locks in that omitting tool_choice and passing "auto" explicitly hash to the same key (true today because both default to "auto", now enforced by a test instead of just an implicit consequence of two signatures happening to agree).

Full test_chat_completion_cache.py file: 29 passed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ChatCompletionCache cache key ignores tool_choice, serves stale cached response when only tool_choice differs

1 participant