Fix entity removal leaks in isy994 (#183862)

This commit is contained in:
Erik Montnemery
2026-10-05 17:24:57 +02:00
committed by GitHub
parent ab32a3331b
commit 9fe2540efb
10 changed files with 482 additions and 15 deletions
@@ -150,7 +150,6 @@ async def async_setup_entry(
entity = ISYBinarySensorHeartbeat(
node, parent_entity, device_info=device_info
)
parent_entity.add_heartbeat_device(entity)
entities.append(entity)
continue
if (
@@ -292,11 +291,17 @@ class ISYInsteonBinarySensorEntity(ISYBinarySensorEntity):
"""Subscribe to the node and subnode event emitters."""
await super().async_added_to_hass()
self._node.control_events.subscribe(self._async_positive_node_control_handler)
self.async_on_remove(
self._node.control_events.subscribe(
self._async_positive_node_control_handler
).unsubscribe
)
if self._negative_node is not None:
self._negative_node.control_events.subscribe(
self._async_negative_node_control_handler
self.async_on_remove(
self._negative_node.control_events.subscribe(
self._async_negative_node_control_handler
).unsubscribe
)
def add_heartbeat_device(self, entity: ISYBinarySensorHeartbeat | None) -> None:
@@ -421,10 +426,7 @@ class ISYBinarySensorHeartbeat(ISYNodeEntity, BinarySensorEntity, RestoreEntity)
def __init__(
self,
node: Node,
parent_device: ISYInsteonBinarySensorEntity
| ISYBinarySensorEntity
| ISYBinarySensorHeartbeat
| ISYBinarySensorProgramEntity,
parent_device: ISYInsteonBinarySensorEntity,
device_info: DeviceInfo | None = None,
) -> None:
"""Initialize the ISY binary sensor device.
@@ -448,10 +450,17 @@ class ISYBinarySensorHeartbeat(ISYNodeEntity, BinarySensorEntity, RestoreEntity)
"""Subscribe to the node and subnode event emitters."""
await super().async_added_to_hass()
self._node.control_events.subscribe(self._heartbeat_node_control_handler)
self.async_on_remove(
self._node.control_events.subscribe(
self._heartbeat_node_control_handler
).unsubscribe
)
self._parent_device.add_heartbeat_device(self)
self.async_on_remove(lambda: self._parent_device.add_heartbeat_device(None))
# Start the timer on boot-up, so we can change from UNKNOWN to OFF
self._restart_timer()
self.async_on_remove(self._cancel_timer)
if (last_state := await self.async_get_last_state()) is not None:
# Only restore the state if it was previously ON (Low Battery)
@@ -479,12 +488,16 @@ class ISYBinarySensorHeartbeat(ISYNodeEntity, BinarySensorEntity, RestoreEntity)
self._restart_timer()
self.async_write_ha_state()
def _restart_timer(self) -> None:
"""Restart the 25 hour timer."""
def _cancel_timer(self) -> None:
"""Cancel the 25 hour timer."""
if self._heartbeat_timer is not None:
self._heartbeat_timer()
self._heartbeat_timer = None
def _restart_timer(self) -> None:
"""Restart the 25 hour timer."""
self._cancel_timer()
@callback
def timer_elapsed(now: datetime) -> None:
"""Heartbeat missed; set state to ON to indicate dead battery."""
@@ -127,6 +127,7 @@ class ISYNodeButtonEntity(ButtonEntity):
},
key=self.unique_id,
)
self.async_on_remove(self._availability_handler.unsubscribe)
@callback
def async_on_update(self, event: NodeProperty, key: str) -> None:
@@ -55,11 +55,13 @@ class ISYEntity(Entity):
async def async_added_to_hass(self) -> None:
"""Subscribe to the node change events."""
self._change_handler = self._node.status_events.subscribe(self.async_on_update)
self.async_on_remove(self._change_handler.unsubscribe)
if hasattr(self._node, "control_events"):
self._control_handler = self._node.control_events.subscribe(
self.async_on_control
)
self.async_on_remove(self._control_handler.unsubscribe)
@callback
def async_on_update(self, event: NodeProperty) -> None:
@@ -252,6 +254,7 @@ class ISYAuxControlEntity(Entity):
event_filter={ATTR_CONTROL: self._control},
key=self.unique_id,
)
self.async_on_remove(self._change_handler.unsubscribe)
self._availability_handler = self._node.isy.nodes.status_events.subscribe(
self.async_on_update,
event_filter={
@@ -260,6 +263,7 @@ class ISYAuxControlEntity(Entity):
},
key=self.unique_id,
)
self.async_on_remove(self._availability_handler.unsubscribe)
@callback
def async_on_update(self, event: NodeProperty | NodeChangedEvent, key: str) -> None:
@@ -207,6 +207,7 @@ class ISYVariableNumberEntity(NumberEntity):
async def async_added_to_hass(self) -> None:
"""Subscribe to the node change events."""
self._change_handler = self._node.status_events.subscribe(self.async_on_update)
self.async_on_remove(self._change_handler.unsubscribe)
@callback
def async_on_update(self, event: NodeProperty) -> None:
@@ -277,6 +278,7 @@ class ISYBacklightNumberEntity(ISYAuxControlEntity, RestoreNumber):
},
key=self.unique_id,
)
self.async_on_remove(self._memory_change_handler.unsubscribe)
@callback
def async_on_memory_write(self, event: NodeChangedEvent, key: str) -> None:
@@ -189,6 +189,7 @@ class ISYBacklightSelectEntity(ISYAuxControlEntity, SelectEntity, RestoreEntity)
},
key=self.unique_id,
)
self.async_on_remove(self._memory_change_handler.unsubscribe)
@callback
def async_on_memory_write(self, event: NodeChangedEvent, key: str) -> None:
@@ -377,6 +377,7 @@ class ISYAuxSensorEntity(ISYSensorEntity):
self._change_handler = self._node.control_events.subscribe(
self.async_on_update, event_filter={ATTR_CONTROL: self._control}
)
self.async_on_remove(self._change_handler.unsubscribe)
self._availability_handler = self._node.isy.nodes.status_events.subscribe(
self.async_on_update,
event_filter={
@@ -384,6 +385,7 @@ class ISYAuxSensorEntity(ISYSensorEntity):
ATTR_ACTION: NC_NODE_ENABLED,
},
)
self.async_on_remove(self._availability_handler.unsubscribe)
@callback
@override
@@ -174,6 +174,7 @@ class ISYEnableSwitchEntity(ISYAuxControlEntity, SwitchEntity):
},
key=self.unique_id,
)
self.async_on_remove(self._change_handler.unsubscribe)
@callback
@override
+13
View File
@@ -1 +1,14 @@
"""Tests for the Universal Devices ISY/IoX integration."""
from pyisy.helpers import EventEmitter, EventListener
from homeassistant.helpers.entity import Entity
def entity_listeners(emitter: EventEmitter, entity: Entity) -> list[EventListener]:
"""Return the listeners of an emitter whose callback is bound to the entity."""
return [
listener
for listener in emitter._subscribers
if getattr(listener.callback, "__self__", None) is entity
]
@@ -0,0 +1,192 @@
"""Test the ISY994 binary sensor platform."""
from collections.abc import Callable
from datetime import timedelta
from typing import Any
from unittest.mock import MagicMock, patch
from freezegun.api import FrozenDateTimeFactory
from pyisy.constants import CMD_ON
from pyisy.helpers import EventEmitter, NodeProperty
import pytest
from homeassistant.components.binary_sensor import DOMAIN as BINARY_SENSOR_DOMAIN
from homeassistant.components.isy994.binary_sensor import (
ISYBinarySensorHeartbeat,
ISYInsteonBinarySensorEntity,
)
from homeassistant.const import STATE_OFF, STATE_ON, Platform
from homeassistant.core import HomeAssistant
from homeassistant.helpers import entity_registry as er
from homeassistant.helpers.entity_component import DATA_INSTANCES
from . import entity_listeners
from tests.common import MockConfigEntry, async_fire_time_changed
LEAK_SENSOR_ENTITY_ID = "binary_sensor.leak_sensor"
HEARTBEAT_ENTITY_ID = "binary_sensor.leak_sensor_heartbeat"
@pytest.fixture(autouse=True)
def mock_binary_sensor_platform():
"""Mock the platforms to only include binary_sensor."""
with patch("homeassistant.components.isy994.PLATFORMS", [Platform.BINARY_SENSOR]):
yield
@pytest.fixture
def leak_sensor_nodes(
mock_isy: MagicMock, mock_node: Callable[..., Any]
) -> tuple[MagicMock, MagicMock, MagicMock]:
"""Return the parent, negative and heartbeat nodes of an Insteon leak sensor."""
parent = mock_node(mock_isy, "1A 2B 3C 1", "Leak Sensor", "BinaryAlarm", "16.8.1.0")
children = [
mock_node(mock_isy, address, name, "BinaryAlarm", "16.8.1.0")
for address, name in (
("1A 2B 3C 2", "Leak Sensor Dry"),
("1A 2B 3C 4", "Leak Sensor Heartbeat"),
)
]
for node in (parent, *children):
node.status_events = EventEmitter()
node.control_events = EventEmitter()
for child in children:
child.parent_node = parent
child.primary_node = parent.address
mock_isy.nodes.status_events = EventEmitter()
mock_isy.nodes.__iter__.return_value = [
(node.name, node) for node in (parent, *children)
]
return parent, children[0], children[1]
async def _async_setup_leak_sensor(
hass: HomeAssistant, config_entry: MockConfigEntry
) -> tuple[ISYInsteonBinarySensorEntity, ISYBinarySensorHeartbeat]:
"""Set up the integration and return the leak sensor and heartbeat entities."""
config_entry.add_to_hass(hass)
assert await hass.config_entries.async_setup(config_entry.entry_id)
await hass.async_block_till_done()
component = hass.data[DATA_INSTANCES][BINARY_SENSOR_DOMAIN]
parent = component.get_entity(LEAK_SENSOR_ENTITY_ID)
heartbeat = component.get_entity(HEARTBEAT_ENTITY_ID)
assert isinstance(parent, ISYInsteonBinarySensorEntity)
assert isinstance(heartbeat, ISYBinarySensorHeartbeat)
return parent, heartbeat
async def test_insteon_listeners_removed_with_entity(
hass: HomeAssistant,
entity_registry: er.EntityRegistry,
mock_config_entry: MockConfigEntry,
leak_sensor_nodes: tuple[MagicMock, MagicMock, MagicMock],
) -> None:
"""Test removing an Insteon sensor unsubscribes its positive and negative nodes."""
parent_node, negative_node, _ = leak_sensor_nodes
parent, _ = await _async_setup_leak_sensor(hass, mock_config_entry)
emitters = (
parent_node.status_events,
parent_node.control_events,
negative_node.control_events,
)
# Control events feed both the base entity and the positive node handler.
assert [len(entity_listeners(emitter, parent)) for emitter in emitters] == [
1,
2,
1,
]
entity_registry.async_remove(LEAK_SENSOR_ENTITY_ID)
await hass.async_block_till_done()
assert hass.states.get(LEAK_SENSOR_ENTITY_ID) is None
assert [emitter._subscribers for emitter in emitters] == [[], [], []]
async def test_heartbeat_attached_to_parent(
hass: HomeAssistant,
freezer: FrozenDateTimeFactory,
mock_config_entry: MockConfigEntry,
leak_sensor_nodes: tuple[MagicMock, MagicMock, MagicMock],
) -> None:
"""Test parent control events reset the heartbeat after it timed out."""
parent_node, _, _ = leak_sensor_nodes
parent, heartbeat = await _async_setup_leak_sensor(hass, mock_config_entry)
assert parent._heartbeat_device is heartbeat
freezer.tick(timedelta(hours=26))
async_fire_time_changed(hass)
await hass.async_block_till_done()
assert hass.states.get(HEARTBEAT_ENTITY_ID).state == STATE_ON
parent_node.control_events.notify(NodeProperty(CMD_ON))
await hass.async_block_till_done()
assert hass.states.get(HEARTBEAT_ENTITY_ID).state == STATE_OFF
async def test_heartbeat_removed_with_entity(
hass: HomeAssistant,
entity_registry: er.EntityRegistry,
freezer: FrozenDateTimeFactory,
mock_config_entry: MockConfigEntry,
leak_sensor_nodes: tuple[MagicMock, MagicMock, MagicMock],
) -> None:
"""Test removing the heartbeat unsubscribes, detaches and stops its timer."""
parent_node, _, heartbeat_node = leak_sensor_nodes
parent, heartbeat = await _async_setup_leak_sensor(hass, mock_config_entry)
emitters = (heartbeat_node.status_events, heartbeat_node.control_events)
# Control events feed both the base entity and the heartbeat handler.
assert [len(entity_listeners(emitter, heartbeat)) for emitter in emitters] == [
1,
2,
]
assert heartbeat._heartbeat_timer is not None
computed_state = heartbeat._computed_state
entity_registry.async_remove(HEARTBEAT_ENTITY_ID)
await hass.async_block_till_done()
assert hass.states.get(HEARTBEAT_ENTITY_ID) is None
assert [emitter._subscribers for emitter in emitters] == [[], []]
assert parent._heartbeat_device is None
assert heartbeat._heartbeat_timer is None
freezer.tick(timedelta(hours=26))
async_fire_time_changed(hass)
await hass.async_block_till_done()
assert heartbeat._computed_state is computed_state
# Parent activity must not re-arm the removed heartbeat's timer.
parent_node.control_events.notify(NodeProperty(CMD_ON))
await hass.async_block_till_done()
assert heartbeat._heartbeat_timer is None
# The parent still handled the event; DON on a leak sensor means dry.
assert hass.states.get(LEAK_SENSOR_ENTITY_ID).state == STATE_OFF
async def test_heartbeat_entity_id_change_keeps_attachment(
hass: HomeAssistant,
entity_registry: er.EntityRegistry,
mock_config_entry: MockConfigEntry,
leak_sensor_nodes: tuple[MagicMock, MagicMock, MagicMock],
) -> None:
"""Test renaming the heartbeat entity keeps it attached and subscribed once."""
_, _, heartbeat_node = leak_sensor_nodes
parent, heartbeat = await _async_setup_leak_sensor(hass, mock_config_entry)
new_entity_id = "binary_sensor.renamed_heartbeat"
entity_registry.async_update_entity(
HEARTBEAT_ENTITY_ID, new_entity_id=new_entity_id
)
await hass.async_block_till_done()
assert hass.states.get(HEARTBEAT_ENTITY_ID) is None
assert hass.states.get(new_entity_id) is not None
assert heartbeat.entity_id == new_entity_id
assert parent._heartbeat_device is heartbeat
assert [
listener.callback
for listener in heartbeat_node.control_events._subscribers
if listener.callback == heartbeat._heartbeat_node_control_handler
] == [heartbeat._heartbeat_node_control_handler]
+242 -4
View File
@@ -4,15 +4,34 @@ from collections.abc import Callable
from typing import Any
from unittest.mock import MagicMock, patch
from pyisy.constants import (
CMD_BACKLIGHT,
CMD_ON,
PROP_ON_LEVEL,
PROP_RAMP_RATE,
TAG_ENABLED,
)
from pyisy.helpers import EventEmitter, NodeProperty
from pyisy.variables import Variable
import pytest
from homeassistant.components.isy994.const import DOMAIN
from homeassistant.components.isy994.const import DOMAIN, EVENT_ISY994_CONTROL
from homeassistant.config_entries import ConfigEntryState
from homeassistant.const import CONF_HOST, CONF_PASSWORD, CONF_USERNAME, CONF_VERIFY_SSL
from homeassistant.const import (
CONF_HOST,
CONF_PASSWORD,
CONF_USERNAME,
CONF_VERIFY_SSL,
Platform,
)
from homeassistant.core import HomeAssistant
from homeassistant.helpers import device_registry as dr
from homeassistant.helpers import device_registry as dr, entity_registry as er
from homeassistant.helpers.entity_component import DATA_INSTANCES
from tests.common import MockConfigEntry
from . import entity_listeners
from .conftest import MOCK_UUID as MOCK_ISY_UUID
from tests.common import MockConfigEntry, async_capture_events
MOCK_UUID = "ce:fb:72:31:b7:b9"
@@ -101,3 +120,222 @@ async def test_node_device_linked_to_isy_device(
assert isy_device is not None
assert node_device is not None
assert node_device.via_device_id == isy_device.id
@pytest.mark.parametrize(
("platform", "node_def_id"),
[
pytest.param(Platform.SWITCH, "RelayLampSwitch_ADV", id="switch"),
pytest.param(Platform.SENSOR, "GenericSensor", id="sensor"),
],
)
async def test_node_listeners_removed_with_entity(
hass: HomeAssistant,
entity_registry: er.EntityRegistry,
mock_config_entry: MockConfigEntry,
mock_isy: MagicMock,
mock_node: Callable[..., Any],
platform: Platform,
node_def_id: str,
) -> None:
"""Test removing an entity unsubscribes it from its node's event emitters."""
mock_config_entry.add_to_hass(hass)
node = mock_node(mock_isy, "22 22 22 1", "Test Node", node_def_id)
node.status_events = EventEmitter()
node.control_events = EventEmitter()
mock_isy.nodes.__iter__.return_value = [("Test Node", node)]
events = async_capture_events(hass, EVENT_ISY994_CONTROL)
with patch("homeassistant.components.isy994.PLATFORMS", [platform]):
assert await hass.config_entries.async_setup(mock_config_entry.entry_id)
await hass.async_block_till_done()
entity_id = f"{platform}.test_node"
assert hass.states.get(entity_id) is not None
assert node.status_events._subscribers
assert node.control_events._subscribers
node.control_events.notify(NodeProperty(node.address, CMD_ON))
await hass.async_block_till_done()
assert len(events) == 1
entity_registry.async_remove(entity_id)
await hass.async_block_till_done()
assert hass.states.get(entity_id) is None
assert not node.status_events._subscribers
assert not node.control_events._subscribers
node.control_events.notify(NodeProperty(node.address, CMD_ON))
await hass.async_block_till_done()
assert len(events) == 1
@pytest.mark.parametrize(
("platform", "node_attrs", "unique_id_suffix", "expected_listeners"),
[
pytest.param(
Platform.NUMBER,
{"aux_properties": {PROP_ON_LEVEL: NodeProperty(PROP_ON_LEVEL)}},
f"_{PROP_ON_LEVEL}",
{"status": 0, "control": 1, "isy": 1},
id="aux_control_number",
),
pytest.param(
Platform.SELECT,
{"aux_properties": {PROP_RAMP_RATE: NodeProperty(PROP_RAMP_RATE)}},
f"_{PROP_RAMP_RATE}",
{"status": 0, "control": 1, "isy": 1},
id="ramp_rate_select",
),
pytest.param(
Platform.SWITCH,
{},
f"_{TAG_ENABLED}",
{"status": 0, "control": 0, "isy": 1},
id="enable_switch",
),
pytest.param(
Platform.NUMBER,
{"is_backlight_supported": True},
f"_{CMD_BACKLIGHT}",
{"status": 0, "control": 1, "isy": 2},
id="backlight_number",
),
pytest.param(
Platform.SELECT,
{"is_backlight_supported": True, "node_def_id": "KeypadDimmer"},
f"_{CMD_BACKLIGHT}",
{"status": 0, "control": 1, "isy": 2},
id="backlight_select",
),
pytest.param(
Platform.SENSOR,
{"aux_properties": {"TPW": NodeProperty("TPW")}},
"_TPW",
{"status": 0, "control": 1, "isy": 1},
id="aux_sensor",
),
pytest.param(
Platform.BUTTON,
{},
"_query",
{"status": 0, "control": 0, "isy": 1},
id="query_button",
),
pytest.param(
Platform.BUTTON,
{},
"_beep",
{"status": 0, "control": 0, "isy": 1},
id="beep_button",
),
],
)
async def test_overridden_listeners_removed_with_entity(
hass: HomeAssistant,
entity_registry: er.EntityRegistry,
mock_config_entry: MockConfigEntry,
mock_isy: MagicMock,
mock_node: Callable[..., Any],
platform: Platform,
node_attrs: dict[str, Any],
unique_id_suffix: str,
expected_listeners: dict[str, int],
) -> None:
"""Test entities overriding the node subscriptions unsubscribe on removal."""
mock_config_entry.add_to_hass(hass)
node = mock_node(mock_isy, "22 22 22 1", "Test Node", "DimmerLampSwitch")
for attr, value in node_attrs.items():
setattr(node, attr, value)
node.status_events = EventEmitter()
node.control_events = EventEmitter()
mock_isy.nodes.status_events = EventEmitter()
mock_isy.nodes.__iter__.return_value = [("Test Node", node)]
emitters = {
"status": node.status_events,
"control": node.control_events,
"isy": mock_isy.nodes.status_events,
}
with patch("homeassistant.components.isy994.PLATFORMS", [platform]):
assert await hass.config_entries.async_setup(mock_config_entry.entry_id)
await hass.async_block_till_done()
entity_id = entity_registry.async_get_entity_id(
platform, DOMAIN, f"{MOCK_ISY_UUID}_{node.address}{unique_id_suffix}"
)
assert entity_id is not None
entity = hass.data[DATA_INSTANCES][platform].get_entity(entity_id)
assert entity is not None
assert {
name: len(entity_listeners(emitter, entity))
for name, emitter in emitters.items()
} == expected_listeners
# Listeners of sibling entities sharing these emitters must survive.
remaining = {
name: [
listener
for listener in emitter._subscribers
if listener not in entity_listeners(emitter, entity)
]
for name, emitter in emitters.items()
}
entity_registry.async_remove(entity_id)
await hass.async_block_till_done()
assert hass.states.get(entity_id) is None
assert {
name: emitter._subscribers for name, emitter in emitters.items()
} == remaining
@pytest.mark.parametrize(
"unique_id_suffix",
[pytest.param("", id="value"), pytest.param("_init", id="initial_value")],
)
async def test_variable_listener_removed_with_entity(
hass: HomeAssistant,
entity_registry: er.EntityRegistry,
mock_config_entry: MockConfigEntry,
mock_isy: MagicMock,
unique_id_suffix: str,
) -> None:
"""Test removing a variable number entity unsubscribes it from the variable."""
mock_config_entry.add_to_hass(hass)
variable = MagicMock(
spec=Variable,
address="1.1",
prec="0",
status=1,
init=0,
last_edited=None,
status_events=EventEmitter(),
)
variable.name = "HA.Test Variable"
mock_isy.variables.children = [(1, variable.name, 1)]
mock_isy.variables.__getitem__.return_value = {1: variable}
with patch("homeassistant.components.isy994.PLATFORMS", [Platform.NUMBER]):
assert await hass.config_entries.async_setup(mock_config_entry.entry_id)
await hass.async_block_till_done()
entity_id = entity_registry.async_get_entity_id(
Platform.NUMBER,
DOMAIN,
f"{MOCK_ISY_UUID}_{variable.address}{unique_id_suffix}",
)
assert entity_id is not None
entity = hass.data[DATA_INSTANCES][Platform.NUMBER].get_entity(entity_id)
assert entity is not None
assert len(entity_listeners(variable.status_events, entity)) == 1
# The sibling value/initial value entity also listens to the variable.
assert len(variable.status_events._subscribers) == 2
entity_registry.async_remove(entity_id)
await hass.async_block_till_done()
assert hass.states.get(entity_id) is None
assert not entity_listeners(variable.status_events, entity)
assert len(variable.status_events._subscribers) == 1