#918 follow-up: self-heal stale truncated iTunes album-tracks cache
The limit=200 fix only helped FRESH fetches. The metadata cache is persistent (SQLite, 30-day TTL), so any album whose tracklist was cached at 50 BEFORE the fix keeps returning 50 from cache in every window that loads it (line returned the cached entry without revalidating) — which is why the Standard-view add-album modal still showed 50 while a freshly-fetched album in the download window showed the full list. Same album, different cache state. Fix: get_album_tracks now marks freshly-fetched entries '_complete'. On a cache hit, a legacy entry without that flag is revalidated against the album's known trackCount (from collection metadata, unaffected by the bug) and re-fetched if short. The '_complete' flag makes the heal one-time and avoids a re-fetch loop on region-restricted albums where available tracks < trackCount. Tests: stale-truncated -> refetch+heal; _complete -> trusted; legacy-complete and unknown-trackCount -> trusted (no regression). Fresh fetch carries _complete.
This commit is contained in:
parent
0f2d6ddb63
commit
462a722cce
2 changed files with 132 additions and 2 deletions
|
|
@ -739,7 +739,23 @@ class iTunesClient:
|
|||
cache = get_metadata_cache()
|
||||
cached = cache.get_entity('itunes', 'album', f"{album_id}_tracks")
|
||||
if cached and cached.get('items'):
|
||||
return cached
|
||||
# #918 follow-up: a tracks entry cached BEFORE the limit=200 fix is truncated
|
||||
# to 50 and survives in the persistent cache (30-day TTL), so every window that
|
||||
# loads this album from cache still shows 50 — not just the one path that was
|
||||
# re-fetched fresh. Self-heal: entries written by the fixed fetch carry
|
||||
# `_complete`; a legacy entry without it is re-validated against the album's
|
||||
# known trackCount and re-fetched if it's short. (trackCount comes from the
|
||||
# collection metadata and is unaffected by the tracks-limit bug.)
|
||||
if cached.get('_complete'):
|
||||
return cached
|
||||
album_meta = cache.get_entity('itunes', 'album', str(album_id))
|
||||
expected = (album_meta or {}).get('trackCount')
|
||||
if not (isinstance(expected, int) and expected > len(cached['items'])):
|
||||
return cached
|
||||
logger.info(
|
||||
"iTunes album %s tracks cache looks truncated (%d cached < %d trackCount) — refetching",
|
||||
album_id, len(cached['items']), expected,
|
||||
)
|
||||
|
||||
# #918: the iTunes Lookup API returns only 50 related entities unless `limit` is
|
||||
# passed (max 200), so albums >50 tracks were truncated in the download window.
|
||||
|
|
@ -851,7 +867,11 @@ class iTunesClient:
|
|||
'items': tracks,
|
||||
'total': len(tracks),
|
||||
'limit': len(tracks),
|
||||
'next': None
|
||||
'next': None,
|
||||
# Marks this entry as fetched with the limit=200 query (#918) so the
|
||||
# stale-cache self-heal above trusts it and never re-fetches in a loop —
|
||||
# important for region-restricted albums where len(tracks) < trackCount.
|
||||
'_complete': True,
|
||||
}
|
||||
|
||||
# Cache the album tracks listing
|
||||
|
|
|
|||
|
|
@ -44,3 +44,113 @@ def test_get_album_tracks_requests_limit_200(monkeypatch):
|
|||
assert captured.get('entity') == 'song'
|
||||
assert captured.get('id') == '123'
|
||||
assert result is not None
|
||||
assert result.get('_complete') is True # fresh fetch is marked complete
|
||||
|
||||
|
||||
# ── #918 follow-up: self-heal a stale truncated cache ─────────────────────────
|
||||
# The metadata cache is persistent (30-day TTL), so a tracks entry written before
|
||||
# the limit=200 fix stays truncated at 50 and is served to EVERY window (e.g. the
|
||||
# Standard-view add-album modal) until it expires. get_album_tracks must detect a
|
||||
# legacy entry shorter than the album's known trackCount and re-fetch.
|
||||
|
||||
class _StubCache:
|
||||
"""Holds entities so cache hits/stores can be asserted."""
|
||||
def __init__(self, entities=None):
|
||||
self.entities = dict(entities or {})
|
||||
self.stored = {}
|
||||
|
||||
def get_entity(self, source, entity_type, entity_id):
|
||||
return self.entities.get((source, entity_type, entity_id))
|
||||
|
||||
def store_entity(self, source, entity_type, entity_id, data):
|
||||
self.stored[(source, entity_type, entity_id)] = data
|
||||
|
||||
def store_entities_bulk(self, *a, **k):
|
||||
pass
|
||||
|
||||
|
||||
def _collection(track_count):
|
||||
return {'wrapperType': 'collection', 'collectionId': 123, 'collectionName': 'Big OST',
|
||||
'artistName': 'Composer', 'trackCount': track_count, 'artworkUrl100': 'http://x/100x100bb.jpg'}
|
||||
|
||||
|
||||
def _track(n):
|
||||
return {'wrapperType': 'track', 'kind': 'song', 'trackId': n, 'trackName': f'T{n}',
|
||||
'trackNumber': n, 'discNumber': 1, 'artistName': 'Composer', 'trackTimeMillis': 1000}
|
||||
|
||||
|
||||
def _full_results(n):
|
||||
return [_collection(n)] + [_track(i) for i in range(1, n + 1)]
|
||||
|
||||
|
||||
def _items(n):
|
||||
return [{'id': str(i)} for i in range(n)]
|
||||
|
||||
|
||||
def test_stale_truncated_legacy_cache_is_refetched(monkeypatch):
|
||||
"""A 50-item legacy entry (no _complete) for a 70-track album → re-fetch full."""
|
||||
client = ic.iTunesClient(country='US')
|
||||
cache = _StubCache({
|
||||
('itunes', 'album', '123_tracks'): {'items': _items(50), 'total': 50}, # legacy, truncated
|
||||
('itunes', 'album', '123'): _collection(70), # real trackCount=70
|
||||
})
|
||||
monkeypatch.setattr(ic, 'get_metadata_cache', lambda: cache)
|
||||
calls = {'n': 0}
|
||||
|
||||
def fake_lookup(**params):
|
||||
calls['n'] += 1
|
||||
return _full_results(70)
|
||||
|
||||
monkeypatch.setattr(client, '_lookup', fake_lookup)
|
||||
|
||||
result = client.get_album_tracks('123')
|
||||
|
||||
assert calls['n'] == 1 # re-fetched (didn't trust the stale 50)
|
||||
assert len(result['items']) == 70
|
||||
assert result['_complete'] is True
|
||||
assert cache.stored[('itunes', 'album', '123_tracks')]['_complete'] is True # healed in cache
|
||||
|
||||
|
||||
def test_complete_cache_is_trusted_no_refetch(monkeypatch):
|
||||
"""A _complete entry is returned as-is even if shorter than trackCount
|
||||
(region-restricted album) — must NOT loop re-fetching."""
|
||||
client = ic.iTunesClient(country='US')
|
||||
complete = {'items': _items(50), 'total': 50, '_complete': True}
|
||||
cache = _StubCache({
|
||||
('itunes', 'album', '123_tracks'): complete,
|
||||
('itunes', 'album', '123'): _collection(70),
|
||||
})
|
||||
monkeypatch.setattr(ic, 'get_metadata_cache', lambda: cache)
|
||||
|
||||
def boom(**params):
|
||||
raise AssertionError('must not re-fetch a _complete entry')
|
||||
|
||||
monkeypatch.setattr(client, '_lookup', boom)
|
||||
|
||||
assert client.get_album_tracks('123') is complete
|
||||
|
||||
|
||||
def test_legacy_complete_cache_not_refetched(monkeypatch):
|
||||
"""A legacy entry whose length already meets trackCount is fine — no re-fetch."""
|
||||
client = ic.iTunesClient(country='US')
|
||||
legacy = {'items': _items(30), 'total': 30} # no _complete, but complete by count
|
||||
cache = _StubCache({
|
||||
('itunes', 'album', '123_tracks'): legacy,
|
||||
('itunes', 'album', '123'): _collection(30),
|
||||
})
|
||||
monkeypatch.setattr(ic, 'get_metadata_cache', lambda: cache)
|
||||
monkeypatch.setattr(client, '_lookup', lambda **p: (_ for _ in ()).throw(AssertionError('no refetch')))
|
||||
|
||||
assert client.get_album_tracks('123') is legacy
|
||||
|
||||
|
||||
def test_legacy_cache_without_album_meta_is_trusted(monkeypatch):
|
||||
"""trackCount unknown (album meta not cached) → trust the cache, don't re-fetch
|
||||
(safe fallback; no regression for direct get_album_tracks callers)."""
|
||||
client = ic.iTunesClient(country='US')
|
||||
legacy = {'items': _items(50), 'total': 50} # no _complete, no album meta
|
||||
cache = _StubCache({('itunes', 'album', '123_tracks'): legacy})
|
||||
monkeypatch.setattr(ic, 'get_metadata_cache', lambda: cache)
|
||||
monkeypatch.setattr(client, '_lookup', lambda **p: (_ for _ in ()).throw(AssertionError('no refetch')))
|
||||
|
||||
assert client.get_album_tracks('123') is legacy
|
||||
|
|
|
|||
Loading…
Reference in a new issue