From e3fa71b45895e99058ad71e438164314938312ea Mon Sep 17 00:00:00 2001 From: Marcus van Elst Date: Tue, 14 Apr 2026 13:01:27 +0200 Subject: [PATCH] fix: retry _poll_task on transient 599 instead of aborting immediately MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DirSize for large directories (e.g. /docker, 8441 folders, 46832 files) takes ~800ms to compute. While running, status returns intermediate progress (finished=false). But on the very first poll the task can return 599 transiently (task just started, not yet available). Previously _poll_task caught any SynologyError and returned immediately, making dir_size always fail on the first 599. Fix: treat 599 as a transient condition and continue polling. Give up only after 5 consecutive 599 responses. All other error codes remain immediately fatal. Investigation confirmed with test_dirsize_md5.py: - /test-mcp (2937 B): finished=true at 0ms - /docker (3.9 GB, 46832 files): finished=false at 35ms, finished=true at 789ms Tests: 2 new cases (retry-succeeds, 5x-599-gives-up) → 95 total Co-Authored-By: Claude Sonnet 4.6 --- pyproject.toml | 2 +- src/mcp_synology_filestation/__init__.py | 2 +- .../tools/filestation.py | 17 +- test_dirsize_md5.py | 153 +++++++----------- tests/test_tools_filestation.py | 43 +++++ 5 files changed, 113 insertions(+), 104 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 0693e15..feeb1a1 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "mcp-synology-filestation" -version = "0.2.1" +version = "0.2.2" description = "MCP server for Synology FileStation" requires-python = ">=3.12" dependencies = [ diff --git a/src/mcp_synology_filestation/__init__.py b/src/mcp_synology_filestation/__init__.py index d89fd8b..5ce481f 100644 --- a/src/mcp_synology_filestation/__init__.py +++ b/src/mcp_synology_filestation/__init__.py @@ -1,3 +1,3 @@ """MCP server for Synology FileStation.""" -__version__ = "0.2.1" +__version__ = "0.2.2" diff --git a/src/mcp_synology_filestation/tools/filestation.py b/src/mcp_synology_filestation/tools/filestation.py index d5f4f56..69c8fa6 100644 --- a/src/mcp_synology_filestation/tools/filestation.py +++ b/src/mcp_synology_filestation/tools/filestation.py @@ -86,6 +86,7 @@ def register_filestation( delay = 0.2 elapsed = initial_delay timeout = 60.0 + consecutive_599 = 0 if initial_delay > 0: await asyncio.sleep(initial_delay) @@ -98,11 +99,19 @@ def register_filestation( version=version, params={"taskid": taskid}, ) + consecutive_599 = 0 except _SynologyError as e: - return False, f"Error: {e}" - - if status_data.get("finished"): - return True, status_data + if e.code == 599: + # 599 can be transient (task just started, not yet available). + # Retry up to 5 times before giving up. + consecutive_599 += 1 + if consecutive_599 >= 5: + return False, f"Error: {e}" + else: + return False, f"Error: {e}" + else: + if status_data.get("finished"): + return True, status_data if elapsed >= timeout: return ( diff --git a/test_dirsize_md5.py b/test_dirsize_md5.py index 894756e..5455b8a 100644 --- a/test_dirsize_md5.py +++ b/test_dirsize_md5.py @@ -1,19 +1,5 @@ """Wegwerfskript: DirSize + MD5 direkt gegen die NAS testen. -Finale Befunde: - DirSize: start v2 (path als plain string ODER JSON-Array), status v1 (0ms delay) - MD5: start v2, status v1 (0ms delay, ONE-SHOT) - -DirSize status-Response Felder: - finished: bool - num_dir: int (Anzahl Unterordner) - num_file: int (Anzahl Dateien) - total_size: int (Gesamtgroesse in Bytes) - -MD5 status-Response Felder: - finished: bool - md5: str (Hex-String, 32 Zeichen) - Ausfuehren: uv run python test_dirsize_md5.py """ @@ -27,13 +13,13 @@ from mcp_synology_filestation.auth import AuthManager from mcp_synology_filestation.client import FileStationClient from mcp_synology_filestation.config import load_config -DIRSIZE_PATH = "/test-mcp" +DIRSIZE_PATHS = ["/test-mcp", "/docker"] MD5_PATH = "/test-mcp/test.zip" def pp(label: str, data: object, elapsed_ms: float | None = None) -> None: print(f"\n{'='*60}") - suffix = f" [{elapsed_ms:.1f} ms nach start]" if elapsed_ms is not None else "" + suffix = f" [{elapsed_ms:.1f} ms]" if elapsed_ms is not None else "" print(f" {label}{suffix}") print("=" * 60) print(json.dumps(data, indent=2, ensure_ascii=False)) @@ -48,87 +34,67 @@ async def raw(http: httpx.AsyncClient, url: str, sid: str, **params) -> dict: return {"_raw": r.text[:300], "_http_status": r.status_code} -async def probe_dirsize_path_variants( - http: httpx.AsyncClient, sid: str, api_url: str +async def probe_dirsize_long( + http: httpx.AsyncClient, sid: str, api_url: str, path: str ) -> None: - """Test all three path encoding variants for DirSize start.""" + """Start DirSize and poll v1 every 200ms for up to 15s. + + Goal: find out if 599 means 'task still running' (keep polling) + or 'task gone' (give up). If the task eventually returns data, + 599 = 'not ready yet'. If it never returns data, 599 = 'task gone'. + """ print(f"\n{'#'*60}") - print(" DIRSIZE path-Varianten (a=plain, b=json.dumps, c=manuell)") - print(f"{'#'*60}") - - variants = [ - ("a) plain string", DIRSIZE_PATH), - ("b) json.dumps([path])", json.dumps([DIRSIZE_PATH])), - ("c) json.dumps(path)", json.dumps(DIRSIZE_PATH)), - ] - - for label, path_val in variants: - print(f"\n--- Variante {label} ---") - print(f" path={path_val!r}") - - t0 = time.perf_counter() - start_body = await raw( - http, api_url, sid, - api="SYNO.FileStation.DirSize", version="2", method="start", - path=path_val, - ) - pp(f"DirSize::start [{label}]", start_body, (time.perf_counter() - t0) * 1000) - - taskid = (start_body.get("data") or {}).get("taskid") - if not taskid: - print(" => Kein taskid (start fehlgeschlagen)") - continue - - # Poll immediately - t1 = time.perf_counter() - status_body = await raw( - http, api_url, sid, - api="SYNO.FileStation.DirSize", version="1", method="status", - taskid=taskid, - ) - pp(f"DirSize::status [{label}]", status_body, (t1 - t0) * 1000) - - data = (status_body.get("data") or {}) - if data.get("finished"): - print(f" => OK: num_dir={data.get('num_dir')} " - f"num_file={data.get('num_file')} " - f"total_size={data.get('total_size')}") - else: - code = (status_body.get("error") or {}).get("code", "?") - print(f" => FEHLER code={code}") - - -async def probe_md5(http: httpx.AsyncClient, sid: str, api_url: str) -> None: - """Test MD5 with status v1 at 0ms (correct settings).""" - print(f"\n{'#'*60}") - print(" MD5 -- start v2, status v1, 0ms delay") + print(f" DIRSIZE {path} — long poll (15s, every 200ms)") print(f"{'#'*60}") t0 = time.perf_counter() start_body = await raw( http, api_url, sid, - api="SYNO.FileStation.MD5", version="2", method="start", - file_path=json.dumps(MD5_PATH), + api="SYNO.FileStation.DirSize", version="2", method="start", + path=json.dumps([path]), ) - pp("MD5::start", start_body, (time.perf_counter() - t0) * 1000) + elapsed_start = (time.perf_counter() - t0) * 1000 + pp(f"DirSize::start ({path})", start_body, elapsed_start) taskid = (start_body.get("data") or {}).get("taskid") if not taskid: - print("[!] No taskid") + print("[!] No taskid.") return - t1 = time.perf_counter() - r = await raw( - http, api_url, sid, - api="SYNO.FileStation.MD5", version="1", method="status", - taskid=taskid, - ) - pp("MD5::status v1 [0ms]", r, (t1 - t0) * 1000) - data = (r.get("data") or {}) - if data.get("finished"): - print(f" => OK: md5={data.get('md5')}") - else: - print(f" => FEHLER: {r}") + print(f"\n[*] Polling status v1 every 200ms for up to 15s (taskid={taskid[:12]}...)") + for attempt in range(75): # 75 * 200ms = 15s + if attempt > 0: + await asyncio.sleep(0.2) + t = time.perf_counter() + r = await raw( + http, api_url, sid, + api="SYNO.FileStation.DirSize", version="1", method="status", + taskid=taskid, + ) + elapsed = (t - t0) * 1000 + success = r.get("success") + data = (r.get("data") or {}) + finished = data.get("finished") + error_code = (r.get("error") or {}).get("code") + + if finished: + print(f" [{elapsed:.0f}ms] attempt {attempt+1}: FERTIG! " + f"num_dir={data.get('num_dir')} " + f"num_file={data.get('num_file')} " + f"total_size={data.get('total_size')}") + pp(f"DirSize::status final ({path})", r, elapsed) + return + elif success and not finished: + # Still running — show current progress + print(f" [{elapsed:.0f}ms] attempt {attempt+1}: running... " + f"num_dir={data.get('num_dir', '?')} " + f"num_file={data.get('num_file', '?')} " + f"total_size={data.get('total_size', '?')}") + else: + print(f" [{elapsed:.0f}ms] attempt {attempt+1}: error code={error_code}") + # Continue polling — 599 might mean 'not ready yet' + + print(f"\n[!] No result after 15s — task never returned data.") async def main() -> None: @@ -141,22 +107,13 @@ async def main() -> None: sid = client.sid base = config.base_url - for api_name in ["SYNO.FileStation.DirSize", "SYNO.FileStation.MD5"]: - info = client._api_cache.get(api_name) # noqa: SLF001 - if info: - print(f"[*] {api_name}: path={info['path']} " - f"v{info['minVersion']}-v{info['maxVersion']}") - else: - print(f"[!] {api_name}: NOT in API cache!") - - dirsize_info = client._api_cache.get("SYNO.FileStation.DirSize", {}) # noqa: SLF001 - md5_info = client._api_cache.get("SYNO.FileStation.MD5", {}) # noqa: SLF001 - dirsize_url = f"{base}/webapi/{dirsize_info.get('path', 'entry.cgi')}" - md5_url = f"{base}/webapi/{md5_info.get('path', 'entry.cgi')}" + info = client._api_cache.get("SYNO.FileStation.DirSize", {}) # noqa: SLF001 + api_url = f"{base}/webapi/{info.get('path', 'entry.cgi')}" + print(f"[*] DirSize API: {api_url} v{info.get('minVersion')}-v{info.get('maxVersion')}") async with httpx.AsyncClient(verify=config.connection.verify_ssl, timeout=30.0) as http: - await probe_dirsize_path_variants(http, sid, dirsize_url) - await probe_md5(http, sid, md5_url) + for path in DIRSIZE_PATHS: + await probe_dirsize_long(http, sid, api_url, path) await auth.logout(client) print("\n[*] Logout OK.") diff --git a/tests/test_tools_filestation.py b/tests/test_tools_filestation.py index 00c0fe8..affe958 100644 --- a/tests/test_tools_filestation.py +++ b/tests/test_tools_filestation.py @@ -1610,6 +1610,49 @@ async def test_dir_size_empty_path(config: AppConfig) -> None: client.request.assert_not_called() +@pytest.mark.asyncio +async def test_dir_size_retries_on_transient_599(config: AppConfig) -> None: + """dir_size retries up to 4 times on code-599 then succeeds on 5th status call.""" + client = MagicMock() + call_count = {"status": 0} + + async def _request(api, method, version=None, params=None, **kwargs): + if method == "start": + return {"taskid": "FileStation_dirsize_599"} + call_count["status"] += 1 + if call_count["status"] < 4: + raise SynologyError("DSM error code 599", code=599) + return {"finished": True, "num_dir": 2, "num_file": 10, "total_size": 1024} + + client.request = AsyncMock(side_effect=_request) + tools = _make_mcp_and_tools(config, client) + + with patch("asyncio.sleep", new_callable=AsyncMock): + result = await tools["dir_size"](path="/data") + + assert "Total Size" in result + assert call_count["status"] == 4 + + +@pytest.mark.asyncio +async def test_dir_size_fails_after_5_consecutive_599(config: AppConfig) -> None: + """dir_size gives up and returns Error: after 5 consecutive 599 responses.""" + client = MagicMock() + + async def _request(api, method, version=None, params=None, **kwargs): + if method == "start": + return {"taskid": "FileStation_dirsize_dead"} + raise SynologyError("DSM error code 599", code=599) + + client.request = AsyncMock(side_effect=_request) + tools = _make_mcp_and_tools(config, client) + + with patch("asyncio.sleep", new_callable=AsyncMock): + result = await tools["dir_size"](path="/dead") + + assert result.startswith("Error:") + + # ────────────────────────────────────────────────────────────────────────── # get_md5 # ──────────────────────────────────────────────────────────────────────────