fix(cache): cache keys too long to be a filename; memory hits judged by the record's own age (#738)

* fix(cache): store keys too long to be a filename The calendar plugin's cache key joins every calendar id the user picked. On hdpi it passed 300 bytes; ext4 caps a filename at 255, so every write (the temp file, the direct-write fallback and the home-directory fallback) failed with ENAMETOOLONG, once an hour, and the final warning said "(permission denied)" whatever the error was. DiskCache.get_cache_path keeps a key of up to 200 UTF-8 bytes as its filename, exactly as before, and turns a longer one into its first 183 bytes (cut on a character boundary) plus a 16-hex-digit hash of the whole key. The temp file adds 15 bytes, so the longest name is 215. The shortened stem is itself short, so the web UI's cache list, which names a key by its filename, deletes the same file. The give-up warning now names the real error. Validated on ledpi's ext4: the old module drops the hdpi-shaped key, the new one writes a 205-byte filename and reads it back. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(cache): judge a memory hit by the record's own timestamp A record loaded from disk went into the memory tier timed from the load, so get(key, max_age=300) could return data close to 600 s old: after a restart, after the memory sweep, or in a second process. A stored ttl was stretched the same way. #728's _fresh_cached works around it for the scoreboard; every other caller was exposed. get_cached_data and load_cache now also check a memory hit against the record's embedded timestamp, with DiskCache.get's rule that a stored ttl wins over max_age. A stale copy is dropped and the read falls through to disk, which returns the other process's newer write if there is one. Records without a timestamp keep the memory tier's own clock. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-04 06:15:09 +00:00 · 2026-10-03 22:18:00 -04:00
parent 41192b9588
commit a6e9e3ef1c
5 changed files with 285 additions and 7 deletions
@@ -0,0 +1,99 @@
+"""A cache key too long to be a filename still gets a cache file.
+
+The calendar plugin's key joins every calendar id the user picked; on a real
+install it passed 300 bytes, and since ext4 caps a filename at 255 every write
+failed with ENAMETOOLONG -- logged as "permission denied", every update.
+"""
+
+import logging
+import os
+from unittest.mock import patch
+
+from src.cache.disk_cache import DiskCache, _MAX_KEY_FILENAME_BYTES, _filename_stem
+from src.cache_manager import CacheManager
+
+# The shape of the key that failed on hdpi, ids anonymised.
+CALENDAR_KEY = (
+    "calendar_events_someone@example.com_en.usa#holiday@group.v.calendar.google.com_"
+    "family13997378751670666433@group.calendar.google.com_ncaaf_-m-07kbp5_"
+    "%47eorgia+%42ulldogs+football#sports@group.v.calendar.google.com_nfl_-m-07l24_"
+    "%54ampa+%42ay+%42uccaneers#sports@group.v.calendar.google.com_primary"
+)
+
+# ext4/xfs/btrfs NAME_MAX; set()'s temp file adds 15 bytes to the stem.
+NAME_MAX = 255
+TEMP_OVERHEAD = len(".") + len(".json") + len(".") + 8
+
+
+def test_the_real_key_was_too_long_to_write():
+    assert len((CALENDAR_KEY + ".json").encode()) > NAME_MAX - 10
+
+
+def test_a_long_key_round_trips(tmp_path):
+    cache = DiskCache(str(tmp_path))
+    cache.set(CALENDAR_KEY, {"events": [1, 2, 3]})
+
+    assert cache.get(CALENDAR_KEY, max_age=None) == {"events": [1, 2, 3]}
+    path = cache.get_cache_path(CALENDAR_KEY)
+    assert os.path.isfile(path)
+    stem = os.path.basename(path)[:-len(".json")]
+    assert len(stem.encode()) + TEMP_OVERHEAD <= NAME_MAX
+
+
+def test_short_keys_keep_their_filename(tmp_path):
+    cache = DiskCache(str(tmp_path))
+    exactly = "k" * _MAX_KEY_FILENAME_BYTES
+    assert cache.get_cache_path("weather_current") == str(tmp_path / "weather_current.json")
+    assert cache.get_cache_path(exactly) == str(tmp_path / f"{exactly}.json")
+    assert cache.get_cache_path(exactly + "k") != str(tmp_path / f"{exactly}k.json")
+
+
+def test_long_keys_sharing_a_prefix_stay_apart(tmp_path):
+    cache = DiskCache(str(tmp_path))
+    first, second = CALENDAR_KEY + "_a", CALENDAR_KEY + "_b"
+    cache.set(first, {"which": "a"})
+    cache.set(second, {"which": "b"})
+
+    assert cache.get_cache_path(first) != cache.get_cache_path(second)
+    assert cache.get(first, max_age=None) == {"which": "a"}
+    assert cache.get(second, max_age=None) == {"which": "b"}
+
+
+def test_the_prefix_never_splits_a_character():
+    key = "news_" + "é" * 300  # two bytes each, so the cut lands mid-character
+    stem = _filename_stem(key)
+
+    assert stem.startswith("news_é")
+    assert len(stem.encode("utf-8")) <= _MAX_KEY_FILENAME_BYTES
+    stem.encode("utf-8").decode("utf-8")  # well-formed
+
+
+def test_a_stem_listed_by_the_web_ui_deletes_the_same_file(tmp_path):
+    with patch('src.cache_manager.CacheManager._get_writable_cache_dir', return_value=str(tmp_path)):
+        manager = CacheManager()
+    try:
+        manager.save_cache(CALENDAR_KEY, {"events": []})
+
+        listed = [entry["key"] for entry in manager.list_cache_files()]
+        assert len(listed) == 1
+        manager.clear_cache(listed[0])
+
+        assert [n for n in os.listdir(tmp_path) if n.endswith(".json")] == []
+    finally:
+        manager.stop_cleanup_thread()
+
+
+def test_a_failed_write_names_the_real_error(tmp_path, monkeypatch, caplog):
+    blocker = tmp_path / "a-file"
+    blocker.write_text("")
+    # No writable fallback either, so set() gives up and says why.
+    monkeypatch.setattr(os.path, "expanduser", lambda _p: str(blocker / "home"))
+    cache = DiskCache(str(tmp_path / "missing"))
+
+    with caplog.at_level(logging.WARNING):
+        cache.set("weather_current", {"t": 1})
+
+    gave_up = [r.getMessage() for r in caplog.records if "Could not write cache" in r.getMessage()]
+    assert len(gave_up) == 1
+    assert "permission denied" not in gave_up[0]
+    assert os.strerror(2) in gave_up[0]  # ENOENT: the directory does not exist
@@ -0,0 +1,95 @@
+"""The memory tier never serves a record older than the reader asked for.
+
+A record read from disk went into the memory tier timed from the read, not
+from when it was written, so get(max_age=300) could hand out data up to twice
+that old: after a restart, after the memory sweep, or in a second process that
+loaded a record once and kept serving it.
+"""
+
+import time
+from unittest.mock import patch
+
+import pytest
+
+from src.cache_manager import CacheManager
+
+
+class Clock:
+    def __init__(self, now):
+        self.now = now
+
+    def __call__(self):
+        return self.now
+
+
+@pytest.fixture
+def clock(monkeypatch):
+    fake = Clock(1_800_000_000.0)
+    monkeypatch.setattr(time, "time", fake)
+    return fake
+
+
+def _manager(path):
+    # No disk sweep: it judges files by their real mtime against the fake
+    # clock and would delete them as months old.
+    with patch('src.cache_manager.CacheManager._get_writable_cache_dir',
+               return_value=str(path)), \
+            patch('src.cache_manager.CacheManager.start_cleanup_thread'):
+        return CacheManager()
+
+
+def test_a_record_loaded_late_expires_on_its_own_timestamp(tmp_path, clock):
+    writer = _manager(tmp_path)
+    writer.set("weather_current", {"t": 1})
+
+    reader = _manager(tmp_path)  # a restart, or the other process
+    clock.now += 250
+    assert reader.get("weather_current", max_age=300) == {"t": 1}
+
+    clock.now += 100  # the data is 350 s old; it sat in memory for 100 s
+    assert reader.get("weather_current", max_age=300) is None
+
+
+def test_a_stored_ttl_bounds_the_memory_copy_too(tmp_path, clock):
+    writer = _manager(tmp_path)
+    writer.set("odds_espn_football_nfl_401", {"spread": 6.5}, ttl=60)
+
+    reader = _manager(tmp_path)
+    clock.now += 55
+    assert reader.get("odds_espn_football_nfl_401", max_age=3600) == {"spread": 6.5}
+
+    clock.now += 60
+    assert reader.get("odds_espn_football_nfl_401", max_age=3600) is None
+
+
+def test_a_stale_memory_copy_gives_way_to_a_newer_write_on_disk(tmp_path, clock):
+    writer = _manager(tmp_path)
+    reader = _manager(tmp_path)
+    writer.set("stocks_AAPL", {"price": 1})
+    assert reader.get("stocks_AAPL", max_age=300) == {"price": 1}
+
+    clock.now += 280
+    writer.set("stocks_AAPL", {"price": 2})
+    clock.now += 40  # reader's copy: 40 s in memory, 320 s old
+
+    assert reader.get("stocks_AAPL", max_age=300) == {"price": 2}
+
+
+def test_fresh_records_are_still_served_from_memory(tmp_path, clock):
+    manager = _manager(tmp_path)
+    manager.set("news_NFL", {"items": []})
+    clock.now += 100
+
+    with patch.object(manager._disk_cache_component, "get") as disk_get:
+        assert manager.get("news_NFL", max_age=300) == {"items": []}
+    disk_get.assert_not_called()
+
+
+def test_max_age_none_and_records_without_a_timestamp_never_expire(tmp_path, clock):
+    manager = _manager(tmp_path)
+    manager.set("plugin_health_x", {"ok": True})
+    manager.save_cache("raw_record", {"no": "timestamp"})
+    clock.now += 10 ** 6
+
+    assert manager.get("plugin_health_x", max_age=None) == {"ok": True}
+    assert manager.get_cached_data("raw_record", max_age=None) == {"no": "timestamp"}