diff --git a/CHANGELOG.md b/CHANGELOG.md index eb23f09..fe8433a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ ### Fixed +- **`mcp-synology setup` now writes the config file atomically** (#71) — closes #70. Cascades the atomic-write helper introduced in PR #69 (`core/fs.py::atomic_write_text`) into `cli/setup.py:178-183`, the last user-visible non-atomic write site in the project. Previously the interactive setup flow persisted the user-edited config via `config_path.write_text(header + raw_yaml, encoding="utf-8")` — same torn-write window as the runtime state files PR #69 fixed: a Ctrl+C, OOM, or power loss between the file truncate and the final write would leave a zero-byte or half-written `.yaml` that the next `mcp-synology check` / `serve` invocation would fail to parse. The window is small (single write of a small YAML payload) and the workflow is interactive, so the practical risk is much lower than the runtime-state case — but the helper was already sitting there ready to use, and closing this site means there are no remaining `path.write_text` calls in the project that persist user/runtime data. Drops the now-redundant `_CONFIG_DIR.mkdir(parents=True, exist_ok=True)` since `atomic_write_text` already creates parent directories. New `TestSetupAtomicConfigWrite::test_setup_writes_config_atomically_with_no_tmp_sibling` regression test in `tests/core/test_cli_setup.py` runs the full interactive setup flow against a not-pre-created `_CONFIG_DIR`, then asserts (a) the dir was auto-created (proves the helper ran), (b) no `.tmp` sibling lingers in the dir after a successful write, and (c) the resulting YAML starts with the generated header and contains the entered host. - **State file writes are now atomic** (#69) — closes #36. `core/state.py:save_state()` and `cli/version.py:_save_global_state()` previously called `path.write_text(...)`, which on POSIX is *not* atomic — a process kill (Claude Desktop quit, OOM, power loss) between the file truncate and the final write produces a zero-byte or half-written `state.yaml`/`global.yaml`. The next startup either fails to load it or silently falls back to default state, losing persistent state (device tokens, API version cache, update-check timestamps) and looping on re-authentication or spamming PyPI. New `core/fs.py` houses the shared `atomic_write_text(path, content, *, encoding="utf-8")` helper: it writes to a sibling `.tmp` file then `Path.replace()`s it onto the target. `os.replace` (which `Path.replace` maps to) is atomic on both POSIX and Windows, so a reader either sees the previous contents or the new contents — never a torn write. The helper also creates parent directories and best-effort cleans up the `.tmp` on rename failure (`FileNotFoundError` swallowed; other `OSError`s during cleanup logged at WARNING and the original exception is re-raised). Both `save_state` and `_save_global_state` now route through it; the per-call-site `path.parent.mkdir(parents=True, exist_ok=True)` is gone since the helper does that. Seven `TestAtomicWriteText` cases in `tests/core/test_fs.py` cover the happy path (writes content, creates missing parent dirs, overwrites existing file, no `.tmp` left behind on success, custom encoding) plus two `Path.replace`-patched cases that verify (a) the original `OSError` propagates, the `.tmp` sibling is removed, and no partial file lands at the target, and (b) an existing target file is preserved untouched when the rename fails. - **vdsm: refresh DSM search index for both setup-time and runtime-created test data** (#67) — fixes the `tests/test_integration.py::TestSearch::test_search_keyword_finds_directory` flake on vdsm CI. DSM Universal Search doesn't crawl non-indexed shares promptly on a freshly-booted vdsm, so search calls returned `0 results found` for several minutes after the data was created — well past the test's 65-second retry budget. Two-part fix: (1) `tests/vdsm/setup_dsm.py` calls `synoindex -A -d` for `/testshare/Documents` and `/testshare/Media` at golden-image build time, registering the static test data with DSM's search index. (2) New `refresh_search_index` async fixture in `tests/test_integration.py` (no-op default for real-NAS runs) and a vdsm override in `tests/vdsm/conftest.py` that invokes `synoindex -A -d` via SSH for the runtime-created `Bambu Studio` subdirectory. Without (2), the round-1 partial fix shipped via this PR's earlier head improved the test from 6-attempt fail to 4-attempt success — still flaky. With (2), search registers the new path synchronously and the test passes on attempt 1. SSH plumbing was lifted from `setup_dsm.py` into a new shared `tests/vdsm/ssh.py` module so both image-build-time and test-fixture-time SSH go through the same helper. Best-effort: a non-zero `synoindex` return logs a warning and falls back to the existing retry loop. Modifying `setup_dsm.py` invalidates the vdsm-workflow golden-image cache key, so the next CI run rebuilds the image with the fix baked in. - **Auth ignores empty / whitespace-only credentials at every read site** (#66) — closes #35. `_resolve_credentials()` in `core/auth.py` previously trusted that env / config / keyring values, if present, contained real credentials. Empty strings already fell through (Python truthiness handled them), but **whitespace-only** values like `auth: {username: " "}` mid-edit slipped through and reached `login()` as `(' ', '\t', None)`, surfacing as a generic DSM 400 that pointed neither at the config nor at the empty values. New `_present_or_none()` helper returns the value unchanged when it has any non-whitespace content, otherwise `None`. Applied at all nine read sites (3 env vars × 3 storage tiers — env / plaintext config / keyring). Meaningful padding is preserved (e.g. a real password `" pwd "` keeps its spaces); only purely empty/whitespace-only inputs are filtered. Six regression tests cover whitespace at each strategy level (env, config, keyring), the empty-string regression, the whitespace-doesn't-shadow-valid-keyring case, and the preserve-internal-padding guarantee. diff --git a/src/mcp_synology/cli/setup.py b/src/mcp_synology/cli/setup.py index 46777a6..1559a81 100644 --- a/src/mcp_synology/cli/setup.py +++ b/src/mcp_synology/cli/setup.py @@ -175,11 +175,12 @@ def _setup_interactive(verbose: bool) -> None: click.echo("Aborted.") return - # Write config file - _CONFIG_DIR.mkdir(parents=True, exist_ok=True) + # Write config file atomically (closes #70) + from mcp_synology.core.fs import atomic_write_text + raw_yaml = yaml.dump(config_dict, default_flow_style=False, sort_keys=False) header = "# Generated by mcp-synology setup\n" - config_path.write_text(header + raw_yaml, encoding="utf-8") + atomic_write_text(config_path, header + raw_yaml) click.echo(f"\nConfig written to {config_path}") # Emit Claude Desktop JSON snippet diff --git a/src/mcp_synology/core/fs.py b/src/mcp_synology/core/fs.py index 365f0b2..fb9433b 100644 --- a/src/mcp_synology/core/fs.py +++ b/src/mcp_synology/core/fs.py @@ -1,7 +1,7 @@ """Filesystem utilities used across `core/`. -Currently houses `atomic_write_text`, used by `core/state.py` and -`cli/version.py` to persist runtime state without a torn-write window. +Currently houses `atomic_write_text`, used by every site that persists +runtime state or user config without a torn-write window. """ from __future__ import annotations diff --git a/tests/core/test_cli_setup.py b/tests/core/test_cli_setup.py index daf3a31..aab2bf3 100644 --- a/tests/core/test_cli_setup.py +++ b/tests/core/test_cli_setup.py @@ -490,3 +490,58 @@ def test_linux_uses_dbus_env_when_set(self, tmp_path: Path) -> None: assert result.exit_code == 0 assert "/custom/bus" in result.output + + +# ---------- atomic config-file write (closes #70) ---------- + + +class TestSetupAtomicConfigWrite: + """Regression for #70 — interactive setup persists the config file via + `atomic_write_text` (sibling .tmp + Path.replace), not the bare + `path.write_text` that the original implementation used. Verifies the + parent dir is auto-created (atomic_write_text handles it, replacing the + explicit `_CONFIG_DIR.mkdir(...)` that the call site previously did) and + that no `.tmp` sibling lingers after a successful write. + """ + + def test_setup_writes_config_atomically_with_no_tmp_sibling(self, tmp_path: Path) -> None: + config_dir = tmp_path / "config" # intentionally not pre-created + + clean_env: dict[str, str] = { + k: val for k, val in os.environ.items() if not k.startswith("SYNOLOGY_") + } + + connect_result: dict[str, Any] = {"success": True} + + runner = CliRunner() + with ( + patch("mcp_synology.cli.setup._CONFIG_DIR", config_dir), + patch("mcp_synology.core.config.discover_config_path", side_effect=FileNotFoundError), + patch("mcp_synology.cli.setup._store_keyring", return_value=True), + patch("mcp_synology.cli.setup.asyncio.run", return_value=connect_result), + patch.dict(os.environ, clean_env, clear=True), + ): + result = runner.invoke( + main, + ["setup"], + input="192.168.1.50\nn\nread\n\nadmin\npassword\n", + ) + + assert result.exit_code == 0, f"setup failed:\n{result.output}" + + # atomic_write_text creates parents; the test deliberately did not + # pre-create config_dir, so its existence proves the helper ran. + assert config_dir.exists() and config_dir.is_dir() + + # No .tmp sibling left behind on the success path. + names = sorted(p.name for p in config_dir.iterdir()) + assert not any(n.endswith(".tmp") for n in names), ( + f"unexpected .tmp file lingered after setup: {names}" + ) + + # Exactly one YAML config file with the expected header + host. + yamls = [p for p in config_dir.iterdir() if p.suffix == ".yaml"] + assert len(yamls) == 1, f"expected exactly one yaml in config dir, got: {names}" + content = yamls[0].read_text(encoding="utf-8") + assert content.startswith("# Generated by mcp-synology setup\n") + assert "192.168.1.50" in content