Skip to content

fix(mcp): close connected servers when context startup is cancelled - #1028

Open
ranjianghong526-hash wants to merge 1 commit into
modelscope:mainfrom
ranjianghong526-hash:codex/fix-mcp-startup-cancellation
Open

ranjianghong526-hash wants to merge 1 commit into
modelscope:mainfrom
ranjianghong526-hash:codex/fix-mcp-startup-cancellation

Conversation

@ranjianghong526-hash

Copy link
Copy Markdown

Change Summary

Cancelling async with MCPClient(...) during parallel startup can leave servers that have already initialized connected. CancelledError bypasses the except Exception in __aenter__, and Python does not call __aexit__ when entry fails. An initialized stdio server continues answering tool calls after the cancelled entry.

  • Run the existing cleanup on BaseException during entry, then re-raise the original exception. Transports still close in their existing owner tasks.
  • Add real stdio MCP regressions for cancellation with one initialized/one initializing server and for normal two-server context exit.
  • Include the existing configured YAPF formatter's adjustments in the touched client file; these do not change behavior.

Related issue number

Independently discovered and reproduced on main at 9b0244a43c6a15b7d3f52d0f1c50617ba00e7268; no existing issue is being closed.

Validation

  • Windows, Python 3.12.14, and Linux, Python 3.12.15: the final related suite (tests/mcp and tests/config/test_mcp_resolver.py) passes all 36 tests on each platform.
  • Pre-fix regression/control: 1 cancellation failure and 1 normal-exit pass. This was checked on untouched Windows upstream source, and rechecked with the final tests on Windows Python 3.10.19 and Linux (retaining formatting-only changes while restoring the original exception handler).
  • Python 3.10.19: both final new regressions pass. The full related suite is 34 passed / 2 failed: the existing test_classify_mcp_failure and test_is_connection_error expect builtin TimeoutError to be the same as asyncio.TimeoutError. Both failures reproduce on the unmodified upstream client; their runtime implementation and tests match upstream byte-for-byte. They are outside this resource-ownership change. The new tests use the already-installed AnyIO timeout context to support Python 3.10.
  • Independent real stdio probe: three runs before/after on each platform. Before, the initialized server still responds pong after failed entry. After, both owner tasks are finished and all session/task/shutdown registries are empty; cancellation still propagates.
  • Applicable pre-commit hooks for the two changed files and git diff --check pass.
  • pre-commit run --all-files was run, but is not green: the clean upstream control fails the same flake8/isort/yapf/trailing-whitespace/string/line-ending hooks. All 69 flake8 diagnostics are identical. Unrelated automatic fixes are excluded from this PR.

This is scoped validation, not a full repository or live model-provider test run. The existing ready-future cancellation diagnostic is reproducible before and after; this PR addresses connected-resource ownership only.

AI assistance

Codex assisted with source/duplication investigation, real-process reproduction, the minimal fix, tests, validation and this draft.

Checklist

  • The pull request title is a good summary of the changes - it will be used in the changelog
  • Unit tests for the changes exist
  • Run pre-commit install and pre-commit run --all-files before git commit, and passed lint check. (Run; existing upstream failures described above.)
  • Documentation reflects the changes where applicable (no API/configuration change)

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant