fix(sound): classify Play/Stop Sound failures instead of collapsing them into False

custom_components/googlefindmy/const.py

@@ -11,6 +11,7 @@
 import math
 import time
 from collections.abc import Mapping, Sequence
+from dataclasses import dataclass
 from enum import StrEnum
 from types import MappingProxyType
 from typing import Final, Literal
@@ -35,6 +36,90 @@
 # would silently skip this file on the automated version bump.
 INTEGRATION_VERSION = "1.7.15.10"
 
+# --------------------------------------------------------------------------------------
+# Sound dispatch outcome (API -> coordinator boundary)
+# --------------------------------------------------------------------------------------
+
+
+class SoundDispatchOutcome(StrEnum):
+    """Why a sound command did or did not reach Google, as classified by ``api.py``.
+
+    ``api.py`` already reaches nine distinct exits (missing action token, empty
+    submitter reply, HTTP 200, NovaAuthError, HTTP 401/403, other HTTP status,
+    rate limit, network error, unexpected exception). Until this type existed,
+    all of them collapsed into a single ``False``, and the coordinator had no
+    choice but to read that ``False`` as "the push transport is broken". A server
+    saying no and a network that never answered then produced the same 90-second
+    cooldown and the same ``FcmStatus.DEGRADED``. This is the same failure class
+    ``StopSoundOutcome`` documents one layer up: a bool cannot carry the state
+    space.
+
+    Only ``TRANSPORT_FAILED`` justifies a push cooldown. Only ``ACCEPTED`` proves
+    that the credentials worked.
+    """
+
+    ACCEPTED = "accepted"
+    """Nova answered HTTP 200. Acceptance of the submission, not proof of a ring.
+
+    See IRR-CA-NO-RING-CONFIRMATION in ``docs/PLAY_SOUND_ARCHITECTURE.md``.
+    """
+
+    REJECTED_AUTH = "rejected_auth"
+    """The server answered and refused on credentials (NovaAuthError, 401, 403).
+
+    The transport worked. A cooldown would mislabel an expired sign-in as a
+    network outage and hide it behind a self-clearing timer.
+    """
+
+    REJECTED_RATE_LIMIT = "rejected_rate_limit"
+    """The server answered HTTP 429. The transport worked; only the pace was wrong."""
+
+    REJECTED_SERVER = "rejected_server"
+    """The server answered and refused for any other reason (5xx, logic error)."""
+
+    TRANSPORT_FAILED = "transport_failed"
+    """No usable answer was obtained: DNS, connect refused/timeout, disconnect,
+    read timeout, or a NovaError leaving the transport after its retries.
+
+    This is the ONLY outcome that justifies arming the push cooldown.
+    """
+
+    NOT_SENT = "not_sent"
+    """A local precondition failed before any transport was used (no action token).
+
+    Neither the server nor the network said anything, so neither may be blamed.
+    """
+
+    INTERNAL_ERROR = "internal_error"
+    """This integration failed on its own: an unexpected exception, or a contract
+    violation such as an empty reply from the submitter.
+
+    Classifying such a bug as a network outage is what this type was written to
+    stop; it is logged with a traceback and never arms the cooldown.
+    """
+
+
+@dataclass(frozen=True, slots=True)
+class PlaySoundResult:
+    """Result of ``api.async_play_sound``: a classification plus the cancel key.
+
+    ``cancel_key`` is non-None in exactly the cases where the device may be
+    ringing and a later Stop needs the handle: an accepted command, or a failure
+    that latched dispatch (``NovaError.dispatched``). Read ``cancel_key`` ONLY for
+    the question "may the device be ringing"; read ``outcome`` for every question
+    about the cause. Deriving the cause from the key is the out-of-band channel
+    this type replaces.
+    """
+
+    outcome: SoundDispatchOutcome
+    cancel_key: str | None = None
+
+    @property
+    def accepted(self) -> bool:
+        """Return True when Nova accepted the submission (HTTP 200)."""
+        return self.outcome is SoundDispatchOutcome.ACCEPTED
+
+
 # --------------------------------------------------------------------------------------
 # Stop Sound outcome
 # --------------------------------------------------------------------------------------

