From 3f9789512df2c6a6af32eb8dda05916a9615aede Mon Sep 17 00:00:00 2001 From: Marc Billow Date: Fri, 14 Aug 2026 17:50:10 +0000 Subject: [PATCH 1/5] Update to smartthings-local 0.1.6, handle redacted typed errors Bumps the smartthings-local floor from >=0.1.2 to >=0.1.6 (manifest, requirements-dev.txt, Dockerfile) and adopts the interface/behavior changes introduced along the way: - 0.1.3 ("redacted typed failures", PR #23) replaced connect()'s ConnectionError(f"DTLS handshake error: {e}") with fixed, redacted exceptions (SessionError, SessionTimeoutError, etc.) that never carry backend text -- including the TLS alert name. The config flow's _classify_handshake_failure relied on parsing that text out of the exception (_alert_name) to tell a rejected certificate from any other handshake failure; against a current library that regex never matches again, silently downgrading every setup failure to the generic "cannot_connect" message. Fixed by adding _resolve_alert: it still tries _alert_name first (a harmless fallback if it ever matches), then falls back to one bounded smartthings_local.protocol.dtls_probe.diagnose_dtls_handshake() call against the specific port that failed, which classifies the fatal Alert straight from the raw record instead of an exception string. _handshake_and_read now threads the resolved per-port alerts into _classify_handshake_failure, so CertRejected vs. HandshakeFailed keeps working the way it did before the redaction. - 0.1.3 also moved the DTLS session onto a connected UDP socket (see endpoint.py's open_connected_udp_socket), which changes why coordinator._local_source_port needs a unique port per device -- the kernel now demuxes by the full local-port/remote-peer tuple instead of relying on an unconnected recvfrom(). Docstring updated to match. - 0.1.6's reader-thread fail-fast fix (_check_live/_reader_running) makes a dead reader raise SessionClosedError immediately instead of hanging a request out to its timeout. No code change needed: SessionClosedError is a ConnectionError subclass (not TimeoutError), so the coordinator's existing _defer_reconnect_for/isinstance(e, TimeoutError) split already routes it to the immediate-reconnect path. Every other new/changed piece (endpoint.py, dtls_probe.py bounded probing, auth.py's CertificateAuth/PskAuth providers) stays behind compatible built-in exception types and unchanged get()/post()/ subscribe()/ping() signatures, per the library's own compatibility table, so the coordinator's and observe.py's broad exception handling needed no changes. Tests: added coverage for _resolve_alert's exception-text vs. diagnostic-handshake fallback, _classify_handshake_failure with a resolved alerts mapping, and an end-to-end config-flow re-mint test against a FakeSession that raises the new redacted SessionError instead of the old text-bearing ConnectionError. Full suite (1517 tests), ruff, and ty all pass against smartthings-local 0.1.6 installed from PyPI. --- Dockerfile | 2 +- custom_components/localthings/config_flow.py | 88 +++++++++++--- custom_components/localthings/coordinator.py | 13 ++- custom_components/localthings/manifest.json | 2 +- requirements-dev.txt | 2 +- tests/localthings/test_config_flow.py | 114 +++++++++++++++++++ 6 files changed, 200 insertions(+), 21 deletions(-) diff --git a/Dockerfile b/Dockerfile index 8f7f9ee..9f4ff04 100644 --- a/Dockerfile +++ b/Dockerfile @@ -6,4 +6,4 @@ FROM ghcr.io/home-assistant/home-assistant:stable # repeats the install attempt on every container recreate. Baking # smartthings-local into the image keeps the dev container usable # offline and avoids relying on that runtime install path. -RUN pip3 install --no-cache-dir "smartthings-local>=0.1.2" +RUN pip3 install --no-cache-dir "smartthings-local>=0.1.6" diff --git a/custom_components/localthings/config_flow.py b/custom_components/localthings/config_flow.py index d965d7b..1c0ae0d 100644 --- a/custom_components/localthings/config_flow.py +++ b/custom_components/localthings/config_flow.py @@ -493,27 +493,75 @@ _CERT_ALERTS = frozenset( } ) -# OpenSSL renders a received fatal alert into its error text as e.g. -# "tlsv1 alert unknown ca", which DtlsCoapSession.connect() wraps in a -# ConnectionError. Reading it back tells us what the appliance objected to. -# -# Deliberately not the library's diagnostic probe (stateless=False): that -# mode commits association state on the device, and an orphaned association -# makes the next attempt time out (RFC 6347 §4.2.8) -- a bad trade on a -# path the user is about to retry. +# Older smartthings-local (< 0.1.3) rendered a received fatal alert straight +# into the handshake exception's text, e.g. "tlsv1 alert unknown ca" wrapped +# in a ConnectionError -- reading it back told us what the appliance +# objected to. 0.1.3's "redacted typed failures" removed that: connect()'s +# exceptions now carry a fixed, non-sensitive message with the real OpenSSL +# text neither included nor chained (see smartthings_local.errors -- +# "backend errors can contain remote endpoints, local paths, or credential +# metadata"). _alert_name is kept as a harmless fallback for exception text +# that does carry it; _resolve_alert below is what actually classifies a +# failure against a current library. _ALERT_RE = re.compile(r"alert ([a-z0-9 ]+)") def _alert_name(exc: Exception) -> str | None: - """The TLS alert an appliance sent, if this failure carried one.""" + """The TLS alert an appliance sent, if this failure's exception text + carried one (only ever true against smartthings-local < 0.1.3).""" match = _ALERT_RE.search(str(exc).lower()) return match.group(1).strip().replace(" ", "_") if match else None +def _diagnostic_alert(host: str, port: int, cert_pem: str, key_pem: str): + """One opt-in stateful handshake against `port`, using our real + credentials, so a fatal Alert can be classified from the raw record + itself rather than parsed out of an exception's text. + + This is the library's diagnose_dtls_handshake -- deliberately not used + for the primary candidate scan (it commits association state on the + device, and an orphaned association makes the *next* attempt time out + per RFC 6347 §4.2.8). Here it only runs once every real candidate has + already failed, to explain a failure that's happening either way -- + one more orphaned association is a fair trade for a message that says + why, on a path the user is about to retry regardless. + + Imported lazily, like `_clienthello_probe`, so an install whose + smartthings-local predates this API degrades to a generic message + instead of failing to load the config flow at all. + """ + from smartthings_local.protocol.dtls_probe import diagnose_dtls_handshake + + return diagnose_dtls_handshake( + host, + port, + cert_pem=cert_pem, + key_pem=key_pem, + timeout=CLIENTHELLO_PROBE_TIMEOUT_S, + retries=CLIENTHELLO_PROBE_RETRIES, + ) + + +def _resolve_alert(exc: Exception, host: str, port: int, cert_pem: str, key_pem: str) -> str | None: + """The TLS alert `port`'s failed handshake carried, if any -- the + exception's own text first (cheap, and all an older library ever + offers), then one bounded diagnostic handshake against a current one + that redacts it (see _diagnostic_alert).""" + name = _alert_name(exc) + if name is not None: + return name + try: + result = _diagnostic_alert(host, port, cert_pem, key_pem) + except Exception: + return None + return result.alert[1] if result.alert else None + + def _classify_handshake_failure( host: str, scan: _PortScan, failures: list[tuple[int, Exception]], + alerts: dict[int, str] | None = None, ) -> CannotConnect: """Turn "no port worked" into the most specific thing we can honestly say, in rough order of how much the evidence tells us: an alert means @@ -521,13 +569,21 @@ def _classify_handshake_failure( certificate); a confirmed DTLS port that then timed out is likely still holding a session from a previous attempt; otherwise the sweep's own shape is the evidence. + + `alerts` is the per-port classification `_handshake_and_read` already + resolved (exception text, or a diagnostic handshake -- see + _resolve_alert); a caller with only raw failures (or an older library) + still gets `_alert_name`'s exception-text reading as a fallback. """ - alerts = [name for name in (_alert_name(exc) for _, exc in failures) if name] - cert_alerts = [name for name in alerts if name in _CERT_ALERTS] + resolved = dict(alerts or {}) + for port, exc in failures: + resolved.setdefault(port, _alert_name(exc)) + alert_names = [name for name in resolved.values() if name] + cert_alerts = [name for name in alert_names if name in _CERT_ALERTS] if cert_alerts: return CertRejected(f"{host} rejected our certificate (alert {cert_alerts[0]})") - if alerts: - return HandshakeFailed(f"{host} refused the DTLS handshake (alert {alerts[0]})") + if alert_names: + return HandshakeFailed(f"{host} refused the DTLS handshake (alert {alert_names[0]})") if scan.confirmed: return HandshakeTimeout( f"DTLS server confirmed on {host}:{scan.confirmed} but the handshake never completed" @@ -631,6 +687,7 @@ def _handshake_and_read(host: str, scan: _PortScan, cert_pem: str, key_pem: str) from smartthings_local.protocol.dtls_session import DtlsCoapSession failures: list[tuple[int, Exception]] = [] + alerts: dict[int, str] = {} for port in scan.candidates: sess = None try: @@ -645,11 +702,14 @@ def _handshake_and_read(host: str, scan: _PortScan, cert_pem: str, key_pem: str) except Exception as exc: failures.append((port, exc)) _LOGGER.debug("port %d failed: %s", port, exc) + alert = _resolve_alert(exc, host, port, cert_pem, key_pem) + if alert is not None: + alerts[port] = alert finally: if sess is not None: with contextlib.suppress(Exception): sess.close() - raise _classify_handshake_failure(host, scan, failures) + raise _classify_handshake_failure(host, scan, failures, alerts) def _probe_and_validate( diff --git a/custom_components/localthings/coordinator.py b/custom_components/localthings/coordinator.py index d0a7ada..f10ff54 100644 --- a/custom_components/localthings/coordinator.py +++ b/custom_components/localthings/coordinator.py @@ -103,10 +103,15 @@ def _local_source_port(host: str) -> int: time per RFC 6347 §4.2.8, instead of holding it 5-15 min. See DTLS_LOCAL_PORT_BASE. Requires smartthings-local >= 0.1.1. - Must stay unique per device on this host too: the library's socket is - unconnected, so two devices sharing a port would mis-demux each other's - datagrams. Last IPv4 octet as offset for the common case; a stable - CRC32 fold otherwise. + Must stay unique per device on this host too. That used to be load- + bearing for demuxing: an unconnected socket handed every device's + datagrams to whichever recvfrom() happened to be listening on their + shared port. smartthings-local >= 0.1.3 connect()s its UDP socket + instead (see endpoint.py's open_connected_udp_socket), so the kernel + already filters incoming datagrams to each session's own resolved peer + -- but a distinct port per device keeps that guarantee from ever + depending on it, and keeps captures/logs unambiguous. Last IPv4 octet + as offset for the common case; a stable CRC32 fold otherwise. """ try: offset = int(ipaddress.IPv4Address(host)) & 0xFF diff --git a/custom_components/localthings/manifest.json b/custom_components/localthings/manifest.json index 60c97e6..0a8f5ea 100644 --- a/custom_components/localthings/manifest.json +++ b/custom_components/localthings/manifest.json @@ -10,7 +10,7 @@ "requirements": [ "cbor2>=5.4.6", "pyOpenSSL>=23.0", - "smartthings-local>=0.1.2" + "smartthings-local>=0.1.6" ], "version": "0.21.2" } diff --git a/requirements-dev.txt b/requirements-dev.txt index 6aa6f59..088db5f 100644 --- a/requirements-dev.txt +++ b/requirements-dev.txt @@ -6,7 +6,7 @@ pytest-homeassistant-custom-component>=0.13.316 # Integration runtime deps, needed to import the component under test # (also declared in custom_components/localthings/manifest.json). -smartthings-local>=0.1.2 +smartthings-local>=0.1.6 cbor2>=5.4.6 pyOpenSSL>=23.0 cryptography>=41.0 diff --git a/tests/localthings/test_config_flow.py b/tests/localthings/test_config_flow.py index a3d8e85..a20d0e6 100644 --- a/tests/localthings/test_config_flow.py +++ b/tests/localthings/test_config_flow.py @@ -238,6 +238,11 @@ class FakeSession: instances: ClassVar[list[FakeSession]] = [] reject_certs: ClassVar[set[str]] = set() + # smartthings-local >= 0.1.3 ("redacted typed failures") no longer puts + # the alert in connect()'s exception text -- set True to model that, so + # a test can check the diagnostic-handshake fallback (_resolve_alert) + # instead of the legacy _alert_name text-parsing path. + redact_rejection: ClassVar[bool] = False def __init__(self, host, port, cert_pem=None, key_pem=None, **kwargs): self.host, self.port, self.cert_pem = host, port, cert_pem @@ -245,6 +250,10 @@ class FakeSession: def connect(self): if self.cert_pem in FakeSession.reject_certs: + if FakeSession.redact_rejection: + from smartthings_local.errors import SessionError + + raise SessionError() raise ConnectionError( "DTLS handshake error: [('SSL routines', '', 'sslv3 alert bad certificate')]" ) @@ -268,6 +277,7 @@ def fake_dtls(monkeypatch): FakeSession.instances = [] FakeSession.reject_certs = set() + FakeSession.redact_rejection = False monkeypatch.setattr(config_flow, "_fetch_samsung_uuid", lambda: "test-uuid") monkeypatch.setattr( config_flow, @@ -480,6 +490,44 @@ async def test_rejected_reused_leaf_is_reminted( assert [s.cert_pem for s in FakeSession.instances] == [MOCK_LEAF_CERT_PEM, "FULLCHAIN"] +async def test_rejected_reused_leaf_is_reminted_against_a_redacted_library( + hass: HomeAssistant, monkeypatch, fake_dtls +) -> None: + """Same flow as test_rejected_reused_leaf_is_reminted, but against a + connect() failure shaped like smartthings-local >= 0.1.3 -- a fixed, + redacted exception with no alert text at all (see errors.py's + "Classified errors"). The re-mint decision has to come from + _resolve_alert's diagnostic-handshake fallback instead of _alert_name.""" + from custom_components.localthings import config_flow + + existing = MockConfigEntry(domain=DOMAIN, data=ENTRY_DATA, unique_id="localthings_other") + existing.add_to_hass(hass) + _patch_clienthello(monkeypatch, {49154}) + FakeSession.reject_certs = {MOCK_LEAF_CERT_PEM} + FakeSession.redact_rejection = True + + class _DiagnosticResult: + alert = (2, "bad_certificate") + + diagnosed: list[int] = [] + + def _diagnostic_alert(host, port, cert_pem, key_pem): + diagnosed.append(port) + return _DiagnosticResult() + + monkeypatch.setattr(config_flow, "_diagnostic_alert", _diagnostic_alert) + + result = await hass.config_entries.flow.async_init(DOMAIN, context={"source": "user"}) + result = await hass.config_entries.flow.async_configure( + result["flow_id"], {CONF_HOST: MOCK_HOST} + ) + + assert result["type"] == FlowResultType.CREATE_ENTRY + assert result["data"][CONF_LEAF_CERT_PEM] == "FULLCHAIN" + assert [s.cert_pem for s in FakeSession.instances] == [MOCK_LEAF_CERT_PEM, "FULLCHAIN"] + assert diagnosed == [49154] + + async def test_unconfirmed_port_failure_is_not_reminted( hass: HomeAssistant, monkeypatch, fake_dtls ) -> None: @@ -554,6 +602,72 @@ def test_cert_alert_is_reported_as_a_certificate_problem() -> None: assert err.error_key == "cert_rejected" +def test_classify_handshake_failure_uses_a_resolved_alert_over_exception_text() -> None: + """_handshake_and_read passes its own resolved `alerts` mapping (built + via _resolve_alert, which is what actually classifies a failure against + smartthings-local >= 0.1.3's redacted exceptions) -- it must win even + when the exception text itself says nothing.""" + from custom_components.localthings.config_flow import ( + CertRejected, + _classify_handshake_failure, + ) + + err = _classify_handshake_failure( + MOCK_HOST, + _scan(confirmed=[49154]), + [(49154, RuntimeError("session operation failed"))], + {49154: "bad_certificate"}, + ) + assert isinstance(err, CertRejected) + assert err.error_key == "cert_rejected" + + +def test_resolve_alert_prefers_exception_text_over_the_diagnostic_handshake() -> None: + """A library still stamping the alert into its exception text (< 0.1.3) + answers for free; the diagnostic handshake must not run at all then.""" + from custom_components.localthings.config_flow import _resolve_alert + + def _must_not_run(*args, **kwargs): + raise AssertionError("must not run the diagnostic handshake") + + with patch("custom_components.localthings.config_flow._diagnostic_alert", _must_not_run): + name = _resolve_alert( + _openssl_alert("tlsv1 alert unknown ca"), MOCK_HOST, 49154, "CERT", "KEY" + ) + assert name == "unknown_ca" + + +def test_resolve_alert_falls_back_to_the_diagnostic_handshake() -> None: + """smartthings-local >= 0.1.3 redacts the exception text (see errors.py's + "Classified errors"), so the only way left to learn *why* a handshake + failed is the library's own classification of the raw alert record.""" + from smartthings_local.errors import SessionError + + from custom_components.localthings import config_flow + + class _Result: + alert = (2, "bad_certificate") + + with patch.object(config_flow, "_diagnostic_alert", lambda *a, **k: _Result()): + name = config_flow._resolve_alert(SessionError(), MOCK_HOST, 49154, "CERT", "KEY") + assert name == "bad_certificate" + + +def test_resolve_alert_is_none_when_the_diagnostic_handshake_also_fails() -> None: + """A best-effort extra probe: its own failure must not raise out of + _resolve_alert, it just leaves the caller with no alert to report.""" + from smartthings_local.errors import SessionError, SessionTimeoutError + + from custom_components.localthings import config_flow + + def _boom(*args, **kwargs): + raise SessionTimeoutError() + + with patch.object(config_flow, "_diagnostic_alert", _boom): + name = config_flow._resolve_alert(SessionError(), MOCK_HOST, 49154, "CERT", "KEY") + assert name is None + + def test_non_cert_alert_is_kept_distinct_from_a_cert_problem() -> None: """A cipher or version mismatch is also a deliberate refusal, but no amount of fiddling with CA credentials will fix it.""" From f949ff04c2823b563d8b214154bcf7d76e94f839 Mon Sep 17 00:00:00 2001 From: Marc Billow Date: Fri, 14 Aug 2026 18:09:09 +0000 Subject: [PATCH 2/5] coordinator: close four uncaught-exception gaps around smartthings_local A follow-up review of the 0.1.6 upgrade found four call sites where a library exception (new typed one or the old bare ConnectionError/ TimeoutError it replaced) could escape this integration's own reconnect/logging or a service call's translation layer entirely, instead of being handled the way equivalent failures already are elsewhere in this file: - _attempt_observe_mode's own _connect_session() reconnect (fires only when the session was closed out from under it concurrently) had no try/except, and neither did either of its two call sites in _async_update_data. A failure there escaped uncaught: HA's DataUpdateCoordinator has its own final safety net so nothing crashed the config entry, but non-TimeoutError failures logged a full ERROR traceback instead of this integration's deliberately quiet "poll failed, reconnecting" voice, and skipped its own reconnect bookkeeping entirely. Fixed by catching around just the connect call (the only unguarded raise path in the method -- subscribe_hrefs/ await_observe_notifies already handle their own failures), landing in the same "give up on push this cycle" state abandon_observe_attempt() already produces for the subscribe-failed and stale-session branches. Deliberately does not touch _close_session() (self._session is already None here -- _connect_session only ever publishes it after a full success), _reconnect_is_frequent() (that window records the poll path's own reconnects; feeding it a secondary path's failure would over-trigger its warning threshold), or _resubscribe_due (that flag means "a live session nothing has tried yet" -- setting it here would re-enter the doomed handshake every cycle instead of letting _last_observe_attempt_ts pace the retry). - _enumerate_subdevices_blocking's _connect_session() call (first discovery only) had the same gap. Fixed the same way: log and fall through on the resources _poll_once already returned this cycle, rather than losing first discovery over a failed subdevice probe. - async_raw_read (backing the read_resource service) had no exception handling at all -- a session/network failure during a live debug read reached the service caller as a raw, untranslated library exception, unlike write_resource's equivalent path. Now wrapped the same way async_send_command/async_raw_write_sequence already are, raising HomeAssistantError with a new debug_read_failed translation key (added to all 7 shipped locales). - async_raw_write_sequence's verify_after tail sat outside the method's own try/except, so a failed confirmation read discarded the write results that had already landed by throwing past them. Now caught per-href inside the verify loop instead: a failed read is treated the same as a 4.04/empty one (held=None, "couldn't verify" -- not lost or misreported as a revert), and the rest of the batch still gets checked. Design for the first fix (the trickiest -- it's mid-lock, and has to interact correctly with observe-mode state and the poll path's own bookkeeping without corrupting either) was worked through with a dedicated review pass before implementing. Tests: new coverage for all four (test_coordinator.py's test_attempt_observe_mode_survives_a_failed_reconnect, a new test_coordinator_error_handling.py for the subdevice-enumeration case, and two additions to test_services.py for the read-service and verify_after cases). Full suite (1521 tests), ruff, and ty all pass. --- custom_components/localthings/coordinator.py | 91 +++++++++++++++++-- .../localthings/translations/cs.json | 3 + .../localthings/translations/de.json | 3 + .../localthings/translations/en.json | 3 + .../localthings/translations/es.json | 3 + .../localthings/translations/it.json | 3 + .../localthings/translations/ko.json | 3 + .../localthings/translations/nl.json | 3 + tests/localthings/test_coordinator.py | 33 +++++++ tests/test_coordinator_error_handling.py | 67 ++++++++++++++ tests/test_services.py | 51 +++++++++++ 11 files changed, 253 insertions(+), 10 deletions(-) create mode 100644 tests/test_coordinator_error_handling.py diff --git a/custom_components/localthings/coordinator.py b/custom_components/localthings/coordinator.py index f10ff54..b6ab809 100644 --- a/custom_components/localthings/coordinator.py +++ b/custom_components/localthings/coordinator.py @@ -1169,7 +1169,38 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): if self._session is None: # _poll_once already connects on a real poll; only fires if # the session was closed out from under us concurrently. - await self.hass.async_add_executor_job(self._connect_session) + try: + await self.hass.async_add_executor_job(self._connect_session) + except Exception as e: + # Not a poll failure -- the poll that reached this line + # already succeeded, and observe mode is only an + # optimization on top of it. Give up on push this cycle + # the same way the two branches below do, rather than + # letting this escape _async_update_data uncaught: none + # of this integration's reconnect bookkeeping would run, + # and the base coordinator logs an ERROR traceback in + # place of the deliberately quiet "poll failed, + # reconnecting" voice used everywhere else in this file. + # + # Not counted by _reconnect_is_frequent() (that window + # records reconnects the poll path itself performed -- + # feeding it a secondary path's failure would push the + # next routine poll reconnect over the warn threshold), + # and nothing to _close_session(): _connect_session only + # publishes self._session once connect() and + # start_reader() have both already succeeded, so it's + # still None here. No _resubscribe_due either -- that + # flag means "a live session nothing has tried yet", + # and setting it would re-enter this doomed handshake + # every cycle; _last_observe_attempt_ts (stamped above) + # already paces the retry to _RECOVERY_RETRY_S. + self._log.info( + "observe-mode reconnect failed (%s), staying on polling: %s", + type(e).__name__, + e, + ) + self._observe.abandon_observe_attempt() + return sess = self._session if sess is None: return @@ -1327,9 +1358,22 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): # cycle's snapshot so discovery sees every subdevice on the # first poll rather than waiting a cycle. async with self._session_lock: - resources = await self.hass.async_add_executor_job( - self._enumerate_subdevices_blocking, resources - ) + try: + resources = await self.hass.async_add_executor_job( + self._enumerate_subdevices_blocking, resources + ) + except Exception as e: + # _connect_session() inside here only runs at all if the + # session the poll above just used got closed out from + # under us within this same cycle -- rare, but not + # impossible, and unguarded before this. Losing the + # subdevice probe isn't losing first discovery: `resources` + # keeps the value _poll_once already returned, so + # discovery below still runs on the master's own data, + # same posture _poll_subdevice_seed takes for one sibling + # going quiet. self.subdevices/_discovered are still + # unset, so the probe retries naturally next cycle. + self._log.debug("subdevice enumeration failed: %s", e) source = "sweep" if self._discovered else "poll" first_cycle = not self._discovered @@ -1631,9 +1675,23 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): ) norm_href = "/" + "/".join(path_segs) async with self._session_lock: - return await self.hass.async_add_executor_job( - self._raw_read_blocking, path_segs, norm_href - ) + try: + return await self.hass.async_add_executor_job( + self._raw_read_blocking, path_segs, norm_href + ) + except Exception as e: + # Unlike async_raw_write_sequence, there's nothing to + # reconnect-and-retry here -- a live debug read either lands + # or it doesn't, and a service call is the one place on this + # path a raw session exception would otherwise reach a user + # untranslated (write_resource already goes through + # HomeAssistantError; this brings read_resource in line). + self._log.warning("debug read failed for %s: %s", norm_href, e) + raise HomeAssistantError( + translation_domain=DOMAIN, + translation_key="debug_read_failed", + translation_placeholders={"href": norm_href, "error": str(e)}, + ) from e async def async_raw_write_sequence( self, @@ -1741,9 +1799,22 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): verified: dict[str, Any] = {} async with self._session_lock: for href in dict.fromkeys(r["href"] for r in results): - vcode, vrep, _vbody = await self.hass.async_add_executor_job( - self._raw_read_blocking, _href_to_path_segs(href), href - ) + try: + vcode, vrep, _vbody = await self.hass.async_add_executor_job( + self._raw_read_blocking, _href_to_path_segs(href), href + ) + except Exception as e: + # The write already landed -- see `results` above, + # built before this wait ever started. A failed + # confirmation read (the session dying in the gap + # verify_after just waited out, say) must not lose + # that outcome behind a raised exception here, and + # one href's failure shouldn't stop the rest of the + # batch from being checked. Same "couldn't verify" + # posture as a 4.04/empty read below: held stays + # None, not False. + self._log.debug("raw write verification read failed for %s: %s", href, e) + vcode, vrep = 0, {} # None, not False, when the re-read brought back nothing # to compare: every comparison against an empty rep is # False, which would report a 4.04 as a revert -- the one diff --git a/custom_components/localthings/translations/cs.json b/custom_components/localthings/translations/cs.json index 9873b61..1b833b5 100644 --- a/custom_components/localthings/translations/cs.json +++ b/custom_components/localthings/translations/cs.json @@ -1558,6 +1558,9 @@ "command_failed": { "message": "Příkaz pro {href} selhal i po opětovném připojení: {error}" }, + "debug_read_failed": { + "message": "Čtení z {href} selhalo: {error}" + }, "debug_too_many_writes": { "message": "Zadejte 1 až 10 zápisů." }, diff --git a/custom_components/localthings/translations/de.json b/custom_components/localthings/translations/de.json index 649bce1..a2d33ee 100644 --- a/custom_components/localthings/translations/de.json +++ b/custom_components/localthings/translations/de.json @@ -1558,6 +1558,9 @@ "command_failed": { "message": "Der Befehl an {href} ist auch nach erneutem Verbinden fehlgeschlagen: {error}" }, + "debug_read_failed": { + "message": "Das Lesen von {href} ist fehlgeschlagen: {error}" + }, "debug_too_many_writes": { "message": "Geben Sie zwischen 1 und 10 Schreibvorgänge an." }, diff --git a/custom_components/localthings/translations/en.json b/custom_components/localthings/translations/en.json index b4e96c8..30839a7 100644 --- a/custom_components/localthings/translations/en.json +++ b/custom_components/localthings/translations/en.json @@ -1558,6 +1558,9 @@ "command_failed": { "message": "The command to {href} failed even after reconnecting: {error}" }, + "debug_read_failed": { + "message": "The read from {href} failed: {error}" + }, "debug_too_many_writes": { "message": "Provide between 1 and 10 writes." }, diff --git a/custom_components/localthings/translations/es.json b/custom_components/localthings/translations/es.json index 05bbe46..763d485 100644 --- a/custom_components/localthings/translations/es.json +++ b/custom_components/localthings/translations/es.json @@ -173,6 +173,9 @@ "command_failed": { "message": "El comando para {href} falló incluso después de reconectar: {error}" }, + "debug_read_failed": { + "message": "La lectura desde {href} falló: {error}" + }, "debug_too_many_writes": { "message": "Proporciona entre 1 y 10 escrituras." }, diff --git a/custom_components/localthings/translations/it.json b/custom_components/localthings/translations/it.json index f96e426..d785c6f 100644 --- a/custom_components/localthings/translations/it.json +++ b/custom_components/localthings/translations/it.json @@ -1558,6 +1558,9 @@ "command_failed": { "message": "Il comando per {href} è fallito anche dopo la riconnessione: {error}" }, + "debug_read_failed": { + "message": "La lettura da {href} è fallita: {error}" + }, "debug_too_many_writes": { "message": "Specificare da 1 a 10 scritture." }, diff --git a/custom_components/localthings/translations/ko.json b/custom_components/localthings/translations/ko.json index c6230ff..540d48e 100644 --- a/custom_components/localthings/translations/ko.json +++ b/custom_components/localthings/translations/ko.json @@ -1558,6 +1558,9 @@ "command_failed": { "message": "재연결 후에도 {href} 명령이 실패했습니다: {error}" }, + "debug_read_failed": { + "message": "{href}에서 읽기가 실패했습니다: {error}" + }, "debug_too_many_writes": { "message": "1~10개의 쓰기를 지정하세요." }, diff --git a/custom_components/localthings/translations/nl.json b/custom_components/localthings/translations/nl.json index 12875d3..4ed576e 100644 --- a/custom_components/localthings/translations/nl.json +++ b/custom_components/localthings/translations/nl.json @@ -1558,6 +1558,9 @@ "command_failed": { "message": "Het commando naar {href} is ook na opnieuw verbinden mislukt: {error}" }, + "debug_read_failed": { + "message": "Het lezen van {href} is mislukt: {error}" + }, "debug_too_many_writes": { "message": "Geef tussen de 1 en 10 schrijfacties op." }, diff --git a/tests/localthings/test_coordinator.py b/tests/localthings/test_coordinator.py index cc289fc..5c11caa 100644 --- a/tests/localthings/test_coordinator.py +++ b/tests/localthings/test_coordinator.py @@ -13,6 +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 custom_components.localthings.const import ( CONF_BYPASS_REMOTE_CONTROL, @@ -786,6 +787,38 @@ async def test_attempt_observe_mode_discards_stale_commit_after_session_swap( assert coordinator._resubscribe_due is True +async def test_attempt_observe_mode_survives_a_failed_reconnect( + hass: HomeAssistant, mock_entry, mock_coordinator_observe_session +) -> None: + """The session was closed out from under this attempt concurrently + (rare, but real -- see the docstring above), and the reconnect it tries + on the way back in fails too (smartthings-local >= 0.1.3's redacted + SessionError, or any other exception). That must not escape + _async_update_data uncaught: it should land in the same "give up on + push this cycle" state the subscribe-failed and stale-session branches + already produce, not skip this integration's own logging/state handling + entirely.""" + 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] + coordinator._session = None + coordinator._reconnect_times = [] + + with patch.object( + coordinator, + "_connect_session", + side_effect=SessionError(), + ): + await coordinator._attempt_observe_mode() # must not raise + + assert coordinator.observe_mode == MODE_POLL + assert coordinator._observe.subscribed_hrefs == set() + assert coordinator._resubscribe_due is False + # Not the poll path's own reconnect-frequency window (see the fix's + # comment) -- this failure must not count toward it. + assert coordinator._reconnect_times == [] + + async def test_maybe_retry_observe_mode_uses_most_recent_attempt_not_just_mode_change( hass: HomeAssistant, mock_entry, mock_coordinator_observe_session ) -> None: diff --git a/tests/test_coordinator_error_handling.py b/tests/test_coordinator_error_handling.py new file mode 100644 index 0000000..967633f --- /dev/null +++ b/tests/test_coordinator_error_handling.py @@ -0,0 +1,67 @@ +"""Regression tests for a handful of call sites in coordinator.py that used +to let a smartthings-local exception (EndpointError, SessionError, +SessionTimeoutError, SessionClosedError, ... -- or the equivalent bare +ConnectionError/TimeoutError/OSError an older library version raised) escape +uncaught instead of going through this integration's own reconnect/logging +or getting translated into a HomeAssistantError for a service caller. +""" + +from __future__ import annotations + +from homeassistant.core import HomeAssistant +from pytest_homeassistant_custom_component.common import MockConfigEntry + +from custom_components.localthings.const import ( + CONF_HOST, + CONF_LEAF_CERT_PEM, + CONF_LEAF_KEY_PEM, + CONF_PORT, + DOMAIN, +) +from custom_components.localthings.coordinator import LocalThingsCoordinator + +ENTRY_DATA = { + CONF_HOST: "10.0.0.198", + CONF_PORT: 49154, + CONF_LEAF_CERT_PEM: "-----BEGIN CERTIFICATE-----\nTEST-LEAF\n-----END CERTIFICATE-----", + CONF_LEAF_KEY_PEM: "-----BEGIN PRIVATE KEY-----\nTEST-LEAF-KEY\n-----END PRIVATE KEY-----", +} + + +def _coordinator(hass: HomeAssistant) -> LocalThingsCoordinator: + entry = MockConfigEntry( + domain=DOMAIN, + data=ENTRY_DATA, + unique_id="localthings_ERRHANDLING-TEST", + ) + entry.add_to_hass(hass) + return LocalThingsCoordinator(hass, entry) + + +async def test_subdevice_enumeration_failure_does_not_abort_first_discovery( + hass: HomeAssistant, +) -> None: + """_enumerate_subdevices_blocking's own _connect_session() call only + fires if the session the poll above just used got closed out from under + it within the same cycle -- rare, but until this fix, unguarded: an + exception there escaped _async_update_data entirely instead of going + through this integration's own logging, matching what already happens + for the main poll's own reconnect. + + Empty resources keep _run_discovery from binding anything (hot/warm + hrefs stay empty), so _attempt_observe_mode's own session touch never + runs either -- this test is purely about the enumeration failure not + escaping _async_update_data. + """ + coordinator = _coordinator(hass) + coordinator._poll_once = dict + + def _boom(_resources): + raise ConnectionError("session closed") + + coordinator._enumerate_subdevices_blocking = _boom + + result = await coordinator._async_update_data() + + assert coordinator._discovered is True + assert result == {} diff --git a/tests/test_services.py b/tests/test_services.py index 84b3b57..3d7f686 100644 --- a/tests/test_services.py +++ b/tests/test_services.py @@ -324,6 +324,39 @@ async def test_write_resource_verify_after_reports_reverted(hass, coordinator, d assert verified["rep"] == {"x.field": "original"} +async def test_write_resource_verify_after_survives_a_failed_confirmation_read( + hass, coordinator, device_id, monkeypatch +): + """The write itself already landed (see `results`, built before + verify_after's wait even starts) by the time the confirmation read runs + -- a session dying in the gap verify_after waits out (smartthings-local's + redacted SessionClosedError/SessionTimeoutError, or any other exception) + must not lose that outcome behind a raised exception. Same "couldn't + verify" posture as a 4.04/empty read: `held` stays None, not False.""" + fake = _FakeSession() + fake.queue_get("mode/vs/0", {"x.field": "target"}) # write's own follow-up read + coordinator._session = fake + + def _boom(path_segs, href): + raise ConnectionError("session closed") + + monkeypatch.setattr(coordinator, "_raw_read_blocking", _boom) + + with patch(_SLEEP_TARGET, new_callable=AsyncMock): + response = await _call_write( + hass, + device_id, + writes=[{"href": "/mode/vs/0", "payload": {"x.field": "target"}}], + verify_after=30, + ) + + # The write's own results survive even though verification blew up. + assert response["results"][0]["accepted"] is True + verified = response["verified"]["/mode/vs/0"] + assert verified["held"] is None + assert verified["rep"] == {} + + async def test_write_resource_no_verified_key_when_verify_after_is_zero( hass, coordinator, device_id ): @@ -605,6 +638,24 @@ async def test_read_resource_surfaces_a_collections_list_body(hass, coordinator, assert response["body"] == batch +async def test_read_resource_failure_is_surfaced_as_a_home_assistant_error( + hass, coordinator, device_id, monkeypatch +): + """A session/network failure during a live debug read (e.g. + smartthings-local's redacted SessionError, or any other exception) must + not reach the service caller raw and untranslated -- write_resource + already goes through HomeAssistantError on failure, and async_raw_read + must match that instead of letting the exception escape uncaught.""" + + def _boom(path_segs, href): + raise ConnectionError("session closed") + + monkeypatch.setattr(coordinator, "_raw_read_blocking", _boom) + + with pytest.raises(HomeAssistantError): + await _call_read(hass, device_id, href="/mode/vs/0") + + async def test_read_resource_without_href_returns_cached_snapshot_and_does_not_get( hass, coordinator, device_id ): From 74bdc04f568376724ec168476923e370f16b599e Mon Sep 17 00:00:00 2001 From: Marc Billow Date: Fri, 14 Aug 2026 18:15:32 +0000 Subject: [PATCH 3/5] 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. --- README.md | 2 ++ custom_components/localthings/coordinator.py | 26 +++++++++++++++- tests/localthings/test_coordinator.py | 32 +++++++++++++++++++- 3 files changed, 58 insertions(+), 2 deletions(-) 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: From 389c65adbc677bb0522f340ad2df01b50f0fe37c Mon Sep 17 00:00:00 2001 From: Marc Billow Date: Fri, 14 Aug 2026 18:16:32 +0000 Subject: [PATCH 4/5] README: drop the reconnect-timing paragraph, keep it in code Excessive for user-facing docs -- this is implementation detail (_defer_reconnect_for's tolerance logic) that belongs in the coordinator's own comments, where it still lives, not in "Known device behavior". No functional change. --- README.md | 2 -- 1 file changed, 2 deletions(-) diff --git a/README.md b/README.md index 8f0e4cc..550cd21 100644 --- a/README.md +++ b/README.md @@ -268,8 +268,6 @@ 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 From 1abaad7f406eff3823b94428fd5b3ef67fbfd3ee Mon Sep 17 00:00:00 2001 From: Marc Billow Date: Fri, 14 Aug 2026 18:43:44 +0000 Subject: [PATCH 5/5] Fix issues found by an Opus review of the smartthings-local upgrade MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An independent review of the last two commits' diff turned up five real problems and one CI-breaking one. Fixed all of them: - tests/test_coordinator_error_handling.py assigned directly onto coordinator instance attributes (coordinator._poll_once = dict), which `ty check custom_components tests` -- what CI actually runs, not the narrower `ty check custom_components` this branch had only been spot-checked against -- flags as invalid-assignment. Switched to monkeypatch.setattr, matching every other new test on this branch. - coordinator.py's subdevice-enumeration failure comment claimed the probe "retries naturally next cycle." It doesn't: _run_discovery sets self._discovered = True unconditionally later in the same cycle, which is what gates the whole block, so a failure here is a first-and-only attempt, not a retried one -- a composite appliance's sibling subdevices are missing for the config entry's lifetime until reload. (A separate flag to retry wouldn't actually fix that either: every platform's async_setup_entry enumerates coordinator.bound exactly once, so a later-successful enumeration still couldn't add entities without a reload.) Corrected the comment and raised debug to warning, since the effect is silent and permanent otherwise. - config_flow.py's new diagnostic-handshake fallback (_resolve_alert) was being called once per failing candidate, inside _handshake_and_read's scan loop -- contradicting _diagnostic_alert's own docstring ("only runs once every real candidate has already failed"). Two real costs: up to CLIENTHELLO_PROBE_TIMEOUT_S extra latency per failing port on the sweep-fallback path (several candidates), and -- more seriously -- the diagnostic commits DTLS association state on each port it touches, which can make _probe_and_validate's own CertRejected re-mint retry (a fresh _handshake_and_read call against that same scan) time out against the very port it just polluted, per the RFC 6347 §4.2.8 concern already documented elsewhere in this file. Moved to a new _diagnose_failures helper called once, after the loop, against the single best (confirmed-live) candidate. - _resolve_alert also ignored the alert's level: ProbeResult.alert is set for a received alert of either severity, but only a fatal one (2) means the appliance broke off the handshake over it -- a warning (e.g. close_notify) was being read as a rejection reason. Older library exception text never had this ambiguity (OpenSSL only renders an exception for a fatal alert), so this was a bug the redaction fallback introduced. Now filters to level == 2. - async_raw_read's new HomeAssistantError wrapper reported a translated error but left a confirmed-dead session installed, so every subsequent read/write would keep failing identically for up to the next full poll interval. Now closes the session on any non-TimeoutError failure, matching _poll_once's own posture. - async_raw_write_sequence's verify_after fallback (vcode, vrep = 0, {}) is indistinguishable from a real 4.04 by raw_code alone. Added a read_error field so a caller can tell "couldn't verify" from "the device said no." Tests: 8 new/extended (once-not-per-port diagnostic count, alert-level filtering, the warning log + permanent-loss framing, session-closing on both async_raw_read and the verify_after path, read_error surfacing). Full suite (1527 tests), ruff, ruff format, and -- critically -- `ty check custom_components tests` (the CI-matching invocation) all pass. --- custom_components/localthings/config_flow.py | 50 ++++++++++++++++-- custom_components/localthings/coordinator.py | 36 +++++++++++-- tests/localthings/test_config_flow.py | 53 ++++++++++++++++++++ tests/test_coordinator_error_handling.py | 17 +++++-- tests/test_services.py | 45 +++++++++++++++++ 5 files changed, 188 insertions(+), 13 deletions(-) diff --git a/custom_components/localthings/config_flow.py b/custom_components/localthings/config_flow.py index 1c0ae0d..3b49039 100644 --- a/custom_components/localthings/config_flow.py +++ b/custom_components/localthings/config_flow.py @@ -554,7 +554,16 @@ def _resolve_alert(exc: Exception, host: str, port: int, cert_pem: str, key_pem: result = _diagnostic_alert(host, port, cert_pem, key_pem) except Exception: return None - return result.alert[1] if result.alert else None + if result.alert is None: + return None + level, name = result.alert + # ProbeResult.alert is set for a *received* alert record of either + # level -- fatal (2) means the appliance actually broke off the + # handshake over it; a warning (1, e.g. close_notify on an otherwise + # ordinary close) is not evidence of a rejection and must not be read + # as one. The old exception-text path never had this ambiguity: an + # OpenSSL exception only ever rendered for a fatal alert. + return name if level == 2 else None def _classify_handshake_failure( @@ -682,12 +691,45 @@ def _read_device(sess, host: str, port: int) -> dict: } +def _diagnose_failures( + host: str, + scan: _PortScan, + failures: list[tuple[int, Exception]], + cert_pem: str, + key_pem: str, +) -> dict[int, str]: + """At most one diagnostic handshake (see _diagnostic_alert) across every + port `_handshake_and_read` just gave up on -- not one per port. + + Called only after that loop has fully exhausted `scan.candidates`, never + interleaved with it: `_diagnostic_alert`'s own docstring says the extra + orphaned association it costs is a fair trade "on a path the user is + about to retry regardless" -- true for the retry `_probe_and_validate` + itself makes on a CertRejected (a fresh `_handshake_and_read` call + against this same `scan`), but only if that retry's real handshake + attempts are the ones landing on a clean slate. Running the diagnostic + per candidate mid-loop would pollute exactly the port(s) that retry is + about to reattempt; running several of them multiplies both the latency + (each is its own bounded handshake) and the pollution for no extra + classification value, since _classify_handshake_failure only ever needs + one alert to decide. + + Targets a confirmed-live port over an unconfirmed sweep candidate -- + the one actually worth spending the extra handshake on. + """ + if not failures: + return {} + by_port = dict(failures) + port = next((p for p in scan.confirmed if p in by_port), next(iter(by_port))) + alert = _resolve_alert(by_port[port], host, port, cert_pem, key_pem) + return {port: alert} if alert is not None else {} + + def _handshake_and_read(host: str, scan: _PortScan, cert_pem: str, key_pem: str) -> dict: """Handshake each candidate in turn, returning the first device that answers.""" from smartthings_local.protocol.dtls_session import DtlsCoapSession failures: list[tuple[int, Exception]] = [] - alerts: dict[int, str] = {} for port in scan.candidates: sess = None try: @@ -702,13 +744,11 @@ def _handshake_and_read(host: str, scan: _PortScan, cert_pem: str, key_pem: str) except Exception as exc: failures.append((port, exc)) _LOGGER.debug("port %d failed: %s", port, exc) - alert = _resolve_alert(exc, host, port, cert_pem, key_pem) - if alert is not None: - alerts[port] = alert finally: if sess is not None: with contextlib.suppress(Exception): sess.close() + alerts = _diagnose_failures(host, scan, failures, cert_pem, key_pem) raise _classify_handshake_failure(host, scan, failures, alerts) diff --git a/custom_components/localthings/coordinator.py b/custom_components/localthings/coordinator.py index 9b40906..c41785c 100644 --- a/custom_components/localthings/coordinator.py +++ b/custom_components/localthings/coordinator.py @@ -1395,9 +1395,24 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): # keeps the value _poll_once already returned, so # discovery below still runs on the master's own data, # same posture _poll_subdevice_seed takes for one sibling - # going quiet. self.subdevices/_discovered are still - # unset, so the probe retries naturally next cycle. - self._log.debug("subdevice enumeration failed: %s", e) + # going quiet. + # + # Not a one-cycle blip, though: `_run_discovery` a few + # lines below sets `self._discovered = True` + # unconditionally this same cycle, which is what gates + # this whole block -- there is no next cycle where this + # is retried. A composite appliance whose enumeration + # fails here loses its sibling subdevices' entities for + # this config entry's lifetime (a reload probes again). + # warning, not debug, because of that: it's silent and + # permanent otherwise, with nothing in the log pointing + # at why a device is missing entities it should have. + self._log.warning( + "subdevice enumeration failed on first discovery; " + "any sibling subdevices will be missing until this " + "config entry is reloaded: %s", + e, + ) source = "sweep" if self._discovered else "poll" first_cycle = not self._discovered @@ -1710,6 +1725,14 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): # path a raw session exception would otherwise reach a user # untranslated (write_resource already goes through # HomeAssistantError; this brings read_resource in line). + if not isinstance(e, TimeoutError): + # Same TimeoutError-vs-anything-else split as + # _poll_once: a block-ACK timeout alone doesn't prove + # the session is dead, but anything else does -- and + # leaving a confirmed-dead one installed would fail + # every read/write identically until the next real + # poll cycle's own reconnect notices. + await self.hass.async_add_executor_job(self._close_session) self._log.warning("debug read failed for %s: %s", norm_href, e) raise HomeAssistantError( translation_domain=DOMAIN, @@ -1823,6 +1846,7 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): verified: dict[str, Any] = {} async with self._session_lock: for href in dict.fromkeys(r["href"] for r in results): + read_error: str | None = None try: vcode, vrep, _vbody = await self.hass.async_add_executor_job( self._raw_read_blocking, _href_to_path_segs(href), href @@ -1836,9 +1860,12 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): # one href's failure shouldn't stop the rest of the # batch from being checked. Same "couldn't verify" # posture as a 4.04/empty read below: held stays - # None, not False. + # None, not False -- but raw_code 0 alone is also + # what a 4.04 produces, so read_error is what tells + # the two apart for a caller inspecting the response. self._log.debug("raw write verification read failed for %s: %s", href, e) vcode, vrep = 0, {} + read_error = str(e) # None, not False, when the re-read brought back nothing # to compare: every comparison against an empty rep is # False, which would report a 4.04 as a revert -- the one @@ -1853,6 +1880,7 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): if read_ok else None ), + "read_error": read_error, } response["verified"] = verified diff --git a/tests/localthings/test_config_flow.py b/tests/localthings/test_config_flow.py index a20d0e6..c531a99 100644 --- a/tests/localthings/test_config_flow.py +++ b/tests/localthings/test_config_flow.py @@ -528,6 +528,39 @@ async def test_rejected_reused_leaf_is_reminted_against_a_redacted_library( assert diagnosed == [49154] +def test_diagnostic_handshake_runs_once_not_once_per_failing_port(monkeypatch) -> None: + """_diagnostic_alert commits association state on the device (see its + own docstring) -- running it once per failing candidate instead of once + overall would both add latency (each is its own bounded handshake) and + multiply that pollution right before _probe_and_validate might retry a + real handshake against these very same ports. Three candidates fail + here; the diagnostic must run exactly once, against the confirmed-live + port, not three times against every candidate in scan order.""" + from custom_components.localthings import config_flow + + FakeSession.instances = [] + FakeSession.reject_certs = {MOCK_LEAF_CERT_PEM} + FakeSession.redact_rejection = True + monkeypatch.setattr("smartthings_local.protocol.dtls_session.DtlsCoapSession", FakeSession) + + diagnosed: list[int] = [] + + class _DiagnosticResult: + alert = (2, "bad_certificate") + + def _diagnostic_alert(host, port, cert_pem, key_pem): + diagnosed.append(port) + return _DiagnosticResult() + + monkeypatch.setattr(config_flow, "_diagnostic_alert", _diagnostic_alert) + + scan = _scan(confirmed=[49153, 49154], candidates=[49153, 49154, 49155]) + with pytest.raises(config_flow.CertRejected): + config_flow._handshake_and_read(MOCK_HOST, scan, MOCK_LEAF_CERT_PEM, "KEY") + + assert diagnosed == [49153] # confirmed-live, and only once + + async def test_unconfirmed_port_failure_is_not_reminted( hass: HomeAssistant, monkeypatch, fake_dtls ) -> None: @@ -653,6 +686,26 @@ def test_resolve_alert_falls_back_to_the_diagnostic_handshake() -> None: assert name == "bad_certificate" +def test_resolve_alert_ignores_a_non_fatal_alert() -> None: + """ProbeResult.alert is set for a *received* alert of either level, but + only a fatal one (2) means the appliance actually broke off the + handshake over it -- a warning-level alert (e.g. close_notify on an + otherwise ordinary close) is not evidence of a rejection. The old + exception-text path never had this ambiguity: OpenSSL's exception only + ever rendered for a fatal alert, so nothing pre-0.1.3 could confuse the + two -- the diagnostic-handshake fallback must not introduce the mix-up.""" + from smartthings_local.errors import SessionError + + from custom_components.localthings import config_flow + + class _Result: + alert = (1, "close_notify") # warning level, not fatal + + with patch.object(config_flow, "_diagnostic_alert", lambda *a, **k: _Result()): + name = config_flow._resolve_alert(SessionError(), MOCK_HOST, 49154, "CERT", "KEY") + assert name is None + + def test_resolve_alert_is_none_when_the_diagnostic_handshake_also_fails() -> None: """A best-effort extra probe: its own failure must not raise out of _resolve_alert, it just leaves the caller with no alert to report.""" diff --git a/tests/test_coordinator_error_handling.py b/tests/test_coordinator_error_handling.py index 967633f..81f8d8b 100644 --- a/tests/test_coordinator_error_handling.py +++ b/tests/test_coordinator_error_handling.py @@ -8,6 +8,7 @@ or getting translated into a HomeAssistantError for a service caller. from __future__ import annotations +import pytest from homeassistant.core import HomeAssistant from pytest_homeassistant_custom_component.common import MockConfigEntry @@ -39,7 +40,7 @@ def _coordinator(hass: HomeAssistant) -> LocalThingsCoordinator: async def test_subdevice_enumeration_failure_does_not_abort_first_discovery( - hass: HomeAssistant, + hass: HomeAssistant, monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture ) -> None: """_enumerate_subdevices_blocking's own _connect_session() call only fires if the session the poll above just used got closed out from under @@ -48,20 +49,28 @@ async def test_subdevice_enumeration_failure_does_not_abort_first_discovery( through this integration's own logging, matching what already happens for the main poll's own reconnect. + Not a one-cycle blip once caught, though: `_discovered` flips True this + same cycle regardless (gating first discovery, not subdevice success), + so this is the *only* attempt a composite appliance's siblings ever get + without a config-entry reload -- logged at warning for exactly that + reason, not debug. + Empty resources keep _run_discovery from binding anything (hot/warm hrefs stay empty), so _attempt_observe_mode's own session touch never runs either -- this test is purely about the enumeration failure not escaping _async_update_data. """ coordinator = _coordinator(hass) - coordinator._poll_once = dict + monkeypatch.setattr(coordinator, "_poll_once", dict) def _boom(_resources): raise ConnectionError("session closed") - coordinator._enumerate_subdevices_blocking = _boom + monkeypatch.setattr(coordinator, "_enumerate_subdevices_blocking", _boom) - result = await coordinator._async_update_data() + with caplog.at_level("WARNING"): + result = await coordinator._async_update_data() assert coordinator._discovered is True assert result == {} + assert "subdevice enumeration failed" in caplog.text diff --git a/tests/test_services.py b/tests/test_services.py index 3d7f686..cfc0a69 100644 --- a/tests/test_services.py +++ b/tests/test_services.py @@ -297,6 +297,7 @@ async def test_write_resource_verify_after_reports_held(hass, coordinator, devic verified = response["verified"]["/mode/vs/0"] assert verified["held"] is True assert verified["rep"] == {"x.field": "target"} + assert verified["read_error"] is None async def test_write_resource_verify_after_reports_reverted(hass, coordinator, device_id): @@ -355,6 +356,9 @@ async def test_write_resource_verify_after_survives_a_failed_confirmation_read( verified = response["verified"]["/mode/vs/0"] assert verified["held"] is None assert verified["rep"] == {} + # raw_code 0 alone is indistinguishable from a real 4.04 -- read_error + # is what tells a caller this was an unreachable session, not a reply. + assert verified["read_error"] == "session closed" async def test_write_resource_no_verified_key_when_verify_after_is_zero( @@ -656,6 +660,47 @@ async def test_read_resource_failure_is_surfaced_as_a_home_assistant_error( await _call_read(hass, device_id, href="/mode/vs/0") +async def test_read_resource_failure_closes_a_confirmed_dead_session( + hass, coordinator, device_id, monkeypatch +): + """A non-timeout failure is unambiguous (same TimeoutError-vs-anything- + else split as _poll_once) -- leaving a confirmed-dead session installed + would fail every subsequent read/write identically until the next real + poll cycle's own reconnect notices, up to a full update_interval later.""" + coordinator._session = _FakeSession() + + def _boom(path_segs, href): + raise ConnectionError("session closed") + + monkeypatch.setattr(coordinator, "_raw_read_blocking", _boom) + + with pytest.raises(HomeAssistantError): + await _call_read(hass, device_id, href="/mode/vs/0") + + assert coordinator._session is None + + +async def test_read_resource_timeout_does_not_close_the_session( + hass, coordinator, device_id, monkeypatch +): + """The other half of the same distinction: a block-ACK TimeoutError + alone doesn't prove the session is dead (see _poll_once), so unlike + any other failure it must not tear down a session that might still be + perfectly fine.""" + fake = _FakeSession() + coordinator._session = fake + + def _boom(path_segs, href): + raise TimeoutError("GET timeout") + + monkeypatch.setattr(coordinator, "_raw_read_blocking", _boom) + + with pytest.raises(HomeAssistantError): + await _call_read(hass, device_id, href="/mode/vs/0") + + assert coordinator._session is fake + + async def test_read_resource_without_href_returns_cached_snapshot_and_does_not_get( hass, coordinator, device_id ):