Fix entity removal leaks in homematicip_cloud (#183861)

homeassistant/components/homematicip_cloud/alarm_control_panel.py

@@ -131,6 +131,9 @@ class HomematicipAlarmControlPanelEntity(AlarmControlPanelEntity):
     async def async_added_to_hass(self) -> None:
         """Register callbacks."""
         self._home.on_update(self._async_device_changed)
+        self.async_on_remove(
+            lambda: self._home.remove_callback(self._async_device_changed)
+        )
 
     @callback
     def _async_device_changed(self, *args, **kwargs) -> None:

homeassistant/components/homematicip_cloud/entity.py

@@ -171,6 +171,12 @@ class HomematicipGenericEntity(Entity):
         self._hap.hmip_device_by_entity_id[self.entity_id] = self._device
         self._device.on_update(self._async_device_changed)
         self._device.on_remove(self._async_device_removed)
+        self.async_on_remove(
+            lambda: self._device.remove_callback(self._async_device_changed)
+        )
+        self.async_on_remove(
+            lambda: self._device.remove_callback(self._async_device_removed)
+        )
 
     @callback
     def _async_device_changed(self, *args, **kwargs) -> None:
@@ -189,24 +195,16 @@ class HomematicipGenericEntity(Entity):
     @override
     async def async_will_remove_from_hass(self) -> None:
         """Run when hmip device will be removed from hass."""
+        self._hap.hmip_device_by_entity_id.pop(self.entity_id, None)
 
         # Only go further if the device/entity should be removed from registries
         # due to a removal of the HmIP device.
-
         if self.hmip_device_removed:
-            try:
-                del self._hap.hmip_device_by_entity_id[self.entity_id]
-                self.async_remove_from_registries()
-            except KeyError as err:
-                _LOGGER.debug("Error removing HMIP device from registry: %s", err)
+            self.async_remove_from_registries()
 
     @callback
     def async_remove_from_registries(self) -> None:
         """Remove entity/device from registry."""
-        # Remove callback from device.
-        self._device.remove_callback(self._async_device_changed)
-        self._device.remove_callback(self._async_device_removed)
-
         if not self.registry_entry:
             return
 

homeassistant/components/homematicip_cloud/event.py

@@ -2,6 +2,7 @@
 
 from collections.abc import Callable
 from dataclasses import dataclass
+from functools import partial
 from typing import override
 
 from homematicip.base.channel_event import ChannelEvent
@@ -15,6 +16,10 @@ from homeassistant.components.event import (
     EventEntityDescription,
 )
 from homeassistant.core import HomeAssistant, callback
+from homeassistant.helpers.dispatcher import (
+    async_dispatcher_connect,
+    async_dispatcher_send,
+)
 from homeassistant.helpers.entity_platform import AddConfigEntryEntitiesCallback
 
 from .entity import HomematicipGenericEntity
@@ -102,6 +107,7 @@ class HomematicipChannelEvent(HomematicipGenericEntity, EventEntity):
             use_description_name=description.is_multi_channel,
         )
         self.entity_description = description
+        self._channel_event_signal: str | None = None
         if description.is_multi_channel:
             self._attr_translation_placeholders = {"channel": str(channel.index)}
 
@@ -110,8 +116,21 @@ class HomematicipChannelEvent(HomematicipGenericEntity, EventEntity):
         """Register callbacks."""
         await super().async_added_to_hass()
 
-        channel = self.get_channel_or_raise()
-        channel.add_on_channel_event_handler(self._async_handle_event)
+        # homematicip can't remove channel event handlers, so register a
+        # forwarder once and (un)subscribe the entity through the dispatcher.
+        if self._channel_event_signal is None:
+            self._channel_event_signal = (
+                f"homematicip_cloud_channel_event_{self.unique_id}"
+            )
+            channel = self.get_channel_or_raise()
+            channel.add_on_channel_event_handler(
+                partial(async_dispatcher_send, self.hass, self._channel_event_signal)
+            )
+        self.async_on_remove(
+            async_dispatcher_connect(
+                self.hass, self._channel_event_signal, self._async_handle_event
+            )
+        )
 
     @callback
     def _async_handle_event(self, *args, **kwargs) -> None:

tests/components/homematicip_cloud/test_alarm_control_panel.py

