Improve enabling of Google local fulfillment (#109192)

* Improve enabling of Google local fulfillment

* Add test

* Improve test coverage

homeassistant/components/cloud/client.py

@@ -29,6 +29,8 @@ from . import alexa_config, google_config
 from .const import DISPATCHER_REMOTE_UPDATE, DOMAIN
 from .prefs import CloudPreferences
 
+_LOGGER = logging.getLogger(__name__)
+
 VALID_REPAIR_TRANSLATION_KEYS = {
     "warn_bad_custom_domain_configuration",
     "reset_bad_custom_domain_configuration",
@@ -149,6 +151,7 @@ class CloudClient(Interface):
 
     async def cloud_connected(self) -> None:
         """When cloud is connected."""
+        _LOGGER.debug("cloud_connected")
         is_new_user = await self.prefs.async_set_username(self.cloud.username)
 
         async def enable_alexa(_: Any) -> None:
@@ -196,6 +199,9 @@ class CloudClient(Interface):
 
     async def cloud_disconnected(self) -> None:
         """When cloud disconnected."""
+        _LOGGER.debug("cloud_disconnected")
+        if self._google_config:
+            self._google_config.async_disable_local_sdk()
 
     async def cloud_started(self) -> None:
         """When cloud is started."""
@@ -207,6 +213,8 @@ class CloudClient(Interface):
         """Cleanup some stuff after logout."""
         await self.prefs.async_set_username(None)
 
+        if self._google_config:
+            self._google_config.async_deinitialize()
         self._google_config = None
 
     @callback

homeassistant/components/cloud/google_config.py

@@ -23,6 +23,7 @@ from homeassistant.components.homeassistant.exposed_entities import (
 from homeassistant.components.sensor import SensorDeviceClass
 from homeassistant.const import CLOUD_NEVER_EXPOSED_ENTITIES
 from homeassistant.core import (
+    CALLBACK_TYPE,
     CoreState,
     Event,
     HomeAssistant,
@@ -144,6 +145,7 @@ class CloudGoogleConfig(AbstractConfig):
         self._prefs = prefs
         self._cloud = cloud
         self._sync_entities_lock = asyncio.Lock()
+        self._on_deinitialize: list[CALLBACK_TYPE] = []
 
     @property
     def enabled(self) -> bool:
@@ -209,9 +211,11 @@ class CloudGoogleConfig(AbstractConfig):
 
     async def async_initialize(self) -> None:
         """Perform async initialization of config."""
+        _LOGGER.debug("async_initialize")
         await super().async_initialize()
 
         async def on_hass_started(hass: HomeAssistant) -> None:
+            _LOGGER.debug("async_initialize on_hass_started")
             if self._prefs.google_settings_version != GOOGLE_SETTINGS_VERSION:
                 _LOGGER.info(
                     "Start migration of Google Assistant settings from v%s to v%s",
@@ -238,16 +242,19 @@ class CloudGoogleConfig(AbstractConfig):
                 await self._prefs.async_update(
                     google_settings_version=GOOGLE_SETTINGS_VERSION
                 )
-            async_listen_entity_updates(
-                self.hass, CLOUD_GOOGLE, self._async_exposed_entities_updated
+            self._on_deinitialize.append(
+                async_listen_entity_updates(
+                    self.hass, CLOUD_GOOGLE, self._async_exposed_entities_updated
+                )
             )
 
         async def on_hass_start(hass: HomeAssistant) -> None:
+            _LOGGER.debug("async_initialize on_hass_start")
             if self.enabled and GOOGLE_DOMAIN not in self.hass.config.components:
                 await async_setup_component(self.hass, GOOGLE_DOMAIN, {})
 
-        start.async_at_start(self.hass, on_hass_start)
-        start.async_at_started(self.hass, on_hass_started)
+        self._on_deinitialize.append(start.async_at_start(self.hass, on_hass_start))
+        self._on_deinitialize.append(start.async_at_started(self.hass, on_hass_started))
 
         # Remove any stored user agent id that is not ours
         remove_agent_user_ids = []
@@ -255,18 +262,33 @@ class CloudGoogleConfig(AbstractConfig):
             if agent_user_id != self.agent_user_id:
                 remove_agent_user_ids.append(agent_user_id)
 
+        if remove_agent_user_ids:
+            _LOGGER.debug("remove non cloud agent_user_ids: %s", remove_agent_user_ids)
         for agent_user_id in remove_agent_user_ids:
             await self.async_disconnect_agent_user(agent_user_id)
 
-        self._prefs.async_listen_updates(self._async_prefs_updated)
-        self.hass.bus.async_listen(
-            er.EVENT_ENTITY_REGISTRY_UPDATED,
-            self._handle_entity_registry_updated,
+        self._on_deinitialize.append(
+            self._prefs.async_listen_updates(self._async_prefs_updated)
         )
-        self.hass.bus.async_listen(
-            dr.EVENT_DEVICE_REGISTRY_UPDATED,
-            self._handle_device_registry_updated,
+        self._on_deinitialize.append(
+            self.hass.bus.async_listen(
+                er.EVENT_ENTITY_REGISTRY_UPDATED,
+                self._handle_entity_registry_updated,
+            )
         )
+        self._on_deinitialize.append(
+            self.hass.bus.async_listen(
+                dr.EVENT_DEVICE_REGISTRY_UPDATED,
+                self._handle_device_registry_updated,
+            )
+        )
+
+    @callback
+    def async_deinitialize(self) -> None:
+        """Remove listeners."""
+        _LOGGER.debug("async_deinitialize")
+        while self._on_deinitialize:
+            self._on_deinitialize.pop()()
 
     def should_expose(self, state: State) -> bool:
         """If a state object should be exposed."""
@@ -365,6 +387,7 @@ class CloudGoogleConfig(AbstractConfig):
 
     async def _async_prefs_updated(self, prefs: CloudPreferences) -> None:
         """Handle updated preferences."""
+        _LOGGER.debug("_async_prefs_updated")
         if not self._cloud.is_logged_in:
             if self.is_reporting_state:
                 self.async_disable_report_state()

homeassistant/components/google_assistant/helpers.py

@@ -316,6 +316,7 @@ class AbstractConfig(ABC):
     @callback
     def async_enable_local_sdk(self) -> None:
         """Enable the local SDK."""
+        _LOGGER.debug("async_enable_local_sdk")
         setup_successful = True
         setup_webhook_ids = []
 
@@ -324,11 +325,16 @@ class AbstractConfig(ABC):
             self._local_sdk_active = False
             return
 
-        for user_agent_id, _ in self._store.agent_user_ids.items():
+        for user_agent_id in self._store.agent_user_ids:
             if (webhook_id := self.get_local_webhook_id(user_agent_id)) is None:
                 setup_successful = False
                 break
 
+            _LOGGER.debug(
+                "Register webhook handler %s for agent user id %s",
+                webhook_id,
+                user_agent_id,
+            )
             try:
                 webhook.async_register(
                     self.hass,
@@ -360,13 +366,18 @@ class AbstractConfig(ABC):
     @callback
     def async_disable_local_sdk(self) -> None:
         """Disable the local SDK."""
+        _LOGGER.debug("async_disable_local_sdk")
         if not self._local_sdk_active:
             return
 
         for agent_user_id in self._store.agent_user_ids:
-            webhook.async_unregister(
-                self.hass, self.get_local_webhook_id(agent_user_id)
+            webhook_id = self.get_local_webhook_id(agent_user_id)
+            _LOGGER.debug(
+                "Unregister webhook handler %s for agent user id %s",
+                webhook_id,
+                agent_user_id,
             )
+            webhook.async_unregister(self.hass, webhook_id)
 
         self._local_sdk_active = False
 

homeassistant/components/homeassistant/exposed_entities.py

@@ -12,7 +12,7 @@ from homeassistant.components import websocket_api
 from homeassistant.components.binary_sensor import BinarySensorDeviceClass
 from homeassistant.components.sensor import SensorDeviceClass
 from homeassistant.const import CLOUD_NEVER_EXPOSED_ENTITIES
-from homeassistant.core import HomeAssistant, callback, split_entity_id
+from homeassistant.core import CALLBACK_TYPE, HomeAssistant, callback, split_entity_id
 from homeassistant.exceptions import HomeAssistantError
 from homeassistant.helpers import entity_registry as er
 from homeassistant.helpers.entity import get_device_class
@@ -129,10 +129,17 @@ class ExposedEntities:
     @callback
     def async_listen_entity_updates(
         self, assistant: str, listener: Callable[[], None]
-    ) -> None:
+    ) -> CALLBACK_TYPE:
         """Listen for updates to entity expose settings."""
+
+        def unsubscribe() -> None:
+            """Stop listening to entity updates."""
+            self._listeners[assistant].remove(listener)
+
         self._listeners.setdefault(assistant, []).append(listener)
 
+        return unsubscribe
+
     @callback
     def async_set_assistant_option(
         self, assistant: str, entity_id: str, key: str, value: Any
@@ -484,10 +491,10 @@ def ws_expose_new_entities_set(
 @callback
 def async_listen_entity_updates(
     hass: HomeAssistant, assistant: str, listener: Callable[[], None]
-) -> None:
+) -> CALLBACK_TYPE:
     """Listen for updates to entity expose settings."""
     exposed_entities: ExposedEntities = hass.data[DATA_EXPOSED_ENTITIES]
-    exposed_entities.async_listen_entity_updates(assistant, listener)
+    return exposed_entities.async_listen_entity_updates(assistant, listener)
 
 
 @callback

tests/components/cloud/conftest.py

@@ -115,6 +115,13 @@ async def cloud_fixture() -> AsyncGenerator[MagicMock, None]:
         type(mock_cloud).is_connected = is_connected
         type(mock_cloud.iot).connected = is_connected
 
+        def mock_username() -> bool:
+            """Return the subscription username."""
+            return "abcdefghjkl"
+
+        username = PropertyMock(side_effect=mock_username)
+        type(mock_cloud).username = username
+
         # Properties that we mock as attributes.
         mock_cloud.expiration_date = utcnow()
         mock_cloud.subscription_expired = False

tests/components/cloud/test_client.py

@@ -428,3 +428,47 @@ async def test_async_create_repair_issue_unknown(
         )
     issue = issue_registry.async_get_issue(domain=DOMAIN, issue_id=identifier)
     assert issue is None
+
+
+async def test_disconnected(hass: HomeAssistant) -> None:
+    """Test cleanup when disconnected from the cloud."""
+    prefs = MagicMock(
+        alexa_enabled=False,
+        google_enabled=True,
+        async_set_username=AsyncMock(return_value=None),
+    )
+    client = CloudClient(hass, prefs, None, {}, {})
+    client.cloud = MagicMock(is_logged_in=True, username="mock-username")
+    client._google_config = Mock()
+    client._google_config.async_disable_local_sdk.assert_not_called()
+
+    await client.cloud_disconnected()
+    client._google_config.async_disable_local_sdk.assert_called_once_with()
+
+
+async def test_logged_out(
+    hass: HomeAssistant,
+    cloud: MagicMock,
+) -> None:
+    """Test cleanup when logged out from the cloud."""
+
+    assert await async_setup_component(hass, "cloud", {"cloud": {}})
+    await hass.async_block_till_done()
+    await cloud.login("test-user", "test-pass")
+
+    alexa_config_mock = Mock(async_enable_proactive_mode=AsyncMock())
+    google_config_mock = Mock(async_sync_entities=AsyncMock())
+    cloud.client._alexa_config = alexa_config_mock
+    cloud.client._google_config = google_config_mock
+
+    await cloud.client.cloud_connected()
+    await hass.async_block_till_done()
+
+    # Simulate logged out
+    await cloud.logout()
+    await hass.async_block_till_done()
+
+    # Alexa is not cleaned up, Google is
+    assert cloud.client._alexa_config is alexa_config_mock
+    assert cloud.client._google_config is None
+    google_config_mock.async_deinitialize.assert_called_once_with()

tests/components/cloud/test_google_config.py

@@ -441,8 +441,10 @@ def test_enabled_requires_valid_sub(
     assert not config.enabled
 
 
-async def test_setup_integration(hass: HomeAssistant, mock_conf, cloud_prefs) -> None:
-    """Test that we set up the integration if used."""
+async def test_setup_google_assistant(
+    hass: HomeAssistant, mock_conf, cloud_prefs
+) -> None:
+    """Test that we set up the google_assistant integration if enabled in cloud."""
     assert await async_setup_component(hass, "homeassistant", {})
     mock_conf._cloud.subscription_expired = False
 
@@ -473,8 +475,10 @@ async def test_google_handle_logout(
         "homeassistant.components.google_assistant.report_state.async_enable_report_state",
     ) as mock_enable:
         gconf.async_enable_report_state()
+        await hass.async_block_till_done()
 
     assert len(mock_enable.mock_calls) == 1
+    assert len(gconf._on_deinitialize) == 6
 
     # This will trigger a prefs update when we logout.
     await cloud_prefs.get_cloud_user()
@@ -484,8 +488,13 @@ async def test_google_handle_logout(
         "async_check_token",
         side_effect=AssertionError("Should not be called"),
     ):
+        # Fake logging out; CloudClient.logout_cleanups sets username to None
+        # and deinitializes the Google config.
         await cloud_prefs.async_set_username(None)
+        gconf.async_deinitialize()
         await hass.async_block_till_done()
+        # Check listeners are removed:
+        assert not gconf._on_deinitialize
 
     assert len(mock_enable.return_value.mock_calls) == 1
 

tests/components/cloud/test_init.py

@@ -103,8 +103,8 @@ async def test_remote_services(
     assert mock_disconnect.called is False
 
 
-async def test_startup_shutdown_events(hass: HomeAssistant, mock_cloud_fixture) -> None:
-    """Test if the cloud will start on startup event."""
+async def test_shutdown_event(hass: HomeAssistant, mock_cloud_fixture) -> None:
+    """Test if the cloud will stop on shutdown event."""
     with patch("hass_nabucasa.Cloud.stop") as mock_stop:
         hass.bus.async_fire(EVENT_HOMEASSISTANT_STOP)
         await hass.async_block_till_done()