fix(coordinator): reset the auth state only on a positive proof

custom_components/googlefindmy/coordinator/locate.py

@@ -329,12 +329,25 @@ async def async_locate_device(self, device_id: str) -> dict[str, Any]:
                     device_id, name
                 )
 
-                # Success path: clear any auth error state
-                self._set_auth_state(failed=False)
-
                 if not location_data:
                     return {}
 
+                # A result WITH content, and only that, proves on this path that
+                # the credentials worked -- the same rule the sound handlers
+                # state at their own sites. An empty return proves only that
+                # nothing raised, and it reaches this method from several
+                # pre-accept failures as well as from a healthy idle tag, so the
+                # guard above runs first. It used not to, and a manual locate on
+                # an idle tag then wiped a pending credential finding raised by
+                # another device.
+                #
+                # The reset sits here and NOT further down, behind the coordinate
+                # check: a record carrying only `last_seen` is an authenticated
+                # server answer. The method returns {} for it, but the account
+                # was accepted, and refusing to count that would be the same
+                # error in the opposite direction.
+                self._set_auth_state(failed=False)
+
                 # Manual locate is upward-only for the reauth budget and never
                 # consumes the poll-only decrypt-proof hint; drop it here (after the
                 # empty guard, so location_data is a non-empty dict) so it cannot

custom_components/googlefindmy/coordinator/polling.py

@@ -1266,7 +1266,28 @@ async def _async_update_data(self) -> list[dict[str, Any]]:
                 self._refresh_canonicless_drop_stats(self._entry_id())
 
                 # Success path: if we were in an auth error state, clear it now.
+                #
+                # This is the strongest proof source in the integration, and
+                # after the poll loop stopped resetting on an empty result it is
+                # also the only everyday one left for the transient counter.
+                # `async_get_basic_device_list` has no non-throwing error exit:
+                # every except branch ends in `raise ConfigEntryAuthFailed` or
+                # `raise UpdateFailed`. A return that got this far therefore
+                # means Nova accepted the account token, which is exactly what
+                # the counter counts -- and an expired login raises before it can
+                # reach this line, so it cannot mask itself here the way it could
+                # through an empty location result.
+                #
+                # Both resets belong in this branch and not above it: the cached
+                # branch skips the call entirely and so proves nothing.
                 self._set_auth_state(failed=False)
+                if self._consecutive_transient_auth_failures > 0:
+                    _LOGGER.info(
+                        "Device list refresh succeeded; clearing %d transient auth failure(s).",
+                        self._consecutive_transient_auth_failures,
+                    )
+                    self._consecutive_transient_auth_failures = 0
+                    self._last_transient_auth_error = None
                 self._set_api_status(ApiStatus.OK)
 
                 # Normalize payloads and filter/dedupe devices using pure helpers
@@ -1788,17 +1809,6 @@ async def _async_start_poll_cycle(
                             timeout=POLL_DEVICE_OUTER_TIMEOUT_S,
                         )
 
-                        # Success path: ensure any previous auth error is cleared
-                        self._set_auth_state(failed=False)
-                        # Reset transient auth failure counter on success
-                        if self._consecutive_transient_auth_failures > 0:
-                            _LOGGER.info(
-                                "Location request succeeded; clearing %d transient auth failure(s).",
-                                self._consecutive_transient_auth_failures,
-                            )
-                            self._consecutive_transient_auth_failures = 0
-                            self._last_transient_auth_error = None
-
                         if not location:
                             # Expected for BLE tags with no reporter nearby: the
                             # inner FCM wait returns an empty result rather than
@@ -1810,6 +1820,39 @@ async def _async_start_poll_cycle(
                             )
                             continue
 
+                        # A location WITH content, and only that, proves on this
+                        # path that the credentials worked -- the same rule the
+                        # sound handlers state at their own sites ("only an
+                        # ACCEPTED submission proves the credentials worked").
+                        # An empty return proves only that nothing raised.
+                        #
+                        # The guard above therefore runs FIRST. It used not to,
+                        # and that was the defect: an empty dict is WEAK evidence
+                        # that the request was accepted, not proof of it. The 5xx,
+                        # the 429, the network error and the failed FCM
+                        # registration raise LocationRequestNotAcceptedError today
+                        # instead of flattening into {}, but four pre-accept
+                        # failures still arrive here as an empty dict (an
+                        # unregistered FCM receiver provider, a provider returning
+                        # None, a missing token cache, and a failure binding the
+                        # lazily imported decrypt / eid-info modules), because they
+                        # are raised before the handler that would convert them.
+                        #
+                        # The cost of getting this wrong is not cosmetic. The
+                        # counter exists to escalate a genuinely expired login
+                        # after _MAX_TRANSIENT_AUTH_FAILURES cycles; a fleet with
+                        # one idle BLE tag cleared it on every single pass, so the
+                        # threshold was never reached and the re-auth prompt this
+                        # mechanism was built for never appeared.
+                        self._set_auth_state(failed=False)
+                        if self._consecutive_transient_auth_failures > 0:
+                            _LOGGER.info(
+                                "Location request succeeded; clearing %d transient auth failure(s).",
+                                self._consecutive_transient_auth_failures,
+                            )
+                            self._consecutive_transient_auth_failures = 0
+                            self._last_transient_auth_error = None
+
                         # A device returned an authenticated coordinate report
                         # without raising a DecryptionError: positive proof the
                         # account-wide shared key still decrypts. Gate the crypto OK

tests/test_coordinator_locate_basics.py

