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

custom_components/googlefindmy/AGENTS.md

@@ -122,15 +122,17 @@ the predicate and not the type; `coordinator/polling.py`, whose `except NovaAuth
 transient-auth counter alone in both directions AND does not record the rejection as the cycle's `last_exception`;
 and `coordinator/locate.py`, whose branch does not flip the account-wide auth state. `location_request.py` names the status in its log records instead of calling every 4xx an
 authentication error, but still only re-raises -- it does not classify.
-Why the location handler raises rather than returning `{}`, since the empty return looks like the gentler option: both
-callers treat ANY non-raising return as positive proof that the credentials work. `polling.py` runs "Success path:
-ensure any previous auth error is cleared" and `locate.py` runs "Success path: clear any auth error state", and both run
-BEFORE the `if not location` guard. An empty return for a rejected device would therefore have RESET the transient-auth
-counter and cleared the auth state, which is the opposite of what those branches say. Worse, it would have done so
-PERMANENTLY: a 5xx clears up, a device deleted from the account does not, so one deleted tracker in the list would reset
-the counter every cycle and a genuinely expired sign-in on another tracker would never reach
-`_MAX_TRANSIENT_AUTH_FAILURES`. The user would never see the reauth prompt at all -- a worse outcome than the defect
-this change exists to fix. Raising keeps a rejected device out of that reset.
+Why the location handler raises rather than returning `{}`, since the empty return looks like the gentler option. The
+original reason no longer holds and is kept here as history: both callers used to treat ANY non-raising return as
+positive proof that the credentials work, running their "Success path" reset BEFORE the `if not location` guard, so an
+empty return for a rejected device would have RESET the transient-auth counter and cleared the auth state -- and
+permanently, because a 5xx clears up while a device deleted from the account does not. That reset now sits BEHIND the
+empty guard on both paths (`PLAN_GFMY_AUTH_RESET_POSITIVE_PROOF`), so raising is no longer the thing that keeps a
+rejection out of it. Raising is still right, for the reason that outlives the defect: a client rejection is a failure
+and belongs in failure handling. An empty dict would flatten it into the shape of a healthy idle tag -- no
+`cycle_failed`, no `failed` in `last_poll_result`, nothing for the post-loop guard to count -- and no caller can tell
+those two apart from the outside, which is why the outcome has to cross the boundary as an exception and not as an
+empty collection.
 Why the poll branch records the rejection without failing the coordinator UPDATE (it does fail the cycle): in the
 cycle's `finally` block `cycle_failed` and `last_exception` drive two different things. `cycle_failed` only writes
 the `last_poll_result` diagnostic attribute that `binary_sensor.py` exposes; `last_exception` drives
@@ -246,14 +248,63 @@ Eight tests pin this: `test_a_rejected_device_does_not_make_every_tracker_unavai
 `test_a_mixed_cycle_of_rejection_and_empty_siblings_stays_silent`,
 `test_a_cycle_where_no_request_was_accepted_reports_an_error` and
 `test_a_mixed_cycle_of_rejection_and_unaccepted_siblings_now_surfaces`.
