Skip to content

.NET: Propagate ChatHistoryMemoryProvider caller cancellation - #8813

Open
quick_fox (quifox) wants to merge 1 commit into
microsoft:mainfrom
quifox:fix/chat-history-memory-cancellation
Open

quick_fox (quifox) wants to merge 1 commit into
microsoft:mainfrom
quifox:fix/chat-history-memory-cancellation

Conversation

@quifox

Copy link
Copy Markdown

Motivation & Context

ChatHistoryMemoryProvider uses best-effort handling for vector search and history storage failures, but its generic exception handlers also swallow caller-triggered cancellation. This can allow search or persistence work to continue after the caller has cancelled the operation.

Description & Review Guide

  • What are the major changes? The search and storage catch filters now propagate OperationCanceledException when the caller token is actually cancelled, with regression coverage for both caller-side and provider-side cancellation.
  • What is the impact of these changes? Caller cancellation is preserved while unrelated provider failures and provider-side cancellation continue to use the existing best-effort behavior.
  • What do you want reviewers to focus on? The filter checks the caller token so the change does not broaden failure propagation beyond caller-triggered cancellation.

Related Issue

Fixes #8811

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 title prefix.

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.

Copilot review overview

🟡 Changes recommended

The storage cancellation test lacks an UpsertAsync setup and therefore fails without exercising the intended behavior.

Review effort: Balanced
Findings: 1 High severity

Open (1)
What changed in this PR

Preserves caller cancellation while retaining best-effort handling for provider-side failures.

Changes:

  • Filters search and storage exceptions based on caller cancellation.
  • Adds cancellation regression tests.
File Description
ChatHistoryMemoryProvider.cs Propagates caller-triggered cancellation.
ChatHistoryMemoryProviderTests.cs Adds search and storage cancellation tests.

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +276 to +277
using var cts = new CancellationTokenSource();
cts.Cancel();

This branch was successfully deployed

1 active deployment
github-app-auth — 365acd7e Deployed Sep 29, 2026 by quifox via team_check #5451
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: ChatHistoryMemoryProvider swallows caller cancellation

2 participants