Skip to content

fix(mcp): rediscover dead cached servers on session startup - #50184

Closed
izumi0uu wants to merge 1 commit into
NousResearch:mainfrom
izumi0uu:fix/mcp-reconnect-fresh-session-50170
Closed

fix(mcp): rediscover dead cached servers on session startup#50184
izumi0uu wants to merge 1 commit into
NousResearch:mainfrom
izumi0uu:fix/mcp-reconnect-fresh-session-50170

Conversation

@izumi0uu

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes a stale-MCP startup bug where a dead MCPServerTask could remain parked in tools.mcp_tool._servers after keepalive/reconnect failure. Later session startup only checked name in _servers, so it skipped fresh MCP discovery and the new session silently lost that server's tools.

This change only treats a cached MCP server as already connected when it still has a live session or a live background reconnect task. If the cached task is dead and detached, startup rediscovery reconnects it and re-registers its tools.

Related Issue

Fixes #50170

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • Added _server_is_connected(...) in tools/mcp_tool.py so startup gating distinguishes live/recovering MCP tasks from dead cached entries.
  • Updated register_mcp_servers() and discover_mcp_tools() to rediscover only dead cached servers, while preserving legitimate in-flight reconnect ownership.
  • Added regression tests in tests/tools/test_mcp_tool.py for both:
    • dead cached server entry must be rediscovered on new session startup
    • live background reconnect must still suppress duplicate startup connects

How to Test

  1. Reproduce the pre-fix state by leaving a stale MCP server entry in _servers with session=None and no live background task.
  2. Run ./.venv/bin/pytest -q tests/tools/test_mcp_tool.py -k "reconnects_server_entry_that_exists_but_is_not_connected or skips_server_that_is_mid_reconnect_with_live_background_task or skips_already_connected_servers or connects_new_servers or logs_summary_on_success".
  3. Confirm the new-session startup path now reconnects dead cached servers, while existing connected servers and in-flight reconnects still follow the old idempotent behavior.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 15.x

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Screenshots / Logs

Targeted validation:

  • ./.venv/bin/pytest -q tests/tools/test_mcp_tool.py -k "reconnects_server_entry_that_exists_but_is_not_connected or skips_server_that_is_mid_reconnect_with_live_background_task or skips_already_connected_servers or connects_new_servers or logs_summary_on_success"
  • ./.venv/bin/pytest -q tests/agent/test_turn_context.py -k "between_turns_refresh"
  • ./.venv/bin/pytest -q tests/test_get_tool_definitions_cache_isolation.py

A dead MCPServerTask could remain parked in _servers after keepalive/reconnect failure, so later session startup treated the server name as already connected and skipped fresh discovery. Reconnect only when the cached entry has neither a live session nor a live background task, while preserving in-flight in-process reconnect ownership.

Constraint: Keep the fix narrow to stale _servers startup gating for issue NousResearch#50170
Rejected: Force fresh connect whenever session is None | would race legitimate in-flight reconnects and duplicate a live server task
Confidence: high
Scope-risk: narrow
Directive: Treat _servers membership as authoritative only while the cached task is still live or the session is still attached
Tested: ./.venv/bin/pytest -q tests/tools/test_mcp_tool.py -k "reconnects_server_entry_that_exists_but_is_not_connected or skips_server_that_is_mid_reconnect_with_live_background_task or skips_already_connected_servers or connects_new_servers or logs_summary_on_success"; ./.venv/bin/pytest -q tests/agent/test_turn_context.py -k "between_turns_refresh"; ./.venv/bin/pytest -q tests/test_get_tool_definitions_cache_isolation.py
Not-tested: full pytest tests/ -q
@alt-glitch alt-glitch added type/bug Something isn't working comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint tool/mcp MCP client and OAuth P2 Medium — degraded but workaround exists labels Jun 21, 2026
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 — the best-engineered PR in this cluster. Your _server_is_connected gate (session None AND task done) plus mixed-state tests were the closest framing to the merged fix. The recent reconnect overhaul (PR #59222) made the task never-done (parks + self-probes), so the liveness half of your gate became a no-op; PR #59331 (merged) covers your issue #50170 by nudging cached dead-transport entries at session startup — credited to you 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

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint 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 tools silently absent in new sessions after keepalive failure

3 participants