perf(mcp): non-blocking startup via background MCP discovery - #32811
perf(mcp): non-blocking startup via background MCP discovery#32811Prontsevich wants to merge 3 commits into
Conversation
|
Related competing PRs for #29726 (MCP startup hang):
Also note #25622 (open) — fixes tool registration race on startup. Background discovery may interact with the same race window if tools aren't registered before the first user message arrives. |
418886a to
1c95d71
Compare
1c95d71 to
4e13266
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating the remaining ACP startup path. Current main still calls discover_mcp_tools() synchronously in acp_adapter/entry.py:255-256, so the ACP premise remains valid; CLI and TUI backgrounding have already landed separately (0c6e133c0, PR #35273).
Problems
- The ACP change needs the race handling already used by the other surfaces.
acp_adapter/session.py:645constructsAIAgent, whose initialization snapshots tools atagent/agent_init.py:1189-1198. ACP has nowait_for_mcp_discoveryat that point and no late refresh for configured MCP servers;acp_adapter/server.py:792-850refreshes only client-supplied ACP MCP servers. A server that finishes after session creation can therefore remain absent from that session. - The proposed test only inspects source text/AST. It does not verify a blocked discovery is non-blocking or that delayed configured tools remain available.
Suggested changes
- Reuse the shared startup helper with ACP-specific bounded snapshot/late-refresh handling that remains cache-safe before the first turn.
- Add a behavioral delayed-discovery regression test.
Automated hermes-sweeper review.
| start_background_mcp_discovery( | ||
| logger=logger, | ||
| thread_name="acp-mcp-discovery", | ||
| ) |
There was a problem hiding this comment.
Please pair this background start with ACP-side bounded snapshot/late-refresh handling. ACP constructs AIAgent in acp_adapter/session.py:645, where its tool list is snapshotted; unlike CLI/TUI, this path has no discovery wait or configured-server late refresh, so a slow server can be missing from an already-created ACP session.
…st path
Fire-and-forget MCP server connections on a daemon thread so the
gateway / CLI / ACP process becomes interactive immediately instead
of blocking on slow remote MCP servers (HTTP timeouts, sluggish
stdio boot). Previously `hermes --tui` waited 2-5 s after the splash
screen before rendering the UI while `discover_mcp_tools()` ran
synchronously on the critical path.
Changes:
- tools/mcp_tool.py: add `discover_mcp_tools_background()` — thin
wrapper that spawns `discover_mcp_tools()` on a named daemon thread
- tui_gateway/entry.py: call `discover_mcp_tools_background()` before
sending gateway.ready (replaces inline call that blocked the JSON-RPC
pipe for the TUI Ink app)
- hermes_cli/main.py:
- skip `\_prepare_agent_startup()` for TUI path — plugins, MCP, and
shell hooks are only needed by the CLI agent loop; the TUI's
gateway subprocess discovers them independently (~370 ms saved)
- fast-path in `\_make_tui_argv()`: when `dist/entry.js` exists and
is fresh, skip npm install / rebuild checks entirely (~350 ms saved)
- cli.py (`\_prepare_deferred_agent_startup`): same background pattern
for deferred startup (Termux interactive CLI)
- acp_adapter/entry.py: same pattern so ACP server launches asyncio
immediately while MCP connects in parallel
Result:
- TUI Python wrapper: ~730 ms → ~80 ms (9× faster)
- gateway.ready: ~2700 ms → ~400 ms (7× faster)
- Total TUI cold start: ~3400 ms → ~480 ms
Related: NousResearch#29726, NousResearch#29184, NousResearch#19326 (closed stale)
Closes NousResearch#29726
ACP entry.py fires MCP discovery in a background daemon thread, but _make_agent snapshots tools once at build and never re-reads the registry. Unlike CLI/TUI, ACP had no bounded wait before the snapshot and no late-refresh for configured (config.yaml) MCP servers — a reachable-but- slow server that finished after agent build was invisible for the whole session. Changes: - acp_adapter/session.py (_make_agent): call wait_for_mcp_discovery() before AIAgent construction, bounded by mcp_discovery_timeout (default ~1.5s). A dead server can't block; servers that miss the bound are picked up by the late-refresh below. - acp_adapter/server.py (_schedule_mcp_late_refresh): new method on HermesACPAgent — if discovery is still in flight after session creation, spawns an off-critical-path daemon that joins it (bounded 30s), then rebuilds the tool snapshot via the shared refresh_agent_mcp_tools helper. Cache-safe: only runs pre-first-turn (_user_turn_count/_api_call_count both 0); once the user has sent a message the snapshot is frozen, exactly as TUI PR NousResearch#48403 does. - Called from new_session, load_session, resume_session. - Mirrors the TUI pattern (tui_gateway _schedule_mcp_late_refresh, PR NousResearch#48403) and the CLI pattern (get_tool_definitions → wait_for_mcp_discovery). Tests: - Replace the AST-based test (source-text inspection) with three behavioral regression tests in tests/acp_adapter/test_acp_mcp_discovery.py: 1. Blocked discovery does not block startup (non-blocking contract) 2. Delayed discovery lands tools via late-refresh (pre-first-turn) 3. Late-refresh is cache-safe: skips rebuild after first turn Addresses teknium1 review on PR NousResearch#32811.
4dfad72 to
2bbb0f7
Compare
|
Thanks for the thorough review — both points addressed in the latest push ( 1. Bounded snapshot + late-refresh for configured MCP servers
Called from 2. Behavioral test replacing AST inspectionRemoved
All 14 tests pass (3 new + 11 existing ACP/mcp_startup). |
ACP entry.py fires MCP discovery in a background daemon thread, but _make_agent snapshots tools once at build and never re-reads the registry. Unlike CLI/TUI, ACP had no bounded wait before the snapshot and no late-refresh for configured (config.yaml) MCP servers — a reachable-but- slow server that finished after agent build was invisible for the whole session. Changes: - acp_adapter/session.py (_make_agent): call wait_for_mcp_discovery() before AIAgent construction, bounded by mcp_discovery_timeout (default ~1.5s). A dead server can't block; servers that miss the bound are picked up by the late-refresh below. - acp_adapter/server.py (_schedule_mcp_late_refresh): new method on HermesACPAgent — if discovery is still in flight after session creation, spawns an off-critical-path daemon that joins it (bounded 30s), then rebuilds the tool snapshot via the shared refresh_agent_mcp_tools helper. Cache-safe: only runs pre-first-turn (_user_turn_count/_api_call_count both 0); once the user has sent a message the snapshot is frozen, exactly as TUI PR NousResearch#48403 does. - Called from new_session, load_session, resume_session. - Mirrors the TUI pattern (tui_gateway _schedule_mcp_late_refresh, PR NousResearch#48403) and the CLI pattern (get_tool_definitions → wait_for_mcp_discovery). Tests: - Replace the AST-based test (source-text inspection) with three behavioral regression tests in tests/acp_adapter/test_acp_mcp_discovery.py: 1. Blocked discovery does not block startup (non-blocking contract) 2. Delayed discovery lands tools via late-refresh (pre-first-turn) 3. Late-refresh is cache-safe: skips rebuild after first turn Addresses teknium1 review on PR NousResearch#32811.
…t agent-build wait Review follow-ups on the NousResearch#32811 salvage: - Hold state.runtime_lock and bail on is_running so the pre-first-turn guard can't race the first prompt dispatch (a refresh publishing mid-turn would swap tools= and break the just-created cache prefix). Regression test mutation-checked (guard removed -> test fails). - In-memory-only session lookup in the daemon: get_session() falls through to a DB restore that builds a whole new AIAgent just to decide no-op (TUI equivalent also checks its in-memory dict only). - Pass quiet_mode=True explicitly, matching the TUI/gateway callers. - Use ensure_mcp_discovery_before_agent_build() (landed on main after the PR) instead of bare wait_for_mcp_discovery() so the ACP agent build is self-sufficient and gets the retry-after-zero-connected allowance, matching CLI/one-shot construction sites.
ACP entry.py fires MCP discovery in a background daemon thread, but _make_agent snapshots tools once at build and never re-reads the registry. Unlike CLI/TUI, ACP had no bounded wait before the snapshot and no late-refresh for configured (config.yaml) MCP servers — a reachable-but- slow server that finished after agent build was invisible for the whole session. Changes: - acp_adapter/session.py (_make_agent): call wait_for_mcp_discovery() before AIAgent construction, bounded by mcp_discovery_timeout (default ~1.5s). A dead server can't block; servers that miss the bound are picked up by the late-refresh below. - acp_adapter/server.py (_schedule_mcp_late_refresh): new method on HermesACPAgent — if discovery is still in flight after session creation, spawns an off-critical-path daemon that joins it (bounded 30s), then rebuilds the tool snapshot via the shared refresh_agent_mcp_tools helper. Cache-safe: only runs pre-first-turn (_user_turn_count/_api_call_count both 0); once the user has sent a message the snapshot is frozen, exactly as TUI PR #48403 does. - Called from new_session, load_session, resume_session. - Mirrors the TUI pattern (tui_gateway _schedule_mcp_late_refresh, PR #48403) and the CLI pattern (get_tool_definitions → wait_for_mcp_discovery). Tests: - Replace the AST-based test (source-text inspection) with three behavioral regression tests in tests/acp_adapter/test_acp_mcp_discovery.py: 1. Blocked discovery does not block startup (non-blocking contract) 2. Delayed discovery lands tools via late-refresh (pre-first-turn) 3. Late-refresh is cache-safe: skips rebuild after first turn Addresses teknium1 review on PR #32811.
…t agent-build wait Review follow-ups on the #32811 salvage: - Hold state.runtime_lock and bail on is_running so the pre-first-turn guard can't race the first prompt dispatch (a refresh publishing mid-turn would swap tools= and break the just-created cache prefix). Regression test mutation-checked (guard removed -> test fails). - In-memory-only session lookup in the daemon: get_session() falls through to a DB restore that builds a whole new AIAgent just to decide no-op (TUI equivalent also checks its in-memory dict only). - Pass quiet_mode=True explicitly, matching the TUI/gateway callers. - Use ensure_mcp_discovery_before_agent_build() (landed on main after the PR) instead of bare wait_for_mcp_discovery() so the ACP agent build is self-sufficient and gets the retry-after-zero-connected allowance, matching CLI/one-shot construction sites.
|
Merged via #75985 — your commits were cherry-picked onto current main with authorship preserved (rebase merge), so both land under your name on main:
Small follow-ups we added on top during review: the late-refresh guard is now serialized with turn start under the session runtime lock (with a mutation-checked regression test), the daemon uses an in-memory-only session lookup, and the docstring was updated to reflect the between-turns prologue refresh on current main. Thanks for the contribution — and for turning the sweeper feedback around with the bounded wait + behavioral tests. |
ACP entry.py fires MCP discovery in a background daemon thread, but _make_agent snapshots tools once at build and never re-reads the registry. Unlike CLI/TUI, ACP had no bounded wait before the snapshot and no late-refresh for configured (config.yaml) MCP servers — a reachable-but- slow server that finished after agent build was invisible for the whole session. Changes: - acp_adapter/session.py (_make_agent): call wait_for_mcp_discovery() before AIAgent construction, bounded by mcp_discovery_timeout (default ~1.5s). A dead server can't block; servers that miss the bound are picked up by the late-refresh below. - acp_adapter/server.py (_schedule_mcp_late_refresh): new method on HermesACPAgent — if discovery is still in flight after session creation, spawns an off-critical-path daemon that joins it (bounded 30s), then rebuilds the tool snapshot via the shared refresh_agent_mcp_tools helper. Cache-safe: only runs pre-first-turn (_user_turn_count/_api_call_count both 0); once the user has sent a message the snapshot is frozen, exactly as TUI PR NousResearch#48403 does. - Called from new_session, load_session, resume_session. - Mirrors the TUI pattern (tui_gateway _schedule_mcp_late_refresh, PR NousResearch#48403) and the CLI pattern (get_tool_definitions → wait_for_mcp_discovery). Tests: - Replace the AST-based test (source-text inspection) with three behavioral regression tests in tests/acp_adapter/test_acp_mcp_discovery.py: 1. Blocked discovery does not block startup (non-blocking contract) 2. Delayed discovery lands tools via late-refresh (pre-first-turn) 3. Late-refresh is cache-safe: skips rebuild after first turn Addresses teknium1 review on PR NousResearch#32811.
…t agent-build wait Review follow-ups on the NousResearch#32811 salvage: - Hold state.runtime_lock and bail on is_running so the pre-first-turn guard can't race the first prompt dispatch (a refresh publishing mid-turn would swap tools= and break the just-created cache prefix). Regression test mutation-checked (guard removed -> test fails). - In-memory-only session lookup in the daemon: get_session() falls through to a DB restore that builds a whole new AIAgent just to decide no-op (TUI equivalent also checks its in-memory dict only). - Pass quiet_mode=True explicitly, matching the TUI/gateway callers. - Use ensure_mcp_discovery_before_agent_build() (landed on main after the PR) instead of bare wait_for_mcp_discovery() so the ACP agent build is self-sufficient and gets the retry-after-zero-connected allowance, matching CLI/one-shot construction sites.
Fire-and-forget MCP server connections on a daemon thread so the gateway / CLI / ACP process becomes interactive immediately.
Previously
hermes --tuiwaited 2-5 s after the splash screen before rendering the UI whilediscover_mcp_tools()ran synchronously on the critical path.Changes:
tools/mcp_tool.py: newdiscover_mcp_tools_background()— daemon thread wrappertui_gateway/entry.py,hermes_cli/main.py,cli.py,acp_adapter/entry.py: use background variantRelated: #29726, #29184, #19326 (closed stale)
Closes: #29726