diff --git a/README.md b/README.md index 550cd21..8f0e4cc 100644 --- a/README.md +++ b/README.md @@ -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 diff --git a/custom_components/localthings/coordinator.py b/custom_components/localthings/coordinator.py index b6ab809..9b40906 100644 --- a/custom_components/localthings/coordinator.py +++ b/custom_components/localthings/coordinator.py @@ -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 diff --git a/tests/localthings/test_coordinator.py b/tests/localthings/test_coordinator.py index 5c11caa..6dfd2d0 100644 --- a/tests/localthings/test_coordinator.py +++ b/tests/localthings/test_coordinator.py @@ -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: