Merge origin/main into claude/remove-skins-and-base-classes

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

test/test_background_data_service.py

@@ -297,3 +297,71 @@ class TestGetBackgroundService:
         shutdown_background_service()
         with pytest.raises(ValueError):
             get_background_service()
+
+
+# ---------------------------------------------------------------------------
+# Sport cache keys
+# ---------------------------------------------------------------------------
+
+class _FrozenDatetime:
+    """Stands in for the datetime class at a fixed instant. now() without a
+    tz answers in a UTC-4 local zone, so a key built from local time shows."""
+
+    def __init__(self, moment):
+        self._moment = moment
+
+    def now(self, tz=None):
+        from datetime import timedelta, timezone
+        if tz is None:
+            return self._moment.astimezone(timezone(timedelta(hours=-4))).replace(tzinfo=None)
+        return self._moment.astimezone(tz)
+
+
+class TestSportCacheKey:
+    """get_sport_cache_key() must produce CacheManager's key format without
+    building a CacheManager (config load + cache-dir probing) to do it."""
+
+    @pytest.mark.parametrize("sport,date_str", [
+        ("nfl", "20260922"), ("ncaa_fb", "20251231"), ("soccer", ""),
+    ])
+    def test_explicit_date_matches_cache_manager(self, service, sport, date_str):
+        from src.cache_manager import CacheManager
+        expected = CacheManager.generate_sport_cache_key(None, sport, date_str)
+        assert service.get_sport_cache_key(sport, date_str) == expected
+
+    def test_default_date_is_today_in_utc_like_cache_manager(self, service):
+        from datetime import datetime, timezone
+        import src.cache_manager as cm_module
+        # 23:30 on the 21st in New York is already the 22nd in UTC.
+        frozen = _FrozenDatetime(datetime(2026, 9, 22, 3, 30, tzinfo=timezone.utc))
+        with patch.object(bds_module, "datetime", frozen), \
+                patch.object(cm_module, "datetime", frozen):
+            ours = service.get_sport_cache_key("nba")
+            theirs = cm_module.CacheManager.generate_sport_cache_key(None, "nba")
+        assert ours == theirs == "nba_20260922"
+
+    def test_does_not_construct_a_cache_manager(self, service):
+        with patch("src.cache_manager.CacheManager.__init__",
+                   side_effect=AssertionError("CacheManager constructed")):
+            assert service.get_sport_cache_key("nhl", "20260101") == "nhl_20260101"
+            assert service.get_sport_cache_key("nhl").startswith("nhl_")
+
+    def test_submit_without_cache_key_looks_up_todays_sport_key(
+            self, service, mock_cache_manager):
+        mock_cache_manager.get.return_value = {"events": []}
+        with patch("src.cache_manager.CacheManager.__init__",
+                   side_effect=AssertionError("CacheManager constructed")):
+            service.submit_fetch_request("mlb", 2026, "http://example.invalid/x")
+        key = mock_cache_manager.get.call_args[0][0]
+        assert key == service.get_sport_cache_key("mlb")
+
+
+class TestPriorityIsAcceptedAndIgnored:
+    def test_priority_keyword_is_accepted(self, service, mock_cache_manager):
+        mock_cache_manager.get.return_value = {"cached": True}
+        rid = service.submit_fetch_request(
+            "nfl", 2026, "http://example.invalid/x", cache_key="k", priority=5)
+        assert service.get_result(rid).cached is True
+
+    def test_statistics_still_report_an_empty_queue(self, service):
+        assert service.get_statistics()["queue_size"] == 0

test/test_cache_manager_memory_tier.py

@@ -0,0 +1,118 @@
+"""CacheManager's memory tier is MemoryCache's, not a copy of it.
+
+CacheManager used to re-implement MemoryCache.cleanup() line for line and
+read the component's private dicts and lock through aliases bound at
+construction. Those aliases went stale the moment the component was replaced
+(tests do exactly that), and the listing of the cache *directory* held the
+memory tier's lock for the whole scan.
+"""
+
+import os
+import time
+from unittest.mock import patch
+
+import pytest
+
+from src.cache.memory_cache import MemoryCache
+from src.cache_manager import CacheManager
+
+
+@pytest.fixture
+def cm(tmp_path):
+    with patch('src.cache_manager.CacheManager._get_writable_cache_dir',
+               return_value=str(tmp_path)):
+        manager = CacheManager()
+    # The disk sweep thread stats this directory too; keep it out of the
+    # os.stat spies below.
+    manager.stop_cleanup_thread()
+    yield manager
+
+
+def test_cleanup_and_stats_follow_a_replaced_component(cm):
+    """Replace the component the way test_cache_ttl_honoured does; cleanup and
+    stats must act on the new one, not on dicts captured at construction."""
+    cm._memory_cache_component = MemoryCache(max_size=7, cleanup_interval=11.0)
+    cm._memory_cache_component.set("fresh", {"v": 1})
+    cm._memory_cache_component.set("stale", {"v": 2})
+    cm._memory_cache_component._timestamps["stale"] = time.time() - 4000
+
+    assert cm._cleanup_memory_cache(force=True) == 1
+    assert cm._memory_cache_component.get("stale") is None
+    assert cm._memory_cache_component.get("fresh") == {"v": 1}
+
+    stats = cm.get_memory_cache_stats()
+    assert stats["size"] == 1
+    assert stats["max_size"] == 7
+    assert stats["cleanup_interval"] == 11.0
+    assert stats["usage_percent"] == pytest.approx(100 / 7)
+
+
+def test_periodic_cleanup_is_throttled_and_records_its_run(cm):
+    mem = cm._memory_cache_component
+    mem.set("stale", {"v": 1})
+    mem._timestamps["stale"] = time.time() - 4000
+
+    # Within the interval: nothing runs, even through the get path.
+    assert cm._cleanup_memory_cache() == 0
+    assert mem.size() == 1
+
+    mem._last_cleanup = time.time() - mem._cleanup_interval - 1
+    before = time.time()
+    cm.get_cached_data("missing")          # triggers the periodic sweep
+    assert mem.size() == 0
+    assert cm.get_memory_cache_stats()["last_cleanup"] >= before
+
+
+def test_stats_have_the_documented_shape(cm):
+    cm.set("k", {"v": 1})
+    stats = cm.get_memory_cache_stats()
+    assert set(stats) == {"size", "max_size", "usage_percent",
+                          "last_cleanup", "cleanup_interval"}
+    assert stats["size"] == 1
+    assert stats["max_size"] == cm._memory_cache_component.max_size()
+
+
+def test_listing_the_cache_dir_does_not_hold_the_memory_lock(cm, tmp_path):
+    """8,864 files on a real rig: every get/set used to wait out the scan."""
+    for name in ("a", "b"):
+        (tmp_path / f"{name}.json").write_text("{}")
+    (tmp_path / "notes.txt").write_text("x")
+
+    lock = cm._memory_cache_component._lock
+    held_during_stat = []
+    real_stat = os.stat
+
+    def spying_stat(path, *args, **kwargs):
+        held_during_stat.append(lock.locked())
+        return real_stat(path, *args, **kwargs)
+
+    with patch('src.cache_manager.os.stat', side_effect=spying_stat):
+        files = cm.list_cache_files()
+
+    assert held_during_stat and not any(held_during_stat)
+    assert sorted(f["key"] for f in files) == ["a", "b"]
+
+
+def test_listing_skips_a_file_deleted_mid_scan(cm, tmp_path):
+    for name in ("a", "b"):
+        (tmp_path / f"{name}.json").write_text("{}")
+    real_stat = os.stat
+
+    def vanishing_stat(path, *args, **kwargs):
+        if str(path).endswith("a.json"):
+            raise FileNotFoundError(path)
+        return real_stat(path, *args, **kwargs)
+
+    with patch('src.cache_manager.os.stat', side_effect=vanishing_stat):
+        files = cm.list_cache_files()
+
+    assert [f["key"] for f in files] == ["b"]
+
+
+def test_listing_is_newest_first(cm, tmp_path):
+    now = time.time()
+    for i, name in enumerate(("old", "mid", "new")):
+        p = tmp_path / f"{name}.json"
+        p.write_text("{}")
+        os.utime(p, (now - 300 + i * 100, now - 300 + i * 100))
+    assert [f["key"] for f in cm.list_cache_files()] == ["new", "mid", "old"]

test/test_cache_strategy_intervals.py

@@ -0,0 +1,136 @@
+"""CacheStrategy intervals, pinned across the whole input grid.
+
+The strategy table used to carry a per-sport defaults dict whose every value
+was 60, and a soccer branch identical to its else. These tests pin the
+returned strategy for every data type x sport key x config shape, so
+simplifying the lookup cannot change what any caller gets back. They were
+written against the pre-cleanup code and pass on it unchanged, except for
+the legacy `<sport>_scoreboard` config shape (see below), which that code
+still read.
+"""
+
+import pytest
+
+from src.cache.cache_strategy import CacheStrategy
+
+
+class _Cfg:
+    def __init__(self, config):
+        self.config = config
+
+
+class _NoConfigAttr:
+    pass
+
+
+# Plugin config sections are keyed by plugin id. Their intervals belong to the
+# plugin, and the strategy table has never read them.
+_PLUGIN_ID_CONFIG = {
+    pid: {"live_update_interval": 5, "recent_update_interval": 7,
+          "upcoming_update_interval": 9}
+    for pid in ("football-scoreboard", "basketball-scoreboard",
+                "baseball-scoreboard", "hockey-scoreboard", "soccer-scoreboard")
+}
+
+# `<sport>_scoreboard` sections come from the built-in scoreboards the plugin
+# system replaced. An install upgraded from that era can still carry them in
+# config.json (nothing deletes them). No current caller passes a sport key to
+# the strategy, but a stale section must not steer cache TTLs if one does.
+_LEGACY_SCOREBOARD_CONFIG = {
+    f"{sport}_scoreboard": {"live_update_interval": 5,
+                            "recent_update_interval": 7,
+                            "upcoming_update_interval": 9}
+    for sport in ("nfl", "nba", "mlb", "nhl", "soccer", "ncaa_fb",
+                  "ncaa_baseball", "ncaam_basketball", "milb")
+}
+
+CONFIG_MANAGERS = {
+    "no_config_manager": None,
+    "empty_config": _Cfg({}),
+    "plugin_id_config": _Cfg(_PLUGIN_ID_CONFIG),
+    "legacy_scoreboard_config": _Cfg(_LEGACY_SCOREBOARD_CONFIG),
+    "config_is_none": _Cfg(None),
+    "config_is_not_a_dict": _Cfg("x"),
+    "config_manager_without_config": _NoConfigAttr(),
+}
+
+SPORT_KEYS = [None, "", "nfl", "nba", "mlb", "nhl", "soccer", "ncaa_fb",
+              "ncaa_baseball", "ncaam_basketball", "milb",
+              "football-scoreboard", "curling"]
+
+
+def _fixed(max_age, memory_ttl, **extra):
+    return {"max_age": max_age, "memory_ttl": memory_ttl,
+            "force_refresh": False, **extra}
+
+
+DEFAULT = _fixed(300, 600)
+FIXED = {
+    "weather_current": _fixed(300, 600),
+    "stocks": _fixed(600, 1200, market_hours_only=True),
+    "crypto": _fixed(300, 600),
+    "sports_recent": _fixed(1800, 3600),
+    "sports_upcoming": _fixed(10800, 21600),
+    "sports_schedules": _fixed(86400, 172800),
+    "leaderboard": _fixed(604800, 1209600),
+    "news": _fixed(3600, 7200),
+    "odds": _fixed(1800, 3600),
+    "odds_live": _fixed(120, 240),
+    "team_info": _fixed(604800, 1209600),
+    "logos": _fixed(2592000, 5184000),
+    "default": DEFAULT,
+}
+
+
+def _expected(data_type, sport_key):
+    if data_type in ("live_scores", "sports_live"):
+        if sport_key:
+            interval = 60
+        else:
+            interval = 15 if data_type == "live_scores" else 30
+        return {"max_age": interval, "memory_ttl": interval * 2,
+                "force_refresh": True}
+    return FIXED.get(data_type, DEFAULT)
+
+
+@pytest.mark.parametrize("cm_name", sorted(CONFIG_MANAGERS))
+def test_live_interval_is_60_for_every_sport(cm_name):
+    strategy = CacheStrategy(config_manager=CONFIG_MANAGERS[cm_name])
+    for sport_key in SPORT_KEYS:
+        assert strategy.get_sport_live_interval(sport_key) == 60, sport_key
+
+
+@pytest.mark.parametrize("cm_name", sorted(CONFIG_MANAGERS))
+def test_strategy_table_for_every_data_type_and_sport(cm_name):
+    strategy = CacheStrategy(config_manager=CONFIG_MANAGERS[cm_name])
+    data_types = ["live_scores", "sports_live", *FIXED, "unknown", ""]
+    for data_type in data_types:
+        for sport_key in SPORT_KEYS:
+            got = strategy.get_cache_strategy(data_type, sport_key)
+            assert got == _expected(data_type, sport_key), (data_type, sport_key)
+
+
+@pytest.mark.parametrize("key", [
+    "soccer_live", "soccer_current", "soccer_scoreboard", "SOCCER_LIVE",
+    "nfl_live", "live", "hockey_current", "nba_live_scores",
+])
+def test_live_keys_including_soccer_are_sports_live(key):
+    assert CacheStrategy().get_data_type_from_key(key) == "sports_live"
+
+
+@pytest.mark.parametrize("key,data_type", [
+    ("odds_soccer_live", "odds_live"),
+    ("odds_x", "odds"),
+    ("weather", "weather_current"),
+    ("crypto_stock", "crypto"),
+    ("stock", "stocks"),
+    ("news_soccer", "news"),
+    ("soccer_schedule", "sports_schedules"),
+    ("soccer_recent", "sports_recent"),
+    ("soccer_upcoming", "sports_upcoming"),
+    ("soccer_logo", "team_info"),
+    ("soccer", "default"),
+    ("", "default"),
+])
+def test_non_live_keys_keep_their_data_type(key, data_type):
+    assert CacheStrategy().get_data_type_from_key(key) == data_type

test/test_config_durable_writes.py

@@ -0,0 +1,347 @@
+"""
+Every write of config.json / config_secrets.json goes through one durable
+writer, atomic_write_text() in src/config_manager_atomic.py: temp file in the
+same directory, fsync, rename, directory fsync.
+
+These pin what that buys on a device that loses power mid-save (the old file
+survives intact), what it costs the SD card (the unchanged secrets file isn't
+rewritten, backup rotation doesn't open every backup), and that the backups
+it keeps are still the five config.json.backup.<version> files anything
+restoring from them expects.
+"""
+
+import json
+import os
+import re
+import stat
+import threading
+from pathlib import Path
+
+import pytest
+
+import src.config_manager_atomic as atomic_module
+from src.config_manager import ConfigManager
+from src.config_manager_atomic import AtomicConfigManager, SaveResultStatus, atomic_write_text
+from src.exceptions import ConfigError
+
+ORIGINAL = {"timezone": "America/Chicago", "display": {"hardware": {"rows": 32}}}
+
+POSIX_ONLY = pytest.mark.skipif(os.name == 'nt', reason="POSIX file modes and directory fsync")
+
+
+def make_manager(tmp_path, secrets=None):
+    config_file = tmp_path / "config.json"
+    config_file.write_text(json.dumps(ORIGINAL, indent=4))
+    secrets_file = tmp_path / "config_secrets.json"
+    if secrets is not None:
+        secrets_file.write_text(json.dumps(secrets, indent=4))
+    manager = ConfigManager(config_path=str(config_file), secrets_path=str(secrets_file))
+    manager.template_path = str(tmp_path / "no-template.json")
+    return manager
+
+
+def temp_leftovers(directory):
+    return [p.name for p in Path(directory).iterdir() if '.tmp.' in p.name]
+
+
+class TestPowerCutLeavesTheOldConfig:
+    """A save that dies before the rename must leave config.json exactly as it was."""
+
+    def test_save_config_that_fails_midway_keeps_the_old_file(self, tmp_path):
+        # json.dump streams: an unencodable value deep in the dict used to
+        # leave config.json truncated at the point the encoder gave up.
+        manager = make_manager(tmp_path)
+        before = (tmp_path / "config.json").read_bytes()
+
+        with pytest.raises(ConfigError):
+            manager.save_config({"timezone": "UTC", "zzz": object()})
+
+        assert (tmp_path / "config.json").read_bytes() == before
+        assert temp_leftovers(tmp_path) == []
+
+    def test_save_config_that_dies_before_the_rename_keeps_the_old_file(self, tmp_path, monkeypatch):
+        manager = make_manager(tmp_path)
+        before = (tmp_path / "config.json").read_bytes()
+
+        def power_cut(*args, **kwargs):
+            raise OSError("power lost")
+
+        monkeypatch.setattr(atomic_module.os, "replace", power_cut)
+
+        with pytest.raises(ConfigError):
+            manager.save_config({"timezone": "UTC"})
+
+        assert (tmp_path / "config.json").read_bytes() == before
+        assert temp_leftovers(tmp_path) == []
+
+    def test_save_raw_file_content_that_dies_before_the_rename_keeps_the_old_file(self, tmp_path, monkeypatch):
+        manager = make_manager(tmp_path)
+        before = (tmp_path / "config.json").read_bytes()
+
+        def power_cut(*args, **kwargs):
+            raise OSError("power lost")
+
+        monkeypatch.setattr(atomic_module.os, "replace", power_cut)
+
+        with pytest.raises(ConfigError):
+            manager.save_raw_file_content("main", {"timezone": "UTC"})
+
+        assert (tmp_path / "config.json").read_bytes() == before
+        assert temp_leftovers(tmp_path) == []
+
+    def test_save_config_atomic_that_dies_before_the_rename_keeps_the_old_file(self, tmp_path, monkeypatch):
+        manager = make_manager(tmp_path)
+        before = json.loads((tmp_path / "config.json").read_text())
+
+        real_replace = os.replace
+
+        def power_cut(src, dst, *args, **kwargs):
+            if Path(dst).name == "config.json":
+                raise OSError("power lost")
+            return real_replace(src, dst, *args, **kwargs)
+
+        monkeypatch.setattr(atomic_module.os, "replace", power_cut)
+
+        result = manager.save_config_atomic({"timezone": "UTC"}, create_backup=False)
+
+        assert result.status == SaveResultStatus.FAILED
+        assert json.loads((tmp_path / "config.json").read_text()) == before
+        assert temp_leftovers(tmp_path) == []
+
+
+class TestTheDataIsOnDiskBeforeTheRename:
+    def test_the_temp_file_is_fsynced_before_it_replaces_the_config(self, tmp_path, monkeypatch):
+        events = []
+        real_fsync, real_replace = os.fsync, os.replace
+
+        def fsync(fd):
+            events.append("fsync")
+            return real_fsync(fd)
+
+        def replace(src, dst, *args, **kwargs):
+            events.append("replace")
+            return real_replace(src, dst, *args, **kwargs)
+
+        monkeypatch.setattr(atomic_module.os, "fsync", fsync)
+        monkeypatch.setattr(atomic_module.os, "replace", replace)
+
+        make_manager(tmp_path).save_config({"timezone": "UTC"})
+
+        assert "replace" in events
+        assert "fsync" in events[:events.index("replace")]
+        assert json.loads((tmp_path / "config.json").read_text()) == {"timezone": "UTC"}
+
+    @POSIX_ONLY
+    def test_the_directory_is_fsynced_after_the_rename(self, tmp_path, monkeypatch):
+        events = []
+        real_fsync, real_replace, real_open = os.fsync, os.replace, os.open
+        directory_fds = set()
+
+        def open_(path, flags, *args, **kwargs):
+            fd = real_open(path, flags, *args, **kwargs)
+            if Path(path) == tmp_path:
+                directory_fds.add(fd)
+            return fd
+
+        def fsync(fd):
+            events.append("dir-fsync" if fd in directory_fds else "fsync")
+            return real_fsync(fd)
+
+        def replace(src, dst, *args, **kwargs):
+            events.append("replace")
+            return real_replace(src, dst, *args, **kwargs)
+
+        monkeypatch.setattr(atomic_module.os, "open", open_)
+        monkeypatch.setattr(atomic_module.os, "fsync", fsync)
+        monkeypatch.setattr(atomic_module.os, "replace", replace)
+
+        atomic_write_text(tmp_path / "config.json", "{}")
+
+        assert events == ["fsync", "replace", "dir-fsync"]
+
+
+class TestPermissionsSurviveTheRename:
+    @POSIX_ONLY
+    def test_config_and_secrets_get_their_shared_modes_before_the_rename(self, tmp_path, monkeypatch):
+        # mkstemp creates 0600. If the chmod came after the rename, the other
+        # service could open the new file in between and be refused.
+        seen = {}
+        real_replace = os.replace
+
+        def replace(src, dst, *args, **kwargs):
+            seen[Path(dst).name] = stat.S_IMODE(os.stat(src).st_mode)
+            return real_replace(src, dst, *args, **kwargs)
+
+        monkeypatch.setattr(atomic_module.os, "replace", replace)
+
+        manager = make_manager(tmp_path, secrets={"weather": {"api_key": "k"}})
+        manager.save_raw_file_content("main", {"timezone": "UTC"})
+        manager.save_raw_file_content("secrets", {"weather": {"api_key": "new"}})
+
+        assert seen == {"config.json": 0o644, "config_secrets.json": 0o640}
+        assert stat.S_IMODE(os.stat(tmp_path / "config.json").st_mode) == 0o644
+        assert stat.S_IMODE(os.stat(tmp_path / "config_secrets.json").st_mode) == 0o640
+
+    @POSIX_ONLY
+    def test_a_directory_named_secrets_does_not_lock_down_config_json(self, tmp_path):
+        # get_config_file_mode() looks for "secrets" anywhere in the string it
+        # is given; handed the full path, an install under e.g.
+        # ~/secrets-lab/LEDMatrix made config.json 0o640.
+        directory = tmp_path / "secrets-lab"
+        directory.mkdir()
+
+        atomic_write_text(directory / "config.json", "{}")
+
+        assert stat.S_IMODE(os.stat(directory / "config.json").st_mode) == 0o644
+
+    def test_a_root_save_hands_the_file_back_to_its_previous_owner(self, tmp_path, monkeypatch):
+        # The display service runs as root. A rename gives the file to the
+        # writer, so without this a root save would leave config.json owned by
+        # root and the web user could only ever replace it, never edit it.
+        target = tmp_path / "config.json"
+        target.write_text("{}")
+        previous = target.stat()
+        chowns = []
+
+        monkeypatch.setattr(atomic_module.os, "geteuid", lambda: 0, raising=False)
+        monkeypatch.setattr(atomic_module.os, "chown",
+                            lambda path, uid, gid: chowns.append((Path(path), uid, gid)),
+                            raising=False)
+        monkeypatch.setattr("src.common.permission_utils.get_shared_group_gid", lambda: None)
+
+        atomic_write_text(target, '{"timezone": "UTC"}')
+
+        assert len(chowns) == 1
+        temp_path, uid, gid = chowns[0]
+        assert temp_path.parent == tmp_path and temp_path.name.startswith(".config.json.tmp.")
+        assert (uid, gid) == (previous.st_uid, previous.st_gid)
+
+    def test_a_non_root_save_never_tries_to_chown(self, tmp_path, monkeypatch):
+        target = tmp_path / "config.json"
+        target.write_text("{}")
+        chowns = []
+
+        monkeypatch.setattr(atomic_module.os, "geteuid", lambda: 1000, raising=False)
+        monkeypatch.setattr(atomic_module.os, "chown",
+                            lambda *args: chowns.append(args), raising=False)
+
+        atomic_write_text(target, '{"timezone": "UTC"}')
+
+        assert chowns == []
+
+
+@pytest.mark.skipif(os.name != 'nt', reason="only Windows refuses to rename over an open file")
+class TestWindowsReaderHoldingTheFile:
+    def test_a_save_waits_out_a_reader_instead_of_failing(self, tmp_path):
+        target = tmp_path / "config.json"
+        target.write_text("{}")
+        reader = open(target)
+        closer = threading.Timer(0.15, reader.close)
+        closer.start()
+        try:
+            atomic_write_text(target, '{"timezone": "UTC"}')
+        finally:
+            closer.join()
+            reader.close()
+        assert json.loads(target.read_text()) == {"timezone": "UTC"}
+
+
+class TestSecretsAreOnlyRewrittenWhenTheyChange:
+    @pytest.fixture
+    def writes(self, monkeypatch):
+        written = []
+        real = atomic_module.atomic_write_text
+
+        def spy(path, text, mode=None):
+            written.append(Path(path).name)
+            return real(path, text, mode)
+
+        monkeypatch.setattr(atomic_module, "atomic_write_text", spy)
+        return written
+
+    def test_config_manager_save_leaves_the_secrets_file_alone(self, tmp_path, writes):
+        secrets = {"weather": {"api_key": "k"}}
+        manager = make_manager(tmp_path, secrets=secrets)
+
+        config = manager.load_config()
+        config["timezone"] = "UTC"
+        result = manager.save_config_atomic(config)
+
+        assert result.status == SaveResultStatus.SUCCESS
+        assert writes == ["config.json"]
+        assert json.loads((tmp_path / "config_secrets.json").read_text()) == secrets
+        on_disk = json.loads((tmp_path / "config.json").read_text())
+        assert on_disk["timezone"] == "UTC" and "api_key" not in on_disk.get("weather", {})
+
+    def test_identical_secrets_are_not_rewritten(self, tmp_path, writes):
+        secrets = {"weather": {"api_key": "k"}}
+        make_manager(tmp_path, secrets=secrets)
+        manager = AtomicConfigManager(str(tmp_path / "config.json"),
+                                      str(tmp_path / "config_secrets.json"))
+
+        manager.save_config_atomic({"timezone": "UTC"}, new_secrets=dict(secrets))
+
+        assert writes == ["config.json"]
+
+    def test_changed_secrets_are_written(self, tmp_path, writes):
+        make_manager(tmp_path, secrets={"weather": {"api_key": "old"}})
+        manager = AtomicConfigManager(str(tmp_path / "config.json"),
+                                      str(tmp_path / "config_secrets.json"))
+
+        manager.save_config_atomic({"timezone": "UTC"}, new_secrets={"weather": {"api_key": "new"}})
+
+        assert writes == ["config.json", "config_secrets.json"]
+        assert json.loads((tmp_path / "config_secrets.json").read_text()) == {"weather": {"api_key": "new"}}
+
+
+class TestBackupRotation:
+    VERSION = re.compile(r"^\d{8}_\d{6}_\d{6}(-\d+)?$")
+
+    def test_five_newest_backups_are_kept_under_the_same_names(self, tmp_path):
+        manager = make_manager(tmp_path, secrets={"weather": {"api_key": "k"}})
+        backup_dir = tmp_path / "backups"
+        backup_dir.mkdir()
+        for version in ("20240101_120000", "20240102_120000"):
+            (backup_dir / f"config.json.backup.{version}").write_text("{}")
+            (backup_dir / f"config_secrets.json.backup.{version}").write_text("{}")
+
+        created = []
+        for i in range(6):
+            result = manager.save_config_atomic({"timezone": f"tz{i}"})
+            assert result.status == SaveResultStatus.SUCCESS
+            created.append(Path(result.backup_path).name)
+
+        config_backups = sorted(p.name for p in backup_dir.glob("config.json.backup.*"))
+        secrets_backups = sorted(p.name for p in backup_dir.glob("config_secrets.json.backup.*"))
+
+        assert config_backups == sorted(created[-5:])
+        for name in config_backups:
+            assert self.VERSION.match(name[len("config.json.backup."):])
+        assert secrets_backups == sorted(
+            n.replace("config.json.backup.", "config_secrets.json.backup.") for n in created[-5:]
+        )
+        assert [b.path for b in manager.list_backups()] == [
+            str(backup_dir / n) for n in reversed(created[-5:])
+        ]
+
+    def test_rotation_does_not_open_the_backups(self, tmp_path, monkeypatch):
+        manager = make_manager(tmp_path)
+        for _ in range(3):
+            manager.save_config_atomic({"timezone": "UTC"})
+
+        parsed = []
+        monkeypatch.setattr(AtomicConfigManager, "_validate_config_file",
+                            lambda self, path: parsed.append(path))
+
+        result = manager.save_config_atomic({"timezone": "UTC"})
+        assert result.status == SaveResultStatus.SUCCESS
+        assert parsed == []
+
+    def test_a_rollback_still_restores_from_a_rotated_backup(self, tmp_path):
+        manager = make_manager(tmp_path)
+        for i in range(7):
+            manager.save_config_atomic({"timezone": f"tz{i}"})
+
+        assert manager.rollback_config()
+        # The newest backup was taken just before the last save.
+        assert json.loads((tmp_path / "config.json").read_text()) == {"timezone": "tz5"}