custom_components/googlefindmy/coordinator/locate.py

@@ -25,7 +25,11 @@
 from homeassistant.exceptions import ConfigEntryAuthFailed, HomeAssistantError
 
 from .._reauth_reason import ReauthReasonCode
-from ..const import DEFAULT_MIN_POLL_INTERVAL, StopSoundOutcome
+from ..const import (
+    DEFAULT_MIN_POLL_INTERVAL,
+    SoundDispatchOutcome,
+    StopSoundOutcome,
+)
 from ..NovaApi.ExecuteAction.LocateTracker.decrypt_locations import (
     DecryptionError,
     OwnerKeyLookupTransientError,
@@ -742,7 +746,15 @@ async def async_play_sound(self, device_id: str) -> bool:
             )
             return False
         try:
-            ok, request_uuid = await self.api.async_play_sound(device_id)
+            play = await self.api.async_play_sound(device_id)
+            # api.async_play_sound now names WHO refused (play.outcome, see
+            # IRR-CA-SOUND-FAILURE-CLASS). Acting on that classification -- above
+            # all, arming the push cooldown only for TRANSPORT_FAILED instead of
+            # for every non-acceptance -- is the next step and deliberately not
+            # part of this one. Until then the two facts are unpacked to exactly
+            # the values the old tuple carried, so this changes the type at the
+            # boundary and nothing else.
+            ok, request_uuid = play.accepted, play.cancel_key
             # Decide whether to (over)write the cached Stop cancel key.
             # api.async_play_sound returns a non-None UUID in exactly the two
             # cases where a ring may be active and Stop needs the key: (1) the
@@ -786,8 +798,14 @@ async def async_play_sound(self, device_id: str) -> bool:
                 await self._async_save_sound_uuids()
             if not ok:
                 self._note_push_transport_problem()
