fix(filestation): per-path serial for multi-path getinfo and delete

CHANGELOG.md

@@ -4,6 +4,7 @@
 
 ### Fixed
 
+- **`get_file_info` and `delete_files` correctly handle multi-path inputs on real DSM 7.x** (#77) — closes #68. DSM 7.x's `SYNO.FileStation.List getinfo` and `SYNO.FileStation.Delete start` do **not** honor the documented comma-joined multi-path format, even on v2: a request with `path=/a,/b` is treated as a single literal path. For `getinfo` this surfaces as one synthetic record whose `path` field IS the literal comma-joined string (the handler's `len(files) == 1` branch then renders it as a single info card). For `delete_files` this surfaces as a successful task that actually no-ops on every input, returning `[+] Deleted N item(s)` with all paths listed but none removed. The round-1 hypothesis (pin `getinfo` to v2 to dodge a v3 quirk) was disproven by vdsm CI on DSM 7.2.2 — the comma-joined-as-single-path symptom reproduces on v2 too. Fix matches the user's documented workaround on #68: **one DSM call per input path**. Both tools now iterate `paths` and issue per-path requests, aggregating results into the same response shape callers already expect (single info card for one path, table for multiple; per-share recycle-bin messaging unchanged for delete). Trade-off is N round-trips for N paths, which is fine for typical small-N usage and trivially correct. Refactor extracted `_delete_one_path` from `delete_files` so the per-path async-task pattern (start → poll → stop in `try`/`finally`) lives in one place; `get_file_info` simply loops `client.request` since it's synchronous. Bumped `tests/conftest.py` `SYNO.FileStation.List max_version` from 2 to 3 to match DSM 7.x reality so future regression tests don't get fooled by a max-resolves-to-2 default. New `TestGetFileInfo::test_multipath_uses_per_path_serial_calls` asserts (a) N requests for N paths, (b) each request carries a single path with no commas, (c) all pinned to v2, (d) results aggregate correctly. New `TestMultiPathDelete` integration test (re-exported in `tests/vdsm/test_vdsm_integration.py`) creates two folders, deletes both in one multi-path call, verifies via `list_files` that both are actually gone — would have caught the original #68 regression before it shipped if it had existed in v0.5.0. 550 unit tests pass at 96.13% coverage.
 - **`delete_files` now reports recycle-bin status correctly per share** (#73) — closes #37. Pre-fix, `modules/filestation/__init__.py:238` constructed `recycle_status: dict[str, bool] = {}` and never populated it. The closure-shared dict flowed into `delete_files`, where `operations.py:386-398` fell through to the `# Assume recycle bin by default` branch for every share, so users with `#recycle` *disabled* on a share saw `"Recycle bin is enabled on /{share} — files can be recovered..."` after a delete that had actually removed the data permanently. New lazy probe `ensure_recycle_status(client, share, recycle_status)` in `modules/filestation/helpers.py` lives off the cache: a missing share triggers `SYNO.FileStation.List` on `/{share}/#recycle` with `limit=0`. Success → `True`, DSM 408 → `False`, 105 (permission denied) or any other DSM error → `True` + WARNING log (preserves prior optimistic-default behavior so messaging stays consistent and the operator sees the diagnostic). Cached in-place so repeated deletes against the same share don't re-probe. Wired into `delete_files` and `list_recycle_bin` (the two correctness-sensitive paths). `list_shares` left alone — it renders whatever's already cached, kept cheap. **Self-correct on observation**: when `list_recycle_bin` sees DSM behavior contradicting the cached value (cached `True` but the actual list returns 408 → flip to `False`; or vice-versa on an unexpected success), the helper updates the cache in place and logs at INFO so subsequent `delete_files` calls in the same session see the corrected state without waiting for re-auth invalidation. **Invalidate-on-reauth**: new `AuthManager.add_on_reauth_callback` API + dispatch loop in `_re_authenticate`; new `SharedClientManager.subscribe_on_reauth` proxy that queues callbacks before the AuthManager is lazily created and flushes them on first `get_client`. Filestation `register()` subscribes `recycle_status.clear` so admin-side toggles to `#recycle` between sessions are picked up after the next session-error-driven re-auth. Eighteen new tests (9 helper + 3 auth + 3 server + 1 delete-files lazy-probe + 2 list_recycle_bin self-correct); 546 passing total at 96.10% coverage. Persistence across server restarts (binding the closure dict to `ServerState.recycle_bin_status`) is intentionally out of scope — that field exists on the model but `load_state`/`save_state` aren't currently wired up at all; treating that as a separate follow-up since plumbing it touches a much larger surface than #37 alone needs. vdsm integration test (verifying real DSM recycle-on/recycle-off shares produce correct messaging end-to-end) is also a follow-up — `synoshare --setopt` recycle-toggle reliability on DSM 7.2.x is unproven (PR #23 reverted a similar setopt for share creation), so unit tests carry the regression load until that's verified.
 
 ### Added

src/mcp_synology/modules/filestation/metadata.py

@@ -16,7 +16,6 @@
     synology_error_response,
 )
 from mcp_synology.modules.filestation.helpers import (
-    escape_multi_path,
     normalize_path,
 )
 
@@ -46,31 +45,62 @@ async def get_file_info(
     paths: list[str],
     additional: list[str] | None = None,
 ) -> str:
-    """Get detailed metadata for specific files or folders."""
+    """Get detailed metadata for specific files or folders.
+
+    Closes #68 (the `get_file_info` half). DSM 7.x's
+    `SYNO.FileStation.List getinfo` does not honor the documented
+    comma-joined multi-path format on v2 — verified against vdsm DSM
+    7.2.2 in CI: a request with `path=/a,/b` returns a single synthetic
+    record whose `path` field IS the literal `/a,/b` string. v3 has the
+    same problem (verified by the round-1 v2-pin attempt that didn't
+    fix the bug). The user's documented workaround on #68 was to loop
+    one path at a time, and that's what this function now does
+    internally: one DSM call per input path, results aggregated into
+    the response shape callers already expect (single-info card for one
+    path, table for multiple). Trade-off is N round-trips for N paths,
+    which is fine for the typical small-N usage and trivially correct.
+    """
     if additional is None:
         additional = ["real_path", "size", "owner", "time", "perm"]
 
-    normalized = [normalize_path(p) for p in paths]
-    path_param = escape_multi_path(normalized)
-
-    try:
-        data = await client.request(
-            "SYNO.FileStation.List",
-            "getinfo",
-            params={
-                "path": path_param,
-                "additional": '["' + '","'.join(additional) + '"]',
-            },
+    if not paths:
+        error_response(
+            ErrorCode.NOT_FOUND,
+            "Get file info failed: No paths provided.",
+            retryable=False,
+            param="paths",
+            value=paths,
+            suggestion="Pass at least one path.",
         )
-    except SynologyError as e:
-        synology_error_response("Get file info", e)
 
-    files = data.get("files", [])
+    normalized = [normalize_path(p) for p in paths]
+
+    # Pin to v2 for parallelism with Delete / CopyMove / Search. v3 doesn't
+    # buy us anything for getinfo and adds risk of further multi-path quirks.
+    getinfo_version = min(2, client.negotiate_version("SYNO.FileStation.List", max_version=2))
+    additional_param = '["' + '","'.join(additional) + '"]'
+
+    files: list[dict[str, Any]] = []
+    for p in normalized:
+        try:
+            data = await client.request(
+                "SYNO.FileStation.List",
+                "getinfo",
+                version=getinfo_version,
+                params={
+                    "path": p,
+                    "additional": additional_param,
+                },
+            )
+        except SynologyError as e:
+            synology_error_response("Get file info", e)
+        per_path_files = data.get("files", [])
+        if per_path_files:
+            files.extend(per_path_files)
 
     if len(files) == 1:
         return _format_single_info(files[0])
 
-    # Multiple files: table format
     if not files:
         error_response(
             ErrorCode.NOT_FOUND,

src/mcp_synology/modules/filestation/operations.py

@@ -285,28 +285,26 @@ async def _copy_move(
     return "\n".join(lines)
 
 
-async def delete_files(
+async def _delete_one_path(
     client: DsmClient,
+    path: str,
     *,
-    paths: list[str],
-    recursive: bool = True,
-    recycle_bin_status: dict[str, bool] | None = None,
-    timeout: float = 120.0,
-) -> str:
-    """Delete files or folders."""
-    normalized = [normalize_path(p) for p in paths]
-    path_param = escape_multi_path(normalized)
-
-    # Pin to version 2 — v3 uses JSON request format with different parameter encoding.
-    delete_version = min(2, client.negotiate_version("SYNO.FileStation.Delete", max_version=2))
+    recursive: bool,
+    delete_version: int,
+    timeout: float,
+) -> None:
+    """Delete a single path via DSM async-task pattern.
 
+    Raises ToolError (via error_response/synology_error_response) on failure;
+    returns None on success. Always stops the background task on exit.
+    """
     try:
         start_data = await client.request(
             "SYNO.FileStation.Delete",
             "start",
             version=delete_version,
             params={
-                "path": path_param,
+                "path": path,
                 "recursive": str(recursive).lower(),
             },
         )
@@ -315,7 +313,6 @@ async def delete_files(
 
     taskid = start_data.get("taskid", "")
 
-    # Poll for completion. Use try/finally to ensure task is always stopped.
     elapsed = 0.0
     interval = 0.5
     status: dict[str, Any] = {}
@@ -335,7 +332,7 @@ async def delete_files(
                 poll_error = e
                 break
 
-            logger.debug("Delete status: %s", status)
+            logger.debug("Delete status (%s): %s", path, status)
 
             if status.get("finished", False):
                 break
@@ -358,21 +355,17 @@ async def delete_files(
     if timed_out:
         error_response(
             ErrorCode.TIMEOUT,
-            f"Delete files failed: timed out after {timeout}s.",
+            f"Delete files failed: timed out after {timeout}s on path: {path}",
             retryable=True,
             param="timeout",
             value=timeout,
             suggestion="The operation may still be running on the NAS.",
         )
 
-    # Check for errors in the completed task
     if "error" in status:
         err = status["error"]
         err_code = err.get("code", 0) if isinstance(err, dict) else err
-        err_path = status.get("path", "")
-        # Route err_code through error_from_code so callers see a specific
-        # envelope (e.g. permission_denied, not_found) matching the synchronous
-        # error paths in this module. Unknown codes fall back to DSM_ERROR.
+        err_path = status.get("path", path)
         mapped = error_from_code(err_code, "SYNO.FileStation.Delete")
         error_response(
             mapped.error_code,
@@ -384,6 +377,54 @@ async def delete_files(
             ),
         )
 
+
+async def delete_files(
+    client: DsmClient,
+    *,
+    paths: list[str],
+    recursive: bool = True,
+    recycle_bin_status: dict[str, bool] | None = None,
+    timeout: float = 120.0,
+) -> str:
+    """Delete files or folders.
+
+    Closes #68 (the `delete_files` half). DSM 7.x's
+    `SYNO.FileStation.Delete start` does not honor the documented
+    comma-joined multi-path format on v2 — the user reported that
+    `delete_files(paths=[a, b, ..., l])` returns success but no paths
+    are actually deleted, while single-path calls work. The verified
+    parallel symptom on `get_file_info` (vdsm CI proves a v2-style
+    `path=/a,/b` returns one synthetic record with the literal
+    comma-joined string as its `path`) makes the same root cause near
+    certain on Delete. Fix matches the user's documented workaround:
+    one DSM Delete task per input path. Per-path serial means N
+    round-trips for N paths, which is fine for typical small-N usage
+    and trivially correct.
+    """
+    if not paths:
+        error_response(
+            ErrorCode.NOT_FOUND,
+            "Delete files failed: No paths provided.",
+            retryable=False,
+            param="paths",
+            value=paths,
+            suggestion="Pass at least one path.",
+        )
+
+    normalized = [normalize_path(p) for p in paths]
+
+    # Pin to v2 — v3 uses JSON request format with different parameter encoding.
+    delete_version = min(2, client.negotiate_version("SYNO.FileStation.Delete", max_version=2))
+
+    for p in normalized:
+        await _delete_one_path(
+            client,
+            p,
+            recursive=recursive,
+            delete_version=delete_version,
+            timeout=timeout,
+        )
+
     # Determine recycle bin status per share. Probes lazily on first observation
     # via `ensure_recycle_status` so the dict stays empty until we actually need
     # the answer; pre-#37 the dict was never populated and every share was

tests/conftest.py

@@ -43,7 +43,11 @@ def make_api_cache() -> dict[str, ApiInfoEntry]:
     return {
         "SYNO.API.Auth": ApiInfoEntry(path="entry.cgi", min_version=1, max_version=7),
         "SYNO.FileStation.Info": ApiInfoEntry(path="entry.cgi", min_version=1, max_version=2),
-        "SYNO.FileStation.List": ApiInfoEntry(path="entry.cgi", min_version=1, max_version=2),
+        # max_version=3 matches DSM 7.x (where #68 surfaced); production code
+        # MUST pin to v2 explicitly via `negotiate_version(..., max_version=2)`
+        # because v3 reinterprets multi-path semantics and silently breaks
+        # comma-joined `path` queries.
+        "SYNO.FileStation.List": ApiInfoEntry(path="entry.cgi", min_version=1, max_version=3),
         "SYNO.FileStation.Search": ApiInfoEntry(path="entry.cgi", min_version=1, max_version=2),
         "SYNO.FileStation.DirSize": ApiInfoEntry(path="entry.cgi", min_version=1, max_version=2),
         "SYNO.FileStation.CreateFolder": ApiInfoEntry(

tests/modules/filestation/test_metadata.py

@@ -88,6 +88,86 @@ async def test_error(self, mock_client: DsmClient) -> None:
         assert body["status"] == "error"
         assert body["error"]["code"] == "not_found"
 
+    @respx.mock
+    async def test_multipath_uses_per_path_serial_calls(self, mock_client: DsmClient) -> None:
+        """Closes #68 (the get_file_info half): DSM 7.x's
+        `SYNO.FileStation.List getinfo` doesn't honor the documented
+        comma-joined multi-path format on v2 (vdsm 7.2.2 verified — the
+        comma-joined string lands as a literal single path, returning one
+        synthetic record). Production therefore issues ONE DSM call per
+        input path. Test asserts (a) N requests for N paths, (b) each
+        request carries a single path (no commas), (c) all pinned to
+        v2, (d) results aggregate into the table-format response.
+        """
+        captured: list[dict[str, str]] = []
+        per_path_files = {
+            "/video/a.mkv": {
+                "name": "a.mkv",
+                "path": "/video/a.mkv",
+                "isdir": False,
+                "additional": {"size": 1, "time": {"mtime": 1710000000}},
+            },
+            "/video/b.srt": {
+                "name": "b.srt",
+                "path": "/video/b.srt",
+                "isdir": False,
+                "additional": {"size": 1, "time": {"mtime": 1710000000}},
+            },
+        }
+
+        def side_effect(request: httpx.Request) -> httpx.Response:
+            params = dict(request.url.params)
+            captured.append(params)
+            requested_path = params.get("path", "")
+            return httpx.Response(
+                200,
+                json={
+                    "success": True,
+                    "data": {"files": [per_path_files[requested_path]]},
+                },
+            )
+
+        respx.get(f"{BASE_URL}/webapi/entry.cgi").mock(side_effect=side_effect)
+        result = await get_file_info(mock_client, paths=["/video/a.mkv", "/video/b.srt"])
+
+        # Two paths → two DSM calls.
+        assert len(captured) == 2, f"expected two DSM calls (one per path), got {len(captured)}"
+        for params in captured:
+            assert params["api"] == "SYNO.FileStation.List"
+            assert params["method"] == "getinfo"
+            assert params["version"] == "2", (
+                f"expected version=2, got version={params['version']!r}"
+            )
+            assert "," not in params["path"], (
+                f"each request must carry a single path (no comma-joined multipath), "
+                f"got path={params['path']!r}"
+            )
+        # Each input path appears in exactly one request.
+        sent_paths = sorted(p["path"] for p in captured)
+        assert sent_paths == ["/video/a.mkv", "/video/b.srt"]
+        # And the aggregated response renders both files in tabular form.
+        assert "a.mkv" in result
+        assert "b.srt" in result
+        # No comma-joined string in the result — that would indicate a regression
+        # back to the single-call form that #68 surfaced.
+        assert "/video/a.mkv,/video/b.srt" not in result
+
+    async def test_empty_paths_list_returns_not_found(self, mock_client: DsmClient) -> None:
+        """Empty paths list short-circuits to a not_found error before any
+        DSM call is attempted. Defensive guard added with the per-path
+        serial refactor — without it, the for-loop would simply do nothing
+        and the downstream `if not files:` branch would fire with a less
+        precise message about missing file information.
+        """
+        with pytest.raises(ToolError) as exc_info:
+            await get_file_info(mock_client, paths=[])
+        body = json.loads(str(exc_info.value))
+        assert body["status"] == "error"
+        assert body["error"]["code"] == "not_found"
+        assert "No paths provided" in body["error"]["message"]
+        assert body["error"]["param"] == "paths"
+        assert body["error"]["value"] == []
+
     @respx.mock
     async def test_empty_files_list_returns_not_found(self, mock_client: DsmClient) -> None:
         """getinfo succeeds but returns no files → not_found.

tests/modules/filestation/test_operations.py

@@ -219,6 +219,21 @@ async def test_delete_multiple_shares(self, mock_client: DsmClient) -> None:
         assert "enabled" in result
         assert "NOT enabled" in result
 
+    async def test_delete_empty_paths_list_returns_not_found(self, mock_client: DsmClient) -> None:
+        """Empty paths list short-circuits to a not_found error before any
+        DSM call is attempted. Defensive guard added with the per-path
+        serial refactor — without it, the per-path for-loop would simply
+        do nothing and return an empty success message.
+        """
+        with pytest.raises(ToolError) as exc_info:
+            await delete_files(mock_client, paths=[])
+        body = json.loads(str(exc_info.value))
+        assert body["status"] == "error"
+        assert body["error"]["code"] == "not_found"
+        assert "No paths provided" in body["error"]["message"]
+        assert body["error"]["param"] == "paths"
+        assert body["error"]["value"] == []
+
     @respx.mock
     async def test_delete_lazily_probes_when_share_missing_from_cache(
         self, mock_client: DsmClient