2 Commits

• .NET: Fix/per service input persistence on stream error (#5744)

  * .NET: Persist input messages on streaming errors in PerServiceCallChatHistoryPersistingChatClient
  
  When the underlying chat service emits an in-stream error (for example a
  `response.error` SSE event from the OpenAI Responses API on rate limit),
  the OpenAI client surfaces it as an `ErrorContent` update and ends the
  stream without throwing. Previously, `PerServiceCallChatHistoryPersistingChatClient`
  only persisted history when the streaming loop completed successfully and
  `NotifyProvidersOfNewMessagesAsync` was called at the end. On the
  in-stream-error path, the input messages handed to that iteration -
  typically `FunctionResultContent` produced by `FunctionInvokingChatClient`
  in the previous iteration - were never persisted. The next run would
  replay session history with a dangling `FunctionCallContent` and the
  service would reject the request with `No tool output found for function
  call <id>`.
  
  This change:
  
  - Adds a `PersistInputOnErrorAsync` helper that persists the input
    messages (with no response messages) so function-call/function-result
    pairings are not split across failures.
  - Calls the helper from every error path: pre-loop enumerator creation,
    the first `MoveNextAsync`, the in-loop `MoveNextAsync`, and a new
    `finally` that handles abnormal iterator disposal.
  - After the streaming loop, scans the assembled response for any
    `ErrorContent` and, if present, persists the input, notifies
    providers of failure, and throws `InvalidOperationException` so the
    error is surfaced to the caller instead of silently corrupting history.
  - Hardens `InMemoryChatHistoryProvider.StoreChatHistoryAsync` to treat
    a null `RequestMessages` as empty, since the new error path can
    invoke it with no response messages.
  
  Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
  
  * Fix dropped FunctionResultContent on streaming pipeline early-disposal
  
  When a consumer of ChatClientAgent.RunStreamingAsync stops iterating early
  (e.g. ToolApprovalAgent yields the approval request and then `yield break`),
  the framework cascades DisposeAsync down the stream. C# async iterators do
  not auto-dispose IAsyncDisposable locals, so the inner enumerator returned
  by IChatClient.GetStreamingResponseAsync(...).GetAsyncEnumerator(ct) was
  left suspended. That suspended FunctionInvokingChatClient downstream, which
  suspended PerServiceCallChatHistoryPersistingChatClient at its `yield
  return`, so its finally block never ran and the in-flight
  FunctionResultContent for the just-completed tool call was not persisted
  to chat history. The next turn then loaded a session that contained a
  FunctionCallContent with no matching FunctionResultContent and the model
  returned HTTP 400 `No tool output found for function call`.
  
  Fixes:
  
  * ChatClientAgent.RunStreamingAsync: wrap the iteration in
    try/finally that disposes the inner enumerator. Disposal now cascades
    through the pipeline and PerService's finally runs on early exit.
  * PerServiceCallChatHistoryPersistingChatClient: in the streaming path,
    snapshot input messages with `messages.ToList()` (the caller, FICC,
    reuses a single mutable buffer across iterations and may mutate it
    before our finally / error path persists), wrap GetAsyncEnumerator,
    the first MoveNextAsync, and in-loop MoveNextAsync in try/catch each
    calling PersistInputOnErrorAsync + NotifyProvidersOfFailureAsync, and
    add a finally that calls PersistInputOnErrorAsync when the loop did
    not exit normally so per-iteration FRCs are persisted on early
    disposal as well as on errors.
  
  Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
  
  * .NET: Add tests for PerService streaming error/dispose persistence paths
  
  Adds five regression tests covering the new error-path persistence in
  
  PerServiceCallChatHistoryPersistingChatClient.GetStreamingResponseInnerAsync:
  
  - Persists input messages when GetStreamingResponseAsync throws synchronously.
  
  - Persists input messages when the first MoveNextAsync throws.
  
  - Persists input messages when a mid-stream MoveNextAsync throws.
  
  - Persists input messages when the consumer abandons enumeration early
  
    (the ToolApprovalAgent yield-break / disposal-cascade case).
  
  - Throws and persists input when the stream emits an in-band ErrorContent.
  
  All 66 tests in the class pass on net10.0 and net472.
  
  Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
  
  * .NET: Address PR feedback on PerService streaming error persistence
  
  Two follow-ups from PR #5744 review:
  
  1. Prevent duplicate persistence on the in-loop MoveNextAsync catch path.
  
     The inner catch persists input messages, then rethrows, which propagates
  
     through the surrounding try/finally where loopExitedNormally is still false,
  
     causing the finally to persist again. Introduced an inputPersisted flag
  
     that the inner catch sets after persisting; the finally now skips when
  
     inputPersisted is true.
  
  2. Use the caller's CancellationToken in the abnormal-exit finally instead
  
     of CancellationToken.None, so cleanup remains responsive to cancellation.
  
     Fall back to CancellationToken.None only when the caller's token is
  
     already canceled (otherwise the persist call would observe the
  
     cancellation, throw, and mask the original early-exit reason).
  
  Tightened all five new streaming-error tests from Times.AtLeastOnce to
  
  Times.Once on the input-persistence matcher to regression-guard against
  
  duplicate persistence. All 66 tests in the class still pass (net10.0 + net472).
  
  Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
  
  * .NET: Scope PerService streaming changes to cooperative early-exit only
  
  Per discussion on PR #5744, scope this PR back to fix only the original
  ToolApprovalAgent dropped-FunctionResultContent bug and address the
  enumerator-disposal review comment. Specifically:
  
  - Remove input-message persistence from the GetAsyncEnumerator and
    MoveNextAsync error paths. Routing failed service calls through the
    success notification channel was breaking the provider contract; we
    will instead rely on inner-agent retries for transient errors. Failure
    paths still call NotifyProvidersOfFailureAsync as before.
  - Remove the in-stream ErrorContent detection block (same rationale).
  - Keep the try/finally that calls the (now narrower) early-exit input
    notification on cooperative disposal (e.g. ToolApprovalAgent yield
    break). A new serviceErrorOccurred flag ensures we do NOT renotify
    on exception paths.
  - Always DisposeAsync the underlying enumerator on every exit path,
    addressing the copilot-reviewer comment about leaked HTTP/streams.
  - Rename PersistInputOnErrorAsync -> NotifyProvidersOfEarlyExitInputAsync
    to better reflect what it does and when it runs (rogerbarreto nit).
  - Apply rogerbarreto nit on InMemoryChatHistoryProvider null-coalescing.
  - Drop the four tests that covered the removed error-path behavior;
    keep RunStreamingAsync_PersistsInputMessages_WhenConsumerAbandons
    EnumerationAsync (regression guard for the cooperative-pause path).
  
  Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
  
  ---------
  
  Co-authored-by: alliscode <bentho@microsoft.com>
  Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

  Ben Thomas · 2026-05-11 20:28:14 +00:00
  0bbedc4fa2
• .NET: [BREAKING] Rename from ServiceStoredSimulatingChatClient to PerServiceCallChatHistoryPersistingChatClient (#4993)

  * Rename from ServiceStoredSimulatingChatClient to PerServiceCallChatHistoryPersistingChatClient
  
  * Address PR comment

  westey · 2026-03-31 17:32:05 +00:00
  6e7254bba7