-            # Success implies credentials worked
-            self._set_auth_state(failed=False)
+            else:
+                # Only an ACCEPTED submission proves the credentials worked.
+                # api.async_play_sound collapses NovaAuthError and HTTP 401/403
+                # into the same False as a read timeout, so clearing the
+                # auth-failure state on a failed play erased the very signal an
+                # expired sign-in produces. async_stop_sound has always applied
+                # this rule and states the reason; the two paths now agree.
+                self._set_auth_state(failed=False)
             return bool(ok)
         except ConfigEntryAuthFailed as auth_exc:
             self._set_auth_state(
@@ -949,7 +967,13 @@ async def async_stop_sound(
                 )
 
         try:
-            submitted = await self.api.async_stop_sound(device_id, request_uuid_to_use)
+            stop_outcome = await self.api.async_stop_sound(
+                device_id, request_uuid_to_use
+            )
+            # Same deliberate narrowing as on the play path above: the boundary
+            # now carries the classification, acting on it is the next step.
+            # Acceptance is the single bit this one keeps looking at.
+            submitted = stop_outcome is SoundDispatchOutcome.ACCEPTED
             if not submitted:
                 self._note_push_transport_problem()
                 # No credential proof on this path: api.async_stop_sound

docs/PLAY_SOUND_ARCHITECTURE.md

@@ -155,6 +155,33 @@ Two consequences follow, and both are already implemented:
 - Even `CANCELLED` claims only "submitted with a correlated cancel key", never
   "the device stopped".
 
+**IRR-CA-SOUND-FAILURE-CLASS (who refused, and who may be blamed).** A sound
+command can fail in ways that have nothing to do with each other, and the layer
+that knows which one happened is `api.py`. `SoundDispatchOutcome` carries that
+knowledge across the boundary instead of collapsing it into a bool:
+
+| Outcome | The server answered | May a caller arm a push cooldown |
+|---|---|---|
+| `ACCEPTED` | yes, HTTP 200 | no |
+| `REJECTED_AUTH` | yes, on credentials | no |
+| `REJECTED_RATE_LIMIT` | yes, HTTP 429 | no |
+| `REJECTED_SERVER` | yes, for any other reason | no |
+| `TRANSPORT_FAILED` | no usable answer was obtained | **yes** |
+| `NOT_SENT` | no transport was used at all | no |
+| `INTERNAL_ERROR` | our own defect or a broken contract | no |
+
+Only `TRANSPORT_FAILED` describes a broken push transport. Reporting anything
+else as one is what let an expired sign-in, a rate limit and a bug of our own
+each produce the same 90-second cooldown and the same `FcmStatus.DEGRADED`, and
+`can_play_sound()` reported the button as unavailable for the duration. This is
+the same failure class as `StopSoundOutcome` one layer up: a bool cannot carry
+the state space.
+
+`PlaySoundResult` pairs that outcome with the cancel key, and the two are
+deliberately independent. The key answers one question only, "may the device be
+ringing" (see IRR-CA-CANCEL-KEY-ON-SUCCESS-ONLY); the cause is read from the
+outcome and never inferred from the presence or absence of a key.
+
 #### Follow-up (not in this change)
 
 Closing the boundary on the cloud path means wiring **Path B**: registering an

tests/helpers/constants.py

@@ -3,6 +3,7 @@
 
 from __future__ import annotations
 
+import sys
 from functools import lru_cache
 from importlib.util import module_from_spec, spec_from_file_location
 from pathlib import Path
@@ -32,7 +33,22 @@ def load_googlefindmy_const_module() -> ModuleType:
         raise RuntimeError("Unable to load googlefindmy const module")
 
     module = module_from_spec(spec)
-    spec.loader.exec_module(module)
+    # Register before exec_module, as the importlib recipe for "importing a source
+    # file directly" prescribes. Skipping this half of the recipe worked only as
+    # long as const.py contained nothing that resolves its own annotations at
+    # class-creation time. @dataclass does: with `from __future__ import
+    # annotations` every field annotation is a string, and dataclasses resolves it
+    # via sys.modules[cls.__module__] to tell a KW_ONLY/ClassVar marker from a real
+    # field. Unregistered, that lookup yields None and the module fails to import
+    # with `AttributeError: 'NoneType' object has no attribute '__dict__'`, which
+    # this conftest turns into a collection error for the entire test suite.
+    sys.modules[spec.name] = module
+    try:
+        spec.loader.exec_module(module)
+    except BaseException:
+        # Do not leave a half-initialised module behind for the next importer.
+        sys.modules.pop(spec.name, None)
+        raise
     return module
 
 

tests/helpers/locate_mixin_stub.py

@@ -19,6 +19,10 @@
 from typing import Any
 from unittest.mock import AsyncMock, MagicMock
 
+from custom_components.googlefindmy.const import (
+    PlaySoundResult,
+    SoundDispatchOutcome,
+)
 from custom_components.googlefindmy.coordinator.locate import LocateOperations
 
 
@@ -73,8 +77,12 @@ def __init__(
         self.min_poll_interval = min_poll_interval
         self.api = api if api is not None else MagicMock()
         self.api.async_get_device_location = AsyncMock(return_value={})
-        self.api.async_play_sound = AsyncMock(return_value=(True, "uuid-stub"))
-        self.api.async_stop_sound = AsyncMock(return_value=True)
+        self.api.async_play_sound = AsyncMock(
+            return_value=PlaySoundResult(SoundDispatchOutcome.ACCEPTED, "uuid-stub")
+        )
+        self.api.async_stop_sound = AsyncMock(
+            return_value=SoundDispatchOutcome.ACCEPTED
+        )
 
         # Cross-mixin methods owned by other mixins or DataUpdateCoordinator:
         # mock them so default behavior is a no-op. Tests override per-case.