Pass the request's options to the selected client - #7685
Open
joshuajyue wants to merge 2 commits into
Open
Conversation
RoutingContext.ChatOptions and the options given to the selected client were two independent clones of the caller's instance: one taken when the context was created, one taken per attempt inside the failover loop. Nothing kept them in sync, so a caller mutating its ChatOptions while a request was running could be observed by a later attempt but not by the context, leaving selection and invocation working from different values. Pass context.ChatOptions to the selected client so one snapshot serves both. The caller's instance is still cloned once when the context is created and is never handed to a client. This also settles where option shaping belongs. Request-level options are the context's options, and modifying them during selection shapes the request. Route-level options belong to the client, typically a ConfigureOptionsChatClient wrapper, which clones the request options and applies the route's values on top. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 74d04840-2379-4615-93f7-84f2299ada74
Contributor
There was a problem hiding this comment.
Pull request overview
Aligns chat-routing behavior so the same request-scoped ChatOptions snapshot is used for both client selection (RoutingContext.ChatOptions) and client invocation, eliminating divergence when caller options are mutated mid-flight and removing redundant per-attempt cloning.
Changes:
- Pass
context.ChatOptions(request-scoped clone) to the selected client inRoutingChatClientandFailoverChatClient. - Update
RoutingContext.ChatOptionsand selection documentation to describe request-level vs route-level option shaping. - Update routing/failover tests to assert selection and invocation share the same request options instance across both non-streaming and streaming paths.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Libraries/Microsoft.Extensions.AI.Abstractions/ChatRouting/RoutingChatClient.cs | Forwards context.ChatOptions to the selected client and updates routing docs. |
| src/Libraries/Microsoft.Extensions.AI/ChatRouting/FailoverChatClient.cs | Removes per-attempt option cloning so all attempts receive the request’s options instance. |
| src/Libraries/Microsoft.Extensions.AI.Abstractions/ChatRouting/RoutingContext.cs | Updates ChatOptions documentation to reflect request-level shaping semantics. |
| test/Libraries/Microsoft.Extensions.AI.Abstractions.Tests/ChatRouting/RoutingChatClientTests.cs | Adjusts tests to validate the same options instance is forwarded to the client. |
| test/Libraries/Microsoft.Extensions.AI.Tests/ChatRouting/FailoverChatClientTests.cs | Reworks failover tests to validate request options are reused across attempts and shared between selection and invocation. |
Suppressed comments (1)
src/Libraries/Microsoft.Extensions.AI.Abstractions/ChatRouting/RoutingContext.cs:49
- The property remarks describe the options as an isolated clone, but ChatOptions.Clone is shallow; referenced objects (e.g., items inside cloned collections) may still be shared. Clarifying this helps prevent incorrect assumptions about mutation safety.
/// This is a clone of the caller's options and is what the selected client receives. Modifying it shapes the
/// request, including any later invocation of a different client for the same request. Options that belong to a
/// particular route should be configured on the client instead.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
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.
Follow-up to #7662. Independent of #7684; whichever merges first, the other rebases.
The problem
RoutingContext.ChatOptionsand the options handed to the selected client are two independent clones of the caller's instance:Nothing keeps them in sync, and both branch off an object the caller still owns. If the caller mutates its
ChatOptionswhile the request is in flight, attempt 2 picks the change up and the context does not — so the selector decides using one set of values while the client receives another. Nothing in the API suggests that two things called "the options" can disagree.The context's options are also never used for anything but inspection today, which is the tell: they are built, handed to the selector, and then ignored by the invocation path.
Changes
Pass
context.ChatOptionsto the selected client. One snapshot, taken once, used by both selection and invocation. The caller's instance is still cloned when the context is created and is never handed to a client.What this settles
Option shaping now has a clean split, and neither half needs new API:
context.ChatOptionsRoute-level options were always expressed by which client you return — a
ConfigureOptionsChatClientwrapper is a client whose identity encodes its options. The two layers compose without routing doing anything, becauseConfigureOptionsChatClientclones the incoming options and applies its own on top:That is the merge a
(client, options)selection result would have required routing to implement, already implemented one layer down.This is not a revival of the
RoutingSelectionproposal from #7662. That proposal was for per-attempt options: a selection result carrying options scoped to the one invocation it accompanied, so two attempts in the same request could be shaped differently. What this change enables is the opposite scope — modifyingcontext.ChatOptionsshapes the request, and the change persists into every later attempt, because there is one instance. Per-attempt shaping is still expressed by selecting a different client, which is whatConfigureOptionsChatClientwrappers are for.It also answers the concern raised in #7662 that
context.ChatOptionswas both an input to selection and, because it is mutable, an implicit output. Under this split it is only ever input to selection and output for the request. Route-specific values never touch it.Trade-off
Attempts now share one options instance, so a client that mutates the options it was handed in place would affect a later attempt. Previously each attempt got a fresh clone.
That is worth giving up. No middleware in this repo mutates options it was handed — every clone in the pipeline is transform-driven, on an object the transformer just cloned itself.
FunctionInvokingChatClientstates the rule directly: "We only need to clone the options if we're actually mutating it." Same inConfigureOptionsChatClient,ImageGeneratingChatClient, andChatClientStructuredOutputExtensions. Routing was the only component paying a per-attempt clone to defend against something the rest of the stack assumes does not happen.In exchange the rule becomes a single sentence — the options are snapshotted once, when the request begins — instead of two that silently disagree. It is also one clone per request instead of
1 + N.Changes
RoutingChatClient— both invocation methods passcontext.ChatOptions.FailoverChatClient— the per-attempt clone is removed; every attempt receivescontext.ChatOptions.RoutingContext.ChatOptions— documentation no longer describes it as a snapshot independent of what the client receives, since that is now false by design. It states that modifying it shapes the request and that route-specific options belong on the client.RoutingChatClientandSelectClientAsync— documentation describes the request-level / route-level split.Failover_UsesFreshRequestOptionsForEachAttemptand its streaming counterpart are replaced byFailover_UsesRequestOptionsForEveryAttempt(theory over both paths), which asserts every attempt receives the request's options and that the caller's instance is untouched.Failover_SelectionAndInvocationShareRequestOptionscovers the fix directly, including shaping during selection.Create_SelectsClientForRequestasserted that a selector's mutation did not reach the client, and now asserts that it does.Validation
Microsoft.Extensions.AI.Tests— 761 passed.Microsoft.Extensions.AI.Abstractions.Tests— 1643 passed.Microsoft Reviewers: Open in CodeFlow