diff --git a/custom_components/localthings/__init__.py b/custom_components/localthings/__init__.py index 64ec7b8..16cc396 100644 --- a/custom_components/localthings/__init__.py +++ b/custom_components/localthings/__init__.py @@ -21,39 +21,31 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: try: await coordinator.async_config_entry_first_refresh() except Exception as err: - # Close whatever the failed refresh left open before handing this - # coordinator to the garbage collector. `_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 will ever - # close. HA retries setup on its own backoff with a *new* - # coordinator, and every device's source port is fixed by design - # (`_local_source_port`), so each abandoned socket keeps squatting - # the exact port the next attempt binds -- SO_REUSEADDR lets the - # bind succeed, and then two sockets on one port race for the - # device's datagrams. Failing setup is precisely when retries are - # most likely, so leaking there is what would hurt most. + # `_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` -- a reload or a remove -- and HA does not unload - # config entries on a plain Core restart, so a restart left the previous - # run's association orphaned on the appliance. The device then holds that - # stale session for minutes, which is what makes the next run's handshake - # time out in the first place. Closing cleanly here means there is - # usually nothing to evict on the way back up. This is belt-and-braces - # with the fixed source port (RFC 6347 section 4.2.8 eviction): that - # handles the unclean-shutdown case this cannot, e.g. a power cut or a - # SIGKILL, where no close_notify is ever sent. + # 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`). # - # Registered as a coroutine listener rather than 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 just as likely be cancelled mid-shutdown, which is - # the exact no-close_notify case this exists to prevent. + # 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() diff --git a/custom_components/localthings/coordinator.py b/custom_components/localthings/coordinator.py index c887bb8..f33e617 100644 --- a/custom_components/localthings/coordinator.py +++ b/custom_components/localthings/coordinator.py @@ -793,31 +793,16 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): session) is unambiguous and always reconnects immediately. Never defers before the first successful discovery (issue #254). - Deferring is a *mid-session* judgement call: it means "keep the - entities we already have and try again next cycle," which is only - coherent once there are entities to keep. On the very first refresh - the same `TimeoutError` means something completely different — - `_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 made - `_async_update_data` return `flatten([], {})` == `{}` instead of - raising; `DataUpdateCoordinator` counts any non-raising return as - success, so `async_config_entry_first_refresh` saw a healthy first - refresh, skipped `ConfigEntryNotReady`, and setup forwarded the - platforms with `bound` still empty. Every platform enumerates - `bound` exactly once in its `async_setup_entry` and has no dynamic - add-listener, so the device came up with zero entities: a later - cycle would repopulate `bound` (`_discovered` was still False), but - nothing would ever add the entities for it, leaving every restored - entity stuck `unavailable` until a manual per-device reload. - - This is reachable on a plain Core restart because HA doesn't unload - entries on restart, so the device is often still holding the - previous run's DTLS association when we reconnect. Returning False - here puts a pre-discovery failure back on the normal path: one - reconnect attempt, and if that fails too the exception propagates as - `UpdateFailed` -> `ConfigEntryNotReady`, so HA retries on its own - backoff until the handshake goes through — no reload needed. + 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 @@ -887,7 +872,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 96f1822..4bc6913 100644 --- a/tests/localthings/test_coordinator.py +++ b/tests/localthings/test_coordinator.py @@ -1250,106 +1250,77 @@ def test_local_source_port_non_ipv4_falls_back_to_hash() -> None: # ---------------------------------------------------------------------- # Issue #254: a pre-discovery poll failure must never look like a -# successful first refresh. +# 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 handshake timeout on the very first poll reconnects instead of - being deferred (issue #254). - - `_poll_once` connects when there is no session, so `connect()`'s own - handshake timeout arrives here as a `TimeoutError` -- indistinguishable - by type from a slow blockwise transfer, but meaning a dead connection. - Deferring it returned `flatten([], {})` == `{}` without raising, which - `DataUpdateCoordinator` counts as a successful refresh, so the entry - loaded with `bound` empty and every platform added zero entities. - """ - calls = {'n': 0} - - def _poll(self): - calls['n'] += 1 - if calls['n'] == 1: - raise TimeoutError('DTLS handshake timed out') - return fridge_resources - + """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', _poll), + 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 retry inside the first refresh is what makes this recoverable + # The reconnect inside the first refresh is what makes this recoverable # without HA having to fail and re-run setup at all. - assert calls['n'] == 2 + 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 this guards is entity-visible, not just state on the - # coordinator: platforms enumerate `bound` once and have no dynamic - # add-listener, so an empty `bound` at forward time means the device - # stays entity-less until a manual reload. + # 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_raises_not_ready( +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 (issue #254).""" + 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=TimeoutError('DTLS handshake timed out'), + 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() - - -async def test_first_refresh_timeout_does_not_consume_timeout_budget( - hass: HomeAssistant, mock_entry, fridge_resources -) -> None: - """The pre-discovery gate must not leave `_consecutive_poll_timeouts` - charged for a failure it already handled by reconnecting -- otherwise a - device that stumbles once at startup reaches `_POLL_TIMEOUT_LIMIT` after - fewer real mid-session timeouts than the limit names.""" - calls = {'n': 0} - - def _poll(self): - calls['n'] += 1 - if calls['n'] == 1: - raise TimeoutError('DTLS handshake timed out') - return fridge_resources - - with ( - patch('custom_components.localthings.coordinator.LocalThingsCoordinator._connect_session'), - patch('custom_components.localthings.coordinator.LocalThingsCoordinator._poll_once', _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() - - coordinator: LocalThingsCoordinator = hass.data[DOMAIN][mock_entry.entry_id] - assert coordinator._consecutive_poll_timeouts == 0 + mock_async_close.assert_awaited_once() async def test_post_discovery_timeout_is_still_deferred( hass: HomeAssistant, mock_entry, mock_coordinator_session ) -> None: - """The issue #254 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).""" + """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() @@ -1373,56 +1344,18 @@ async def test_post_discovery_timeout_is_still_deferred( assert coordinator.last_update_success -async def test_defer_reconnect_gate_is_false_before_discovery() -> None: - """`_defer_reconnect_for` unit-level: the pre-discovery gate wins over - every other deferral reason, including a healthy-push session.""" - coordinator = LocalThingsCoordinator.__new__(LocalThingsCoordinator) - coordinator._discovered = False - assert coordinator._defer_reconnect_for(TimeoutError('handshake')) is False - - -async def test_session_closed_when_first_refresh_fails( - hass: HomeAssistant, mock_entry -) -> None: - """A failed setup must not leak the session it opened. `_poll_once` - leaves the session up on a `TimeoutError` by design, and every device's - source port is fixed, so an abandoned socket squats the exact port HA's - next setup attempt binds.""" - with ( - patch('custom_components.localthings.coordinator.LocalThingsCoordinator._connect_session'), - patch( - 'custom_components.localthings.coordinator.LocalThingsCoordinator._poll_once', - side_effect=TimeoutError('DTLS handshake timed out'), - ), - patch( - 'custom_components.localthings.coordinator.LocalThingsCoordinator._close_session', - ) as mock_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 mock_close.called, "failed setup left the DTLS socket open" - - 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 (issue #254). - - HA does not unload config entries on a Core restart, so `async_close`'s - only other caller (`async_unload_entry`) never runs -- leaving the - association orphaned on the appliance, which is what makes the *next* - run's handshake time out. - """ + """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_async_close: + 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_async_close.assert_awaited_once() + mock_close.assert_awaited_once()