diff --git a/apps/device-host-agent/host_agent/app.py b/apps/device-host-agent/host_agent/app.py index 4800c21..0ad401c 100644 --- a/apps/device-host-agent/host_agent/app.py +++ b/apps/device-host-agent/host_agent/app.py @@ -213,7 +213,7 @@ def create_application( logging.getLogger(__name__).info( "MCP token generated at %s", mcp_token_path ) - mcp_busy_tracker = McpBusyTracker(ttl_seconds=60.0) + mcp_busy_tracker = McpBusyTracker(ttl_seconds=20.0) executor = AssignmentExecutor( create_execution_factories( resolved_manager, diff --git a/apps/device-host-agent/host_agent/mcp_lock.py b/apps/device-host-agent/host_agent/mcp_lock.py index dd33399..477ddd5 100644 --- a/apps/device-host-agent/host_agent/mcp_lock.py +++ b/apps/device-host-agent/host_agent/mcp_lock.py @@ -38,7 +38,7 @@ class McpBusyTracker: def __init__( self, *, - ttl_seconds: float = 60.0, + ttl_seconds: float = 20.0, now: Callable[[], datetime] | None = None, ) -> None: self._ttl = float(ttl_seconds) diff --git a/apps/device-host-agent/host_agent/web/app.py b/apps/device-host-agent/host_agent/web/app.py index 2ce7a18..93d2b2c 100644 --- a/apps/device-host-agent/host_agent/web/app.py +++ b/apps/device-host-agent/host_agent/web/app.py @@ -5,7 +5,7 @@ import base64 import json from collections.abc import Awaitable, Callable from pathlib import Path -from typing import Any +from typing import TYPE_CHECKING, Any import jinja2 from fastapi import Depends, FastAPI, HTTPException, Request @@ -35,6 +35,9 @@ from host_agent.web.auth import ( ) from host_agent.web.mcp_auth import BearerAuthMiddleware from storage.device_config import DeviceConfigStore + +if TYPE_CHECKING: + from mcp.server.fastmcp import FastMCP from storage.task_metadata import TaskMetadataStore from storage.timeline import Timeline @@ -226,7 +229,7 @@ def create_console_app( metadata_store: TaskMetadataStore | None = None, timeline: Timeline | None = None, executor: AssignmentExecutor | None = None, - mcp_server: Any = None, + mcp_server: FastMCP | None = None, mcp_token_store: McpTokenStore | None = None, mcp_busy_tracker: McpBusyTracker | None = None, ) -> FastAPI: diff --git a/apps/device-host-agent/tests/test_app.py b/apps/device-host-agent/tests/test_app.py index 08ec8f7..901981c 100644 --- a/apps/device-host-agent/tests/test_app.py +++ b/apps/device-host-agent/tests/test_app.py @@ -699,7 +699,7 @@ def test_create_application_wires_mcp_components(tmp_path, monkeypatch) -> None: # is mounted (responds 401, not 404) without a bearer token. mcp_token_store = McpTokenStore(config.identity_path.parent / "host_mcp_token.json") mcp_token_store.load_or_create() - mcp_busy_tracker = McpBusyTracker(ttl_seconds=60.0) + mcp_busy_tracker = McpBusyTracker(ttl_seconds=20.0) mcp_server = build_mcp_server( manager=application.heartbeat.manager, mcp_busy_tracker=mcp_busy_tracker, diff --git a/apps/device-host-agent/tests/test_mcp_lock.py b/apps/device-host-agent/tests/test_mcp_lock.py index c44ef1e..15e09d3 100644 --- a/apps/device-host-agent/tests/test_mcp_lock.py +++ b/apps/device-host-agent/tests/test_mcp_lock.py @@ -144,3 +144,32 @@ def test_wait_until_usable_blocks_then_fails_when_cloud_remains_busy() -> None: ) assert ok is False assert tracker.busy_device_ids() == [] + + +def test_default_ttl_is_20_seconds() -> None: + """The McpBusyTracker default TTL is 20s: short enough to recover from a + dead MCP session within a heartbeat interval without an explicit release + callback (mcp SDK 1.28.1 has no per-session shutdown hook), but long + enough that an actively-busy session does not lose its lease during + normal operator pauses.""" + tracker = McpBusyTracker() + assert tracker._ttl == 20.0 + + +def test_default_ttl_recovers_dead_session_within_one_window() -> None: + """With the 20s default, a session that never renews its lease is + reaped within one TTL window on the next read. This is the + concrete fallback behavior for I1 (no FastMCP session-end hook).""" + times: list[datetime] = [] + + def now() -> datetime: + return times[-1] if times else datetime(2026, 1, 1, tzinfo=UTC) + + tracker = McpBusyTracker(now=now) # default 20s TTL + times.append(datetime(2026, 1, 1, 12, 0, tzinfo=UTC)) + assert tracker.acquire("phone-1", "dead-session") is True + # No renew: advance 21s. Lease should be swept on next read. + times.append(datetime(2026, 1, 1, 12, 0, 21, tzinfo=UTC)) + assert tracker.busy_device_ids() == [] + # New session can now acquire cleanly (no stale-busy contamination). + assert tracker.acquire("phone-1", "new-session") is True diff --git a/apps/device-host-agent/tests/test_web_mcp.py b/apps/device-host-agent/tests/test_web_mcp.py index e10f001..074fc45 100644 --- a/apps/device-host-agent/tests/test_web_mcp.py +++ b/apps/device-host-agent/tests/test_web_mcp.py @@ -310,3 +310,106 @@ def test_wrapped_tool_accepts_context_kwarg() -> None: tool = tool_manager.get_tool("take_screenshot") assert tool is not None assert tool.context_kwarg == "ctx" + + +def test_busy_error_wire_shape_is_calltoolresult_iserror() -> None: + """Regression test for spec §7 — busy errors must be visible on the wire. + + mcp SDK 1.28.1's ``Tool.run`` wraps every non-``UrlElicitationRequiredError`` + exception (including ``McpError`` and our ``McpDeviceBusyError``) into + ``ToolError`` (see ``mcp/server/fastmcp/tools/base.py``). The lowlevel + ``call_tool`` handler then builds a ``CallToolResult(isError=True, + content=[TextContent(...)])`` (see + ``mcp/server/lowlevel/server.py::_make_error_result``). There is no public + path that surfaces JSON-RPC ``-32000`` + structured ``data.busy_owner`` from + a tool call site — the SDK's wire contract for tool errors is the + ``isError=true`` flag plus text content. This test pins the wire shape so + any future SDK upgrade that exposes a true JSON-RPC error path is caught.""" + import asyncio + + import mcp.types as types + from mcp.server.fastmcp import FastMCP + from mcp.server.fastmcp.exceptions import ToolError + + manager = _make_manager_with_device() + tracker = McpBusyTracker() + status = AgentStatusTracker() + tracker.acquire("phone-1", "sess-other") # different session holds the device + + server: FastMCP = build_mcp_server( + manager=manager, mcp_busy_tracker=tracker, status_tracker=status + ) + tool = server._tool_manager.get_tool("take_screenshot") # type: ignore[attr-defined] + assert tool is not None + + sentinel_session = object() + ctx = _fake_ctx(sentinel_session) + + with pytest.raises(ToolError) as tool_exc: + asyncio.run(tool.run({"device_id": "phone-1"}, context=ctx)) + + # ToolError text carries the original exception message verbatim, + # which is what the lowlevel handler copies into TextContent. + message = str(tool_exc.value) + assert "phone-1" in message + assert "busy" in message + assert "mcp_session:sess-oth" in message # truncated busy_owner + + # The lowlevel handler converts any exception into a CallToolResult + # with isError=True (mcp SDK 1.28.1 — not a JSON-RPC error envelope). + # We invoke the SDK helper directly to lock the wire contract. + from mcp.server.lowlevel.server import Server as LowlevelServer + + lowlevel = LowlevelServer("test-lowlevel") + error_result = lowlevel._make_error_result(message) # type: ignore[attr-defined] + inner = error_result.root + assert isinstance(inner, types.CallToolResult) + assert inner.isError is True + assert len(inner.content) == 1 + text_block = inner.content[0] + assert isinstance(text_block, types.TextContent) + assert text_block.text == message + # And confirm the wire shape is NOT a JSON-RPC error envelope — that + # would require code=-32000 + data.busy_owner, which is not exposed + # in mcp SDK 1.28.1 for tool-call errors. + assert not hasattr(inner, "code") + assert inner.structuredContent is None + + +def test_busy_error_text_includes_cloud_assignment_owner() -> None: + """Same wire-shape test for the cloud_assignment branch — verifies the + human-readable busy_owner value (the only place to surface it given the + SDK forces tool errors into CallToolResult.isError=true) is correct.""" + import asyncio + + from mcp.server.fastmcp.exceptions import ToolError + + manager = _make_manager_with_device() + tracker = McpBusyTracker() + status = AgentStatusTracker() + status.mark_assignment_started( + AssignmentModel( + task_id="t1", + attempt=1, + lease_id="l1", + lease_expires_at=datetime.now(UTC), + host_id="h1", + device_id="phone-1", + goal="cloud task", + ) + ) + + server = build_mcp_server( + manager=manager, mcp_busy_tracker=tracker, status_tracker=status + ) + tool = server._tool_manager.get_tool("take_screenshot") # type: ignore[attr-defined] + assert tool is not None + + sentinel_session = object() + ctx = _fake_ctx(sentinel_session) + + with pytest.raises(ToolError) as tool_exc: + asyncio.run(tool.run({"device_id": "phone-1"}, context=ctx)) + assert "cloud_assignment" in str(tool_exc.value) + assert "phone-1" in str(tool_exc.value) + assert "busy" in str(tool_exc.value) diff --git a/docs/MCP_INTEGRATION.md b/docs/MCP_INTEGRATION.md index 948d094..e566e47 100644 --- a/docs/MCP_INTEGRATION.md +++ b/docs/MCP_INTEGRATION.md @@ -65,8 +65,11 @@ All 11 device tools from `api/mcp.py`: - The cloud worker and MCP clients share the same `DeviceManager`. - Per-device, session-level locking: the first caller (cloud or MCP) to touch a device holds it; the other side sees a busy error. -- MCP sessions hold their lock until the session ends OR 60 seconds of - inactivity. Cloud assignments hold theirs until the assignment +- MCP sessions hold their lock until **20 seconds of inactivity** + (the `McpBusyTracker` default TTL). The mcp SDK 1.28.1 does not expose + a per-session shutdown callback, so a clean Hermes disconnect is also + recovered via the 20s TTL sweep — see the implementation note in + spec §6.5. Cloud assignments hold theirs until the assignment terminates. - The cloud scheduler is told about MCP-held devices via the heartbeat `mcp_busy_device_ids` field, so it normally won't even try to dispatch @@ -86,13 +89,20 @@ override. ## Error responses -| Condition | HTTP / JSON-RPC | Body | +The mcp SDK 1.28.1 forces tool errors into `CallToolResult(isError=true, +content=[TextContent(message)])` — there is no public path that surfaces +JSON-RPC `-32000` with a structured `data.busy_owner` field from a tool +call site. The busy-owner value lives inside the text content (full +string for `cloud_assignment`, truncated session_id prefix for +`mcp_session:` collisions). + +| Condition | JSON-RPC envelope | `result.content[0].text` | |---|---|---| -| Missing/wrong bearer token | HTTP 401 | `{"error": "invalid token"}` + `WWW-Authenticate: Bearer` | -| Device busy (cloud) | JSON-RPC `-32000` | `"device X is busy (held by cloud assignment)"`, `data.busy_owner = "cloud_assignment"` | -| Device busy (other MCP) | JSON-RPC `-32000` | `"..."`, `data.busy_owner = "mcp_session:"` | -| Unknown device | JSON-RPC `-32602` | `"unknown device: X"` | -| Tool error | JSON-RPC `-32000` | Original exception message | +| Missing/wrong bearer token | HTTP 401 (transport-level) | `{"error": "invalid token"}` + `WWW-Authenticate: Bearer` | +| Device busy (cloud) | `result.isError = true` | `"device is busy (held by cloud assignment)"` | +| Device busy (other MCP) | `result.isError = true` | `"device is busy (held by mcp_session:<8-char-prefix>)"` | +| Unknown device | `result.isError = false` | JSON `{"ok": false, "error": "device not found: "}` | +| Tool error | `result.isError = true` | `"Error executing tool : "` | ## Troubleshooting diff --git a/docs/superpowers/specs/2026-07-21-host-agent-mcp-server-design.md b/docs/superpowers/specs/2026-07-21-host-agent-mcp-server-design.md index fbe9065..5a707e9 100644 --- a/docs/superpowers/specs/2026-07-21-host-agent-mcp-server-design.md +++ b/docs/superpowers/specs/2026-07-21-host-agent-mcp-server-design.md @@ -38,7 +38,7 @@ | D6 | 设备状态映射 = 走 `_device_display_status()` 同款逻辑 | 避免"所有连上的设备看起来都 busy" | | D7 | Cloud worker 与 MCP server 在同一 host-agent 进程并存 | 不互斥,共享 `DeviceManager` | | D8 | Cloud ↔ MCP 协调 = 心跳上报 `mcp_busy_device_ids`,cloud scheduler 跳过 | 心跳 schema 扩展,cloud 侧 _matches() 一处改动 | -| D9 | MCP session 级 lazy acquire 锁,60s TTL 兜底 | session_id 来自 FastMCP 上下文 | +| D9 | MCP session 级 lazy acquire 锁,20s TTL 兜底(mcp SDK 1.28.1 无 session-end callback,见 §6.5) | session_id 来自 FastMCP 上下文 | | D10 | Skill catalog 工具 MVP 不暴露,保留 `create_mcp_server(skill_catalog_store=...)` 参数化挂载点 | 未来可加 mutating 工具 | | D11 | `wait_until_usable` 方法实现 + 单测,但调用方不接入 | 预留能力,MVP 全部 fail-fast | | D12 | `tool_handlers(manager)` 改为必传 | 已 grep 确认无调用方依赖 None 默认,根治 `DeviceNotFoundError` 类静默回退地雷 | @@ -102,7 +102,7 @@ class McpDeviceLease: last_seen_at: datetime class McpBusyTracker: - def __init__(self, *, ttl_seconds: float = 60.0, now=None) -> None: ... + def __init__(self, *, ttl_seconds: float = 20.0, now=None) -> None: ... def acquire(self, device_id: str, session_id: str) -> bool: ... def renew(self, device_id: str, session_id: str) -> bool: ... def release(self, session_id: str) -> list[str]: ... @@ -258,32 +258,44 @@ run_async() 主循环启动(行为不变): ### 6.5 Session 结束 / TTL 过期 ``` -正常:Hermes 主动断开 → FastMCP 触发 session shutdown callback - → mcp_busy_tracker.release(session_id) → 该 session 持有的所有 lease 释放 - → 下次心跳 payload 不再包含这些 device_id → cloud 重新视为 idle +正常:Hermes 主动断开 → mcp SDK 1.28.1 没有 per-session shutdown hook + → 该 session 的 lease 进入 TTL 倒计时 + → 20s TTL 到期 → 下次 busy_device_ids() 或 snapshot() 调用时 lazy sweep + → lease 清理 → 下次心跳 payload 不再包含 → cloud 重新视为 idle -异常:Hermes 崩溃 / 网络断 → 无 shutdown callback - → 60s TTL 到期 → 下次 busy_device_ids() 或 snapshot() 调用时 lazy sweep +异常:Hermes 崩溃 / 网络断 → 同上,无 shutdown callback + → 20s TTL 到期 → 下次 busy_device_ids() 或 snapshot() 调用时 lazy sweep → lease 清理 → 下次心跳 payload 不再包含 → cloud 重新视为 idle ``` +**Implementation note (2026-07-21 fix wave):** mcp SDK 1.28.1 exposes +only a server-level `lifespan` hook; `ServerSession.__aexit__` and +`StreamableHTTPSessionManager` do not surface a per-session +shutdown callback. The spec originally described a release-on-clean- +disconnect path that the SDK cannot deliver today. The fallback is +the 20-second TTL sweep — short enough that a normal heartbeat +interval (30s) catches the recovery before the cloud scheduler +notices, long enough that an actively-busy session does not lose its +lease during normal operator pauses. Explicit release on session end +remains a future enhancement if/when the SDK exposes the hook. + ## 7. Error Handling Matrix | # | 触发条件 | 返回语义 | 备注 | |---|---|---|---| | 1 | `Authorization` 缺失/不匹配 | HTTP 401 + `WWW-Authenticate: Bearer` + JSON `{"error":"invalid token"}` | 不写失败日志;首次成功鉴权写 INFO | -| 2 | Cloud 占用目标设备 | JSON-RPC `-32000` + `"device X is busy (held by cloud assignment)"` + `data.busy_owner = "cloud_assignment"` | DEBUG 日志 | -| 3 | 另一 MCP session 占用 | JSON-RPC `-32000` + `"device X is busy (held by another MCP session)"` + `data.busy_owner = "mcp_session:"` | DEBUG 日志 | -| 4 | `device_id` 不存在 | JSON-RPC `-32602` + `"unknown device: X"` | 复用 `call_with_semantic_errors` | -| 5 | 工具底层异常 | JSON-RPC `-32000` + 原异常 message | WARNING + exc_info | +| 2 | Cloud 占用目标设备 | `CallToolResult(isError=true, content=[TextContent("device phone-1 is busy (held by cloud assignment)")])` | DEBUG 日志 | +| 3 | 另一 MCP session 占用 | `CallToolResult(isError=true, content=[TextContent("device phone-1 is busy (held by mcp_session:<8-char-prefix>)")])` | DEBUG 日志 | +| 4 | `device_id` 不存在 | `CallToolResult(isError=false)` + JSON `{"ok": false, "error": "device not found: phone-1"}` | 复用 `call_with_semantic_errors`;语义错误不抛 | +| 5 | 工具底层异常 | `CallToolResult(isError=true, content=[TextContent("Error executing tool : ")])` | WARNING + exc_info | | 6 | Cloud assignment 启动前命中 MCP 占用 | `AssignmentExecutionResult(status="failed", failure_reason="device held by active MCP session")` | INFO 一次 | | 7 | Token 文件损坏 JSON | host-agent 启动失败,stderr 提示 | 不静默重新生成 | | 8 | Token 文件不可写 | host-agent 启动失败 | 同上 | -| 9 | MCP session 异常断开 | lease 进入 TTL 倒计时 | 60s 后 lazy sweep | +| 9 | MCP session 异常断开 | lease 进入 TTL 倒计时 | 20s 后 lazy sweep | | 10 | TTL 过期瞬间 Hermes 重连 | renew 容忍边界:session_id 匹配 → 重新 acquire 而非报错 | 无感 | | 11 | 同 session 并发不同设备 | 各自独立 acquire | per-device 设计 | | 12 | 同 session 并发同一设备 | 第一个 acquire;第二个 renew(同 session_id) | 并发 safe | -| 13 | FastMCP 提取不到 session_id | JSON-RPC `-32001` + `"cannot determine MCP session"` | ERROR + exc_info | +| 13 | FastMCP 提取不到 session_id | `CallToolResult(isError=true, content=[TextContent("cannot determine MCP session")])` | ERROR + exc_info | | 14 | host-agent 关停时有 active MCP session | lease 随进程退出消失 | 不需显式清理 | ### 错误返回格式 @@ -292,17 +304,32 @@ run_async() 主循环启动(行为不变): { "jsonrpc": "2.0", "id": "", - "error": { - "code": -32000, - "message": "device phone-1 is busy (held by cloud assignment)", - "data": { - "device_id": "phone-1", - "busy_owner": "cloud_assignment" - } + "result": { + "content": [ + { + "type": "text", + "text": "device phone-1 is busy (held by cloud assignment)" + } + ], + "isError": true } } ``` +**Implementation note (2026-07-21 fix wave):** mcp SDK 1.28.1's +`Tool.run` wraps every non-`UrlElicitationRequiredError` exception +(including `McpError` with a typed `ErrorData`) into `ToolError`. +The lowlevel `call_tool` handler then serializes any exception as +`CallToolResult(isError=true, content=[TextContent(message)])` via +`_make_error_result`. There is no public path that surfaces JSON-RPC +`-32000` with a structured `data.busy_owner` field from a tool call +site — the SDK's wire contract for tool errors is the `isError=true` +flag plus text content. The busy-owner value lives in the text +content (truncated session_id for `mcp_session:` collisions, full +string for `cloud_assignment`). Unknown-device and other semantic +errors are returned as normal `ok=False` payloads inside a successful +`CallToolResult(isError=false)` (see `api/mcp.py::call_with_semantic_errors`). + ## 8. Testing Strategy ### 8.1 单元测试 @@ -376,13 +403,13 @@ run_async() 主循环启动(行为不变): - **Q2**:`mcp-token` CLI 是否需要鉴权?→ MVP 不鉴权,假定能访问宿主机的操作者可信(与 `setup` 子命令同款)。后续可加 `--password` 校验 local_account。 - **Q3**:MCP 工具调用是否需要 wall-clock 超时?→ MVP 不加,依赖底层超时;如有"Hermes 调用挂死"报告再加。 - **Q4**:心跳扩展是 Alembic migration 还是仅 schema 字段?→ MVP 仅 transient 字段(heartbeat 接收 → scheduler 用 → 丢弃),无需 migration。 -- **Q5(待实测)**:60s TTL 是否合适?→ 实测后调整。太短容易误释放正常 session 的锁;太长 Hermes 崩溃后设备不可用窗口大。 +- **Q5(已答,2026-07-21 fix wave)**:TTL 从 60s 降到 20s,因为 mcp SDK 1.28.1 没有 per-session shutdown callback(见 §6.5)。20s 仍能容许正常 operator 暂停,但能在一次 30s 心跳窗口内回收崩盘 session 的锁;新 `test_default_ttl_is_20_seconds` 和 `test_default_ttl_recovers_dead_session_within_one_window` 锁定该值。如有"Hermes 长操作横跨 20s 静默"报告再调高。 ## 11. Risks - **R1**:Cloud 心跳窗口期(30s)冲突。缓解:AssignmentExecutor 启动前 fail-fast。残留:cloud 可能基于过期心跳派任务、host-agent fail、cloud 重试——浪费 attempt 配额。**接受**。 - **R2**:Hermes 长时间占用设备导致 cloud 任务反复 fail。MVP 无自动缓解;用户手动管控;未来启用 `wait_until_usable`。**接受**。 -- **R3**:Hermes 崩溃后 60s 内设备不可用。TTL 兜底但窗口存在。**接受,记入 Q5**。 +- **R3**:Hermes 崩溃后 ≤20s 设备不可用(TTL 兜底,但窗口存在)。窗口仍 ≤30s 心跳间隔,cloud 侧下次心跳能学到。**接受,记入 Q5**。 - **R4(已消除)**:`tool_handlers` 签名改动影响面。Grep 确认 0 调用方依赖 None 默认。 - **R5**:FastMCP `session_id` 提取依赖 `mcp` SDK 内部 API。缓解:e2e 测试覆盖;SDK 升级 CI 能及时暴露。 - **R6**:`mcp` SDK 需作为 device-host-agent 直接依赖(目前通过 Runtime 传递)。需加进 `apps/device-host-agent/pyproject.toml`(与 `filelock` 直接化先例一致)。