fix(ingest): narrow mcp_timeout to our OWN deadline, not the exception type (kø-z follow-up)
Advisor review of the prior commit (5269b7d) found the except TimeoutError
branch was wider than the brief asked for: builtin TimeoutError is also
socket.timeout (3.10+) and asyncio.TimeoutError (3.11+), so any TimeoutError
reaching that clause got relabeled mcp_timeout regardless of source. Gate on
anyio.CancelScope.cancelled_caught instead, mirroring _unwrap_ingest_error's
ownership rule (own it, wrap it; otherwise, untouched).
Measured: no live trigger exists today (MCP's own internal read-timeout
converts to McpError before reaching us; a server-side TimeoutError becomes
an ordinary isError result) -- pinned with a synthetic test raising from
StdioServerParameters construction, inside our fail_after scope but before
either nested task group, so it arrives ungrouped. Four mutations red against
the full 625-test suite: drop the translation, revert to asyncio.wait_for,
relabel the code, and drop the cancelled_caught gate.
Also promotes anyio to a declared direct dependency (was transitive via mcp
only) -- ingest_mcp.py now imports it directly.
This commit is contained in:
parent
5269b7ddd5
commit
8d1d29edf0
4 changed files with 64 additions and 10 deletions
|
|
@ -125,11 +125,14 @@ def test_tool_timeout_reaches_the_caller_as_ingest_error(
|
|||
) -> None:
|
||||
"""A tool that outlives ``timeout_seconds`` must surface as a typed ``IngestError``.
|
||||
|
||||
``asyncio.wait_for`` wraps ``run()`` — itself two nested anyio task groups
|
||||
(``stdio_client``, ``ClientSession``). Only a REAL hanging server proves what the timeout
|
||||
cancellation actually surfaces as: a canned-tool test never enters a task group and cannot
|
||||
observe this at all (the same reason (x)'s ``mcp_tool_error`` unwrap needed a real subprocess).
|
||||
RED until the timeout is caught and re-raised as an owned ``IngestError``.
|
||||
MEASURED (2026-08-03), not assumed: wrapping ``run()`` — itself two nested anyio task groups
|
||||
(``stdio_client``, ``ClientSession``) — in ``asyncio.wait_for`` from OUTSIDE anyio's own
|
||||
structure does not raise ``TimeoutError`` at all here. anyio's cancellation and asyncio's do
|
||||
not compose across that boundary; the actual failure was an ``anyio.BrokenResourceError``
|
||||
inside a ``BaseExceptionGroup`` (a background reader losing its write end mid-teardown). Only
|
||||
a REAL hanging server proves this: a canned-tool test never enters a task group and cannot
|
||||
observe it. RED until the deadline moves to ``anyio.fail_after``, nested INSIDE both task
|
||||
groups, where anyio tears its own structure down cleanly and raises a plain ``TimeoutError``.
|
||||
"""
|
||||
script = _write_server(
|
||||
tmp_path,
|
||||
|
|
@ -142,6 +145,37 @@ def test_tool_timeout_reaches_the_caller_as_ingest_error(
|
|||
assert excinfo.value.code == "mcp_timeout"
|
||||
|
||||
|
||||
def test_unrelated_timeout_error_inside_the_scope_is_not_mislabeled(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
"""A ``TimeoutError`` NOT caused by our own deadline firing must reach the caller untouched.
|
||||
|
||||
Mirrors ``_unwrap_ingest_error``'s ownership rule: own it, wrap it; otherwise, untouched.
|
||||
Raised from ``StdioServerParameters`` construction — inside our ``anyio.fail_after`` scope,
|
||||
but BEFORE either nested task group (``stdio_client``, ``ClientSession``) is entered, so it
|
||||
reaches our ``except`` clause as a bare ``TimeoutError``, not grouped (that grouping is a
|
||||
separate, already-covered property — see the ``mcp_tool_error`` test above). ``timeout_seconds``
|
||||
is generous (30s) and nothing here waits on it, so the scope never actually cancels: this proves
|
||||
the label tracks the SCOPE's ``cancelled_caught``, not the exception TYPE.
|
||||
|
||||
No live trigger for this exists today, checked rather than assumed: a server-side
|
||||
``TimeoutError`` becomes an ordinary ``isError`` result (same as any other tool exception,
|
||||
verified against a real server), and MCP's own internal per-request read-timeout
|
||||
(``ClientSession.send_request``) converts its ``anyio.fail_after`` timeout to ``McpError``
|
||||
before it ever reaches us. This test pins the discriminator against that defect class
|
||||
directly, since the class has no reachable path to exercise it end-to-end.
|
||||
"""
|
||||
|
||||
def boom(*args: object, **kwargs: object) -> None:
|
||||
raise TimeoutError("unrelated — not our deadline")
|
||||
|
||||
monkeypatch.setattr("mcp.StdioServerParameters", boom)
|
||||
get = _transport(GOLDEN / "server.py", monkeypatch, timeout_seconds=30.0)
|
||||
|
||||
with pytest.raises(TimeoutError, match="unrelated"):
|
||||
get(f"mcp+stdio://{_SERVER_REF}/cost_docs", None)
|
||||
|
||||
|
||||
def test_missing_server_ref_fails_before_any_process_is_spawned(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue