From 23927bab0776b5d6e310ddb2ec8ac263ebffdae2 Mon Sep 17 00:00:00 2001 From: Marcel van der Veldt Date: Sat, 22 Feb 2025 10:20:23 +0100 Subject: [PATCH] Fix: race condition in provider config handling The config object within a provider is meant to be read-only but it was being used by providers also to update refresh tokens etc. This lead to race conditions because the provider has a cached copy of the config object. Also a failed login when the provider was already loaded was not properly handled. That is all adjusted now. --- music_assistant/controllers/config.py | 3 -- music_assistant/mass.py | 5 ++ music_assistant/models/provider.py | 10 ++++ music_assistant/providers/builtin/__init__.py | 2 +- .../providers/radiobrowser/__init__.py | 8 +-- music_assistant/providers/spotify/__init__.py | 16 +++--- music_assistant/providers/tidal/__init__.py | 52 +++++++++---------- 7 files changed, 53 insertions(+), 43 deletions(-) diff --git a/music_assistant/controllers/config.py b/music_assistant/controllers/config.py index 6a0f6a52..1749d87d 100644 --- a/music_assistant/controllers/config.py +++ b/music_assistant/controllers/config.py @@ -707,9 +707,6 @@ class ConfigController: self.set(f"{CONF_PROVIDERS}/{provider_instance}/{key}", value) return self.set(f"{CONF_PROVIDERS}/{provider_instance}/values/{key}", value) - # also update the cached value in the provider itself - if prov := self.mass.get_provider(provider_instance, return_unavailable=True): - prov.config.values[key].value = value def set_raw_core_config_value(self, core_module: str, key: str, value: ConfigValueType) -> None: """ diff --git a/music_assistant/mass.py b/music_assistant/mass.py index 0007d487..aaf2d628 100644 --- a/music_assistant/mass.py +++ b/music_assistant/mass.py @@ -577,6 +577,11 @@ class MusicAssistant: await self._update_available_providers_cache() self.signal_event(EventType.PROVIDERS_UPDATED, data=self.get_providers()) + async def unload_provider_with_error(self, instance_id: str, error: str) -> None: + """Unload a provider when it got into trouble which needs user interaction.""" + self.config.set(f"{CONF_PROVIDERS}/{instance_id}/last_error", error) + await self.unload_provider(instance_id) + def _register_api_commands(self) -> None: """Register all methods decorated as api_command within a class(instance).""" for cls in ( diff --git a/music_assistant/models/provider.py b/music_assistant/models/provider.py index 01c21d05..1fa03893 100644 --- a/music_assistant/models/provider.py +++ b/music_assistant/models/provider.py @@ -106,6 +106,16 @@ class Provider: return f"{self.manifest.name} {postfix}" return self.manifest.name + def update_config_value(self, key: str, value: Any, encrypted: bool = False) -> None: + """Update a config value.""" + self.mass.config.set_raw_provider_config_value(self.instance_id, key, value, encrypted) + # also update the cached copy within the provider instance + self.config.values[key].value = value + + def unload_with_error(self, error: str) -> None: + """Unload provider with error message.""" + self.mass.call_later(1, self.mass.unload_provider, self.instance_id, error) + def to_dict(self, *args, **kwargs) -> dict[str, Any]: """Return Provider(instance) as serializable dict.""" return { diff --git a/music_assistant/providers/builtin/__init__.py b/music_assistant/providers/builtin/__init__.py index 9dacc398..8a84b4bd 100644 --- a/music_assistant/providers/builtin/__init__.py +++ b/music_assistant/providers/builtin/__init__.py @@ -368,7 +368,7 @@ class BuiltinProvider(MusicProvider): if media_type == MediaType.PLAYLIST and prov_item_id in BUILTIN_PLAYLISTS: # user wants to disable/remove one of our builtin playlists # to prevent it comes back, we mark it as disabled in config - self.mass.config.set_raw_provider_config_value(self.instance_id, prov_item_id, False) + self.update_config_value(prov_item_id, False) return True if media_type == MediaType.TRACK: # regular manual track URL/path diff --git a/music_assistant/providers/radiobrowser/__init__.py b/music_assistant/providers/radiobrowser/__init__.py index a70c0944..c8902da0 100644 --- a/music_assistant/providers/radiobrowser/__init__.py +++ b/music_assistant/providers/radiobrowser/__init__.py @@ -207,9 +207,7 @@ class RadioBrowserProvider(MusicProvider): return False self.logger.debug("Adding radio %s to stored radios", item.item_id) stored_radios = [*stored_radios, item.item_id] - self.mass.config.set_raw_provider_config_value( - self.instance_id, CONF_STORED_RADIOS, stored_radios - ) + self.update_config_value(CONF_STORED_RADIOS, stored_radios) return True async def library_remove(self, prov_item_id: str, media_type: MediaType) -> bool: @@ -221,9 +219,7 @@ class RadioBrowserProvider(MusicProvider): return False self.logger.debug("Removing radio %s from stored radios", prov_item_id) stored_radios = [x for x in stored_radios if x != prov_item_id] - self.mass.config.set_raw_provider_config_value( - self.instance_id, CONF_STORED_RADIOS, stored_radios - ) + self.update_config_value(CONF_STORED_RADIOS, stored_radios) return True @use_cache(3600 * 24) diff --git a/music_assistant/providers/spotify/__init__.py b/music_assistant/providers/spotify/__init__.py index 804ac419..dff4ef40 100644 --- a/music_assistant/providers/spotify/__init__.py +++ b/music_assistant/providers/spotify/__init__.py @@ -829,11 +829,13 @@ class SpotifyProvider(MusicProvider): if response.status != 200: err = await response.text() if "revoked" in err: + err_msg = f"Failed to refresh access token: {err}" # clear refresh token if it's invalid - self.mass.config.set_raw_provider_config_value( - self.instance_id, CONF_REFRESH_TOKEN, "" - ) - raise LoginFailed(f"Failed to refresh access token: {err}") + self.update_config_value(CONF_REFRESH_TOKEN, None) + if self.available: + # If we're already loaded, we need to unload and set an error + self.unload_with_error(err_msg) + raise LoginFailed(err_msg) # the token failed to refresh, we allow one retry await asyncio.sleep(2) continue @@ -843,13 +845,13 @@ class SpotifyProvider(MusicProvider): self.logger.debug("Successfully refreshed access token") break else: + if self.available: + self.mass.create_task(self.mass.unload_provider_with_error(self.instance_id)) raise LoginFailed(f"Failed to refresh access token: {err}") # make sure that our updated creds get stored in memory + config self._auth_info = auth_info - self.mass.config.set_raw_provider_config_value( - self.instance_id, CONF_REFRESH_TOKEN, auth_info["refresh_token"], encrypted=True - ) + self.update_config_value(CONF_REFRESH_TOKEN, auth_info["refresh_token"], encrypted=True) # check if librespot still has valid auth args = [ self._librespot_bin, diff --git a/music_assistant/providers/tidal/__init__.py b/music_assistant/providers/tidal/__init__.py index 61859920..20661290 100644 --- a/music_assistant/providers/tidal/__init__.py +++ b/music_assistant/providers/tidal/__init__.py @@ -354,24 +354,13 @@ class TidalProvider(MusicProvider): _tidal_session: TidalSession | None = None _tidal_user_id: str # rate limiter needs to be specified on provider-level, - # so make it an instance attribute + # so make it an class attribute throttler = ThrottlerManager(rate_limit=1, period=2) async def handle_async_init(self) -> None: """Handle async initialization of the provider.""" self._tidal_user_id = str(self.config.get_value(CONF_USER_ID)) - try: - self._tidal_session = await self._get_tidal_session() - except Exception as err: - if "401 Client Error: Unauthorized" in str(err): - self.mass.config.set_raw_provider_config_value( - self.instance_id, CONF_AUTH_TOKEN, None - ) - self.mass.config.set_raw_provider_config_value( - self.instance_id, CONF_REFRESH_TOKEN, None - ) - raise LoginFailed("Credentials expired, you need to re-setup") - raise + await self._get_tidal_session() @property def supported_features(self) -> set[ProviderFeature]: @@ -676,27 +665,38 @@ class TidalProvider(MusicProvider): > (datetime.now() + timedelta(days=1)) ): return self._tidal_session - self._tidal_session = await self._load_tidal_session( - token_type="Bearer", - quality=str(self.config.get_value(CONF_QUALITY)), - access_token=str(self.config.get_value(CONF_AUTH_TOKEN)), - refresh_token=str(self.config.get_value(CONF_REFRESH_TOKEN)), - expiry_time=datetime.fromisoformat(str(self.config.get_value(CONF_EXPIRY_TIME))), - ) - self.mass.config.set_raw_provider_config_value( - self.config.instance_id, + + try: + self._tidal_session = await self._load_tidal_session( + token_type="Bearer", + quality=str(self.config.get_value(CONF_QUALITY)), + access_token=str(self.config.get_value(CONF_AUTH_TOKEN)), + refresh_token=str(self.config.get_value(CONF_REFRESH_TOKEN)), + expiry_time=datetime.fromisoformat(str(self.config.get_value(CONF_EXPIRY_TIME))), + ) + except Exception as err: + if "401 Client Error: Unauthorized" in str(err): + err_msg = "Credentials expired, you need to re-setup" + # clear stored creds + self.update_config_value(CONF_AUTH_TOKEN, None) + self.update_config_value(CONF_REFRESH_TOKEN, None) + # if we're already loaded and the login got invalid, we need to unload + if self.available: + self.unload_with_error(err_msg) + raise LoginFailed(err_msg) + raise + + self.update_config_value( CONF_AUTH_TOKEN, self._tidal_session.access_token, encrypted=True, ) - self.mass.config.set_raw_provider_config_value( - self.config.instance_id, + self.update_config_value( CONF_REFRESH_TOKEN, self._tidal_session.refresh_token, encrypted=True, ) - self.mass.config.set_raw_provider_config_value( - self.config.instance_id, + self.update_config_value( CONF_EXPIRY_TIME, self._tidal_session.expiry_time.isoformat(), ) -- 2.34.1