From bb2241498f50af0549561b15f70e67e3a5886eb4 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Sat, 30 May 2026 23:22:05 -0700 Subject: [PATCH] Metadata cache: hard LRU row cap to stop unbounded growth (7.6GB incident) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Investigation (not assumption): the cache's TTL eviction + junk cleanup ARE correct and DO run automatically every 6h (CacheEvictorJob, auto_fix=True). The real gap is there's NO SIZE CEILING — TTL-only eviction means 'how big can it get' = 'however much you fetch within the 30-day window', so heavy discovery/enrichment legitimately grew metadata_cache_entities to ~1.8M rows / 7.6 GB, bloating the main DB (a factor in the corruption incident). Fix — add a bounded LRU cap: - entities_to_evict_for_capacity(total, max_rows): pure decision fn (cap<=0 disables), unit-testable like core.db_integrity.prune_backups. - MetadataCache.evict_over_capacity(): deletes the least-recently-ACCESSED rows (uses the already-stored last_accessed_at; NULL = never-touched = evicted first) down to the ceiling. Default 250k rows, tunable. - Wired as Phase 5 of CacheEvictorJob — runs LAST, after TTL/junk/orphan/null cleanup, so it only trims a still-oversized HEALTHY cache. Verified safe to bound/wipe: audited every cache reader (get_entity/ get_entities_batch/get_search_results/get_entity_detail/browse) — all degrade to None/[]/empty on miss, treated as 'go fetch'. Nothing depends on a row existing, so eviction can't break callers. Tests: tests/metadata/test_cache_capacity_eviction.py (8) — pure-fn coverage + real temp-DB proof that it drops the LRU rows specifically (not arbitrary) and NULL-access rows go first. 18 adjacent cache tests still green; ruff clean. Follow-ups (separate phases, scoped): (2) move the cache to its own bounded metadata_cache.sqlite3 (no JOINs to library tables — confirmed clean to split; invalidate-and-rebuild rather than migrate the 7.6GB), (3) kill the raw_json + 22-extracted-column double storage. --- core/metadata/cache.py | 57 ++++++ core/repair_jobs/cache_evictor.py | 17 ++ .../metadata/test_cache_capacity_eviction.py | 186 ++++++++++++++++++ 3 files changed, 260 insertions(+) create mode 100644 tests/metadata/test_cache_capacity_eviction.py diff --git a/core/metadata/cache.py b/core/metadata/cache.py index b49daae5..1d296da8 100644 --- a/core/metadata/cache.py +++ b/core/metadata/cache.py @@ -14,6 +14,26 @@ from typing import Optional, Dict, List, Tuple logger = logging.getLogger(__name__) +# Hard ceiling on cached entity rows. TTL-only eviction has no upper bound, so +# heavy discovery/enrichment within the TTL window let this table reach ~1.8M +# rows / 7.6 GB (and helped bloat the main DB toward a corruption incident). +# Beyond this cap we evict least-recently-ACCESSED rows (LRU) — the data to do +# it (last_accessed_at) is already stored. Tunable via config; 0 disables. +_DEFAULT_MAX_CACHE_ENTITIES = 250_000 + + +def entities_to_evict_for_capacity(total_rows: int, max_rows: int) -> int: + """Pure decision: how many LRU rows to drop to bring the cache to ``max_rows``. + + Separated from SQL so it's unit-testable (mirrors core.db_integrity. + prune_backups). ``max_rows <= 0`` means "no cap" -> never evict. Never + returns negative. + """ + if max_rows <= 0: + return 0 + return max(0, total_rows - max_rows) + + # Singleton _cache_instance = None _cache_lock = threading.Lock() @@ -619,6 +639,43 @@ class MetadataCache: return self._run_maintenance_write("Cache eviction", _operation) + def evict_over_capacity(self, max_rows: int = _DEFAULT_MAX_CACHE_ENTITIES) -> int: + """Enforce a hard row ceiling: if the entity cache exceeds ``max_rows``, + delete the least-recently-ACCESSED rows down to the cap (LRU). This is + the bound TTL-only eviction lacked. Returns the count evicted. + + Runs AFTER evict_expired in the maintenance job, so TTL-expired and junk + rows are already gone — this only trims a still-oversized healthy cache. + """ + def _operation(conn): + cursor = conn.cursor() + total = cursor.execute( + "SELECT COUNT(*) FROM metadata_cache_entities" + ).fetchone()[0] + to_evict = entities_to_evict_for_capacity(total, max_rows) + if to_evict <= 0: + return 0 + # Delete the oldest-accessed rows. NULL last_accessed_at sorts first + # (evicted earliest) — those were never touched since insert. + cursor.execute(""" + DELETE FROM metadata_cache_entities + WHERE id IN ( + SELECT id FROM metadata_cache_entities + ORDER BY last_accessed_at ASC + LIMIT ? + ) + """, (to_evict,)) + evicted = cursor.rowcount + conn.commit() + if evicted > 0: + logger.info( + "Cache over capacity (%d > %d) — evicted %d least-recently-used entities", + total, max_rows, evicted, + ) + return evicted + + return self._run_maintenance_write("Cache capacity eviction", _operation) + def clean_junk_entities(self) -> int: """Delete cached entities with empty/placeholder names.""" def _operation(conn): diff --git a/core/repair_jobs/cache_evictor.py b/core/repair_jobs/cache_evictor.py index e2559fc9..6fa22c17 100644 --- a/core/repair_jobs/cache_evictor.py +++ b/core/repair_jobs/cache_evictor.py @@ -89,4 +89,21 @@ class CacheEvictorJob(RepairJob): logger.error("MB null cleanup failed: %s", e, exc_info=True) result.errors += 1 + if context.check_stop(): + return result + + # Phase 5: Hard capacity cap (LRU). Runs LAST so TTL/junk/orphan/null + # rows are already gone; this only trims a still-oversized HEALTHY cache + # down to the row ceiling — the bound TTL-only eviction never had (the + # cache previously reached ~1.8M rows / 7.6 GB). + try: + over = cache.evict_over_capacity() + result.auto_fixed += over + result.scanned += over + if over > 0: + logger.info("Phase 5 — capacity cap: evicted %d LRU entries", over) + except Exception as e: + logger.error("Capacity eviction failed: %s", e, exc_info=True) + result.errors += 1 + return result diff --git a/tests/metadata/test_cache_capacity_eviction.py b/tests/metadata/test_cache_capacity_eviction.py new file mode 100644 index 00000000..e4937b2e --- /dev/null +++ b/tests/metadata/test_cache_capacity_eviction.py @@ -0,0 +1,186 @@ +"""Tests for the metadata-cache hard capacity cap (LRU eviction). + +TTL-only eviction had no upper bound, so heavy in-window caching let +metadata_cache_entities reach ~1.8M rows / 7.6 GB. evict_over_capacity adds an +LRU row ceiling. We test the pure decision function directly and the SQL +behavior against a real temp DB (proves it drops the LEAST-recently-accessed +rows, not arbitrary ones). +""" + +from __future__ import annotations + +import sqlite3 +import sys +import types + +import pytest + +# Minimal stubs so importing core.metadata.cache doesn't drag in spotipy/config. +if "spotipy" not in sys.modules: + spotipy = types.ModuleType("spotipy") + spotipy.Spotify = object + oauth2 = types.ModuleType("spotipy.oauth2") + oauth2.SpotifyOAuth = object + oauth2.SpotifyClientCredentials = object + spotipy.oauth2 = oauth2 + sys.modules["spotipy"] = spotipy + sys.modules["spotipy.oauth2"] = oauth2 +if "config.settings" not in sys.modules: + config_pkg = types.ModuleType("config") + settings_mod = types.ModuleType("config.settings") + + class _DummyCM: + def get(self, key, default=None): + return default + + def get_active_media_server(self): + return "plex" + + settings_mod.config_manager = _DummyCM() + config_pkg.settings = settings_mod + sys.modules["config"] = config_pkg + sys.modules["config.settings"] = settings_mod + +from core.metadata.cache import ( # noqa: E402 + MetadataCache, + entities_to_evict_for_capacity, +) + + +# ── pure decision function ───────────────────────────────────────────────── + +def test_evict_count_over_cap(): + assert entities_to_evict_for_capacity(1000, 250) == 750 + + +def test_evict_count_at_or_under_cap_is_zero(): + assert entities_to_evict_for_capacity(250, 250) == 0 + assert entities_to_evict_for_capacity(10, 250) == 0 + + +def test_cap_zero_or_negative_means_no_eviction(): + assert entities_to_evict_for_capacity(10_000, 0) == 0 + assert entities_to_evict_for_capacity(10_000, -1) == 0 + + +def test_never_negative(): + assert entities_to_evict_for_capacity(0, 100) == 0 + + +# ── evict_over_capacity SQL behavior (real temp DB) ──────────────────────── + +class _NonClosingConn: + def __init__(self, real): + self._real = real + + def cursor(self): + return self._real.cursor() + + def commit(self): + return self._real.commit() + + def close(self): + pass + + def __enter__(self): + return self + + def __exit__(self, *a): + pass + + +class _TempCache(MetadataCache): + """MetadataCache whose _get_db returns a shim over a shared in-memory DB + holding just the entities table — enough to exercise evict_over_capacity.""" + + def __init__(self): + self._conn = sqlite3.connect(":memory:") + self._conn.row_factory = sqlite3.Row + self._conn.execute(""" + CREATE TABLE metadata_cache_entities ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + source TEXT, entity_type TEXT, entity_id TEXT, + name TEXT, raw_json TEXT, + last_accessed_at TIMESTAMP, + ttl_days INTEGER DEFAULT 30 + ) + """) + self._conn.commit() + + def _get_db(self): + outer = self + + class _DB: + def _get_connection(self_inner): + return _NonClosingConn(outer._conn) + return _DB() + + # evict_over_capacity uses self._run_maintenance_write -> _get_db; the base + # _run_maintenance_write just calls the operation. Add a tiny passthrough + # if the base needs it (it does in the real class). + def _run_maintenance_write(self, label, operation, default=0): + try: + return operation(self._get_db()._get_connection()) + except Exception: + return default + + +def _add_rows(cache, specs): + """specs: list of (entity_id, last_accessed_at). Inserts rows.""" + cur = cache._conn.cursor() + for eid, ts in specs: + cur.execute( + "INSERT INTO metadata_cache_entities (source, entity_type, entity_id, name, raw_json, last_accessed_at) " + "VALUES ('spotify','artist',?,?, '{}', ?)", + (eid, eid, ts), + ) + cache._conn.commit() + + +def _ids(cache): + return [r["entity_id"] for r in cache._conn.execute( + "SELECT entity_id FROM metadata_cache_entities ORDER BY entity_id" + ).fetchall()] + + +def test_evict_over_capacity_drops_least_recently_accessed(): + cache = _TempCache() + # 5 rows, distinct access times. Cap at 3 -> evict the 2 oldest-accessed. + _add_rows(cache, [ + ("a", "2026-05-01T00:00:00"), # oldest -> evicted + ("b", "2026-05-02T00:00:00"), # oldest -> evicted + ("c", "2026-05-03T00:00:00"), + ("d", "2026-05-04T00:00:00"), + ("e", "2026-05-05T00:00:00"), # newest -> kept + ]) + evicted = cache.evict_over_capacity(max_rows=3) + assert evicted == 2 + assert _ids(cache) == ["c", "d", "e"] # a, b gone (LRU) + + +def test_evict_over_capacity_noop_under_cap(): + cache = _TempCache() + _add_rows(cache, [("a", "2026-05-01T00:00:00"), ("b", "2026-05-02T00:00:00")]) + assert cache.evict_over_capacity(max_rows=10) == 0 + assert _ids(cache) == ["a", "b"] + + +def test_evict_over_capacity_disabled_with_zero_cap(): + cache = _TempCache() + _add_rows(cache, [(str(i), f"2026-05-0{i}T00:00:00") for i in range(1, 6)]) + assert cache.evict_over_capacity(max_rows=0) == 0 + assert len(_ids(cache)) == 5 + + +def test_null_access_times_evicted_first(): + """Rows never accessed since insert (NULL last_accessed_at) are the + coldest — they should go before any touched row.""" + cache = _TempCache() + _add_rows(cache, [ + ("never1", None), + ("never2", None), + ("touched", "2026-05-01T00:00:00"), + ]) + evicted = cache.evict_over_capacity(max_rows=1) + assert evicted == 2 + assert _ids(cache) == ["touched"]