fix(coordinator): two gaps a follow-up Opus review found in the split
A second review of the observe-mode phase split (previous commit) found two real regressions it introduced, both in the same failure family it was built to close: - async_send_command's failed-retry branch closed the session, then raised without downgrading observe mode -- the downgrade only ran on the retry's success path. A retry that also fails still leaves the session dead, so mode was left claiming "Push" on a session that no longer exists, same as the bug this whole fix targets. Moved the downgrade to run right after the close, unconditionally on how the retry goes. - _attempt_observe_mode's stale-session abandon (the identity-check branch added in the previous commit) didn't flag a resubscribe. A session swap discovered there means a fresh, never-tried session now exists, but _last_observe_attempt_ts was already stamped for the now-abandoned attempt -- so that new session sat unsubscribed for up to _RECOVERY_RETRY_S (600s) instead of being retried on the next cycle. Now sets _resubscribe_due, same as the two reconnect paths do. Also closes two test-coverage gaps the same review surfaced by mutation testing: no test asserted the lock actually holds during the subscribe burst (only that it's released for the wait), and no test distinguished the max() in _maybe_retry_observe_mode's throttle from using _last_observe_attempt_ts alone -- both mutations left the full suite green. Added one test for each, plus extended two existing tests for the bug fixes above; all four confirmed via mutation testing (revert the fix, watch the new/extended test fail; restore it, watch it pass). One finding from the same review is intentionally left open: async_close is the one self._session writer that doesn't take _session_lock, so a close racing _attempt_observe_mode isn't covered by today's identity check. This is pre-existing (the lock didn't cover any of _attempt_observe_mode before this branch's earlier commits either), not a regression from this branch's work, and is a shutdown/unload-path question rather than the write-vs-observe-mode race this branch set out to fix.
This commit is contained in:
2 files changed
+95
-10
No files matched your search
@@ -776,8 +776,18 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]):
|
||||
self._observe.await_observe_notifies, subscribed, self._OBSERVE_GRACE_PERIOD_S
|
||||
)
|
||||
async with self._session_lock:
|
||||
if not reached or self._session is not sess:
|
||||
stale_session = self._session is not sess
|
||||
if not reached or stale_session:
|
||||
self._observe.abandon_observe_attempt()
|
||||
if stale_session:
|
||||
# A reconnect elsewhere replaced the session while this
|
||||
# attempt waited -- that session has never been tried,
|
||||
# so retry it next cycle instead of leaving it
|
||||
# unsubscribed for up to _RECOVERY_RETRY_S, which
|
||||
# _last_observe_attempt_ts (already stamped above, for
|
||||
# the now-abandoned session) would otherwise throttle
|
||||
# for (issue #294).
|
||||
self._resubscribe_due = True
|
||||
return
|
||||
self._observe.enter_observe_mode(sess, subscribed)
|
||||
|
||||
@@ -1078,6 +1088,15 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]):
|
||||
except Exception as e:
|
||||
self._log.warning("command failed for %s, reconnecting: %s", write_href, e)
|
||||
await self.hass.async_add_executor_job(self._close_session)
|
||||
# The session is dead the moment it's closed, so any OBSERVE
|
||||
# subscriptions on it are too -- downgrade here, before the
|
||||
# retry, so a retry that also fails doesn't leave mode
|
||||
# claiming "Push" on a session that no longer exists
|
||||
# (issue #294; the poll path handles the same fact for its
|
||||
# own reconnect the same way, unconditionally on close).
|
||||
if self._observe.mode == MODE_OBSERVE:
|
||||
self._observe.downgrade_to_poll()
|
||||
self._resubscribe_due = True
|
||||
await asyncio.sleep(self._RECONNECT_PAUSE_S)
|
||||
try:
|
||||
await self.hass.async_add_executor_job(_do_put)
|
||||
@@ -1097,14 +1116,6 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]):
|
||||
self._observe.mark_write_pending(
|
||||
write_href, settle_s=self._POST_TIMEOUT_S + self._POLL_TIMEOUT_S
|
||||
)
|
||||
# A reconnect hands back a session with zero OBSERVE
|
||||
# registrations, same as the poll path's own reconnect --
|
||||
# keeping observe mode here would leave those subscriptions
|
||||
# claimed on the closed session with no poll failure left
|
||||
# to notice (issue #294).
|
||||
if self._observe.mode == MODE_OBSERVE:
|
||||
self._observe.downgrade_to_poll()
|
||||
self._resubscribe_due = True
|
||||
await self.async_request_refresh()
|
||||
|
||||
# ------------------------------------------------------------------
|
||||
|
||||
Reference in new issue
Block a user