Spotify: enrichment can prefer Free, and the budget→Free bridge actually diverts
Two related fixes at one root cause. Every catalog method gated the official API on `use_spotify = is_spotify_authenticated()` and only fell to Free *after* official failed. So when the client was authed but should defer to Free — specifically when the worker's daily real-API budget was spent — it kept hitting the official API anyway (just stopped *counting* it). The budget "bridge" never actually diverted; it only stopped pausing. Root-cause fix: the official gate is now `is_spotify_authenticated() and not self._free_active()` across all 8 catalog methods (search_artists/albums/tracks, get_artist/album/album_tracks/track_details/artist_albums). No-auth and rate-limited are unchanged (auth is already False there); the change only affects the cases where auth is True but we deliberately defer to Free. The user-account methods and the metadata-availability helper are untouched. New opt-in: metadata.spotify_free_enrichment. When set, the worker puts `_prefer_free` on its OWN client and _free_active() honors it (needs only the package installed — the flag is the opt-in — not the 'Spotify Free' source choice). So a connected user can run bulk enrichment on the no-creds source to spare their official quota, while interactive search/resolve stay official-first (they use a different client that never sets the flag). Default off. Tests: _free_active honors prefer_free (and is inert without the package); search_albums defers to Free — official .sp raises if touched — both under prefer_free AND under budget-exhaustion (the divert that previously never happened). 215 Spotify tests pass.
This commit is contained in:
parent
4a6d6fc17b
commit
38461295c2
3 changed files with 136 additions and 13 deletions
|
|
@ -646,13 +646,21 @@ class SpotifyClient:
|
||||||
term covers the brief window before the auth cache refreshes. When authed
|
term covers the brief window before the auth cache refreshes. When authed
|
||||||
+ healthy the official path returns first, so this never opens.
|
+ healthy the official path returns first, so this never opens.
|
||||||
|
|
||||||
Three activations fall out of this: a no-auth user who chose Spotify
|
Activations that fall out of this: a no-auth user who chose Spotify
|
||||||
Free (free is their source), a connected user mid-rate-limit (free
|
Free (free is their source), a connected user mid-rate-limit (free
|
||||||
bridges the ban), and a connected user who has spent the enrichment
|
bridges the ban), a connected user who has spent the enrichment worker's
|
||||||
worker's real-API daily budget (``_budget_exhausted_use_free``, set by
|
real-API daily budget (``_budget_exhausted_use_free``, set by the worker)
|
||||||
the worker) — so a Spotify-Free user is never paused by the budget, it
|
— so a Spotify-Free user is never paused by the budget — and the worker
|
||||||
just switches to the uncapped free source. See _free_wanted()."""
|
opt-in below. See _free_wanted()."""
|
||||||
from core.spotify_free_metadata import should_use_free_fallback
|
from core.spotify_free_metadata import should_use_free_fallback
|
||||||
|
# Worker opt-in (metadata.spotify_free_enrichment): prefer the no-creds
|
||||||
|
# source for enrichment even while authed + healthy + under budget, to
|
||||||
|
# spare the official quota for interactive use. The flag IS the explicit
|
||||||
|
# opt-in, so it only needs the package installed — not the 'Spotify Free'
|
||||||
|
# metadata-source choice — and it's set only on the enrichment worker's
|
||||||
|
# own client, so interactive search/resolve stay official-first.
|
||||||
|
if getattr(self, '_prefer_free', False) and self._free_installed():
|
||||||
|
return True
|
||||||
if not self._free_available():
|
if not self._free_available():
|
||||||
return False
|
return False
|
||||||
try:
|
try:
|
||||||
|
|
@ -1426,7 +1434,11 @@ class SpotifyClient:
|
||||||
if tracks:
|
if tracks:
|
||||||
return tracks
|
return tracks
|
||||||
|
|
||||||
use_spotify = self.is_spotify_authenticated()
|
# Skip the official API when the no-creds free source should serve this
|
||||||
|
# (no-auth / rate-limited — where auth is already False — plus the
|
||||||
|
# budget-bridge and the worker's prefer-free opt-in, where auth is True
|
||||||
|
# but we deliberately defer to free). The free branch below then runs.
|
||||||
|
use_spotify = self.is_spotify_authenticated() and not self._free_active()
|
||||||
|
|
||||||
if use_spotify:
|
if use_spotify:
|
||||||
try:
|
try:
|
||||||
|
|
@ -1492,7 +1504,11 @@ class SpotifyClient:
|
||||||
artists.sort(key=lambda a: (0 if a.name.lower().strip() == query_lower else 1))
|
artists.sort(key=lambda a: (0 if a.name.lower().strip() == query_lower else 1))
|
||||||
return artists
|
return artists
|
||||||
|
|
||||||
use_spotify = self.is_spotify_authenticated()
|
# Skip the official API when the no-creds free source should serve this
|
||||||
|
# (no-auth / rate-limited — where auth is already False — plus the
|
||||||
|
# budget-bridge and the worker's prefer-free opt-in, where auth is True
|
||||||
|
# but we deliberately defer to free). The free branch below then runs.
|
||||||
|
use_spotify = self.is_spotify_authenticated() and not self._free_active()
|
||||||
|
|
||||||
if use_spotify:
|
if use_spotify:
|
||||||
try:
|
try:
|
||||||
|
|
@ -1574,7 +1590,11 @@ class SpotifyClient:
|
||||||
if albums:
|
if albums:
|
||||||
return albums
|
return albums
|
||||||
|
|
||||||
use_spotify = self.is_spotify_authenticated()
|
# Skip the official API when the no-creds free source should serve this
|
||||||
|
# (no-auth / rate-limited — where auth is already False — plus the
|
||||||
|
# budget-bridge and the worker's prefer-free opt-in, where auth is True
|
||||||
|
# but we deliberately defer to free). The free branch below then runs.
|
||||||
|
use_spotify = self.is_spotify_authenticated() and not self._free_active()
|
||||||
|
|
||||||
if use_spotify:
|
if use_spotify:
|
||||||
try:
|
try:
|
||||||
|
|
@ -1643,7 +1663,7 @@ class SpotifyClient:
|
||||||
# Fallback cache hit — delegate to fallback client which reconstructs enhanced format
|
# Fallback cache hit — delegate to fallback client which reconstructs enhanced format
|
||||||
return self._fallback.get_track_details(track_id)
|
return self._fallback.get_track_details(track_id)
|
||||||
|
|
||||||
if self.is_spotify_authenticated():
|
if self.is_spotify_authenticated() and not self._free_active():
|
||||||
try:
|
try:
|
||||||
track_data = self.sp.track(track_id)
|
track_data = self.sp.track(track_id)
|
||||||
|
|
||||||
|
|
@ -1745,7 +1765,7 @@ class SpotifyClient:
|
||||||
# Fallback cache hit — delegate to fallback client
|
# Fallback cache hit — delegate to fallback client
|
||||||
return self._fallback.get_album(album_id)
|
return self._fallback.get_album(album_id)
|
||||||
|
|
||||||
if self.is_spotify_authenticated():
|
if self.is_spotify_authenticated() and not self._free_active():
|
||||||
try:
|
try:
|
||||||
album_data = self.sp.album(album_id)
|
album_data = self.sp.album(album_id)
|
||||||
if album_data:
|
if album_data:
|
||||||
|
|
@ -1788,7 +1808,7 @@ class SpotifyClient:
|
||||||
if cached:
|
if cached:
|
||||||
return cached
|
return cached
|
||||||
|
|
||||||
if self.is_spotify_authenticated():
|
if self.is_spotify_authenticated() and not self._free_active():
|
||||||
try:
|
try:
|
||||||
# Get first page of tracks
|
# Get first page of tracks
|
||||||
first_page = self.sp.album_tracks(album_id)
|
first_page = self.sp.album_tracks(album_id)
|
||||||
|
|
@ -1882,7 +1902,7 @@ class SpotifyClient:
|
||||||
except Exception as e:
|
except Exception as e:
|
||||||
logger.debug("artist albums cache reuse: %s", e)
|
logger.debug("artist albums cache reuse: %s", e)
|
||||||
|
|
||||||
if self.is_spotify_authenticated():
|
if self.is_spotify_authenticated() and not self._free_active():
|
||||||
try:
|
try:
|
||||||
albums = []
|
albums = []
|
||||||
raw_items = []
|
raw_items = []
|
||||||
|
|
@ -2000,7 +2020,7 @@ class SpotifyClient:
|
||||||
return self._fallback.get_artist(artist_id)
|
return self._fallback.get_artist(artist_id)
|
||||||
return None
|
return None
|
||||||
|
|
||||||
if self.is_spotify_authenticated():
|
if self.is_spotify_authenticated() and not self._free_active():
|
||||||
try:
|
try:
|
||||||
result = self.sp.artist(artist_id)
|
result = self.sp.artist(artist_id)
|
||||||
if result:
|
if result:
|
||||||
|
|
|
||||||
|
|
@ -218,6 +218,21 @@ class SpotifyWorker:
|
||||||
# protect the REAL authenticated API from bans — they don't apply
|
# protect the REAL authenticated API from bans — they don't apply
|
||||||
# to free (a different, anonymous path). Computed once and reused
|
# to free (a different, anonymous path). Computed once and reused
|
||||||
# below; the loop already probes auth, so no extra quota cost.
|
# below; the loop already probes auth, so no extra quota cost.
|
||||||
|
# Worker opt-in: when the user enables Spotify Free for enrichment
|
||||||
|
# (metadata.spotify_free_enrichment), prefer the no-creds source
|
||||||
|
# even while authed + under budget — bulk enrichment is the work
|
||||||
|
# that bans the real API, so this spares the official quota for
|
||||||
|
# interactive use. _free_active() honors this flag (and still falls
|
||||||
|
# back to official only if free can't serve); set only on the
|
||||||
|
# worker's own client, so interactive paths stay official-first.
|
||||||
|
# Cheap config read; harmless when Spotify Free isn't installed.
|
||||||
|
try:
|
||||||
|
from config.settings import config_manager as _cfg
|
||||||
|
self.client._prefer_free = bool(
|
||||||
|
_cfg.get('metadata.spotify_free_enrichment', False))
|
||||||
|
except Exception: # noqa: S110 — prefer-free toggle is best-effort
|
||||||
|
self.client._prefer_free = False
|
||||||
|
|
||||||
budget_exhausted = self._is_daily_budget_exhausted()
|
budget_exhausted = self._is_daily_budget_exhausted()
|
||||||
|
|
||||||
# Daily budget is a REAL-API ban protection. When it's spent, if
|
# Daily budget is a REAL-API ban protection. When it's spent, if
|
||||||
|
|
|
||||||
|
|
@ -305,3 +305,91 @@ def test_client_search_albums_uses_free_via_artist_when_active():
|
||||||
|
|
||||||
assert len(results) == 1 and results[0].name == 'GNX'
|
assert len(results) == 1 and results[0].name == 'GNX'
|
||||||
assert results[0].id == 'al2'
|
assert results[0].id == 'al2'
|
||||||
|
|
||||||
|
|
||||||
|
# ── prefer-free (enrichment opt-in) + the use_spotify root-cause fix ─────────
|
||||||
|
|
||||||
|
def _free_active_with(prefer_free, installed, authed=True, rate_limited=False,
|
||||||
|
selected=False, budget=False):
|
||||||
|
c = SpotifyClient.__new__(SpotifyClient)
|
||||||
|
if prefer_free:
|
||||||
|
c._prefer_free = True
|
||||||
|
if budget:
|
||||||
|
c._budget_exhausted_use_free = True
|
||||||
|
with patch.object(SpotifyClient, 'is_spotify_authenticated', return_value=authed), \
|
||||||
|
patch('core.spotify_client.config_manager') as cm, \
|
||||||
|
patch('core.spotify_client._is_globally_rate_limited', return_value=rate_limited), \
|
||||||
|
patch.object(_sfm, 'spotify_free_installed', return_value=installed):
|
||||||
|
cm.get.side_effect = lambda k, d=None: selected if k == 'metadata.spotify_free' else d
|
||||||
|
return c._free_active()
|
||||||
|
|
||||||
|
|
||||||
|
def test_prefer_free_activates_even_when_authed_healthy_under_budget():
|
||||||
|
# The enrichment opt-in: free serves even though official is perfectly usable.
|
||||||
|
assert _free_active_with(prefer_free=True, installed=True) is True
|
||||||
|
|
||||||
|
|
||||||
|
def test_prefer_free_inert_without_package():
|
||||||
|
# Graceful: opt-in set but SpotipyFree missing -> stays on official.
|
||||||
|
assert _free_active_with(prefer_free=True, installed=False) is False
|
||||||
|
|
||||||
|
|
||||||
|
def test_no_prefer_free_authed_healthy_stays_official():
|
||||||
|
assert _free_active_with(prefer_free=False, installed=True, selected=True) is False
|
||||||
|
|
||||||
|
|
||||||
|
def _client_that_forbids_official(prefer_free=False, budget=False):
|
||||||
|
"""A SpotifyClient whose official .sp blows up if touched + a Free client that
|
||||||
|
serves albums — so a passing search proves official was skipped."""
|
||||||
|
c = SpotifyClient.__new__(SpotifyClient)
|
||||||
|
if prefer_free:
|
||||||
|
c._prefer_free = True
|
||||||
|
if budget:
|
||||||
|
c._budget_exhausted_use_free = True
|
||||||
|
|
||||||
|
class _Sp:
|
||||||
|
def search(self, *a, **k):
|
||||||
|
raise AssertionError("official Spotify API must not be called when deferring to Free")
|
||||||
|
c.sp = _Sp()
|
||||||
|
fake_free = SpotifyFreeMetadataClient()
|
||||||
|
fake_free.search_albums_via_artist = lambda artist, album, limit: [
|
||||||
|
{'id': 'al2', 'name': 'GNX', 'artists': [{'name': 'Kendrick Lamar', 'id': 'art_k'}]}]
|
||||||
|
c._free_meta_client = fake_free
|
||||||
|
return c
|
||||||
|
|
||||||
|
|
||||||
|
def test_search_albums_prefers_free_when_authed_and_prefer_free_set():
|
||||||
|
"""Root-cause regression: prefer_free makes an AUTHED, healthy client defer to
|
||||||
|
Spotify Free instead of the official API. Previously the use_spotify gate
|
||||||
|
(= is_spotify_authenticated()) ignored _free_active() and hit official."""
|
||||||
|
c = _client_that_forbids_official(prefer_free=True)
|
||||||
|
fake_cache = type('C', (), {'get_search_results': lambda *a, **k: None})()
|
||||||
|
with patch.object(SpotifyClient, 'is_spotify_authenticated', return_value=True), \
|
||||||
|
patch('core.spotify_client.config_manager') as cm, \
|
||||||
|
patch('core.spotify_client._is_globally_rate_limited', return_value=False), \
|
||||||
|
patch('core.spotify_client.get_metadata_cache', return_value=fake_cache), \
|
||||||
|
patch.object(_sfm, 'spotify_free_installed', return_value=True):
|
||||||
|
cm.get_spotify_config.return_value = {'client_id': 'x', 'client_secret': 'y'}
|
||||||
|
cm.get.side_effect = lambda k, d=None: d
|
||||||
|
results = c.search_albums('Kendrick Lamar GNX', limit=5,
|
||||||
|
artist='Kendrick Lamar', album='GNX')
|
||||||
|
assert len(results) == 1 and results[0].id == 'al2' # Free served; official untouched
|
||||||
|
|
||||||
|
|
||||||
|
def test_search_albums_diverts_to_free_when_budget_exhausted_and_authed():
|
||||||
|
"""Budget→Free bridge regression: an AUTHED client that has spent the daily
|
||||||
|
budget defers to Free instead of hammering the official API (which the budget
|
||||||
|
exists to protect). This is the divert that previously never happened."""
|
||||||
|
c = _client_that_forbids_official(budget=True)
|
||||||
|
fake_cache = type('C', (), {'get_search_results': lambda *a, **k: None})()
|
||||||
|
with patch.object(SpotifyClient, 'is_spotify_authenticated', return_value=True), \
|
||||||
|
patch('core.spotify_client.config_manager') as cm, \
|
||||||
|
patch('core.spotify_client._is_globally_rate_limited', return_value=False), \
|
||||||
|
patch('core.spotify_client.get_metadata_cache', return_value=fake_cache), \
|
||||||
|
patch.object(_sfm, 'spotify_free_installed', return_value=True):
|
||||||
|
cm.get_spotify_config.return_value = {'client_id': 'x', 'client_secret': 'y'}
|
||||||
|
# metadata.spotify_free=True so the budget path's _free_available() holds
|
||||||
|
cm.get.side_effect = lambda k, d=None: True if k == 'metadata.spotify_free' else d
|
||||||
|
results = c.search_albums('Kendrick Lamar GNX', limit=5,
|
||||||
|
artist='Kendrick Lamar', album='GNX')
|
||||||
|
assert len(results) == 1 and results[0].id == 'al2'
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue