From 0969335dfc9238e0a7b50b2f41f561a08b569b4f Mon Sep 17 00:00:00 2001 From: yellowcooln <12516003+yellowcooln@users.noreply.github.com> Date: Tue, 8 Sep 2026 09:29:31 -0400 Subject: [PATCH] fix(config): preserve persisted and runtime state on save failure --- repeater/config_manager.py | 128 ++++++++++++++++-------- tests/test_config_persistence_safety.py | 121 ++++++++++++++++++++++ 2 files changed, 206 insertions(+), 43 deletions(-) create mode 100644 tests/test_config_persistence_safety.py diff --git a/repeater/config_manager.py b/repeater/config_manager.py index 68c35f7..ba2c037 100644 --- a/repeater/config_manager.py +++ b/repeater/config_manager.py @@ -1,5 +1,9 @@ +import copy import logging import os +import stat +import tempfile +import threading from typing import Any, Dict, List, Optional import yaml @@ -9,6 +13,9 @@ from repeater.modem_config import normalize_modem_config_in_place logger = logging.getLogger("ConfigManager") +# Serialize staging, persistence and publication across managers in this process. +_CONFIG_WRITE_LOCK = threading.RLock() + class ConfigManager: """Manages configuration persistence and live updates to the daemon.""" @@ -236,32 +243,69 @@ class ConfigManager: return True def save_to_file(self) -> bool: - """ - Save current config to YAML file. + """Atomically save current config, normalizing shared state only on success.""" + with _CONFIG_WRITE_LOCK: + try: + candidate = copy.deepcopy(self.config) + if not self._persist_config(candidate): + return False + normalize_modem_config_in_place(self.config) + return True + except Exception: + logger.exception("Failed to save config to %s", self.config_path) + return False - Returns: - True if successful, False otherwise - """ + def _persist_config(self, config: dict) -> bool: + """Write a private candidate; the atomic replace is the commit point.""" + temporary_path = None try: - normalize_modem_config_in_place(self.config) - os.makedirs(os.path.dirname(self.config_path), exist_ok=True) - with open(self.config_path, "w", encoding="utf-8") as f: - # Use safe_dump with explicit width to prevent line wrapping - # Setting width to a very large number prevents truncation of long strings like identity keys + normalize_modem_config_in_place(config) + # Follow existing symlinks rather than replacing the link itself. + path = os.path.realpath(self.config_path) + directory = os.path.dirname(path) + os.makedirs(directory, exist_ok=True) + try: + previous = os.stat(path) + except FileNotFoundError: + previous = None + with tempfile.NamedTemporaryFile( + mode="w", encoding="utf-8", dir=directory, prefix=".config-", delete=False + ) as f: + temporary_path = f.name yaml.safe_dump( - self.config, + config, f, default_flow_style=False, indent=2, - width=1000000, # Very large width to prevent any line wrapping + width=1000000, sort_keys=False, allow_unicode=True, ) - logger.info(f"Configuration saved to {self.config_path}") + f.flush() + if previous is not None: + current = os.fstat(f.fileno()) + if (current.st_uid, current.st_gid) != (previous.st_uid, previous.st_gid): + os.fchown(f.fileno(), previous.st_uid, previous.st_gid) + os.fchmod(f.fileno(), stat.S_IMODE(previous.st_mode)) + os.fsync(f.fileno()) + os.replace(temporary_path, path) + temporary_path = None + logger.info("Configuration saved to %s", self.config_path) return True - except Exception as e: - logger.error(f"Failed to save config to {self.config_path}: {e}", exc_info=True) + except Exception: + logger.exception("Failed to save config to %s", self.config_path) return False + finally: + if temporary_path is not None: + os.unlink(temporary_path) + + @staticmethod + def _apply_updates(config: dict, updates: dict) -> None: + for section, values in updates.items(): + if isinstance(values, dict): + config.setdefault(section, {}).update(values) + else: + config[section] = values def live_update_daemon(self, sections: Optional[List[str]] = None) -> bool: """ @@ -411,39 +455,37 @@ class ConfigManager: """ result: Dict[str, Any] = {"success": False, "saved": False, "live_updated": False} - try: - # Apply updates to config - for section, values in updates.items(): - if section not in self.config: - self.config[section] = {} + with _CONFIG_WRITE_LOCK: + try: + # Stage privately so a failed write cannot leak into running state. + updates = copy.deepcopy(updates) + candidate = copy.deepcopy(self.config) + self._apply_updates(candidate, updates) + result["saved"] = self._persist_config(candidate) - if isinstance(values, dict): - self.config[section].update(values) - else: - self.config[section] = values + if not result["saved"]: + result["error"] = "Failed to save config to file" + return result - # Save to file - result["saved"] = self.save_to_file() + # Preserve shared section references used by daemon helpers. + self._apply_updates(self.config, updates) + normalize_modem_config_in_place(self.config) - if not result["saved"]: - result["error"] = "Failed to save config to file" + # Live update daemon if requested + if live_update: + # Auto-detect sections if not specified + if live_update_sections is None: + live_update_sections = list(updates.keys()) + + result["live_updated"] = self.live_update_daemon(live_update_sections) + + result["success"] = result["saved"] return result - # Live update daemon if requested - if live_update: - # Auto-detect sections if not specified - if live_update_sections is None: - live_update_sections = list(updates.keys()) - - result["live_updated"] = self.live_update_daemon(live_update_sections) - - result["success"] = result["saved"] - return result - - except Exception as e: - logger.error(f"Error in update_and_save: {e}", exc_info=True) - result["error"] = str(e) - return result + except Exception as e: + logger.error(f"Error in update_and_save: {e}", exc_info=True) + result["error"] = str(e) + return result def update_nested(self, path: str, value: Any, live_update: bool = True) -> Dict[str, Any]: """ diff --git a/tests/test_config_persistence_safety.py b/tests/test_config_persistence_safety.py new file mode 100644 index 0000000..4cd4cc7 --- /dev/null +++ b/tests/test_config_persistence_safety.py @@ -0,0 +1,121 @@ +import copy +import os +import stat +from concurrent.futures import ThreadPoolExecutor +from threading import Event +from unittest.mock import Mock + +import pytest +import yaml + +from repeater.config_manager import ConfigManager + + +@pytest.mark.parametrize("failure", ["dump", "fsync", "replace"]) +def test_failed_update_preserves_file_and_shared_config(tmp_path, monkeypatch, failure): + path = tmp_path / "config.yaml" + config = {"repeater": {"node_name": "before"}, "radio_type": "pymc_tcp"} + before = copy.deepcopy(config) + path.write_text(yaml.safe_dump(config)) + original = path.read_bytes() + section = config["repeater"] + manager = ConfigManager(str(path), config) + live = Mock() + monkeypatch.setattr(manager, "live_update_daemon", live) + + def fail(*args, **kwargs): + if failure == "dump": + args[1].write("partial: ") + assert config == before + raise OSError("synthetic write failure") + + target = "yaml.safe_dump" if failure == "dump" else f"os.{failure}" + monkeypatch.setattr(f"repeater.config_manager.{target}", fail) + result = manager.update_and_save({"repeater": {"node_name": "after"}}) + assert result["success"] is False + assert result["saved"] is False + assert result["live_updated"] is False + assert config == before + assert config["repeater"] is section + assert path.read_bytes() == original + assert list(tmp_path.iterdir()) == [path] + live.assert_not_called() + + +def test_failed_direct_save_does_not_normalize_shared_config(tmp_path, monkeypatch): + config = {"radio_type": "pymc_tcp", "pymc_tcp": {"host": "synthetic"}} + before = copy.deepcopy(config) + path = tmp_path / "config.yaml" + path.write_text(yaml.safe_dump(config)) + original = path.read_bytes() + monkeypatch.setattr("repeater.config_manager.yaml.safe_dump", Mock(side_effect=OSError())) + assert not ConfigManager(str(path), config).save_to_file() + assert config == before + assert path.read_bytes() == original + assert list(tmp_path.iterdir()) == [path] + + +@pytest.mark.parametrize("mode", [None, 0o600, 0o640]) +def test_atomic_save_permissions_and_publication(tmp_path, monkeypatch, mode): + path = tmp_path / "config.yaml" + config = {"repeater": {"node_name": "before", "other": 1}} + section = config["repeater"] + if mode is not None: + path.write_text(yaml.safe_dump(config)) + path.chmod(mode) + replace = os.replace + observed = [] + + def checked_replace(source, destination): + assert config["repeater"]["node_name"] == "before" + assert stat.S_IMODE(os.stat(source).st_mode) == (mode or 0o600) + assert os.stat(source).st_uid == os.getuid() + observed.append(True) + replace(source, destination) + + monkeypatch.setattr("repeater.config_manager.os.replace", checked_replace) + assert ConfigManager(str(path), config).update_and_save( + {"repeater": {"node_name": "after"}}, live_update=False + )["saved"] + assert observed == [True] + assert config["repeater"] is section + assert yaml.safe_load(path.read_text()) == config + assert stat.S_IMODE(path.stat().st_mode) == (mode or 0o600) + assert list(tmp_path.iterdir()) == [path] + + +def test_concurrent_managers_serialize_staging_and_saving(tmp_path, monkeypatch): + path = tmp_path / "config.yaml" + config = {"repeater": {"node_name": "before"}} + first = ConfigManager(str(path), config) + second = ConfigManager(str(path), config) + entered, release, second_started, second_dump = Event(), Event(), Event(), Event() + dump = yaml.safe_dump + + def paused_dump(data, *args, **kwargs): + if data["repeater"].get("first") and not data["repeater"].get("second"): + entered.set() + assert release.wait(5) + else: + second_dump.set() + return dump(data, *args, **kwargs) + + def second_update(): + second_started.set() + return second.update_and_save({"repeater": {"second": True}}, live_update=False) + + monkeypatch.setattr("repeater.config_manager.yaml.safe_dump", paused_dump) + with ThreadPoolExecutor(max_workers=2) as pool: + a = pool.submit(first.update_and_save, {"repeater": {"first": True}}, False) + try: + assert entered.wait(5) + b = pool.submit(second_update) + assert second_started.wait(5) + assert not second_dump.wait(0.1) + assert config == {"repeater": {"node_name": "before"}} + finally: + release.set() + assert a.result()["saved"] + assert b.result()["saved"] + assert config["repeater"] == {"node_name": "before", "first": True, "second": True} + assert yaml.safe_load(path.read_text()) == config