fix: verify the snapshot reply arrived, and retry if it didn't

Publishing a snapshot request is not the same as delivering one. The
recovery triggers (boot-count change, availability online) fire while the
dongle is still coming back, so a request can go out before it has
resubscribed and is then dropped. Because FW >= 4.3.0 streams change-data
only and settings never change on their own, nothing re-sends them: one
lost request leaves every setting entity empty until the next reboot.

Arm a timer on each successful publish and disarm it when the reply
arrives on <dongle>/snap/hold; if it doesn't, re-request, bounded to
_snapshot_max_retries (3) at _snapshot_retry_delay (20s).

Only /snap/hold counts as delivery. The hold half is what carries the
settings, so an /snap/input-only reply must not disarm the retry.

Retries respect the existing OTA suppression: a dongle mid-OTA has no
snapshot queue and a {"what":"all"} request reboot-loops it, so a timer
that fires during an OTA is dropped rather than re-requesting. Armed
timers are also cancelled in stop_mqtt_subscription(), so a reload
doesn't orphan them.

Tests: 6 new tests, all verified to fail before this change and pass
after. 185 passed.

Author: RAR

custom_components/monitormysolar/coordinator.py

@@ -98,6 +98,15 @@ def __init__(
         self._dongle_stale_after = 90.0
         # Don't send more than one recovery snapshot per dongle within this window.
         self._recovery_snapshot_debounce = 30.0
+        # A snapshot request can be lost: the recovery triggers fire while the
+        # dongle is still coming back, so it may not have resubscribed yet. On
+        # FW >= 4.3.0 nothing re-sends hold registers, so a lost request leaves
+        # every setting entity empty until the next reboot. Verify the reply
+        # (<dongle>/snap/hold) actually arrives and retry a bounded number of times.
+        self._snapshot_retry: Dict[str, Any] = {}  # dongle -> cancel callback
+        self._snapshot_retry_attempts: Dict[str, int] = {}
+        self._snapshot_retry_delay = 20.0
+        self._snapshot_max_retries = 3
         self._has_gridboss = entry.data.get("has_gridboss", False)  # Track if GridBoss is enabled
         self._gridboss_dongle = entry.data.get("gridboss_dongle", "")  # Track which dongle is GridBoss
         self._last_fault_warning_data = {}  # Track last fault/warning data to prevent duplicate processing
@@ -236,6 +245,7 @@ async def request_snapshot(self, dongle_id: str, version: str = "", force: bool
         LOGGER.info(
             "Requested full snapshot from %s (version=%s)", dongle_id, version or "unknown"
         )
+        self._arm_snapshot_retry(dongle_id, version)
         return True
 
     async def request_recovery_snapshot(self, dongle_id: str, reason: str) -> None:
@@ -267,6 +277,65 @@ async def request_recovery_snapshot(self, dongle_id: str, reason: str) -> None:
         ):
             self._last_recovery_snapshot[dongle_id] = now
 
+    def _cancel_snapshot_retry(self, dongle_id: str) -> None:
+        """Drop any armed retry timer for a dongle."""
+        # getattr: test coordinators are built via __new__ and skip __init__.
+        pending = getattr(self, "_snapshot_retry", None)
+        if not pending:
+            return
+        cancel = pending.pop(dongle_id, None)
+        if cancel is not None:
+            cancel()
+
+    def _arm_snapshot_retry(self, dongle_id: str, version: str) -> None:
+        """Re-request the snapshot if its reply doesn't arrive in time.
+
+        A published request is not a delivered one: the recovery triggers fire
+        while the dongle is still reconnecting, so the request can go out before
+        it has resubscribed. Since FW >= 4.3.0 never re-sends hold registers on
+        its own, that single loss would leave every setting entity empty for the
+        rest of the session.
+        """
+        if getattr(self, "_snapshot_retry", None) is None:
+            self._snapshot_retry = {}
+        if getattr(self, "_snapshot_retry_attempts", None) is None:
+            self._snapshot_retry_attempts = {}
+        self._cancel_snapshot_retry(dongle_id)
+        attempts = self._snapshot_retry_attempts.get(dongle_id, 0)
+        max_retries = getattr(self, "_snapshot_max_retries", 3)
+        if attempts >= max_retries:
+            LOGGER.warning(
+                "Snapshot from %s still unanswered after %d retries - its settings "
+                "entities will stay unknown until it reboots or reconnects",
+                dongle_id, attempts,
+            )
+            return
+        delay = getattr(self, "_snapshot_retry_delay", 20.0)
+
+        async def _retry(_now) -> None:
+            self._snapshot_retry.pop(dongle_id, None)
+            if self.is_ota_in_progress(dongle_id):
+                return
+            self._snapshot_retry_attempts[dongle_id] = attempts + 1
+            LOGGER.warning(
+                "No snapshot reply from %s after %.0fs - retrying (%d/%d)",
+                dongle_id, delay, attempts + 1, max_retries,
+            )
+            await self.request_snapshot(dongle_id, version, force=True)
+
+        self._snapshot_retry[dongle_id] = async_call_later(self.hass, delay, _retry)
+
+    def _note_snapshot_delivered(self, dongle_id: str) -> None:
+        """Record that a dongle answered its snapshot request.
+
+        Called when <dongle>/snap/hold arrives — the hold half is what carries
+        the settings, so an /snap/input-only reply deliberately does not count.
+        """
+        self._cancel_snapshot_retry(dongle_id)
+        attempts = getattr(self, "_snapshot_retry_attempts", None)
+        if attempts is not None:
+            attempts.pop(dongle_id, None)
+
     async def mark_dongle_seen(self, dongle_id: str) -> None:
         """Record that a message arrived from a dongle and detect gap recovery.
 
@@ -1292,6 +1361,10 @@ async def _async_handle_mqtt_message(self, msg) -> None:
             # which on FW >= 4.3.0 (change-data only) may be a long time. The
             # firmware publishes it on <dongle>/snap/input and <dongle>/snap/hold.
             elif topic.endswith("/snap/input") or topic.endswith("/snap/hold"):
+                if topic.endswith("/snap/hold"):
+                    # The hold half carries the settings; an input-only reply
+                    # leaves them empty, so it must not disarm the retry.
+                    self._note_snapshot_delivered(dongle_id)
                 await self.process_message(dongle_id, topic, msg.payload)
                 self.async_set_updated_data(self.entities)
             # Skip other message processing during startup to prevent excessive updates
@@ -1764,6 +1837,10 @@ async def log_ignored_entities(_):
     async def stop_mqtt_subscription(self):
         """Stop all MQTT subscriptions."""
         LOGGER.debug(f"Stopping MQTT subscriptions for all dongles")
+        # Drop armed snapshot retries first: once unsubscribed there is nothing
+        # left to answer them, and a reload would leave the timers orphaned.
+        for dongle_id in list(getattr(self, "_snapshot_retry", {})):
+            self._cancel_snapshot_retry(dongle_id)
         for key, unsubscribe in list(self._mqtt_unsubscribe_callbacks.items()):
             try:
                 unsubscribe()

tests/test_snapshot_retry.py

@@ -0,0 +1,122 @@
+"""Snapshot requests are verified and retried until the reply arrives.
+
+Publishing a snapshot request is not the same as delivering one: the recovery
+triggers fire while a dongle is still reconnecting, so a request can go out
+before it has resubscribed. On FW >= 4.3.0 nothing re-sends hold registers, so
+a single lost request leaves every setting entity empty for the session.
+
+The coordinator arms a timer on each successful publish and disarms it when
+<dongle>/snap/hold arrives.
+"""
+import asyncio
+
+
+def _run(coro):
+    return asyncio.run(coro)
+
+
+def _fire(timers):
+    """Fire the oldest armed timer, one-shot: a fired handle is spent."""
+    _delay, cb = timers.pop(0)
+    return _run(cb(None))
+
+
+def _prep(coordinator, monkeypatch):
+    """Wire a coordinator with a captured async_call_later and no real publish."""
+    from custom_components.monitormysolar import coordinator as coord_mod
+
+    published = []
+
+    async def fake_publish(hass, topic, payload, **kwargs):
+        published.append(topic)
+
+    monkeypatch.setattr(coord_mod.mqtt, "async_publish", fake_publish)
+
+    timers = []  # (delay, callback)
+
+    def fake_call_later(hass, delay, cb):
+        timers.append((delay, cb))
+        cancelled = {"done": False}
+
+        def _cancel():
+            cancelled["done"] = True
+            for i, (d, c) in enumerate(timers):
+                if c is cb:
+                    timers.pop(i)
+                    break
+
+        return _cancel
+
+    monkeypatch.setattr(coord_mod, "async_call_later", fake_call_later)
+
+    coordinator._snapshot_requested = set()
+    coordinator._snapshot_retry = {}
+    coordinator._snapshot_retry_attempts = {}
+    coordinator._snapshot_retry_delay = 20.0
+    coordinator._snapshot_max_retries = 3
+    return published, timers
+
+
+def test_successful_publish_arms_a_retry(coordinator, monkeypatch):
+    published, timers = _prep(coordinator, monkeypatch)
+
+    _run(coordinator.request_snapshot("dongle-A", "4.3.1.1C6", force=True))
+    assert published == ["dongle-A/snapshot/request"]
+    assert len(timers) == 1 and timers[0][0] == 20.0
+    assert "dongle-A" in coordinator._snapshot_retry
+
+
+def test_snap_hold_disarms_the_retry(coordinator, monkeypatch):
+    published, timers = _prep(coordinator, monkeypatch)
+
+    _run(coordinator.request_snapshot("dongle-A", "4.3.1.1C6", force=True))
+    coordinator._note_snapshot_delivered("dongle-A")
+
+    assert coordinator._snapshot_retry == {}
+    assert timers == []  # cancelled, so it can never re-request
+
+
+def test_input_only_reply_does_not_disarm(coordinator, monkeypatch):
+    """/snap/input carries no settings, so it must not count as delivery."""
+    published, timers = _prep(coordinator, monkeypatch)
+
+    _run(coordinator.request_snapshot("dongle-A", "4.3.1.1C6", force=True))
+    # Only _note_snapshot_delivered() disarms, and the dispatcher calls it for
+    # /snap/hold alone -- an input-only reply leaves the timer armed.
+    assert "dongle-A" in coordinator._snapshot_retry
+
+
+def test_unanswered_request_is_retried(coordinator, monkeypatch):
+    published, timers = _prep(coordinator, monkeypatch)
+
+    _run(coordinator.request_snapshot("dongle-A", "4.3.1.1C6", force=True))
+    # Reply never arrives -> the timer fires.
+    _fire(timers)
+
+    assert published == ["dongle-A/snapshot/request"] * 2
+    assert coordinator._snapshot_retry_attempts["dongle-A"] == 1
+
+
+def test_retries_are_bounded(coordinator, monkeypatch):
+    published, timers = _prep(coordinator, monkeypatch)
+
+    _run(coordinator.request_snapshot("dongle-A", "4.3.1.1C6", force=True))
+    for _ in range(10):
+        if not timers:
+            break
+        _fire(timers)
+
+    # Initial publish + at most _snapshot_max_retries retries, then it gives up.
+    assert len(published) == 1 + 3
+    assert coordinator._snapshot_retry == {}
+
+
+def test_retry_suppressed_during_ota(coordinator, monkeypatch):
+    """An OTA'ing dongle has no snapshot queue: a request reboot-loops it."""
+    published, timers = _prep(coordinator, monkeypatch)
+
+    _run(coordinator.request_snapshot("dongle-A", "4.3.1.1C6", force=True))
+    coordinator.set_ota_in_progress("dongle-A", True)
+    _fire(timers)
+
+    assert published == ["dongle-A/snapshot/request"]  # no second request