Skip to content

fix(mcp): reconnect stale server entries - #37772

Closed
LeonSGP43 wants to merge 1 commit into
NousResearch:mainfrom
LeonSGP43:codex/fix-37768-mcp-reconnect
Closed

fix(mcp): reconnect stale server entries#37772
LeonSGP43 wants to merge 1 commit into
NousResearch:mainfrom
LeonSGP43:codex/fix-37768-mcp-reconnect

Conversation

@LeonSGP43

Copy link
Copy Markdown
Contributor

Summary

  • reconnect configured MCP servers when a stale _servers[name] entry exists but its session has already dropped to None
  • keep register_mcp_servers() from treating disconnected entries as healthy, which avoids exposing handlers that only return "MCP server '<name>' is not connected"
  • add a targeted regression test covering reconnect of a stale same-name server entry

Fixes #37768.

Verification

  • /Users/leongong/Desktop/LeonProjects/worktrees/hermes-agent/.base/.venv/bin/pytest -o addopts='' tests/tools/test_mcp_tool.py -q -k 'TestRegisterMcpServers'
  • /Users/leongong/Desktop/LeonProjects/worktrees/hermes-agent/.base/.venv/bin/ruff check tools/mcp_tool.py tests/tools/test_mcp_tool.py
  • git diff --check

@alt-glitch alt-glitch added type/bug Something isn't working tool/mcp MCP client and OAuth P2 Medium — degraded but workaround exists labels Jun 3, 2026

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: Approved

Overview

Fixes MCP server reconnection: when an MCP server entry becomes stale (connection dropped), the agent now properly reconnects instead of silently using a dead session.

✅ Looks Good

  • Small, focused fix: 33 additions, 3 deletions
  • Addresses real failure mode in long-running gateway sessions
  • Properly detects and re-establishes stale MCP connections
  • No security concerns
  • The fix is defensive: handles edge case without breaking existing reconnect logic

Reviewed by Hermes Agent

@DavidMetcalfe

Copy link
Copy Markdown
Contributor

The PR implements the same fix as #37899 — reconnecting servers whose session is None. A few points to consider before merging:

1. TOCTOU / duplicate-task risk
The check getattr(_servers[k], "session", None) is None runs under _lock, but the background MCPServerTask updates self.session without holding that lock. This can lead to two concurrent reconnection attempts for the same server. The mitigation: cancel the old task and delete the stale entry before the reconnection logic runs, so the race window is eliminated.

# Cancel lingering tasks for stale entries
for k in list(_servers.keys()):
    srv = _servers[k]
    if getattr(srv, "session", None) is None:
        old_task = getattr(srv, "_task", None)
        if old_task and not old_task.done():
            old_task.cancel()
        del _servers[k]

2. Stale-task cleanup
When a stale entry is detected, the existing MCPServerTask._task may still be running. Canceling that task (and deleting the stale entry) before the reconnection logic prevents resource leaks and duplicate loops.

3. Test coverage
#37899 adds two tests: one for a stale server reconnecting and another ensuring a healthy server is skipped. This PR only includes the stale-reconnect test. Adding a "healthy-skip" test (or a mixed-state test with both stale and healthy servers) would guard against regressions.

If you'd like, I can help back-port the cleanup code and the extra test from #37899. Let me know!

teknium1 added a commit that referenced this pull request Jul 6, 2026
register_mcp_servers now nudges cached entries whose session is None
via _signal_reconnect, so a new agent session recovers a parked server
immediately instead of waiting up to _PARKED_RETRY_INTERVAL for the
next self-probe (#50170). Gate-check idea credit: @izumi0uu (#50184),
@LeonSGP43 (#37772), @Tranquil-Flow (#37899).
teknium1 added a commit that referenced this pull request Jul 6, 2026
register_mcp_servers now nudges cached entries whose session is None
via _signal_reconnect, so a new agent session recovers a parked server
immediately instead of waiting up to _PARKED_RETRY_INTERVAL for the
next self-probe (#50170). Gate-check idea credit: @izumi0uu (#50184),
@LeonSGP43 (#37772), @Tranquil-Flow (#37899).
@teknium1

teknium1 commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Closing with credit — you were the earliest (Jun 3, 01:27) to propose rediscovering stale entries in the register gate. The design went a different way: the run task now never exits (parks + self-probes, PR #59222), so respawning a duplicate task would race the always-alive one. Instead, register_mcp_servers now nudges cached entries whose transport is down via _reconnect_event (PR #59331, merged) — same outcome (new sessions recover dead servers immediately), no task duplication. Credited in the salvage commit. Thanks!

@teknium1 teknium1 closed this Jul 6, 2026
habarmc1223-sudo pushed a commit to habarmc1223-sudo/hermes-agent-fluxmem that referenced this pull request Jul 8, 2026
register_mcp_servers now nudges cached entries whose session is None
via _signal_reconnect, so a new agent session recovers a parked server
immediately instead of waiting up to _PARKED_RETRY_INTERVAL for the
next self-probe (NousResearch#50170). Gate-check idea credit: @izumi0uu (NousResearch#50184),
@LeonSGP43 (NousResearch#37772), @Tranquil-Flow (NousResearch#37899).
santhreal pushed a commit to santhreal/hermes-agent that referenced this pull request Jul 13, 2026
register_mcp_servers now nudges cached entries whose session is None
via _signal_reconnect, so a new agent session recovers a parked server
immediately instead of waiting up to _PARKED_RETRY_INTERVAL for the
next self-probe (NousResearch#50170). Gate-check idea credit: @izumi0uu (NousResearch#50184),
@LeonSGP43 (NousResearch#37772), @Tranquil-Flow (NousResearch#37899).
Gravezzz pushed a commit to Gravezzz/hermes-agent that referenced this pull request Jul 21, 2026
register_mcp_servers now nudges cached entries whose session is None
via _signal_reconnect, so a new agent session recovers a parked server
immediately instead of waiting up to _PARKED_RETRY_INTERVAL for the
next self-probe (NousResearch#50170). Gate-check idea credit: @izumi0uu (NousResearch#50184),
@LeonSGP43 (NousResearch#37772), @Tranquil-Flow (NousResearch#37899).
leewenjie pushed a commit to leewenjie/hermes-agent that referenced this pull request Aug 7, 2026
register_mcp_servers now nudges cached entries whose session is None
via _signal_reconnect, so a new agent session recovers a parked server
immediately instead of waiting up to _PARKED_RETRY_INTERVAL for the
next self-probe (NousResearch#50170). Gate-check idea credit: @izumi0uu (NousResearch#50184),
@LeonSGP43 (NousResearch#37772), @Tranquil-Flow (NousResearch#37899).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

MCP tool call returns 'not connected' while hermes mcp test passes

5 participants