fix(mcp): MCP provider lifecycle architecture — three-tier connection scoping

[Leoyzen]

Summary

Complete overhaul of MCP provider lifecycle: migration from deprecated MCPServer* to MCPToolset, three-tier connection scoping architecture, ACP transport concurrent session support, and streaming adapter CancelScope fix.

Closes #70

Problem

Subagent sessions could not access MCP-over-ACP tools (e.g. workspace-fs). Four root causes:

1. Race condition: receive_request() starts the turn immediately, but session/load (which creates ACP MCP transports) arrives ~34ms later. get_agentlet() runs before transports are available.
2. Cross-task CancelScope error: Cached MCPToolset instances shared between parent (task A) and child (task B) triggered RuntimeError: Attempted to exit cancel scope in a different task.
3. Stream contention: AcpMcpTransport.connect_session() used shared memory streams — multiple ClientSession instances competed for the same streams.
4. get_tools() hang: MCPResourceProvider.get_tools() blocked waiting for ACP session, hanging the entire turn.

Changes

1. Migration: MCPServer* → MCPToolset (prior work, squashed)

• to_transport() on all config classes (StdioMCPServerConfig, SSEMCPServerConfig, StreamableHTTPMCPServerConfig)
• Removed all to_pydantic_ai() methods and deprecated MCPServer* imports
• as_capability() creates fresh MCPToolset per call (no caching)
• _make_elicitation_handler() with 4-arg FastMCP signature
• MCPClient._get_client() uses config.to_transport()

2. Three-Tier MCP Connection Scoping Architecture

Tier 1: McpConfigSnapshot (immutable config, inheritable)

• Frozen dataclass capturing pool/agent/session/skill MCP configs
• Child sessions inherit parent's session_configs at creation time
• Eliminates race condition — config is available before run_stream()

Tier 2a: GlobalConnectionPool (pool-level singleton)

• Owner-task pattern for stdio (dedicated asyncio.Task enters/exits CM)
• _SharedSessionTransport wrapper: yields shared ClientSession without duplicate connect_session() calls
• HTTP/SSE: fresh transport per call (no stream contention)
• LRU eviction (MAX_SESSIONS=256), ref counting, threading.Lock

Tier 2b: SessionConnectionPool (per-session isolation)

• (client_id, skill_name) keying for skill MCP isolation
• Owner-task for stdio, add_transport() for pre-created ACP transports
• copy_pre_created_transports(): child inherits parent's ACP transport

Tier 3: Lazy toolset materialization in get_agentlet()

• Fresh MCPToolset per agentlet from snapshot + connection pools
• Bypasses agent.tools.providers for MCP (uses snapshot path)
• native=False to force local MCP tool fallback

3. AcpMcpTransport Concurrent connect_session() Support

• Per-session stream pairs in AcpMcpConnection (register_session() / unregister_session())
• send_to_acp() routes response to caller's stream (no JSON-RPC id tracking — synchronous send_to_client naturally correlates)
• broadcast_to_sessions() for server-initiated notifications
• connect_session() finally only cleans up own pair, not connection's

4. GlobalConnectionPool Transport Sharing

• stdio: get_transport() returns _SharedSessionTransport wrapper that yields the shared ClientSession managed by the owner task. Underlying transport's connect_session() called only once.
• HTTP/SSE: Fresh transport per get_transport() call. HTTP connections are cheap and stateless — no need to share.

5. Streaming Adapter CancelScope Fix

• Moved yield outside anyio.create_task_group()
• Producer as asyncio.ensure_future(), consumer in main coroutine
• Same pattern as base_agent.py and acp_agent.py fixes

6. SkillMcpManager Integration

• SkillMcpServerConfig.to_mcp_server_config() bridge method
• SkillCapability.get_toolset() reads from snapshot + SessionConnectionPool
• on_run_ended() uses isinstance instead of getattr

7. Other Fixes

• get_tools() 5s timeout to prevent subagent hang on ACP MCP provider
• ACPSessionManager.create_session() takes agent_name: str instead of agent: BaseAgent
• native=False for MCP capabilities (ack-dev doesn't support native MCPServerTool)
• Session data overwrite fix in _get_or_create_session_locked()
• session/load on active session → ignore; on stored session → restore with mcpServers
• Removed session_mcp_providers list from ACPSession (replaced by snapshot)
• Removed all add_provider(mcp_provider) calls for MCP from agent.tools.providers path

8. Code Cleanup (commit 2, -310 lines)

• Removed legacy AcpMcpConnection shared stream path (send_to_client(), open(), properties, handle_client_message() fallback) — ~235 lines
• Removed __main__ test block from manager.py
• Downgraded 16 debug logger.info() → logger.debug() in turn.py, run.py, core.py
• Removed duplicate AcpMCPServerConfig import, vestigial _ref_count/_lock from _SharedSessionTransport
• Removed unused _warn parameter from MCPManager.__init__()
• Inlined trivial _build_mcp_toolsets_legacy() wrapper

New Files

File	Purpose
src/agentpool/mcp_server/config_snapshot.py	McpConfigEntry + McpConfigSnapshot frozen dataclasses
src/agentpool/mcp_server/global_pool.py	GlobalConnectionPool with owner-task pattern + _SharedSessionTransport
src/agentpool/mcp_server/session_pool.py	SessionConnectionPool per-session isolation
src/agentpool/mcp_server/session_stream_pair.py	SessionStreamPair dataclass for per-session streams

Modified Files (summary)

File	Key Changes
agentpool_config/mcp_server.py	to_transport() methods, removed to_pydantic_ai() (-104 lines)
agentpool/mcp_server/manager.py	as_capability() with pools, send_to_acp(), broadcast_to_sessions()
agentpool/mcp_server/client.py	Uses config.to_transport()
agentpool_server/acp_server/acp_mcp_manager.py	Per-session streams, register/unregister/send_to_acp
agentpool_server/acp_server/acp_mcp_transport.py	Per-session connect_session()
agentpool_server/acp_server/session.py	initialize_mcp_servers() builds config entries, removed session_mcp_providers
agentpool_server/acp_server/session_manager.py	session/load semantics, create_session() takes agent_name: str
agentpool_server/acp_server/handler.py	Removed MCP provider registration
agentpool_server/acp_server/acp_agent.py	agent_name refactoring
agentpool/agents/native_agent/agent.py	_mcp_snapshot, _build_pool/agent_configs(), snapshot-aware get_agentlet()
agentpool/agents/native_agent/turn.py	get_tools() timeout, debug logging
agentpool/orchestrator/core.py	Snapshot building in child/main/non-native paths, copy_pre_created_transports()
agentpool/messaging/streaming_adapter.py	CancelScope fix
agentpool/skills/capability.py	Snapshot-aware toolset building
agentpool/skills/skill_mcp_manager.py	build_config_entries()
agentpool_config/skills.py	to_mcp_server_config() bridge
agentpool/resource_providers/mcp_provider.py	Removed debug logging
agentpool/resource_providers/base.py	Cleanup

Testing

• 134 MCP server tests (45 unit + 42 integration + 47 existing)
• 3 ACP transport reuse tests (concurrent connect_session, one-exit-doesn't-break-other, 4-way concurrent)
• 3 GlobalConnectionPool sharing tests (HTTP not shared, stdio single connect, shared stdio)
• 17 cross-task lifecycle integration tests
• ruff clean, mypy clean on changed files
• 0 new test failures (21 pre-existing failures in acp_server unrelated)

Files Changed

48 files, +7991/-1172 (commit 1) + 9 files, +28/-316 (commit 2) = 50 files, +8019/-1488

Ultraworked with Sisyphus

[gemini-code-assist]

Code Review

This pull request refactors the MCP provider lifecycle in AgentPool to a three-tier connection scoping architecture, introducing immutable config snapshots, global and session connection pools, and lazy toolset materialization. These changes resolve subagent tool inheritance race conditions and cross-task CancelScope errors. The reviewer identified several critical issues in the new implementation, including a severe race condition in GlobalConnectionPool.release() where dying connections could be prematurely reused, a resource leak for HTTP/SSE connections, a ref-counting bug for HTTP/SSE connection reuse, a runtime NameError in session.py due to a missing Agent import, and silent exception swallowing in base.py.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[gemini-code-assist]

The Agent class is used in isinstance(self.agent, Agent) checks at lines 486 and 519, but it is not imported at runtime in session.py. This will raise a NameError at runtime when initializing MCP servers.

Please import Agent from agentpool at the top of the file.

from agentpool import Agent
from agentpool.agents.acp_agent import ACPAgent

[Leoyzen]

❌ Not a bug. Agent is imported at line 28 of session.py:

from agentpool import Agent, AgentPool

This is a runtime import, not a TYPE_CHECKING import. isinstance(self.agent, Agent) works correctly at runtime.

[Leoyzen]

Confirmed: from agentpool import Agent, AgentPool at line 28 of session.py is a runtime import, not TYPE_CHECKING. isinstance(self.agent, Agent) works correctly. No fix needed.

[gemini-code-assist]

Swallowing unexpected exceptions silently inside _build_toolset makes troubleshooting extremely difficult. If an MCP server fails to return tools, the error is completely swallowed and the operator has no visibility into why the tools are missing.

Please log the exception at warning or error level before returning None.

Suggested change

	async def _build_toolset(ctx: Any) -> Any:
	tools = await self.get_tools()
	try:
	tools = await self.get_tools()
	except Exception:
	return None
	if not tools:
	return None
	async def _build_toolset(ctx: Any) -> Any:
	try:
	tools = await self.get_tools()
	except Exception as e:
	logger.warning("Failed to retrieve tools from provider %s: %s", self.name, e, exc_info=True)
	return None
	if not tools:
	return None

[Leoyzen]

✅ Fixed in commit 157fc4799 (originally in 83d7eb592). Added logger.warning() with provider name and exc_info=True:

except Exception:
    logger.warning(
        "Failed to retrieve tools from provider",
        provider=self.name,
        exc_info=True,
    )
    return None

Test: test_build_toolset_logs_warning_on_exception in test_global_pool_review_fixes.py.

[Leoyzen]

Gemini Code Review — Response

Thanks for the thorough review! All valid issues have been fixed in commit 83d7eb592 with TDD (5 new tests in test_global_pool_review_fixes.py).

#1 🔴 CRITICAL: release() race condition — ✅ Fixed

Valid. _signal_shutdown_locked for stdio only set close_event without popping from _connections. The connection remained in cache until release() popped it after done_event.wait() (up to 10s window).

Fix: _signal_shutdown_locked now always pops from _connections, regardless of transport type. The redundant second pop in release() and _evict_if_needed() has been removed.

Test: test_dying_connection_removed_from_cache — calls _signal_shutdown_locked directly and asserts connection is immediately removed.

#2 🔴 CRITICAL: HTTP/SSE connection leak — ⚠️ Not an issue (by design)

Partially valid analysis, but the conclusion is incorrect. HTTP/SSE connections in _connections are lightweight transport objects, not active network connections. The actual HTTP connection lifecycle is managed by MCPToolset.__aenter__/__aexit__. The _connections entry serves as a cache for client_id → transport mapping, not as a connection holder.

However, the entry IS cleaned up: _signal_shutdown_locked pops HTTP/SSE entries (now unified for all types). release() calls _signal_shutdown_locked when ref_count <= 0. So no leak occurs.

The proposed _HTTPReleaseTransport wrapper is unnecessary — it would add complexity without solving a real problem, since HTTP connections don't need owner-task management.

#3 🟠 HIGH: _signal_shutdown_locked should always pop — ✅ Fixed (same as #1)

Unified: _signal_shutdown_locked now always pops, simplifying the logic.

#4 🟠 HIGH: HTTP/SSE ref count not incremented on reuse — ⚠️ Not an issue in practice

Valid analysis of the code, but not a real bug. The ref count for HTTP/SSE is indeed not incremented when reusing an existing entry. However:

• release() calls _signal_shutdown_locked when ref_count <= 0, which pops the entry
• Subsequent release() calls find conn is None and return early (L291-293)
• So ref_count never actually goes negative in a harmful way

That said, the semantics are cleaner now with the unified pop. The ref count for HTTP/SSE could be improved in a follow-up, but it's not a blocking issue.

#5 🟠 HIGH: Missing Agent import in session.py — ❌ Not a bug

Incorrect. Agent is imported at line 28:

from agentpool import Agent, AgentPool

This is a runtime import, not a TYPE_CHECKING import. isinstance(self.agent, Agent) works correctly.

#6 🟡 MEDIUM: Silent exception in _build_toolset — ✅ Fixed

Valid. Added logger.warning() with provider name and exc_info=True:

except Exception:
    logger.warning(
        "Failed to retrieve tools from provider",
        provider=self.name,
        exc_info=True,
    )
    return None

Test: test_build_toolset_logs_warning_on_exception — verifies logger.warning is present in the as_capability() source code.

Summary

#	Severity	Verdict	Action
1	CRITICAL	✅ Valid	Fixed: unified pop in _signal_shutdown_locked
2	CRITICAL	⚠️ By design	No fix needed — HTTP/SSE entries are cleaned up by release()
3	HIGH	✅ Valid	Fixed (same as #1)
4	HIGH	⚠️ Not harmful	Ref count semantics improved by #1 fix
5	HIGH	❌ Incorrect	Import exists at session.py:28
6	MEDIUM	✅ Valid	Fixed: added logger.warning()

Test results: 139 tests pass, ruff clean.

[Million-mo]

PR Review: MCP Provider Lifecycle Architecture

🧹 PR 卫生问题

以下文件不应包含在此 PR 中,建议移除:

• xeno-agent/config/diag-agent-ng.yaml (186 行) — 这是特定部署的配置文件(使用 svc/glm-4.7 等私有模型名),不属于上游 agentpool 框架。

📐 架构问题 1: 不同层级的 MCP 连接和复用

三层架构设计整体正确,解决了 4 个根因。逐层验证:

层级	复用机制	验证结论
Pool 级 (GlobalConnectionPool)	client_id 缓存,stdio owner-task 复用,HTTP/SSE 每次 fresh	✅ 正确
Session 级 (SessionConnectionPool)	(client_id, skill_name) 二元组键,ACP 预创建 transport 复用	✅ 正确
Snapshot 级 (McpConfigSnapshot)	frozen dataclass,父子继承 session_configs	✅ 正确

遗留问题 — HTTP/SSE ref_count 语义不正确 (global_pool.py:1957-1963):

elif existing is not None and not existing.is_stdio:
    # HTTP/SSE: 不递增 ref_count,但创建新 transport
    transport = config.to_transport()

get_transport() 不递增 ref_count,但 release() 递减 → ref_count 会变为负数。当前因 _signal_shutdown_locked 统一 pop 而不会泄漏,但语义不正确。建议:

• 方案 A: 实现 Gemini 建议的 _HTTPReleaseTransport wrapper,在 connect_session() 退出时自动 release()
• 方案 B: HTTP/SSE 路径完全不使用 ref_count(每次 get_transport() 后立即 pop 缓存条目)

📐 架构问题 2: 生命周期时序

我构建了三个层级的完整生命周期时序图,以下是需要注意的跨层级交互风险:

⚠️ copy_pre_created_transports 浅复制 transport 引用

session_pool.py:2782-2788:子 session 复制父的 ACP transport 引用(非深拷贝)。这是有意为之(ACP transport 支持并发 connect_session()),但意味着:

• 父 session 关闭时如果调用 conn.close(),会关闭所有 pair,包括子 session 正在使用的
• 当前 close_session() 只调用 session_pool.cleanup(),不调用 conn.close(),所以不会触发
• 但如果未来有人修改清理逻辑,可能引入问题

建议:在 SessionConnectionPool.cleanup() 中明确注释:只清理 owner_task(stdio),不清理 pre-created transport 的底层连接(由 AcpMcpConnectionManager 管理)。

⚠️ threading.Lock 在 asyncio 场景下的设计意图

GlobalConnectionPool 和 SessionConnectionPool 都用 threading.Lock。agentpool 是单事件循环架构,通常 asyncio.Lock 更自然。用 threading.Lock 的原因可能是:

• 防止多线程调用(虽然 agentpool 设计为单 loop)
• threading.Lock 不需要 await,在非 async 上下文中也能用

但要注意:release() 在锁内调用 _signal_shutdown_locked 后,锁外 await stdio owner task — 这是正确的(不在锁内 await),但如果有人误在锁内加 await,会死锁。建议添加注释说明锁边界。

✅ 总结

架构设计正确,4 个根因均已解决。建议:

1. 移除 xeno-agent/config/diag-agent-ng.yaml
2. 修正 HTTP/SSE ref_count 语义
3. 添加跨层级交互的注释

[Million-mo]

架构评审:当前 MCP 设计是否最优?

从资深架构师角度的深度分析。当前设计正确但不优雅 — 它是"防御性编程"的产物,每个问题都有一层防护,但层与层之间有大量重复和胶水代码。

---

问题 1:连接池代码重复 — 两个池 80% 逻辑相同

GlobalConnectionPool 和 SessionConnectionPool 各自独立实现了:

• owner-task 模式(stdio transport 生命周期管理)
• _PooledConnection / _SessionConnection dataclass(字段几乎相同)
• get_transport() 方法(锁内创建 + 锁外 await ready)
• cleanup() / shutdown_all()(signal close + await + cancel)

区别仅在于:键不同(client_id vs (client_id, skill_name))、HTTP/SSE 策略不同、ACP 支持不同。

任何 owner-task 逻辑的修改都要改两处,容易出现行为不一致。

改进:统一为 MCPConnectionPool + 策略注入

class MCPConnectionPool:
    """统一的 MCP 连接池,行为由策略决定。"""
    
    def __init__(
        self,
        scope: Literal["global", "session"],
        session_id: str | None = None,
        reuse_policy: ConnectionReusePolicy = "shared",  # shared | per_call | pre_created_only
    ) -> None:
        self._scope = scope
        self._session_id = session_id
        self._reuse_policy = reuse_policy
        self._connections: dict[ConnectionKey, _PooledConnection] = {}
        self._lock = asyncio.Lock()

GlobalConnectionPool → MCPConnectionPool(scope="global", reuse_policy="shared"),SessionConnectionPool → MCPConnectionPool(scope="session", reuse_policy="pre_created_only")。-300 行重复代码,行为统一。

---

问题 2:threading.Lock 在纯 asyncio 系统中是设计气味

两个池都用 threading.Lock,但 agentpool 是单事件循环架构。threading.Lock 是阻塞调用 — 锁被持有时事件循环会卡住。

当前代码小心地在锁外做 await,但这是一个脆弱的约定 — 没有编译器或运行时检查来防止未来开发者违反它。

改进:使用 asyncio.Lock

class MCPConnectionPool:
    def __init__(self) -> None:
        self._lock = asyncio.Lock()
    
    async def get_transport(self, config: BaseMCPServerConfig) -> ClientTransport:
        async with self._lock:
            # 全部在锁内,因为 asyncio.Lock 不会阻塞事件循环
            existing = self._connections.get(config.client_id)
            if existing is not None and existing.is_stdio:
                existing.ref_count += 1
                return existing.shared_session_transport or existing.transport

asyncio.Lock 的关键优势:await lock.acquire() 不会阻塞事件循环,其他协程可以继续运行。当前 threading.Lock 在高并发下会阻塞整个事件循环。

---

问题 3:MCPToolset 每次 get_agentlet() 重建 — 浪费且不必要

manager.py:2359:"A new MCPToolset instance is created on every call — no caching."

get_agentlet() 在每次 turn 中调用(可能每秒多次),但 MCPToolset 构造涉及配置解析、elicitation handler 创建、timeout logger 创建 — 这些每次都一样。transport 已经复用了,但 toolset 层没有。

改进:transport 级别复用 + toolset 级别缓存

class MCPManager:
    def __init__(self) -> None:
        self._toolset_cache: dict[str, MCP] = {}  # client_id → MCP capability
    
    async def as_capability(self, snapshot, session_pool) -> list[MCP]:
        capabilities = []
        for entry in snapshot.all_configs:
            cache_key = f"{entry.source}:{server.client_id}:{entry.skill_name or ""}"
            
            if cache_key in self._toolset_cache:
                capabilities.append(self._toolset_cache[cache_key])
                continue
            
            transport = await self._get_transport(entry, session_pool)
            cap = _make_capability(server, transport)
            self._toolset_cache[cache_key] = cap  # 缓存
            capabilities.append(cap)
        return capabilities

toolset 缓存在 transport 失效时清理(通过 release() 时清理对应 cache entry)。

---

问题 4:三层 snapshot 传递 — 信息穿越太多层级

MCP 配置从 YAML → MCPManager.servers → McpConfigSnapshot → Agent._mcp_snapshot → as_capability(),涉及 5 个层级。snapshot 被修改了 3 次(with_session_configs + with_skill_configs),虽然 frozen 但每次创建新实例替换。Agent 类承担了不属于它的职责 — 管理 MCP snapshot 的增量构建。

改进:引入 McpConfigRegistry 作为单一配置源

class McpConfigRegistry:
    """MCP 配置的单一真相源,分层注册,统一查询。"""
    
    def register_pool(self, configs: list[McpConfigEntry]) -> None: ...
    def register_agent(self, configs: list[McpConfigEntry]) -> None: ...
    def register_session(self, configs: list[McpConfigEntry]) -> None: ...
    def register_skill(self, skill_name: str, configs: list[McpConfigEntry]) -> None: ...
    
    def snapshot(self) -> McpConfigSnapshot:
        """构建不可变快照,用于 capability 构建。"""
        ...

Agent 不再持有 _mcp_snapshot,而是持有 registry 引用。配置注册在各层自然发生,get_agentlet() 调用 registry.snapshot() 获取一次性快照。关注点分离,Agent 不再管 MCP 配置增量构建。

---

问题 5:ACP transport register_session() 的整数 key 是竞态隐患

register_session() 不是 async,但操作了共享状态(_session_streams, _next_session_key)。在 asyncio 单循环中通常安全,但如果未来有 asyncio.to_thread 调用会出竞态。unregister_session() 用线性搜索找 pair 对象,O(n) 复杂度。

改进:用 id(pair) 或 UUID 做 key,O(1) 注销

def register_session(self) -> SessionStreamPair:
    pair = SessionStreamPair()
    self._session_streams[id(pair)] = pair
    return pair

def unregister_session(self, pair: SessionStreamPair) -> None:
    self._session_streams.pop(id(pair), None)  # O(1)

或更彻底:让 SessionStreamPair 实现 __aenter__/__aexit__,自动管理生命周期。

---

更根本的思考:是否需要这么多层?

从使用者视角思考,get_agentlet() 只需要:"给我这个 agent 在此 session 中可用的 MCP capabilities 列表"。这个问题可以由一个单一组件回答:

class McpCapabilityProvider:
    """MCP 能力的单一提供者,内部管理所有复杂性。"""
    
    async def get_capabilities(
        self, agent: Agent, session_id: str, skill_configs: list[McpConfigEntry]
    ) -> list[MCP]:
        """返回 agent 在此 session 中可用的所有 MCP capabilities。"""
        session_pool = self._get_or_create_session_pool(session_id)
        configs = self._collect_configs(agent, session_pool, skill_configs)
        return [self._make_capability(c, await self._get_transport(c, session_pool))
                for c in configs]

收益:

• Agent 不需要 _mcp_snapshot, _session_connection_pool, _build_pool_configs(), _build_agent_configs() — 全部移除
• AgentPool 不需要在 get_or_create_session_agent() 中构建 snapshot
• ACPSession.initialize_mcp_servers() 不需要操作 agent._mcp_snapshot
• snapshot 作为内部优化,而非跨层传递的协议

as_capability() 的双路径问题

当前有两条路径(snapshot vs legacy),get_agentlet() 和 as_capability() 都有分支。双路径意味着测试要覆盖 2x 组合,且 legacy 路径可能逐渐腐烂。建议移除 legacy 路径,snapshot 作为唯一路径。

---

改进后的目标架构

┌─────────────────────────────────────────────────────────────┐
│                    McpCapabilityProvider                     │
│  ┌─────────────────────────────────────────────────────┐   │
│  │  get_capabilities(agent, session_id, skill_configs) │   │
│  │  → 收集配置 → 分流到 pool → 构建 MCPToolset        │   │
│  └─────────────────────────────────────────────────────┘   │
│                              │                              │
│              ┌───────────────┼───────────────┐              │
│              ▼               ▼               ▼              │
│  ┌────────────────┐ ┌────────────────┐ ┌───────────────┐  │
│  │ MCPConnectionPool│ │ MCPConnectionPool│ │ ConfigRegistry│  │
│  │ scope="global"  │ │ scope="session" │ │ (配置真相源)  │  │
│  │ stdio: owner-task│ │ per-session     │ │              │  │
│  │ HTTP: per-call  │ │ ACP: pre-created│ │              │  │
│  └────────────────┘ └────────────────┘ └───────────────┘  │
└─────────────────────────────────────────────────────────────┘

关键变化:

1. 两个池 → 一个池类 + 策略(-300 行重复)
2. 三层 snapshot 传递 → ConfigRegistry + 一次性 snapshot(-~100 行胶水代码)
3. threading.Lock → asyncio.Lock(消除事件循环阻塞风险)
4. MCPToolset 每次重建 → 按需缓存(减少 per-turn 开销)
5. 双路径 → 单路径(移除 legacy,降低测试矩阵)

---

改进优先级

改进	影响	难度	优先级
1. 统一两个连接池类	-300 行重复,行为一致性	中(接口对齐)	P0
2. threading.Lock → asyncio.Lock	消除事件循环阻塞风险	低(机械替换)	P0
3. 移除 legacy 路径	降低测试矩阵,防止腐烂	低(删除代码)	P1
4. ConfigRegistry 替代 snapshot 传递	关注点分离,-100 行胶水	中(重构 Agent/Pool)	P1
5. MCPToolset 缓存	减少 per-turn 开销	高(失效逻辑复杂)	P2

---

结论

当前设计正确但不优雅。最优雅的设计应该满足三个标准:

1. 单一职责:每个组件只做一件事(当前 Agent 管了太多 MCP 配置)
2. 无重复:相同逻辑只写一次(当前两个池 80% 代码重复)
3. 最小接口:使用者不需要知道内部分层(当前 get_agentlet() 要感知 snapshot 是否存在)

改进后的架构将 3 个类合并为 2 个(MCPConnectionPool + ConfigRegistry),消除重复,简化接口,同时保持所有正确性保证。这不是理论上的优化 — 它能减少约 400 行代码,同时让系统更容易理解和维护。

以上建议可以作为 follow-up 重构的方向,不阻塞当前 PR 合并。

[Leoyzen]

回复架构评审

感谢详细的架构评审！逐条回复：

PR 卫生：xeno-agent/config/diag-agent-ng.yaml

✅ 已在 commit 157fc4799 中移除。

HTTP/SSE ref_count 语义

⚠️ 已添加 TODO 注释。语义不正确但当前无 bug（release() 只在 disconnect_all() 中调用，不是 per-turn）。Follow-up 采纳方案 B（HTTP/SSE 不使用 ref_count）。

Oracle 裁决：非阻塞，follow-up 项。

copy_pre_created_transports 浅复制

✅ 已在 SessionConnectionPool.cleanup() docstring 中添加注释，说明 pre-created ACP transport 的生命周期由 AcpMcpConnectionManager 管理，不在 cleanup() 中关闭。

threading.Lock vs asyncio.Lock

⚠️ 保持 threading.Lock，已添加注释说明锁边界。理由：

1. 锁内操作都是同步 dict 操作，持锁时间微秒级
2. 改 asyncio.Lock 需要把 _signal_shutdown_locked 等同步方法变 async，改动面大
3. 代码中无锁内 await，不会阻塞事件循环

Oracle 裁决：PR owner 正确，follow-up 项。

统一两个连接池（P0）

❌ 不做。两个池的差异是结构性的（stdio 共享 vs per-session、HTTP per-call vs 缓存、ACP 不支持 vs 支持），不是策略注入能简洁覆盖的。如果未来两个池趋同，可以提取公共逻辑到基类。

Oracle 裁决：PR owner 正确，差异是结构性的。

移除 legacy 路径（P1）

⚠️ Follow-up。需要同时处理 standalone agent 场景（无 pool 时 snapshot 为 None）。

MCPToolset 缓存（P2）

❌ 不做。缓存 MCPToolset 正是导致跨 task CancelScope 错误的根因——这个 PR 就是为了修复它。transport 已通过连接池复用，MCPToolset 构造是轻量的（配置解析，不是 I/O）。

Oracle 裁决：PR owner 正确，缓存会重新引入 bug。

McpConfigRegistry 替代 snapshot 传递（P1）

⚠️ 有价值的方向，但属于较大重构。当前 snapshot 传递虽然有胶水代码，但逻辑清晰且有测试覆盖。Follow-up PR。

register_session() 整数 key（P2）

⚠️ 有效建议。当前用自增整数 + 线性搜索注销，可以改成 id(pair) 做 key。非阻塞，follow-up。

总结

建议	采纳	理由
移除 yaml	✅ 已修	—
ref_count 修正	⚠️ Follow-up	加了 TODO，当前无 bug
cleanup 注释	✅ 已修	—
Lock 注释	✅ 已修	—
统一连接池	❌ 不做	差异结构性
MCPToolset 缓存	❌ 不做	会重新引入 bug
移除 legacy	⚠️ Follow-up	需处理 standalone
ConfigRegistry	⚠️ Follow-up	较大重构
register_session key	⚠️ Follow-up	非阻塞

所有争议点经 Oracle 裁决均不阻塞合并。1010 tests pass 验证了功能正确性。

[Leoyzen]

/gemini review

[gemini-code-assist]

Code Review

This pull request implements a robust three-tier MCP connection scoping architecture (immutable config snapshots, global/session connection pools, and lazy toolset materialization) to resolve race conditions, cross-task CancelScope errors, and support stateful MCP isolation. It introduces McpConfigSnapshot and McpConfigEntry to manage configurations, replaces the legacy MCPConnectionPool with GlobalConnectionPool and SessionConnectionPool, and updates the orchestrator and ACP session handlers to bypass the mutable agent.tools.providers list. Additionally, it fixes a CancelScope error in streaming_adapter.py and adds comprehensive tests. The review feedback highlights two critical issues: a missing runtime import of Agent in session.py that will cause a NameError during MCP server initialization, and the need for GlobalConnectionPool to clean up dead stdio connections from its cache in the owner task's finally block to prevent returning crashed connections on subsequent requests.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.