Skip to content

.NET: [BREAKING] fix: scope workflow MCP clients to invocations - #9722

Open
Vincent Biret (baywet) wants to merge 6 commits into
mainfrom
fix/mcp-workflow-session-isolation-revamp-dotnet
Open

Vincent Biret (baywet) wants to merge 6 commits into
mainfrom
fix/mcp-workflow-session-isolation-revamp-dotnet

Conversation

@baywet

@baywet Vincent Biret (baywet) commented Oct 26, 2026 •

Copy link
Copy Markdown
Member

Motivation & Context

Unattended workflow tool calls need deterministic client ownership without retaining
live MCP clients across completed calls, approval waits, or checkpoint resumes.
Keep protocol client lifetime separate from reusable HTTP infrastructure.

Description & Review Guide

  • What are the major changes? Remove initialized-client caching and its header
    hashing. Each invocation owns and disposes its MCP client and transport. Add an
    IHttpClientFactory constructor for owned, per-invocation HTTP wrappers over
    factory-managed handlers. The non-DI fallback owns a dedicated HTTP pipeline;
    callback-supplied clients remain borrowed. Shutdown drains active calls, while
    admission limits and invocation deadlines remain host policy.
    Recover and revise ADR 0047, add regression coverage, and update consumer guidance.
    Migrate both MCP workflow samples to application-owned HTTP factories with
    handler rotation and origin-bound, per-request token acquisition, removing
    manual tracking of HTTP clients.
  • What is the impact of these changes? The no-provider path no longer retains
    implicit server-side MCP session state between invocations. This is an intentional
    behavioral breaking change. Applications needing that continuity should use an
    explicitly scoped custom handler or explicit remote state handles. Provider callback
    frequency, caller-owned HTTP clients, and existing public API signatures are
    unchanged; the factory constructor is additive and recorded for every target.
    Applications configure named-client transport policy and a finite handler lifetime.
  • What do you want reviewers to focus on? Invocation cleanup and shutdown ordering,
    factory versus borrowed-client ownership, application transport policy, and
    migration from cached sessions.

From dotnet/, the original regression was executed before implementation edits:
dotnet test --project tests\Microsoft.Agents.AI.Workflows.Declarative.Mcp.UnitTests -f net10.0 --filter-method '*NoProvider_SequentialInvocations_UseSeparateSessionsAsync' --no-progress
failed because the two returned session identities were equal. The same regression
then passed with its distinct-session assertion unchanged.

The endpoint-validation regression also failed before its correction:
dotnet test --project tests\Microsoft.Agents.AI.Workflows.Declarative.Mcp.UnitTests -f net10.0 --no-restore --filter-method '*UnsupportedScheme_DoesNotAcquireHttpClientAsync' --no-progress
failed in all six cases because HTTP resources were acquired before SDK validation.
The same command passed all six cases after moving SDK options validation before
client acquisition, covering default, callback, and factory paths.

Completed validation:

  • dotnet build src\Microsoft.Agents.AI.Workflows.Declarative.Mcp --no-restore --tl:off --verbosity quiet
    • Passed on every supported target framework; zero warnings and zero errors.
  • dotnet test --project tests\Microsoft.Agents.AI.Workflows.Declarative.Mcp.UnitTests --no-restore --no-progress
    • Passed: 224 tests across net10.0 and net472, with no failures or skips.
  • dotnet build agent-framework-dotnet.slnx --artifacts-path $artifacts -p:CustomBeforeDirectoryBuildProps=$outputProps --tl:off --verbosity quiet -m:2
    • Passed on factory revision 9cb20a4e3: full solution, zero warnings and errors.
      Temporary output was placed
      on another drive after disk exhaustion; a validation-only props file gave
      duplicate Client/Server sample projects unique artifact directory names.
      No repository build configuration was changed for this workaround.
  • dotnet test --solution mcp-factory-unit-validation.slnx -f net10.0 --no-restore --no-progress --max-parallel-test-modules 2 --ignore-exit-code 8
    • Failed on factory revision 9cb20a4e3: 9,810 passed, 63 failed, 76 skipped. The temporary solution was
      generated with the documented filter script, selecting unit tests and
      excluding samples. Failures were in unchanged projects: 61 AgentHooks tests
      could not load agent_hooks_ffi, one Windows symlink-cleanup test failed
      with UnauthorizedAccessException, and one hosting assertion failed.
      The hosting case passed in isolation; the native-loading and symlink
      failures reproduced in isolation. The MCP tests also passed in this run.
  • dotnet format src\Microsoft.Agents.AI.Workflows.Declarative.Mcp --no-restore --verbosity quiet
    • Passed.
  • dotnet format tests\Microsoft.Agents.AI.Workflows.Declarative.Mcp.UnitTests --no-restore --verbosity quiet
    • Passed.
  • dotnet build samples\03-workflows\Declarative\InvokeMcpTool --no-restore --tl:off --verbosity quiet
    • Passed: zero warnings and zero errors.
  • dotnet build samples\03-workflows\Declarative\InvokeFoundryToolboxMcp --no-restore --tl:off --verbosity quiet
    • Passed: zero warnings and zero errors.
  • dotnet format samples\03-workflows\Declarative\InvokeMcpTool --no-restore --verbosity quiet
    • Passed.
  • dotnet format samples\03-workflows\Declarative\InvokeFoundryToolboxMcp --no-restore --verbosity quiet
    • Passed.

Integration tests were not run locally. Markdown link checking could not run
locally because the configured npm policy blocked a dependency fetch.
Neither updated sample was run against live services because
FOUNDRY_PROJECT_ENDPOINT was not set.

Related Issue

N/A - maintainer-directed bugfix; no new issue was opened.
This replaces the workflow-lifetime connection ownership approach proposed in #9545
with invocation-owned clients and reusable HTTP handlers. Python is out of scope.

AI Assistance

  • No material AI assistance was used.
  • This is an AI-assisted contribution. I reviewed, understood, and verified all submitted content and accept responsibility for it.

AI assistance details: Copilot assisted with implementation, regression tests,
documentation, validation, and this PR description.

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 links to an agreed issue with no competing open PR, or the Related Issue section documents a trivial-change or repository-automation exception.
  • 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.

Separate unattended workflow state continuity from MCP client security isolation and document protocol evolution.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 27dcc08f-0b3a-40a6-a3ad-2c45eafce159
Give each invocation its own MCP client and transport while reusing the handler-owned HTTP pipeline. Drain active invocations before shutdown and document the changed cross-call session lifetime.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 27dcc08f-0b3a-40a6-a3ad-2c45eafce159
@baywet Vincent Biret (baywet) added the breaking change Usage: [PRs], Target: all PRs that introduce changes that are not backward compatible label Oct 26, 2026
Copilot AI balanced review requested due to automatic review settings October 26, 2026 16:44
@github-actions github-actions Bot changed the title .NET: fix: scope workflow MCP clients to invocations .NET: [BREAKING] fix: scope workflow MCP clients to invocations Oct 26, 2026
@agent-framework-automation agent-framework-automation Bot added documentation Usage: [Issues, PRs], Target: documentation in the code base and learn docs .NET Usage: [Issues, PRs], Target: .Net workflows Usage: [Issues, PRs], Target: Workflows labels Oct 26, 2026

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.

🔵 Needs a closer look

The breaking lifetime contract and shared-transport design require maintainer sign-off, with connection refresh still unresolved.

1 open issue
What changed in this PR

This PR changes the .NET declarative MCP handler from cached sessions to invocation-owned clients, separating protocol lifetime from reusable HTTP infrastructure.

Changes:

  • Creates and disposes MCP clients per invocation while sharing the default HTTP pipeline.
  • Extends lifetime tests for session isolation, cleanup, concurrency, and shutdown.
  • Revises ADR 0047 and documents the breaking change and migration options.
File Description
skills/​agent-framework-builder-dotnet/​references/​declarative.md Adds consumer guidance on invocation lifetime and ownership.
dotnet/​tests/​Microsoft.Agents.AI.Workflows.Declarative.Mcp.UnitTests/​DefaultMcpToolHandlerTests.cs Removes obsolete cache tests and updates comments.
dotnet/​tests/​Microsoft.Agents.AI.Workflows.Declarative.Mcp.UnitTests/​DefaultMcpToolHandlerLifetimeTests.cs Tests separate sessions, shared transport, and cleanup ordering.
dotnet/​src/​Microsoft.Agents.AI.Workflows.Declarative/​README.md Documents session-lifetime changes and migration guidance.
dotnet/​src/​Microsoft.Agents.AI.Workflows.Declarative.Mcp/​DefaultMcpToolHandler.cs Replaces session caching with invocation ownership and shutdown draining.
docs/​decisions/​0047-dotnet-workflow-owned-mcp-sessions.md Records the revised lifetime design and compatibility considerations.

