Skip to content

Wire ServerState load_state / save_state lifecycle in SharedClientManager #75

Description

@cmeans-claude-dev

Summary

Surfaced as F2 on PR #73 (#37 fix) — one of two ACs from #37 that's deferred to a follow-up. Filing now so "Closes #37" on PR #73 is honest about scope.

src/mcp_synology/core/state.py defines ServerState with api_info_cache, negotiated_versions, recycle_bin_status, and a few connection metadata fields. The model exists, load_state(instance_id) / save_state(instance_id, state) are implemented and (since #69) write atomically. But:

  • src/mcp_synology/server.py:83 constructs a fresh self._server_state: ServerState = ServerState() at every SharedClientManager.__init__ and never calls load_state or save_state anywhere in src/.
  • git grep -n 'load_state\|save_state\|ServerState' src/ returns the model definition + the ServerState() construction in server.py and nothing else.

So persistence is aspirational infrastructure that has never been wired up. Across server restarts, every persisted-but-not-actually-persisted dict starts fresh: API version negotiations re-run, recycle-bin probes re-fire on first use, etc. It's not a bug (everything works correctly via lazy re-population), just wasted work and lost UX continuity.

Scope

Wire the full state lifecycle:

  1. Load on startup: SharedClientManager.__init__ (or get_client first call) calls load_state(instance_id) to populate self._server_state from disk.
  2. Save trigger: at minimum on shutdown via the existing _cleanup_session path; ideally also after notable mutations (post-probe in filestation; after query_api_info cache update). Save is already atomic (atomic_write_text from fix(state): atomic write for runtime state files #69).
  3. Bind module-owned dicts: filestation's recycle_status closure (PR fix(filestation): probe recycle-bin status per share #73) is currently a fresh dict[str, bool] = {} at module register time. Bind it to manager._server_state.recycle_bin_status so probes survive restarts.
  4. Same for api_info_cache: client.query_api_info() populates a client-side cache today; bind it to the persisted state so the SYNO.API.Info call can be skipped on warm start.
  5. Cache invalidation on re-auth: PR fix(filestation): probe recycle-bin status per share #73 already invalidates the in-memory recycle_status on session re-auth via AuthManager.add_on_reauth_callback. Confirm that this propagates correctly when the dict IS the persisted dict (i.e. clear() should also dirty the persisted state for the next save).

Why this is its own issue

Wiring is a much larger surface than #37 alone needs (every load site, every save trigger, schema-versioning if ServerState ever evolves, error handling for corrupt state files, etc.) and is genuinely orthogonal to the recycle-bin-messaging bug #37 was filed for.

Acceptance criteria

  • SharedClientManager calls load_state at startup and binds _server_state to the loaded result.
  • save_state is called on shutdown (best-effort, swallows IO errors so a failing save doesn't block process exit).
  • Filestation's recycle_status closure is bound to manager._server_state.recycle_bin_status instead of a fresh {}.
  • core/client.py's API info cache is bound to manager._server_state.api_info_cache; query_api_info is a no-op on warm start when the cache is non-empty (with a TTL or version-skew guard if appropriate).
  • Unit tests: load returns default state on missing file (already tested in test_state.py); manager startup calls load once; manager shutdown calls save; corrupt state file falls back to defaults with a WARNING.
  • Integration sanity: mcp-synology serve then kill + restart, confirm state.yaml was written and is parseable on reload.
  • CHANGELOG entry under ### Changed (this is a behavior change — first run after upgrade may take ~1 second longer to write the file; subsequent starts skip API probing).

Notes

  • ServerState already has the fields ready. No model changes expected unless we add a TTL or schema version.
  • Atomic writes (fix(state): atomic write for runtime state files #69) cover the persistence safety case.
  • This unblocks future "warm-start" optimizations across modules (e.g. share list cache, last-known-version metadata).

References

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions