diff --git a/nextcloud_mcp_server/models/tables.py b/nextcloud_mcp_server/models/tables.py index 735a78e6a..ae31d7c25 100644 --- a/nextcloud_mcp_server/models/tables.py +++ b/nextcloud_mcp_server/models/tables.py @@ -71,7 +71,11 @@ class Table(BaseModel): id: int = Field(description="Table ID") title: str = Field(description="Table title") emoji: Optional[str] = Field(None, description="Table emoji") - ownership: str = Field(description="Table ownership") + # Same failure mode as owner_display_name below: a required field here fails + # the *whole* nc_tables_list_tables call (server/tables.py splats each dict + # straight into Table), so one absent key costs every table. Nothing in this + # codebase reads `ownership`, so Optional costs nothing. + ownership: Optional[str] = Field(None, description="Table ownership") # Tables app v2.0.1 stopped emitting owner_display_name at the top level # (still present inside views via get_schema). Optional avoids a 100% failure # rate on list_tables — see #728. @@ -99,8 +103,13 @@ class TableSchema(BaseModel): """Model for complete table schema including columns and views.""" table: Table = Field(description="Table information") - columns: List[TableColumn] = Field(description="Table columns") - views: List[TableView] = Field(description="Table views") + # default_factory for the same reason: a schema response that omits either + # list (an empty table has no views) should degrade to an empty list rather + # than failing the call. + columns: List[TableColumn] = Field( + default_factory=list, description="Table columns" + ) + views: List[TableView] = Field(default_factory=list, description="Table views") class ListTablesResponse(BaseResponse): diff --git a/nextcloud_mcp_server/providers/anthropic.py b/nextcloud_mcp_server/providers/anthropic.py index 0ba56bed1..4cb2a03d3 100644 --- a/nextcloud_mcp_server/providers/anthropic.py +++ b/nextcloud_mcp_server/providers/anthropic.py @@ -2,6 +2,7 @@ import logging +import httpx from anthropic import AsyncAnthropic from anthropic.types import TextBlock @@ -18,8 +19,17 @@ class AnthropicProvider(Provider): Note: Anthropic doesn't provide embedding models, only text generation. """ + # 120s read / 5s connect is the house convention (ollama.py, openai.py). The + # Anthropic SDK otherwise defaults to 600s, long enough that a wedged request + # looks like a hang rather than a failure. + DEFAULT_TIMEOUT_SECONDS = 120.0 + DEFAULT_CONNECT_TIMEOUT_SECONDS = 5.0 + def __init__( - self, api_key: str, generation_model: str = "claude-3-5-sonnet-20241022" + self, + api_key: str, + generation_model: str = "claude-3-5-sonnet-20241022", + timeout: httpx.Timeout | None = None, ): """ Initialize Anthropic provider. @@ -27,8 +37,15 @@ def __init__( Args: api_key: Anthropic API key generation_model: Model name (e.g., "claude-3-5-sonnet-20241022") + timeout: Optional httpx timeout. Defaults to 120s read / 5s connect, + matching the other providers. """ - self.client = AsyncAnthropic(api_key=api_key) + if timeout is None: + timeout = httpx.Timeout( + timeout=self.DEFAULT_TIMEOUT_SECONDS, + connect=self.DEFAULT_CONNECT_TIMEOUT_SECONDS, + ) + self.client = AsyncAnthropic(api_key=api_key, timeout=timeout) self.model = generation_model logger.info("Initialized Anthropic provider (model=%s)", self.model) diff --git a/nextcloud_mcp_server/providers/bedrock.py b/nextcloud_mcp_server/providers/bedrock.py index d8afa40fa..9a47a6389 100644 --- a/nextcloud_mcp_server/providers/bedrock.py +++ b/nextcloud_mcp_server/providers/bedrock.py @@ -6,6 +6,7 @@ try: import boto3 + from botocore.config import Config as BotoConfig from botocore.exceptions import BotoCoreError, ClientError BOTO3_AVAILABLE = True @@ -31,6 +32,10 @@ class BedrockProvider(Provider): - IAM role (when running on AWS) """ + # Matches ollama.py / openai.py / anthropic.py. + DEFAULT_TIMEOUT_SECONDS = 120 + DEFAULT_CONNECT_TIMEOUT_SECONDS = 5 + def __init__( self, region_name: str | None = None, @@ -63,8 +68,18 @@ def __init__( self.generation_model = generation_model self._dimension: int | None = None # Detected dynamically - # Initialize bedrock-runtime client - client_kwargs: dict[str, Any] = {} + # Initialize bedrock-runtime client. + # + # botocore's defaults are 60s connect and 60s read with 3 retries, so a + # wedged endpoint can hold a request for minutes. Pin the same 120s read + # / 5s connect the other providers use; retries stay at botocore's + # default since Bedrock throttling is expected and handled upstream. + client_kwargs: dict[str, Any] = { + "config": BotoConfig( + connect_timeout=self.DEFAULT_CONNECT_TIMEOUT_SECONDS, + read_timeout=self.DEFAULT_TIMEOUT_SECONDS, + ) + } if region_name: client_kwargs["region_name"] = region_name if aws_access_key_id: diff --git a/tests/unit/providers/test_provider_timeouts.py b/tests/unit/providers/test_provider_timeouts.py new file mode 100644 index 000000000..3fba6d9d9 --- /dev/null +++ b/tests/unit/providers/test_provider_timeouts.py @@ -0,0 +1,55 @@ +"""Every provider must pin an explicit timeout. + +The house convention is 120s read / 5s connect (ollama.py, openai.py). Without +it the underlying SDKs fall back to their own defaults — 600s for the Anthropic +SDK, 60s read with 3 retries for botocore — long enough that a wedged endpoint +looks like a hang rather than a failure. +""" + +from __future__ import annotations + +import pytest + +pytestmark = pytest.mark.unit + + +def test_anthropic_provider_pins_a_timeout(mocker): + from nextcloud_mcp_server.providers.anthropic import AnthropicProvider + + ctor = mocker.patch("nextcloud_mcp_server.providers.anthropic.AsyncAnthropic") + + AnthropicProvider(api_key="k") + + timeout = ctor.call_args.kwargs["timeout"] + assert timeout.read == AnthropicProvider.DEFAULT_TIMEOUT_SECONDS + assert timeout.connect == AnthropicProvider.DEFAULT_CONNECT_TIMEOUT_SECONDS + + +def test_anthropic_provider_accepts_an_explicit_timeout(mocker): + import httpx + + from nextcloud_mcp_server.providers.anthropic import AnthropicProvider + + ctor = mocker.patch("nextcloud_mcp_server.providers.anthropic.AsyncAnthropic") + supplied = httpx.Timeout(timeout=7, connect=1) + + AnthropicProvider(api_key="k", timeout=supplied) + + assert ctor.call_args.kwargs["timeout"] is supplied + + +def test_bedrock_provider_pins_botocore_timeouts(mocker): + bedrock = pytest.importorskip("nextcloud_mcp_server.providers.bedrock") + if not bedrock.BOTO3_AVAILABLE: + pytest.skip("boto3 not installed") + + client = mocker.patch.object(bedrock.boto3, "client") + + bedrock.BedrockProvider(region_name="us-east-1") + + config = client.call_args.kwargs["config"] + assert ( + config.connect_timeout + == bedrock.BedrockProvider.DEFAULT_CONNECT_TIMEOUT_SECONDS + ) + assert config.read_timeout == bedrock.BedrockProvider.DEFAULT_TIMEOUT_SECONDS diff --git a/tests/unit/test_response_models.py b/tests/unit/test_response_models.py index b4d0ff142..a28404b67 100644 --- a/tests/unit/test_response_models.py +++ b/tests/unit/test_response_models.py @@ -18,7 +18,7 @@ SamplingSearchResponse, SemanticSearchResult, ) -from nextcloud_mcp_server.models.tables import Table +from nextcloud_mcp_server.models.tables import Table, TableSchema from nextcloud_mcp_server.server.calendar import _event_dict_to_summary from nextcloud_mcp_server.server.contacts import _raw_contact_to_model @@ -1042,3 +1042,26 @@ def test_contact_mapping_truncates_overlong_address_components(): assert len(contact.addresses[0].components) == 7 assert contact.addresses[0].components == ["0", "1", "2", "3", "4", "5", "6"] + + +@pytest.mark.unit +def test_table_tolerates_missing_ownership(): + """A required `ownership` failed the *entire* nc_tables_list_tables call. + + server/tables.py splats each raw dict straight into Table, so one table + missing the key costs every table in the response — the same failure mode as + owner_display_name in #728. Nothing in the codebase reads `ownership`. + """ + table = Table(id=1, title="Budget") + + assert table.ownership is None + assert table.owner_display_name is None + + +@pytest.mark.unit +def test_table_schema_tolerates_missing_columns_and_views(): + """An empty table has no views; a schema response may omit either list.""" + schema = TableSchema(table=Table(id=1, title="Budget")) + + assert schema.columns == [] + assert schema.views == []