From 38461295c21eeb26a7e6f3c3d8dd761861346920 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Tue, 9 Jun 2026 12:24:57 -0700 Subject: [PATCH] =?UTF-8?q?Spotify:=20enrichment=20can=20prefer=20Free,=20?= =?UTF-8?q?and=20the=20budget=E2=86=92Free=20bridge=20actually=20diverts?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- core/spotify_client.py | 46 ++++++++++----- core/spotify_worker.py | 15 +++++ tests/test_spotify_free_metadata.py | 88 +++++++++++++++++++++++++++++ 3 files changed, 136 insertions(+), 13 deletions(-) diff --git a/core/spotify_client.py b/core/spotify_client.py index 66b120bf..64c3c495 100644 --- a/core/spotify_client.py +++ b/core/spotify_client.py @@ -646,13 +646,21 @@ class SpotifyClient: term covers the brief window before the auth cache refreshes. When authed + 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 - bridges the ban), and a connected user who has spent the enrichment - worker's real-API daily budget (``_budget_exhausted_use_free``, set by - the worker) — so a Spotify-Free user is never paused by the budget, it - just switches to the uncapped free source. See _free_wanted().""" + bridges the ban), a connected user who has spent the enrichment worker's + real-API daily budget (``_budget_exhausted_use_free``, set by the worker) + — so a Spotify-Free user is never paused by the budget — and the worker + opt-in below. See _free_wanted().""" 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(): return False try: @@ -1426,7 +1434,11 @@ class SpotifyClient: if 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: try: @@ -1492,7 +1504,11 @@ class SpotifyClient: artists.sort(key=lambda a: (0 if a.name.lower().strip() == query_lower else 1)) 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: try: @@ -1574,7 +1590,11 @@ class SpotifyClient: if 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: try: @@ -1643,7 +1663,7 @@ class SpotifyClient: # Fallback cache hit — delegate to fallback client which reconstructs enhanced format return self._fallback.get_track_details(track_id) - if self.is_spotify_authenticated(): + if self.is_spotify_authenticated() and not self._free_active(): try: track_data = self.sp.track(track_id) @@ -1745,7 +1765,7 @@ class SpotifyClient: # Fallback cache hit — delegate to fallback client return self._fallback.get_album(album_id) - if self.is_spotify_authenticated(): + if self.is_spotify_authenticated() and not self._free_active(): try: album_data = self.sp.album(album_id) if album_data: @@ -1788,7 +1808,7 @@ class SpotifyClient: if cached: return cached - if self.is_spotify_authenticated(): + if self.is_spotify_authenticated() and not self._free_active(): try: # Get first page of tracks first_page = self.sp.album_tracks(album_id) @@ -1882,7 +1902,7 @@ class SpotifyClient: except Exception as 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: albums = [] raw_items = [] @@ -2000,7 +2020,7 @@ class SpotifyClient: return self._fallback.get_artist(artist_id) return None - if self.is_spotify_authenticated(): + if self.is_spotify_authenticated() and not self._free_active(): try: result = self.sp.artist(artist_id) if result: diff --git a/core/spotify_worker.py b/core/spotify_worker.py index 49ddb429..0d83354e 100644 --- a/core/spotify_worker.py +++ b/core/spotify_worker.py @@ -218,6 +218,21 @@ class SpotifyWorker: # protect the REAL authenticated API from bans — they don't apply # to free (a different, anonymous path). Computed once and reused # 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() # Daily budget is a REAL-API ban protection. When it's spent, if diff --git a/tests/test_spotify_free_metadata.py b/tests/test_spotify_free_metadata.py index a66f4975..6df8bdc2 100644 --- a/tests/test_spotify_free_metadata.py +++ b/tests/test_spotify_free_metadata.py @@ -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 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'