Skip to content

fix(mcp): drain connecting tasks before loop shutdown - #67956

Closed
nuch1011 wants to merge 2 commits into
NousResearch:mainfrom
nuch1011:fix/mcp-one-shot-shutdown-t_1dcc96b6
Closed

fix(mcp): drain connecting tasks before loop shutdown#67956
nuch1011 wants to merge 2 commits into
NousResearch:mainfrom
nuch1011:fix/mcp-one-shot-shutdown-t_1dcc96b6

Conversation

@nuch1011

Copy link
Copy Markdown

Summary

  • drain in-flight MCP tasks on their dedicated event loop before closing it
  • make MCP task submission atomic with loop shutdown and clear stale connecting state
  • add regression coverage for connecting-server cancellation and the schedule/shutdown race

Root cause

shutdown_mcp_servers() only knew about fully registered servers. A fast one-shot command could exit while background discovery still had an MCPServerTask.run() coroutine connecting. The loop was closed first, so later coroutine finalization tried to cancel work on a closed event loop and printed RuntimeError: Event loop is closed despite exit code 0.

Verification

  • scripts/run_tests.sh tests/tools/test_mcp_tool.py tests/tools/test_mcp_stdio_watchdog.py — 219 passed
  • scripts/run_tests.sh tests/hermes_cli/test_tui_resume_flow.py — 71 passed
  • .venv/bin/ruff check tools/mcp_tool.py tests/tools/test_mcp_tool.py — passed
  • real configured-MCP smoke: hermes chat -q returned the expected response and exited 0 with no traceback, pending-task warning, or event-loop-closed error
  • same smoke against origin/main reproduced Exception ignored in: <coroutine object MCPServerTask.run ...> / RuntimeError: Event loop is closed

No credentials or secret values are included in this PR.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/tools Tool registry, model_tools, toolsets tool/mcp MCP client and OAuth needs-decision Awaiting maintainer decision before any implementation labels Jul 20, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related to #60104 and #61466. This branch covers the connecting-task/scheduling race; #60104 has a broader pre-close pending-task drain, while #61466 guards cancellation after closure. Maintainer choice is needed.

@GottZ

GottZ commented Jul 20, 2026

Copy link
Copy Markdown

This was generated by AI during triage.

Recommend consolidating on #67956 because it is the older of the two identical PRs (opened ~21 min earlier); #67961 can be closed as a duplicate. Neither has CI checks configured, both are mergeable. Reviewers: note that the one-shot drain in _stop_mcp_loop runs outside _lock and is timeout-bounded (5s drain + 5s join), so worst case is a bounded teardown delay on a hung loop thread, not a deadlock.

@nuch1011

Copy link
Copy Markdown
Author

Closed at the repository owner's direction. Superseded patch remains private; no change should be proposed against the official NousResearch branch.

@nuch1011 nuch1011 closed this Jul 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/tools Tool registry, model_tools, toolsets needs-decision Awaiting maintainer decision before any implementation P2 Medium — degraded but workaround exists 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