Artist pages: stop watchlist probes from poisoning the album-list cache
Boulder: "Taylor Swift shows only 8 albums, nothing before 2022, no singles, no EPs" — for every artist (actually: every WATCHLIST artist). Traced live: get_artist_albums caches its result under an UNQUALIFIED key (no limit/page info), and the watchlist's new-release probe (limit=5, max_pages=1 — the April "reduce watchlist API calls ~90%" optimization) stored its truncated single page in that same slot. The artist detail page reads the cache first, so a watchlisted artist's page showed only the newest handful of releases — newest-first, hence "nothing before 2022" — re-poisoned on every scan, with a 30-day TTL. When the source-priority fetch comes back tiny, the page's fallback path quietly serves it, so the symptom looked like a discography filter bug. Not related to the #808 matching change (that is a pure max(), provably additive). Three pieces: - get_artist_albums tracks whether the fetch stopped while more pages existed (truncated) and only caches COMPLETE discographies. Individual albums keep their opportunistic caching — they're complete entities regardless of pagination. A small real discography that fits one page stays cacheable even under max_pages=1. - MetadataCache.purge_artist_album_lists(): delete the already-poisoned album-list entries (TTL would have kept them for weeks); lists rebuild lazily on the next artist-page visit. - one-time startup purge in web_server, config-guarded (maintenance.album_cache_purge_v1), mirroring the startup-repair pattern. Tests: truncated probe never stores the list (but still returns its page), complete multi-page fetch caches, and a genuinely-small one-page discography under max_pages=1 still caches. 1087 spotify/cache/watchlist/artist tests pass.
This commit is contained in:
parent
f250eaa228
commit
ef751ce4e4
4 changed files with 144 additions and 1 deletions
|
|
@ -830,6 +830,36 @@ class MetadataCache:
|
|||
logger.error(f"Cache health stats error: {e}")
|
||||
return {}
|
||||
|
||||
def purge_artist_album_lists(self, source: str = 'spotify') -> int:
|
||||
"""One-time repair: delete cached artist-ALBUM-LIST entries.
|
||||
|
||||
Partial watchlist probes (limit=5, max_pages=1) used to be stored in
|
||||
the same unqualified ``<artist>_albums_<types>`` slot the artist
|
||||
detail page reads, so every watchlist artist's page showed only the
|
||||
newest handful of releases. The writer is fixed to skip truncated
|
||||
fetches; this clears the already-poisoned entries (30-day TTL would
|
||||
otherwise keep them for weeks). Lists rebuild lazily on the next
|
||||
artist-page visit. Returns the number of entries removed."""
|
||||
try:
|
||||
db = self._get_db()
|
||||
conn = db._get_connection()
|
||||
try:
|
||||
cursor = conn.cursor()
|
||||
cursor.execute(
|
||||
"DELETE FROM metadata_cache_entities "
|
||||
"WHERE source = ? AND entity_type = 'artist' "
|
||||
"AND entity_id LIKE '%\\_albums\\_%' ESCAPE '\\'",
|
||||
(source,),
|
||||
)
|
||||
count = cursor.rowcount
|
||||
conn.commit()
|
||||
return count
|
||||
finally:
|
||||
conn.close()
|
||||
except Exception as e:
|
||||
logger.debug("artist album-list purge failed: %s", e)
|
||||
return 0
|
||||
|
||||
def clear(self, source: str = None, entity_type: str = None) -> int:
|
||||
"""Clear cache entries. Optional filters by source and/or entity_type."""
|
||||
try:
|
||||
|
|
|
|||
|
|
@ -1859,6 +1859,7 @@ class SpotifyClient:
|
|||
try:
|
||||
albums = []
|
||||
raw_items = []
|
||||
truncated = False # did we stop while more pages existed?
|
||||
# Spotify caps artist_albums at 10 per page
|
||||
results = self.sp.artist_albums(artist_id, album_type=album_type, limit=min(limit, 10))
|
||||
pages_fetched = 1
|
||||
|
|
@ -1871,6 +1872,7 @@ class SpotifyClient:
|
|||
|
||||
# Stop if we've hit the page limit (0 = unlimited)
|
||||
if max_pages and pages_fetched >= max_pages:
|
||||
truncated = bool(results.get('next'))
|
||||
break
|
||||
|
||||
# Get next batch if available — throttle pagination to respect rate limits
|
||||
|
|
@ -1893,8 +1895,17 @@ class SpotifyClient:
|
|||
(f" (page limit: {max_pages})" if max_pages else ""))
|
||||
|
||||
# Cache the full artist albums result (wrapped in dict for cache compatibility)
|
||||
# Only cache COMPLETE discographies. The cache key carries no
|
||||
# limit/page info, so a partial probe (the watchlist's
|
||||
# new-release check: limit=5, max_pages=1) stored here used to
|
||||
# POISON the slot — the artist detail page then showed only
|
||||
# the 5-10 newest releases for every watchlist artist until
|
||||
# the 30-day TTL expired ("Taylor Swift has 8 albums, nothing
|
||||
# before 2022"). Individual albums are still cached — they're
|
||||
# complete entities regardless of how many pages we walked.
|
||||
if raw_items:
|
||||
cache.store_entity('spotify', 'artist', cache_key, {'name': f'albums_{artist_id}', '_albums': raw_items})
|
||||
if not truncated:
|
||||
cache.store_entity('spotify', 'artist', cache_key, {'name': f'albums_{artist_id}', '_albums': raw_items})
|
||||
# Also cache individual albums opportunistically
|
||||
entries = [(ad.get('id'), ad) for ad in raw_items if ad.get('id')]
|
||||
if entries:
|
||||
|
|
|
|||
84
tests/test_artist_albums_cache_poisoning.py
Normal file
84
tests/test_artist_albums_cache_poisoning.py
Normal file
|
|
@ -0,0 +1,84 @@
|
|||
"""Artist album-list cache poisoning (Boulder: 'Taylor Swift has 8 albums,
|
||||
nothing before 2022').
|
||||
|
||||
get_artist_albums caches its result under an UNQUALIFIED key (no limit/page
|
||||
info). The watchlist's new-release probe (limit=5, max_pages=1) stored its
|
||||
truncated page in that slot, so the artist detail page — which reads the
|
||||
cache — showed only the newest handful of releases for every watchlist
|
||||
artist. The writer must never cache a fetch that stopped while more pages
|
||||
existed; complete fetches (even small real discographies) stay cacheable.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from unittest.mock import MagicMock
|
||||
|
||||
import core.spotify_client as sc
|
||||
from core.spotify_client import SpotifyClient
|
||||
|
||||
|
||||
def _album(i):
|
||||
return {
|
||||
'id': f'al{i}', 'name': f'Album {i}', 'album_type': 'album',
|
||||
'artists': [{'id': 'ar1', 'name': 'Taylor Swift'}],
|
||||
'release_date': '2024-01-01', 'total_tracks': 12, 'images': [],
|
||||
'external_urls': {},
|
||||
}
|
||||
|
||||
|
||||
def _client(monkeypatch, pages):
|
||||
"""Fake sp.artist_albums + sp.next over a list of page dicts."""
|
||||
client = SpotifyClient.__new__(SpotifyClient)
|
||||
fake = MagicMock()
|
||||
fake.artist_albums.return_value = pages[0]
|
||||
fake.next.side_effect = pages[1:]
|
||||
client.sp = fake
|
||||
monkeypatch.setattr(client, 'is_spotify_authenticated', lambda: True)
|
||||
monkeypatch.setattr(sc, '_last_api_call_time', 0)
|
||||
store_calls = []
|
||||
cache = MagicMock()
|
||||
cache.get_entity.return_value = None
|
||||
cache.store_entity.side_effect = lambda *a, **k: store_calls.append(a)
|
||||
monkeypatch.setattr(sc, 'get_metadata_cache', lambda: cache)
|
||||
return client, store_calls
|
||||
|
||||
|
||||
def test_truncated_fetch_is_not_cached(monkeypatch):
|
||||
# Two pages exist; max_pages=1 stops with a 'next' pending -> truncated.
|
||||
pages = [
|
||||
{'items': [_album(1), _album(2)], 'next': 'page2-url'},
|
||||
{'items': [_album(3)], 'next': None},
|
||||
]
|
||||
client, stores = _client(monkeypatch, pages)
|
||||
|
||||
albums = client.get_artist_albums('ar1', limit=5, skip_cache=True, max_pages=1)
|
||||
|
||||
assert len(albums) == 2 # the probe still works
|
||||
album_list_stores = [s for s in stores if s[1] == 'artist']
|
||||
assert album_list_stores == [] # but never poisons the slot
|
||||
|
||||
|
||||
def test_complete_fetch_is_cached(monkeypatch):
|
||||
pages = [
|
||||
{'items': [_album(1), _album(2)], 'next': 'page2-url'},
|
||||
{'items': [_album(3)], 'next': None},
|
||||
]
|
||||
client, stores = _client(monkeypatch, pages)
|
||||
|
||||
albums = client.get_artist_albums('ar1', limit=50, skip_cache=True, max_pages=0)
|
||||
|
||||
assert len(albums) == 3
|
||||
album_list_stores = [s for s in stores if s[1] == 'artist']
|
||||
assert len(album_list_stores) == 1 # full discography cached
|
||||
|
||||
|
||||
def test_small_real_discography_with_page_cap_still_cached(monkeypatch):
|
||||
# Artist genuinely has one page; max_pages=1 didn't truncate anything.
|
||||
pages = [{'items': [_album(1)], 'next': None}]
|
||||
client, stores = _client(monkeypatch, pages)
|
||||
|
||||
albums = client.get_artist_albums('ar1', limit=5, skip_cache=True, max_pages=1)
|
||||
|
||||
assert len(albums) == 1
|
||||
album_list_stores = [s for s in stores if s[1] == 'artist']
|
||||
assert len(album_list_stores) == 1 # complete -> cacheable
|
||||
|
|
@ -35645,6 +35645,24 @@ def start_runtime_services():
|
|||
logger.info("Starting OAuth callback servers...")
|
||||
start_oauth_callback_servers()
|
||||
|
||||
# One-time repair: purge artist album-list cache entries poisoned by
|
||||
# partial watchlist probes (limit=5/max_pages=1 results stored in the
|
||||
# full-discography slot — artist pages showed only the newest handful
|
||||
# of releases for every watchlist artist). The writer is fixed; this
|
||||
# clears what's already bad. Guarded so it runs once per install.
|
||||
try:
|
||||
if not config_manager.get('maintenance.album_cache_purge_v1', False):
|
||||
from core.metadata.cache import get_metadata_cache as _gmc
|
||||
_purged = _gmc().purge_artist_album_lists('spotify')
|
||||
config_manager.set('maintenance.album_cache_purge_v1', True)
|
||||
if _purged:
|
||||
logger.warning(
|
||||
"[Startup] Purged %d poisoned artist album-list cache "
|
||||
"entries (partial watchlist probes); artist pages will "
|
||||
"refetch full discographies lazily", _purged)
|
||||
except Exception as _purge_err:
|
||||
logger.debug("album cache purge skipped: %s", _purge_err)
|
||||
|
||||
# Startup diagnostics: Check and recover stuck flags
|
||||
logger.info("Running startup diagnostics...")
|
||||
stuck_flags_recovered = check_and_recover_stuck_flags()
|
||||
|
|
|
|||
Loading…
Reference in a new issue