fix(cli): lock global.yaml read-modify-write (closes #93)

CHANGELOG.md

@@ -12,6 +12,8 @@
 
 ### Fixed
 
+- **Lock the read-modify-write sequence on `~/.local/state/mcp-synology/global.yaml`** (#101) — closes #93, also closes the deferred 8th item from #45. `atomic_write_text` (#69) prevents torn writes, but two callers can each load the file, mutate distinct keys, and save atomically — and the later writer's save loses the earlier writer's update because each computed its new state from a stale read. The fix is a `_with_global_state_lock()` context manager (Unix `fcntl.flock(LOCK_EX)`; no-op on Windows per #93's "single-user dev box" risk surface) wrapped around every load → mutate → save site: the three flag handlers and the startup version-tracking block in `cli/main.py`, the auto-upgrade-trigger save in `cli/main.py`, the `_do_auto_upgrade` and `_do_revert` post-subprocess saves in `cli/version.py`, and the background update-task save in `server.py`. Both `_do_auto_upgrade` and `_do_revert` now re-load fresh state under the lock at save time rather than mutating the pre-subprocess copy, so the lock never spans a `subprocess.run`. `_do_auto_upgrade` lost its now-unused `state` parameter as a result. The background update task in `server.py` runs the full load → check → save inside `run_in_executor` so the synchronous `fcntl.flock` never spans an `await` — holding a sync lock across an awaited point would be a deadlock footgun for any future async caller of `_with_global_state_lock` on the same event loop (e.g. #75's `ServerState` lifecycle). New regression test `TestGlobalStateLock::test_concurrent_writers_preserve_both_updates` runs two threads each incrementing a distinct counter 50 times under the lock; both counters reach exactly 50 (without the lock, lost updates leave at least one below 50). 605 unit tests pass at 96.22% coverage.
+
 - **`create_folder` correctly handles multi-path inputs on real DSM 7.x** (#97) — closes #95. Same root-cause family as #68 (delete + getinfo) and #84 (move/copy): DSM 7.x's `SYNO.FileStation.CreateFolder create` does not honor the documented comma-joined multi-path format — a request with `name=a,b` and `folder_path=/X,/X` is treated as a single literal name, creating one mangled folder while the tool reports `Created 1 folder(s)`. QA reproducer: `create_folder(paths=['/share/__cftest_a', '/share/__cftest_b'])` produced a single folder named `__cftest_a,__cftest_b` at the share root instead of two folders. Per-path multipath was missed when the broader array-flattening fix landed for delete/copy/move; `create_folder` was the last unfixed write tool in the family. Fix matches that pattern: one DSM CreateFolder call per input path inside `create_folder`, aggregating `folders[]` from each response. `escape_multi_path` is no longer called there. Per-path serial means N round-trips for N paths, which is fine for typical small-N usage and trivially correct. New `TestCreateFolder::test_multipath_uses_per_path_serial_calls` asserts (a) N create requests for N paths, (b) each request carries a single bare `name` and a single `folder_path` parent (no commas), and (c) the aggregated response names every input folder. The unit-test gap for create_folder multipath is closed by this test. 600 unit tests pass at 96.25% coverage.
 
 - **Tidy-up bundle: seven small items from the 2026-04-16 review** (#92) — addresses 7 of 8 items on #45.

src/mcp_synology/cli/__init__.py

@@ -13,6 +13,7 @@
     _check_for_update,
     _load_global_state,
     _save_global_state,
+    _with_global_state_lock,
 )
 
 __all__ = [
@@ -21,5 +22,6 @@
     "_load_global_state",
     "_save_global_state",
     "_store_keyring",
+    "_with_global_state_lock",
     "main",
 ]

src/mcp_synology/cli/main.py

@@ -17,6 +17,7 @@
     _get_current_version,
     _load_global_state,
     _save_global_state,
+    _with_global_state_lock,
 )
 
 _PYPI_PACKAGE = "mcp-synology"
@@ -49,9 +50,10 @@ def main(
 ) -> None:
     """mcp-synology — MCP server for Synology NAS."""
     if check_update:
-        state = _load_global_state()
-        latest = _check_for_update(state, force=True)
-        _save_global_state(state)
+        with _with_global_state_lock():
+            state = _load_global_state()
+            latest = _check_for_update(state, force=True)
+            _save_global_state(state)
         current = _get_current_version()
         if latest:
             click.echo(f"Update available: {current} -> {latest}")
@@ -67,9 +69,10 @@ def main(
         ctx.exit()
 
     if auto_upgrade is not None:
-        state = _load_global_state()
-        state["auto_upgrade"] = auto_upgrade == "enable"
-        _save_global_state(state)
+        with _with_global_state_lock():
+            state = _load_global_state()
+            state["auto_upgrade"] = auto_upgrade == "enable"
+            _save_global_state(state)
         status = "enabled" if state["auto_upgrade"] else "disabled"
         click.echo(f"Auto-upgrade {status}.")
         ctx.exit()
@@ -79,21 +82,24 @@ def main(
         ctx.exit()
 
     # Track version changes for --revert
-    state = _load_global_state()
-    current = _get_current_version()
-    last_known = state.get("running_version")
-    if last_known and last_known != current:
-        state["previous_version"] = last_known
-    state["running_version"] = current
-    _save_global_state(state)
+    with _with_global_state_lock():
+        state = _load_global_state()
+        current = _get_current_version()
+        last_known = state.get("running_version")
+        if last_known and last_known != current:
+            state["previous_version"] = last_known
+        state["running_version"] = current
+        _save_global_state(state)
 
     # Auto-upgrade check — only on interactive commands, not serve
     # (serve is launched by Claude Desktop; upgrading mid-launch is risky)
     if state.get("auto_upgrade") and ctx.invoked_subcommand != "serve":
-        latest = _check_for_update(state)
-        _save_global_state(state)
+        with _with_global_state_lock():
+            fresh = _load_global_state()
+            latest = _check_for_update(fresh)
+            _save_global_state(fresh)
         if latest:
-            _do_auto_upgrade(state)
+            _do_auto_upgrade()
 
     if ctx.invoked_subcommand is None:
         click.echo(ctx.get_help())

src/mcp_synology/cli/version.py

@@ -2,15 +2,20 @@
 
 from __future__ import annotations
 
+import contextlib
 import json as json_mod
 import logging
 import re
 import subprocess
+import sys
 from datetime import UTC, datetime
 from pathlib import Path
-from typing import Any
+from typing import TYPE_CHECKING, Any
 from urllib.request import Request, urlopen
 
+if TYPE_CHECKING:
+    from collections.abc import Iterator
+
 import click
 import yaml
 
@@ -94,9 +99,13 @@ def _detect_installer() -> str | None:
     return None
 
 
+def _global_state_path() -> Path:
+    return Path.home() / ".local" / "state" / "mcp-synology" / "global.yaml"
+
+
 def _load_global_state() -> dict[str, Any]:
     """Load global (non-instance) state for version tracking."""
-    path = Path.home() / ".local" / "state" / "mcp-synology" / "global.yaml"
+    path = _global_state_path()
     if not path.exists():
         return {}
     try:
@@ -110,9 +119,39 @@ def _save_global_state(state: dict[str, Any]) -> None:
     """Save global state atomically."""
     from mcp_synology.core.fs import atomic_write_text
 
-    path = Path.home() / ".local" / "state" / "mcp-synology" / "global.yaml"
     raw_text = yaml.dump(state, default_flow_style=False, sort_keys=False)
-    atomic_write_text(path, "# Auto-generated by mcp-synology.\n" + raw_text)
+    atomic_write_text(_global_state_path(), "# Auto-generated by mcp-synology.\n" + raw_text)
+
+
+@contextlib.contextmanager
+def _with_global_state_lock() -> Iterator[None]:
+    """Hold an exclusive lock around a load → mutate → save sequence on global.yaml.
+
+    Save-side atomicity (`atomic_write_text` from PR #69) prevents torn writes,
+    but the read-modify-write sequence has no inter-process synchronization. Two
+    callers can each load the file, mutate distinct keys, then save atomically —
+    and the later writer's update overwrites the earlier writer's because each
+    computed its new state from a stale read. The lock spans the whole critical
+    section so callers (not the save function) hold it.
+
+    Windows lacks `fcntl`; fall through with no locking. The practical risk
+    surface is single-user dev boxes (per #93).
+    """
+    if sys.platform == "win32":
+        yield
+        return
+
+    import fcntl
+
+    lock_dir = _global_state_path().parent
+    lock_dir.mkdir(parents=True, exist_ok=True)
+    lock_path = lock_dir / "global.yaml.lock"
+    with lock_path.open("a+") as fh:
+        fcntl.flock(fh.fileno(), fcntl.LOCK_EX)
+        try:
+            yield
+        finally:
+            fcntl.flock(fh.fileno(), fcntl.LOCK_UN)
 
 
 def _check_for_update(state: dict[str, Any], *, force: bool = False) -> str | None:
@@ -150,7 +189,7 @@ def _check_for_update(state: dict[str, Any], *, force: bool = False) -> str | No
     return None
 
 
-def _do_auto_upgrade(state: dict[str, Any]) -> bool:
+def _do_auto_upgrade() -> bool:
     """Upgrade to the latest version. Returns True on success."""
     installer = _detect_installer()
     current = _get_current_version()
@@ -169,8 +208,10 @@ def _do_auto_upgrade(state: dict[str, Any]) -> bool:
     click.echo(f"Upgrading {_PYPI_PACKAGE} via {installer}...")
     result = subprocess.run(cmd, capture_output=True, text=True)
     if result.returncode == 0:
-        state["previous_version"] = current
-        _save_global_state(state)
+        with _with_global_state_lock():
+            state = _load_global_state()
+            state["previous_version"] = current
+            _save_global_state(state)
         click.echo(click.style("Upgrade complete — takes effect on next run.", fg="green"))
         return True
     click.echo(click.style(f"Upgrade failed: {result.stderr.strip()}", fg="red"), err=True)
@@ -215,9 +256,11 @@ def _do_revert(target_version: str | None = None) -> None:
     click.echo(f"Reverting from {current} to {prev}...")
     result = subprocess.run(cmd, capture_output=True, text=True)
     if result.returncode == 0:
-        state["previous_version"] = None
-        state["auto_upgrade"] = False
-        _save_global_state(state)
+        with _with_global_state_lock():
+            fresh = _load_global_state()
+            fresh["previous_version"] = None
+            fresh["auto_upgrade"] = False
+            _save_global_state(fresh)
         click.echo(click.style(f"Reverted to {prev}. Takes effect on next run.", fg="green"))
         click.echo("Auto-upgrade has been disabled to prevent immediately re-upgrading.")
         click.echo(f"Re-enable with: {_PYPI_PACKAGE} --auto-upgrade enable")

src/mcp_synology/server.py

@@ -202,18 +202,31 @@ async def _bg_update_check(self) -> None:
                 _check_for_update,
                 _load_global_state,
                 _save_global_state,
+                _with_global_state_lock,
             )
 
             loop = asyncio.get_running_loop()
-            gstate = _load_global_state()
+
             # Run the blocking PyPI check in a thread, bounded so a stuck
             # executor (slow socket, slow YAML parse, hung disk) can't
             # keep this coroutine alive forever. The inner urlopen has
             # its own 5s timeout; the 10s outer bound is a safety margin
             # for everything else _check_for_update does.
+            #
+            # The full load → check → save sequence runs inside the executor
+            # so the synchronous fcntl.flock never spans an `await`. Holding
+            # a sync lock across an await would be a deadlock footgun for any
+            # future async caller of `_with_global_state_lock` on this event
+            # loop (e.g. #75's ServerState lifecycle).
+            def _check_under_lock() -> str | None:
+                with _with_global_state_lock():
+                    gstate = _load_global_state()
+                    result = _check_for_update(gstate)
+                    _save_global_state(gstate)
+                    return result
+
             async with asyncio.timeout(10):
-                latest = await loop.run_in_executor(None, _check_for_update, gstate)
-            _save_global_state(gstate)
+                latest = await loop.run_in_executor(None, _check_under_lock)
             if latest:
                 from mcp_synology import __version__
 

tests/core/test_cli_version.py

@@ -174,6 +174,51 @@ def test_save_then_load_round_trips(self, tmp_path: Path) -> None:
             assert v._load_global_state() == original
 
 
+# ---------- _with_global_state_lock ----------
+
+
+class TestGlobalStateLock:
+    def test_concurrent_writers_preserve_both_updates(self, tmp_path: Path) -> None:
+        """Regression test for #93 — load/mutate/save under the lock prevents lost updates.
+
+        Two threads each increment a distinct counter 50 times. Without the lock,
+        interleaved load/save sequences cause lost updates and at least one
+        counter ends below 50. With the lock, both counters reach exactly 50.
+        """
+        import threading
+
+        with patch("pathlib.Path.home", return_value=tmp_path):
+            v._save_global_state({"counter_a": 0, "counter_b": 0})
+
+            def writer(key: str) -> None:
+                for _ in range(50):
+                    with v._with_global_state_lock():
+                        state = v._load_global_state()
+                        state[key] = state.get(key, 0) + 1
+                        v._save_global_state(state)
+
+            t1 = threading.Thread(target=writer, args=("counter_a",))
+            t2 = threading.Thread(target=writer, args=("counter_b",))
+            t1.start()
+            t2.start()
+            t1.join()
+            t2.join()
+
+            final = v._load_global_state()
+
+        assert final["counter_a"] == 50
+        assert final["counter_b"] == 50
+
+    def test_lock_creates_state_dir_if_missing(self, tmp_path: Path) -> None:
+        """Lockfile location is `.../mcp-synology/global.yaml.lock`; the parent
+        dir must be created on first use even if no save has happened yet."""
+        with patch("pathlib.Path.home", return_value=tmp_path), v._with_global_state_lock():
+            pass
+        lock_dir = tmp_path / ".local" / "state" / "mcp-synology"
+        assert lock_dir.is_dir()
+        assert (lock_dir / "global.yaml.lock").exists()
+
+
 # ---------- _check_for_update ----------
 
 
@@ -279,49 +324,48 @@ def _fake_completed(returncode: int, stderr: str = "") -> subprocess.CompletedPr
         )
 
     def test_uv_installer_success(self, tmp_path: Path) -> None:
-        state: dict[str, Any] = {}
         with (
             patch("pathlib.Path.home", return_value=tmp_path),
             patch("mcp_synology.cli.version._detect_installer", return_value="uv"),
             patch("mcp_synology.cli.version._get_current_version", return_value="0.5.0"),
             patch("subprocess.run", return_value=self._fake_completed(0)) as run,
         ):
-            assert v._do_auto_upgrade(state) is True
+            assert v._do_auto_upgrade() is True
+            saved = v._load_global_state()
         cmd = run.call_args.args[0]
         assert cmd[:3] == ["uv", "tool", "install"]
         assert "mcp-synology@latest" in cmd
-        assert state["previous_version"] == "0.5.0"
+        assert saved["previous_version"] == "0.5.0"
 
     def test_pipx_installer_success(self, tmp_path: Path) -> None:
-        state: dict[str, Any] = {}
         with (
             patch("pathlib.Path.home", return_value=tmp_path),
             patch("mcp_synology.cli.version._detect_installer", return_value="pipx"),
             patch("mcp_synology.cli.version._get_current_version", return_value="0.5.0"),
             patch("subprocess.run", return_value=self._fake_completed(0)),
         ):
-            assert v._do_auto_upgrade(state) is True
-        assert state["previous_version"] == "0.5.0"
+            assert v._do_auto_upgrade() is True
+            saved = v._load_global_state()
+        assert saved["previous_version"] == "0.5.0"
 
-    def test_unknown_installer_returns_false(self) -> None:
-        state: dict[str, Any] = {}
+    def test_unknown_installer_returns_false(self, tmp_path: Path) -> None:
         with (
+            patch("pathlib.Path.home", return_value=tmp_path),
             patch("mcp_synology.cli.version._detect_installer", return_value=None),
             patch("mcp_synology.cli.version._get_current_version", return_value="0.5.0"),
         ):
-            assert v._do_auto_upgrade(state) is False
-        assert "previous_version" not in state
+            assert v._do_auto_upgrade() is False
+            assert "previous_version" not in v._load_global_state()
 
     def test_subprocess_failure_returns_false(self, tmp_path: Path) -> None:
-        state: dict[str, Any] = {}
         with (
             patch("pathlib.Path.home", return_value=tmp_path),
             patch("mcp_synology.cli.version._detect_installer", return_value="uv"),
             patch("mcp_synology.cli.version._get_current_version", return_value="0.5.0"),
             patch("subprocess.run", return_value=self._fake_completed(1, stderr="boom")),
         ):
-            assert v._do_auto_upgrade(state) is False
-        assert "previous_version" not in state
+            assert v._do_auto_upgrade() is False
+            assert "previous_version" not in v._load_global_state()
 
 
 # ---------- _validate_version_string ----------