diff --git a/src/Libraries/Microsoft.Extensions.AI.Abstractions/ChatRouting/RoutingChatClient.cs b/src/Libraries/Microsoft.Extensions.AI.Abstractions/ChatRouting/RoutingChatClient.cs index 58a1f050a2f..00b98377326 100644 --- a/src/Libraries/Microsoft.Extensions.AI.Abstractions/ChatRouting/RoutingChatClient.cs +++ b/src/Libraries/Microsoft.Extensions.AI.Abstractions/ChatRouting/RoutingChatClient.cs @@ -16,12 +16,20 @@ namespace Microsoft.Extensions.AI; /// Provides a template for an that selects and invokes another chat client. /// /// +/// /// Derived classes implement to supply one client for each request. The selected /// client is invoked once, and its response or failure is propagated to the caller. /// The exact client instance represents the selected routing identity. Caller-supplied options may vary per /// invocation, but they are ephemeral and do not participate in that identity. Applications can use distinct /// configured client wrappers when configurations require distinct routing identities, while custom policies may /// maintain separate application-specific grouping keys. +/// +/// +/// holds the options for the request and is what the selected client +/// receives. Options that belong to a particular route are configured on the client instead, typically with a +/// ConfigureOptionsChatClient wrapper, which clones the request options and applies the route's own values +/// on top of them. +/// /// [Experimental(DiagnosticIds.Experiments.AIRoutingChat, UrlFormat = DiagnosticIds.UrlFormat)] public abstract class RoutingChatClient : IChatClient @@ -44,8 +52,9 @@ public static RoutingChatClient Create( /// The cancellation token supplied for the request. /// The client to invoke. /// - /// Client-specific behavior should generally be attached to the returned client. Exceptions from this method - /// propagate to the caller. + /// Client-specific behavior should generally be attached to the returned client. Modifying + /// changes the options for the request itself, including any later + /// invocation of a different client for the same request. Exceptions from this method propagate to the caller. /// protected abstract ValueTask SelectClientAsync( RoutingContext context, @@ -63,7 +72,7 @@ public virtual async Task GetResponseAsync( return await client.GetResponseAsync( context.Messages, - options?.Clone(), + context.ChatOptions, cancellationToken).ConfigureAwait(false); } @@ -79,7 +88,7 @@ public virtual async IAsyncEnumerable GetStreamingResponseAs throw new InvalidOperationException($"{nameof(SelectClientAsync)} returned null."); await foreach (ChatResponseUpdate update in - client.GetStreamingResponseAsync(context.Messages, options?.Clone(), cancellationToken) + client.GetStreamingResponseAsync(context.Messages, context.ChatOptions, cancellationToken) .WithCancellation(cancellationToken) .ConfigureAwait(false)) { diff --git a/src/Libraries/Microsoft.Extensions.AI.Abstractions/ChatRouting/RoutingContext.cs b/src/Libraries/Microsoft.Extensions.AI.Abstractions/ChatRouting/RoutingContext.cs index edd5bb7760f..08bbf91eec3 100644 --- a/src/Libraries/Microsoft.Extensions.AI.Abstractions/ChatRouting/RoutingContext.cs +++ b/src/Libraries/Microsoft.Extensions.AI.Abstractions/ChatRouting/RoutingContext.cs @@ -15,8 +15,9 @@ namespace Microsoft.Extensions.AI; /// started from the sequence returned by . /// /// -/// is cloned when the context is created. It is provided for client selection and is -/// independent of both the caller's instance and the options passed to the selected client. +/// is cloned (via ) from the caller-supplied instance when +/// the context is created, so that instance is never handed to a selected client and subsequent changes to it are not +/// observed. The clone is shallow, so referenced objects may still be shared. /// /// [Experimental(DiagnosticIds.Experiments.AIRoutingChat, UrlFormat = DiagnosticIds.UrlFormat)] @@ -41,10 +42,11 @@ public RoutingContext( /// public IEnumerable Messages { get; } - /// Gets a snapshot of the request options supplied to client selection. + /// Gets the options for the request. /// - /// Changes do not affect the caller's instance or the options passed to the selected client. Client-specific - /// behavior should generally be attached to the returned client. + /// 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. /// public ChatOptions? ChatOptions { get; } } diff --git a/src/Libraries/Microsoft.Extensions.AI/ChatRouting/FailoverChatClient.cs b/src/Libraries/Microsoft.Extensions.AI/ChatRouting/FailoverChatClient.cs index d825a8a054d..1737e0feb01 100644 --- a/src/Libraries/Microsoft.Extensions.AI/ChatRouting/FailoverChatClient.cs +++ b/src/Libraries/Microsoft.Extensions.AI/ChatRouting/FailoverChatClient.cs @@ -108,7 +108,6 @@ public sealed override async Task GetResponseAsync( IChatClient selectedClient = await SelectClientAsync(context, cancellationToken).ConfigureAwait(false) ?? throw new InvalidOperationException($"{nameof(SelectClientAsync)} returned null."); - ChatOptions? attemptOptions = options?.Clone(); attemptCount++; ChatResponse? response = null; @@ -119,7 +118,7 @@ await SelectClientAsync(context, cancellationToken).ConfigureAwait(false) ?? { response = await selectedClient.GetResponseAsync( context.Messages, - attemptOptions, + context.ChatOptions, cancellationToken).ConfigureAwait(false); } catch (Exception ex) @@ -178,7 +177,6 @@ public sealed override async IAsyncEnumerable GetStreamingRe IChatClient selectedClient = await SelectClientAsync(context, cancellationToken).ConfigureAwait(false) ?? throw new InvalidOperationException($"{nameof(SelectClientAsync)} returned null."); - ChatOptions? attemptOptions = options?.Clone(); attemptCount++; bool reachedAttemptLimit = maximumAttempts is int limit && attemptCount >= limit; @@ -192,7 +190,7 @@ await SelectClientAsync(context, cancellationToken).ConfigureAwait(false) ?? enumerator = selectedClient .GetStreamingResponseAsync( context.Messages, - attemptOptions, + context.ChatOptions, cancellationToken) .GetAsyncEnumerator(cancellationToken); diff --git a/test/Libraries/Microsoft.Extensions.AI.Abstractions.Tests/ChatRouting/RoutingChatClientTests.cs b/test/Libraries/Microsoft.Extensions.AI.Abstractions.Tests/ChatRouting/RoutingChatClientTests.cs index b8b7d8439c3..23459711f45 100644 --- a/test/Libraries/Microsoft.Extensions.AI.Abstractions.Tests/ChatRouting/RoutingChatClientTests.cs +++ b/test/Libraries/Microsoft.Extensions.AI.Abstractions.Tests/ChatRouting/RoutingChatClientTests.cs @@ -50,9 +50,8 @@ public async Task Create_SelectsClientForRequest() Assert.Same(expected, response); Assert.Same(messages, observedContext!.Messages); Assert.NotSame(options, observedContext.ChatOptions); - Assert.NotSame(observedContext.ChatOptions, forwardedOptions); - Assert.Equal("selected", observedContext.ChatOptions!.ModelId); - Assert.Equal("request", forwardedOptions!.ModelId); + Assert.Same(observedContext.ChatOptions, forwardedOptions); + Assert.Equal("selected", forwardedOptions!.ModelId); Assert.Equal("request", options.ModelId); Assert.Equal(cancellationSource.Token, observedToken); Assert.Equal(1, selectionCount); diff --git a/test/Libraries/Microsoft.Extensions.AI.Tests/ChatRouting/FailoverChatClientTests.cs b/test/Libraries/Microsoft.Extensions.AI.Tests/ChatRouting/FailoverChatClientTests.cs index 6af1df345aa..e98bdcebe92 100644 --- a/test/Libraries/Microsoft.Extensions.AI.Tests/ChatRouting/FailoverChatClientTests.cs +++ b/test/Libraries/Microsoft.Extensions.AI.Tests/ChatRouting/FailoverChatClientTests.cs @@ -230,8 +230,10 @@ public async Task Dispatch_ConfiguredClientPreservesAndOverridesRequestOptions() Assert.Equal("request", requestOptions.ModelId); } - [Fact] - public async Task Failover_UsesFreshRequestOptionsForEachAttempt() + [Theory] + [InlineData(false)] + [InlineData(true)] + public async Task Failover_UsesRequestOptionsForEveryAttempt(bool streaming) { var requestOptions = new ChatOptions { @@ -244,9 +246,13 @@ public async Task Failover_UsesFreshRequestOptionsForEachAttempt() GetResponseAsyncCallback = (_, options, _) => { invokedOptions.Add(options!); - options!.ModelId = "changed by first client"; throw new InvalidOperationException("failed"); }, + GetStreamingResponseAsyncCallback = (_, options, _) => + { + invokedOptions.Add(options!); + return ThrowingStream("failed"); + }, }; ChatResponse expected = new(new ChatMessage(ChatRole.Assistant, "ok")); using var second = new TestChatClient @@ -256,25 +262,72 @@ public async Task Failover_UsesFreshRequestOptionsForEachAttempt() invokedOptions.Add(options!); return Task.FromResult(expected); }, + GetStreamingResponseAsyncCallback = (_, options, _) => + { + invokedOptions.Add(options!); + return YieldUpdates("ok"); + }, }; int selections = 0; using var router = new DelegatingFailoverTestRouter( _ => ++selections == 1 ? first : second); - ChatResponse response = await router.GetResponseAsync( - [new(ChatRole.User, "hi")], - requestOptions); + ChatResponse response = streaming + ? await router.GetStreamingResponseAsync([new(ChatRole.User, "hi")], requestOptions).ToChatResponseAsync() + : await router.GetResponseAsync([new(ChatRole.User, "hi")], requestOptions); - Assert.Same(expected, response); + Assert.Equal("ok", response.Text); Assert.Equal(2, selections); Assert.Equal(2, invokedOptions.Count); - Assert.NotSame(invokedOptions[0], invokedOptions[1]); - Assert.Equal("changed by first client", invokedOptions[0].ModelId); - Assert.Equal("request", invokedOptions[1].ModelId); - Assert.Equal("caller", invokedOptions[1].Instructions); + + // Every attempt receives the request's options, which are a clone of the caller's instance. + Assert.Same(invokedOptions[0], invokedOptions[1]); + Assert.NotSame(requestOptions, invokedOptions[0]); + Assert.Equal("request", invokedOptions[0].ModelId); + Assert.Equal("caller", invokedOptions[0].Instructions); Assert.Equal("request", requestOptions.ModelId); } + [Theory] + [InlineData(false)] + [InlineData(true)] + public async Task Failover_SelectionAndInvocationShareRequestOptions(bool streaming) + { + var requestOptions = new ChatOptions { ModelId = "request" }; + var invokedOptions = new List(); + using var inner = new TestChatClient + { + GetResponseAsyncCallback = (_, options, _) => + { + invokedOptions.Add(options); + return Task.FromResult(new ChatResponse(new ChatMessage(ChatRole.Assistant, "ok"))); + }, + GetStreamingResponseAsyncCallback = (_, options, _) => + { + invokedOptions.Add(options); + return YieldUpdates("ok"); + }, + }; + ChatOptions? selectedOptions = null; + using var router = new DelegatingFailoverTestRouter( + context => + { + selectedOptions = context.ChatOptions; + + // Mutating the request's options during selection shapes the request. + context.ChatOptions!.Temperature = 0.25f; + return inner; + }); + + _ = streaming + ? await router.GetStreamingResponseAsync([new(ChatRole.User, "hi")], requestOptions).ToChatResponseAsync() + : await router.GetResponseAsync([new(ChatRole.User, "hi")], requestOptions); + + Assert.Same(selectedOptions, invokedOptions[0]); + Assert.Equal(0.25f, invokedOptions[0]!.Temperature); + Assert.Null(requestOptions.Temperature); + } + [Fact] public async Task Failure_Propagates() { @@ -602,49 +655,6 @@ public async Task Streaming_FallsBackBeforeFirstUpdate() Assert.Equal(2, selections); } - [Fact] - public async Task Streaming_FailoverUsesFreshRequestOptionsForEachAttempt() - { - var requestOptions = new ChatOptions - { - Instructions = "caller", - ModelId = "request", - }; - var invokedOptions = new List(); - using var first = new TestChatClient - { - GetStreamingResponseAsyncCallback = (_, options, _) => - { - invokedOptions.Add(options!); - options!.ModelId = "changed by first client"; - return ThrowingStream("failed"); - }, - }; - using var second = new TestChatClient - { - GetStreamingResponseAsyncCallback = (_, options, _) => - { - invokedOptions.Add(options!); - return YieldUpdates("ok"); - }, - }; - int selections = 0; - using var router = new DelegatingFailoverTestRouter( - _ => ++selections == 1 ? first : second); - - ChatResponse response = await router.GetStreamingResponseAsync( - [new(ChatRole.User, "hi")], - requestOptions).ToChatResponseAsync(); - - Assert.Equal("ok", response.Text); - Assert.Equal(2, selections); - Assert.NotSame(invokedOptions[0], invokedOptions[1]); - Assert.Equal("changed by first client", invokedOptions[0].ModelId); - Assert.Equal("request", invokedOptions[1].ModelId); - Assert.Equal("caller", invokedOptions[1].Instructions); - Assert.Equal("request", requestOptions.ModelId); - } - [Fact] public async Task Streaming_CancellationDuringUpdateIsObservedByNextAttempt() {