-What is still NOT fixed there, stated so it is not mistaken for solved: that success path still treats every empty
-result as proof of working credentials. The 5xx and the 429 no longer reach it -- they raise before they can -- but
-every idle BLE tag still does, and so do the four pre-accept failures named above. Deciding what an
-empty result may prove about credentials is a behaviour change of its own with a far wider blast radius (every healthy
-idle poll takes the same path); it is tracked separately (`PLAN_GFMY_AUTH_RESET_POSITIVE_PROOF`) and must not be assumed done. Three tests pin the
-current state so it cannot drift silently: `test_an_empty_return_still_clears_the_counter` characterises the reset that
-stays, `test_an_unaccepted_request_no_longer_clears_the_counter` is its contract pair for the requests that no longer
-reach it, and `test_a_non_credential_4xx_location_is_passed_through` pins the seam that keeps a rejection out of it.
+What that success path does NOW, because this paragraph used to say it was still broken and must not be read that way
+any more: the reset is bound to a positive proof instead of to the absence of an exception
+(`PLAN_GFMY_AUTH_RESET_POSITIVE_PROOF`). The proof differs per path and each one is named: in the poll cycle a location
+WITH content, in the manual locate a record that survives the empty guard, and for the transient-auth counter a
+successful `async_get_basic_device_list` -- the strongest source in the tree, because it has no non-throwing error exit
+and an expired login raises before it can reach the reset. On the poll path the same location WITH content clears the
+counter as well, so the device list is the everyday source, not the only one. An empty result clears nothing at all any
+more AT THE DEVICE SITE: not the idle BLE tag, not the four pre-accept failures named above. Read that as the narrow
+statement it is. A cycle in which nothing was rejected still releases the pending marker, so an idle tag can still open
+the door for the NEXT device-list refresh to clear the count -- one refresh later and only while no rejection stands. The price was measured and accepted with the
+change, and it is bounded: an auth error already on screen is now cleared by the next device-list refresh instead of by
+the next poll of any kind. `DEVICE_LIST_POLL_INTERVAL` is a fixed 300 seconds while the poll interval is an option
+between 60 and 3600 seconds (default 300), so at the default both run on the same cadence and at the shortest setting
+the wait grows to at most five poll cycles.
+Why that reset is CONDITIONAL, written down because the unconditional version starved the very escalation the counter
+feeds. The device-list refresh runs inside the same `_async_update_data` and BEFORE the poll cycle is scheduled, and at
+the default settings both cadences are 300 seconds. Clearing the counter there on every pass therefore zeroed it
+immediately before every attempt: a tracker whose action RPC kept returning a non-permanent credential rejection could
+only ever climb back to 1 and never reached `_MAX_TRANSIENT_AUTH_FAILURES`. That was the state of the first revision of
+this change, recorded as `R-3` in the plan, and an external reviewer found it before a user did. What the list proves is
+the ACCOUNT token, and only that; it says nothing about the action RPC accepting the same token again, which is what the
+counter counts. So a booked location failure sets `_transient_auth_failure_pending`, and while that marker stands the
+refresh leaves the count alone. Released by a POLL CYCLE that books no rejection, never by the refresh itself, and that
+difference is the whole design. A marker a refresh consumed would assume exactly one refresh per cycle, and nothing
+couples the two: `DEVICE_LIST_POLL_INTERVAL` is a fixed 300 s while `location_poll_interval` reaches 3600 s, and a
+deferred empty list re-fetches on the next 60 s tick without moving `_last_list_poll_mono`. At any poll setting above
+600 s, and already at the default whenever one empty list is deferred, a second refresh would fall into the same gap and
+clear the count -- the same starvation one layer along. Bound to the cycle it holds at every cadence. A one-off hiccup
+still heals: the next clean cycle releases the marker and the refresh after that clears the count. Suppressing the reset
+outright was rejected for the same reason it was introduced: it would strand any counter a single hiccup ever raised. A
+genuinely expired sign-in does not depend on the counter anyway: `async_get_basic_device_list` raises
+`ConfigEntryAuthFailed` and the reauth flow starts from there. What the change does buy is that a pending auth error
+survives long enough for a user to see it, rather than being wiped by the next idle tag inside the same cycle.
+The limit of that, measured over ten cycles rather than reasoned about: a PERSISTENT rejection escalates -- with a
+broken tracker next to an idle one the threshold is reached on cycle three, because every cycle books and nothing is
+ever released. An INTERMITTENT rejection on a single device does NOT escalate: the cycles in between book nothing,
+release the marker, and the next refresh clears the count. That is what a consecutive counter means, and the reset is
+sampled at refresh time rather than at cycle end, so "consecutive" is measured on a coarser clock than the poll. If
+that ever needs to change, the release would have to hang on a positive proof (a location WITH content) instead of on
+the absence of a rejection -- which would make the device-list refresh a dead reset source, because a location with
+content already clears the counter outright at its own site. The two cannot both be true at once.
+`test_the_production_order_still_reaches_the_reauth_threshold` drives the real `_async_update_data` ->
+`_async_start_poll_cycle` sequence for three cycles and pins the escalation; the tests it replaces called
+`_async_start_poll_cycle()` directly and so never saw the refresh that preceded it in production.
+`test_two_refreshes_between_two_polls_do_not_clear_the_counter` pins the cadence independence, and
+`test_a_clean_cycle_releases_the_counter_for_the_next_refresh` pins the other side, so the fix cannot be tightened into
+a sticky suppression.
+What this does NOT change, named because the reasoning above invites the opposite reading: `_set_auth_state(failed=
+False)` on that same success path stays UNCONDITIONAL. The account token is exactly what that state reports, and the
+list proves it. The consequence is real and accepted: after an escalation raised from the action RPC, the next refresh
+clears the repairs issue while the reauth flow it started stays open in the UI. Two channels, one of which is
+account-scoped and now correct; binding the auth state to the marker as well would delay every legitimate recovery by a
+poll cycle for a display-only gain. Three tests pin the new state so it cannot drift back:
+`test_an_empty_return_proves_nothing_about_the_credentials` carries the inverted assertion together with the history of
+the characterisation it replaces, `test_an_unaccepted_request_no_longer_clears_the_counter` is its contract pair for the
+requests that no longer reach the path at all, and `test_a_non_credential_4xx_location_is_passed_through` pins the seam
+that keeps a rejection out of it.
 What is NOT fixed, stated so the rule is not mistaken for a solved problem: `nova_request.py` still raises a type named
 "auth" for all of them, so a new handler that reads the type repeats the defect, and this paragraph is the only thing
 standing in its way. Giving the non-credential case its own exception class is the open follow-up; it needs an

custom_components/googlefindmy/NovaApi/ExecuteAction/LocateTracker/location_request.py

