Stale MCP toolset in shared _toolset_cache causes silent session failure on resume

[Leoyzen]

Bug Description

When an ACP session is resumed after WebSocket reconnection, the agent's run silently fails. The handle_prompt() returns end_turn immediately (by design for turn_complete capability), but the background run fails 5 minutes later with an MCP timeout — no session/update events are ever produced.

Root Cause

MCPManager._toolset_cache on the shared pool-level MCPManager is never invalidated between sessions.

When an agent shares the pool's MCPManager (_mcp_shared = True, which is the default when the agent has no agent-level MCP servers), the _toolset_cache: dict[str, MCPToolset] persists across session boundaries. On session resume:

1. A new SessionConnectionPool is created with fresh transports (correct)
2. agent._mcp_snapshot is updated with new McpConfigEntry (correct)
3. get_agentlet() calls self.mcp.as_capability(snapshot=..., session_pool=...)
4. as_capability() → _make_capability() checks _toolset_cache[client_id] first
5. Cache hit → returns OLD MCPToolset (which holds the OLD transport → OLD AcpMcpConnection → dead WebSocket)
6. Agentlet's MCP toolset __aenter__() tries to initialize via the dead connection → 300s timeout → McpError

Why the cache key collides

AcpMCPServerConfig.client_id = f"acp_{self.acp_id}" — deterministic across sessions. The same ACP server always maps to the same _toolset_cache key, so session resume always hits the stale cache entry.

Evidence (from production logs)

09:30:27 — Session resume: new MCP connections mcp_conn_4459a3ed, mcp_conn_1523435e created
09:30:28 — /fta-eval skill command executed (has_instructions=True)
09:30:28 — handle_prompt() returns {"stopReason":"end_turn"} immediately
09:35:28 — MCP timeout (300s): Error sending mcp/message to client connection_id=mcp_conn_57fde2f2 (ORIGINAL WebSocket, from 08:39:42!)
09:35:29 — NativeTurn execution failed: McpError: Internal error

The error uses mcp_conn_57fde2f2 — the OLDEST connection, not the fresh ones created at resume time.

Reproduction Test

tests/mcp_server/test_stale_mcp_connection.py — 5 tests, all passing:

1. test_session_resume_returns_stale_toolset_from_cache — Main reproduction: creates shared MCPManager, simulates session 1 (cache populated) → session 2 resume (fresh transport ignored, stale toolset returned)
2. test_acp_client_id_is_deterministic_across_sessions — Documents why cache key collides
3. test_session_pool_provides_fresh_transport_on_resume — Proves SessionConnectionPool is NOT the source of the bug
4. test_multiple_acp_servers_all_go_stale_on_resume — All ACP servers go stale, not just one
5. test_disconnect_all_clears_cache_but_not_called_on_resume — disconnect_all() would fix it, but session resume never calls it

Key Code Path

File	Line	Role
messagenode.py	130-132	self.mcp = agent_pool.mcp when shared
manager.py	147	_toolset_cache: dict[str, Any] = {}
manager.py	374-394	_make_capability() — checks cache first, creates if missing
manager.py	414-425	Session-scoped configs still go through _make_capability() with caching
session_pool.py	198-219	add_transport() — correctly overwrites old transport in per-session pool
session_manager.py	264-286	get_or_create_session_agent() — creates fresh SessionConnectionPool
session_controller.py	932-947	close_session() — does NOT clear shared _toolset_cache
mcp_server.py	414-416	AcpMCPServerConfig.client_id = f"acp_{self.acp_id}" (deterministic)

Proposed Fix

Don't cache session-scoped toolsets in the shared _toolset_cache.

In MCPManager._make_capability() (manager.py:374-394), add a cache parameter:

def _make_capability(server: BaseMCPServerConfig, transport: Any, *, cache: bool = True) -> MCP:
    client_id = server.client_id
    toolset = self._toolset_cache.get(client_id) if cache else None
    if toolset is None:
        toolset = MCPToolset(client=transport, **_make_kwargs(server))
        if cache:
            self._toolset_cache[client_id] = toolset
    return MCP(url=_derive_url(server), local=toolset, ...)

Then in _process_snapshot(), pass cache=False for session-scoped configs:

# Session-scoped configs (session + skill) — don't cache, transports are per-session
for entry in snap.session_scoped_configs:
    ...
    capabilities.append(_make_capability(server, transport, cache=False))

Why this approach:

• Minimal change — only affects session-scoped path, global configs still benefit from caching
• Correct semantics — session-scoped transports are per-session by definition; caching them in a shared manager is a category error
• No lifecycle hooks needed — doesn't require changes to session close/resume paths
• disconnect_all() still works for global configs

Secondary Issues (address separately)

1. ACPSession.close() doesn't clean MCP state (session.py:795+) — Cleans env, signals, sys_prompts but does NOT touch _mcp_manager._connections, agent._session_connection_pool, or agent._mcp_snapshot
2. session_manager.resume_session() early-return (session_manager.py:244-249) — If session is in _acp_sessions, returns immediately, ignoring new mcp_servers parameter
3. No WebSocket disconnect cleanup — No hook fires when ACP WebSocket drops
4. MCP snapshot merge is append-only (session.py:525-535) — Deduplicates by client_id but never prunes old entries

Acceptance Criteria

• test_session_resume_returns_stale_toolset_from_cache passes with fix (toolset2.client is transport_b, not transport_a)
• test_multiple_acp_servers_all_go_stale_on_resume passes with fix (all toolsets use session-2 transports)
• Global (pool-level + agent-level) MCP toolsets still benefit from caching
• No regressions in test_mcpmanager_caching.py tests
• Session resume produces session/update events normally (manual ACP test)

Related Issues

• ACP tool_call status shows "completed" when question_for_user tool fails #87 — ACP tool_call status shows "completed" when tool fails (similar silent failure pattern)
• Bridge MCP Elicitation to Deferred/Durable Execution Pipeline #107 — Bridge MCP Elicitation to Deferred Execution (MCP + session lifecycle)
• [Tracking] ACP v1→v2 协议迁移 #109 — ACP v1→v2 migration (v2 unifies session/load + session/resume, may affect this flow)

[Leoyzen]

Git History Analysis — Root cause is the re-introduction of _toolset_cache

The sharing of the pool's MCPManager (self.mcp = agent_pool.mcp) was intentional and correct — it enables subagent MCP inheritance. The bug was introduced by re-adding _toolset_cache after it was deliberately removed.

Timeline

Commit	Date	Change	Intent
e4fa0fcd5	earlier	Add _toolset_cache with MCPToolset caching	Initial caching for connection reuse
88bdc1758	Jun 30	Remove _toolset_cache to fix cross-task CancelScope error	MCPToolset instances were cached per client_id and shared across asyncio tasks (parent agent in task A, subagent in task B). anyio's CancelScope is task-affine → RuntimeError when __exit__ crossed task boundaries.
6bf3e7235 (#88)	Jul 2	Three-tier connection scoping architecture	Deliberately designed "Fresh MCPToolset per agentlet from snapshot + connection pools" and "as_capability() creates fresh MCPToolset per call (no caching)". Solved subagent inheritance via copy_pre_created_transports() + McpConfigSnapshot instead of caching.
d3b4966c8	Jul 5	Re-add _toolset_cache for MCPToolset connection reuse	Reintroduced caching by @Million-mo to "avoid duplicate MCP connections for the same server config". This is the commit that introduced the stale connection bug.
0089d6c71 (#93)	Jul 6	Thin-wrapper refactor	Carried forward the re-added _toolset_cache as-is.

Why the sharing itself is NOT the problem

The self.mcp = agent_pool.mcp sharing (messagenode.py:130-132) is correct and necessary:

1. Subagent MCP inheritance — When a subagent is spawned, get_or_create_session_agent() (session_controller.py:505-518) creates a fresh SessionConnectionPool and calls copy_pre_created_transports() to copy ACP transports from the parent's pool. The child also inherits the parent's session_configs via McpConfigSnapshot.

2. Per-session transports — Each session has its own SessionConnectionPool with its own transports. The SessionConnectionPool correctly overwrites old transports on add_transport().

3. Fresh toolset per agentlet — get_agentlet() calls as_capability(snapshot=..., session_pool=...) per-turn. In fix(mcp): MCP provider lifecycle architecture — three-tier connection scoping #88's design, this was supposed to create a fresh MCPToolset each time, reading the transport from the per-session pool. No caching needed — the transport is already shared at the pool level.

Why the re-added _toolset_cache breaks things

_toolset_cache is on the shared MCPManager instance. When as_capability() is called with a session-scoped config, _make_capability() checks _toolset_cache[client_id] first — before looking at the session pool's transport. If a previous session cached a toolset with the same client_id, the stale toolset (holding a dead transport) is returned instead of creating a fresh one from the new session pool.

This is a regression from #88's design. #88 solved subagent inheritance via the snapshot + copy_pre_created_transports() mechanism, making the cache unnecessary for that purpose. The cache was re-added for "connection reuse" but it re-introduced the exact cross-task issue that 88bdc1758 removed it for, plus the stale connection bug.

Updated fix assessment

The proposed fix (don't cache session-scoped toolsets) is still correct and aligns with #88's original design. But we should also consider:

Option A (proposed, minimal): Skip caching for session-scoped configs in _make_capability(cache=False). Global configs still cached.

Option B (align with #88 design): Remove _toolset_cache entirely. #88 proved that fresh MCPToolset per call works correctly — the transport sharing happens at the SessionConnectionPool level, not the toolset level. The cross-task CancelScope issue was solved by the owner-task pattern in GlobalConnectionPool and per-session stream pairs in AcpMcpConnection.

Recommendation: Option B (full removal) is cleaner and restores #88's intent. The "duplicate MCP connections" concern that motivated d3b4966c8 is mitigated by:

• ACP transports are shared via copy_pre_created_transports() (no duplicate connection)
• Stdio transports use the owner-task pattern in GlobalConnectionPool (shared session)
• HTTP/SSE transports are cheap to create per-call

Option A is the safe fallback if Option B reveals performance issues.

[Leoyzen]

Complete MCP-over-ACP Lifecycle Audit

The explore agent mapped every allocation and cleanup site. The lifecycle is genuinely chaotic — 6 separate resources leak across 4 different owners with no coordination.

Resource Leak Matrix

Resource	Owner	Allocated At	Cleaned Up At	Leaks?
AcpMcpConnection	AgentPoolACPAgent._mcp_manager (global)	connect_acp_mcp_server()	Only close_all() (global shutdown)	YES
AcpMcpTransport	Agent._session_connection_pool (per-session)	initialize_mcp_servers()	SessionConnectionPool.cleanup() — never called	YES
SessionStreamPair	AcpMcpConnection._session_streams (per-connection)	connect_session() on transport	unregister_session() — never called on session close	YES
McpConfigSnapshot session configs	Agent._mcp_snapshot (per-agent)	initialize_mcp_servers()	never reverted	YES
SessionConnectionPool itself	Agent._session_connection_pool (per-agent)	get_or_create_session_agent()	cleanup() exists but never called from close path	YES
ACPSession in _acp_sessions	ACPSessionManager (global)	create_session() / resume_session()	Only on explicit close_session RPC or close_all_sessions()	YES (on WebSocket drop)

The 3 Critical Gaps

Gap 1: ACPSession.close() omits MCP entirely (session.py:795)

Cleans: acp_env, state_updated signal, cwd_context sys_prompt, skill command callback.
Does NOT clean: _session_connection_pool, _mcp_snapshot, ACP MCP connections, session stream pairs.

Gap 2: SessionConnectionPool.cleanup() is never called (session_pool.py:249)

The method exists, with a comment explaining it deliberately skips ACP pre-created transports (because they're shared with child sessions). But it's never invoked from ANY close path — not ACPSession.close(), not SessionController.close_session(), not agent.__aexit__().

Gap 3: WebSocket disconnect doesn't trigger session cleanup (transports.py:355)

When WebSocket drops, _handle_websocket_client() closes the AgentSideConnection but doesn't call ACPSessionManager.close_session(). Sessions remain in _acp_sessions with zombie agents. Next resume_session() hits the early-return at line 244 and returns the broken session.

Does Option B (remove _toolset_cache) solve the problem?

No — Option B is necessary but not sufficient.

Option B removes one leak (_toolset_cache), but the other 5 resources still leak. The root cause is architectural: session-scoped MCP state is scattered across 4 different objects with no coordinated lifecycle:

AgentPoolACPAgent (global)     → _mcp_manager._connections (flat, no session tracking)
Agent (per-session)            → _session_connection_pool (never cleaned)
                               → _mcp_snapshot (never reverted)
AcpMcpConnection (per-conn)    → _session_streams (never unregistered)
ACPSessionManager (global)     → _acp_sessions (not cleaned on WebSocket drop)

Proposed Architecture: Session-Owned MCP Context

Instead of weakref (which is non-deterministic for resources with async cleanup), make the session explicitly own ALL its MCP resources:

class SessionMcpContext:
    """Per-session MCP resource container. Session lifecycle drives MCP lifecycle."""
    
    def __init__(self, session_id: str, pool_mcp: MCPManager):
        self.session_id = session_id
        self.pool_mcp = pool_mcp           # shared, for global configs only
        self.connection_pool = SessionConnectionPool(session_id)
        self.snapshot: McpConfigSnapshot | None = None
        self._acp_connections: list[str] = []  # connection_ids owned by this session
    
    async def cleanup(self):
        """Called from ACPSession.close() — cleans ALL session-scoped MCP resources."""
        # 1. Clean session connection pool (transports)
        await self.connection_pool.cleanup()
        # 2. Unregister session stream pairs from AcpMcpConnections
        for conn_id in self._acp_connections:
            conn = pool_mcp_acp_manager.get_connection(conn_id)
            if conn:
                conn.unregister_session(self.session_id)
        # 3. Revert snapshot (remove session_configs)
        self.snapshot = None
        # 4. Remove ACP connections that are exclusively owned by this session
        #    (not shared with child sessions)

Then ACPSession.close() calls self.mcp_context.cleanup() before cleaning env/signals.

Phased Fix Plan

Phase	Scope	Fixes
P0 (immediate)	Remove _toolset_cache	Fixes the specific stale toolset bug. Restores #88's design.
P1 (short-term)	Wire SessionConnectionPool.cleanup() into close path	Add await agent._session_connection_pool.cleanup() to SessionController._close_session_run_turn() before agent.__aexit__()
P1 (short-term)	Fix ACPSession.close() MCP cleanup	Add cleanup for _mcp_snapshot, ACP MCP connections, session stream pairs
P2 (short-term)	WebSocket disconnect hook	When AgentSideConnection closes, trigger ACPSessionManager.close_session() for all sessions on that connection
P2 (short-term)	Fix resume_session() early-return	Don't return stale session; either clean+reinit or remove+recreate
P3 (long-term)	SessionMcpContext refactor	Consolidate all session-scoped MCP state into one explicitly-scoped container

P0 unblocks the immediate bug. P1+P2 fix the lifecycle chaos. P3 makes it architecturally clean.

[Leoyzen]

Architecture Design: 2-Tier MCP Lifecycle (Global + Session)

Design Decision

After evaluating 3 approaches (3-tier with SessionMcpContext, global+session tracking, and 2-tier with allow/block), the agreed design is a 2-phase migration:

• Phase 1 (non-breaking): Fix lifecycle by adding session tracking to the shared MCPManager. No config changes.
• Phase 2 (breaking): Remove per-agent MCP tier entirely. All MCP servers declared at pool level, agents filter via allow/block list.

Final state: 2-tier model (global + session), no per-agent MCPManager.

---

Final Architecture (after both phases)

MCPManager (global, session-aware)
├── Global resources (pool-level, shared, long-lived)
│   ├── _global_pool: GlobalConnectionPool (stdio/HTTP/SSE transports)
│   ├── _global_cache: dict[str, MCPToolset] (pool configs only)
│   └── providers: list[MCPResourceProvider]
│
└── Session resources (per-session, short-lived)
    └── _session_contexts: dict[str, _SessionContext]
        ├── connection_pool: SessionConnectionPool
        ├── toolset_cache: dict[str, MCPToolset] (per-session, cleared on close)
        ├── snapshot: McpConfigSnapshot
        └── acp_connection_ids: list[str]

Agent
  └── mcp = pool.mcp  (always shared, no per-agent MCPManager)

Config:
  mcp_servers:        # all servers at pool level
    - name: search
      command: uvx
      args: [mcp-server-search]
  agents:
    coder:
      mcp:
        allow: [search, filesystem]   # whitelist
    reviewer:
      mcp:
        block: [code_tools]           # blacklist

Core principle: Resource scope = config scope. Toolset scope = transport scope. Global resources are shared; session resources are isolated and cleaned on session close.

---

Phase 1: Fix Lifecycle (non-breaking, no config changes)

Goal: Make session cleanup work across all paths. Keep existing 3-tier config model.

Task	ID	Files	Fix
Add session tracking to MCPManager	P1a	manager.py	_session_contexts: dict[str, _SessionContext], as_capability(session_id), cleanup_session(session_id)
Remove session-scoped caching from _toolset_cache	P1b	manager.py:374-394	_make_capability(cache=False) for session-scoped configs; per-session cache in _SessionContext.toolset_cache
Fix AcpMcpConnectionManager session tracking	P1c	acp_mcp_manager.py	Add _session_connections: dict[str, set[str]], cleanup_session(session_id); fix AcpMcpConnection._session_streams to track session_id
Wire cleanup_session() into close paths	P1d	session_controller.py, session.py	ACPSession.close() → pool.mcp.cleanup_session(session_id); SessionController.close_session() → same
Fix resume_session() early-return	P1e	session_manager.py:244	Close stale session first, then recreate with fresh MCP resources
WebSocket disconnect hook	P1f	transports.py, session_manager.py	On disconnect → close all sessions for that connection → cleanup_session()
Call SessionConnectionPool.cleanup()	P1g	(via P1a)	Wired into cleanup_session() automatically

Dependency graph:

P1a (MCPManager session tracking)  ←── foundation
P1c (AcpMcpConnectionManager)      ←── parallel with P1a
P1b (remove session cache)         ←── depends on P1a
P1d (wire cleanup)                 ←── depends on P1a + P1c
P1e (fix resume)                   ←── depends on P1d
P1f (WebSocket hook)               ←── depends on P1d + P1c

No config changes — mcp_servers on agents still works, _mcp_shared flag retained. Just cleanup now works.

Effort: ~1-2 days

---

Phase 2: Remove Per-Agent MCP Tier (breaking, config API change)

Goal: Simplify to 2-tier model. All MCP servers at pool level, agents filter via allow/block.

Task	ID	Files	Change
Add mcp.allow / mcp.block agent config	P2a	agentpool_config/agents.py	New optional fields on agent config
Migration shim for legacy mcp_servers	P2b	manifest.py	Promote agent-level servers to pool level + auto-generate allow list
Remove _mcp_shared condition	P2c	messagenode.py:130-135	Always self.mcp = agent_pool.mcp
Remove per-agent MCPManager creation	P2d	messagenode.py, manager.py	Delete else branch, remove mcp_servers param from MCPManager constructor
Allow/block filtering in as_capability()	P2e	manager.py	Filter pool_configs by agent's allow/block when building snapshot
Consolidate skill MCP dual paths	P2f	skill_mcp_manager.py	Remove legacy _providers cache path; skills register via snapshot only
Remove MCPManager mcp_servers param	P2g	messagenode.py, manager.py	Cleanup

Phase 1 work is directly reused — _session_contexts and cleanup_session() become the only non-global tier. Phase 2 just removes the agent-level tier.

Effort: ~1 day

---

Why Not weakref?

MCP resources have async cleanup (__aexit__, stream close, process kill). GC timing is non-deterministic. Explicit lifecycle via cleanup_session() is deterministic, debuggable, and correct for all resource types.

Why Not per-session MCPManager?

• Breaks MCPResourceProvider model (N copies of each pool-level provider)
• Requires re-inventing transport sharing below the manager
• Violates "resource scope = config scope" principle
• 2-tier model is the right factoring

Reproduction Tests

tests/mcp_server/test_stale_mcp_connection.py — 5 tests, all passing, documenting the bug. These tests will flip from documenting the bug to verifying the fix once Phase 1 lands.

References

• Original bug analysis: issue body above
• Git history: _toolset_cache removed in 88bdc1758 (cross-task CancelScope fix), re-added in d3b4966c8 (regression)
• fix(mcp): MCP provider lifecycle architecture — three-tier connection scoping #88 design intent: "Fresh MCPToolset per agentlet from snapshot + connection pools" + "as_capability() creates fresh MCPToolset per call (no caching)"
• Oracle validation: 3-tier model sound, per-session toolset cache recommended, migration ordering P1→P0 (not P0-first)