fix(mcp): cancel the in-flight tool call when the caller is cancelled - #7199
Open
AtulJoshi1206 wants to merge 1 commit into
Open
AtulJoshi1206 wants to merge 1 commit into
AtulJoshi1206 wants to merge 1 commit into
Conversation
asyncio.wait does not own the futures it waits on, so _run_guarded left session.call_tool() running when its caller was cancelled. McpTool then ran its finally and released the session, dropping the pool's in-flight count to zero and stamping _session_last_used while the orphaned call was still reading the transport, which leaves _evict_idle_sessions free to close that transport underneath it. Cancel and drain the call when the wait itself is interrupted. The transport-crash branch already did exactly this, so both paths now share one _cancel_and_drain helper.
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.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
No existing issue; described below following the bug-report structure.
Describe the Bug:
SessionContext._run_guardedraces the in-flight MCP call against the session's background task withasyncio.wait:asyncio.waitdoes not own the futures it waits on. It re-raisesCancelledErrorat theawaitand leavescoro_taskrunning. So when the caller is cancelled, the livesession.call_tool()is orphaned: nothing holds it, nothing awaits it, its exception is never retrieved, and the remote call is never cancelled.Both branches that exit normally already cancel
coro_task, andtest_run_guarded_cancels_coro_when_task_dies_firstpins that contract for the transport-crash case. The cancellation path is the one exit that misses it.This path is the default:
McpTool._run_async_implcalls_run_guardedwheneverFeatureName._MCP_GRACEFUL_ERROR_HANDLINGis on. Cancellation is routine here, arriving from an aborted agent run, a human-in-the-loop interrupt, an outer timeout, or a disconnecting client.Impact:
McpTool._run_async_implreleases the session in afinally:On cancellation that
finallyruns immediately, so the pool's in-flight count drops to zero and_session_last_usedis stamped while the orphan is still reading the transport._evict_idle_sessionsis then free to close that transport underneath the orphaned call once the idle TTL passes. The pool's accounting is actively wrong for as long as the orphan lives.Steps to Reproduce:
On current
main(665ec9835), start a guarded call, cancel the caller, and check whether the inner call is still running:Observed Behavior:
The inner call runs to completion after the caller is gone.
Expected Behavior:
The in-flight call is cancelled and drained before
_run_guardedpropagates the cancellation, so the session is only released once nothing is still reading the transport.Solution:
Cancel and drain
coro_taskwhen the wait itself is interrupted, then re-raise. The existing cancel-and-drain in the transport-crash branch is identical, so this change factors both into one_cancel_and_drainhelper rather than duplicating it.BaseExceptionrather thanCancelledError, so aKeyboardInterruptorSystemExitthrough the same frame does not leak either.Testing Plan
Unit Tests:
Added
test_run_guarded_cancels_coro_when_caller_is_cancellednext to the existingtest_run_guarded_cancels_coro_when_task_dies_first, covering the mirror case. It fails onmainwith the assertion above and passes with this change.Manual End-to-End (E2E) Tests:
Driven through
SessionContextdirectly, since reaching this path end-to-end needs a live MCP server. With a started session and a call blocked on a long read, cancelling the caller task:The ordering flip is the point: after the change the inner call has already unwound by the time the caller's cancellation propagates, so
_end_session_usecannot run while the transport is still in use.Checklist
Additional context
No public API change.
_cancel_and_drainis module-private, and the normal-exit behaviour of_run_guardedis byte-for-byte what it was.🤖 Generated with Claude Code