Harden DsmClient against a slow/stalled NAS so no tool can hang

A second, independent failure mode behind the intermittent "MCP not responding" reports: every tool shares one HTTP client with generous timeouts (FileStation download/upload used 60s) and no retry. When DSM briefly stalled (FileStation busy, NAS under load) a single call blocked for up to a minute, and follow-up calls - even read-only ones like read_compose - stalled with it, so the whole MCP appeared dead. The path to the NAS is direct LAN (dsm.gecheckt.de -> 192.168.0.2), so this is DSM responsiveness, not a network/NAT-loopback issue. Route every DSM round-trip through a new DsmClient._send that adds: - a hard wall-clock ceiling (asyncio.wait_for, HARD_CALL_TIMEOUT=25s) so a call returns even if DSM accepts a request but never responds; - a single retry on a fresh connection for transient transport failures - always for connection-establishment errors (request provably never reached DSM, safe even for POST), and for GETs also on read/protocol errors (also absorbs the keepalive race where DSM drops an idle conn); - a clean SynologyError ("NAS did not respond, it may be busy") instead of a raw transport traceback or a long silent hang. Also tighten timeouts: default read 30s->15s, FileStation 60s->20s, build_stream per-read 60s->20s. build_stream stays out of _send (it streams and manages its own budget). - add 6 tests for retry/no-retry/ceiling behaviour (320 pass) - update CLAUDE.md and CHANGELOG; bump 0.8.0 -> 0.9.0 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-06-16 07:58:34 +02:00
parent 4a49407883
commit 46b36f6b08
6 changed files with 270 additions and 15 deletions
@@ -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)
@@ -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
@@ -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 = [
@@ -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:
@@ -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
 # ──────────────────────────────────────────────────────────────────────
@@ -362,7 +362,7 @@ wheels = [

 [[package]]
 name = "mcp-synology-container"
-version = "0.8.0"
+version = "0.9.0"
 source = { editable = "." }
 dependencies = [
    { name = "click" },