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

[cmeans-claude-dev]

Summary

Closes #37. Replaces the always-empty recycle_status: dict[str, bool] = {} at modules/filestation/__init__.py:238 with a lazy per-share probe that populates the cache on first observation. Pre-fix, delete_files always reported "Recycle bin is enabled on /{share}" regardless of actual DSM state — confusing UX for users with #recycle disabled on a share, since their data was actually permanently gone.

Design

Lazy probe — ensure_recycle_status(client, share, recycle_status) in modules/filestation/helpers.py:

• Cache hit → return cached bool
• Cache miss → SYNO.FileStation.List on /{share}/#recycle with limit=0 (cheap, just probes existence)
• Success → True, DSM 408 (path not found) → False
• DSM 105 (permission denied) or any other error → True + WARNING log (preserves prior optimistic-default behavior; surfaces unknown states in logs for operator visibility)
• Result cached in-place for the session lifetime

Self-correct on observation — correct_recycle_status_from_observation updates the cache when list_recycle_bin sees DSM behavior contradicting the cached value (cached True but the actual list returns 408 → flip to False; or cache says False then the live call succeeds → flip to True). Logs at INFO. Means subsequent delete_files calls in the same session see corrected state without waiting for re-auth.

Invalidate on re-auth — Two-layer hook:

• AuthManager.add_on_reauth_callback(cb) registers callbacks, fired after _re_authenticate succeeds. Exceptions in callbacks are logged (WARNING) and don't block other callbacks.
• SharedClientManager.subscribe_on_reauth(cb) proxies subscriptions made before the AuthManager is lazily created (sync register() time) and flushes them on first get_client.
• Filestation register() subscribes recycle_status.clear so admin-side #recycle toggles between sessions are picked up after the next session-error-driven re-auth.

list_shares left alone — renders whatever's cached at the moment; not a correctness path. Kept cheap.

Out of scope (intentional)

