Tighten the #254 fix after review
No behavior change to the fix itself; cleanup only. Production: - Trim the narrative that was told three times over (coordinator docstring, test docstring, inline comment) down to one telling in the docstring, where someone tempted to remove the gate will be standing. - Guard the second degraded return in _async_update_data on self._discovered too. That arm is currently unreachable before discovery only because every _observe.apply() call site happens to be gated on post-discovery state -- a non-local accident across four call sites. Stating the precondition where it is relied on makes it the same explicit rule _defer_reconnect_for now applies. Tests, 7 -> 4 with better discrimination: - test_session_closed_when_first_refresh_fails asserted _close_session was called, which the reconnect path already does on its own -- so it passed with the fix removed. Merged into the persistent-timeout test and re-pointed at async_close, which only setup calls. - Dropped the __new__-built unit test: it set one attribute on an otherwise uninitialized instance, so it asserted the gate's position in the function rather than any behavior, and would have errored rather than failed if reordered. - Folded the timeout-budget test into the recovery test it was a byte-for-byte copy of, and replaced both hand-rolled call counters with the side_effect=[exc, resources] idiom already used in this file. Each of the three production changes is now independently covered: removing any one of them alone fails the suite.
This commit is contained in:
3 files changed
+75
-157
No files matched your search
@@ -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()
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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()
|
||||
Reference in new issue
Block a user