Repository navigation
.NET: fix(dotnet): avoid duplicate chat history with background respo… - #9874
feiyun0112 wants to merge 2 commits into
Conversation
…nses and continuation tokens (microsoft#9793) When RequirePerServiceCallChatHistoryPersistence is enabled, runs that use AllowBackgroundResponses or that are resumed with a continuation token persisted the same messages twice: once per service call from PerServiceCallChatHistoryPersistingChatClient and again from the forced end-of-run persistence in ChatClientAgent. - GetResponseAsync/GetStreamingResponseAsync no longer notify the providers for continuation/background runs; the agent's forced end-of-run notification owns them. - The streaming early-exit path no longer persists the input messages when a continuation token was handed out (or the run is a continuation/background run), since the resumed run persists them. - Adds regression tests covering both persistence modes for background runs, RunAsync continuation resume, streaming break-then-resume, and the abandoned stream that emitted a continuation token.
GitHub Actions
Inactive
GitHub Actions
Inactive
There was a problem hiding this comment.
🟡 Changes recommended
Injected messages reach the model but are omitted from persisted history when per-service-call persistence, message injection, and background responses are enabled, requiring a fix and regression tests for non-streaming and fully consumed streaming runs.
1 open issue
What changed in this PR
Addresses #9793 by preventing duplicate chat-history persistence in .NET background-response and continuation runs, leaving persistence to ChatClientAgent at the end of the run.
Changes:
- Skips per-service-call provider notifications for background and continuation runs.
- Avoids persisting inputs on early stream exit when persistence is deferred to a resumed run.
- Adds regression tests for background completion, continuation resume, and abandoned streams.
| File | Description |
|---|---|
dotnet/tests/Microsoft.Agents.AI.UnitTests/ChatClient/PerServiceCallChatHistoryPersistingChatClientTests.cs |
Adds history assertions and regression scenarios for both persistence modes. |
dotnet/src/Microsoft.Agents.AI/ChatClient/PerServiceCallChatHistoryPersistingChatClient.cs |
Defers background and continuation persistence to the agent and guards early-exit persistence. |
Review effort: Balanced
| // Continuation/background run — the agent's forced end-of-run handles session | ||
| // ConversationId and persistence. Notifying the providers here as well would | ||
| // persist the same messages twice. | ||
| return response; |
There was a problem hiding this comment.
I don't believe this comment is currently actionable without some really hacky changes, since we don't have a way to distinguish between injected messages and non-injected. Let's not address this without more thought.
| // Continuation/background runs, and runs that handed out a continuation token, are | ||
| // excluded because the (possibly resumed) run persists these inputs itself at the | ||
| // end of the run; persisting here too would duplicate them. | ||
| if (!loopExitedNormally && !serviceErrorOccurred && !isContinuationOrBackground && !continuationTokenObserved) |
There was a problem hiding this comment.
This condition introduces a case where the user message is no longer saved:
- Start a streaming run with
RequirePerServiceCallChatHistoryPersistence = trueandAllowBackgroundResponses = true. - Receive an update with some answer text, but no continuation token.
- Stop reading the stream with
break.
ChatClientAgent does force end-of-run persistence when AllowBackgroundResponses is true, even if no continuation token was returned. However, stopping the stream early skips that code: its finally block only disposes the stream. This new !isContinuationOrBackground check also skips the cleanup that previously saved the input messages. As a result, the user message is lost from saved history, and there is no continuation token to resume with.
I reproduced this by extending RunStreamingAsync_PersistsInputMessages_WhenConsumerAbandonsEnumerationAsync to run with background responses both disabled and enabled. On this PR, 37 tests in the class passed and the new background-enabled case failed because the history provider was never notified. Removing only !isContinuationOrBackground from this cleanup condition made all 38 tests pass.
Please preserve input-message saving when a stream stops before any continuation token has been received, and add this case to the tests. The completed-call guards can still skip the duplicate notifications as intended.
…ound run is abandoned before any token The early-exit persistence in PerServiceCallChatHistoryPersistingChatClient was skipped for all continuation/background runs. That is only correct once a continuation token has been handed out, because the resumed run persists the inputs itself. If the consumer stops the stream before any token is received, nothing resumes the run and ChatClientAgent's forced end-of-run persistence never executes (its finally block only disposes the stream), so the user message was silently dropped from history. Gate the early-exit persistence on the observed continuation token only, and add regression tests for an abandoned background run and for a resumed run that is abandoned again.
…nses and continuation tokens (#9793)
When RequirePerServiceCallChatHistoryPersistence is enabled, runs that use AllowBackgroundResponses or that are resumed with a continuation token persisted the same messages twice: once per service call from PerServiceCallChatHistoryPersistingChatClient and again from the forced end-of-run persistence in ChatClientAgent.
Motivation & Context
Description & Review Guide
Related Issue
Fixes #
AI Assistance
AI assistance details:
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.