@@ -406,8 +406,14 @@ async def test_manual_locate_client_error_leaves_the_auth_state_alone(
         Flagging it was the original defect. Clearing it is the mirror image and
         just as wrong: a manual locate on a tracker the server rejects proves
         nothing about the credentials, so it must not wipe a pending auth error
-        raised by some other device. Only the second half of this assertion pair
-        depends on api.py passing the error through rather than returning {}.
+        raised by some other device.
+
+        The second half of this pair used to depend on ``api.py`` passing the
+        error through rather than returning ``{}``; it no longer does. The reset
+        now sits behind the empty guard, so an empty return would not clear the
+        state either. Both routes are covered, and
+        ``test_an_empty_manual_locate_does_not_clear_the_auth_state`` is the one
+        that covers the other route on purpose.
         """
         coord.api.async_get_device_location.side_effect = NovaAuthError(404, "gone")
 
@@ -455,6 +461,74 @@ async def test_manual_locate_client_error_does_not_say_authentication(
         }
         assert levels == {logging.WARNING}
 
+    async def test_an_empty_manual_locate_does_not_clear_the_auth_state(
+        self, coord: LocateStub
+    ) -> None:
+        """An empty result proves nothing here either, so it clears nothing.
+
+        The manual path carried the same false-success reasoning as the poll
+        cycle: it cleared the auth state before it had looked at whether the
+        result had any content. A user pressing "locate" on an idle BLE tag
+        therefore wiped a pending credential finding raised by another device,
+        which is precisely the evidence the Repairs issue rests on.
+        """
+        coord.api.async_get_device_location.return_value = {}
+
+        result = await coord.async_locate_device("dev-1")
+
+        assert result == {}
+        assert not [
+            c
+            for c in coord._set_auth_state.call_args_list
+            if c.kwargs.get("failed") is False
+        ]
+
+    async def test_a_successful_manual_locate_still_clears_the_auth_state(
+        self, coord: LocateStub
+    ) -> None:
+        """The positive half survives the move: a real fix still counts as proof."""
+        coord.api.async_get_device_location.return_value = {
+            "latitude": 50.0,
+            "longitude": 10.0,
+            "accuracy": 5.0,
+            "last_seen": 1234567890,
+        }
+
+        await coord.async_locate_device("dev-1")
+
+        assert [
+            c
+            for c in coord._set_auth_state.call_args_list
+            if c.kwargs.get("failed") is False
+        ]
+
+    async def test_a_record_without_coordinates_still_proves_the_credentials(
+        self, coord: LocateStub
+    ) -> None:
+        """The guard against overshooting in the other direction.
+
+        A record carrying only ``last_seen`` is an authenticated server answer:
+        the account was accepted, the row simply has no coordinate report. The
+        method returns ``{}`` for it (see
+        ``test_payload_without_coords_returns_empty``), which makes it tempting
+        to push the reset further down, past the coordinate check. That would
+        throw away a proof the server actually gave us. The reset therefore sits
+        behind the EMPTY guard and in front of the COORDINATE check, and this
+        test is what holds it there.
+        """
+        coord.api.async_get_device_location.return_value = {
+            "last_seen": 1234567890,
+        }
+
+        result = await coord.async_locate_device("dev-1")
+
+        assert result == {}
+        assert [
+            c
+            for c in coord._set_auth_state.call_args_list
+            if c.kwargs.get("failed") is False
+        ]
+
 
 # One row per ``SoundDispatchOutcome`` member: (outcome, accepted, may arm the
 # push cooldown). Kept at module level so the exhaustiveness guard below reads

tests/test_coordinator_semantic_mappings.py

@@ -1002,35 +1002,124 @@ async def test_a_rejected_device_never_clears_the_auth_state() -> None:
 
 
 @pytest.mark.asyncio
-async def test_an_empty_return_still_clears_the_counter() -> None:
-    """Characterisation of the reset this change deliberately leaves alone.
-
-    An empty result USUALLY MEANS an accepted request that came back without a
-    report, and it still counts as success: the counter goes back to zero and
-    the auth state is cleared. The precision matters, because the sentence that
-    used to stand here -- "that is true today for every 5xx and every 429" -- is
-    no longer true. Those raise before they can reach this path, which is
-    exactly what the change did; the pair to this test is
-    ``test_an_unaccepted_request_no_longer_clears_the_counter``. "Usually" and
-    not "always", because four pre-accept faults still arrive as an empty dict;
-    they are enumerated at the post-loop guard in ``polling.py``.
-
-    What remains wrong on its own terms is the reset itself: an accepted request
-    that returned nothing proves only that nothing raised, not that the
-    credentials work. That is tracked as a finding of its own with its own
-    approval gate (`PLAN_GFMY_AUTH_RESET_POSITIVE_PROOF`). This test is here so
-    the day it changes, it changes on purpose.
+async def test_an_empty_return_proves_nothing_about_the_credentials() -> None:
+    """The reset moved behind the empty guard; this is the same test, inverted.
+
+    History, kept deliberately: this function used to be called
+    ``test_an_empty_return_still_clears_the_counter`` and asserted the opposite
+    of what it asserts now. It was a characterisation of a reset that was known
+    to be wrong on its own terms and was left alone at the time, with the
+    finding tracked separately. This is the day it changes, and it changes on
+    purpose.
+
+    What is wrong with the old behaviour: an empty dict is WEAK evidence that
+    the request was accepted, not proof of it. The 5xx, the 429, the network
+    error and the failed FCM registration now raise
+    ``LocationRequestNotAcceptedError`` instead of flattening into ``{}``, but
+    four pre-accept failures still arrive here as an empty dict (enumerated in
+    ``test_a_cycle_of_only_empty_results_still_reports_success``). Clearing the
+    auth state and the transient counter on that evidence is the false-success
+    reasoning this change removes: the counter exists to escalate a genuinely
+    expired login after three cycles, and a fleet with one idle BLE tag reset
+    it in every single cycle, so the threshold was never reached.
+
+    The positive half is not given up, it moves: a location WITH content still
+    clears both, which is ``test_a_real_location_still_clears_the_auth_state``
+    and ``test_a_real_location_still_resets_the_transient_counter``.
     """
     coordinator = _polling_coordinator({}, _TrackingFilter(), {})
     coordinator.api = _PerDeviceAPI({"dev-1": {}})
     auth_calls: list[dict[str, Any]] = []
     coordinator._set_auth_state = lambda **kwargs: auth_calls.append(kwargs)
     coordinator._consecutive_transient_auth_failures = 2
+    coordinator._last_transient_auth_error = "expired"
+
+    await coordinator._async_start_poll_cycle([{"id": "dev-1", "name": "Hub"}])
+
+    assert not [kw for kw in auth_calls if kw.get("failed") is False]
+    assert coordinator._consecutive_transient_auth_failures == 2
+    assert coordinator._last_transient_auth_error == "expired"
+
+
+@pytest.mark.asyncio
+async def test_a_real_location_still_clears_the_auth_state() -> None:
+    """The positive half of the rule must survive the fix.
+
+    Moving the reset behind the empty guard is only correct if a location WITH
+    content still counts as proof. Without this test the fix could be "improved"
+    into removing the reset altogether, which would strand a pending auth error
+    until the next device-list refresh.
+    """
+    coordinator = _polling_coordinator({}, _TrackingFilter(), {})
+    coordinator.api = _PerDeviceAPI(
+        {
+            "dev-1": {
+                "latitude": 50.0,
+                "longitude": 10.0,
+                "accuracy": 5.0,
+                "last_seen": 100.0,
+            }
+        }
+    )
+    auth_calls: list[dict[str, Any]] = []
+    coordinator._set_auth_state = lambda **kwargs: auth_calls.append(kwargs)
 
     await coordinator._async_start_poll_cycle([{"id": "dev-1", "name": "Hub"}])
 
     assert [kw for kw in auth_calls if kw.get("failed") is False]
+
+
+@pytest.mark.asyncio
+async def test_a_real_location_still_resets_the_transient_counter() -> None:
+    """Same as above for the counter and the stored cause.
+
+    Separate from the auth-state test on purpose: the two live in one block
+    today, and a fix that moves only one of them would otherwise pass.
+    """
+    coordinator = _polling_coordinator({}, _TrackingFilter(), {})
+    coordinator.api = _PerDeviceAPI(
+        {
+            "dev-1": {
+                "latitude": 50.0,
+                "longitude": 10.0,
+                "accuracy": 5.0,
+                "last_seen": 100.0,
+            }
+        }
+    )
+    coordinator._set_auth_state = lambda **kwargs: None
+    coordinator._consecutive_transient_auth_failures = 2
+    coordinator._last_transient_auth_error = "expired"
+
+    await coordinator._async_start_poll_cycle([{"id": "dev-1", "name": "Hub"}])
+
     assert coordinator._consecutive_transient_auth_failures == 0
+    assert coordinator._last_transient_auth_error is None
+
+
+@pytest.mark.asyncio
+async def test_an_empty_return_does_not_clear_a_pending_auth_error() -> None:
+    """The cross-device case, which is why the order of the guard matters.
+
+    One tracker raises a credential failure, a second one comes back empty in
+    the same cycle. With the reset in front of the empty guard, the second
+    device wiped the first device's finding on every pass -- and in a fleet with
+    one idle BLE tag that is every pass, forever. The devices are ordered so the
+    empty one is polled last, which is the order that used to lose the finding.
+    """
+    coordinator = _polling_coordinator({}, _TrackingFilter(), {})
+    coordinator.api = _PerDeviceAPI(
+        {"dev-1": NovaAuthError(401, "expired"), "dev-2": {}}
+    )
+    auth_calls: list[dict[str, Any]] = []
+    coordinator._set_auth_state = lambda **kwargs: auth_calls.append(kwargs)
+    coordinator.config_entry.async_start_reauth = MagicMock()
+
+    await coordinator._async_start_poll_cycle(
+        [{"id": "dev-1", "name": "Hub"}, {"id": "dev-2", "name": "Tag"}]
+    )
+
+    assert not [kw for kw in auth_calls if kw.get("failed") is False]
 
 
 @pytest.mark.asyncio
@@ -1741,15 +1830,17 @@ async def test_an_unaccepted_device_does_not_touch_the_transient_auth_counter()
 
 @pytest.mark.asyncio
 async def test_an_unaccepted_request_no_longer_clears_the_counter() -> None:
-    """The contract pair to ``test_an_empty_return_still_clears_the_counter``.
+    """The contract pair to
+    ``test_an_empty_return_proves_nothing_about_the_credentials``.
 
     The two are deliberately adjacent claims about the same success path, read
-    from opposite sides. An ACCEPTED request that came back empty still clears
-    the counter -- that reset is wrong on its own terms and is tracked
-    separately (`PLAN_GFMY_AUTH_RESET_POSITIVE_PROOF`), so it is characterised,
-    not changed here. A request that was never accepted no longer reaches that
-    path at all. Splitting them is what makes the difference between the two
-    outcomes checkable instead of a matter of reading the branch.
+    from opposite sides. A request that was never accepted does not reach that
+    path at all; a request that WAS accepted but came back empty reaches it and
+    no longer clears anything either, because an empty dict is weak evidence of
+    acceptance rather than proof of working credentials. The two mechanisms are
+    different and stay separate: one is a raise before the success path, the
+    other is a guard inside it. Splitting them is what makes the difference
+    between the outcomes checkable instead of a matter of reading the branch.
     """
     coordinator = _polling_coordinator({}, _TrackingFilter(), {})
     coordinator.api = _PerDeviceAPI(