diff --git a/CHANGELOG.md b/CHANGELOG.md index df6d0f1..a9fc297 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,35 @@ All notable changes to this project will be documented in this file. +## [0.9.0] - 2026-06-16 + +### Changed (resilience against a slow/stalled NAS) + +Hardened `DsmClient` so a single slow or stalled DSM response can no longer +hang a tool — including read-only ones like `read_compose`. Previously every +tool shared one HTTP client with generous timeouts (FileStation download/upload +used 60 s) and no retry, so when DSM briefly stalled (FileStation busy, NAS +under load) a call blocked for up to a minute and follow-up calls stalled with +it, making the whole MCP appear dead. + +- **Hard per-call ceiling** — every DSM round-trip is wrapped in + `asyncio.wait_for(…, HARD_CALL_TIMEOUT=25 s)`, a backstop that guarantees a + call returns even if DSM accepts a request but never responds. +- **Tighter timeouts** — default read timeout 30 s → 15 s; FileStation + download/upload 60 s → 20 s; `build_stream` per-read 60 s → 20 s. +- **Single retry on a fresh connection** for transient transport failures: any + connection-establishment error is retried (the request provably never reached + DSM, so it is safe even for non-idempotent POSTs), and read/protocol errors + are additionally retried for idempotent GETs. This also absorbs the keepalive + race where DSM closes an idle connection just as a request is sent. +- **Clean error instead of a raw exception** — when both attempts fail, tools + now return a `SynologyError` "NAS … did not respond, it may be busy — please + retry shortly" instead of a raw transport traceback or a long silent hang. + +Note: each tool may still issue several DSM calls, so a sustained NAS outage is +bounded per-call (~15–25 s) rather than per-tool; a per-tool ceiling is a +possible future addition. + ## [0.8.0] - 2026-06-16 ### Changed (fixes intermittent "MCP not responding" timeouts) diff --git a/CLAUDE.md b/CLAUDE.md index 79ad06e..c34e726 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -130,6 +130,16 @@ Only a second consecutive failure is treated as a real auth problem. `update_compose`, `delete_container`, `stop_container`, `restart_container`, `pull_image` - After compose changes: suggest `redeploy_project` +- **Transport resilience (DsmClient):** every DSM round-trip goes through + `DsmClient._send`, which enforces a hard wall-clock ceiling + (`HARD_CALL_TIMEOUT`, 25 s) via `asyncio.wait_for` and retries once on a + fresh connection for transient transport failures (always for + connection-establishment errors; for GETs also on read/protocol errors). + This is what stops a slow/stalled NAS from hanging a tool — including + read-only ones — for minutes. Read timeouts are deliberately tight (15 s + general, 20 s FileStation/build_stream). Keep `build_stream` out of `_send`: + it streams and manages its own budget. Don't widen these timeouts without a + reason — they are the guardrail against the "MCP not responding" hangs. - DSM errors → human-readable message, no stack traces - No secrets in stderr output - Type hints and docstrings everywhere diff --git a/pyproject.toml b/pyproject.toml index 2dc7f81..f5d65e0 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "mcp-synology-container" -version = "0.8.0" +version = "0.9.0" description = "MCP server for Synology Container Manager" requires-python = ">=3.12" dependencies = [ diff --git a/src/mcp_synology_container/dsm_client.py b/src/mcp_synology_container/dsm_client.py index 4f3180a..92e4912 100644 --- a/src/mcp_synology_container/dsm_client.py +++ b/src/mcp_synology_container/dsm_client.py @@ -31,6 +31,30 @@ _SESSION_ERROR_CODES = frozenset({106, 107, 119}) # (and don't escalate 407 "IP blocked" lockouts). INIT_ERROR_COOLDOWN = 60.0 +# Hard wall-clock ceiling (seconds) for a single HTTP round-trip to DSM, +# enforced with asyncio.wait_for as a backstop. DSM occasionally accepts a +# request but is slow to respond (FileStation busy, NAS under load); without a +# ceiling a single stalled call blocks its tool — including read-only ones like +# read_compose — for the full read timeout (which used to be 60 s for +# FileStation). Kept above the per-request read timeouts so httpx's own timeout +# normally fires first; this guarantees no call hangs much longer even in cases +# httpx's timeout does not catch (e.g. a connection-level stall). +HARD_CALL_TIMEOUT = 25.0 + +# Per-request read timeout (seconds) for FileStation download/upload. Small +# compose files transfer in well under a second; a higher value only prolongs +# hangs when DSM stalls. Must stay below HARD_CALL_TIMEOUT. +FILESTATION_TIMEOUT = 20.0 + +# Transport failures where the request provably never reached the server +# (connection not established), so a retry is safe even for non-idempotent +# POSTs. +_PRESEND_TRANSPORT_ERRORS = (httpx.ConnectError, httpx.ConnectTimeout, httpx.PoolTimeout) + +# Transport failures where DSM may already have received (and acted on) the +# request, so a retry is only safe for idempotent GETs. +_READ_TRANSPORT_ERRORS = (httpx.ReadTimeout, httpx.WriteTimeout, httpx.RemoteProtocolError) + # Parameters to mask in debug logging _SENSITIVE_PARAMS = frozenset({"passwd", "_sid", "device_id", "otp_code", "device_token"}) @@ -102,7 +126,7 @@ class DsmClient: self, base_url: str, verify_ssl: bool = True, - timeout: int = 30, + timeout: int = 15, ) -> None: self._base_url = base_url.rstrip("/") self._verify_ssl = verify_ssl @@ -202,6 +226,74 @@ class DsmClient: raise RuntimeError(msg) return self._http + async def _send( + self, + method: str, + url: str, + *, + retry_safe: bool, + **kwargs: Any, + ) -> httpx.Response: + """Perform one HTTP request to DSM with a hard ceiling and one retry. + + Two robustness guarantees on top of the raw httpx call: + + 1. **Hard wall-clock ceiling** (``asyncio.wait_for`` / + :data:`HARD_CALL_TIMEOUT`): even if DSM accepts the request but never + responds, the call returns within the ceiling — so no tool, not even + a read-only one, can hang for minutes on a busy NAS. + 2. **Single retry on a fresh connection** for transient transport + failures: always for connection-establishment errors (the request + provably never reached DSM), and additionally for read/protocol + errors when the operation is idempotent (``retry_safe``). The failed + connection is closed by httpx, so the retry lands on a fresh one — + this also absorbs the keepalive race where DSM closes an idle + connection just as a request is sent. + + Args: + method: ``"GET"`` or ``"POST"``. + url: Full request URL. + retry_safe: True for idempotent operations (GET). Read-level + failures are only retried when True; a POST that may already + have been processed is never retried. + **kwargs: Forwarded to the underlying httpx call (params, data, + files, timeout, ...). + + Returns: + The httpx.Response. The status is not checked here — the caller + still calls ``raise_for_status()``. + + Raises: + SynologyError: If both attempts fail with a transport error or the + hard ceiling is hit (code 0). + """ + http = self._get_http() + send = http.get if method == "GET" else http.post + last_exc: Exception | None = None + for attempt in range(2): + try: + return await asyncio.wait_for(send(url, **kwargs), timeout=HARD_CALL_TIMEOUT) + except _PRESEND_TRANSPORT_ERRORS as e: + last_exc = e # request never reached DSM — safe to retry + except (*_READ_TRANSPORT_ERRORS, TimeoutError) as e: + # TimeoutError covers the asyncio.wait_for ceiling. DSM may have + # acted on the request, so only retry idempotent GETs. + last_exc = e + if not retry_safe: + break + if attempt == 0: + logger.warning( + "DSM %s %s failed (%s) — retrying once on a fresh connection", + method, + _scrub_url(url), + type(last_exc).__name__, + ) + raise SynologyError( + f"NAS at {self._base_url} did not respond ({type(last_exc).__name__}). " + "It may be busy — please retry shortly.", + code=0, + ) from last_exc + async def query_api_info(self) -> dict[str, dict[str, Any]]: """Query SYNO.API.Info to discover all available APIs and cache them. @@ -210,7 +302,6 @@ class DsmClient: Returns: Dict mapping API name -> {path, minVersion, maxVersion}. """ - http = self._get_http() url = f"{self._base_url}/webapi/query.cgi" params = { "api": "SYNO.API.Info", @@ -220,7 +311,7 @@ class DsmClient: } logger.debug("Querying API info from %s", url) - resp = await http.get(url, params=params) + resp = await self._send("GET", url, params=params, retry_safe=True) try: resp.raise_for_status() except httpx.HTTPStatusError as e: @@ -281,7 +372,6 @@ class DsmClient: # The API cache is populated before login, so the cache is ready at this point. if not self._initializing: await self._ensure_initialized() - http = self._get_http() if api not in self._api_cache: raise SynologyError( @@ -310,7 +400,7 @@ class DsmClient: "DSM GET%s: %s/%s v%d — %s", retry_tag, api, method, resolved_version, log_params ) - resp = await http.get(url, params=req_params) + resp = await self._send("GET", url, params=req_params, retry_safe=True) try: resp.raise_for_status() except httpx.HTTPStatusError as e: @@ -374,7 +464,6 @@ class DsmClient: sys.stderr.write(f"[dsm] post_request: {api}/{method}\n") sys.stderr.flush() await self._ensure_initialized() - http = self._get_http() if api not in self._api_cache: raise SynologyError( @@ -401,7 +490,7 @@ class DsmClient: log_form = {k: ("***" if k in _SENSITIVE_PARAMS else v) for k, v in form.items()} logger.debug("DSM POST: %s/%s v%d — %s", api, method, resolved_version, log_form) - resp = await http.post(url, params=query_params, data=form) + resp = await self._send("POST", url, params=query_params, data=form, retry_safe=False) try: resp.raise_for_status() except httpx.HTTPStatusError as e: @@ -513,7 +602,7 @@ class DsmClient: params=params, timeout=httpx.Timeout( connect=10.0, - read=60.0, + read=20.0, write=10.0, pool=5.0, ), @@ -617,7 +706,6 @@ class DsmClient: """ api = "SYNO.FileStation.Upload" await self._ensure_initialized() - http = self._get_http() if api not in self._api_cache: raise SynologyError(f"API '{api}' not found. Call query_api_info() first.", code=102) @@ -647,12 +735,14 @@ class DsmClient: ) encoded = content.encode("utf-8") - resp = await http.post( + resp = await self._send( + "POST", url, params=query_params, data=form_data, files={"file": (filename, encoded, "text/plain")}, - timeout=httpx.Timeout(60.0), + timeout=httpx.Timeout(FILESTATION_TIMEOUT), + retry_safe=False, ) try: resp.raise_for_status() @@ -681,7 +771,6 @@ class DsmClient: """ api = "SYNO.FileStation.Download" await self._ensure_initialized() - http = self._get_http() if api not in self._api_cache: raise SynologyError(f"API '{api}' not found. Call query_api_info() first.", code=102) @@ -703,7 +792,9 @@ class DsmClient: log_params = {k: ("***" if k in _SENSITIVE_PARAMS else v) for k, v in params.items()} logger.debug("DSM GET: %s/download v%d — %s", api, resolved_version, log_params) - resp = await http.get(url, params=params, timeout=httpx.Timeout(60.0)) + resp = await self._send( + "GET", url, params=params, timeout=httpx.Timeout(FILESTATION_TIMEOUT), retry_safe=True + ) try: resp.raise_for_status() except httpx.HTTPStatusError as e: diff --git a/tests/test_dsm_client.py b/tests/test_dsm_client.py index 492c2fa..8143116 100644 --- a/tests/test_dsm_client.py +++ b/tests/test_dsm_client.py @@ -485,6 +485,131 @@ async def test_request_reauth_thundering_herd_login_called_once() -> None: assert client._http.get.call_count == 4 +# ────────────────────────────────────────────────────────────────────── +# _send: hard ceiling + single retry on transient transport failures +# ────────────────────────────────────────────────────────────────────── + + +@pytest.mark.asyncio +async def test_request_retries_once_on_connect_error() -> None: + """A GET that fails to connect is retried once on a fresh connection.""" + async with DsmClient(base_url="https://nas.local:443") as client: + mark_initialized(client) + client._http = AsyncMock() + client._http.get = AsyncMock( + side_effect=[ + httpx.ConnectError("connection refused"), + make_response({"success": True, "data": {"ok": 1}}), + ] + ) + + result = await client.request("SYNO.Docker.Project", "list") + + assert result == {"ok": 1} + assert client._http.get.call_count == 2 + + +@pytest.mark.asyncio +async def test_request_retries_on_read_timeout_for_get() -> None: + """A read timeout on an idempotent GET is retried (server may have stalled).""" + async with DsmClient(base_url="https://nas.local:443") as client: + mark_initialized(client) + client._http = AsyncMock() + client._http.get = AsyncMock( + side_effect=[ + httpx.ReadTimeout("slow"), + make_response({"success": True, "data": {"ok": 2}}), + ] + ) + + result = await client.request("SYNO.Docker.Project", "list") + + assert result == {"ok": 2} + assert client._http.get.call_count == 2 + + +@pytest.mark.asyncio +async def test_request_both_attempts_fail_raises_clean_error() -> None: + """When both attempts fail, a clean SynologyError (code 0) is raised — no + raw transport exception leaks to the tool layer.""" + async with DsmClient(base_url="https://nas.local:443") as client: + mark_initialized(client) + client._http = AsyncMock() + client._http.get = AsyncMock(side_effect=httpx.ConnectError("nas down")) + + with pytest.raises(SynologyError) as exc_info: + await client.request("SYNO.Docker.Project", "list") + + assert exc_info.value.code == 0 + assert "did not respond" in str(exc_info.value) + assert client._http.get.call_count == 2 + + +@pytest.mark.asyncio +async def test_post_request_not_retried_on_read_timeout() -> None: + """A read timeout on a POST is NOT retried — DSM may already have acted on + the (non-idempotent) request.""" + async with DsmClient(base_url="https://nas.local:443") as client: + mark_initialized(client) + client._http = AsyncMock() + client._http.post = AsyncMock(side_effect=httpx.ReadTimeout("slow")) + + with pytest.raises(SynologyError) as exc_info: + await client.post_request("SYNO.Docker.Project", "create") + + assert exc_info.value.code == 0 + assert client._http.post.call_count == 1 # no retry for non-idempotent POST + + +@pytest.mark.asyncio +async def test_post_request_retried_on_connect_error() -> None: + """A pre-send connection failure on a POST IS retried — the request provably + never reached DSM, so it is safe even though POST is non-idempotent.""" + async with DsmClient(base_url="https://nas.local:443") as client: + mark_initialized(client) + client._http = AsyncMock() + client._http.post = AsyncMock( + side_effect=[ + httpx.ConnectError("connection refused"), + make_response({"success": True, "data": {}}), + ] + ) + + result = await client.post_request("SYNO.Docker.Project", "create") + + assert result == {} + assert client._http.post.call_count == 2 + + +@pytest.mark.asyncio +async def test_send_hard_ceiling_bounds_a_stalled_call(monkeypatch) -> None: + """If a call exceeds HARD_CALL_TIMEOUT (DSM accepts but never responds), the + wall-clock ceiling fires, the GET is retried once, then a clean error is + raised — the tool never hangs for the full read timeout.""" + import mcp_synology_container.dsm_client as dc + + monkeypatch.setattr(dc, "HARD_CALL_TIMEOUT", 0.02) + + calls = {"n": 0} + + async def stalled_get(_url: str, **_kwargs: Any) -> Any: + calls["n"] += 1 + await asyncio.sleep(5.0) # cancelled by the ceiling well before this + return make_response({"success": True, "data": {}}) + + async with DsmClient(base_url="https://nas.local:443") as client: + mark_initialized(client) + client._http = AsyncMock() + client._http.get = stalled_get + + with pytest.raises(SynologyError) as exc_info: + await client.request("SYNO.Docker.Project", "list") + + assert exc_info.value.code == 0 + assert "did not respond" in str(exc_info.value) + assert calls["n"] == 2 # ceiling fired on both the initial GET and the retry + + # ────────────────────────────────────────────────────────────────────── # trigger_build_stream # ────────────────────────────────────────────────────────────────────── diff --git a/uv.lock b/uv.lock index b36fd98..5f894ef 100644 --- a/uv.lock +++ b/uv.lock @@ -362,7 +362,7 @@ wheels = [ [[package]] name = "mcp-synology-container" -version = "0.8.0" +version = "0.9.0" source = { editable = "." } dependencies = [ { name = "click" },