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
 # --------------------------------------------------------------------------------------

docs/PLAY_SOUND_ARCHITECTURE.md

@@ -155,8 +155,106 @@ 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** (see the stop-path qualifier below) |
+| `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.
+
+The rule has exactly one consumer, and stating it in a type is not the same as
+enforcing it. `coordinator/locate.py` holds every call to
+`_note_push_transport_problem()` on the sound paths, and `async_play_sound` and
+`async_stop_sound` each arm it on `TRANSPORT_FAILED` alone. On the stop path the
+call is indirect and carries one further condition: it goes through
+`_note_stop_transport_problem_without_extending()`, which reports the failure in
+full but restores a window that was already running instead of restarting it
+(IRR-CA-STOP-BREAKS-SELF-INFLICTED-COOLDOWN below). Their blanket
+`except Exception` handlers do not arm it at all: `api.py` classifies every
+unexpected exception in band and returns `INTERNAL_ERROR` rather than raising
+(`api.py`, the final handler of both sound methods), so what still reaches those
+coordinator handlers is a failure of the coordinator's own bookkeeping around
+the call, and none of that is the push transport. The two typed handlers for
+`TimeoutError`/`ClientConnectionError`/`ClientError` do keep the cooldown: `api`
+is a Protocol, and an aiohttp error surfacing from an implementation that does
+not wrap it really is a transport failure. The `pytest.mark.parametrize` tables
+in `tests/test_coordinator_locate_basics.py` carry one row per enum member and
+are guarded against a member being added without a decision, so the "may a
+caller arm a push cooldown" column above is a measured claim, not a wish.
+
+**IRR-CA-STOP-BREAKS-SELF-INFLICTED-COOLDOWN (the window must not silence its
+own remedy).** A play that reaches the wire and then loses the answer does two
+things in one breath: it stores a cancel key, and it correctly arms the
+90-second push cooldown. `_api_push_ready()` short-circuits on that cooldown, so
+the stop the key exists for used to be suppressed for the first 90 seconds after
+that play, which is exactly when a user reaches for the Stop button. (How long
+the ring itself lasts is a different timer on a different layer, and nothing
+here claims to know it -- see the note on the aged cached key in
+`async_stop_sound`.) `async_stop_sound` therefore passes the readiness
+gate in exactly one case: a window is running **and** the stop would be
+correlated, that is, it carries our own cached key and that key is still fresh.
+
+The polarity is deliberately the opposite of `can_play_sound()` and the manual
+locate guard, which treat a running window as the hard block and an unconfirmed
+transport as passable. Two limits keep the exception honest:
+
+- **What it can distinguish.** While a window runs, `_api_push_ready()` never
+  asks the transport, so the transport state is unknown. The guard separates
+  "not ready because a window is running" from "not ready for another reason"
+  and nothing more; a transport failure is the only thing that arms the window,
+  so the two overlap by construction.
+- **It cannot feed itself.** A broken-through stop that fails on the transport
+  must not restart the window. `_note_push_transport_problem()` sets
+  `_push_cooldown_until` absolutely, and the stop button has no availability
+  guard, so an unguarded call would let repeated presses keep Play Sound and
+  manual locate disabled indefinitely. `async_stop_sound` routes both of its
+  cooldown calls through
+  `_note_stop_transport_problem_without_extending()`, which arms a window only
+  when none is running.
+
+`_stop_would_be_correlated()` is the single definition of "correlated" behind
+both this gate and the decision to spend the cached key
+(IRR-CA-POP-ON-CORRELATED-CANCEL-ONLY); a second inline derivation at either
+site is the drift the extraction exists to prevent, and
+`tests/test_coordinator_locate_basics.py` pins both sites to it.
+
+The exception is not free, and the price is a changed outcome class rather than
+a changed guarantee: a stop that used to end as `SUPPRESSED` now reaches the
+transport and can end as `FAILED`, which `services.py` reports with a different
+translation key (`stop_sound_rejected` instead of `stop_sound_suppressed`). It
+also removes, for the duration of a window, the only rate brake that sat in
+front of the stop dispatch -- outside a window there never was one, and a
+`REJECTED_RATE_LIMIT` answer deliberately arms no cooldown.
+
 #### Follow-up (not in this change)
 
+The play path arms the same cooldown without this guard
+(`coordinator/locate.py`, `async_play_sound`), and `can_play_sound()` returns
+early on a cached `can_ring` capability before it ever looks at the window, so a
+play can reach the arming call while a window runs. That behaviour predates the
+stop-path exception and is unchanged by it; guarding it is a separate decision
+with its own tests.
+
 Closing the boundary on the cloud path means wiring **Path B**: registering an
 FCM callback for sound events and matching the incoming
 `ExecuteActionRequestMetadata.requestUuid` against the key held by the

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.