Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions openjiuwen/core/foundation/tool/mcp/client/mcp_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ def __init__(self, config: McpServerConfig):
super().__init__()
self._server_path = config.server_path
self._include_image_content = bool(getattr(config, "include_image_content", False))
self._last_connect_error: Optional[BaseException] = None

@abstractmethod
async def connect(self, *, retry_times: int = 1, timeout: float = NO_TIMEOUT) -> bool:
Expand Down
3 changes: 1 addition & 2 deletions openjiuwen/core/foundation/tool/mcp/client/sse_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -157,8 +157,7 @@ async def _do_connect(self, *, timeout: float) -> bool:
except Exception as e:
logger.error("[SseClient] SSE connection failed to %s: %s: %r", self._server_path,
type(e).__name__, e)
# Clean up whatever partial state we have, but don't let cleanup
# exceptions mask the original connection error.
self._last_connect_error = e
try:
await self._do_disconnect(timeout=NO_TIMEOUT)
except Exception as cleanup_exc:
Expand Down
1 change: 1 addition & 0 deletions openjiuwen/core/foundation/tool/mcp/client/stdio_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ async def connect(self, *, timeout: float = NO_TIMEOUT) -> bool:
return True
except Exception as e:
logger.error(f"Stdio connection failed: {e}")
self._last_connect_error = e
await self.disconnect()
return False

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
# coding: utf-8
# Copyright (c) Huawei Technologies Co., Ltd. 2025. All rights reserved.
import asyncio
from contextlib import AsyncExitStack
from typing import Any, Dict, List, Optional

Expand Down Expand Up @@ -113,9 +114,32 @@ async def connect(self, *, timeout: float = NO_TIMEOUT) -> bool:
self._is_disconnected = False
logger.info(f"Streamable-http client connected successfully to {self._server_path}")
return True
except Exception as e:
except BaseException as e:
# Extract HTTPStatusError from BaseExceptionGroup (caused by GeneratorExit
# being BaseException) so real HTTP errors aren't misreported as cancelled.
# CancelledError/KeyboardInterrupt propagate.
if isinstance(e, (KeyboardInterrupt, asyncio.CancelledError)) \
and not isinstance(e, BaseExceptionGroup):
raise
logger.error(f"Streamable-http connection failed to {self._server_path}: {e}")
await self.disconnect()
real_err: BaseException = e
if isinstance(e, BaseExceptionGroup):
exc_sub, _ = e.split(Exception)
if exc_sub is not None:
subs = list(exc_sub.exceptions)
real_err = subs[0] if len(subs) == 1 else exc_sub
else:
real_err = RuntimeError(f"MCP server '{self._name}' connect failed: {e}")
if isinstance(real_err, Exception):
self._last_connect_error = real_err
else:
self._last_connect_error = RuntimeError(
f"MCP server '{self._name}' connect failed: {type(e).__name__}: {e}"
)
try:
await self.disconnect()
except BaseException as disc_exc: # noqa: BLE001
logger.debug("[StreamableHttpClient] disconnect after connect failure failed: %r", disc_exc)
return False

async def disconnect(self, *, timeout: float = NO_TIMEOUT) -> bool:
Expand Down
10 changes: 10 additions & 0 deletions openjiuwen/core/runner/resources_manager/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,16 @@ def error(self) -> E:
"""
return self._error

@property
def error_message(self) -> E:
"""Property alias for the error value (mirrors ``msg()``/``error()``).

Downstream callers commonly probe a Result via ``getattr(result,
"error_message", None)``; expose the value through that name so the
underlying error is reachable without calling a method.
"""
return self._error


Result: TypeAlias = Ok[T] | Error[E]
"""
Expand Down
58 changes: 54 additions & 4 deletions openjiuwen/core/runner/resources_manager/tool_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

from openjiuwen.core.common.clients.client_registry import get_client_registry
from openjiuwen.core.common.exception.codes import StatusCode
from openjiuwen.core.common.exception.errors import build_error
from openjiuwen.core.common.exception.errors import BaseError, build_error
from openjiuwen.core.common.logging import runner_logger as logger
from openjiuwen.core.foundation.tool import (
McpClient,
Expand Down Expand Up @@ -120,18 +120,68 @@ async def add_tool_server(
cards.append(deepcopy(tool.card))
return cards
client = self._create_client(server_config)
connected = False
try:
connected = await client.connect()
if not connected:
last_err = getattr(client, "_last_connect_error", None)
reason = f"{type(last_err).__name__}: {last_err}" if last_err else ""
raise build_error(
StatusCode.RESOURCE_MCP_SERVER_CONNECTION_ERROR, server_config=server_config, reason=""
StatusCode.RESOURCE_MCP_SERVER_CONNECTION_ERROR,
server_config=server_config, reason=reason,
)
results = await self._inner_refresh_mcp_tools(client, server_config, expiry_time)
self._mcp_server_name_to_ids.setdefault(server_config.server_name, []).append(server_config.server_id)
return results
except Exception as e:
except BaseException as e:
# ``connect``/``list_tools`` may raise a bare ``CancelledError`` or a
# ``BaseExceptionGroup`` (anyio task-group teardown, closing the
# generator-based client). Both are ``BaseException`` subclasses that
# escape ``except Exception``; coerce them into a ``BaseError`` so
# ``add_mcp_server`` wraps it in an ``Error`` and the real reason
# reaches the frontend instead of a silent "task cancelled".
if isinstance(e, KeyboardInterrupt):
raise
# ``connect()`` False path already raised CONNECTION_ERROR above;
# let it through to keep its status/reason.
if isinstance(e, BaseError) and e.status is StatusCode.RESOURCE_MCP_SERVER_CONNECTION_ERROR:
raise
if connected:
try:
await client.disconnect()
except BaseException as disconnect_exc: # noqa: BLE001
logger.warning(
"add_tool_server cleanup failed: %s, server_id=%s",
disconnect_exc, server_config.server_id,
)
if isinstance(e, Exception) and not isinstance(e, asyncio.CancelledError):
cause = e
elif isinstance(e, asyncio.CancelledError):
cause = RuntimeError(
f"MCP server '{server_config.server_name}' connect/register cancelled "
f"(anyio task-group teardown): {e}"
)
elif isinstance(e, BaseExceptionGroup):
exc_subgroup, base_subgroup = e.split(Exception)
if exc_subgroup is not None and base_subgroup is None:
subs = list(exc_subgroup.exceptions)
cause = subs[0] if len(subs) == 1 else exc_subgroup
elif base_subgroup is not None:
cause = RuntimeError(
f"MCP server '{server_config.server_name}' register failed: {base_subgroup}"
)
else:
cause = RuntimeError(
f"MCP server '{server_config.server_name}' register failed: {e}"
)
else:
cause = RuntimeError(
f"MCP server '{server_config.server_name}' register failed: "
f"{type(e).__name__}: {e}"
)
raise build_error(
StatusCode.RESOURCE_MCP_SERVER_ADD_ERROR, cause=e, server_config=server_config, reason=str(e)
StatusCode.RESOURCE_MCP_SERVER_ADD_ERROR, cause=cause, server_config=server_config,
reason=str(cause),
) from e

@staticmethod
Expand Down
87 changes: 87 additions & 0 deletions tests/unit_tests/core/runner/test_tool_manager_mcp_dedup.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@

import pytest

from openjiuwen.core.common.exception.codes import StatusCode
from openjiuwen.core.common.exception.errors import BaseError
from openjiuwen.core.foundation.tool import McpServerConfig, McpToolCard
from openjiuwen.core.runner.resources_manager.tool_manager import ToolMgr

Expand Down Expand Up @@ -132,3 +134,88 @@ def fake_create(config: McpServerConfig) -> MagicMock:
assert [c.name for c in cards_b] == ["y"]
assert fake_client_a.connect.await_count == 1
assert fake_client_b.connect.await_count == 1


@pytest.mark.asyncio
async def test_add_tool_server_surfaces_connect_false_reason_as_connection_error() -> None:
"""connect() returning False must raise CONNECTION_ERROR carrying the real
underlying exception in reason, and must NOT be re-wrapped into ADD_ERROR
by the BaseException handler below."""
mgr = ToolMgr()
cfg = _make_server_config(server_id="false-srv")

fake_client = MagicMock()
fake_client.connect = AsyncMock(return_value=False)
fake_client._last_connect_error = TimeoutError("handshake timed out")
fake_client.disconnect = AsyncMock(return_value=True)

with patch.object(ToolMgr, "_create_client", staticmethod(lambda c: fake_client)):
with pytest.raises(BaseError) as exc_info:
await mgr.add_tool_server(cfg)

err = exc_info.value
# Status preserved as CONNECTION_ERROR, not re-wrapped to ADD_ERROR.
assert err.status == StatusCode.RESOURCE_MCP_SERVER_CONNECTION_ERROR
# Real exception surfaced in reason, not a bare or nested-wrapped message.
assert "TimeoutError" in err.message
assert "handshake timed out" in err.message


@pytest.mark.asyncio
async def test_add_tool_server_coerces_cancelled_error_into_base_error() -> None:
"""A bare ``CancelledError`` from connect (anyio task-group teardown) must be
coerced into a ``BaseError`` so ``add_mcp_server`` can wrap it in an ``Error``
result and the frontend sees a connect failure instead of a silent cancel."""
mgr = ToolMgr()
cfg = _make_server_config(server_id="cancel-srv")

fake_client = MagicMock()
fake_client.connect = AsyncMock(side_effect=asyncio.CancelledError("teardown"))
fake_client.disconnect = AsyncMock(return_value=True)

with patch.object(ToolMgr, "_create_client", staticmethod(lambda c: fake_client)):
with pytest.raises(BaseError) as exc_info:
await mgr.add_tool_server(cfg)

err = exc_info.value
assert err.status == StatusCode.RESOURCE_MCP_SERVER_ADD_ERROR
# The original CancelledError is preserved on __cause__ for diagnosis.
assert isinstance(err.__cause__, asyncio.CancelledError)
# The surfaced reason must mention the server, not read as a bare "cancelled".
assert "cancel-srv" in err.message
# connect() never returned True, so disconnect must not be attempted.
assert fake_client.disconnect.await_count == 0


@pytest.mark.asyncio
async def test_add_tool_server_coerces_exception_group_into_base_error() -> None:
"""A ``BaseExceptionGroup`` mixing a regular ``Exception`` with a
``GeneratorExit`` (the anyio task-group teardown shape) must be coerced into
a ``BaseError`` carrying a readable reason instead of escaping as a bare
cancel/silent teardown."""
mgr = ToolMgr()
cfg = _make_server_config(server_id="egroup-srv")

inner = RuntimeError("boom from list_tools")
# Mix a non-Exception BaseException (GeneratorExit) so the group stays a
# real BaseExceptionGroup rather than collapsing to ExceptionGroup.
fake_client = MagicMock()
fake_client.connect = AsyncMock(return_value=True)
fake_client.list_tools = AsyncMock(
side_effect=BaseExceptionGroup("group", [inner, GeneratorExit()])
)
fake_client.disconnect = AsyncMock(return_value=True)

with patch.object(ToolMgr, "_create_client", staticmethod(lambda c: fake_client)):
with pytest.raises(BaseError) as exc_info:
await mgr.add_tool_server(cfg)

err = exc_info.value
assert err.status == StatusCode.RESOURCE_MCP_SERVER_ADD_ERROR
# The mixed group is wrapped into a RuntimeError cause (the non-Exception
# sub-group cannot be re-raised as a plain Exception), surfacing a readable
# reason that names the server.
assert isinstance(err.cause, RuntimeError)
assert "demo" in str(err.cause) # server_name
# connect() returned True, so disconnect cleanup must run on the failure path.
assert fake_client.disconnect.await_count == 1