fix: propagate CancellationToken through async MCP tooling and observability call chains (fixes #223)#224
Draft
fix: propagate CancellationToken through async MCP tooling and observability call chains (fixes #223)#224
Conversation
Co-authored-by: rido-min <14916339+rido-min@users.noreply.github.com> Agent-Logs-Url: https://github.com/microsoft/Agent365-dotnet/sessions/2bd402aa-e691-40be-8866-7ab931a2f87f
…ability call chains (fixes #223) Co-authored-by: rido-min <14916339+rido-min@users.noreply.github.com> Agent-Logs-Url: https://github.com/microsoft/Agent365-dotnet/sessions/2bd402aa-e691-40be-8866-7ab931a2f87f
Copilot created this pull request from a session on behalf of
rido-min
March 25, 2026 00:51
View session
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
...icrosoft.Agents.A365.Observability.Runtime.IntegrationTests/Agent365ExporterAsyncE2ETests.cs
Fixed
Show fixed
Hide fixed
...icrosoft.Agents.A365.Observability.Runtime.IntegrationTests/Agent365ExporterAsyncE2ETests.cs
Fixed
Show fixed
Hide fixed
...icrosoft.Agents.A365.Observability.Runtime.IntegrationTests/Agent365ExporterAsyncE2ETests.cs
Fixed
Show fixed
Hide fixed
|
|
||
| // Act | ||
| var token = options.TokenResolver!("test-agent", "test-tenant"); | ||
| var token = options.TokenResolver!("test-agent", "test-tenant", default); |
| // Act | ||
| var token = options.TokenResolver!("test-agent", "test-tenant"); | ||
|
|
||
| var token = options.TokenResolver!("test-agent", "test-tenant", default); |
...icrosoft.Agents.A365.Observability.Runtime.IntegrationTests/Agent365ExporterAsyncE2ETests.cs
Fixed
Show fixed
Hide fixed
...icrosoft.Agents.A365.Observability.Runtime.IntegrationTests/Agent365ExporterAsyncE2ETests.cs
Fixed
Show fixed
Hide fixed
...icrosoft.Agents.A365.Observability.Runtime.IntegrationTests/Agent365ExporterAsyncE2ETests.cs
Fixed
Show fixed
Hide fixed
…null' Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
…null' Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
… IDisposable' Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
…ect' Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
Member
|
@copilot fix the CI pipeline |
| } | ||
|
|
||
| this._receivedRequest.Should().BeTrue("Exporter should make the expected HTTP request."); | ||
| this._receivedContent.Should().NotBeNull("Exporter should send a request body."); |
...icrosoft.Agents.A365.Observability.Runtime.IntegrationTests/Agent365ExporterAsyncE2ETests.cs
Fixed
Show fixed
Hide fixed
| } | ||
|
|
||
| this._receivedRequest.Should().BeTrue("Exporter should make the expected HTTP request."); | ||
| this._receivedContent.Should().NotBeNull("Exporter should send a request body."); |
| receivedContents.Add(req.Content?.ReadAsStringAsync().GetAwaiter().GetResult() ?? ""); | ||
| return new HttpResponseMessage(System.Net.HttpStatusCode.OK); | ||
| }); | ||
| var httpClient = new HttpClient(handler); |
Comment on lines
+329
to
+343
| foreach (var content in receivedContents) | ||
| { | ||
| using var doc = JsonDocument.Parse(content); | ||
| var root = doc.RootElement; | ||
| var spans = root | ||
| .GetProperty("resourceSpans")[0] | ||
| .GetProperty("scopeSpans")[0] | ||
| .GetProperty("spans") | ||
| .EnumerateArray(); | ||
|
|
||
| allOperationNames.AddRange( | ||
| spans | ||
| .Select(span => this.GetAttribute(span.GetProperty("attributes"), "gen_ai.operation.name")) | ||
| .Where(opName => opName != null)!); | ||
| } |
…broke CI build Co-authored-by: rido-min <14916339+rido-min@users.noreply.github.com> Agent-Logs-Url: https://github.com/microsoft/Agent365-dotnet/sessions/7825aa1c-1b48-439e-9e3b-2500cba9fd81
Contributor
Author
Fixed in 9802b82. Three Copilot Autofix commits ( |
Contributor
There was a problem hiding this comment.
Pull request overview
Propagates CancellationToken through the MCP tooling registration/enumeration flow and the observability exporter token-resolution flow to fix issue #223 (CancellationTokenSource has been disposed) and improve end-to-end cancellation behavior across async call chains.
Changes:
- Add
CancellationToken(with defaults) to key MCP server/configuration and tool registration async methods, and pass it through to HTTP + MCP client calls. - Update observability token cache + exporter token resolver delegate/signatures to accept and propagate
CancellationToken. - Update unit/integration tests and Moq setups to match the new signatures.
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Tooling/Extensions/SemanticKernel/Services/McpToolRegistrationService.cs | Passes CancellationToken into MCP tool enumeration from Semantic Kernel integration. |
| src/Tooling/Extensions/SemanticKernel/Services/IMcpToolRegistrationService.cs | Adds CancellationToken parameter to public Semantic Kernel MCP registration API. |
| src/Tooling/Extensions/AzureAIFoundry/Services/McpToolRegistrationService.cs | Propagates CancellationToken and removes blocking .GetAwaiter().GetResult() usage. |
| src/Tooling/Extensions/AzureAIFoundry/Services/IMcpToolRegistrationService.cs | Adds CancellationToken to Azure AI Foundry MCP registration APIs. |
| src/Tooling/Extensions/AgentFramework/Services/McpToolRegistrationService.cs | Propagates CancellationToken through Agent Framework MCP tool registration APIs. |
| src/Tooling/Extensions/AgentFramework/Services/IMcpToolRegistrationService.cs | Adds CancellationToken parameters to public Agent Framework MCP registration API. |
| src/Tooling/Core/Services/McpToolServerConfigurationService.cs | Threads CancellationToken through server listing, gateway fetch, MCP client creation, and tool listing. |
| src/Tooling/Core/Services/McpToolServerConfigurationService.ToolEnumeration.cs | Threads CancellationToken through parallel tool enumeration across servers. |
| src/Tooling/Core/Services/IMcpToolServerConfigurationService.cs | Adds CancellationToken parameters to public MCP server configuration interface. |
| src/Tests/Microsoft.Agents.A365.Tooling.Extensions.SemanticKernel.Tests/Services/McpToolRegistrationServiceTests.cs | Updates Moq setups/verifications for new CancellationToken parameter. |
| src/Tests/Microsoft.Agents.A365.Tooling.Extensions.AzureAIFoundry.Tests/McpToolRegistrationServiceTests.cs | Updates Moq setups/verifications for new CancellationToken parameter. |
| src/Tests/Microsoft.Agents.A365.Tooling.Extensions.AgentFramework.Tests/Services/McpToolRegistrationServiceTests/McpToolRegistrationServiceTestBase.cs | Updates common test mock wiring for CancellationToken propagation. |
| src/Tests/Microsoft.Agents.A365.Tooling.Extensions.AgentFramework.Tests/Services/McpToolRegistrationServiceTests/GetMcpToolsAsync_Tests.cs | Updates verification to include CancellationToken. |
| src/Tests/Microsoft.Agents.A365.Tooling.Extensions.AgentFramework.Tests/Services/McpToolRegistrationServiceTests/AddToolServersToAgent_Tests.cs | Updates verification to include CancellationToken. |
| src/Tests/Microsoft.Agents.A365.Tooling.Core.Tests/McpToolServerConfigurationService_ToolEnumerationTests.cs | Updates mocks for new CancellationToken parameters in server/tool enumeration. |
| src/Tests/Microsoft.Agents.A365.Observability.Runtime.Tests/Tracing/Exporters/ObservabilityBuilderExtensionsTests.cs | Updates exporter option TokenResolver lambdas to new delegate signature. |
| src/Tests/Microsoft.Agents.A365.Observability.Runtime.Tests/Tracing/Exporters/Agent365ExporterTests.cs | Updates token resolver setups/callbacks to accept CancellationToken. |
| src/Tests/Microsoft.Agents.A365.Observability.Runtime.Tests/BuilderTests.cs | Updates token resolver lambdas for builder tests. |
| src/Tests/Microsoft.Agents.A365.Observability.Runtime.IntegrationTests/Agent365ExporterE2ETests.cs | Updates token resolver lambda to new signature. |
| src/Tests/Microsoft.Agents.A365.Observability.Runtime.IntegrationTests/Agent365ExporterAsyncE2ETests.cs | Refactors integration test and updates token resolver lambda to new signature. |
| src/Tests/Microsoft.Agents.A365.Observability.Hosting.Tests/ObservabilityBuilderExtensionsTests.cs | Updates token resolver lambdas to new signature. |
| src/Tests/Microsoft.Agents.A365.Observability.Hosting.Tests/Extensions/ObservabilityServiceCollectionExtensionsTests.cs | Updates token resolver invocation to include CancellationToken. |
| src/Observability/Runtime/Tracing/Exporters/Agent365ExporterOptions.cs | Updates AsyncAuthTokenResolver delegate to accept CancellationToken. |
| src/Observability/Runtime/Tracing/Exporters/Agent365ExporterCore.cs | Updates core export path to pass CancellationToken into token resolver. |
| src/Observability/Runtime/Tracing/Exporters/Agent365ExporterAsync.cs | Wires new token resolver signature and passes CancellationToken through. |
| src/Observability/Runtime/Tracing/Exporters/Agent365Exporter.cs | Updates sync exporter’s resolver adapter to new signature. |
| src/Observability/Hosting/ObservabilityServiceCollectionExtensions.cs | Updates DI registration to produce new 3-arg token resolver delegate. |
| src/Observability/Hosting/Caching/ServiceTokenCache.cs | Adds CancellationToken parameter and simplifies async anti-pattern. |
| src/Observability/Hosting/Caching/IExporterTokenCache.cs | Adds CancellationToken parameter to token cache interface method. |
| src/Observability/Hosting/Caching/AgenticTokenCache.cs | Adds CancellationToken parameter to agentic token cache interface implementation. |
| [TestCleanup] | ||
| public void Cleanup() | ||
| { | ||
| this._httpClient?.Dispose(); |
There was a problem hiding this comment.
TestCleanup disposes only the HttpClient, but the ServiceProvider created in SetupExporterTest (and the provider created in the nested scopes test) is never disposed. This can leak background OpenTelemetry/exporter resources across tests and can also race with disposing HttpClient. Consider disposing the ServiceProvider (and/or using using var in tests) and letting it own/dispose the registered HttpClient instance.
Suggested change
| this._httpClient?.Dispose(); | |
| (this._provider as IDisposable)?.Dispose(); | |
| this._provider = null; |
| /// <param name="cancellationToken">A cancellation token to cancel the operation.</param> | ||
| /// <returns>Returns the list of MCP Servers that are configured.</returns> | ||
| Task<List<MCPServerConfig>> ListToolServersAsync(string agentInstanceId, string authToken); | ||
| Task<List<MCPServerConfig>> ListToolServersAsync(string agentInstanceId, string authToken, CancellationToken cancellationToken = default); |
There was a problem hiding this comment.
This is a public interface; changing the method signatures by adding a CancellationToken parameter is a binary-breaking change for any consumers compiled against prior versions (optional parameters don’t preserve binary compatibility). Consider keeping the existing signatures and adding new overloads (or a v2 interface) and marking the old ones [Obsolete] to preserve compatibility.
| /// <returns>Returns a new object of the kernel</returns> | ||
| /// <exception cref="ArgumentNullException"></exception> | ||
| Task AddToolServersToAgentAsync(Kernel kernel, UserAuthorization userAuthorization, string authHandlerName, ITurnContext turnContext, string? authToken = null); | ||
| Task AddToolServersToAgentAsync(Kernel kernel, UserAuthorization userAuthorization, string authHandlerName, ITurnContext turnContext, string? authToken = null, CancellationToken cancellationToken = default); |
There was a problem hiding this comment.
This public interface change (adding CancellationToken to the signature) is binary-breaking for existing compiled consumers, even with a default value. If compatibility matters, prefer adding an overload/new interface while keeping the existing signature (optionally obsolete the old one) rather than modifying the existing method.
| /// <param name="turnContext"></param> | ||
| /// <param name="authToken">Auth token to access the MCP servers</param> | ||
| /// <param name="cancellationToken">A cancellation token to cancel the operation.</param> | ||
| /// <returns>Returns a new object of the kernel</returns> |
There was a problem hiding this comment.
The XML doc <returns> says this “Returns a new object of the kernel”, but the method returns Task and appears to mutate the provided Kernel. Please update the <returns> text to describe the actual behavior (e.g., “A task that completes when tools are added”).
Suggested change
| /// <returns>Returns a new object of the kernel</returns> | |
| /// <returns>A task that completes when the MCP tool servers have been added to the provided kernel.</returns> |
Comment on lines
+30
to
+36
| Task AddToolServersToAgentAsync( | ||
| PersistentAgentsClient agentClient, | ||
| UserAuthorization userAuthorization, | ||
| string authHandlerName, | ||
| ITurnContext turnContext, | ||
| string? authToken = null); | ||
| string? authToken = null, | ||
| CancellationToken cancellationToken = default); |
There was a problem hiding this comment.
This public interface change (adding CancellationToken parameters) is binary-breaking for previously compiled consumers even with optional defaults. If you need to preserve binary compatibility, keep the existing signature and add overloads/new interface members, then obsolete the old ones over time.
Comment on lines
+38
to
+47
| @@ -41,7 +43,8 @@ Task<AIAgent> AddToolServersToAgent( | |||
| UserAuthorization userAuthorization, | |||
| string authHandlerName, | |||
| ITurnContext turnContext, | |||
| string? authToken = null); | |||
| string? authToken = null, | |||
| CancellationToken cancellationToken = default); | |||
There was a problem hiding this comment.
This public interface signature change (adding CancellationToken) is binary-breaking for consumers compiled against earlier versions; optional parameters don’t avoid that. If compatibility is required, keep the original signature and add an overload/new interface member instead of modifying the existing one.
Comment on lines
+14
to
15
| public delegate Task<string?> AsyncAuthTokenResolver(string agentId, string tenantId, CancellationToken cancellationToken = default); | ||
|
|
There was a problem hiding this comment.
Changing this public delegate signature by adding a CancellationToken parameter is a binary-breaking API change for consumers compiled against previous versions (delegate Invoke signature changes). If compatibility is needed, consider introducing a new delegate type/property (or overload pattern) while keeping the existing delegate for older consumers.
| /// Returns an observability token (cached inside the credential) or null on failure/not registered. | ||
| /// </summary> | ||
| Task<string?> GetObservabilityToken(string agentId, string tenantId); | ||
| Task<string?> GetObservabilityToken(string agentId, string tenantId, CancellationToken cancellationToken = default); |
There was a problem hiding this comment.
This public interface method signature change is binary-breaking for previously compiled implementations/consumers. If maintaining binary compatibility matters, consider keeping the original GetObservabilityToken(string, string) signature and adding a new overload (or a new interface) that accepts a CancellationToken.
Suggested change
| Task<string?> GetObservabilityToken(string agentId, string tenantId, CancellationToken cancellationToken = default); | |
| Task<string?> GetObservabilityToken(string agentId, string tenantId); | |
| /// <summary> | |
| /// Returns an observability token (cached inside the credential) or null on failure/not registered, with cancellation support. | |
| /// </summary> | |
| Task<string?> GetObservabilityToken(string agentId, string tenantId, CancellationToken cancellationToken); |
| } | ||
|
|
||
| this._receivedRequest.Should().BeTrue("Exporter should make the expected HTTP request."); | ||
| this._receivedContent.Should().NotBeNull("Exporter should send a request body."); |
Member
|
@copilot apply changes based on the comments in this thread |
…estCleanup disposal, XML doc fix Agent-Logs-Url: https://github.com/microsoft/Agent365-dotnet/sessions/bf4ab31c-8d71-4206-bb8c-d96357a4d305 Co-authored-by: rido-min <14916339+rido-min@users.noreply.github.com>
Contributor
Author
Applied all 8 reviewer suggestions in ef5e191:
Build: 0 errors/warnings. All 628 tests pass. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
.Subjectanti-pattern introduced by Copilot AutofixServiceProviderdisposal beforeHttpClientinAgent365ExporterAsyncE2ETests<returns>in SKIMcpToolRegistrationService(was "Returns a new object of the kernel")IExporterTokenCache(both with and withoutCancellationToken); updateAgenticTokenCacheandServiceTokenCacheIMcpToolServerConfigurationService(ListToolServersAsync×2,GetMcpClientToolsAsync,EnumerateToolsFromServersAsync,EnumerateAllToolsAsync) + update implementationIMcpToolRegistrationService.AddToolServersToAgentAsync+ update implementationIMcpToolRegistrationService(AddToolServersToAgent,GetMcpToolsAsync) + update implementationIMcpToolRegistrationService(AddToolServersToAgentAsync,GetMcpToolDefinitionsAndResourcesAsync) + update implementation📱 Kick off Copilot coding agent tasks wherever you are with GitHub Mobile, available on iOS and Android.