@@ -104,8 +104,8 @@ class LocationRequestNotAcceptedError(Exception):
     a request that never got that far (no FCM token, a failed FCM registration,
     a 429, a 5xx, a network error, an unclassified Nova failure, or a surfacing
     error before the accept line). ``api.async_get_device_location`` mapped both
-    to ``{}``, so both coordinator callers read every non-raising return as
-    positive proof that the credentials work. This type carries the second state
+    to ``{}``, so both coordinator callers used to read every non-raising return
+    as positive proof that the credentials work. This type carries the second state
     across the ``location_request.py`` boundary -- the layer that flattens it --
     instead of leaving it to be reconstructed downstream once the evidence is
     already gone.

custom_components/googlefindmy/api.py

@@ -25,10 +25,13 @@
   expired". A non-credential rejection therefore leaves the device list as
   `UpdateFailed`, and the location request passes the error on to its callers,
   whose own branches skip the device without touching the re-auth machinery. It
-  is deliberately NOT collapsed to an empty result: both callers read a
-  non-raising return as proof that the credentials work and clear the auth state
-  before they check for emptiness, so a permanently rejected tracker would have
-  masked a real 401 on another tracker. `_classify_nova_auth_error` is the sound-path adapter over the
+  is deliberately NOT collapsed to an empty result. That began as protection
+  against a second defect, now fixed: both callers used to read a non-raising
+  return as proof that the credentials work and cleared the auth state before
+  they checked for emptiness, so a permanently rejected tracker masked a real
+  401 on another tracker. Those resets sit behind the empty guard since
+  (`PLAN_GFMY_AUTH_RESET_POSITIVE_PROOF`); passing the error on stays right
+  because a rejection is a failure and an empty dict cannot say so. `_classify_nova_auth_error` is the sound-path adapter over the
   same predicate. Still open, stated so it is not mistaken for coverage: the
   transport keeps raising a type named "auth" for all of them, so a future
   handler that reads the type instead of the predicate repeats the defect.
@@ -1455,21 +1458,33 @@ def _cache_provider() -> CacheProtocol | None:
                 # branch for exactly this status.
                 #
                 # Do NOT collapse this to `return {}`. That was the first attempt
-                # and it traded one defect for another: a non-raising return is
+                # and it traded one defect for another. The first reason is now
+                # history and is kept as such: a non-raising return used to be
                 # what both callers read as positive proof that the sign-in
-                # works. coordinator/polling.py clears the auth state and resets
-                # the transient-auth counter, and coordinator/locate.py clears
-                # the auth state, each BEFORE looking at whether the result is
-                # empty. A permanently rejected tracker would then wipe a real
-                # 401 from another tracker in every cycle, and the re-auth prompt
-                # that this whole change exists to postpone would never appear at
-                # all. Raising keeps that reset out of reach.
+                # works. coordinator/polling.py cleared the auth state and reset
+                # the transient-auth counter, and coordinator/locate.py cleared
+                # the auth state, each BEFORE looking at whether the result was
+                # empty, so a permanently rejected tracker wiped a real 401 from
+                # another tracker in every cycle. Both resets now sit BEHIND the
+                # empty guard (`PLAN_GFMY_AUTH_RESET_POSITIVE_PROOF`), so an
+                # empty return cannot reach them any more.
                 #
-                # The 5xx branch below still returns {} and still reaches that
-                # reset. That is pre-existing, it is wrong on its own terms, and
-                # it is tracked as a separate finding
-                # (`PLAN_GFMY_EMPTY_RESULT_DISTINGUISHABLE`) -- not fixed here,
-                # and not made worse here either.
+                # The other half of the reason outlives that fix and is why this
+                # still raises: a client rejection is a failure, and `{}` is the
+                # shape of a healthy idle tag. Handing the caller that shape
+                # hands it a result it cannot classify -- no `cycle_failed`,
+                # nothing for the post-loop guard to count -- and ending exactly
+                # that ambiguity is what the raise sites exist for.
+                #
+                # The 5xx branch below still returns {}. It no longer clears
+                # anything, and measured it is not reached at all any more:
+                # `get_location_data_for_device` converts the 5xx and the 429
+                # into `LocationRequestNotAcceptedError` itself
+                # (`PLAN_GFMY_EMPTY_RESULT_DISTINGUISHABLE`), and its broad
+                # handler re-raises only before the accept line and returns []
+                # after it -- so no `NovaHTTPError` leaves that layer. The sync
+                # wrapper is no exception: it calls this very method. The branch
+                # stays as the guard for the day that conversion moves or goes.
                 #
                 # DEBUG, not WARNING: both callers already log a WARNING naming
                 # the device, so a WARNING here would print the same event twice

custom_components/googlefindmy/coordinator/_mixin_typing.py

@@ -111,6 +111,7 @@ class _MixinBase:
     _fcm_defer_started_mono: float
     _consecutive_transient_auth_failures: int
     _last_transient_auth_error: str | None
+    _transient_auth_failure_pending: bool
     # Structured, redaction-safe record of the most recent reauth trigger (FIX 3).
     _reauth_reason: ReauthReason | None
     _consecutive_decrypt_failures: int