@@ -1,5 +1,6 @@
 """Tests for HomematicIP Cloud alarm control panel."""
 
+from typing import Any
 from unittest.mock import Mock
 
 from homematicip.async_home import AsyncHome
@@ -8,6 +9,7 @@ import pytest
 from homeassistant.components.alarm_control_panel import AlarmControlPanelState
 from homeassistant.core import HomeAssistant
 from homeassistant.exceptions import HomeAssistantError
+from homeassistant.helpers import entity_registry as er
 
 from .helper import HomeFactory, get_and_check_entity_basics
 
@@ -155,3 +157,37 @@ async def test_hmip_alarm_control_panel_activation_failed(
             {"entity_id": entity_id},
             blocking=True,
         )
+
+
+async def test_hmip_alarm_control_panel_removal_removes_home_callback(
+    hass: HomeAssistant,
+    entity_registry: er.EntityRegistry,
+    default_mock_hap_factory: HomeFactory,
+) -> None:
+    """Test removing the alarm panel removes its home update handler."""
+    entity_id = "alarm_control_panel.hmip_alarm_control_panel"
+    mock_hap = await default_mock_hap_factory.async_get_mock_hap(
+        test_groups=["EXTERNAL", "INTERNAL"]
+    )
+    home = mock_hap.home
+
+    def _entity_handlers() -> list[Any]:
+        return [
+            handler
+            for handler in home._on_update
+            if getattr(getattr(handler, "__self__", None), "entity_id", None)
+            == entity_id
+        ]
+
+    assert len(_entity_handlers()) == 1
+
+    entity_registry.async_remove(entity_id)
+    await hass.async_block_till_done()
+
+    assert _entity_handlers() == []
+    assert hass.states.get(entity_id) is None
+
+    await _async_manipulate_security_zones(
+        hass, home, internal_active=True, external_active=True
+    )
+    assert hass.states.get(entity_id) is None

tests/components/homematicip_cloud/test_device.py

@@ -1,5 +1,6 @@
 """Common tests for HomematicIP devices."""
 
+from collections.abc import Callable
 from typing import Any
 from unittest.mock import patch
 
