review: re-arm the settle window on retry, tighten comments, close a test gap
An Opus review of the three prior commits on this branch (PR #306) turned up two real defects and a documentation/test gap, all fixed here: - async_send_command's retry (issue #294) armed the write-settle window before the retry existed, so the reconnect pause plus a second PUT could eat into the time meant for the confirming poll, reviving the revert-then-reapply symptom the window was sized to prevent (issue #9). Re-arm it after a successful retry lands. - test_send_command_reconnects_and_retries_after_socket_closed relied on the observe-session fixture's no-op _close_session, so self._session never actually went None and _do_put's reconnect guard was never exercised -- the test passed even with that guard deleted. Now overrides _close_session/_connect_session to actually drop and rebuild the session, and asserts the reconnect happened. - async_send_command's docstring still said "Fire-and-forget", which stopped being true the moment it started retrying and raising. Also trimmed the three comment blocks the review flagged as reproducing their commit messages verbatim, per CONTRIBUTING.md's comment-style rules. One review finding is not addressed here and needs a decision: the new _close_session() call in the command-retry path isn't synchronized against _attempt_observe_mode, which touches the session without _session_lock. A write's reconnect can race an in-flight observe-mode subscribe attempt and tear down the session it's using. Fixing it properly means broadening lock scope around observe-mode entry, which risks blocking a write behind an up to ~15s subscribe grace period -- a tradeoff not made unilaterally here. A second finding (dropping the old .strip()'s per-line whitespace handling in _normalize_pem) did not reproduce against a real certificate/key, only against the test suite's placeholder PEM body, so it's left as-is.
This commit is contained in:
Marc Billow
@@ -164,19 +164,10 @@ def _fetch_samsung_uuid() -> str:
|
||||
|
||||
|
||||
def _normalize_pem(text: str) -> str:
|
||||
"""Clean up a pasted PEM blob before handing it to `cryptography`'s
|
||||
parser (issue #291).
|
||||
|
||||
A PEM copied out of a text editor can carry a few bytes `cryptography`
|
||||
refuses outright: a UTF-8 BOM some Windows editors silently prepend, CR
|
||||
line endings (or CRLF), and blank lines a paste can introduce between
|
||||
the header/body/footer. Any of those surfaces as an opaque
|
||||
`InvalidHeader` with no hint of what's actually wrong -- which is why
|
||||
the same certificate pasted from `type` (no BOM, no stray blank lines)
|
||||
loads fine while the editor's copy doesn't. None of the stripped bytes
|
||||
are meaningful in PEM: the format is BOM-free, line-ending-agnostic, and
|
||||
has no blank-line syntax of its own.
|
||||
"""
|
||||
"""Strip a pasted PEM's BOM, CRLF endings, and blank lines before
|
||||
`cryptography` sees it -- a text editor's copy carries all three and
|
||||
fails with an opaque InvalidHeader, while the same file dumped via
|
||||
`type` doesn't (issue #291)."""
|
||||
text = text.lstrip("\ufeff")
|
||||
text = text.replace("\r\n", "\n").replace("\r", "\n")
|
||||
lines = [line for line in text.split("\n") if line.strip()]
|
||||
@@ -742,12 +733,8 @@ class LocalThingsConfigFlow(config_entries.ConfigFlow, domain=DOMAIN):
|
||||
if leaf_cert and leaf_key:
|
||||
existing_leaf = (leaf_cert, leaf_key)
|
||||
else:
|
||||
# Normalized before the probe even runs (issue #291), not
|
||||
# just before minting: the same pasted blob is also what
|
||||
# gets stored in the config entry and reused to re-mint the
|
||||
# leaf on a future reconfigure, so a raw copy with a BOM or
|
||||
# CRLF line endings would keep failing every time it's read
|
||||
# back, not just this once.
|
||||
# Normalized here, not just before minting: this is also
|
||||
# what gets stored and reused to re-mint the leaf later.
|
||||
self._ca_cert_pem = _normalize_pem(user_input[CONF_CA_CERT_PEM])
|
||||
self._ca_key_pem = _normalize_pem(user_input[CONF_CA_KEY_PEM])
|
||||
|
||||
|
||||
@@ -834,18 +834,11 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]):
|
||||
resources = await self.hass.async_add_executor_job(self._poll_once)
|
||||
except Exception as e2:
|
||||
self._log.error("poll failed after reconnect: %s", e2)
|
||||
# The reconnect itself proved the old session is gone, so
|
||||
# any OBSERVE subscription on it is too -- this cycle has
|
||||
# no live session to resubscribe on, unlike the success
|
||||
# branch below, so downgrade without setting
|
||||
# just_downgraded_from_observe (that would attempt a
|
||||
# resubscribe this same cycle against a session we just
|
||||
# confirmed is unreachable). Without this, a device that
|
||||
# drops off the network entirely leaves the connection-mode
|
||||
# sensor reporting "Push" forever, since only the success
|
||||
# path below ever changed it (issue #287) -- the poll-mode
|
||||
# retry timer (_maybe_retry_observe_mode) still re-attempts
|
||||
# observe once the device is actually reachable again.
|
||||
# Without this, a fully unreachable device left the
|
||||
# connection-mode sensor stuck on "Push" forever -- only
|
||||
# the success branch below ever downgraded it (issue
|
||||
# #287). No just_downgraded_from_observe here: there's no
|
||||
# live session this cycle to resubscribe on.
|
||||
if self._observe.mode == MODE_OBSERVE:
|
||||
self._observe.downgrade_to_poll()
|
||||
snapshot = self._cache.snapshot()
|
||||
@@ -924,7 +917,8 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]):
|
||||
# ------------------------------------------------------------------
|
||||
|
||||
async def async_send_command(self, bound_entity: BoundEntity, payload: Any) -> None:
|
||||
"""Write a value to the device. Fire-and-forget.
|
||||
"""Write a value to the device. Retries once on a dead session
|
||||
(issue #294); raises HomeAssistantError if that retry fails too.
|
||||
|
||||
A description-level validate_fn (SwitchDesc only, currently) rejects
|
||||
a write with a user-facing message ahead of write_fn's silent
|
||||
@@ -1034,17 +1028,9 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]):
|
||||
code, _ = sess.post(path_segs, cbor2.dumps(body), timeout=self._POST_TIMEOUT_S)
|
||||
self._log.info("PUT %s → code %#04x", write_href, code)
|
||||
|
||||
# A session Samsung's firmware closed between polls (the same "known
|
||||
# device behavior" _async_update_data reconnects around) previously
|
||||
# sank the PUT here with nothing but a log line -- no retry, no
|
||||
# user-facing error, so the command was silently lost until the next
|
||||
# manual reload (issue #294). Mirror the poll path's own recovery:
|
||||
# one reconnect, one retry, and this time raise on final failure so
|
||||
# the user gets a red toast instead of nothing.
|
||||
#
|
||||
# Locked the same as the poll path so a write landing mid-reconnect
|
||||
# doesn't tear down a session _async_update_data is simultaneously
|
||||
# rebuilding (or vice versa).
|
||||
# Mirrors the poll path's reconnect-and-retry (issue #294): a PUT
|
||||
# landing on a session Samsung's firmware closed between polls used
|
||||
# to be silently lost -- no retry, no user-facing error.
|
||||
async with self._session_lock:
|
||||
try:
|
||||
await self.hass.async_add_executor_job(_do_put)
|
||||
@@ -1061,6 +1047,15 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]):
|
||||
translation_key="command_failed",
|
||||
translation_placeholders={"href": write_href, "error": str(e2)},
|
||||
) from e2
|
||||
# The retry's own reconnect pause + second PUT can eat well
|
||||
# into the settle window armed above, leaving too little of
|
||||
# it for the confirming poll below and reviving the
|
||||
# revert-then-reapply symptom settle_s exists to prevent
|
||||
# (issue #9). Re-arm it fresh now that the write actually
|
||||
# landed.
|
||||
self._observe.mark_write_pending(
|
||||
write_href, settle_s=self._POST_TIMEOUT_S + self._POLL_TIMEOUT_S
|
||||
)
|
||||
await self.async_request_refresh()
|
||||
|
||||
# ------------------------------------------------------------------
|
||||
|
||||
@@ -1015,7 +1015,14 @@ async def test_send_command_reconnects_and_retries_after_socket_closed(
|
||||
"""A command lost to a session Samsung's firmware closed between polls
|
||||
must not just vanish (issue #294): `_do_put` failing once is now
|
||||
followed by a reconnect and a single retry, mirroring the poll path's
|
||||
own recovery in `_async_update_data`."""
|
||||
own recovery in `_async_update_data`.
|
||||
|
||||
`mock_coordinator_observe_session` patches `_close_session` to a no-op,
|
||||
which would leave `self._session` never actually going `None` -- and
|
||||
with it, `_do_put`'s own `if self._session is None: self._connect_session()`
|
||||
guard never exercised, so a broken reconnect could still pass. Overridden
|
||||
here to actually drop the session, so the retry only succeeds if that
|
||||
guard really rebuilds it."""
|
||||
from custom_components.localthings.registry.discovery import BoundEntity
|
||||
from custom_components.localthings.registry.entities import NumberDesc
|
||||
|
||||
@@ -1038,8 +1045,19 @@ async def test_send_command_reconnects_and_retries_after_socket_closed(
|
||||
raise ConnectionError("socket closed")
|
||||
return (0x44, b"")
|
||||
|
||||
def _drop_session():
|
||||
coordinator._session = None
|
||||
|
||||
reconnects = {"n": 0}
|
||||
|
||||
def _reconnect():
|
||||
reconnects["n"] += 1
|
||||
coordinator._session = fake
|
||||
|
||||
with (
|
||||
patch.object(fake, "subscribe"),
|
||||
patch.object(coordinator, "_close_session", side_effect=_drop_session),
|
||||
patch.object(coordinator, "_connect_session", side_effect=_reconnect),
|
||||
patch(
|
||||
"custom_components.localthings.coordinator.asyncio.sleep",
|
||||
new=AsyncMock(),
|
||||
@@ -1048,6 +1066,8 @@ async def test_send_command_reconnects_and_retries_after_socket_closed(
|
||||
fake.post = _post
|
||||
await coordinator.async_send_command(bound, 5)
|
||||
|
||||
assert reconnects["n"] == 1
|
||||
|
||||
assert calls["n"] == 2
|
||||
assert coordinator._cache.get("/test/vs/0") == {"value": 5}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user