fix(filestation): probe recycle-bin status per share

CHANGELOG.md

@@ -2,6 +2,10 @@
 
 ## Unreleased
 
+### Fixed
+
+- **`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
 
 - **README: per-installer and per-OS download-breakdown badges** (#72) — adds two new badge groups to `README.md` mirroring the layout `cmeans/mcp-clipboard` adopted after the upstream `cmeans/pypi-winnow-downloads` service grew its installer/OS endpoints. Group 1 (six badges): `pip`, `pipenv`, `pipx`, `uv`, `poetry`, `pdm` 30d non-CI download counts via `installer-{installer}-30d-non-ci.json` endpoints. Group 2 (three badges): `linux`, `macos`, `windows` 30d non-CI download counts via `os-{os}-30d-non-ci.json` endpoints. All nine new badges link to [`cmeans/pypi-winnow-downloads`](https://github.com/cmeans/pypi-winnow-downloads) (the dogfooded service) rather than to PyPI itself, consistent with the existing aggregate Downloads badge from #62 — keeps the "powered by" attribution implicit and gives a curious reader a single click into the data source. Verified the endpoints are live for `mcp-synology` (e.g. `installer-pip-30d-non-ci.json` returns the expected schemaVersion-1 payload). No code or test changes — README-only.

src/mcp_synology/core/auth.py

@@ -18,6 +18,8 @@
 from mcp_synology.core.errors import AuthenticationError, SynologyError
 
 if TYPE_CHECKING:
+    from collections.abc import Callable
+
     from mcp_synology.core.client import DsmClient
     from mcp_synology.core.config import AppConfig
 
@@ -46,11 +48,26 @@ def __init__(self, config: AppConfig, client: DsmClient) -> None:
         self._client = client
         self._lock = asyncio.Lock()
         self._session_name = self._build_session_name()
+        # Subscriber list — fired after a successful re-auth so module-owned
+        # caches (e.g. filestation's `recycle_status`) can invalidate any
+        # state that may have changed on the NAS between sessions. See #37.
+        self._on_reauth_callbacks: list[Callable[[], None]] = []
 
         # Register ourselves as the re-auth callback
         self._client.set_re_auth_callback(self._re_authenticate)
         logger.debug("AuthManager initialized, session name: %s", self._session_name)
 
+    def add_on_reauth_callback(self, cb: Callable[[], None]) -> None:
+        """Register a callback to fire after each successful session re-auth.
+
+        Modules that cache NAS state which can drift mid-session (the canonical
+        case is filestation's recycle-bin probe cache) subscribe here so they
+        can clear the cache and let the next operation re-probe against fresh
+        DSM state. Callbacks must be cheap and synchronous; an exception
+        from one callback is logged and does not prevent the rest from firing.
+        """
+        self._on_reauth_callbacks.append(cb)
+
     def _build_session_name(self) -> str:
         """Build a unique DSM session name: MCPSynology_{instance_id}_{uuid}."""
         instance_id = self._config.instance_id or "default"
@@ -217,9 +234,22 @@ async def _re_authenticate(self) -> None:
         """Re-authenticate transparently (called by DsmClient on session errors).
 
         Uses asyncio.Lock to prevent concurrent re-auth from multiple requests.
+        Fires every registered ``on_reauth`` callback after the new session is
+        live so module-owned caches (filestation's recycle-bin probe, etc.)
+        can drop entries that may have drifted while the session was down.
         """
         async with self._lock:
             # Another coroutine may have already re-authenticated
             logger.info("Re-authenticating session '%s'.", self._session_name)
             self._client.sid = None
             await self.login()
+            # NOTE: callbacks fire INSIDE `self._lock`. Per `add_on_reauth_callback`'s
+            # contract they must be cheap and synchronous — anything heavy here would
+            # block every other coroutine waiting to re-auth. Today's only subscriber
+            # is `recycle_status.clear` (O(n_shares)); keep new callbacks similarly
+            # trivial or move the work to a deferred task.
+            for cb in self._on_reauth_callbacks:
+                try:
+                    cb()
+                except Exception as e:  # noqa: BLE001 — best-effort dispatch
+                    logger.warning("on_reauth callback %r raised: %s", cb, e)

src/mcp_synology/modules/filestation/__init__.py

@@ -235,11 +235,21 @@ def register(ctx: RegisterContext) -> None:
     search_poll_interval = settings.search_poll_interval
     hide_recycle = settings.hide_recycle_in_listings
 
+    # Closure-shared cache of per-share recycle-bin enabled-ness. Populated
+    # lazily by `ensure_recycle_status` on first observation per share, and
+    # cleared on session re-auth so admin-side toggles get picked up next
+    # time. See helpers.py for the probe and self-correct logic. Closes #37.
     recycle_status: dict[str, bool] = {}
     hostname = ctx.display_name
     server = ctx.server
     manager = ctx.manager
 
+    # Subscribe a cache-invalidator that fires after every successful
+    # AuthManager re-auth. Safe to call before the AuthManager exists —
+    # SharedClientManager queues the callback and flushes on first
+    # `get_client`.
+    manager.subscribe_on_reauth(recycle_status.clear)
+
     # Build a lookup for tool annotations
     _tool_annos: dict[str, ToolAnnotations] = {}
     for t in MODULE_INFO.tools:

src/mcp_synology/modules/filestation/helpers.py

@@ -8,6 +8,7 @@
 from datetime import UTC, datetime
 
 from mcp_synology.core.client import DsmClient
+from mcp_synology.core.errors import SynologyError
 
 logger = logging.getLogger(__name__)
 
@@ -167,3 +168,104 @@ def escape_multi_path(paths: list[str]) -> str:
 def matches_pattern(filename: str, pattern: str) -> bool:
     """Check if a filename matches a glob pattern (case-insensitive)."""
     return fnmatch.fnmatch(filename.lower(), pattern.lower())
+
+
+# Closes #37. Lazy per-share recycle-bin probe + observation-based self-correction.
+#
+# `recycle_status` is a closure-captured `dict[str, bool]` shared across every
+# tool handler in the filestation module (see modules/filestation/__init__.py
+# where it's created and threaded through the `recycle_bin_status=` kwarg).
+# Before this helper landed, the dict was created empty and never populated:
+# every share looked like recycle-on by default, so `delete_files` always told
+# the user their files were recoverable from `#recycle` even when the share
+# had recycle disabled and the data was actually gone.
+#
+# Strategy: probe lazily per share on first observation, cache the result for
+# the life of the session, and let the auth manager invalidate the cache on
+# session re-auth (since admin could toggle `#recycle` between sessions).
+#
+# Probe: `SYNO.FileStation.List` on `/{share}/#recycle/` with `limit=0`. Cheap;
+# only fetches the directory entry, not its contents.
+#   - Success                     -> recycle bin enabled  (True)
+#   - DSM 408 (path not found)    -> recycle bin disabled (False)
+#   - DSM 105 (permission denied) -> unknown; default True + WARN so the
+#       message stays optimistic about recoverability and the operator sees a
+#       diagnostic in the log
+#   - Other errors                -> unknown; same optimistic-True + WARN
+async def ensure_recycle_status(
+    client: DsmClient,
+    share_name: str,
+    recycle_status: dict[str, bool],
+) -> bool:
+    """Return cached recycle-bin status for `share_name`, probing on first call.
+
+    Caches the result in `recycle_status` in-place so subsequent calls hit the
+    dict and skip the probe. Probe failures fall back to True (the optimistic
+    default that preserves prior messaging behavior) and emit a WARNING.
+    """
+    if share_name in recycle_status:
+        return recycle_status[share_name]
+
+    try:
+        await client.request(
+            "SYNO.FileStation.List",
+            "list",
+            params={"folder_path": f"/{share_name}/#recycle", "limit": 0},
+        )
+        recycle_status[share_name] = True
+        logger.debug("Probed recycle bin on /%s: enabled", share_name)
+    except SynologyError as e:
+        if e.code == 408:
+            recycle_status[share_name] = False
+            logger.debug("Probed recycle bin on /%s: disabled (#recycle missing)", share_name)
+        elif e.code == 105:
+            # Permission denied — the bot user lacks read access on
+            # `/{share}/#recycle`. Operator-actionable: grant the MCP
+            # service account read on the recycle subfolder. We can't
+            # tell whether the bin is on or off from here; default to
+            # enabled so messaging stays optimistic.
+            recycle_status[share_name] = True
+            logger.warning(
+                "Recycle-bin probe on /%s returned DSM 105 (permission denied); "
+                "assuming enabled. Grant the MCP service account read access on "
+                "/%s/#recycle for accurate delete-files messaging.",
+                share_name,
+                share_name,
+            )
+        else:
+            recycle_status[share_name] = True
+            logger.warning(
+                "Recycle-bin probe on /%s returned DSM error %s; assuming enabled. "
+                "Delete-files messaging may be incorrect for this share until re-auth.",
+                share_name,
+                e.code,
+            )
+
+    return recycle_status[share_name]
+
+
+def correct_recycle_status_from_observation(
+    share_name: str,
+    observed_enabled: bool,
+    recycle_status: dict[str, bool],
+) -> None:
+    """Update the cache when a tool observes DSM behavior that contradicts it.
+
+    For example, `list_recycle_bin` may catch a DSM 408 ``not_found`` on
+    `/share/#recycle` when the cache says the share has recycle enabled —
+    which means the cache is stale (admin disabled it mid-session). Flip the
+    bit, log it, and let subsequent calls see the corrected state without
+    waiting for re-auth invalidation.
+    """
+    cached = recycle_status.get(share_name)
+    if cached is None or cached == observed_enabled:
+        # Nothing to correct; let `ensure_recycle_status` populate normally.
+        recycle_status.setdefault(share_name, observed_enabled)
+        return
+    logger.info(
+        "Self-correcting recycle-bin cache on /%s: cached=%s, observed=%s",
+        share_name,
+        cached,
+        observed_enabled,
+    )
+    recycle_status[share_name] = observed_enabled

src/mcp_synology/modules/filestation/listing.py

@@ -16,6 +16,8 @@
     synology_error_response,
 )
 from mcp_synology.modules.filestation.helpers import (
+    correct_recycle_status_from_observation,
+    ensure_recycle_status,
     file_type_icon,
     normalize_path,
 )
@@ -90,7 +92,16 @@ async def list_shares(
         owner = add_info.get("owner", {}).get("user", "—")
         row = [name, path, size, owner]
         if recycle_bin_status:
-            row.append("enabled" if recycle_bin_status.get(name, False) else "disabled")
+            # Distinguish unprobed-yet from observed-disabled. Pre-#37 the
+            # cache was always empty so this column never appeared at all;
+            # post-fix the cache fills lazily as `delete_files` /
+            # `list_recycle_bin` touch shares, so a `list_shares` after
+            # touching only some shares would otherwise render unprobed
+            # shares as "disabled" when their actual state is just unknown.
+            if name in recycle_bin_status:
+                row.append("enabled" if recycle_bin_status[name] else "disabled")
+            else:
+                row.append("unknown")
         rows.append(row)
 
     total = data.get("total", len(shares))
@@ -201,14 +212,20 @@ async def list_recycle_bin(
     # Normalize share name
     share_name = share.strip("/").split("/")[0]
 
-    # Check recycle bin status
-    if recycle_bin_status and not recycle_bin_status.get(share_name, True):
-        return f"Recycle bin is not enabled on /{share_name}. Deleted files cannot be recovered."
+    # Probe (or hit cache) for recycle-bin status before issuing the actual list
+    # call. Pre-#37 the cache was always empty, so this fast-path early-return
+    # never fired and every share fell into the list-and-catch-408 path below.
+    if recycle_bin_status is not None:
+        enabled = await ensure_recycle_status(client, share_name, recycle_bin_status)
+        if not enabled:
+            return (
+                f"Recycle bin is not enabled on /{share_name}. Deleted files cannot be recovered."
+            )
 
     recycle_path = f"/{share_name}/#recycle"
 
     try:
-        return await list_files(
+        result = await list_files(
             client,
             path=recycle_path,
             pattern=pattern,
@@ -226,10 +243,23 @@ async def list_recycle_bin(
             body = json.loads(str(e))
             if body.get("error", {}).get("code") == "not_found":
                 logger.debug("Recycle bin path %s not found", recycle_path)
+                # Self-correct: cache may have said enabled (or been empty);
+                # the live DSM call just contradicted it.
+                if recycle_bin_status is not None:
+                    correct_recycle_status_from_observation(
+                        share_name, observed_enabled=False, recycle_status=recycle_bin_status
+                    )
                 return (
                     f"Recycle bin is not enabled on /{share_name}. "
                     "Deleted files cannot be recovered."
                 )
         except (json.JSONDecodeError, TypeError):
             pass
         raise
+
+    # Successful list confirms recycle is enabled — self-correct if cache disagreed.
+    if recycle_bin_status is not None:
+        correct_recycle_status_from_observation(
+            share_name, observed_enabled=True, recycle_status=recycle_bin_status
+        )
+    return result

src/mcp_synology/modules/filestation/operations.py

@@ -14,6 +14,7 @@
     synology_error_response,
 )
 from mcp_synology.modules.filestation.helpers import (
+    ensure_recycle_status,
     escape_multi_path,
     normalize_path,
 )
@@ -383,19 +384,34 @@ async def delete_files(
             ),
         )
 
-    # Determine recycle bin status per share
+    # 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
+    # incorrectly reported as recycle-on. The dict is shared across all tool
+    # handlers (closure-captured in modules/filestation/__init__.py) and cleared
+    # on session re-auth (see `_register_recycle_status_invalidator` in __init__).
     shares_with_recycle: set[str] = set()
     shares_without_recycle: set[str] = set()
-    for p in normalized:
-        parts = p.split("/")
-        share_name = parts[1] if len(parts) > 1 else ""
-        if recycle_bin_status and share_name in recycle_bin_status:
-            if recycle_bin_status[share_name]:
+    if recycle_bin_status is None:
+        # Defensive: caller passed nothing. Skip probing; preserve prior
+        # default-true messaging for the touched shares.
+        for p in normalized:
+            parts = p.split("/")
+            share_name = parts[1] if len(parts) > 1 else ""
+            shares_with_recycle.add(share_name)
+    else:
+        seen: set[str] = set()
+        for p in normalized:
+            parts = p.split("/")
+            share_name = parts[1] if len(parts) > 1 else ""
+            if share_name in seen:
+                continue
+            seen.add(share_name)
+            enabled = await ensure_recycle_status(client, share_name, recycle_bin_status)
+            if enabled:
                 shares_with_recycle.add(share_name)
             else:
                 shares_without_recycle.add(share_name)
-        else:
-            shares_with_recycle.add(share_name)  # Assume recycle bin by default
 
     has_recycle = bool(shares_with_recycle)
     verb = "Deleted" if has_recycle else "Permanently deleted"