diff --git a/CHANGELOG.md b/CHANGELOG.md index 09238c1..78dfa43 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/src/mcp_synology/core/auth.py b/src/mcp_synology/core/auth.py index 0662fc7..04b3a6e 100644 --- a/src/mcp_synology/core/auth.py +++ b/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) diff --git a/src/mcp_synology/modules/filestation/__init__.py b/src/mcp_synology/modules/filestation/__init__.py index 165f202..5595fcb 100644 --- a/src/mcp_synology/modules/filestation/__init__.py +++ b/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: diff --git a/src/mcp_synology/modules/filestation/helpers.py b/src/mcp_synology/modules/filestation/helpers.py index 20795c8..6a36e01 100644 --- a/src/mcp_synology/modules/filestation/helpers.py +++ b/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 diff --git a/src/mcp_synology/modules/filestation/listing.py b/src/mcp_synology/modules/filestation/listing.py index 34363a2..694bd0f 100644 --- a/src/mcp_synology/modules/filestation/listing.py +++ b/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,6 +243,12 @@ 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." @@ -233,3 +256,10 @@ async def list_recycle_bin( 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 diff --git a/src/mcp_synology/modules/filestation/operations.py b/src/mcp_synology/modules/filestation/operations.py index 6dbf892..95375ee 100644 --- a/src/mcp_synology/modules/filestation/operations.py +++ b/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" diff --git a/src/mcp_synology/server.py b/src/mcp_synology/server.py index 4c43cf3..5653eed 100644 --- a/src/mcp_synology/server.py +++ b/src/mcp_synology/server.py @@ -21,6 +21,7 @@ from mcp_synology.modules import system as _system_mod if TYPE_CHECKING: + from collections.abc import Callable from types import ModuleType from mcp_synology.core.config import AppConfig @@ -84,6 +85,10 @@ def __init__(self, config: AppConfig) -> None: self._update_notice: str | None = None self._bg_task: asyncio.Task[None] | None = None self._cleanup_task: asyncio.Task[None] | None = None + # Re-auth callbacks queued by modules during sync `register()` — flushed + # to the AuthManager once it's lazily created in `get_client`. Modules + # use `subscribe_on_reauth` to register cache invalidators (see #37). + self._pending_reauth_callbacks: list[Callable[[], None]] = [] async def get_client(self) -> DsmClient: """Lazily initialize and return the DSM client.""" @@ -104,6 +109,12 @@ async def get_client(self) -> DsmClient: auth = AuthManager(self._config, client) await auth.login() + # Flush any reauth callbacks that modules queued during sync + # `register()` (before the AuthManager existed). + for cb in self._pending_reauth_callbacks: + auth.add_on_reauth_callback(cb) + self._pending_reauth_callbacks.clear() + self._client = client self._auth = auth @@ -115,6 +126,18 @@ async def get_client(self) -> DsmClient: raise RuntimeError("Client initialization failed") return self._client + def subscribe_on_reauth(self, cb: Callable[[], None]) -> None: + """Register an on-reauth callback at any point in the lifecycle. + + Safe to call from sync `register()` before the AuthManager exists — + the callback is queued and forwarded once the manager comes up. + Subsequent calls (e.g. after `get_client`) attach directly. + """ + if self._auth is not None: + self._auth.add_on_reauth_callback(cb) + else: + self._pending_reauth_callbacks.append(cb) + def with_update_notice(self, result: str) -> str: """Append update notice to tool result (first call only, then clears).""" notice = self._update_notice or "" diff --git a/tests/core/test_auth.py b/tests/core/test_auth.py index 1f385dc..9f30d3b 100644 --- a/tests/core/test_auth.py +++ b/tests/core/test_auth.py @@ -354,6 +354,101 @@ async def test_get_session_logs_in_when_no_sid(self) -> None: assert sid == "fresh-sid" +class TestOnReauthCallbacks: + """`add_on_reauth_callback` + dispatch from `_re_authenticate` (closes #37).""" + + @respx.mock + async def test_callback_fires_after_reauth(self) -> None: + """A registered callback runs once after a successful re-auth.""" + call_count = 0 + + def side_effect(request: httpx.Request) -> httpx.Response: + nonlocal call_count + call_count += 1 + params = dict(request.url.params) + if params.get("method") == "list_share" and call_count == 1: + return httpx.Response(200, json={"success": False, "error": {"code": 106}}) + if params.get("method") == "login": + return httpx.Response(200, json={"success": True, "data": {"sid": "new-sid"}}) + return httpx.Response(200, json={"success": True, "data": {"shares": []}}) + + respx.get(f"{BASE_URL}/webapi/entry.cgi").mock(side_effect=side_effect) + + invocations: list[str] = [] + + config = _make_config(auth={"username": "admin", "password": "secret"}) + async with _make_client() as client: + auth = AuthManager(config, client) + auth.add_on_reauth_callback(lambda: invocations.append("fired")) + client.sid = "old-sid" + + with ( + patch.dict(os.environ, _clean_env(), clear=True), + patch("mcp_synology.core.auth.kr", _no_keyring()), + ): + await client.request("SYNO.FileStation.List", "list_share", version=2) + + assert invocations == ["fired"] + + @respx.mock + async def test_callback_exception_is_logged_and_does_not_block_others( + self, caplog: pytest.LogCaptureFixture + ) -> None: + """A raising callback must not stop later callbacks from firing.""" + call_count = 0 + + def side_effect(request: httpx.Request) -> httpx.Response: + nonlocal call_count + call_count += 1 + params = dict(request.url.params) + if params.get("method") == "list_share" and call_count == 1: + return httpx.Response(200, json={"success": False, "error": {"code": 106}}) + if params.get("method") == "login": + return httpx.Response(200, json={"success": True, "data": {"sid": "new-sid"}}) + return httpx.Response(200, json={"success": True, "data": {"shares": []}}) + + respx.get(f"{BASE_URL}/webapi/entry.cgi").mock(side_effect=side_effect) + + invocations: list[str] = [] + + def boom() -> None: + raise RuntimeError("simulated callback failure") + + config = _make_config(auth={"username": "admin", "password": "secret"}) + async with _make_client() as client: + auth = AuthManager(config, client) + auth.add_on_reauth_callback(boom) + auth.add_on_reauth_callback(lambda: invocations.append("after-boom")) + client.sid = "old-sid" + + import logging + + with ( + patch.dict(os.environ, _clean_env(), clear=True), + patch("mcp_synology.core.auth.kr", _no_keyring()), + caplog.at_level(logging.WARNING, logger="mcp_synology.core.auth"), + ): + await client.request("SYNO.FileStation.List", "list_share", version=2) + + # Second callback fired even though the first raised. + assert invocations == ["after-boom"] + # Warning was logged about the callback failure. + warning_messages = [r.getMessage() for r in caplog.records] + assert any( + "on_reauth callback" in msg and "simulated callback failure" in msg + for msg in warning_messages + ) + + def test_callback_can_be_added_pre_reauth(self) -> None: + """Just verifies the registration API is non-coroutine and accepts a callable.""" + config = _make_config(auth={"username": "admin", "password": "secret"}) + client = _make_client() + auth = AuthManager(config, client) + auth.add_on_reauth_callback(lambda: None) + # No exception, internal list now has one entry. + assert len(auth._on_reauth_callbacks) == 1 + + class TestCredentialPriority: """Test that credential resolution follows: env > config > keyring.""" diff --git a/tests/core/test_server.py b/tests/core/test_server.py index e4df529..6c51fca 100644 --- a/tests/core/test_server.py +++ b/tests/core/test_server.py @@ -410,3 +410,80 @@ async def test_bg_update_check_swallows_errors(self) -> None: ): await manager._bg_update_check() # must not raise assert manager._update_notice is None + + +class TestSharedClientManagerSubscribeOnReauth: + """Reauth-callback subscription works pre-auth (queued) and post-auth (direct). + + Closes #37 — modules use this hook to invalidate caches that may have + drifted on the NAS between sessions (e.g. filestation's recycle-bin + probe cache). + """ + + def test_pre_auth_subscription_is_queued(self) -> None: + """Subscribing before AuthManager exists queues the callback.""" + config = make_test_config() + manager = SharedClientManager(config) + + cb = MagicMock() + manager.subscribe_on_reauth(cb) + + assert manager._auth is None + assert cb in manager._pending_reauth_callbacks + + async def test_pending_callbacks_flush_on_get_client(self) -> None: + """Once `get_client` lazily creates the AuthManager, queued callbacks + are forwarded and the pending list is cleared. + """ + config = make_test_config() + manager = SharedClientManager(config) + + cb1 = MagicMock() + cb2 = MagicMock() + manager.subscribe_on_reauth(cb1) + manager.subscribe_on_reauth(cb2) + + fake_client = MagicMock() + fake_client.__aenter__ = AsyncMock(return_value=fake_client) + fake_client.query_api_info = AsyncMock(return_value={}) + + forwarded_callbacks: list[object] = [] + fake_auth = MagicMock() + fake_auth.login = AsyncMock(return_value="sid") + fake_auth.add_on_reauth_callback = MagicMock(side_effect=forwarded_callbacks.append) + + with ( + patch("mcp_synology.server.DsmClient", return_value=fake_client), + patch("mcp_synology.server.AuthManager", return_value=fake_auth), + ): + await manager.get_client() + + # Both callbacks were forwarded to the AuthManager in the order + # they were subscribed, and the pending queue was cleared. + assert forwarded_callbacks == [cb1, cb2] + assert manager._pending_reauth_callbacks == [] + + async def test_post_auth_subscription_attaches_directly(self) -> None: + """Subscribing after AuthManager exists skips the queue.""" + config = make_test_config() + manager = SharedClientManager(config) + + fake_client = MagicMock() + fake_client.__aenter__ = AsyncMock(return_value=fake_client) + fake_client.query_api_info = AsyncMock(return_value={}) + fake_auth = MagicMock() + fake_auth.login = AsyncMock(return_value="sid") + fake_auth.add_on_reauth_callback = MagicMock() + + with ( + patch("mcp_synology.server.DsmClient", return_value=fake_client), + patch("mcp_synology.server.AuthManager", return_value=fake_auth), + ): + await manager.get_client() + + cb = MagicMock() + manager.subscribe_on_reauth(cb) + + # Attached directly — no queueing. + fake_auth.add_on_reauth_callback.assert_called_once_with(cb) + assert manager._pending_reauth_callbacks == [] diff --git a/tests/modules/filestation/test_helpers.py b/tests/modules/filestation/test_helpers.py index b001a94..f677b75 100644 --- a/tests/modules/filestation/test_helpers.py +++ b/tests/modules/filestation/test_helpers.py @@ -2,9 +2,15 @@ from __future__ import annotations +import logging +from unittest.mock import AsyncMock + import pytest +from mcp_synology.core.errors import SynologyError from mcp_synology.modules.filestation.helpers import ( + correct_recycle_status_from_observation, + ensure_recycle_status, escape_multi_path, file_type_icon, matches_pattern, @@ -176,3 +182,133 @@ def test_case_insensitive(self) -> None: def test_wildcard(self) -> None: assert matches_pattern("Severance.S02E10.mkv", "*Severance*") + + +# ---------- Recycle-bin probe + self-correct (closes #37) ---------- + + +class TestEnsureRecycleStatus: + """Lazy per-share recycle-bin probe with caching.""" + + @pytest.mark.asyncio + async def test_returns_cached_value_without_probing(self) -> None: + client = AsyncMock() + recycle_status = {"video": True, "music": False} + + result_video = await ensure_recycle_status(client, "video", recycle_status) + result_music = await ensure_recycle_status(client, "music", recycle_status) + + assert result_video is True + assert result_music is False + client.request.assert_not_called() # cache hit; no probe + + @pytest.mark.asyncio + async def test_probe_success_caches_true(self) -> None: + client = AsyncMock() + client.request.return_value = {"files": []} # any non-error response + recycle_status: dict[str, bool] = {} + + result = await ensure_recycle_status(client, "video", recycle_status) + + assert result is True + assert recycle_status == {"video": True} + client.request.assert_awaited_once_with( + "SYNO.FileStation.List", + "list", + params={"folder_path": "/video/#recycle", "limit": 0}, + ) + + @pytest.mark.asyncio + async def test_probe_dsm_408_caches_false(self) -> None: + client = AsyncMock() + client.request.side_effect = SynologyError("not found", code=408) + recycle_status: dict[str, bool] = {} + + result = await ensure_recycle_status(client, "scratch", recycle_status) + + assert result is False + assert recycle_status == {"scratch": False} + + @pytest.mark.asyncio + async def test_probe_permission_denied_falls_back_to_true_with_warning( + self, caplog: pytest.LogCaptureFixture + ) -> None: + client = AsyncMock() + client.request.side_effect = SynologyError("permission denied", code=105) + recycle_status: dict[str, bool] = {} + + with caplog.at_level(logging.WARNING, logger="mcp_synology.modules.filestation.helpers"): + result = await ensure_recycle_status(client, "admin_only", recycle_status) + + assert result is True + assert recycle_status == {"admin_only": True} + assert any( + "Recycle-bin probe on /admin_only" in r.getMessage() + and "DSM 105 (permission denied)" in r.getMessage() + and "Grant the MCP service account" in r.getMessage() + for r in caplog.records + ) + + @pytest.mark.asyncio + async def test_probe_unknown_error_falls_back_to_true_with_warning( + self, caplog: pytest.LogCaptureFixture + ) -> None: + client = AsyncMock() + client.request.side_effect = SynologyError("server confused", code=500) + recycle_status: dict[str, bool] = {} + + with caplog.at_level(logging.WARNING, logger="mcp_synology.modules.filestation.helpers"): + result = await ensure_recycle_status(client, "weird", recycle_status) + + assert result is True + assert recycle_status == {"weird": True} + assert any("DSM error 500" in r.getMessage() for r in caplog.records) + + @pytest.mark.asyncio + async def test_memoization_second_call_skips_probe(self) -> None: + """Second call against the same share must not re-issue the List request.""" + client = AsyncMock() + client.request.return_value = {"files": []} + recycle_status: dict[str, bool] = {} + + await ensure_recycle_status(client, "video", recycle_status) + await ensure_recycle_status(client, "video", recycle_status) + + # Probe ran exactly once despite two ensure calls. + client.request.assert_awaited_once() + + +class TestCorrectRecycleStatusFromObservation: + """In-place cache update when DSM behavior contradicts the cached bool.""" + + def test_disagreement_flips_cache_and_logs(self, caplog: pytest.LogCaptureFixture) -> None: + recycle_status = {"video": True} + with caplog.at_level(logging.INFO, logger="mcp_synology.modules.filestation.helpers"): + correct_recycle_status_from_observation( + "video", observed_enabled=False, recycle_status=recycle_status + ) + assert recycle_status["video"] is False + assert any( + "Self-correcting recycle-bin cache on /video" in r.getMessage() for r in caplog.records + ) + + def test_agreement_is_no_op(self, caplog: pytest.LogCaptureFixture) -> None: + recycle_status = {"video": True} + with caplog.at_level(logging.INFO, logger="mcp_synology.modules.filestation.helpers"): + correct_recycle_status_from_observation( + "video", observed_enabled=True, recycle_status=recycle_status + ) + assert recycle_status == {"video": True} + # No "Self-correcting" log when cache agrees. + assert not any( + "Self-correcting" in r.getMessage() + for r in caplog.records + if r.name == "mcp_synology.modules.filestation.helpers" + ) + + def test_missing_key_sets_default_without_warning(self) -> None: + recycle_status: dict[str, bool] = {} + correct_recycle_status_from_observation( + "scratch", observed_enabled=False, recycle_status=recycle_status + ) + assert recycle_status == {"scratch": False} diff --git a/tests/modules/filestation/test_listing.py b/tests/modules/filestation/test_listing.py index 0cbd3aa..86ec3d9 100644 --- a/tests/modules/filestation/test_listing.py +++ b/tests/modules/filestation/test_listing.py @@ -5,6 +5,7 @@ import json from typing import TYPE_CHECKING +import httpx import pytest import respx from mcp.server.fastmcp.exceptions import ToolError @@ -81,6 +82,59 @@ async def test_list_shares_with_recycle_status(self, mock_client: DsmClient) -> ) assert "enabled" in result + @respx.mock + async def test_list_shares_renders_unknown_for_unprobed_shares( + self, mock_client: DsmClient + ) -> None: + """F3 regression (#73 round 1): once the cache fills lazily, the + Recycle Bin column appears — but shares that haven't been probed + must render as "unknown", not falsely as "disabled". Otherwise + list_shares is *worse* than pre-#37 (which simply omitted the + column when the dict was empty) because partial correctness has + the appearance of full correctness. + """ + respx.get(f"{BASE_URL}/webapi/entry.cgi").respond( + json={ + "success": True, + "data": { + "shares": [ + { + "name": "video", + "path": "/video", + "isdir": True, + "additional": {"size": {"total_size": 0}, "owner": {"user": "admin"}}, + }, + { + "name": "music", + "path": "/music", + "isdir": True, + "additional": {"size": {"total_size": 0}, "owner": {"user": "admin"}}, + }, + { + "name": "scratch", + "path": "/scratch", + "isdir": True, + "additional": {"size": {"total_size": 0}, "owner": {"user": "admin"}}, + }, + ], + "total": 3, + }, + } + ) + # /video has been probed (enabled); /scratch has been probed (disabled); + # /music is NOT in the cache — must render as unknown, not disabled. + result = await list_shares( + mock_client, + recycle_bin_status={"video": True, "scratch": False}, + ) + # Tabular output — find the per-row recycle-bin cell. Order in shares + # is video, music, scratch; the column is the last one in each row. + # We can't anchor on column position alone (table renderer pads), so + # assert presence of the three distinct values. + assert "enabled" in result + assert "disabled" in result + assert "unknown" in result + @respx.mock async def test_list_shares_empty(self, mock_client: DsmClient) -> None: respx.get(f"{BASE_URL}/webapi/entry.cgi").respond( @@ -245,3 +299,101 @@ async def _raise_non_json(*args: object, **kwargs: object) -> str: monkeypatch.setattr(listing, "list_files", _raise_non_json) with pytest.raises(ToolError, match="plain text error"): await list_recycle_bin(mock_client, share="video") + + @respx.mock + async def test_self_correct_when_dsm_disagrees_with_cache(self, mock_client: DsmClient) -> None: + """Closes #37: if cache says recycle-on but DSM returns 408 on the actual + list, list_recycle_bin should flip the cache to False so subsequent + delete_files calls in the same session emit the correct messaging. + """ + # First the lazy probe in `ensure_recycle_status` returns success + # (the list call against /share/#recycle?limit=0 succeeds), so the + # cache stays True. Then the FULL list (no limit=0) returns 408 — + # contradicting the probe's view. + call_count = 0 + + def side_effect(request: httpx.Request) -> httpx.Response: + nonlocal call_count + call_count += 1 + params = dict(request.url.params) + # Probe: limit=0 → success (recycle path exists at this moment) + if params.get("limit") == "0": + return httpx.Response( + 200, json={"success": True, "data": {"files": [], "total": 0}} + ) + # Subsequent full list: simulate the recycle dir disappeared + return httpx.Response(200, json={"success": False, "error": {"code": 408}}) + + respx.get(f"{BASE_URL}/webapi/entry.cgi").mock(side_effect=side_effect) + + recycle_status = {"video": True} # stale cached value + result = await list_recycle_bin( + mock_client, + share="video", + recycle_bin_status=recycle_status, + ) + + assert "not enabled" in result + # Cache was self-corrected from True to False. + assert recycle_status == {"video": False} + + @respx.mock + async def test_self_correct_when_observed_enabled_disagrees_with_cache( + self, mock_client: DsmClient + ) -> None: + """Inverse self-correct: cached False, DSM list succeeds → flip to True. + + Triggered when admin enabled the recycle bin mid-session after the + cache was populated with False. + """ + + def side_effect(request: httpx.Request) -> httpx.Response: + params = dict(request.url.params) + # The early ensure_recycle_status fast-path returns the cached + # False without probing, so we never reach a probe call here — + # the test SHOULDN'T enter this side_effect for the early-return + # path. But if recycle_bin_status[share] is False, the early + # return fires WITHOUT calling DSM, so this side_effect is unused + # in that case. + # + # To exercise self-correct-on-success, we simulate cache==True by + # NOT pre-populating, then return success. + if params.get("limit") == "0": + return httpx.Response( + 200, json={"success": True, "data": {"files": [], "total": 0}} + ) + return httpx.Response( + 200, + json={ + "success": True, + "data": { + "files": [ + { + "name": "old.mkv", + "isdir": False, + "additional": { + "size": 100, + "time": {"mtime": 1700000000}, + }, + } + ], + "total": 1, + }, + }, + ) + + respx.get(f"{BASE_URL}/webapi/entry.cgi").mock(side_effect=side_effect) + + # Empty cache; ensure_recycle_status will probe (success → True), + # full list also succeeds, self-correct is a no-op since cache and + # observation agree. This test exercises the "agreement no-op" + # branch on the success path of list_recycle_bin. + recycle_status: dict[str, bool] = {} + result = await list_recycle_bin( + mock_client, + share="video", + recycle_bin_status=recycle_status, + ) + + assert "old.mkv" in result + assert recycle_status == {"video": True} diff --git a/tests/modules/filestation/test_operations.py b/tests/modules/filestation/test_operations.py index fcdaa65..273a9b6 100644 --- a/tests/modules/filestation/test_operations.py +++ b/tests/modules/filestation/test_operations.py @@ -219,6 +219,46 @@ async def test_delete_multiple_shares(self, mock_client: DsmClient) -> None: assert "enabled" in result assert "NOT enabled" in result + @respx.mock + async def test_delete_lazily_probes_when_share_missing_from_cache( + self, mock_client: DsmClient + ) -> None: + """Closes #37: an empty recycle_bin_status dict triggers the per-share + probe via ensure_recycle_status. Pre-#37 the dict was always empty AND + nothing populated it, so every delete reported recycle-on. Now the + delete path probes lazily — a 408 on `/share/#recycle` flips messaging + to the permanent-delete variant for that share. + """ + + def side_effect(request: httpx.Request) -> httpx.Response: + params = dict(request.url.params) + api = params.get("api") + method = params.get("method") + # Probe call: SYNO.FileStation.List on `/scratch/#recycle` → 408 + if api == "SYNO.FileStation.List" and method == "list": + if params.get("folder_path") == "/scratch/#recycle": + return httpx.Response(200, json={"success": False, "error": {"code": 408}}) + return httpx.Response( + 200, json={"success": True, "data": {"files": [], "total": 0}} + ) + # Otherwise fall through to the standard async-task fixture. + return _async_task_side_effect()(request) + + respx.get(f"{BASE_URL}/webapi/entry.cgi").mock(side_effect=side_effect) + + recycle_status: dict[str, bool] = {} + result = await delete_files( + mock_client, + paths=["/scratch/temp.bin"], + recycle_bin_status=recycle_status, + ) + + assert "Permanently deleted" in result + assert "NOT enabled" in result + # Probe result was cached so a subsequent delete in the same share + # would not re-probe. + assert recycle_status == {"scratch": False} + class TestBackgroundTaskErrors: """Error paths shared across CopyMove and Delete background tasks.