diff --git a/custom_components/localthings/__init__.py b/custom_components/localthings/__init__.py index 741a19f..6983be2 100644 --- a/custom_components/localthings/__init__.py +++ b/custom_components/localthings/__init__.py @@ -5,7 +5,8 @@ from __future__ import annotations import logging from homeassistant.config_entries import ConfigEntry -from homeassistant.core import HomeAssistant +from homeassistant.const import EVENT_HOMEASSISTANT_STOP +from homeassistant.core import Event, HomeAssistant from homeassistant.exceptions import ConfigEntryNotReady from homeassistant.helpers import device_registry as dr @@ -21,8 +22,38 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: try: await coordinator.async_config_entry_first_refresh() except Exception as err: + # `_poll_once` deliberately leaves the session up on a `TimeoutError` + # (the transfer may just be slow -- see its docstring), so a refresh + # that fails that way ends here with a live, bound UDP socket that + # nothing would ever close. HA retries setup on its own backoff with + # a *new* coordinator, and each device's source port is fixed by + # design (`_local_source_port`), so an abandoned socket squats the + # exact port the next attempt binds -- SO_REUSEADDR lets that bind + # succeed, leaving two sockets racing for the device's datagrams. + await coordinator.async_close() raise ConfigEntryNotReady(f"Cannot connect to device: {err}") from err hass.data[DOMAIN][entry.entry_id] = coordinator + + # Send the DTLS close_notify on Core shutdown, not just on unload (issue + # #254). `async_close` otherwise only runs via `async_unload_entry`, and + # HA does not unload entries on a plain Core restart -- so a restart left + # the previous run's association orphaned on the appliance, which is what + # makes the *next* run's handshake time out. Complements the fixed source + # port, which covers the unclean-exit case this cannot (see + # `_local_source_port`). + # + # A coroutine listener, not one that spawns its own task: the event bus + # runs it as a hass-tracked job, so the close is awaited by the + # `async_block_till_done()` inside `hass.async_stop`. A detached task + # would likely be cancelled mid-shutdown -- the exact no-close_notify + # case this exists to prevent. + async def _async_close_on_stop(_event: Event) -> None: + await coordinator.async_close() + + entry.async_on_unload( + hass.bus.async_listen_once(EVENT_HOMEASSISTANT_STOP, _async_close_on_stop) + ) + await hass.config_entries.async_forward_entry_setups(entry, PLATFORMS) return True diff --git a/custom_components/localthings/coordinator.py b/custom_components/localthings/coordinator.py index fa38f30..b1c4930 100644 --- a/custom_components/localthings/coordinator.py +++ b/custom_components/localthings/coordinator.py @@ -826,7 +826,21 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): normal on a flaky device; a run of them is a real problem. Any other exception (a `ConnectionError`, an explicitly closed session) is unambiguous and always reconnects immediately. + + Never defers before the first successful discovery (issue #254). + Deferring is a *mid-session* judgement call — "keep the entities we + already have and try again next cycle" — which is only coherent once + there are entities to keep. Pre-discovery the same exception type + means something else entirely: `_poll_once` calls `_connect_session`, + so `connect()`'s own handshake timeout surfaces here as a + `TimeoutError` too, and that is a dead connection, not a slow + transfer. Deferring it returned an empty dict instead of raising, + which `DataUpdateCoordinator` counts as a successful first refresh — + and since platforms enumerate `bound` exactly once, the entry loaded + with zero entities and stayed that way until a manual reload. """ + if not self._discovered: + return False if not isinstance(e, TimeoutError): return False if self._observe.mode == MODE_OBSERVE and self._observe.recently_notified(): @@ -893,7 +907,15 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): except Exception as e2: self._log.error("poll failed after reconnect: %s", e2) snapshot = self._cache.snapshot() - if snapshot: + # `self._discovered` is the same precondition + # `_defer_reconnect_for` applies (issue #254): returning + # degraded-but-successful data is only meaningful once + # there are bound entities to carry it. Pre-discovery the + # cache happens to always be empty -- every apply() site + # is gated on post-discovery state -- so this arm is + # unreachable then, but that is a non-local accident + # across four call sites, not something to rely on. + if self._discovered and snapshot: self._log.debug("Full error:", exc_info=e2) return flatten(self.bound, snapshot) raise UpdateFailed(f"poll failed after reconnect: {e2}") from e2 diff --git a/tests/localthings/test_coordinator.py b/tests/localthings/test_coordinator.py index 36deccd..47efdb2 100644 --- a/tests/localthings/test_coordinator.py +++ b/tests/localthings/test_coordinator.py @@ -7,6 +7,8 @@ from unittest.mock import AsyncMock, patch import cbor2 import pytest +from homeassistant.config_entries import ConfigEntryState +from homeassistant.const import EVENT_HOMEASSISTANT_STOP from homeassistant.core import HomeAssistant from homeassistant.exceptions import ServiceValidationError from homeassistant.helpers import issue_registry as ir @@ -1281,3 +1283,116 @@ def test_local_source_port_non_ipv4_falls_back_to_hash() -> None: port = _local_source_port("some-hostname") assert port == _local_source_port("some-hostname") assert DTLS_LOCAL_PORT_BASE <= port <= DTLS_LOCAL_PORT_BASE + 0xFF + + +# ---------------------------------------------------------------------- +# Issue #254: a pre-discovery poll failure must never look like a +# successful first refresh. See `_defer_reconnect_for` for the mechanism. +# ---------------------------------------------------------------------- + +_HANDSHAKE_TIMEOUT = TimeoutError('DTLS handshake timed out') + + +async def test_first_refresh_timeout_recovers_via_reconnect( + hass: HomeAssistant, mock_entry, fridge_resources +) -> None: + """A pre-discovery handshake timeout reconnects instead of deferring.""" + with ( + patch('custom_components.localthings.coordinator.LocalThingsCoordinator._connect_session'), + patch( + 'custom_components.localthings.coordinator.LocalThingsCoordinator._poll_once', + side_effect=[_HANDSHAKE_TIMEOUT, fridge_resources], + ) as mock_poll, + patch('custom_components.localthings.coordinator.LocalThingsCoordinator._close_session'), + ): + await hass.config_entries.async_setup(mock_entry.entry_id) + await hass.async_block_till_done() + + # The reconnect inside the first refresh is what makes this recoverable + # without HA having to fail and re-run setup at all. + assert mock_poll.call_count == 2 + assert mock_entry.state is ConfigEntryState.LOADED + coordinator: LocalThingsCoordinator = hass.data[DOMAIN][mock_entry.entry_id] + assert coordinator.bound, "discovery must have run before platforms are forwarded" + # The regression is entity-visible, not just coordinator state: platforms + # enumerate `bound` once, so an empty `bound` at forward time leaves the + # device with no entities until a manual reload. + assert hass.states.async_all(), "device came up with zero entities" + # The reconnect handled this failure, so it must not also be charged + # against the mid-session timeout budget. + assert coordinator._consecutive_poll_timeouts == 0 + + +async def test_first_refresh_persistent_timeout_fails_setup( + hass: HomeAssistant, mock_entry +) -> None: + """When the reconnect times out too, the first refresh must fail so HA + retries on its backoff -- not load an entity-less entry. The session it + left open is closed on the way out (`_poll_once` keeps it up on a + `TimeoutError`, and the source port is fixed per device).""" + with ( + patch('custom_components.localthings.coordinator.LocalThingsCoordinator._connect_session'), + patch( + 'custom_components.localthings.coordinator.LocalThingsCoordinator._poll_once', + side_effect=_HANDSHAKE_TIMEOUT, + ), + patch('custom_components.localthings.coordinator.LocalThingsCoordinator._close_session'), + # Asserted on `async_close` rather than `_close_session`: the + # reconnect path calls the latter on its own, so it is already true + # whether or not setup cleans up after itself. + patch.object( + LocalThingsCoordinator, 'async_close', autospec=True, + ) as mock_async_close, + ): + await hass.config_entries.async_setup(mock_entry.entry_id) + await hass.async_block_till_done() + + assert mock_entry.state is ConfigEntryState.SETUP_RETRY + assert not hass.states.async_all() + mock_async_close.assert_awaited_once() + + +async def test_post_discovery_timeout_is_still_deferred( + hass: HomeAssistant, mock_entry, mock_coordinator_session +) -> None: + """The gate is scoped to the pre-discovery case only: once discovery has + run, a lone poll timeout must still be absorbed without a reconnect (the + slow-blockwise-transfer behavior `_POLL_TIMEOUT_LIMIT` exists for).""" + await hass.config_entries.async_setup(mock_entry.entry_id) + await hass.async_block_till_done() + + coordinator: LocalThingsCoordinator = hass.data[DOMAIN][mock_entry.entry_id] + assert coordinator._discovered + + with ( + patch( + 'custom_components.localthings.coordinator.LocalThingsCoordinator._poll_once', + side_effect=TimeoutError('GET /device/0 block 11 timeout'), + ), + patch( + 'custom_components.localthings.coordinator.LocalThingsCoordinator._close_session', + ) as mock_close, + ): + await coordinator.async_request_refresh() + await hass.async_block_till_done() + + mock_close.assert_not_called() + assert coordinator._consecutive_poll_timeouts == 1 + assert coordinator.last_update_success + + +async def test_close_notify_sent_on_homeassistant_stop( + hass: HomeAssistant, mock_entry, mock_coordinator_session +) -> None: + """A plain Core restart must close the DTLS session: HA does not unload + entries on restart, so `async_unload_entry` never runs and the + association would be left orphaned on the appliance.""" + await hass.config_entries.async_setup(mock_entry.entry_id) + await hass.async_block_till_done() + + coordinator: LocalThingsCoordinator = hass.data[DOMAIN][mock_entry.entry_id] + with patch.object(coordinator, 'async_close', new=AsyncMock()) as mock_close: + hass.bus.async_fire(EVENT_HOMEASSISTANT_STOP) + await hass.async_block_till_done() + + mock_close.assert_awaited_once()