Skip to content

fix(mcp): cancel timed-out coroutines, check session inside async callback - #6665

Open
aaronlab wants to merge 1 commit into
NousResearch:mainfrom
aaronlab:fix/mcp-timeout-coroutine-leak-and-session-toctou
Open

fix(mcp): cancel timed-out coroutines, check session inside async callback#6665
aaronlab wants to merge 1 commit into
NousResearch:mainfrom
aaronlab:fix/mcp-timeout-coroutine-leak-and-session-toctou

Conversation

@aaronlab

@aaronlab aaronlab commented Apr 9, 2026

Copy link
Copy Markdown
Contributor

Summary

  • mcp_tool: cancel the in-flight Future when _run_on_mcp_loop times out, preventing zombie coroutines from accumulating on the MCP event loop
  • mcp_tool: move session-is-None check from caller thread into async _call() to eliminate TOCTOU race during reconnection

Details

Coroutine leak on timeout (HIGH)

_run_on_mcp_loop submits a coroutine to the MCP event loop via asyncio.run_coroutine_threadsafe and waits with future.result(timeout=...). On timeout, the TimeoutError propagates but the coroutine continues running on the event loop. Since MCP sessions are per-server (one session handles all tool calls), a backlog of zombie coroutines can congest the transport — no new calls complete because the session is processing replies for timed-out calls.

Fix: future.cancel() in except handler before re-raising.

Session TOCTOU race (MEDIUM)

_make_tool_handler checks server.session on the caller thread, then the actual call_tool runs later on the MCP event loop thread. If the server reconnects between these (setting session = None in the finally block of the reconnection loop), server.session is None and call_tool raises AttributeError. Same pattern in all 5 handler factories.

Fix: move the session is None check inside the async _call() coroutine.

Test plan

  • Verify timed-out MCP calls don't leave zombie coroutines
  • Verify session disconnect during call returns error JSON (not crash)
  • Run pytest tests/ -q

🤖 Generated with Claude Code

…lback

- _run_on_mcp_loop: cancel the in-flight Future when result() raises
  TimeoutError — without cancellation the coroutine lingers on the MCP
  event loop holding session resources, and a backlog of zombie
  coroutines can effectively deadlock per-server sessions
- _make_tool_handler: move the session-is-None check from the caller
  thread into the async _call() coroutine to eliminate a TOCTOU race
  where the session can disconnect between the check and the actual
  call_tool RPC, causing AttributeError on NoneType

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
@alt-glitch alt-glitch added type/bug Something isn't working tool/mcp MCP client and OAuth P2 Medium — degraded but workaround exists labels Apr 29, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for identifying two real MCP lifecycle concerns.

Problems

  • The timeout-cancellation portion is already implemented on current main: tools/mcp_tool.py:3668 cancels the future at deadline, with cancellation coverage in tests/tools/test_mcp_tool.py:888-917 (introduced by 80548f9a4fd1f33edd67c9ae415176a6b3666afc).
  • The session-race portion needs a current-main adaptation. The primary handler still prechecks server.session at tools/mcp_tool.py:3900 and later dereferences it at :3943; utility handlers retain the same pattern, e.g. :4050 and :4058. The submitted diff only changes the primary tool handler despite the body describing five factories.
  • No regression test exercises session loss between handler entry and MCP-loop execution.

Suggested changes

  • Salvage the session guard after the RPC lock is acquired, apply the invariant across all five handler factories, and add a deterministic interleaving test. Preserve the current reconnect/readiness/circuit-breaker flow.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform area/sessions Session lifecycle, resume, persistence, history labels Jul 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sessions Session lifecycle, resume, persistence, history P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state tool/mcp MCP client and OAuth type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants