diff --git a/redisvl/mcp/config.py b/redisvl/mcp/config.py index 244e0298..562bbb96 100644 --- a/redisvl/mcp/config.py +++ b/redisvl/mcp/config.py @@ -57,6 +57,32 @@ def builtin_tool_names() -> frozenset[str]: return _BUILTIN_TOOL_NAMES +def validate_tool_name(name: str, *, kind: str) -> None: + """Raise ``ValueError`` unless ``name`` is usable for a custom tool. + + Shared by every kind of custom tool so that one set of rules decides what a + tool may be called. ``kind`` names the source in the message, so an error + points at the config section or decorator that produced the name. + """ + # fullmatch, because `$` also matches before a trailing newline. + if not _TOOL_NAME_PATTERN.fullmatch(name): + raise ValueError( + f"{kind} name '{name}' is invalid; names must start with a " + "lowercase letter and contain only lowercase letters, digits, " + "hyphens, or underscores (max 64 characters)" + ) + if name in builtin_tool_names(): + raise ValueError( + f"{kind} name '{name}' collides with a built-in tool; " + "built-in names are reserved" + ) + if name.startswith(_RESERVED_TOOL_NAME_PREFIXES): + raise ValueError( + f"{kind} name '{name}' uses a reserved prefix " + f"({', '.join(_RESERVED_TOOL_NAME_PREFIXES)})" + ) + + class MCPRuntimeConfig(BaseModel): """Runtime limits and validated field mappings for MCP requests.""" @@ -765,22 +791,7 @@ def _validate_profile(self) -> "MCPCustomToolConfig": raise ValueError( f"custom_tools '{self.name}' description must not be blank" ) - if not _TOOL_NAME_PATTERN.match(self.name): - raise ValueError( - f"custom_tools name '{self.name}' is invalid; names must start with a " - "lowercase letter and contain only lowercase letters, digits, " - "hyphens, or underscores (max 64 characters)" - ) - if self.name in builtin_tool_names(): - raise ValueError( - f"custom_tools name '{self.name}' collides with a built-in tool; " - "built-in names are reserved" - ) - if self.name.startswith(_RESERVED_TOOL_NAME_PREFIXES): - raise ValueError( - f"custom_tools name '{self.name}' uses a reserved prefix " - f"({', '.join(_RESERVED_TOOL_NAME_PREFIXES)})" - ) + validate_tool_name(self.name, kind="custom_tools") unknown_params = sorted(set(self.params) - _PROFILE_PARAM_NAMES) if unknown_params: diff --git a/redisvl/mcp/server.py b/redisvl/mcp/server.py index 69c83abc..ab550318 100644 --- a/redisvl/mcp/server.py +++ b/redisvl/mcp/server.py @@ -266,7 +266,15 @@ async def run_guarded( self._close_awaitable(awaitable) raise RuntimeError("MCP server is not running") - async with semaphore: + try: + await semaphore.acquire() + except BaseException: + # Cancelled while waiting for a slot, as by a code tool's whole-call + # timeout, so the awaitable never started. + self._close_awaitable(awaitable) + raise + + try: async with self._request_state_lock: if self._lifecycle_state is not _LifecycleState.RUNNING: self._close_awaitable(awaitable) @@ -286,6 +294,8 @@ async def run_guarded( self._active_requests -= 1 if self._active_requests == 0: self._active_requests_drained.set() + finally: + semaphore.release() @staticmethod def _build_vectorizer(binding: MCPIndexBindingConfig) -> Any: diff --git a/redisvl/mcp/tools/code.py b/redisvl/mcp/tools/code.py new file mode 100644 index 00000000..2c3bba83 --- /dev/null +++ b/redisvl/mcp/tools/code.py @@ -0,0 +1,230 @@ +"""Code tools: async Python functions registered as MCP tools. + +A code tool is an async function whose first parameter is a +:class:`~redisvl.mcp.context.ToolContext` and whose remaining, annotated +parameters are the arguments the model supplies. Its docstring (or an explicit +description) is what the model reads when choosing a tool. + +Everything that makes a built-in safe is applied around the function rather +than left to its author: the read or write scope and any per-tool scope are +checked before it runs, the context it receives can only reach Redis through a +guarded, allow-listed handle, the whole call is bounded by the binding's +request timeout, and any exception it raises is mapped to the stable MCP error +contract instead of leaking a traceback. +""" + +import asyncio +import inspect +import typing +from collections.abc import Callable +from dataclasses import dataclass +from typing import Any + +from pydantic import TypeAdapter + +from redisvl.mcp.auth import ensure_read_scope, ensure_tool_scope, ensure_write_scope +from redisvl.mcp.config import validate_tool_name +from redisvl.mcp.context import ToolContext +from redisvl.mcp.errors import MCPErrorCode, RedisVLMCPError, map_exception + +_ACCEPTED_KINDS = ( + inspect.Parameter.POSITIONAL_OR_KEYWORD, + inspect.Parameter.KEYWORD_ONLY, +) + + +@dataclass(frozen=True) +class CodeToolSpec: + """One validated code tool, ready to register against a binding.""" + + fn: Callable[..., Any] + name: str + description: str + index: str | None + writes: bool + scope: str | None + signature: inspect.Signature + annotations: dict[str, Any] + + +def _qualified(fn: Callable[..., Any]) -> str: + return f"{getattr(fn, '__module__', '?')}.{getattr(fn, '__qualname__', fn)}" + + +def _tool_signature( + fn: Callable[..., Any], +) -> tuple[inspect.Signature, dict[str, Any]]: + """Return the model-facing signature: the function's, minus the context.""" + where = _qualified(fn) + try: + # Resolved rather than read raw, so a module using postponed + # annotations is checked against the real types. + hints = typing.get_type_hints(fn, include_extras=True) + except Exception as exc: + raise TypeError( + f"code tool {where} has annotations that cannot be resolved: {exc}" + ) from exc + + parameters = list(inspect.signature(fn).parameters.values()) + # Positional, because the wrapper passes the context positionally; a + # keyword-only one would be accepted here and fail on every call. + if ( + not parameters + or hints.get(parameters[0].name) is not ToolContext + or parameters[0].kind + not in ( + inspect.Parameter.POSITIONAL_ONLY, + inspect.Parameter.POSITIONAL_OR_KEYWORD, + ) + ): + raise TypeError( + f"code tool {where} must take a ToolContext as its first, positional " + "parameter" + ) + + exposed = [] + annotations: dict[str, Any] = {} + for parameter in parameters[1:]: + if parameter.kind not in _ACCEPTED_KINDS: + raise TypeError( + f"code tool {where} parameter '{parameter.name}' must be a plain " + "named parameter; *args, **kwargs and positional-only " + "parameters cannot be offered to a model" + ) + if parameter.name not in hints: + raise TypeError( + f"code tool {where} parameter '{parameter.name}' needs a type " + "annotation; it is what the tool's input schema is built from" + ) + annotation = hints[parameter.name] + try: + # Built here so a type the input schema cannot express fails at + # declaration, not at startup after every binding has connected. + TypeAdapter(annotation).json_schema() + except Exception as exc: + raise TypeError( + f"code tool {where} parameter '{parameter.name}' has a type " + f"that no input schema can describe: {exc}" + ) from exc + # Keyword-only, because the wrapper is always called with keyword + # arguments decoded from the model's JSON. + exposed.append( + parameter.replace( + kind=inspect.Parameter.KEYWORD_ONLY, annotation=annotation + ) + ) + annotations[parameter.name] = annotation + + return_annotation = hints.get("return", inspect.Signature.empty) + if return_annotation is not inspect.Signature.empty: + annotations["return"] = return_annotation + return ( + inspect.Signature(exposed, return_annotation=return_annotation), + annotations, + ) + + +def build_code_tool_spec( + fn: Callable[..., Any], + *, + name: str | None = None, + namespace: str | None = None, + index: str | None = None, + writes: bool = False, + scope: str | None = None, + description: str | None = None, +) -> CodeToolSpec: + """Validate ``fn`` as a code tool and return its spec. + + Everything checkable without a running server is checked here, so a + malformed tool fails where it is declared rather than at startup. + Whether ``index`` names a real binding, and whether a writing tool is + allowed on it, depends on the server's config and is checked at startup. + """ + where = _qualified(fn) + if not inspect.iscoroutinefunction(fn): + raise TypeError(f"code tool {where} must be an async function") + + signature, annotations = _tool_signature(fn) + + resolved_description = ( + description if description is not None else inspect.getdoc(fn) + ) + if not resolved_description or not resolved_description.strip(): + raise ValueError( + f"code tool {where} needs a description: give it a docstring or pass " + "description=; it is what the model chooses tools by" + ) + + resolved_name = name if name is not None else fn.__name__.replace("_", "-") + if namespace is not None: + resolved_name = f"{namespace}-{resolved_name}" + validate_tool_name(resolved_name, kind=f"code tool {where}") + + if scope is not None and not scope.strip(): + raise ValueError(f"code tool {where} scope must not be blank") + if scope is not None and scope != scope.strip(): + # Scopes are matched exactly, so a padded one would refuse every caller. + raise ValueError( + f"code tool {where} scope {scope!r} has surrounding whitespace" + ) + + return CodeToolSpec( + fn=fn, + name=resolved_name, + description=resolved_description.strip(), + index=index, + writes=writes, + scope=scope, + signature=signature, + annotations=annotations, + ) + + +def register_code_tool(server: Any, spec: CodeToolSpec, binding_id: str) -> None: + """Register one code tool against ``binding_id``.""" + + async def code_tool(**kwargs: Any) -> Any: + # Before anything else, so a caller without the scope never reaches the + # binding or the function. The tool's own scope is required on top of + # the server-wide one, so it can only make a tool stricter. A writing + # tool needs the read scope too, because its handle can also read. + ensure_read_scope(server) + if spec.writes: + ensure_write_scope(server) + ensure_tool_scope(server, spec.scope) + + try: + runtime = server.resolve_binding(binding_id) + if spec.writes and runtime.effective_read_only: + # Startup already refuses this pairing; checked again per call, + # as upsert_records does, so a write never depends on that + # check alone. + raise RedisVLMCPError( + f"index '{binding_id}' is read-only", + code=MCPErrorCode.FORBIDDEN, + retryable=False, + ) + ctx = ToolContext.for_request(server, runtime, writes=spec.writes) + timeout = runtime.binding.runtime.request_timeout_seconds + try: + # The handle bounds each Redis call; this bounds the tool's own + # code too, so a tool cannot hold a request open indefinitely. + return await asyncio.wait_for(spec.fn(ctx, **kwargs), timeout) + except asyncio.TimeoutError as exc: + raise RedisVLMCPError( + f"tool '{spec.name}' did not finish within {timeout:g}s", + code=MCPErrorCode.BACKEND_UNAVAILABLE, + retryable=True, + ) from exc + except RedisVLMCPError: + raise + except Exception as exc: + raise map_exception(exc) from exc + + code_tool.__name__ = spec.name.replace("-", "_") + code_tool.__doc__ = spec.description + code_tool.__signature__ = spec.signature # type: ignore[attr-defined] + code_tool.__annotations__ = dict(spec.annotations) + + server.tool(name=spec.name, description=spec.description)(code_tool) diff --git a/tests/unit/test_mcp/test_code_tools_unit.py b/tests/unit/test_mcp/test_code_tools_unit.py new file mode 100644 index 00000000..6cbd01aa --- /dev/null +++ b/tests/unit/test_mcp/test_code_tools_unit.py @@ -0,0 +1,493 @@ +"""Code tools: what a valid one looks like, and what wraps every call.""" + +import asyncio +import inspect +from dataclasses import replace +from types import SimpleNamespace +from typing import Any + +import pytest +from conftest import _schema + +fastmcp = pytest.importorskip( + "fastmcp", reason="fastmcp not installed (install redisvl[mcp])" +) + +from redisvl.mcp.config import MCPConfig +from redisvl.mcp.context import IndexHandle, ToolContext, WritableIndexHandle +from redisvl.mcp.errors import MCPErrorCode, RedisVLMCPError +from redisvl.mcp.runtime import BindingRuntime +from redisvl.mcp.server import RedisVLMCPServer +from redisvl.mcp.settings import MCPSettings +from redisvl.mcp.tools.code import build_code_tool_spec, register_code_tool + + +async def recent_tickets(ctx: ToolContext, query: str, limit: int = 5) -> dict: + """Find recent support tickets matching a query.""" + return {"query": query, "limit": limit} + + +class FakeIndex: + async def exists(self): + return True + + +class FakeServer: + def __init__(self, *, timeout: float = 30.0, read_only: bool = False): + self.config = MCPConfig.model_validate( + { + "server": {"redis_url": "redis://localhost:6379"}, + "indexes": { + "tickets": { + "redis_name": "tickets-index", + "search": {"type": "fulltext"}, + "runtime": {"text_field_name": "content"}, + } + }, + } + ) + # Copied rather than validated in, so a test can use a sub-second + # timeout the config model (whole seconds) would refuse. + binding = self.config.indexes["tickets"] + self.binding = binding.model_copy( + update={ + "runtime": binding.runtime.model_copy( + update={"request_timeout_seconds": timeout} + ) + } + ) + self.read_only = read_only + self.resolved: list[str | None] = [] + self.registered: dict[str, Any] = {} + self.auth_config = SimpleNamespace(read_scope="kb.read", write_scope="kb.write") + self._auth_enabled = False + + def resolve_binding(self, index_id=None): + self.resolved.append(index_id) + return BindingRuntime( + binding_id="tickets", + binding=self.binding, + index=FakeIndex(), + schema=_schema(), + vectorizer=None, + supports_native_hybrid_search=False, + effective_read_only=self.read_only, + ) + + async def run_guarded(self, operation_name, awaitable, *, timeout_seconds): + return await awaitable + + def tool(self, name=None, description=None, **kwargs): + def decorator(fn): + self.registered[name] = fn + return fn + + return decorator + + +def _registered(fn, server=None, **options): + server = server or FakeServer() + spec = build_code_tool_spec(fn, **options) + register_code_tool(server, spec, "tickets") + return server.registered[spec.name] + + +def _as_caller(monkeypatch, scopes=None): + token = None if scopes is None else SimpleNamespace(scopes=scopes, claims={}) + monkeypatch.setattr( + "fastmcp.server.dependencies.get_access_token", lambda: token, raising=False + ) + + +# -------------------------------------------------------------------------- +# What counts as a code tool +# -------------------------------------------------------------------------- + + +def test_a_sync_function_is_refused(): + def sync_tool(ctx: ToolContext, query: str) -> dict: + """Search.""" + return {} + + with pytest.raises(TypeError, match="must be an async function"): + build_code_tool_spec(sync_tool) + + +async def _no_context(query: str) -> dict: + """Search.""" + return {} + + +async def _untyped_context(ctx, query: str) -> dict: + """Search.""" + return {} + + +async def _keyword_only_context(*, ctx: ToolContext, query: str) -> dict: + """Search.""" + return {} + + +async def _unannotated(ctx: ToolContext, query) -> dict: + """Search.""" + return {} + + +async def _varargs(ctx: ToolContext, *queries: str) -> dict: + """Search.""" + return {} + + +async def _varkw(ctx: ToolContext, **options: str) -> dict: + """Search.""" + return {} + + +async def _positional_only(ctx: ToolContext, query: str, /) -> dict: + """Search.""" + return {} + + +async def _unresolvable(ctx: ToolContext, query: "NoSuchType") -> dict: # noqa: F821 + """Search.""" + return {} + + +class _NoSchema: + pass + + +async def _no_schema(ctx: ToolContext, query: _NoSchema) -> dict: + """Search.""" + return {} + + +@pytest.mark.parametrize( + "fn, message", + [ + pytest.param(_no_context, "ToolContext as its first", id="no-context"), + pytest.param(_untyped_context, "ToolContext as its first", id="untyped"), + pytest.param( + _keyword_only_context, "first, positional parameter", id="kw-only-context" + ), + pytest.param(_unannotated, "'query' needs a type", id="unannotated"), + pytest.param(_varargs, "'queries' must be a plain", id="varargs"), + pytest.param(_varkw, "'options' must be a plain", id="varkw"), + pytest.param(_positional_only, "'query' must be a plain", id="pos-only"), + pytest.param(_unresolvable, "cannot be resolved", id="unresolvable"), + pytest.param(_no_schema, "no input schema can describe", id="no-schema"), + ], +) +def test_a_malformed_signature_is_refused_where_it_is_declared(fn, message): + with pytest.raises(TypeError, match=message): + build_code_tool_spec(fn) + + +def test_string_annotations_are_resolved_to_real_types(): + async def postponed(ctx: "ToolContext", query: "str") -> "dict": + """Search.""" + return {} + + spec = build_code_tool_spec(postponed) + + assert spec.annotations == {"query": str, "return": dict} + + +def test_a_tool_without_a_description_is_refused(): + async def undocumented(ctx: ToolContext, query: str) -> dict: + return {} + + with pytest.raises(ValueError, match="needs a description"): + build_code_tool_spec(undocumented) + with pytest.raises(ValueError, match="needs a description"): + build_code_tool_spec(recent_tickets, description=" ") + + +def test_the_description_is_the_cleaned_docstring_unless_given(): + async def documented(ctx: ToolContext, query: str) -> dict: + """ + Find tickets. + + Use for support questions. + """ + return {} + + assert ( + build_code_tool_spec(documented).description + == "Find tickets.\n\nUse for support questions." + ) + assert ( + build_code_tool_spec(documented, description="Search tickets.").description + == "Search tickets." + ) + + +@pytest.mark.parametrize( + "options, expected", + [ + ({}, "recent-tickets"), + ({"namespace": "support"}, "support-recent-tickets"), + ({"name": "tickets"}, "tickets"), + ({"name": "tickets", "namespace": "support"}, "support-tickets"), + ], +) +def test_the_name_comes_from_the_function_unless_given(options, expected): + assert build_code_tool_spec(recent_tickets, **options).name == expected + + +@pytest.mark.parametrize( + "options, message", + [ + ({"name": "Recent.Tickets"}, "is invalid"), + ({"name": "recent-tickets\n"}, "is invalid"), + # The namespace is applied before the name is checked. + ({"namespace": "redisvl"}, "uses a reserved prefix"), + ], +) +def test_a_name_the_profile_rules_refuse_is_refused_here_too(options, message): + with pytest.raises( + ValueError, match=f"(?s)code tool .*recent_tickets name .* {message}" + ): + build_code_tool_spec(recent_tickets, **options) + + +@pytest.mark.parametrize( + "scope, message", + [ + (" ", "scope must not be blank"), + # Scopes match exactly, so this one would refuse every caller. + (" tickets.read", "has surrounding whitespace"), + ], +) +def test_an_unusable_scope_is_refused(scope, message): + with pytest.raises(ValueError, match=message): + build_code_tool_spec(recent_tickets, scope=scope) + + +# -------------------------------------------------------------------------- +# What the model sees +# -------------------------------------------------------------------------- + + +async def test_the_model_sees_the_arguments_but_never_the_context(monkeypatch): + # A real FastMCP server and client, so the call below goes through FastMCP's + # own argument validation against the synthesised signature. + _as_caller(monkeypatch) + fake = FakeServer() + mcp = fastmcp.FastMCP("code-tools") + mcp.resolve_binding = fake.resolve_binding + mcp.run_guarded = fake.run_guarded + register_code_tool(mcp, build_code_tool_spec(recent_tickets), "tickets") + + async with fastmcp.Client(mcp) as client: + (tool,) = await client.list_tools() + called = await client.call_tool("recent-tickets", {"query": "refund"}) + refused = await client.call_tool( + "recent-tickets", {"query": "refund", "ctx": {}}, raise_on_error=False + ) + + assert tool.name == "recent-tickets" + assert tool.description == "Find recent support tickets matching a query." + assert set(tool.inputSchema["properties"]) == {"query", "limit"} + assert tool.inputSchema["required"] == ["query"] + assert called.data == {"query": "refund", "limit": 5} + assert refused.is_error + + +# -------------------------------------------------------------------------- +# What wraps every call +# -------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + "writes, handle_type", [(False, IndexHandle), (True, WritableIndexHandle)] +) +async def test_the_tool_runs_with_a_context_for_its_binding( + monkeypatch, writes, handle_type +): + _as_caller(monkeypatch) + seen: list[ToolContext] = [] + + async def inspect_context(ctx: ToolContext, query: str) -> dict: + """Search.""" + seen.append(ctx) + return {"exists": await ctx.index.exists()} + + server = FakeServer() + fn = _registered(inspect_context, server, writes=writes) + + assert await fn(query="refund") == {"exists": True} + assert server.resolved == ["tickets"] + assert type(seen[0].index) is handle_type + + +async def test_a_writing_tool_is_refused_on_an_index_now_read_only(monkeypatch): + # Startup refuses the pairing too; the call checks again on its own. + _as_caller(monkeypatch) + called: list[str] = [] + + async def write(ctx: ToolContext, ticket_id: str) -> dict: + """Close a ticket.""" + called.append(ticket_id) + return {} + + fn = _registered(write, FakeServer(read_only=True), writes=True) + + with pytest.raises(RedisVLMCPError) as exc_info: + await fn(ticket_id="t1") + + assert exc_info.value.code == MCPErrorCode.FORBIDDEN + assert called == [] + + +@pytest.mark.parametrize( + "writes, scopes", + [ + pytest.param(False, ["kb.write", "tickets.read"], id="read-without-read-scope"), + pytest.param(True, ["kb.read", "tickets.read"], id="write-without-write-scope"), + pytest.param(True, ["kb.write", "tickets.read"], id="write-without-read-scope"), + pytest.param(False, ["kb.read"], id="read-without-tool-scope"), + pytest.param(True, ["kb.read", "kb.write"], id="write-without-tool-scope"), + ], +) +async def test_a_token_missing_a_required_scope_never_reaches_the_tool( + monkeypatch, writes, scopes +): + called: list[str] = [] + + async def tool_fn(ctx: ToolContext, query: str) -> dict: + """Search.""" + called.append(query) + return {} + + server = FakeServer() + server._auth_enabled = True + _as_caller(monkeypatch, scopes) + fn = _registered(tool_fn, server, writes=writes, scope="tickets.read") + + with pytest.raises(RedisVLMCPError) as exc_info: + await fn(query="refund") + + assert exc_info.value.code == MCPErrorCode.FORBIDDEN + assert called == [] + + +@pytest.mark.parametrize( + "writes, scopes", + [ + (False, ["kb.read", "tickets.read"]), + (True, ["kb.read", "kb.write", "tickets.read"]), + ], +) +async def test_the_server_scope_and_the_tool_scope_together_admit_the_caller( + monkeypatch, writes, scopes +): + server = FakeServer() + server._auth_enabled = True + _as_caller(monkeypatch, scopes) + fn = _registered(recent_tickets, server, writes=writes, scope="tickets.read") + + assert await fn(query="refund") == {"query": "refund", "limit": 5} + + +async def test_a_tool_that_overruns_the_binding_timeout_is_cut_off(monkeypatch): + _as_caller(monkeypatch) + + async def slow(ctx: ToolContext, query: str) -> dict: + """Search slowly.""" + await asyncio.sleep(5) + return {} + + fn = _registered(slow, FakeServer(timeout=0.05)) + + with pytest.raises(RedisVLMCPError) as exc_info: + await fn(query="refund") + + assert exc_info.value.code == MCPErrorCode.BACKEND_UNAVAILABLE + assert exc_info.value.retryable is True + assert "tool 'slow' did not finish within 0.05s" in str(exc_info.value) + + +@pytest.mark.parametrize( + "raised, code", + [ + (ValueError("bad sku"), MCPErrorCode.INVALID_REQUEST), + (RuntimeError("boom"), MCPErrorCode.INTERNAL_ERROR), + ], +) +async def test_an_exception_from_the_tool_is_mapped(monkeypatch, raised, code): + _as_caller(monkeypatch) + + async def failing(ctx: ToolContext, query: str) -> dict: + """Search.""" + raise raised + + with pytest.raises(RedisVLMCPError) as exc_info: + await _registered(failing)(query="refund") + + assert exc_info.value.code == code + assert exc_info.value.__cause__ is raised + + +async def test_an_mcp_error_from_the_tool_passes_through_unchanged(monkeypatch): + _as_caller(monkeypatch) + error = RedisVLMCPError( + "sku not found", code=MCPErrorCode.INVALID_REQUEST, retryable=False + ) + + async def failing(ctx: ToolContext, query: str) -> dict: + """Search.""" + raise error + + with pytest.raises(RedisVLMCPError) as exc_info: + await _registered(failing)(query="refund") + + assert exc_info.value is error + + +@pytest.mark.parametrize("slot_free", [True, False], ids=["mid-call", "queued"]) +async def test_a_timed_out_call_gives_back_its_concurrency_slot(monkeypatch, slot_free): + # Against the real run_guarded: a tool cut off mid-call must release the + # semaphore and the active-request count, or the cap leaks one slot per + # timeout until the server stops answering. One cut off while queued for + # the slot must close the Redis call it never started. + _as_caller(monkeypatch) + started: list[Any] = [] + + class HangingIndex: + def exists(self): + started.append(asyncio.sleep(5)) + return started[-1] + + server = RedisVLMCPServer(MCPSettings(config="/tmp/mcp.yaml")) + server._semaphore = asyncio.Semaphore(1) + server._lifecycle_state = server._LifecycleState.RUNNING + server.config = FakeServer().config + fake = FakeServer(timeout=0.05) + runtime = fake.resolve_binding("tickets") + server._bindings = {"tickets": replace(runtime, index=HangingIndex())} + registered: dict[str, Any] = {} + monkeypatch.setattr( + server, + "tool", + lambda name=None, description=None: lambda fn: registered.setdefault(name, fn), + ) + + async def probe(ctx: ToolContext) -> dict: + """Probe the index.""" + return {"exists": await ctx.index.exists()} + + register_code_tool(server, build_code_tool_spec(probe), "tickets") + if not slot_free: + await server._semaphore.acquire() + + with pytest.raises(RedisVLMCPError): + # Bounded here too, so a lost whole-call timeout fails the test + # instead of leaving it queued forever behind the slot held above. + await asyncio.wait_for(registered["probe"](), 2) + + assert server._active_requests == 0 + assert inspect.getcoroutinestate(started[0]) == inspect.CORO_CLOSED + if not slot_free: + server._semaphore.release() + assert server._semaphore.locked() is False diff --git a/tests/unit/test_mcp/test_config.py b/tests/unit/test_mcp/test_config.py index a16267f9..4f76c77a 100644 --- a/tests/unit/test_mcp/test_config.py +++ b/tests/unit/test_mcp/test_config.py @@ -693,6 +693,8 @@ def test_mcp_config_custom_tool_defaults(): # character, and the body character class. "1foo", "shop.foo", + # `$` alone would accept this. + "foo\n", ], ) def test_mcp_config_rejects_invalid_custom_tool_names(name):