Fix issues found by an Opus review of the smartthings-local upgrade
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.
This commit is contained in:
5 files changed
+188
-13
No files matched your search
@@ -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)
|
||||
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
Reference in new issue
Block a user