Python: Clean up MCP HTTP resources after failed connections - #8126
Python: Clean up MCP HTTP resources after failed connections#8126Eduard van Valkenburg (eavanvalkenburg) wants to merge 2 commits into
Conversation
Register header-hook and owned HTTP client cleanup on the MCP session exit stack so failed initialization, cancellation, reconnects, and shutdown release resources without closing caller-owned clients. Preserve the request-scoping and redirect behavior from #8039 and cover shared-client declarative cache reuse and eviction. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
GitHub Actions
Active
GitHub Actions
Active
GitHub Actions
Active
agent-framework-automation
Bot
added
documentation
There was a problem hiding this comment.
🟢 Approval recommended
The cleanup ordering and ownership behavior are consistent and comprehensively covered by targeted regressions.
Pull request overview
Ensures MCP HTTP authentication hooks and framework-owned clients are cleaned up across failures, shutdowns, and reconnects.
Changes:
- Registers client and hook cleanup on the session exit stack.
- Clears closed client references to support reconnection.
- Adds shared-client, failure, cancellation, concurrency, and cache lifecycle tests.
File summaries
| File | Description |
|---|---|
python/packages/core/agent_framework/_mcp.py |
Implements exit-stack resource cleanup. |
python/packages/core/tests/core/test_mcp_http_auth.py |
Adds MCP HTTP lifecycle regressions. |
python/packages/declarative/tests/test_default_mcp_tool_handler.py |
Tests shared-client cache cleanup. |
python/packages/core/AGENTS.md |
Documents MCP HTTP ownership semantics. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
GitHub Actions
Active
Code Coverage OverviewLanguages: Python Python / code-coverage/pythonThe overall line coverage in commit d97c41e in the Show a line coverage summary of the most covered files.
Updated |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
MAF Automated Review — Iteration 1
Result: Findings reported
Scope: full PR (1 commit(s)): 3590960149b8
Model: gpt-5.6-sol-fast
Overview
The change correctly registers tool-specific hook removal and framework-owned client closure before transport entry, preserves caller-owned clients, and clears closed client references for reconnect. Its tests provide strong coverage for transport entry, cancellation, initialization, shared-client isolation, and cache eviction. One connection setup phase remains outside cleanup-on-failure handling: server-controlled tool or prompt discovery can fail after initialization and retain the newly registered resources.
Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
1 verified finding remained after source verification (1 medium) across 1 file. Details are attached to the affected lines below.
Affected areas: python/packages/core/agent_framework/_mcp.py
Close the lifecycle-owned session when post-transport setup fails, reset discovery flags, and roll back partially loaded functions and metadata before retrying. Preserve caller-owned HTTP clients and the original setup exception. Correct the Python 3.10 cancellation expectation, explicitly type async fixture values for Zuban, and assert the joined task result instead of ignoring it. Add discovery failure, cancellation, context-entry, and paginated retry coverage. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
GitHub Actions
Active
| if failure == "cancellation": | ||
| task = asyncio.current_task() | ||
| assert task is not None | ||
| task.cancel() | ||
| await asyncio.sleep(0) |
There was a problem hiding this comment.
Are we covering cancellation of the task awaiting connect(), rather than cancellation raised inside the lifecycle owner? connect() queues work and awaits a future, so cancelling the caller cancels only that future; the owner can finish setup and leave is_connected, the authentication hook, and an owned HTTP client live even though async with never entered. Could the queued action observe caller cancellation and close its resources before completing?
| except (Exception, asyncio.CancelledError): | ||
| try: | ||
| await self._close_on_owner() |
There was a problem hiding this comment.
Should this cleanup preserve a caller-supplied session? _close_on_owner() unconditionally sets self.session to None, even though that session was never entered on _exit_stack; after a transient load_tools() failure, the next connect() silently opens the subclass URL or command instead of retrying the caller's still-live session. Could we track session ownership and only discard sessions created by the framework?
Motivation & Context
#8039 scopes provider headers per tool, but failed MCP HTTP connections can still retain authentication request hooks until an explicit close, and framework-created HTTP clients are not owned by the MCP session. This focused follow-up ties cleanup to the session exit stack so failed initialization or cancellation, normal shutdown, and reconnects release resources without replacing #8039.
Description & Review Guide
Related Issue
N/A — no public issue is linked. Follow-up to the merged #8039, limited to remaining resource-lifecycle gaps and regression coverage. The open #7892 adds static connect-time headers and addresses a different concern.
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.