Review effort: Balanced

Comment thread dotnet/src/Microsoft.Agents.AI.Workflows.Declarative.Mcp/DefaultMcpToolHandler.cs Outdated

@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.

MAF Automated Review — Iteration 1

Result: Findings reported
Scope: full PR (2 commit(s)): bc8520df0486, 964ac704e3f2
Model: gpt-5.6-sol

Overview

The PR cleanly moves initialized MCP clients to invocation scope while preserving caller-owned clients, per-origin credential isolation, and coordinated cleanup through explicit lifecycle tests. The disposal and nested-invocation guards are strong, but unrestricted admission lets stalled calls retain process resources and block shutdown. The newly shared fallback HTTP pool also lacks the DNS-refresh policy required for a handler-lifetime integration.

Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
2 verified findings remained after source verification (1 high, 1 medium) across 1 file. Details are attached to the affected lines below.

Affected areas: dotnet/src/Microsoft.Agents.AI.Workflows.Declarative.Mcp/DefaultMcpToolHandler.cs

Comment thread dotnet/src/Microsoft.Agents.AI.Workflows.Declarative.Mcp/DefaultMcpToolHandler.cs Outdated
Create invocation-owned named factory clients, retain dedicated non-DI fallback and borrowed callback clients, and document host-owned admission and deadlines.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 27dcc08f-0b3a-40a6-a3ad-2c45eafce159

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.

🔵 Needs a closer look

The breaking lifetime and ownership changes require maintainer review, and an unresolved cleanup defect also needs correction.

1 open issue
1 resolved since last review

Review effort: Balanced

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 27dcc08f-0b3a-40a6-a3ad-2c45eafce159

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.

🔵 Needs a closer look

The intentional session-lifetime breaking change and proposed ownership ADR require final maintainer review of migration implications and transport contracts.

No open issues 🎉

Review effort: Balanced

@github-code-quality

github-code-quality Bot commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Code coverage overview

Languages: C#

C# / code-coverage/dotnet

main 3f61bf5 fix/mcp-workflow-ses... 51d7639 +/-
86% 86% 0%
View coverage of the 1 most impacted file.
File main 3f61bf5 fix/mcp-workflow-ses... 51d7639 +/-
/home/runner/wo...pToolHandler.cs 88% 89% +1%

ℹ️ Calculations represent line coverage. Results may change as calculations are updated.


Updated October 26, 2026 19:29 UTC

@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.

MAF Automated Review — Iteration 2

Result: Findings reported
Scope: 11 net-new commit(s): 3f61bf5bdfc0, 3f2705269ab8, 1c19834d996b, 0c1e6eed959c, f86c0ebd86cd, 172ed9139441, 18adf808a57e, 1168c001dd22, 9cb20a4e36b0, bfeec8fe1be8, 9a273cb401c5
Model: gpt-5.6-sol

Overview

The review found 2 verified inline finding(s).

Reviewed the supplied incremental change set across correctness, security/reliability, architecture, and failure behavior.
2 verified findings remained after source verification (1 high, 1 medium) across 1 file. Details are attached to the affected lines below.

Affected areas: dotnet/src/Microsoft.Agents.AI.Workflows.Declarative.Mcp/DefaultMcpToolHandler.cs

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 27dcc08f-0b3a-40a6-a3ad-2c45eafce159

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.

🔵 Needs a closer look

The breaking session-lifetime change and revised ownership design need maintainer sign-off, with live-service integration validation still outstanding.

No open issues 🎉

Review effort: Balanced

This branch was successfully deployed

1 active and 1 inactive deployments
integration — 51d7639c Deployed Oct 26, 2026 by baywet via dotnet-foundry-hosted-it #31270
github-app-auth — 51d7639c Deployed Oct 26, 2026 by baywet via add_label #26229
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change Usage: [PRs], Target: all PRs that introduce changes that are not backward compatible documentation Usage: [Issues, PRs], Target: documentation in the code base and learn docs .NET Usage: [Issues, PRs], Target: .Net workflows Usage: [Issues, PRs], Target: Workflows

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants