diff --git a/music_assistant/controllers/players/protocol_linking.py b/music_assistant/controllers/players/protocol_linking.py index f4330e5cc8..9703a9856a 100644 --- a/music_assistant/controllers/players/protocol_linking.py +++ b/music_assistant/controllers/players/protocol_linking.py @@ -1007,6 +1007,9 @@ def _remove_protocol_id_from_cache( def _save_protocol_parent_id(self, protocol_player_id: str, parent_id: str) -> None: """Save the parent ID for a protocol player for persistence across restarts.""" + # Only save if the player config still exists to avoid creating partial entries + if not self.mass.config.get(f"{CONF_PLAYERS}/{protocol_player_id}"): + return conf_key = f"{CONF_PLAYERS}/{protocol_player_id}/values/{CONF_PROTOCOL_PARENT_ID}" self.mass.config.set(conf_key, parent_id) @@ -1134,6 +1137,9 @@ def _cleanup_protocol_links(self, player: Player) -> None: ) else: parent_player.update_state() + else: + # Parent not registered yet — still purge the cached id + self._remove_protocol_id_from_cache(parent_id, player.player_id) else: # Native/universal player being removed: handle all linked protocol players. # Collect all known protocol IDs from both active links and cached state, diff --git a/music_assistant/providers/chromecast/provider.py b/music_assistant/providers/chromecast/provider.py index b61ac39a81..c1836573e6 100644 --- a/music_assistant/providers/chromecast/provider.py +++ b/music_assistant/providers/chromecast/provider.py @@ -189,7 +189,7 @@ async def _create_and_register_player( await castplayer.async_setup() await self.mass.players.register_or_update(castplayer) # Set up Sendspin bridge - await self.bridge_manager.setup_bridge(castplayer) + await self.bridge_manager.evaluate_bridge(castplayer) finally: self._pending_discoveries.discard(player_id) diff --git a/music_assistant/providers/chromecast/sendspin_bridge.py b/music_assistant/providers/chromecast/sendspin_bridge.py index 587efb6dfc..613c08dbbd 100644 --- a/music_assistant/providers/chromecast/sendspin_bridge.py +++ b/music_assistant/providers/chromecast/sendspin_bridge.py @@ -635,6 +635,13 @@ def __init__(self, provider: ChromecastProvider) -> None: self._unsub_config_updated = self.mass.subscribe( self._on_player_config_updated, EventType.PLAYER_CONFIG_UPDATED ) + self._unsub_player_updated = self.mass.subscribe( + self._on_player_updated, EventType.PLAYER_UPDATED + ) + self._airplay_was_available = self.mass.get_provider("airplay") is not None + self._unsub_providers_updated = self.mass.subscribe( + self._on_providers_updated, EventType.PROVIDERS_UPDATED + ) @property def sendspin_provider(self) -> SendspinProvider | None: @@ -651,6 +658,73 @@ def sendspin_server(self) -> SendspinServer | None: return provider.server_api return None + def _should_have_bridge(self, cast_player: ChromecastPlayer) -> bool: + """ + Return whether this cast player should have a Sendspin bridge. + + :param cast_player: The Chromecast player to evaluate. + """ + # Audio groups (non-stereo-pair) have their own playback mechanism + if cast_player.cast_info.is_audio_group and not cast_player.cast_info.is_multichannel_group: + return False + + if not get_bridge_client_id(cast_player): + return False + + manufacturer = cast_player.device_info.manufacturer or "" + model = cast_player.device_info.model or "" + if is_sendspin_cast_blocked(manufacturer, model): + self.logger.debug( + "Skipping Sendspin Cast bridge for %s (%s / %s) — device is blocklisted", + cast_player.display_name, + manufacturer, + model, + ) + return False + + if not self.sendspin_server: + self.logger.debug( + "Sendspin provider not available, skipping bridge for %s", + cast_player.display_name, + ) + return False + + # Prefer the airplay bridge over the cast bridge (better sync performance) + # when the cast player shares a parent with an airplay protocol player. + if cast_player.protocol_parent_id: + parent_player = self.mass.players.get_player(cast_player.protocol_parent_id) + if parent_player: + for protocol in parent_player.linked_output_protocols: + if protocol.protocol_domain != "airplay": + continue + airplay_player = self.mass.players.get_player(protocol.output_protocol_id) + if airplay_player and airplay_player.available: + return False + + return True + + async def evaluate_bridge(self, cast_player: ChromecastPlayer) -> None: + """ + Reconcile the Sendspin bridge state for this cast player. + + Creates or removes the bridge to match the desired state derived from + the current player graph. Idempotent and safe to call repeatedly. + + :param cast_player: The Chromecast player to evaluate. + """ + player_id = cast_player.player_id + desired = self._should_have_bridge(cast_player) + has_bridge = player_id in self._bridges or player_id in self._claimed_clients + + if desired and not has_bridge: + await self.setup_bridge(cast_player) + elif not desired and has_bridge: + self.logger.info( + "Removing redundant Sendspin Cast bridge for %s", + cast_player.display_name, + ) + await self.remove_bridge(player_id) + async def setup_bridge(self, cast_player: ChromecastPlayer) -> None: """ Set up a Sendspin bridge for a Chromecast player. @@ -671,45 +745,19 @@ async def setup_bridge(self, cast_player: ChromecastPlayer) -> None: ) return - # Skip audio groups (non-stereo-pair) — they have their own playback mechanism - if ( - cast_player.cast_info.is_audio_group - and not cast_player.cast_info.is_multichannel_group - ): + if not self._should_have_bridge(cast_player): return - # skip if the cast player's parent also has airplay linked - # (we prefer the airplay bridge due to better sync performance) - if cast_player.protocol_parent_id: - parent_player = self.mass.players.get_player(cast_player.protocol_parent_id) - if parent_player: - for protocol in parent_player.linked_output_protocols: - if protocol.protocol_domain == "airplay": - return - bridge_client_id = get_bridge_client_id(cast_player) - if not bridge_client_id: - return - - # Skip devices on the blocklist - manufacturer = cast_player.device_info.manufacturer or "" - model = cast_player.device_info.model or "" - if is_sendspin_cast_blocked(manufacturer, model): - self.logger.debug( - "Skipping Sendspin Cast bridge for %s (%s / %s) — device is blocklisted", - cast_player.display_name, - manufacturer, - model, - ) - return - sendspin_server = self.sendspin_server - if not sendspin_server: + if bridge_client_id is None or sendspin_server is None: self.logger.debug( - "Sendspin provider not available, skipping bridge for %s", + "Skipping bridge setup for %s — preconditions changed since evaluation", cast_player.display_name, ) return + manufacturer = cast_player.device_info.manufacturer or "" + model = cast_player.device_info.model or "" existing_client_id = self._find_existing_sendspin_client( sendspin_server, bridge_client_id, cast_player @@ -776,12 +824,23 @@ async def remove_bridge(self, cast_player_id: str) -> None: :param cast_player_id: The player ID to remove the bridge for. """ async with self._lock: - if bridge := self._bridges.pop(cast_player_id, None): - await bridge.stop() + bridge = self._bridges.pop(cast_player_id, None) + client_id = bridge.bridge_client_id if bridge else None claimed_client_id = self._claimed_clients.pop(cast_player_id, None) if claimed_client_id and (unsub := self._rebridge_unsubs.pop(claimed_client_id, None)): with suppress(Exception): unsub() + target_id = client_id or claimed_client_id + if target_id: + if self.mass.players.get_player(target_id): + await self.mass.players.unregister(target_id, permanent=True) + else: + self.mass.players.delete_player_config(target_id) + if bridge: + await bridge.stop() + elif claimed_client_id and (sendspin_server := self.sendspin_server): + with suppress(Exception): + await sendspin_server.remove_client(claimed_client_id) self.logger.debug("Sendspin bridge removed for Chromecast player %s", cast_player_id) @@ -798,6 +857,8 @@ async def stop_all(self) -> None: async def close(self) -> None: """Stop all bridges and unsubscribe event listeners.""" self._unsub_config_updated() + self._unsub_player_updated() + self._unsub_providers_updated() for unsub in list(self._rebridge_unsubs.values()): with suppress(Exception): unsub() @@ -913,6 +974,30 @@ def get_bridge(self, cast_player_id: str) -> SendspinChromecastBridge | None: """ return self._bridges.get(cast_player_id) + async def _on_player_updated(self, event: MassEvent) -> None: + """Re-evaluate bridge state for cast players affected by a player update.""" + if not event.object_id: + return + updated_player = self.mass.players.get_player(event.object_id) + match_ids = {event.object_id} + if updated_player and updated_player.protocol_parent_id: + match_ids.add(updated_player.protocol_parent_id) + for player in self.provider.players: + cast_player = cast("ChromecastPlayer", player) + if cast_player.player_id in match_ids or ( + cast_player.protocol_parent_id and cast_player.protocol_parent_id in match_ids + ): + await self.evaluate_bridge(cast_player) + + async def _on_providers_updated(self, event: MassEvent) -> None: + """Re-evaluate cast bridges when the airplay provider availability flips.""" + airplay_available = self.mass.get_provider("airplay") is not None + if airplay_available == self._airplay_was_available: + return + self._airplay_was_available = airplay_available + for player in self.provider.players: + await self.evaluate_bridge(cast("ChromecastPlayer", player)) + def get_bridge_by_client_id(self, bridge_client_id: str) -> SendspinChromecastBridge | None: """Return the bridge whose `bridge_client_id` matches, if any. diff --git a/music_assistant/providers/universal_player/provider.py b/music_assistant/providers/universal_player/provider.py index f326852c41..5f45a6fc6f 100644 --- a/music_assistant/providers/universal_player/provider.py +++ b/music_assistant/providers/universal_player/provider.py @@ -59,7 +59,7 @@ async def discover_players(self) -> None: # The stored protocol IDs enable fast matching when protocols register await self._restore_player(player_conf.player_id) - async def _restore_player(self, player_id: str) -> None: + async def _restore_player(self, player_id: str) -> None: # noqa: PLR0915 """ Restore a universal player from config. @@ -78,7 +78,7 @@ async def _restore_player(self, player_id: str) -> None: stored_identifiers = values.get(CONF_DEVICE_IDENTIFIERS, {}) stored_device_info = values.get(CONF_DEVICE_INFO, {}) - # Filter out protocol IDs that are no longer PROTOCOL type players + # Filter out protocol IDs that are no longer valid for this universal valid_protocol_ids = [] for protocol_id in stored_protocol_ids: protocol_config = self.mass.config.get(f"{CONF_PLAYERS}/{protocol_id}") @@ -87,15 +87,27 @@ async def _restore_player(self, player_id: str) -> None: valid_protocol_ids.append(protocol_id) continue protocol_player_type = protocol_config.get("player_type") - if protocol_player_type == "protocol": - valid_protocol_ids.append(protocol_id) - else: + if protocol_player_type != "protocol": self.logger.info( "Removing %s from universal player %s - player type changed to %s", protocol_id, player_id, protocol_player_type, ) + continue + protocol_values = protocol_config.get("values") or {} + if not protocol_values.get("protocol_parent_id"): + self.logger.info( + "Deleting orphaned protocol player config %s (was linked to %s)", + protocol_id, + player_id, + ) + if self.mass.players.get_player(protocol_id): + await self.mass.players.unregister(protocol_id, permanent=True) + else: + self.mass.players.delete_player_config(protocol_id) + continue + valid_protocol_ids.append(protocol_id) # If no valid protocol IDs remain, delete this stale universal player if not valid_protocol_ids: diff --git a/tests/core/test_protocol_linking.py b/tests/core/test_protocol_linking.py index d58ec40ab4..02abbcf8e2 100644 --- a/tests/core/test_protocol_linking.py +++ b/tests/core/test_protocol_linking.py @@ -6180,7 +6180,14 @@ class TestProtocolParentIdPersistence: def test_parent_id_saved_for_universal_parent(self, mock_mass: MagicMock) -> None: """protocol_parent_id should be saved when linking to a universal player.""" controller = PlayerController(mock_mass) - mock_mass.config.get = MagicMock(return_value=[]) + + def _config_get(key: str, default: object = None) -> object: + # Protocol player config must exist for _save_protocol_parent_id to write + if key == "players/airplay_test": + return {"enabled": True} + return default if default is not None else [] + + mock_mass.config.get = MagicMock(side_effect=_config_get) universal_provider = create_mock_universal_provider(mock_mass) parent = UniversalPlayer( @@ -6515,3 +6522,131 @@ def test_keeps_valid_parent_ids(self, mock_mass: MagicMock) -> None: # Verify no set calls were made to clear the parent_id for call in mock_mass.config.set.call_args_list: assert "protocol_parent_id" not in str(call), "Valid parent_id should not be cleared" + + +class TestUniversalPlayerRestoreOrphanCleanup: + """Tests for UniversalPlayerProvider._restore_player orphan cleanup behavior.""" + + @staticmethod + def _setup_config_get( + mock_mass: MagicMock, + universal_id: str, + linked_protocol_ids: list[str], + protocol_configs: dict[str, dict[str, object]], + ) -> None: + """Wire mass.config.get to return universal/protocol configs by key.""" + universal_conf = { + "values": { + "linked_protocol_ids": list(linked_protocol_ids), + "device_identifiers": {}, + "device_info": {}, + }, + "name": "Test Universal", + } + + def _get(key: str, default: object = None) -> object: + if key == f"players/{universal_id}": + return universal_conf + for pid, conf in protocol_configs.items(): + if key == f"players/{pid}": + return conf + return default + + mock_mass.config.get.side_effect = _get + + @pytest.mark.asyncio + async def test_orphan_protocol_deleted_when_not_registered(self, mock_mass: MagicMock) -> None: + """Orphan protocol with no live registration → delete_player_config path.""" + provider = create_mock_universal_provider(mock_mass) + universal_id = "up_test" + orphan_id = "spb_orphan" + + self._setup_config_get( + mock_mass, + universal_id, + [orphan_id], + { + orphan_id: { + "player_type": "protocol", + "values": {"protocol_parent_id": None}, + }, + }, + ) + mock_mass.players = MagicMock() + mock_mass.players.get_player = MagicMock(return_value=None) + mock_mass.players.delete_player_config = MagicMock() + mock_mass.players.unregister = AsyncMock() + mock_mass.config.remove_player_config = AsyncMock() + + await provider._restore_player(universal_id) + + mock_mass.players.delete_player_config.assert_called_once_with(orphan_id) + mock_mass.players.unregister.assert_not_called() + # With no valid protocols left, the universal is also removed + mock_mass.config.remove_player_config.assert_called_once_with(universal_id) + + @pytest.mark.asyncio + async def test_orphan_protocol_unregistered_when_active(self, mock_mass: MagicMock) -> None: + """Orphan protocol that is currently registered → unregister(permanent=True) path.""" + provider = create_mock_universal_provider(mock_mass) + universal_id = "up_test" + orphan_id = "spb_orphan" + + self._setup_config_get( + mock_mass, + universal_id, + [orphan_id], + { + orphan_id: { + "player_type": "protocol", + "values": {"protocol_parent_id": None}, + }, + }, + ) + mock_mass.players = MagicMock() + mock_mass.players.get_player = MagicMock(return_value=MagicMock()) + mock_mass.players.delete_player_config = MagicMock() + mock_mass.players.unregister = AsyncMock() + mock_mass.config.remove_player_config = AsyncMock() + + await provider._restore_player(universal_id) + + mock_mass.players.unregister.assert_awaited_once_with(orphan_id, permanent=True) + mock_mass.players.delete_player_config.assert_not_called() + mock_mass.config.remove_player_config.assert_called_once_with(universal_id) + + @pytest.mark.asyncio + async def test_valid_protocol_kept_and_no_cleanup(self, mock_mass: MagicMock) -> None: + """Protocol with a valid protocol_parent_id is kept and no cleanup runs.""" + provider = create_mock_universal_provider(mock_mass) + universal_id = "up_test" + protocol_id = "spb_valid" + + self._setup_config_get( + mock_mass, + universal_id, + [protocol_id], + { + protocol_id: { + "player_type": "protocol", + "values": {"protocol_parent_id": universal_id}, + }, + }, + ) + mock_mass.players = MagicMock() + mock_mass.players.get_player = MagicMock(return_value=None) + mock_mass.players.delete_player_config = MagicMock() + mock_mass.players.unregister = AsyncMock() + mock_mass.players.register_or_update = AsyncMock() + mock_mass.config.remove_player_config = AsyncMock() + mock_mass.config.get_base_player_config.return_value = create_mock_config("Test Universal") + + await provider._restore_player(universal_id) + + mock_mass.players.delete_player_config.assert_not_called() + mock_mass.players.unregister.assert_not_called() + mock_mass.config.remove_player_config.assert_not_called() + # Universal is constructed and registered with the valid protocol kept + mock_mass.players.register_or_update.assert_awaited_once() + registered = mock_mass.players.register_or_update.call_args.args[0] + assert protocol_id in registered._protocol_player_ids