Document the reconnect-timing change from smartthings-local's fail-fast fix
Answers a review callout on the 0.1.6 upgrade that wasn't actually addressed, only mentioned in a commit message: a dead reader now raises SessionClosedError (a ConnectionError) instead of hanging a request out to its timeout and surfacing as an ambiguous TimeoutError. Mechanically this was already routed correctly -- SessionClosedError isn't a TimeoutError, so _defer_reconnect_for's isinstance check already skips its multi-cycle tolerance for it -- but nothing recorded *why*, and the callout's whole point was that downstream (i.e. this repo) should hear about the resulting timing change explicitly, not infer it from the dependency bump. Before 0.1.6, a truly dead reader was indistinguishable here from a slow blockwise transfer: both could only ever surface as a TimeoutError, so _POLL_TIMEOUT_LIMIT's multi-cycle tolerance (~2 minutes at the default 30s interval) was the only thing standing between a genuinely dead session and a reconnect. Now that the library confirms reader death directly, that failure mode skips the tolerance and reconnects on the very first occurrence -- intended, and strictly faster recovery, but a real change in observed timing worth calling out for anyone correlating reconnect-log cadence with device behavior. - _poll_once, _defer_reconnect_for, and the _POLL_TIMEOUT_LIMIT comment now say so directly, cross-referencing each other. - README's "Known device behavior" section gets a paragraph so this isn't only visible to someone reading the coordinator's source. - Two new unit tests pin the distinction directly: _defer_reconnect_for(SessionClosedError()) is False (no tolerance), while SessionTimeoutError keeps the existing _POLL_TIMEOUT_LIMIT tolerance -- so a future change can't quietly merge the two paths back together. Full suite (1523 tests), ruff, and ty pass.
This commit is contained in:
1 parent
f949ff04c2
commit
74bdc04f56
3 files changed
+58
-2
No files matched your search
@@ -268,6 +268,8 @@ Samsung's firmware occasionally drops the DTLS session briefly — this is norma
|
||||
|
||||
If reconnects become persistent (more than a handful per minute), something's actually wrong. Check the appliance's Wi-Fi link first, then look for a competing DTLS client on the LAN — only one active session per appliance is allowed at a time.
|
||||
|
||||
Reconnect *timing* depends on how confidently the drop was detected. An ambiguous failure — one block's ACK on the summary poll came back late, which doesn't by itself prove the session is dead — is tolerated for a few consecutive poll cycles (worst case ~2 minutes at the default 30s interval) before the integration gives up and reconnects, so one slow transfer can't tear down a working push subscription. A confirmed dead connection skips that tolerance and reconnects immediately: `smartthings-local` (>= 0.1.6) detects its own reader thread exiting and raises a distinct error for it, rather than the ambiguous timeout that failure used to produce on older library versions. So you may see a reconnect log line appear sooner than that old ~2-minute worst case for a session that's actually gone — that's the fast path working as intended, not a new source of flapping.
|
||||
|
||||
Deregistering a device in SmartThings causes a reset of its network settings as soon as it accesses Samsung's servers, dropping it off Wi-Fi until it's re-onboarded through the SmartThings app. As such, consider keeping devices registered even if egress-blocked, to avoid them resetting upon brief internet access.
|
||||
|
||||
### Multi-subdevice ("2-in-1") air conditioner systems
|
||||
|
||||
@@ -208,7 +208,10 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]):
|
||||
# A block-level ACK timeout on the summary GET doesn't prove the session
|
||||
# is dead (see _poll_once) -- require this many in a row before treating
|
||||
# it as one, so one slow transfer doesn't tear down a working OBSERVE
|
||||
# subscription.
|
||||
# subscription. Only covers that ambiguous case: smartthings-local
|
||||
# >= 0.1.6 raises a distinct SessionClosedError, not a TimeoutError, the
|
||||
# moment a dead reader thread is confirmed, and _defer_reconnect_for
|
||||
# never defers that -- see its docstring for what changed there.
|
||||
_POLL_TIMEOUT_LIMIT: int = 3
|
||||
|
||||
# Named (not inline literals) so the write-settle window in
|
||||
@@ -722,6 +725,14 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]):
|
||||
not that the session is dead (earlier blocks succeeded). Left open;
|
||||
`_async_update_data` decides whether repeated timeouts warrant a
|
||||
reconnect. Any other exception is unambiguous -- close immediately.
|
||||
|
||||
smartthings-local >= 0.1.6 tells those two cases apart itself now:
|
||||
a reader thread that has actually died raises `SessionClosedError`
|
||||
(a ConnectionError, not a TimeoutError) the moment the next request
|
||||
notices, instead of the old behavior of quietly hanging out to this
|
||||
call's own timeout and surfacing as an ambiguous `TimeoutError`.
|
||||
See `_defer_reconnect_for` for what that changes about how soon a
|
||||
confirmed-dead session gets reconnected.
|
||||
"""
|
||||
if self._session is None:
|
||||
self._connect_session()
|
||||
@@ -1256,6 +1267,19 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]):
|
||||
`_POLL_TIMEOUT_LIMIT` consecutive timeouts pile up. Any other
|
||||
exception reconnects immediately.
|
||||
|
||||
That includes `SessionClosedError`, which is the point: before
|
||||
smartthings-local 0.1.6, a reader thread that had actually died was
|
||||
indistinguishable from a slow transfer -- both surfaced here only as
|
||||
a `TimeoutError`, so this tolerance was the only thing standing
|
||||
between a truly dead session and a reconnect, worst case about
|
||||
`_POLL_TIMEOUT_LIMIT` poll cycles (~2 minutes at the default 30s
|
||||
interval). 0.1.6 confirms reader death directly and raises a
|
||||
ConnectionError subclass for it instead, which isn't a TimeoutError
|
||||
and so skips this tolerance entirely -- a genuinely dead session now
|
||||
reconnects on the very first occurrence. Intended (see this repo's
|
||||
README, "Known device behavior"), not a regression, but a real
|
||||
change in observed reconnect timing for that one failure mode.
|
||||
|
||||
Never defers before first discovery (issue #254): deferring returns
|
||||
an empty dict, which the base coordinator treats as a successful
|
||||
first refresh -- and since platforms enumerate `bound` once, the
|
||||
|
||||
@@ -13,7 +13,7 @@ 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
|
||||
from smartthings_local.errors import SessionError
|
||||
from smartthings_local.errors import SessionClosedError, SessionError, SessionTimeoutError
|
||||
|
||||
from custom_components.localthings.const import (
|
||||
CONF_BYPASS_REMOTE_CONTROL,
|
||||
@@ -532,6 +532,36 @@ async def test_total_poll_failure_downgrades_observe_mode_to_poll(
|
||||
assert coordinator.last_update_success is True
|
||||
|
||||
|
||||
def test_defer_reconnect_for_reconnects_immediately_on_a_confirmed_dead_session(
|
||||
hass: HomeAssistant, mock_entry
|
||||
) -> None:
|
||||
"""smartthings-local >= 0.1.6 raises SessionClosedError -- a
|
||||
ConnectionError, not a TimeoutError -- the moment a dead reader thread
|
||||
is confirmed, instead of the old behavior of letting the request hang
|
||||
out to its own timeout and surface as an ambiguous TimeoutError.
|
||||
_defer_reconnect_for must never extend the block-ACK tolerance to a
|
||||
failure this unambiguous; see its docstring for the full reasoning."""
|
||||
coordinator = LocalThingsCoordinator(hass, mock_entry)
|
||||
coordinator._discovered = True
|
||||
|
||||
assert coordinator._defer_reconnect_for(SessionClosedError()) is False
|
||||
|
||||
|
||||
def test_defer_reconnect_for_still_tolerates_an_ambiguous_timeout(
|
||||
hass: HomeAssistant, mock_entry
|
||||
) -> None:
|
||||
"""The other half of the same distinction: a plain block-ACK timeout --
|
||||
still a TimeoutError, including smartthings-local's own
|
||||
SessionTimeoutError subclass -- keeps its multi-cycle tolerance rather
|
||||
than being swept into the immediate-reconnect path above."""
|
||||
coordinator = LocalThingsCoordinator(hass, mock_entry)
|
||||
coordinator._discovered = True
|
||||
|
||||
for _ in range(coordinator._POLL_TIMEOUT_LIMIT - 1):
|
||||
assert coordinator._defer_reconnect_for(SessionTimeoutError()) is True
|
||||
assert coordinator._defer_reconnect_for(SessionTimeoutError()) is False
|
||||
|
||||
|
||||
async def test_poll_timeout_skips_reconnect_when_push_is_healthy(
|
||||
hass: HomeAssistant, mock_entry, mock_coordinator_observe_session
|
||||
) -> None:
|
||||
|
||||
Reference in new issue
Block a user