diff --git a/CHANGELOG.md b/CHANGELOG.md index 790fa30..e8a171f 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/src/mcp_synology/cli/__init__.py b/src/mcp_synology/cli/__init__.py index 65e0430..5ac7fd5 100644 --- a/src/mcp_synology/cli/__init__.py +++ b/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", ] diff --git a/src/mcp_synology/cli/main.py b/src/mcp_synology/cli/main.py index 6b7a5a7..237c74c 100644 --- a/src/mcp_synology/cli/main.py +++ b/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()) diff --git a/src/mcp_synology/cli/version.py b/src/mcp_synology/cli/version.py index 78636a7..d7a7cbf 100644 --- a/src/mcp_synology/cli/version.py +++ b/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") diff --git a/src/mcp_synology/server.py b/src/mcp_synology/server.py index 761664d..8586ef3 100644 --- a/src/mcp_synology/server.py +++ b/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__ diff --git a/tests/core/test_cli_version.py b/tests/core/test_cli_version.py index 8436009..94208c7 100644 --- a/tests/core/test_cli_version.py +++ b/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 ----------