Skip to content

Python: stop the MCP lifecycle owner after a failed connect - #8755

Merged
Eduard van Valkenburg (eavanvalkenburg) merged 1 commit into
microsoft:mainfrom
manjunathshiva:python-mcp-lifecycle-leak-8752
Sep 28, 2026
Merged

Eduard van Valkenburg (eavanvalkenburg) merged 1 commit into
microsoft:mainfrom
manjunathshiva:python-mcp-lifecycle-leak-8752

Conversation

@manjunathshiva

Copy link
Copy Markdown
Contributor

Motivation & Context

When a connect action fails, _run_lifecycle_owner sets the exception on the caller's future and loops back to await queue.get(). The task then owns nothing and never exits, so asyncio reports Task was destroyed but it is pending! once the tool is garbage collected, as an unrelated-looking error with no link to the failed run.

Reaching this needs only a server that rejects the credentials. Agent._prepare_run_context enters every MCP tool through its AsyncExitStack, MCPTool.__aenter__ re-raises ToolException, and __aexit__ never runs because __aenter__ raised.

Description & Review Guide

  • What are the major changes? The lifecycle owner now retires after a connect that failed and left no session behind. It already retired in three situations, each guarded by having nothing connected and nothing queued: a cancelled connect, a connect whose caller never accepted the result, and an explicit close. A failed connect was the missing fourth case, so this adds it with the same guard. Three tests cover it: the owner stops after a failed connect, the tool stays usable afterwards because a later connect gets a fresh owner, and a failure that leaves a live session keeps the owner alive.
  • What is the impact of these changes? No public API changes and no change to what callers see; the ToolException and its message are identical. Repeated and concurrent failures no longer accumulate tasks, and a defensive close() after a failed connect remains safe. Since the fix is in the shared base, it covers all three transports: stdio leaks the same way with a command that cannot start, and is fixed by the same change.
  • What do you want reviewers to focus on? The not self.is_connected condition. It is load-bearing rather than defensive, because is_connected is set before tools and prompts are loaded, so a failure while loading leaves a live session that still needs this owner to close it later. Only a connect that left no session behind may retire the owner. I teeth-checked this by dropping the condition and confirming the third test fails.

Two notes that may be useful. The existing coverage for this scenario, test_connect_cleanup_on_initialization_failure, asserts only that the exit stack is closed, which is why the leaked task went unnoticed. Separately, the Could not cleanly close MCP exit stack due to cleanup error group warning on a failed connect is pre-existing, appears with and without this change, and is left alone here.

The issue also suggests calling close() from __aenter__ before re-raising. I left that out: every ToolException path runs inside _connect_on_owner, so the owner already sees the failure, and the owner-side fix additionally covers callers who use connect() directly rather than async with. Happy to add it as well if you would prefer the symmetry with the generic except Exception branch.

Related Issue

Fixes #8752

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.

When a connect action failed, _run_lifecycle_owner set the exception on the
caller's future and looped back to await queue.get(). The task then owned
nothing and never exited, so asyncio reported "Task was destroyed but it is
pending!" once the tool was garbage collected. Reaching this needs only a
server that rejects the credentials: Agent._prepare_run_context enters every
MCP tool through its AsyncExitStack, __aenter__ re-raises ToolException, and
__aexit__ never runs because __aenter__ raised.

The owner already retires in three situations, all guarded by having nothing
connected and nothing queued: a cancelled connect, a connect whose caller never
accepted the result, and an explicit close. A failed connect was the missing
fourth case, so this adds it with the same guard.

The connected check is load-bearing rather than defensive. is_connected is set
before tools and prompts are loaded, so a failure while loading leaves a live
session that still needs this owner to close it later. Only a connect that left
no session behind may retire the owner.

Verified on all three transports. Stdio leaks the same way with a command that
cannot start, so the fix is placed in the shared base rather than in
MCPStreamableHTTPTool, and callers see an unchanged ToolException.

The existing coverage for this scenario, test_connect_cleanup_on_initialization
_failure, asserts only that the exit stack is closed, which is why the leaked
task went unnoticed.
Copilot AI balanced review requested due to automatic review settings September 25, 2026 15:37
@agent-framework-automation agent-framework-automation Bot added the python Usage: [Issues, PRs], Target: Python label Sep 25, 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.

Copilot review overview

🟢 Approval recommended

The targeted lifecycle fix is correctly guarded and comprehensively tested.

Review effort: Balanced
Findings: None

What changed in this PR

Fixes #8752 by retiring idle MCP lifecycle-owner tasks after failed connections while preserving owners for live sessions.

Changes:

  • Stops owner tasks when failed connects leave no session or queued work.
  • Adds coverage for cleanup, retry, and live-session behavior.
File Description
python/​packages/​core/​agent_framework/​_mcp.py Retires unused lifecycle owners after connection failure.
python/​packages/​core/​tests/​core/​test_mcp.py Tests cleanup, retries, and live-session retention.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Sep 28, 2026
Merged via the queue into microsoft:main with commit 08eb7d9 Sep 28, 2026
53 checks passed

This branch was successfully deployed

1 active deployment
github-app-auth — 923dcd20 Deployed Sep 25, 2026 by manjunathshiva via team_check #5267
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

python Usage: [Issues, PRs], Target: Python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Python: [Bug]: MCPTool leaks its mcp-lifecycle owner task when connect() fails ("Task was destroyed but it is pending!")

3 participants