Skip to content

fix: restart mcp loop during reload - #46520

Open
itsflownium wants to merge 1 commit into
NousResearch:mainfrom
itsflownium:fix/mcp-reload-stdio-46512
Open

itsflownium wants to merge 1 commit into
NousResearch:mainfrom
itsflownium:fix/mcp-reload-stdio-46512

Conversation

@itsflownium

Copy link
Copy Markdown

Summary

  • Add a serialized MCP reload helper that fully shuts down existing servers before rediscovery.
  • Wait for MCP loop startup readiness and for loop-thread shutdown before closing/reusing loop state.
  • Route TUI, CLI, and gateway reload paths through the shared helper.

Validation

  • scripts/run_tests.sh tests/tools/test_mcp_stability.py tests/gateway/test_mcp_reload_refreshes_cached_agents.py
  • scripts/run_tests.sh tests/tools/test_mcp_tool.py tests/tools/test_mcp_cancelled_error_propagation.py
  • scripts/run_tests.sh tests/cli/test_cli_loading_indicator.py tests/hermes_cli/test_mcp_reload_confirm_gate.py
  • $HOME/.hermes/hermes-agent/venv/bin/python -m py_compile tools/mcp_tool.py tui_gateway/server.py cli.py gateway/run.py tests/tools/test_mcp_stability.py tests/gateway/test_mcp_reload_refreshes_cached_agents.py
  • $HOME/.hermes/hermes-agent/venv/bin/ruff check tools/mcp_tool.py tui_gateway/server.py cli.py gateway/run.py tests/tools/test_mcp_stability.py tests/gateway/test_mcp_reload_refreshes_cached_agents.py

Refs #46512

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery comp/cli CLI entry point, hermes_cli/, setup wizard tool/mcp MCP client and OAuth labels Jun 15, 2026
@liuhao1024

Copy link
Copy Markdown

Verification: looks good

Reviewed the full diff. The serialization approach is well-structured:

  • _mcp_lifecycle_lock (RLock) guards loop lifecycle; _mcp_reload_lock (Lock) serializes full reload cycles — no deadlock since they are separate lock objects
  • _ensure_mcp_loop now uses threading.Event for ready signaling with a bounded 2s timeout — good fail-safe
  • _stop_mcp_loop returns bool and uses a bounded 15s join — the old 5s hardcoded timeout was tight for stdio transports with anyio cleanup
  • reload_mcp_servers() raises RuntimeError if the old loop did not stop cleanly — prevents discovery on a partially shut down loop
  • All three UI entry points (CLI, gateway, TUI) updated to use reload_mcp_servers() instead of manually stitching shutdown+discover

One minor note: the test test_reload_mcp_servers_discovers_on_fresh_loop verifies the old loop thread is joined and a new loop is created, which is the critical path. Test coverage looks solid.

@teknium1

Copy link
Copy Markdown
Collaborator

Thanks for consolidating the reload lifecycle behind one helper. The premise remains present on current main: tools/mcp_tool.py:5611-5622 clears loop references and joins for five seconds without checking whether the old thread is still alive, while cli.py:10757-10760, gateway/run.py:14080-14083, and tui_gateway/server.py:11560-11561 immediately rediscover afterward.

Problems

  • The added success-path test in tests/tools/test_mcp_stability.py does not exercise the helper's new refusal path: shutdown_mcp_servers() -> False must raise and must not call discovery.

Suggested changes

  • Add a unit test for that failed-stop branch, asserting RuntimeError and that discover_mcp_tools() was not called.

Automated hermes-sweeper review.

@teknium1 teknium1 added 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 labels Jul 14, 2026

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

comp/cli CLI entry point, hermes_cli/, setup wizard comp/gateway Gateway runner, session dispatch, delivery 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 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.

4 participants