@@ -364,3 +365,57 @@ async def test_hmip_unknown_device_type(
         state = hass.states.get(entry.entity_id)
         assert state is not None
         assert state.state != STATE_UNAVAILABLE
+
+
+def _entity_handlers(handlers: list[Callable[..., Any]], entity_id: str) -> list[Any]:
+    """Return the handlers bound to the entity with the given entity_id."""
+    return [
+        handler
+        for handler in handlers
+        if getattr(getattr(handler, "__self__", None), "entity_id", None) == entity_id
+    ]
+
+
+async def test_hmip_entity_removal_removes_device_callbacks(
+    hass: HomeAssistant,
+    entity_registry: er.EntityRegistry,
+    default_mock_hap_factory: HomeFactory,
+) -> None:
+    """Test removing an entity drops its device handlers and mapping."""
+    entity_id = "switch.schrank"
+    mock_hap = await default_mock_hap_factory.async_get_mock_hap(
+        test_devices=["Schrank"]
+    )
+    hmip_device = mock_hap.hmip_device_by_entity_id[entity_id]
+    assert len(_entity_handlers(hmip_device._on_update, entity_id)) == 1
+    assert len(_entity_handlers(hmip_device._on_remove, entity_id)) == 1
+
+    entity_registry.async_remove(entity_id)
+    await hass.async_block_till_done()
+
+    assert _entity_handlers(hmip_device._on_update, entity_id) == []
+    assert _entity_handlers(hmip_device._on_remove, entity_id) == []
+    assert entity_id not in mock_hap.hmip_device_by_entity_id
+
+
+async def test_hmip_entity_id_change_keeps_single_device_callbacks(
+    hass: HomeAssistant,
+    entity_registry: er.EntityRegistry,
+    default_mock_hap_factory: HomeFactory,
+) -> None:
+    """Test an entity_id change re-keys the mapping without duplicate handlers."""
+    entity_id = "switch.schrank"
+    new_entity_id = "switch.renamed"
+    mock_hap = await default_mock_hap_factory.async_get_mock_hap(
+        test_devices=["Schrank"]
+    )
+    hmip_device = mock_hap.hmip_device_by_entity_id[entity_id]
+
+    # Changing the entity_id removes and re-adds the same entity object.
+    entity_registry.async_update_entity(entity_id, new_entity_id=new_entity_id)
+    await hass.async_block_till_done()
+
+    assert entity_id not in mock_hap.hmip_device_by_entity_id
+    assert mock_hap.hmip_device_by_entity_id[new_entity_id] is hmip_device
+    assert len(_entity_handlers(hmip_device._on_update, new_entity_id)) == 1
+    assert len(_entity_handlers(hmip_device._on_remove, new_entity_id)) == 1

tests/components/homematicip_cloud/test_event.py

@@ -1,10 +1,16 @@
 """Tests for the HomematicIP Cloud event."""
 
+from unittest.mock import patch
+
 from homematicip.base.channel_event import ChannelEvent
+from homematicip.base.functionalChannels import FunctionalChannel
 import pytest
 
+from homeassistant.components.event import DOMAIN as EVENT_DOMAIN
 from homeassistant.const import STATE_UNKNOWN
 from homeassistant.core import HomeAssistant
+from homeassistant.helpers import entity_registry as er
+from homeassistant.helpers.entity_component import DATA_INSTANCES
 
 from .helper import HomeFactory, get_and_check_entity_basics
 
@@ -143,3 +149,69 @@ async def test_wrc6_button_ignores_repeating_long(
     state_after_long = hass.states.get(entity_id)
     assert state_after_long.state == state_after_short.state
     assert state_after_long.attributes["event_type"] == "short_release"
+
+
+DOORBELL_ENTITY_ID = "event.dsdpcb_klingel_doorbell"
+
+
+def _door_bell_event(channel: FunctionalChannel) -> ChannelEvent:
+    """Return a door bell channel event for the channel."""
+    return ChannelEvent(
+        channelEventType="DOOR_BELL_SENSOR_EVENT",
+        channelIndex=channel.index,
+        deviceId=channel.device.id,
+    )
+
+
+async def test_door_bell_event_after_entity_removed(
+    hass: HomeAssistant,
+    entity_registry: er.EntityRegistry,
+    default_mock_hap_factory: HomeFactory,
+) -> None:
+    """Test a removed event entity no longer handles channel events."""
+    mock_hap = await default_mock_hap_factory.async_get_mock_hap(
+        test_devices=["dsdpcb_klingel"]
+    )
+    channel = mock_hap.hmip_device_by_entity_id[DOORBELL_ENTITY_ID].functionalChannels[
+        1
+    ]
+    entity = hass.data[DATA_INSTANCES][EVENT_DOMAIN].get_entity(DOORBELL_ENTITY_ID)
+
+    entity_registry.async_remove(DOORBELL_ENTITY_ID)
+    await hass.async_block_till_done()
+
+    with patch.object(entity, "_trigger_event") as mock_trigger_event:
+        channel.fire_channel_event(_door_bell_event(channel))
+        await hass.async_block_till_done()
+
+    mock_trigger_event.assert_not_called()
+
+
+async def test_door_bell_event_after_entity_id_change(
+    hass: HomeAssistant,
+    entity_registry: er.EntityRegistry,
+    default_mock_hap_factory: HomeFactory,
+) -> None:
+    """Test a renamed event entity handles each channel event once."""
+    mock_hap = await default_mock_hap_factory.async_get_mock_hap(
+        test_devices=["dsdpcb_klingel"]
+    )
+    channel = mock_hap.hmip_device_by_entity_id[DOORBELL_ENTITY_ID].functionalChannels[
+        1
+    ]
+    entity = hass.data[DATA_INSTANCES][EVENT_DOMAIN].get_entity(DOORBELL_ENTITY_ID)
+    handler_count = len(channel._on_channel_event_handler)
+
+    # Changing the entity_id removes and re-adds the same entity object.
+    entity_registry.async_update_entity(
+        DOORBELL_ENTITY_ID, new_entity_id="event.renamed_doorbell"
+    )
+    await hass.async_block_till_done()
+
+    assert len(channel._on_channel_event_handler) == handler_count
+
+    with patch.object(entity, "_trigger_event") as mock_trigger_event:
+        channel.fire_channel_event(_door_bell_event(channel))
+        await hass.async_block_till_done()
+
+    mock_trigger_event.assert_called_once_with(event_type="ring")