• Persistence: ServerState.recycle_bin_status exists on the model but load_state/save_state aren't currently wired up at all in SharedClientManager. Wiring persistence touches a much larger surface than recycle_status passed to delete_files is always empty — incorrect recycle-bin messaging #37 alone needs. Per signoff, treating as a separate follow-up.
• vdsm integration test: synoshare --setopt recycle-toggle reliability on DSM 7.2.x is unproven (PR Fix all 5 remaining vdsm test failures (42/47 → 47/47) #23 reverted a similar setopt for share creation). Per signoff, unit tests carry the regression load until vdsm behavior is verified — vdsm follow-up will land separately.

Acceptance criteria

• Module init / first-call lazy init probes each configured share for #recycle existence and caches the result. (Probes lazily on first observation per share, not at init.)
• Result is passed into delete_files() so the correct with-recycle vs NOT-enabled message fires per-share. (Plus the same on list_recycle_bin.)
• Cache invalidation strategy documented. (Clear-on-reauth + self-correct-on-observation.)
• Unit test: a share with #recycle: False produces the permanent-delete message; with True produces the recoverable message. (Existing tests + new lazy-probe test in TestDeleteFiles.)
• Integration test against vdsm verifying real DSM recycle-bin state matches. (Out of scope per signoff; follow-up.)
• CHANGELOG ### Fixed entry.

QA

Manual tests

1. • Run uv run pytest — 546 pass, 96.10% coverage (gate: 95%).
2. • Run uv run ruff check src/ tests/ scripts/ — clean.
3. • Run uv run ruff format --check src/ tests/ scripts/ — clean.
4. • Run uv run mypy src/ scripts/ — strict-mode clean.
5. • Spot-check git grep -n 'recycle_status\b\|recycle_bin_status\b' src/ shows the closure dict reaches delete_files, list_recycle_bin, and list_shares (the last unchanged), with no remaining # Assume recycle bin by default fall-through branch on the populated path.
6. • Spot-check git grep -n 'subscribe_on_reauth\|on_reauth_callbacks' src/ shows the AuthManager API + SharedClientManager proxy + filestation register() subscription are all wired together.
7. • Run uv run pytest tests/modules/filestation/test_helpers.py::TestEnsureRecycleStatus tests/modules/filestation/test_helpers.py::TestCorrectRecycleStatusFromObservation -v — 9 pass; covers cache hit, probe-success, probe-408, probe-105+WARN, probe-other+WARN, memoization, and the three self-correct cases.

New test classes

Test class	File	Cases
TestEnsureRecycleStatus	tests/modules/filestation/test_helpers.py	6 (cache hit, success-True, 408-False, 105+WARN, other-error+WARN, memoization)
TestCorrectRecycleStatusFromObservation	tests/modules/filestation/test_helpers.py	3 (disagreement flips, agreement no-op, missing-key sets default)
TestOnReauthCallbacks	tests/core/test_auth.py	3 (callback fires after re-auth, exception in one doesn't block others, registration API works pre-reauth)
TestSharedClientManagerSubscribeOnReauth	tests/core/test_server.py	3 (pre-auth queues, pending flushes on get_client, post-auth attaches directly)
Added to TestDeleteFiles	tests/modules/filestation/test_operations.py	1 (lazy probe on missing share, with respx-driven 408)
Added to TestListRecycleBin	tests/modules/filestation/test_listing.py	2 (self-correct on disagreement, self-correct no-op on agreement)

Verification I already ran

Check	Result
uv run pytest	546 passed, 94 deselected, 96.10% coverage
uv run ruff check src/ tests/ scripts/	clean
uv run ruff format --check src/ tests/ scripts/	clean
uv run mypy src/ scripts/	clean (30 files, strict-mode)

🤖 Generated with Claude Code

[codecov-commenter]

Codecov Report

❌ Patch coverage is 92.75362% with 5 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/mcp_synology/modules/filestation/operations.py	64.28%	5 Missing ⚠️

📢 Thoughts on this report? Let us know!

[cmeans]

QA Round 1 — Three blockers, two observations

The design is thoughtful (lazy probe + observation-based self-correct + invalidate-on-reauth) and the 18 new tests are well-shaped — happy paths, error codes, memoization, dispatch-exception isolation, pre-AuthManager queue, all covered. Code reads cleanly and threading of recycle_status through __init__.py (3 sites) → helpers.py → operations.py/listing.py is correct per git grep. Lock + callback dispatch in _re_authenticate is structurally sound.

That said, three blockers stop signoff — one CI, one issue-scope, one regression I caught while re-reading list_shares.

Verification

Check	Result
uv run pytest	546 passed (+18 from #72), 94 deselected, 96.10% coverage
New test classes (TestEnsureRecycleStatus, TestCorrectRecycleStatusFromObservation, TestOnReauthCallbacks, TestSharedClientManagerSubscribeOnReauth)	15/15 pass directly
uv run ruff check src/ tests/ scripts/	clean
uv run ruff format --check src/ tests/ scripts/	72 files already formatted
uv run mypy src/ scripts/	clean (30 files, strict-mode)
git grep -n 'recycle_status\b|recycle_bin_status\b' src/	matches PR body — 3 callsites in __init__.py thread the closure dict to delete_files/list_recycle_bin/list_shares, helper signatures consistent
git grep -n 'subscribe_on_reauth|on_reauth_callbacks' src/	AuthManager API + dispatch loop + SharedClientManager proxy + filestation register() subscription all present and wired
Required CI on 1f19bc54	vdsm FAILED (see F1); other 12 green

PR-body manual tests 1–7 all verified and checked.

Findings

ID	Finding
F1 (blocker — CI gate)	vdsm integration tests failed on tests/vdsm/test_vdsm_integration.py::TestSearch::test_search_keyword_finds_directory. Search for Bambu in /testshare returned 0 results across all 6 retry attempts (~75s). The synoindex fixture from PR #67 IS firing per the captured log (synoindex registered /volume1/testshare/Documents/Bambu Studio with DSM search index), but DSM Universal Search hadn't returned the result within the budget. PR #73 does not touch any vdsm test, search code, or the auth-on-init flow (the new on-reauth callback list is only consulted on re-auth, not initial login), so this is almost certainly a re-surfacing of the same flake #67 was meant to root-cause. Per the project's CI gate rule any failed required check is automatic QA Failed; first action is just gh workflow run vdsm.yml --ref fix/recycle-status-probe (or push an empty commit) to confirm transient. If it persists across two consecutive runs, that's a real signal the #67 mitigation isn't sufficient on this branch and needs a follow-up — possibly extending the retry budget past 75s, or adding a synchronous "search service ready" probe before the test fires. Not blaming this PR; just can't sign off on a red required check.
F2 (substantive)	Issue-scope drift. PR body says "Closes #37" but two of the issue's six ACs are deferred to follow-ups — the vdsm integration test (AC#5) and the persistence wiring (ServerState.recycle_bin_status, mentioned as "out of scope" in the design section). I searched gh issue list --search recycle and gh issue list --search vdsm recycle: the only open issue is #37 itself. No follow-up tickets exist for either deferral, and I couldn't locate a "signoff" record in awareness for the scope reduction. Two paths: (a) file follow-up issues now and link them in the PR body so "Closes #37" becomes honest (everything tracked somewhere), or (b) downgrade to "Refs #37" in the PR body and the merge commit so the issue stays open until the deferred work lands. (a) is the project's existing pattern — see the cycle that filed #70 from the F2 of PR #69.
F3 (substantive — UX regression)	list_shares "Recycle Bin" column shows misleading partial truth post-fix. At listing.py:83-95: the column is added when recycle_bin_status is truthy, and unknown shares default to False via recycle_bin_status.get(name, False). Pre-fix the dict was always empty so if recycle_bin_status: was always False and the column never appeared — never misleading. Post-fix the dict fills lazily; once any one share has been probed (e.g. via delete_files /video/x), the cache is non-empty so list_shares adds the column, and every other share — none of which have been probed — renders as "disabled" with no indication that the value isn't observed. Concrete user flow: delete_files /video/foo → list_shares → shows /video correctly + /music, /home, /photo, etc. all falsely as "disabled". The PR body's "list_shares left alone — renders whatever's cached at the moment; not a correctness path" is the design rationale, but the result is worse than the pre-fix behavior because partial correctness has the appearance of full correctness. Cleanest fix is at listing.py:95: render the state when the share is in the cache, otherwise an explicit unknown indicator (e.g. "?" or "unknown"). One-line change: row.append("enabled" if recycle_bin_status.get(name, False) else "disabled" if name in recycle_bin_status else "?"). Add one test in tests/modules/filestation/test_listing.py::TestListShares covering "share not in cache → ? rendered".

Observations (each blocks signoff per the project's observations-block rule)

ID	Observation
O1	helpers.py:191-192 file-header comment claims DSM 105 (permission denied) -> unknown; default True + WARN as a specific case, but the implementation at helpers.py:217-228 doesn't branch on 105 at all — both 105 and any other DSM error code fall through the same generic else arm. The behavior IS correct (105 lands in the right bucket); only the comment over-promises specificity. Either drop the 105 line from the file-header inventory, or add elif e.code == 105 for cosmetic alignment with the documented contract.
O2	The on-reauth callback dispatch loop in core/auth.py:240-249 runs INSIDE self._lock. The add_on_reauth_callback docstring says callbacks must be cheap and synchronous — but only convention enforces this. Worth a one-line inline comment in the dispatch loop noting that callbacks block the auth lock so future contributors don't add an expensive sync hook here. The current recycle_status.clear is O(n_shares) and trivial; this is forward-looking maintenance hygiene, not a present-day correctness issue.

Verdict

QA Failed Round 1. F1 is the simplest fix path (re-run, see whether it sticks) and the most ambiguous on what counts as "Dev's responsibility" since the test isn't in this PR's diff. F2 is procedural (file two follow-up issues, optionally relink in PR body). F3 is a single-line correctness fix plus one regression test — the meaningful blocker on the actual diff. O1+O2 are small.

[cmeans]

Applying QA Failed as the final act of round 1. F1 (vdsm CI red — likely flake re-surfacing), F2 (issue scope: 2 deferred ACs need follow-up issues filed before "Closes #37" is honest), F3 (list_shares partial-truth UX regression on the Recycle Bin column once cache fills). O1 + O2 are smaller. Local stack 546/546 pass, ruff/mypy clean — the failure is in CI vdsm + the things I caught reading the diff.

[cmeans-claude-dev]

Round 2 — F2/F3/O1/O2 addressed; F1 awaiting CI re-run

All four code-side findings closed in commit 616ea3c. F2 (issue scope) addressed by filing two follow-up issues. F1 (vdsm flake) is a transient-flake hypothesis — the round-2 push re-triggers the vdsm workflow; if it goes green this run, F1 closes itself.

F1 — vdsm test_search_keyword_finds_directory flake

PR diff still doesn't touch vdsm tests, search code, or initial-login auth flow. The push of 616ea3c re-runs the required check; reporting back when the workflow finishes. If it fails again on this branch, we have a real signal that #67's mitigation isn't enough on this PR (suspected: search-service warmup variance under the new test load) and I'll open a follow-up to extend the retry budget or add a synchronous "search ready" probe.

F2 — Issue-scope follow-ups filed

Two new tracking issues so "Closes #37" is honest about scope:

• Wire ServerState load_state / save_state lifecycle in SharedClientManager #75 — Wire ServerState load_state / save_state lifecycle in SharedClientManager. Captures the persistence-wiring deferral. ACs include: load on startup, save on shutdown, bind recycle_status and api_info_cache to persisted state, corrupt-state-file fallback, integration-restart sanity test.
• vdsm integration test for recycle-bin messaging in delete_files #76 — vdsm integration test for recycle-bin messaging in delete_files. Captures the AC#5 deferral. Scope explicitly probes synoshare --setopt reliability on DSM 7.2.2 first (since PR Fix all 5 remaining vdsm test failures (42/47 → 47/47) #23 reverted a similar setopt call) before committing to the implementation path.

Both filed under cmeans-claude-dev[bot] via activate.sh. PR body's "Closes #37" is now accurate — anything not landed here is tracked on a real ticket.

F3 — list_shares partial-truth UX regression fixed

listing.py:95 previously rendered "disabled" for any share missing from the cache. Now distinguishes:

```python
if name in recycle_bin_status:
row.append("enabled" if recycle_bin_status[name] else "disabled")
else:
row.append("unknown")
```

So a list_shares after only some shares have been probed shows the observed state on the probed shares and "unknown" on the rest, rather than falsely homogenizing everything to "disabled". Pre-#37 the column was simply absent (dict was always empty so the if recycle_bin_status: gate never opened); the new behavior preserves that "no false claims" property while still surfacing the values we DO know.

New regression test TestListShares::test_list_shares_renders_unknown_for_unprobed_shares sets up a 3-share fixture with /video probed-True, /scratch probed-False, /music never probed → asserts all three values (enabled, disabled, unknown) appear in the rendered output.

O1 — helpers.py 105 branch alignment

Chose the "add elif e.code == 105" path over "drop the 105 line from the docstring" because permission-denied is operator-actionable in a way the generic-else case isn't — the new WARNING log points at the actual fix (Grant the MCP service account read access on /<share>/#recycle) instead of the generic "may be incorrect for this share until re-auth." Updated TestEnsureRecycleStatus::test_probe_permission_denied_falls_back_to_true_with_warning to assert the new specific message; the generic-else test (test_probe_unknown_error_falls_back_to_true_with_warning against code 500) still covers the catch-all arm.

O2 — auth.py lock-discipline note

Added an inline NOTE: comment at the on-reauth dispatch loop in _re_authenticate warning future contributors that callbacks fire inside self._lock and must remain cheap and synchronous — anything heavy here would block every coroutine waiting to re-auth. Today's only subscriber (recycle_status.clear, O(n_shares)) is well within budget; the note is forward-looking.

Verification

Check	Result
uv run pytest	547 passed (+1 from F3 regression test), 94 deselected, 96.11% coverage
uv run ruff check src/ tests/ scripts/	clean
uv run ruff format --check src/ tests/ scripts/	72 files already formatted
uv run mypy src/ scripts/	clean (30 files, strict-mode)

Ready for round 2 — pending the F1 re-run verdict.

[cmeans]

QA Round 2 — PASS

All five round-1 items closed.

F1 — vdsm CI

Re-ran on 616ea3c and the vdsm integration tests job is green. test_search_keyword_finds_directory was indeed a transient flake unrelated to this PR — the diff doesn't touch any vdsm/search/auth-init path so the round-1 failure was the same #67-targeted flake re-surfacing once. If it recurs on a future PR, that's a follow-up for #67's mitigation, not this branch.

F2 — issue scope

Both deferred ACs now have follow-up issues filed by Dev with explicit cross-reference to #73's F2:

• #75 — Wire ServerState load_state / save_state lifecycle in SharedClientManager (the persistence wiring out-of-scope item).
• #76 — vdsm integration test for recycle-bin messaging in delete_files (the AC#5 deferral).

Closes #37 is now honest because the deferred work is tracked somewhere durable.

F3 — list_shares partial-truth regression

Fixed at listing.py:94-104 with the recommended approach: name in recycle_bin_status → render enabled/disabled, otherwise unknown. Three-state output instead of two-state-with-misleading-default. The new TestListShares::test_list_shares_renders_unknown_for_unprobed_shares regression seeds a cache with {video: True, scratch: False} against three rendered shares (video, music, scratch) and asserts all three values (enabled, disabled, unknown) appear in the output — covers the exact scenario from the round-1 finding. Passes locally.

O1 — DSM 105 specificity

Resolved with an explicit elif e.code == 105 branch at helpers.py:221-232. The new operator-actionable WARNING is significantly better than the generic round-1 message — it names the resolution path: "Grant the MCP service account read access on //#recycle for accurate delete-files messaging." The updated test asserts both the DSM-105 marker and the actionable-fix substring. Code now matches the file-header inventory.

O2 — auth-lock dispatch contract

Five-line inline comment added at core/auth.py:246-250 making the cheap-and-synchronous contract visible at the call site. Names today's only subscriber (recycle_status.clear, O(n_shares)) and points future contributors at "deferred task" if they need to do heavy work. Cheapest possible mitigation; sufficient.

Verification (re-run on 616ea3c)

Check	Result
uv run pytest	547 passed (+1 from R1: the new test_list_shares_renders_unknown_for_unprobed_shares), 94 deselected, 96.11% coverage
uv run ruff check src/ tests/ scripts/	clean
uv run ruff format --check src/ tests/ scripts/	72 files already formatted
uv run mypy src/ scripts/	clean (30 files, strict-mode)
Targeted F3 + O1 tests	both pass
Required CI on 616ea3c	13/13 green (vdsm completed SUCCESS)

Verdict

Ready for QA Signoff. F1 confirmed flake, F2 tracked in #75 + #76, F3/O1/O2 all closed in code with regression coverage. Final maintainer call.

[cmeans]

Applying Ready for QA Signoff as the final act of round 2. All five round-1 items closed: F1 transient flake (vdsm green on 616ea3c), F2 tracked in #75 + #76, F3/O1/O2 closed in code with regression coverage. 547/547 pass, 96.11% coverage, ruff/mypy clean, 13/13 required CI green. Final maintainer call.

[cmeans]

LGTM