Skip to content

.NET: fix InMemoryChatHistoryProvider persisting when service stores history#7284

Merged
westey-m merged 4 commits into
microsoft:mainfrom
westey-m:dotnet-chatclientagent-perservicecall-regression-fix
Jul 24, 2026
Merged

.NET: fix InMemoryChatHistoryProvider persisting when service stores history#7284
westey-m merged 4 commits into
microsoft:mainfrom
westey-m:dotnet-chatclientagent-perservicecall-regression-fix

Conversation

@westey-m

Copy link
Copy Markdown
Contributor

Motivation & Context

When running against a service that stores chat history server-side (e.g. the OpenAI Responses API), the framework's default InMemoryChatHistoryProvider was still persisting messages into the session, so history ended up stored in two places. The session was expected to only carry the service conversation id in this case.

The root cause was in ChatClientAgent.ResolveChatHistoryProvider: it decided whether to disengage the chat history provider based solely on chatOptions.ConversationId. That value is only seeded at the start of a run (from session.ConversationId) and is never refreshed at end-of-run, whereas the service conversation id lands on session.ConversationId only after the response. So on the first turn against a service-stored-history service, the provider stayed engaged and persisted the messages. This was a regression introduced by #4974, which changed the check from session.ConversationId to chatOptions.ConversationId.

Description & Review Guide

  • What are the major changes?

    • ResolveChatHistoryProvider now takes the session and disengages any chat history provider when a service conversation id is present on either the session or the chat options (excluding the AG-UI provider and the per-service-call persistence path). The chatOptions-only check is replaced by a uniform session || options check.
    • The three call sites (NotifyProvidersOfNewMessagesAsync, NotifyProvidersOfFailureAsync, LoadChatHistoryAsync) now pass the session through, and the override-provider conflict branch keys off the same computed value.
    • Added red/green regression tests: default in-memory provider not populated when a conversation id is returned (both RunAsync and RunStreamingAsync), a multi-turn service-stored-history test that also asserts prior turns are not replayed, and an explicit-provider test.
  • What is the impact of these changes?

    • The default in-memory provider no longer double-stores history when the service manages it server-side, and multi-turn runs no longer replay history the service already owns.
    • One secondary behavior change: an explicitly-configured provider with all conflict handling disabled (Throw/Clear/Warn OnChatHistoryProviderConflict all false) is now also disengaged when the service returns a conversation id (previously it persisted the first turn only). Conflict detection (Throw/Warn/Clear) is unchanged, as it runs before persistence.
  • What do you want reviewers to focus on?

    • The interaction with PerServiceCallChatHistoryPersistingChatClient: it calls back into these methods precisely when per-service-call persistence is active and stamps a sentinel onto session.ConversationId, so the !RequiresPerServiceCallChatHistoryPersistence gate is load-bearing (verified: removing it breaks the per-service-call tests).
    • The secondary explicit-provider behavior change described above.

Related Issue

Fixes #6120

PR #6209 also targets this issue. This PR differs by disengaging any provider uniformly when a service conversation id is present (on the session or the options) and does not introduce a _usesImplicitDefaultChatHistoryProvider distinction between the default and an explicitly-configured provider.

Contribution Checklist

  • The code builds clean without any errors or warnings
  • All unit tests pass, and I have added new tests where possible
  • The PR follows the Contribution Guidelines
  • This PR is linked to an issue and there is no other open PR for this issue (see Related Issue above).
  • This is not a breaking change. If it is a breaking change, add the breaking change label (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.

Copilot AI review requested due to automatic review settings July 23, 2026 11:11
@westey-m
westey-m temporarily deployed to github-app-auth July 23, 2026 11:12 — with GitHub Actions Inactive
@westey-m
westey-m temporarily deployed to github-app-auth July 23, 2026 11:12 — with GitHub Actions Inactive
@westey-m
westey-m temporarily deployed to github-app-auth July 23, 2026 11:12 — with GitHub Actions Inactive
@agent-framework-automation agent-framework-automation Bot added the .NET Usage: [Issues, PRs], Target: .Net label Jul 23, 2026
@westey-m
westey-m temporarily deployed to github-app-auth July 23, 2026 11:13 — with GitHub Actions Inactive
@westey-m
westey-m marked this pull request as ready for review July 23, 2026 11:13
@westey-m
westey-m temporarily deployed to github-app-auth July 23, 2026 11:13 — with GitHub Actions Inactive

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated Code Review

Reviewers: 5 | Confidence: 92% | Result: All clear

Reviewed: Correctness, Security Reliability, Test Coverage, Failure Modes, Design Approach


Automated review by westey-m's agents

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a regression in .NET ChatClientAgent chat history handling so the default InMemoryChatHistoryProvider does not persist messages into the session when the backing service manages history server-side (e.g., via a returned ConversationId), avoiding double-storage and unintended history replay across turns.

Changes:

  • Update ChatClientAgent.ResolveChatHistoryProvider to consider both session.ConversationId and chatOptions.ConversationId when deciding to disengage chat history persistence for service-stored history scenarios.
  • Thread the session through internal provider notification/load call sites (NotifyProvidersOfNewMessagesAsync, NotifyProvidersOfFailureAsync, LoadChatHistoryAsync) to make the resolution decision accurate on first turn.
  • Add regression tests covering non-streaming, streaming, and multi-turn behavior for service-stored history, plus an explicit-provider behavior pin.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
dotnet/src/Microsoft.Agents.AI/ChatClient/ChatClientAgent.cs Adjusts chat history provider resolution to prevent persisting local history when the service conversation id indicates server-side history management.
dotnet/tests/Microsoft.Agents.AI.UnitTests/ChatClient/ChatClientAgent_ChatHistoryManagementTests.cs Adds regression tests ensuring the default in-memory provider is not populated when the service returns a conversation id, including streaming and multi-turn scenarios.

Comment thread dotnet/src/Microsoft.Agents.AI/ChatClient/ChatClientAgent.cs
Comment thread dotnet/src/Microsoft.Agents.AI/ChatClient/ChatClientAgent.cs
@westey-m
westey-m added this pull request to the merge queue Jul 24, 2026
Merged via the queue into microsoft:main with commit c59a65d Jul 24, 2026
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

.NET Usage: [Issues, PRs], Target: .Net

Projects

None yet

Development

Successfully merging this pull request may close these issues.

.NET: [Bug]: InMemoryChatHistoryProvider used even when service stores history (OpenAI Responses)

4 participants