Skip to content

feat(mcp): keep unchanged MCP servers alive across a classic /reload - #377

Merged
code-yeongyu merged 4 commits into
mainfrom
perf/mcp-reload-measure
Jul 26, 2026
Merged

feat(mcp): keep unchanged MCP servers alive across a classic /reload#377
code-yeongyu merged 4 commits into
mainfrom
perf/mcp-reload-measure

Conversation

@code-yeongyu

@code-yeongyu code-yeongyu commented Jul 26, 2026

Copy link
Copy Markdown
Owner

Why

/reload disposes every MCP connection and the next session respawns every server, even when nothing about their configuration changed. Measured on real stdio children:

p50 p95
Dispose + respawn (current /reload) 411 ms 419 ms
Preserve + reconcile 0.76 ms 0.82 ms

The cost is a roughly constant ~400 ms floor per reload (independent of server count — 2 vs 5 servers barely moves it), against a total reload of ~477 ms. It is the single largest remaining reload cost, and it is entirely self-inflicted: the service already has config-hash reconciliation (#syncFromConfig, keyed name\0configHash) that disposes only changed or removed servers. That path ships today for shutdown reason "new"/reload just never used it.

What changed

Classic reload preserves the MCP singleton. The next attach re-enters reconciliation: unchanged servers keep their process, changed ones reconnect, removed ones are disposed. quit still disposes.

Provider-scoped services still dispose on reload, deliberately. mcpExtension builds sessionOwned ? new McpService() : getMcpService(), so under provider scopes (multi-session RPC) every factory execution creates a fresh service, and reload re-executes factories. Skipping disposal there would orphan the previous instance's child processes.

Core now tells an extension it was removed by a reload. After the runtime is rebuilt, AgentSession compares old vs new extension identities and emits session_extensions_removed to any extension present in the old runner but absent in the new one. This is the part that makes preservation safe: if the MCP builtin is disabled during that same reload, shutdown alone would skip disposal and nothing would ever re-attach, leaking the preserved connections. MCP subscribes and disposes when its own <builtin:mcp> entry is removed. The mechanism is extension-agnostic — core knows nothing about MCP.

This closes both leak vectors the earlier attempts hit. Removing "reload" from shouldDisposeMcpService outright leaked provider-scoped services; making that scope-aware alone then leaked when MCP was disabled mid-reload. Each is now covered by a dedicated test.

/mcp reconnect <name> remains the wedged-server escape hatch, covered by a kill-the-child-then-reconnect test.

Evidence

58 tests pass in one run across the MCP and reload suites; 127 pass across the 16 affected suites. npm run check exits 0. The cost table above is from a dedicated A/B probe using the existing fixtures (local-ignore/mcp-reload-cost.mts, reason "reload" vs reason "new", real stdio children, 6 cycles).

The leak-guard tests that matter:

  • Classic reload keeps the same service object, same child pid, spawn counter 1, still connected.
  • Disable MCP during a classic reload: the preserved singleton is disposed and its child pid is dead (this is the second-round leak, now guarded).
  • Provider-scoped reload: real ProviderScope, real stdio child, asserts the child pid is dead (the first-round leak, guarded).
  • quit disposes in both modes; three consecutive classic reloads hold connection and spawn counts steady.
  • Core: an extension removed from the rebuilt runner receives session_extensions_removed.

Deliberately flipped assertions

test/mcp/extension-load.test.ts pinned the old always-dispose contract and was updated on purpose: reload moved from the disposal set to the retained-session set; the reload-followup test now asserts the same singleton survives with two session starts instead of expecting a fresh service.


Summary by cubic

Classic /reload now preserves unchanged MCP servers by reusing the shared McpService, cutting reloads by ~400ms. Core reports session_extensions_removed on the old runner for all session replacements and even if a reload rebuild fails, so disabling the MCP builtin cleanly disposes preserved servers.

  • New Features

    • Keep the classic McpService alive across /reload; unchanged servers keep their process, changed reconnect, removed dispose.
    • Provider-scoped MCP services still dispose on reload.
    • /mcp reconnect <name> remains the manual recovery path for wedged servers.
  • Bug Fixes

    • Emit session_extensions_removed on the old runner for all replacements (/new, /resume, /fork, imports, cross-cwd) and in a finallyfor/reload; MCP listens and disposes when "builtin:mcp"` is removed.
    • Skip removal reporting when runners don’t expose getExtensionIdentities, avoiding crashes in partial test hosts.

Written for commit 9962222. Summary will update on new commits.

Review in cubic

@code-yeongyu
code-yeongyu force-pushed the perf/mcp-reload-measure branch from c85d0a4 to b22189c Compare July 26, 2026 10:14
Classic reload no longer disposes the MCP singleton, so the next attach re-enters
config-hash reconciliation: unchanged servers keep their process, changed ones
reconnect, removed ones are disposed. Provider-scoped services still dispose on
reload, since every factory execution owns a fresh McpService.

To make that safe when the MCP builtin is disabled during the same reload, core
now emits session_extensions_removed to extensions present in the old runner but
absent in the new one, right after the runtime is rebuilt. MCP subscribes and
disposes when its own <builtin:mcp> entry is removed, so a disabled MCP can never
leak its connections, child processes, or reconnect timers.

Measured motivation: dispose+respawn is ~400ms per reload (constant, independent
of server count) versus ~1ms for preserve+reconcile, on a reload that costs
~477ms total. /mcp reconnect <name> stays the wedged-server escape hatch, covered
by a kill-the-child-then-reconnect test.
Reload only reported session_extensions_removed after _buildRuntime succeeded,
so a rebuild that threw (for example _refreshToolRegistry rejecting an
extension's tool metadata) left a removed extension orphaned. The emit now runs
in a finally block.

/new, /resume, /fork, import, and cross-cwd switches never reported removal at
all, so a classic MCP singleton preserved across those shutdown reasons could
leak when the rebuilt runtime omitted it. teardownCurrent now captures the old
runner and its extension identities, and apply reports the diff through the same
event, so every replacement path notifies removed extensions.
…runners

Test hosts and partial ExtensionRunner implementations do not expose
getExtensionIdentities; teardownCurrent now only captures and
reportRemovedExtensions only emits when both runners expose it, so those hosts
degrade to the previous behavior instead of throwing during session replacement.
Caught by CI's rpc-session-registry suite, which uses a minimal runner stub.
@code-yeongyu
code-yeongyu force-pushed the perf/mcp-reload-measure branch from b22189c to 9962222 Compare July 26, 2026 10:50
@code-yeongyu
code-yeongyu merged commit 8fd7a02 into main Jul 26, 2026
6 checks passed
@code-yeongyu
code-yeongyu deleted the perf/mcp-reload-measure branch July 26, 2026 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant