diff --git a/app/core/settings.py b/app/core/settings.py index c50da598..da093aa2 100644 --- a/app/core/settings.py +++ b/app/core/settings.py @@ -25,6 +25,8 @@ import logging import os import threading +import time +import uuid from pathlib import Path from app.core.config import ( @@ -70,16 +72,97 @@ def _default_allow_network() -> bool: return os.environ.get("STEMDECK_DESKTOP") != "1" -def _load() -> dict: +def _mirror_path() -> Path | None: + """Where the per-user copy lives, or None when the shell did not set one. + + The path comes from the shell (STEMDECK_SETTINGS_MIRROR) so the platform + logic stays in one place -- see _mirror_settings.""" + target = os.environ.get("STEMDECK_SETTINGS_MIRROR", "").strip() + return Path(target) if target else None + + +def _read_json_dict(path: Path) -> dict | None: + """Parse `path` as a JSON object. + + None means "there is nothing usable here" -- absent, unreadable, not JSON, + or JSON that is not an object. Callers that need to tell *absent* from + *unusable* must check existence themselves; that distinction is the whole + point of _load below.""" try: - data = json.loads(_SETTINGS_PATH.read_text(encoding="utf-8")) - if isinstance(data, dict): - return data + data = json.loads(path.read_text(encoding="utf-8")) except FileNotFoundError: - pass # no settings file yet — first run; use defaults + return None + except Exception: + _log.warning("could not read settings from %s", path, exc_info=True) + return None + return data if isinstance(data, dict) else None + + +def _atomic_write_json(path: Path, data: dict) -> bool: + """Write `data` to `path` so an interrupted write cannot destroy what was + there before. + + write_text() truncates first and writes second, so a process that dies in + between leaves a file that exists and does not parse -- which _load then + could not distinguish from a first run, and the next setting change + persisted a one-key file over both this and the mirror (#509). Same + same-directory temp + replace the registry already uses; the temp name is + unique per call so two concurrent writers cannot interleave on it.""" + try: + path.parent.mkdir(parents=True, exist_ok=True) + tmp = path.with_name(f"{path.name}.{uuid.uuid4().hex}.tmp") + try: + tmp.write_text(json.dumps(data), encoding="utf-8") + tmp.replace(path) + finally: + tmp.unlink(missing_ok=True) except Exception: - # Corrupt/unreadable file: fall back to defaults rather than crash. - _log.warning("could not read settings from %s", _SETTINGS_PATH, exc_info=True) + _log.warning("could not persist settings to %s", path, exc_info=True) + return False + return True + + +def _quarantine_corrupt(path: Path) -> None: + """Move an unusable settings file aside rather than leaving it to be + overwritten by the next save. + + Renaming keeps the bytes for diagnosis. Deleting or writing over them + destroys the only remaining evidence of what the user had configured.""" + try: + target = path.with_name(f"{path.name}.corrupt-{int(time.time())}") + path.replace(target) + _log.warning("settings at %s were unreadable; moved aside to %s", path, target) + except OSError: + _log.warning("could not move unreadable settings at %s aside", path, exc_info=True) + + +def _load() -> dict: + """Read settings, telling "no file yet" apart from "file we cannot read". + + Conflating the two is what lost real user settings: a torn write left an + unparsable file, this returned {} exactly as it would on a first run, and + the next set_*() then persisted a single key over both settings.json and + the mirror that existed to protect it.""" + if not _SETTINGS_PATH.exists(): + return {} # no settings file yet — genuine first run; use defaults + + data = _read_json_dict(_SETTINGS_PATH) + if data is not None: + return data + + # The file is there but unusable. Preserve it, then try the per-user copy + # the shell keeps outside the install directory. + _quarantine_corrupt(_SETTINGS_PATH) + mirror = _mirror_path() + if mirror is not None: + recovered = _read_json_dict(mirror) + if recovered: + _log.warning("recovered settings from mirror %s", mirror) + # Put them back immediately. Without this the recovery only lasts + # until the next start, which would read a now-absent primary and + # silently fall back to defaults again. + _atomic_write_json(_SETTINGS_PATH, recovered) + return recovered return {} @@ -106,11 +189,7 @@ def _save() -> bool: is still reported back, because one caller (set_jobs_dir) is coupled to something irreversible enough that silently swallowing a failure there would be actively misleading rather than merely inconvenient (#403).""" - try: - _SETTINGS_PATH.parent.mkdir(parents=True, exist_ok=True) - _SETTINGS_PATH.write_text(json.dumps(_ensure()), encoding="utf-8") - except Exception: - _log.warning("could not persist settings to %s", _SETTINGS_PATH, exc_info=True) + if not _atomic_write_json(_SETTINGS_PATH, _ensure()): return False _mirror_settings() return True @@ -135,19 +214,13 @@ def _mirror_settings() -> None: the shell (STEMDECK_SETTINGS_MIRROR) so the platform logic stays in one place and both halves cannot drift apart. """ - target = os.environ.get("STEMDECK_SETTINGS_MIRROR", "").strip() - if not target: + path = _mirror_path() + if path is None: return - try: - path = Path(target) - path.parent.mkdir(parents=True, exist_ok=True) - # Same-directory temp + replace: a torn write here would be restored - # verbatim into the user's next install. - tmp = path.with_suffix(".json.tmp") - tmp.write_text(json.dumps(_ensure()), encoding="utf-8") - tmp.replace(path) - except Exception: - _log.warning("could not mirror settings to %s", target, exc_info=True) + # Same-directory temp + replace: a torn write here would be restored + # verbatim into the user's next install. _atomic_write_json also gives the + # temp file a unique name, so two writers cannot interleave on it. + _atomic_write_json(path, _ensure()) def _num(v: object) -> int | None: diff --git a/tests/test_settings_durability.py b/tests/test_settings_durability.py new file mode 100644 index 00000000..942669e3 --- /dev/null +++ b/tests/test_settings_durability.py @@ -0,0 +1,110 @@ +"""Settings must survive a write that does not finish (#509). + +`_save()` used to call `write_text`, which truncates first and writes second. +A process that died in between left a file that existed and did not parse, and +`_load()` returned `{}` for it -- indistinguishable from a first run. The next +`set_*()` then persisted a single key over both settings.json and the mirror +that exists to protect it, so a real user lost `port` and `allow_network` from +both copies with only a warning in the log. +""" + +from __future__ import annotations + +import json +import pathlib + +from app.core import settings as _settings + + +def _read(path): + return json.loads(path.read_text(encoding="utf-8")) + + +def test_an_absent_file_is_a_first_run(tmp_path): + assert not _settings._SETTINGS_PATH.exists() + assert _settings._load() == {} + + +def test_an_unreadable_file_is_not_mistaken_for_a_first_run(tmp_path, monkeypatch): + # The distinction is the whole bug: defaults are right for a first run and + # catastrophic for a settings file we merely failed to read. + mirror = tmp_path / "shared" / "settings.json" + mirror.parent.mkdir(parents=True) + mirror.write_text(json.dumps({"port": 8081, "allow_network": True}), encoding="utf-8") + monkeypatch.setenv("STEMDECK_SETTINGS_MIRROR", str(mirror)) + + _settings._SETTINGS_PATH.write_text('{"port": 80', encoding="utf-8") # torn write + + assert _settings._load() == {"port": 8081, "allow_network": True} + + +def test_an_unreadable_file_is_kept_for_diagnosis(tmp_path): + _settings._SETTINGS_PATH.write_text('{"port": 80', encoding="utf-8") + + _settings._load() + + corrupt = list(tmp_path.glob("settings.json.corrupt-*")) + assert len(corrupt) == 1, "the unreadable bytes must not be silently destroyed" + assert corrupt[0].read_text(encoding="utf-8") == '{"port": 80' + + +def test_recovered_settings_are_written_back_immediately(tmp_path, monkeypatch): + # Recovering only into memory would last until the next start, which would + # read a now-absent primary and fall back to defaults again. + mirror = tmp_path / "shared" / "settings.json" + mirror.parent.mkdir(parents=True) + mirror.write_text(json.dumps({"port": 8081}), encoding="utf-8") + monkeypatch.setenv("STEMDECK_SETTINGS_MIRROR", str(mirror)) + _settings._SETTINGS_PATH.write_text("", encoding="utf-8") # truncated to nothing + + _settings._load() + + assert _read(_settings._SETTINGS_PATH) == {"port": 8081} + + +def test_no_mirror_and_a_corrupt_file_falls_back_to_defaults(tmp_path, monkeypatch): + monkeypatch.delenv("STEMDECK_SETTINGS_MIRROR", raising=False) + _settings._SETTINGS_PATH.write_text("not json at all", encoding="utf-8") + + assert _settings._load() == {} + + +def test_a_non_object_settings_file_is_treated_as_unusable(tmp_path, monkeypatch): + # Valid JSON, wrong shape. Returning it would make every later .get() raise. + monkeypatch.delenv("STEMDECK_SETTINGS_MIRROR", raising=False) + _settings._SETTINGS_PATH.write_text("[1, 2, 3]", encoding="utf-8") + + assert _settings._load() == {} + assert list(tmp_path.glob("settings.json.corrupt-*")) + + +def test_a_failed_write_leaves_the_previous_settings_intact(tmp_path, monkeypatch): + # The heart of it: temp + replace means an interrupted write cannot truncate + # what was already there. + # + # Only the temp write is made to fail, and monkeypatch.undo() is deliberately + # not used: the same monkeypatch instance carries conftest's _SETTINGS_PATH + # isolation, so undoing here would point the assertion at the developer's + # real settings file. + path = _settings._SETTINGS_PATH + path.write_text(json.dumps({"port": 8081, "allow_network": True}), encoding="utf-8") + + real_write_text = pathlib.Path.write_text + + def _boom(self, *a, **kw): + if self.name.endswith(".tmp"): + raise OSError("disk full") + return real_write_text(self, *a, **kw) + + monkeypatch.setattr("pathlib.Path.write_text", _boom) + + assert _settings._atomic_write_json(path, {"port": 9000}) is False + assert _read(path) == {"port": 8081, "allow_network": True} + assert not list(tmp_path.glob("*.tmp")), "a failed write must not leave a temp file" + + +def test_atomic_write_leaves_no_temp_files_behind(tmp_path): + assert _settings._atomic_write_json(_settings._SETTINGS_PATH, {"port": 8081}) is True + + assert _read(_settings._SETTINGS_PATH) == {"port": 8081} + assert not list(tmp_path.glob("*.tmp")), "temp files must not accumulate next to settings"