From 9b3fb8557e277702d63d1ad213f5c5fa9ba1810c Mon Sep 17 00:00:00 2001 From: jleinenbach <1786119+jleinenbach@users.noreply.github.com> Date: Tue, 25 Aug 2026 18:05:36 +0000 Subject: [PATCH 1/7] fix(sound): only clear the auth-failure state when a play is accepted `async_play_sound` ran `_set_auth_state(failed=False)` unconditionally, on the failure path as well. `api.async_play_sound` collapses `NovaAuthError` and HTTP 401/403 into the same `(False, None)` it returns for a read timeout, so a play rejected by an expired sign-in did not merely fail: it erased the very signal that the sign-in had expired, and did so on every press for as long as the credentials stayed bad. The sister path already applies the opposite rule and states the reason in a comment (`async_stop_sound`: "No credential proof on this path"). The two paths now agree; the fix is the `else:` that was missing. Scope, so the change is not read as more than it is: a failed play still does not SET the auth-failure state, and still does not trigger reauth. It only stops clearing it. Setting it would raise a Repairs issue and fire an event off a `NovaAuthError` that its own docstring calls possibly transient, which would trade one false signal for another. Tests: `test_failed_play_does_not_clear_auth_state` was written first and verified red against 97edca64 -- exit 1 with `Expected 'mock' to not have been called. Called 1 times. Calls: [call(failed=False)]`, i.e. the defect itself and not a missing file (exit 4) or an unknown test name (exit 5). `test_accepted_play_still_clears_auth_state` guards the positive half and was green before and after. 81 passed across `test_coordinator_locate_basics.py`, `test_coordinator_sound_uuid.py` and `test_stop_sound_correlation.py` (79 before). Known forward breakage, recorded rather than discovered later: the new negative test pins today's `(False, None)` contract of `api.async_play_sound`. Replacing that contract with a typed dispatch outcome will break it by design, and its migration is part of that work. --- .../googlefindmy/coordinator/locate.py | 10 +++++-- tests/test_coordinator_locate_basics.py | 29 +++++++++++++++++++ 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/custom_components/googlefindmy/coordinator/locate.py b/custom_components/googlefindmy/coordinator/locate.py index ad4332339..514b14993 100644 --- a/custom_components/googlefindmy/coordinator/locate.py +++ b/custom_components/googlefindmy/coordinator/locate.py @@ -786,8 +786,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( diff --git a/tests/test_coordinator_locate_basics.py b/tests/test_coordinator_locate_basics.py index a7e725223..832f8ef55 100644 --- a/tests/test_coordinator_locate_basics.py +++ b/tests/test_coordinator_locate_basics.py @@ -391,6 +391,35 @@ async def test_unexpected_exception_returns_false(self, coord: LocateStub) -> No coord.note_error.assert_called_once() coord._note_push_transport_problem.assert_called_once() + async def test_failed_play_does_not_clear_auth_state( + self, coord: LocateStub + ) -> None: + """A play that was not accepted must not vouch for the credentials. + + ``api.async_play_sound`` collapses a 401/403 rejection into the same + ``(False, None)`` as a timeout, so clearing the auth-failure state here + deleted the signal an expired sign-in produces. The stop path never did + this (see ``async_stop_sound``); the two paths now agree. + """ + + coord._device_caps["dev-1"] = {"can_ring": True} + coord.api.async_play_sound.return_value = (False, None) + + assert await coord.async_play_sound("dev-1") is False + + coord._set_auth_state.assert_not_called() + + async def test_accepted_play_still_clears_auth_state( + self, coord: LocateStub + ) -> None: + """The positive half of the rule must not be lost with the fix.""" + + coord._device_caps["dev-1"] = {"can_ring": True} + + assert await coord.async_play_sound("dev-1") is True + + coord._set_auth_state.assert_called_once_with(failed=False) + class TestAsyncStopSoundGating: """Exercise gating branches of ``async_stop_sound``.""" From e62204e732c384842d3df0abee6700646e9b3bba Mon Sep 17 00:00:00 2001 From: jleinenbach <1786119+jleinenbach@users.noreply.github.com> Date: Tue, 25 Aug 2026 21:05:26 +0000 Subject: [PATCH 2/7] refactor(sound): add the API -> coordinator sound dispatch contract api.py already reaches nine distinct exits for a Play Sound command (missing action token, empty submitter reply, HTTP 200, NovaAuthError, HTTP 401/403, other HTTP status, rate limit, network error, unexpected exception), and all of them collapse into a single False at the boundary. The coordinator has no choice but to read that False as "the push transport is broken", so a server saying no, an expired sign-in and a bug of our own each arm the same 90-second cooldown and the same FcmStatus.DEGRADED. This is the failure class StopSoundOutcome already documents one layer up: a bool cannot carry the state space. Add SoundDispatchOutcome (seven values, only TRANSPORT_FAILED justifies a cooldown) and PlaySoundResult (outcome plus cancel key, deliberately two independent facts). Nothing consumes them yet; this commit is additive and changes no behaviour. Also fix tests/helpers/constants.py, which loads const.py through spec_from_file_location/exec_module without registering the module in sys.modules -- the half of the documented importlib recipe that only matters once the module resolves its own annotations at class-creation time. @dataclass does exactly that: 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 returns None and const.py fails to import with AttributeError: 'NoneType' object has no attribute '__dict__' in tests/conftest.py:375, i.e. as a collection error for the whole suite. Without this, const.py cannot host a dataclass at all. Verified: tests/test_sound_dispatch_contract.py fails to collect before the type exists (exit 2, ImportError: cannot import name 'PlaySoundResult') and passes after; mypy --strict and ruff check/format clean; the six sound- related test files stay green (173 passed). --- custom_components/googlefindmy/const.py | 85 +++++++++++++++++++++++++ tests/helpers/constants.py | 18 +++++- tests/test_sound_dispatch_contract.py | 43 +++++++++++++ 3 files changed, 145 insertions(+), 1 deletion(-) create mode 100644 tests/test_sound_dispatch_contract.py diff --git a/custom_components/googlefindmy/const.py b/custom_components/googlefindmy/const.py index 644f08fc8..6fa4f0868 100644 --- a/custom_components/googlefindmy/const.py +++ b/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 # -------------------------------------------------------------------------------------- diff --git a/tests/helpers/constants.py b/tests/helpers/constants.py index f3fd1322a..59c72f8b0 100644 --- a/tests/helpers/constants.py +++ b/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 diff --git a/tests/test_sound_dispatch_contract.py b/tests/test_sound_dispatch_contract.py new file mode 100644 index 000000000..ce6a61cc7 --- /dev/null +++ b/tests/test_sound_dispatch_contract.py @@ -0,0 +1,43 @@ +# tests/test_sound_dispatch_contract.py +"""Pin the API -> coordinator sound contract as a type, not as a bool.""" + +from __future__ import annotations + +import dataclasses + +from custom_components.googlefindmy.const import ( + PlaySoundResult, + SoundDispatchOutcome, +) + +# NOTE: "only TRANSPORT_FAILED may arm the push cooldown" is a property of +# coordinator/locate.py, not of const.py. Asserting it here would only restate +# the enum against itself, so it is pinned where it is observable, in +# tests/test_coordinator_locate_basics.py (see AP-4 of +# PLAN_GFMY_SOUND_FAILURE_CLASSIFICATION), parametrised over every member of +# SoundDispatchOutcome so that future members are covered automatically. + + +def test_result_is_frozen_and_defaults_to_no_cancel_key() -> None: + """A result must not be mutated after the fact, and must not invent a key.""" + + result = PlaySoundResult(SoundDispatchOutcome.REJECTED_AUTH) + + assert result.cancel_key is None + assert result.accepted is False + assert dataclasses.is_dataclass(result) + try: + result.outcome = SoundDispatchOutcome.ACCEPTED # type: ignore[misc] + except dataclasses.FrozenInstanceError: + pass + else: # pragma: no cover - only reached if the dataclass loses frozen=True + raise AssertionError("PlaySoundResult must be frozen") + + +def test_accepted_is_the_only_true_predicate() -> None: + """``accepted`` must not creep into meaning "no error".""" + + for member in SoundDispatchOutcome: + assert PlaySoundResult(member).accepted is ( + member is SoundDispatchOutcome.ACCEPTED + ) From 72e4602ff23f1ca11f9f73d87679058c797454db Mon Sep 17 00:00:00 2001 From: jleinenbach <1786119+jleinenbach@users.noreply.github.com> Date: Tue, 25 Aug 2026 21:05:55 +0000 Subject: [PATCH 3/7] refactor(sound): classify every sound exit in api.py async_play_sound now returns PlaySoundResult and async_stop_sound returns SoundDispatchOutcome, so each exit names who refused instead of collapsing into False: NOT_SENT for a missing local token, REJECTED_AUTH / _SERVER / _RATE_LIMIT for a server that answered and refused, TRANSPORT_FAILED for a network that never gave a usable answer, INTERNAL_ERROR for our own defect or a broken submitter contract. Two of those are new placements rather than renamings. NovaLogicError and NovaProtobufDecodeError were caught by `except NovaError` and would now be reported as transport failures; they are caught before it, as REJECTED_SERVER (the server answered with an error code in a 200 body) and INTERNAL_ERROR (we could not decode what the transport delivered). And async_stop_sound had no `except NovaError` at all -- a network failure after exhausted retries fell through to `except Exception` -- so the branch is added, which is what lets the stop path carry the same table as the play path. Both `except Exception` handlers now log with a traceback: a bug of our own deserves one, and it is no longer indistinguishable from an outage. coordinator/locate.py is adapted minimally and on purpose. A tuple unpack does not work on a dataclass, and a StrEnum is always truthy, so `if not submitted` would have read every rejected stop as a success. It therefore reads play.accepted / play.cancel_key and `stop_outcome is ACCEPTED`, which are exactly the values the old contract carried. Acting on the classification -- above all, arming the push cooldown only for TRANSPORT_FAILED -- is deliberately not part of this commit. The test stubs move with the contract on the same rule: the old False maps to TRANSPORT_FAILED, because today every non-acceptance arms the cooldown, so each existing test keeps asserting the same behaviour as before. docs/PLAY_SOUND_ARCHITECTURE.md defines IRR-CA-SOUND-FAILURE-CLASS, the anchor api.py now cites; leaving the reference dangling is the defect tests/test_stop_sound_correlation.py already guards against. Verified: test_play_sound_classifies_every_exit fails before the change (exit 1, nine failures, AttributeError: 'tuple' object has no attribute 'outcome') and passes after; tests/test_api_basics.py green (152 -> 168); mypy --strict on api.py, const.py and coordinator/locate.py clean; ruff check and ruff format --check clean tree-wide; changed-code coverage measured against the diff: 207 changed lines in api.py and 20 in locate.py, none uncovered. Full suite 6605 tests, 6584 passed, 19 skipped, 2 failed -- both in tests/test_main.py, both failing on the untouched parent commit as well (subprocess tests whose child is SIGKILLed after a 30s timeout in this environment). --- custom_components/googlefindmy/api.py | 283 +++++++++++++----- .../googlefindmy/coordinator/locate.py | 24 +- docs/PLAY_SOUND_ARCHITECTURE.md | 27 ++ tests/helpers/locate_mixin_stub.py | 12 +- tests/test_api_basics.py | 178 +++++++++-- tests/test_api_fcm_token_scoping.py | 25 +- tests/test_api_location_selection.py | 12 +- tests/test_coordinator_locate_basics.py | 16 +- tests/test_coordinator_sound_uuid.py | 106 ++++--- tests/test_options_flow_credentials_cache.py | 9 +- tests/test_stop_sound_correlation.py | 5 +- 11 files changed, 539 insertions(+), 158 deletions(-) diff --git a/custom_components/googlefindmy/api.py b/custom_components/googlefindmy/api.py index 9625084bd..d98500b2f 100644 --- a/custom_components/googlefindmy/api.py +++ b/custom_components/googlefindmy/api.py @@ -43,6 +43,8 @@ CONTRIBUTOR_MODE_HIGH_TRAFFIC, CONTRIBUTOR_MODE_IN_ALL_AREAS, DEFAULT_CONTRIBUTOR_MODE, + PlaySoundResult, + SoundDispatchOutcome, ) from .NovaApi import nova_request from .NovaApi.ExecuteAction.LocateTracker.decrypt_locations import ( @@ -103,24 +105,32 @@ def _short_err(e: Exception | str) -> str: return msg -def _cancel_key_after_failure( - err: NovaError, request_uuid: str -) -> tuple[bool, str | None]: - """Decide the Play-Sound cancel-key fate for a non-accepted command. +def _play_result_after_failure( + err: NovaError, request_uuid: str, outcome: SoundDispatchOutcome +) -> PlaySoundResult: + """Build the Play Sound result for a non-accepted command. + + Two independent facts are carried, and they must not be confused. ``outcome`` + names WHO refused: the server, the network, or this integration. ``cancel_key`` + answers only "may the device be ringing", which the transport latches onto the + error (``NovaError.dispatched``) at its retry-loop choke point. Before this + type existed, the second fact was the only one that survived the boundary, and + the coordinator had to reconstruct the first from it, which is why a server + rejection and a dead network both ended up as a push transport problem. A raised ``NovaError`` is never an acceptance (the submitter returns a tuple - only on HTTP 200), so ``success`` is always ``False``. The cancel key is - preserved ONLY when the failure latched dispatch (``err.dispatched``): some - attempt in the retry sequence reached the wire, so the device may already be - ringing and a later Stop needs the key. Pure rejections (401/403/5xx/429 + only on HTTP 200). The cancel key is preserved ONLY when the failure latched + dispatch (``err.dispatched``): some attempt in the retry sequence reached the + wire, so the device may already be ringing and a later Stop needs the key. + Pure rejections (401/403/5xx/429 with no wire-reaching attempt) and provable pre-dispatch failures inherit ``dispatched is False`` and drop the key, so they can never overwrite a previous, possibly still-ringing play's valid cancel key. Centralizing the decision here keeps every non-acceptance exit on one rule instead of - re-deriving it per ``except`` handler. See ``NovaError.dispatched`` and - IRR-CA-CANCEL-KEY-ON-SUCCESS-ONLY. + re-deriving it per ``except`` handler. See ``NovaError.dispatched``, + IRR-CA-CANCEL-KEY-ON-SUCCESS-ONLY and IRR-CA-SOUND-FAILURE-CLASS. """ - return (False, request_uuid if err.dispatched else None) + return PlaySoundResult(outcome, cancel_key=request_uuid if err.dispatched else None) # Backward-compatible export for tests and legacy call sites. @@ -266,17 +276,20 @@ def stop_sound(self, device_id: str, request_uuid: str | None = None) -> bool: """Stop playing sound on the device (sync wrapper).""" ... - async def async_play_sound(self, device_id: str) -> tuple[bool, str | None]: + async def async_play_sound(self, device_id: str) -> PlaySoundResult: """Play a sound on the device (async). - Returns (success, request_uuid) tuple. + Returns the classification and the cancel key; see PlaySoundResult. """ ... async def async_stop_sound( self, device_id: str, request_uuid: str | None = None - ) -> bool: - """Stop playing sound on the device (async).""" + ) -> SoundDispatchOutcome: + """Stop playing sound on the device (async). + + Returns the classification; see SoundDispatchOutcome. + """ ... @@ -1550,11 +1563,15 @@ def can_play_sound(self, device_id: str) -> bool | None: def play_sound(self, device_id: str) -> bool: """Thin sync wrapper around async_play_sound for non-HA contexts. + The classification is deliberately dropped here: this entry point exists + for CLI use and has no consumer that could act on it. HA callers use + ``async_play_sound`` and read ``PlaySoundResult.outcome``. + Args: device_id: The canonical ID of the device. Returns: - True if the command was sent successfully, False otherwise. + True if Nova accepted the command (HTTP 200), False otherwise. """ result = self._run_sync_helper( lambda: self.async_play_sound(device_id), @@ -1562,19 +1579,22 @@ def play_sound(self, device_id: str) -> bool: "play_sound() called inside an active event loop; use async_play_sound()." ), context=f"play sound on {device_id}", - default=(False, None), + default=PlaySoundResult(SoundDispatchOutcome.INTERNAL_ERROR), ) - success, _request_uuid = cast(tuple[bool, str | None], result) - return success + return cast(PlaySoundResult, result).accepted def stop_sound(self, device_id: str, request_uuid: str | None = None) -> bool: """Thin sync wrapper around async_stop_sound for non-HA contexts. + The classification is deliberately dropped here, for the same reason as in + ``play_sound``: this entry point is for CLI use. HA callers use + ``async_stop_sound`` and read the ``SoundDispatchOutcome``. + Args: device_id: The canonical ID of the device. Returns: - True if the command was sent successfully, False otherwise. + True if Nova accepted the command (HTTP 200), False otherwise. """ result = self._run_sync_helper( lambda: self.async_stop_sound(device_id, request_uuid), @@ -1582,36 +1602,41 @@ def stop_sound(self, device_id: str, request_uuid: str | None = None) -> bool: "stop_sound() called inside an active event loop; use async_stop_sound()." ), context=f"stop sound on {device_id}", - default=False, + default=SoundDispatchOutcome.INTERNAL_ERROR, ) - return cast(bool, result) + return cast(SoundDispatchOutcome, result) is SoundDispatchOutcome.ACCEPTED # ---------- Play/Stop Sound (async; HA-first) ---------- - async def async_play_sound(self, device_id: str) -> tuple[bool, str | None]: + async def async_play_sound(self, device_id: str) -> PlaySoundResult: """Send a 'Play Sound' command to a device (async path for HA). Auth mapping note: - If an auth error occurs here, we log and return False (service call context), - since re-auth is primarily driven by the coordinator’s data update path. + If an auth error occurs here, we log and return REJECTED_AUTH (service + call context), since re-auth is primarily driven by the coordinator’s + data update path. Args: device_id: The canonical ID of the device. Returns: - A tuple `(success, request_uuid)` where `success` indicates whether the - command was *accepted* (HTTP 200) and `request_uuid` captures the - client-generated cancel key. The UUID is generated locally *before* + A `PlaySoundResult` carrying two independent facts. `outcome` names who + refused (see `SoundDispatchOutcome`); only `TRANSPORT_FAILED` describes + a broken push transport, and only that value may make a caller arm a + cooldown. `cancel_key` captures the client-generated cancel key and + answers one question only: may the device be ringing. Never derive the + cause from the key -- that out-of-band inference is what this type + removes. The UUID is generated locally *before* dispatch. It is returned (non-None) in two cases where the device may be ringing and a later Stop needs the key: - 1. Acceptance — the server answered 200. `success` is True. + 1. Acceptance — the server answered 200, `outcome` is `ACCEPTED`. 2. Post-dispatch ambiguity — a network failure occurred at or after the request reached the wire (server disconnect, read timeout, payload error), so the play may have started even though no 200 - was read. `success` is False, but the key is preserved. + was read. `outcome` is not `ACCEPTED`, but the key is preserved. Every failure that provably never reached the wire — no FCM token, missing cache, username/payload/token resolution, connection setup (DNS/connect refused/connect timeout) — and every explicit server - rejection (401/403/4xx/5xx/429) returns `(False, None)` so it cannot + rejection (401/403/4xx/5xx/429) returns a null `cancel_key` so it cannot overwrite a still-valid cancel key of a previous, possibly still-ringing play. The sole exception is a rejection/HTTP-status exit whose retry sequence latched dispatch on an *earlier* wire-reaching attempt: there @@ -1623,8 +1648,8 @@ async def async_play_sound(self, device_id: str) -> tuple[bool, str | None]: is additionally bound to "the cancel key was our own and fresh", so a foreign key passed in by a service call never evicts our handle. - True means Nova accepted the submission (HTTP 200). It is NOT a - confirmation that the device received or executed the command: no + `ACCEPTED` means Nova accepted the submission (HTTP 200). This is + NOT a confirmation that the device received or executed the command: no ExecuteActionResponse schema exists and no FCM callback is registered for sound, so nothing on this path can observe the ring. See IRR-CA-NO-RING-CONFIRMATION in @@ -1633,8 +1658,11 @@ async def async_play_sound(self, device_id: str) -> tuple[bool, str | None]: # Pass cache explicitly for multi-account isolation token = self._get_fcm_token_for_action() if not token: - # PRE-dispatch: nothing was sent, so there is no cancel key to keep. - return (False, None) + # PRE-dispatch: no transport was used at all, so there is no cancel key + # to keep and neither the server nor the network may be blamed. + # NOT_SENT keeps the caller from arming a push cooldown for a missing + # local token. + return PlaySoundResult(SoundDispatchOutcome.NOT_SENT) # Generate the cancel key locally *before* dispatch so it is in scope on # every path. Acceptance is still derived structurally from the submitter's # return contract: it returns a result tuple exclusively on an HTTP 200 and @@ -1669,9 +1697,11 @@ async def async_play_sound(self, device_id: str) -> tuple[bool, str | None]: ) # Defensive: the submitter returns a tuple on every accepted (200) # command and re-raises otherwise, so None is not an expected - # outcome. Treat the unconfirmed result conservatively — drop the - # key rather than risk overwriting a previous play's valid one. - return (False, None) + # outcome. That is a broken contract on our side, not an outage: + # INTERNAL_ERROR, never TRANSPORT_FAILED. Treat the unconfirmed + # result conservatively — drop the key rather than risk overwriting + # a previous play's valid one. + return PlaySoundResult(SoundDispatchOutcome.INTERNAL_ERROR) response_hex, _response_uuid = result _LOGGER.info("Play Sound (async) submitted successfully for %s", device_id) @@ -1682,7 +1712,9 @@ async def async_play_sound(self, device_id: str) -> tuple[bool, str | None]: len(response_hex) // 2 if response_hex else 0, response_hex[:200] if response_hex else "(empty)", ) - return (True, request_uuid) + return PlaySoundResult( + SoundDispatchOutcome.ACCEPTED, cancel_key=request_uuid + ) except NovaAuthError as err: _LOGGER.error( @@ -1695,9 +1727,21 @@ async def async_play_sound(self, device_id: str) -> tuple[bool, str | None]: # latched dispatch (err.dispatched): a pure 401/403 rejection with no # wire-reaching attempt drops it, so it can never overwrite a # previous, possibly still-ringing play's valid key. - return _cancel_key_after_failure(err, request_uuid) + # The server answered and refused: REJECTED_AUTH, not a transport + # failure. An expired sign-in must not be hidden behind a self-clearing + # 90-second cooldown. + return _play_result_after_failure( + err, request_uuid, SoundDispatchOutcome.REJECTED_AUTH + ) except NovaHTTPError as err: + # Non-acceptance (401/403/5xx/other): drop the key UNLESS an earlier + # attempt latched dispatch — a prior attempt that reached the server + # (a post-send network failure, or a 5xx/429 status read) may already + # be ringing. err.dispatched carries that sticky sequence latch + # (stamped at the transport's retry-loop choke point). Either way the + # server answered, so the transport worked and the outcome names the + # server, never the network. if getattr(err, "status", None) in (401, 403): _LOGGER.error( "Authentication failed (HTTP %s) while playing sound on %s: %s", @@ -1705,19 +1749,18 @@ async def async_play_sound(self, device_id: str) -> tuple[bool, str | None]: device_id, _short_err(err), ) - else: - _LOGGER.warning( - "Server error (%s) while playing sound on %s: %s", - err.status, - device_id, - _short_err(err), + return _play_result_after_failure( + err, request_uuid, SoundDispatchOutcome.REJECTED_AUTH ) - # Non-acceptance (401/403/5xx/other): drop the key UNLESS an earlier - # attempt latched dispatch — a prior attempt that reached the server - # (a post-send network failure, or a 5xx/429 status read) may already - # be ringing. err.dispatched carries that sticky sequence latch - # (stamped at the transport's retry-loop choke point). - return _cancel_key_after_failure(err, request_uuid) + _LOGGER.warning( + "Server error (%s) while playing sound on %s: %s", + err.status, + device_id, + _short_err(err), + ) + return _play_result_after_failure( + err, request_uuid, SoundDispatchOutcome.REJECTED_SERVER + ) except NovaRateLimitError as err: _LOGGER.warning( @@ -1725,8 +1768,40 @@ async def async_play_sound(self, device_id: str) -> tuple[bool, str | None]: ) # Same rule as the other non-acceptances: a rate-limited final attempt # never rang, but if a *prior* attempt reached the wire the latch - # (err.dispatched) preserves the key. - return _cancel_key_after_failure(err, request_uuid) + # (err.dispatched) preserves the key. The server answered; only the + # pace was wrong, so this is neither an outage nor a credential + # problem and gets its own value. + return _play_result_after_failure( + err, request_uuid, SoundDispatchOutcome.REJECTED_RATE_LIMIT + ) + + except NovaProtobufDecodeError as err: + # A 200 whose body we could not decode. The transport delivered; the + # payload broke on our side of the contract, so it is our bug, not an + # outage. Caught BEFORE `except NovaError` on purpose: that handler + # would otherwise classify it as TRANSPORT_FAILED and arm a cooldown + # for a decoding defect. + _LOGGER.error( + "Undecodable Play Sound response for %s: %s", + device_id, + _short_err(err), + exc_info=True, + ) + return _play_result_after_failure( + err, request_uuid, SoundDispatchOutcome.INTERNAL_ERROR + ) + + except NovaLogicError as err: + # HTTP 200 with an error code in the payload: the server answered and + # refused (unknown device, permission denied at API level). Same + # ordering rationale as above -- `except NovaError` would blame the + # network for a decision the server made. + _LOGGER.warning( + "Play Sound refused by Nova for %s: %s", device_id, _short_err(err) + ) + return _play_result_after_failure( + err, request_uuid, SoundDispatchOutcome.REJECTED_SERVER + ) except NovaError as err: # A network failure wrapped by async_nova_request after its retries, @@ -1735,9 +1810,9 @@ async def async_play_sound(self, device_id: str) -> tuple[bool, str | None]: # any attempt may have been processed by the server (a post-send # network failure, or a 5xx/429 status read), the play may already be # ringing, so keep the cancel key; a provable pre-connect failure - # never rang, so drop it. Other - # subclasses (NovaLogicError, NovaProtobufDecodeError) inherit - # dispatched=False and keep the conservative drop. + # never rang, so drop it. The two subclasses that are NOT transport + # failures (NovaLogicError, NovaProtobufDecodeError) are caught above, + # so what reaches this handler really is the transport giving up. if err.dispatched: _LOGGER.warning( "Play Sound for %s failed after reaching the server (%s); " @@ -1751,7 +1826,10 @@ async def async_play_sound(self, device_id: str) -> tuple[bool, str | None]: device_id, _short_err(err), ) - return _cancel_key_after_failure(err, request_uuid) + # The one outcome that justifies arming a push cooldown. + return _play_result_after_failure( + err, request_uuid, SoundDispatchOutcome.TRANSPORT_FAILED + ) except ClientError as err: # Defensive: async_nova_request wraps aiohttp errors into NovaError @@ -1763,22 +1841,29 @@ async def async_play_sound(self, device_id: str) -> tuple[bool, str | None]: device_id, _short_err(err), ) - return (False, None) + return PlaySoundResult(SoundDispatchOutcome.TRANSPORT_FAILED) except Exception as err: + # A bug on our side, not an outage. It gets a traceback and its own + # class, because reporting it as a transport failure is what put the + # integration into a self-inflicted 90-second cooldown. _LOGGER.error( - "Failed to play sound (async) on %s: %s", device_id, _short_err(err) + "Failed to play sound (async) on %s: %s", + device_id, + _short_err(err), + exc_info=True, ) - return (False, None) + return PlaySoundResult(SoundDispatchOutcome.INTERNAL_ERROR) async def async_stop_sound( self, device_id: str, request_uuid: str | None = None - ) -> bool: + ) -> SoundDispatchOutcome: """Send a 'Stop Sound' command to a device (async path for HA). Auth mapping note: - If an auth error occurs here, we log and return False (service call context), - since re-auth is primarily driven by the coordinator’s data update path. + If an auth error occurs here, we log and return REJECTED_AUTH (service + call context), since re-auth is primarily driven by the coordinator’s + data update path. Args: device_id: The canonical ID of the device. @@ -1790,9 +1875,13 @@ async def async_stop_sound( limitation; see StopSoundOutcome.UNCORRELATED. Returns: - True if the command was submitted successfully, False otherwise. + A `SoundDispatchOutcome` naming who refused, on the same contract as + `async_play_sound`. Only `TRANSPORT_FAILED` describes a broken push + transport and may make a caller arm a cooldown; a server rejection, a + rate limit, a missing local token and a bug of our own each keep their + own value. - True means Nova accepted the submission (HTTP 200). It is NOT a + `ACCEPTED` means Nova accepted the submission (HTTP 200). It is NOT a confirmation that the device received or executed the command, and in particular not that the ring stopped: no ExecuteActionResponse schema exists and no FCM callback is registered for sound, so @@ -1802,7 +1891,9 @@ async def async_stop_sound( # Pass cache explicitly for multi-account isolation token = self._get_fcm_token_for_action() if not token: - return False + # PRE-dispatch: no transport was used, so neither the server nor the + # network may be blamed for the missing local token. + return SoundDispatchOutcome.NOT_SENT # Idempotent guard, not a second normalisation policy: the coordinator # funnel already maps blank to None. This entry point is public and # documented "for non-HA contexts", so a blank string can still arrive @@ -1862,7 +1953,12 @@ async def async_stop_sound( "empty response from server (no error details available)", device_id, ) - return bool(submitted) + # The submitter returns a hex body on acceptance and re-raises + # otherwise, so an empty reply breaks its own contract: our bug, not + # an outage. + if submitted: + return SoundDispatchOutcome.ACCEPTED + return SoundDispatchOutcome.INTERNAL_ERROR except NovaAuthError as err: _LOGGER.error( @@ -1870,7 +1966,9 @@ async def async_stop_sound( device_id, _short_err(err), ) - return False + # The server answered and refused on credentials. The transport + # worked, so no cooldown may be armed for this. + return SoundDispatchOutcome.REJECTED_AUTH except NovaHTTPError as err: if getattr(err, "status", None) in (401, 403): @@ -1880,20 +1978,51 @@ async def async_stop_sound( device_id, _short_err(err), ) - return False + return SoundDispatchOutcome.REJECTED_AUTH _LOGGER.warning( "Server error (%s) while stopping sound on %s: %s", err.status, device_id, _short_err(err), ) - return False + return SoundDispatchOutcome.REJECTED_SERVER except NovaRateLimitError as err: _LOGGER.warning( "Stop Sound rate-limited for %s: %s", device_id, _short_err(err) ) - return False + # The server answered; only the pace was wrong. + return SoundDispatchOutcome.REJECTED_RATE_LIMIT + + except NovaProtobufDecodeError as err: + # Caught before `except NovaError` for the same reason as on the play + # path: an undecodable body is our defect, not a dead network. + _LOGGER.error( + "Undecodable Stop Sound response for %s: %s", + device_id, + _short_err(err), + exc_info=True, + ) + return SoundDispatchOutcome.INTERNAL_ERROR + + except NovaLogicError as err: + _LOGGER.warning( + "Stop Sound refused by Nova for %s: %s", device_id, _short_err(err) + ) + return SoundDispatchOutcome.REJECTED_SERVER + + except NovaError as err: + # A network failure wrapped by async_nova_request after its retries, + # or any other NovaError leaving the transport. Before the sound + # contract existed this fell through to `except Exception` and was + # reported as a plain False, indistinguishable from a server saying + # no. It is the one outcome that justifies arming a push cooldown. + _LOGGER.error( + "Network error while stopping sound on %s: %s", + device_id, + _short_err(err), + ) + return SoundDispatchOutcome.TRANSPORT_FAILED except ClientError as err: _LOGGER.error( @@ -1901,13 +2030,17 @@ async def async_stop_sound( device_id, _short_err(err), ) - return False + return SoundDispatchOutcome.TRANSPORT_FAILED except Exception as err: + # Our own bug: traceback, own class, and never a cooldown. _LOGGER.error( - "Failed to stop sound (async) on %s: %s", device_id, _short_err(err) + "Failed to stop sound (async) on %s: %s", + device_id, + _short_err(err), + exc_info=True, ) - return False + return SoundDispatchOutcome.INTERNAL_ERROR if ( diff --git a/custom_components/googlefindmy/coordinator/locate.py b/custom_components/googlefindmy/coordinator/locate.py index 514b14993..cbce29439 100644 --- a/custom_components/googlefindmy/coordinator/locate.py +++ b/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 @@ -955,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 diff --git a/docs/PLAY_SOUND_ARCHITECTURE.md b/docs/PLAY_SOUND_ARCHITECTURE.md index becdb17dc..69d4c9c73 100644 --- a/docs/PLAY_SOUND_ARCHITECTURE.md +++ b/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 diff --git a/tests/helpers/locate_mixin_stub.py b/tests/helpers/locate_mixin_stub.py index 9a24bac3d..c4d0732eb 100644 --- a/tests/helpers/locate_mixin_stub.py +++ b/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. diff --git a/tests/test_api_basics.py b/tests/test_api_basics.py index d2c3c4c72..29920f92f 100644 --- a/tests/test_api_basics.py +++ b/tests/test_api_basics.py @@ -39,6 +39,7 @@ CONTRIBUTOR_MODE_HIGH_TRAFFIC, CONTRIBUTOR_MODE_IN_ALL_AREAS, DEFAULT_CONTRIBUTOR_MODE, + SoundDispatchOutcome, ) from custom_components.googlefindmy.exceptions import MissingTokenCacheError from custom_components.googlefindmy.NovaApi.ExecuteAction.LocateTracker.decrypt_locations import ( @@ -1215,7 +1216,8 @@ def test_missing_token_short_circuits(self) -> None: # PRE-dispatch guard: nothing was sent, so there is no cancel key to keep. api_module._FCM_ReceiverGetter = None api = GoogleFindMyAPI(cache=StubCache(entry_id="e")) - assert run_coro(api.async_play_sound("d")) == (False, None) + result = run_coro(api.async_play_sound("d")) + assert (result.accepted, result.cancel_key) == (False, None) def test_empty_submission_drops_cancel_key( self, monkeypatch: pytest.MonkeyPatch @@ -1227,7 +1229,8 @@ def test_empty_submission_drops_cancel_key( api = self._api_with_token(monkeypatch) self._patch_generate_uuid(monkeypatch) self._patch_submit(monkeypatch, None) - assert run_coro(api.async_play_sound("d")) == (False, None) + result = run_coro(api.async_play_sound("d")) + assert (result.accepted, result.cancel_key) == (False, None) def test_success_returns_injected_uuid( self, monkeypatch: pytest.MonkeyPatch @@ -1244,7 +1247,8 @@ async def _echo_submit(*_a: Any, **_k: Any) -> Any: monkeypatch.setattr( api_module, "async_submit_start_sound_request", _echo_submit ) - assert run_coro(api.async_play_sound("d")) == (True, "uuid-injected") + result = run_coro(api.async_play_sound("d")) + assert (result.accepted, result.cancel_key) == (True, "uuid-injected") @pytest.mark.parametrize( "exc", @@ -1283,7 +1287,8 @@ def test_any_submitter_exception_drops_cancel_key( api = self._api_with_token(monkeypatch) self._patch_generate_uuid(monkeypatch) self._patch_submit(monkeypatch, None, raises=exc) - assert run_coro(api.async_play_sound("d")) == (False, None) + result = run_coro(api.async_play_sound("d")) + assert (result.accepted, result.cancel_key) == (False, None) def test_post_dispatch_network_failure_keeps_cancel_key( self, monkeypatch: pytest.MonkeyPatch @@ -1299,7 +1304,8 @@ def test_post_dispatch_network_failure_keeps_cancel_key( err = NovaError("network failed after retries") err.dispatched = True self._patch_submit(monkeypatch, None, raises=err) - assert run_coro(api.async_play_sound("d")) == (False, "uuid-injected") + result = run_coro(api.async_play_sound("d")) + assert (result.accepted, result.cancel_key) == (False, "uuid-injected") def test_pre_dispatch_network_failure_drops_cancel_key( self, monkeypatch: pytest.MonkeyPatch @@ -1312,7 +1318,104 @@ def test_pre_dispatch_network_failure_drops_cancel_key( err = NovaError("connect failed before the wire") err.dispatched = False self._patch_submit(monkeypatch, None, raises=err) - assert run_coro(api.async_play_sound("d")) == (False, None) + result = run_coro(api.async_play_sound("d")) + assert (result.accepted, result.cancel_key) == (False, None) + + @pytest.mark.parametrize( + ("raised", "expected"), + [ + (NovaAuthError(401, "expired"), SoundDispatchOutcome.REJECTED_AUTH), + (NovaHTTPError(403, "forbidden"), SoundDispatchOutcome.REJECTED_AUTH), + (NovaHTTPError(503, "unavailable"), SoundDispatchOutcome.REJECTED_SERVER), + (NovaRateLimitError("slow down"), SoundDispatchOutcome.REJECTED_RATE_LIMIT), + (NovaLogicError(3, "logic"), SoundDispatchOutcome.REJECTED_SERVER), + ( + NovaProtobufDecodeError("garbage"), + SoundDispatchOutcome.INTERNAL_ERROR, + ), + (NovaError("socket died"), SoundDispatchOutcome.TRANSPORT_FAILED), + (ClientError("pre-dispatch"), SoundDispatchOutcome.TRANSPORT_FAILED), + (ValueError("our own bug"), SoundDispatchOutcome.INTERNAL_ERROR), + ], + ) + def test_play_sound_classifies_every_exit( + self, + monkeypatch: pytest.MonkeyPatch, + raised: Exception, + expected: SoundDispatchOutcome, + ) -> None: + """Every exit of async_play_sound must name its own cause. + + A server saying no, a network that never answered and a bug on our side + used to be the same ``(False, None)``. The coordinator read that as a + push transport problem and armed a 90-second cooldown for all three. + """ + + api = self._api_with_token(monkeypatch) + self._patch_generate_uuid(monkeypatch) + self._patch_submit(monkeypatch, None, raises=raised) + assert run_coro(api.async_play_sound("d")).outcome is expected + + def test_missing_action_token_is_not_sent(self) -> None: + """No FCM token means no transport was used, so nobody may be blamed.""" + + api_module._FCM_ReceiverGetter = None + api = GoogleFindMyAPI(cache=StubCache(entry_id="e")) + result = run_coro(api.async_play_sound("d")) + assert result.outcome is SoundDispatchOutcome.NOT_SENT + assert result.cancel_key is None + + def test_empty_submitter_reply_is_internal_error( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """The submitter returns a tuple on 200 and re-raises otherwise. + + A None therefore breaks its own contract: our bug, not an outage. + """ + + api = self._api_with_token(monkeypatch) + self._patch_generate_uuid(monkeypatch) + self._patch_submit(monkeypatch, None) + result = run_coro(api.async_play_sound("d")) + assert result.outcome is SoundDispatchOutcome.INTERNAL_ERROR + assert result.cancel_key is None + + def test_http_200_is_accepted(self, monkeypatch: pytest.MonkeyPatch) -> None: + """Acceptance carries the cancel key, and only acceptance sets accepted.""" + + api = self._api_with_token(monkeypatch) + self._patch_generate_uuid(monkeypatch) + + async def _echo_submit(*_a: Any, **_k: Any) -> Any: + return ("AB", _k.get("request_uuid")) + + monkeypatch.setattr( + api_module, "async_submit_start_sound_request", _echo_submit + ) + result = run_coro(api.async_play_sound("d")) + assert result.outcome is SoundDispatchOutcome.ACCEPTED + assert result.accepted is True + assert result.cancel_key == "uuid-injected" + + def test_dispatched_transport_failure_keeps_key_and_names_the_transport( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """Outcome and cancel key are two independent facts, not one. + + The key answers "may the device be ringing", the outcome answers "who + refused". Reading the cause off the key is the out-of-band channel that + PlaySoundResult replaces. + """ + + api = self._api_with_token(monkeypatch) + self._patch_generate_uuid(monkeypatch) + err = NovaError("network failed after retries") + err.dispatched = True + self._patch_submit(monkeypatch, None, raises=err) + result = run_coro(api.async_play_sound("d")) + assert result.outcome is SoundDispatchOutcome.TRANSPORT_FAILED + assert result.cancel_key == "uuid-injected" + assert result.accepted is False class TestAsyncStopSoundErrorMapping: @@ -1340,17 +1443,26 @@ async def _submit(*_a: Any, **_k: Any) -> Any: def test_missing_token_short_circuits(self) -> None: api_module._FCM_ReceiverGetter = None api = GoogleFindMyAPI(cache=StubCache(entry_id="e")) - assert run_coro(api.async_stop_sound("d")) is False + # No transport was used, so neither the server nor the network is at fault. + assert run_coro(api.async_stop_sound("d")) is SoundDispatchOutcome.NOT_SENT def test_none_response_returns_false(self, monkeypatch: pytest.MonkeyPatch) -> None: api = self._api_with_token(monkeypatch) self._patch_submit(monkeypatch, None) - assert run_coro(api.async_stop_sound("d", "uuid-1234")) is False + # The submitter re-raises on every non-acceptance, so an empty reply is a + # broken contract on our side, not an outage. + assert ( + run_coro(api.async_stop_sound("d", "uuid-1234")) + is SoundDispatchOutcome.INTERNAL_ERROR + ) def test_success_returns_true(self, monkeypatch: pytest.MonkeyPatch) -> None: api = self._api_with_token(monkeypatch) self._patch_submit(monkeypatch, "CDEF") - assert run_coro(api.async_stop_sound("d", "uuid-5678")) is True + assert ( + run_coro(api.async_stop_sound("d", "uuid-5678")) + is SoundDispatchOutcome.ACCEPTED + ) def test_stop_without_uuid_does_not_claim_success( self, monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture @@ -1367,7 +1479,7 @@ def test_stop_without_uuid_does_not_claim_success( self._patch_submit(monkeypatch, "CDEF") with caplog.at_level(logging.DEBUG): - assert run_coro(api.async_stop_sound("d")) is True + assert run_coro(api.async_stop_sound("d")) is SoundDispatchOutcome.ACCEPTED assert "successfully" not in caplog.text warnings = [ @@ -1387,7 +1499,10 @@ def test_stop_with_uuid_logs_the_cancel_key_branch( self._patch_submit(monkeypatch, "CDEF") with caplog.at_level(logging.DEBUG): - assert run_coro(api.async_stop_sound("d", "uuid-5678")) is True + assert ( + run_coro(api.async_stop_sound("d", "uuid-5678")) + is SoundDispatchOutcome.ACCEPTED + ) assert "cancel key present" in caplog.text assert "without a cancel key" not in caplog.text @@ -1412,25 +1527,44 @@ def test_blank_uuid_logs_as_uncorrelated_not_as_a_key( self._patch_submit(monkeypatch, "CDEF") with caplog.at_level(logging.DEBUG): - assert run_coro(api.async_stop_sound("d", blank)) is True + assert ( + run_coro(api.async_stop_sound("d", blank)) + is SoundDispatchOutcome.ACCEPTED + ) assert "cancel key present" not in caplog.text assert "without a cancel key" in caplog.text @pytest.mark.parametrize( - "exc", + ("exc", "expected"), [ - NovaAuthError(401, "auth"), - NovaHTTPError(403, "http"), - NovaHTTPError(500, "http"), - NovaRateLimitError("rate"), - ClientError("net"), - Exception("boom"), + (NovaAuthError(401, "auth"), SoundDispatchOutcome.REJECTED_AUTH), + (NovaHTTPError(403, "http"), SoundDispatchOutcome.REJECTED_AUTH), + (NovaHTTPError(500, "http"), SoundDispatchOutcome.REJECTED_SERVER), + (NovaRateLimitError("rate"), SoundDispatchOutcome.REJECTED_RATE_LIMIT), + (NovaLogicError(3, "logic"), SoundDispatchOutcome.REJECTED_SERVER), + ( + NovaProtobufDecodeError("garbage"), + SoundDispatchOutcome.INTERNAL_ERROR, + ), + (NovaError("socket died"), SoundDispatchOutcome.TRANSPORT_FAILED), + (ClientError("net"), SoundDispatchOutcome.TRANSPORT_FAILED), + (Exception("boom"), SoundDispatchOutcome.INTERNAL_ERROR), ], ) - def test_documented_exceptions_return_false( - self, monkeypatch: pytest.MonkeyPatch, exc: BaseException + def test_documented_exceptions_are_classified( + self, + monkeypatch: pytest.MonkeyPatch, + exc: BaseException, + expected: SoundDispatchOutcome, ) -> None: + """Stop carries the same classification contract as Play. + + None of these is an acceptance, but only the two transport rows may make + a caller arm a push cooldown. Before the contract existed all nine + collapsed into a single ``False``. + """ + api = self._api_with_token(monkeypatch) self._patch_submit(monkeypatch, None, raises=exc) - assert run_coro(api.async_stop_sound("d")) is False + assert run_coro(api.async_stop_sound("d")) is expected diff --git a/tests/test_api_fcm_token_scoping.py b/tests/test_api_fcm_token_scoping.py index 93b159484..29cc579d7 100644 --- a/tests/test_api_fcm_token_scoping.py +++ b/tests/test_api_fcm_token_scoping.py @@ -10,6 +10,7 @@ from custom_components.googlefindmy import api as api_module from custom_components.googlefindmy.api import GoogleFindMyAPI +from custom_components.googlefindmy.const import SoundDispatchOutcome @dataclass @@ -137,14 +138,22 @@ async def fake_submit_stop( ) async def _exercise() -> None: - ok1, uuid1 = await api_entry_1.async_play_sound("device-1") - ok2, uuid2 = await api_entry_2.async_play_sound("device-2") + play1 = await api_entry_1.async_play_sound("device-1") + play2 = await api_entry_2.async_play_sound("device-2") + ok1, uuid1 = play1.accepted, play1.cancel_key + ok2, uuid2 = play2.accepted, play2.cancel_key assert ok1 and ok2 # Each play generates its own client-side cancel key (non-None, distinct). assert uuid1 and uuid2 assert uuid1 != uuid2 - assert await api_entry_1.async_stop_sound("device-1") - assert await api_entry_2.async_stop_sound("device-2") + assert ( + await api_entry_1.async_stop_sound("device-1") + is SoundDispatchOutcome.ACCEPTED + ) + assert ( + await api_entry_2.async_stop_sound("device-2") + is SoundDispatchOutcome.ACCEPTED + ) asyncio.run(_exercise()) @@ -223,10 +232,14 @@ async def fake_submit_stop( monkeypatch.setattr(api, "_get_fcm_token_for_action", lambda: "tok-uuid") async def _exercise() -> None: - success, request_uuid = await api.async_play_sound("device-uuid") + play = await api.async_play_sound("device-uuid") + success, request_uuid = play.accepted, play.cancel_key assert success is True assert request_uuid == "uuid-device-uuid" - assert await api.async_stop_sound("device-uuid", request_uuid) + assert ( + await api.async_stop_sound("device-uuid", request_uuid) + is SoundDispatchOutcome.ACCEPTED + ) asyncio.run(_exercise()) diff --git a/tests/test_api_location_selection.py b/tests/test_api_location_selection.py index 0779d7ee5..f833d61a7 100644 --- a/tests/test_api_location_selection.py +++ b/tests/test_api_location_selection.py @@ -11,6 +11,10 @@ import custom_components.googlefindmy.api as api_module from custom_components.googlefindmy.api import GoogleFindMyAPI +from custom_components.googlefindmy.const import ( + PlaySoundResult, + SoundDispatchOutcome, +) from tests.helpers import drain_loop @@ -53,15 +57,15 @@ async def async_get_device_location( self.calls.append(("loc", (device_id, device_name))) return {"id": device_id, "name": device_name} - async def async_play_sound(self, device_id: str) -> tuple[bool, str | None]: + async def async_play_sound(self, device_id: str) -> PlaySoundResult: self.calls.append(("play", (device_id,))) - return True, "uuid-play" + return PlaySoundResult(SoundDispatchOutcome.ACCEPTED, "uuid-play") async def async_stop_sound( self, device_id: str, request_uuid: str | None = None - ) -> bool: + ) -> SoundDispatchOutcome: self.calls.append(("stop", (device_id, request_uuid))) - return True + return SoundDispatchOutcome.ACCEPTED class _LoopCaptureHarness(GoogleFindMyAPI): diff --git a/tests/test_coordinator_locate_basics.py b/tests/test_coordinator_locate_basics.py index 832f8ef55..4dbad7d71 100644 --- a/tests/test_coordinator_locate_basics.py +++ b/tests/test_coordinator_locate_basics.py @@ -21,6 +21,8 @@ from custom_components.googlefindmy.const import ( DEFAULT_MIN_POLL_INTERVAL, + PlaySoundResult, + SoundDispatchOutcome, StopSoundOutcome, ) from custom_components.googlefindmy.NovaApi.ExecuteAction.LocateTracker.decrypt_locations import ( @@ -378,7 +380,9 @@ async def test_success_stores_uuid(self, coord: LocateStub) -> None: async def test_failure_notes_problem(self, coord: LocateStub) -> None: coord._device_caps["dev-1"] = {"can_ring": True} - coord.api.async_play_sound.return_value = (False, None) + coord.api.async_play_sound.return_value = PlaySoundResult( + SoundDispatchOutcome.TRANSPORT_FAILED + ) ok = await coord.async_play_sound("dev-1") assert ok is False coord._note_push_transport_problem.assert_called_once() @@ -403,7 +407,9 @@ async def test_failed_play_does_not_clear_auth_state( """ coord._device_caps["dev-1"] = {"can_ring": True} - coord.api.async_play_sound.return_value = (False, None) + coord.api.async_play_sound.return_value = PlaySoundResult( + SoundDispatchOutcome.TRANSPORT_FAILED + ) assert await coord.async_play_sound("dev-1") is False @@ -445,7 +451,7 @@ async def test_rejected_submission_is_failed_not_suppressed( user with an expired sign-in to wait a moment. """ - coord.api.async_stop_sound.return_value = False + coord.api.async_stop_sound.return_value = SoundDispatchOutcome.TRANSPORT_FAILED outcome = await coord.async_stop_sound("dev-1") assert outcome is StopSoundOutcome.FAILED coord.api.async_stop_sound.assert_awaited_once() @@ -498,7 +504,7 @@ async def test_missing_uuid_reports_uncorrelated(self, coord: LocateStub) -> Non coord.api.async_stop_sound.assert_awaited_once_with("dev-1", None) async def test_failure_notes_problem(self, coord: LocateStub) -> None: - coord.api.async_stop_sound.return_value = False + coord.api.async_stop_sound.return_value = SoundDispatchOutcome.TRANSPORT_FAILED outcome = await coord.async_stop_sound("dev-1", request_uuid="x") assert outcome is StopSoundOutcome.FAILED coord._note_push_transport_problem.assert_called_once() @@ -509,7 +515,7 @@ async def test_failed_stop_keeps_a_fresh_cancel_key( """IRR-CA-CANCEL-KEY-ON-SUCCESS-ONLY: a rejected stop spends nothing.""" coord._sound_request_uuids["dev-1"] = "cached-uuid" - coord.api.async_stop_sound.return_value = False + coord.api.async_stop_sound.return_value = SoundDispatchOutcome.TRANSPORT_FAILED outcome = await coord.async_stop_sound("dev-1") assert outcome is StopSoundOutcome.FAILED assert coord._sound_request_uuids["dev-1"] == "cached-uuid" diff --git a/tests/test_coordinator_sound_uuid.py b/tests/test_coordinator_sound_uuid.py index e7fe0704e..3f94e6b22 100644 --- a/tests/test_coordinator_sound_uuid.py +++ b/tests/test_coordinator_sound_uuid.py @@ -7,7 +7,11 @@ import pytest -from custom_components.googlefindmy.const import StopSoundOutcome +from custom_components.googlefindmy.const import ( + PlaySoundResult, + SoundDispatchOutcome, + StopSoundOutcome, +) from custom_components.googlefindmy.coordinator import GoogleFindMyCoordinator from custom_components.googlefindmy.coordinator.helpers.cache import ( SOUND_UUID_MAX_AGE_S, @@ -27,9 +31,9 @@ async def test_async_play_sound_stores_uuid() -> None: api_calls: list[SimpleNamespace] = [] - async def _async_play_sound(device_id: str) -> tuple[bool, str]: + async def _async_play_sound(device_id: str) -> PlaySoundResult: api_calls.append(SimpleNamespace(device_id=device_id)) - return True, "uuid-1" + return PlaySoundResult(SoundDispatchOutcome.ACCEPTED, "uuid-1") coordinator.api = SimpleNamespace(async_play_sound=_async_play_sound) # type: ignore[attr-defined] @@ -61,9 +65,9 @@ async def test_async_play_sound_skips_store_on_non_accepted_play() -> None: ) coordinator._set_auth_state = lambda **kwargs: None # type: ignore[attr-defined] - async def _async_play_sound(device_id: str) -> tuple[bool, str | None]: + async def _async_play_sound(device_id: str) -> PlaySoundResult: # Non-accepted play (pre-dispatch guard / rejection): no cancel key. - return False, None + return PlaySoundResult(SoundDispatchOutcome.TRANSPORT_FAILED) coordinator.api = SimpleNamespace(async_play_sound=_async_play_sound) # type: ignore[attr-defined] @@ -95,10 +99,10 @@ async def test_non_accepted_play_keeps_existing_cancel_key() -> None: coordinator._note_push_transport_problem = lambda: None # type: ignore[attr-defined] coordinator._set_auth_state = lambda **kwargs: None # type: ignore[attr-defined] - async def _async_play_sound(device_id: str) -> tuple[bool, str | None]: + async def _async_play_sound(device_id: str) -> PlaySoundResult: # New attempt not accepted (e.g. server 401/403, or pre-dispatch): the # success-only contract yields (False, None) — no cancel key. - return False, None + return PlaySoundResult(SoundDispatchOutcome.TRANSPORT_FAILED) coordinator.api = SimpleNamespace(async_play_sound=_async_play_sound) # type: ignore[attr-defined] @@ -130,10 +134,10 @@ async def test_post_dispatch_ambiguous_play_caches_uuid() -> None: ) coordinator._set_auth_state = lambda **kwargs: None # type: ignore[attr-defined] - async def _async_play_sound(device_id: str) -> tuple[bool, str | None]: + async def _async_play_sound(device_id: str) -> PlaySoundResult: # Post-dispatch ambiguity: not confirmed (ok=False), but a ring may be # active, so the cancel key is returned for caching. - return False, "uuid-postsend" + return PlaySoundResult(SoundDispatchOutcome.TRANSPORT_FAILED, "uuid-postsend") coordinator.api = SimpleNamespace(async_play_sound=_async_play_sound) # type: ignore[attr-defined] @@ -170,9 +174,11 @@ async def test_ambiguous_play_does_not_overwrite_known_cancel_key() -> None: ) coordinator._set_auth_state = lambda **kwargs: None # type: ignore[attr-defined] - async def _async_play_sound(device_id: str) -> tuple[bool, str | None]: + async def _async_play_sound(device_id: str) -> PlaySoundResult: # Ambiguous transient 5xx: not accepted (ok=False), fresh UUID present. - return False, "uuid-ambiguous-new" + return PlaySoundResult( + SoundDispatchOutcome.TRANSPORT_FAILED, "uuid-ambiguous-new" + ) coordinator.api = SimpleNamespace(async_play_sound=_async_play_sound) # type: ignore[attr-defined] @@ -201,8 +207,10 @@ async def test_ambiguous_play_keeps_fresh_tracked_cancel_key() -> None: coordinator._note_push_transport_problem = lambda: None # type: ignore[attr-defined] coordinator._set_auth_state = lambda **kwargs: None # type: ignore[attr-defined] - async def _async_play_sound(device_id: str) -> tuple[bool, str | None]: - return False, "uuid-ambiguous-new" + async def _async_play_sound(device_id: str) -> PlaySoundResult: + return PlaySoundResult( + SoundDispatchOutcome.TRANSPORT_FAILED, "uuid-ambiguous-new" + ) coordinator.api = SimpleNamespace(async_play_sound=_async_play_sound) # type: ignore[attr-defined] @@ -237,8 +245,10 @@ async def test_ambiguous_play_replaces_expired_cancel_key() -> None: ) coordinator._set_auth_state = lambda **kwargs: None # type: ignore[attr-defined] - async def _async_play_sound(device_id: str) -> tuple[bool, str | None]: - return False, "uuid-fresh-ambiguous" + async def _async_play_sound(device_id: str) -> PlaySoundResult: + return PlaySoundResult( + SoundDispatchOutcome.TRANSPORT_FAILED, "uuid-fresh-ambiguous" + ) coordinator.api = SimpleNamespace(async_play_sound=_async_play_sound) # type: ignore[attr-defined] @@ -277,9 +287,11 @@ async def test_async_stop_sound_sends_expired_cached_uuid_uncorrelated() -> None api_calls: list[tuple[str, str | None]] = [] - async def _async_stop_sound(device_id: str, request_uuid: str | None) -> bool: + async def _async_stop_sound( + device_id: str, request_uuid: str | None + ) -> SoundDispatchOutcome: api_calls.append((device_id, request_uuid)) - return True + return SoundDispatchOutcome.ACCEPTED coordinator.api = SimpleNamespace(async_stop_sound=_async_stop_sound) # type: ignore[attr-defined] @@ -317,9 +329,11 @@ async def _save() -> None: api_calls: list[tuple[str, str | None]] = [] - async def _async_stop_sound(device_id: str, request_uuid: str | None) -> bool: + async def _async_stop_sound( + device_id: str, request_uuid: str | None + ) -> SoundDispatchOutcome: api_calls.append((device_id, request_uuid)) - return True + return SoundDispatchOutcome.ACCEPTED coordinator.api = SimpleNamespace(async_stop_sound=_async_stop_sound) # type: ignore[attr-defined] @@ -348,8 +362,10 @@ async def test_ambiguous_play_keeps_untracked_cancel_key() -> None: coordinator._note_push_transport_problem = lambda: None # type: ignore[attr-defined] coordinator._set_auth_state = lambda **kwargs: None # type: ignore[attr-defined] - async def _async_play_sound(device_id: str) -> tuple[bool, str | None]: - return False, "uuid-ambiguous-new" + async def _async_play_sound(device_id: str) -> PlaySoundResult: + return PlaySoundResult( + SoundDispatchOutcome.TRANSPORT_FAILED, "uuid-ambiguous-new" + ) coordinator.api = SimpleNamespace(async_play_sound=_async_play_sound) # type: ignore[attr-defined] @@ -389,9 +405,11 @@ async def test_async_stop_sound_uses_cached_uuid() -> None: api_calls: list[tuple[str, str | None]] = [] - async def _async_stop_sound(device_id: str, request_uuid: str | None) -> bool: + async def _async_stop_sound( + device_id: str, request_uuid: str | None + ) -> SoundDispatchOutcome: api_calls.append((device_id, request_uuid)) - return True + return SoundDispatchOutcome.ACCEPTED coordinator.api = SimpleNamespace(async_stop_sound=_async_stop_sound) # type: ignore[attr-defined] @@ -416,9 +434,11 @@ async def test_async_stop_sound_warns_when_uuid_missing( api_calls: list[tuple[str, str | None]] = [] - async def _async_stop_sound(device_id: str, request_uuid: str | None) -> bool: + async def _async_stop_sound( + device_id: str, request_uuid: str | None + ) -> SoundDispatchOutcome: api_calls.append((device_id, request_uuid)) - return True + return SoundDispatchOutcome.ACCEPTED coordinator.api = SimpleNamespace(async_stop_sound=_async_stop_sound) # type: ignore[attr-defined] @@ -456,9 +476,11 @@ async def _save() -> None: coordinator._async_save_sound_uuids = _save # type: ignore[attr-defined] - async def _api_stop(device_id: str, request_uuid: str | None) -> bool: + async def _api_stop( + device_id: str, request_uuid: str | None + ) -> SoundDispatchOutcome: sent.append(request_uuid) - return True + return SoundDispatchOutcome.ACCEPTED coordinator.api = SimpleNamespace(async_stop_sound=_api_stop) # type: ignore[attr-defined] @@ -482,9 +504,11 @@ async def test_blank_request_uuid_without_a_cached_key_is_uncorrelated() -> None coordinator._note_push_transport_problem = lambda: None # type: ignore[attr-defined] coordinator._set_auth_state = lambda **kwargs: None # type: ignore[attr-defined] - async def _api_stop(device_id: str, request_uuid: str | None) -> bool: + async def _api_stop( + device_id: str, request_uuid: str | None + ) -> SoundDispatchOutcome: sent.append(request_uuid) - return True + return SoundDispatchOutcome.ACCEPTED coordinator.api = SimpleNamespace(async_stop_sound=_api_stop) # type: ignore[attr-defined] @@ -522,9 +546,11 @@ async def _save() -> None: sent: list[str | None] = [] - async def _api_stop(device_id: str, request_uuid: str | None) -> bool: + async def _api_stop( + device_id: str, request_uuid: str | None + ) -> SoundDispatchOutcome: sent.append(request_uuid) - return True + return SoundDispatchOutcome.ACCEPTED coordinator.api = SimpleNamespace(async_stop_sound=_api_stop) # type: ignore[attr-defined] @@ -560,11 +586,13 @@ async def _save() -> None: coordinator._async_save_sound_uuids = _save # type: ignore[attr-defined] - async def _async_stop_sound(device_id: str, request_uuid: str | None) -> bool: + async def _async_stop_sound( + device_id: str, request_uuid: str | None + ) -> SoundDispatchOutcome: # A Play completes while the stop is in flight. coordinator._sound_request_uuids[device_id] = "uuid-new" # type: ignore[attr-defined] coordinator._sound_request_timestamps[device_id] = time.time() # type: ignore[attr-defined] - return True + return SoundDispatchOutcome.ACCEPTED coordinator.api = SimpleNamespace(async_stop_sound=_async_stop_sound) # type: ignore[attr-defined] @@ -592,8 +620,10 @@ async def test_a_rejected_stop_does_not_clear_the_auth_failure_state() -> None: auth_calls: list[dict[str, object]] = [] coordinator._set_auth_state = lambda **kwargs: auth_calls.append(kwargs) # type: ignore[attr-defined] - async def _async_stop_sound(device_id: str, request_uuid: str | None) -> bool: - return False + async def _async_stop_sound( + device_id: str, request_uuid: str | None + ) -> SoundDispatchOutcome: + return SoundDispatchOutcome.TRANSPORT_FAILED coordinator.api = SimpleNamespace(async_stop_sound=_async_stop_sound) # type: ignore[attr-defined] @@ -622,8 +652,10 @@ async def test_an_explicit_key_that_is_ours_but_aged_is_not_called_foreign( coordinator._note_push_transport_problem = lambda: None # type: ignore[attr-defined] coordinator._set_auth_state = lambda **kwargs: None # type: ignore[attr-defined] - async def _async_stop_sound(device_id: str, request_uuid: str | None) -> bool: - return True + async def _async_stop_sound( + device_id: str, request_uuid: str | None + ) -> SoundDispatchOutcome: + return SoundDispatchOutcome.ACCEPTED coordinator.api = SimpleNamespace(async_stop_sound=_async_stop_sound) # type: ignore[attr-defined] diff --git a/tests/test_options_flow_credentials_cache.py b/tests/test_options_flow_credentials_cache.py index 4ae96604c..698b95966 100644 --- a/tests/test_options_flow_credentials_cache.py +++ b/tests/test_options_flow_credentials_cache.py @@ -28,6 +28,7 @@ SUBENTRY_TYPE_SERVICE, SUBENTRY_TYPE_TRACKER, TRACKER_SUBENTRY_KEY, + SoundDispatchOutcome, ) from custom_components.googlefindmy.NovaApi.ExecuteAction.PlaySound import ( start_sound_request as start_module, @@ -492,10 +493,14 @@ async def _fake_stop(scope: str, payload: str, **kwargs: Any) -> str: monkeypatch.setattr(start_module, "async_nova_request", _fake_start) monkeypatch.setattr(stop_module, "async_nova_request", _fake_stop) - success, request_uuid = await api_primary.async_play_sound("device-42") + play = await api_primary.async_play_sound("device-42") + success, request_uuid = play.accepted, play.cancel_key assert success is True assert request_uuid is not None - assert await api_primary.async_stop_sound("device-42", request_uuid) + assert ( + await api_primary.async_stop_sound("device-42", request_uuid) + is SoundDispatchOutcome.ACCEPTED + ) assert start_calls and stop_calls diff --git a/tests/test_stop_sound_correlation.py b/tests/test_stop_sound_correlation.py index eace0def4..9faf5c3c7 100644 --- a/tests/test_stop_sound_correlation.py +++ b/tests/test_stop_sound_correlation.py @@ -193,8 +193,9 @@ def test_api_docstrings_declare_no_ring_confirmation() -> None: Drift guard in the style of the FMDN constant checks in tests/test_ble_battery_sensor.py: a boundary that lives only in a design - document gets re-discovered as a bug. Both public sound entry points return - a plain bool, which is exactly the shape that invites "True means it + document gets re-discovered as a bug. Both public sound entry points now + return a classification instead of a bool, but ACCEPTED still only means + "Nova took the submission", which is exactly the shape that invites "so it stopped", so each one carries the disclaimer in its own Returns block. """ From dd950f0bc715568cd3864a8934bde9652842ba88 Mon Sep 17 00:00:00 2001 From: jleinenbach <1786119+jleinenbach@users.noreply.github.com> Date: Tue, 25 Aug 2026 22:06:28 +0000 Subject: [PATCH 4/7] fix(sound): arm the push cooldown only for a transport failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `SoundDispatchOutcome` documents that only `TRANSPORT_FAILED` justifies arming the push cooldown (const.py, "This is the ONLY outcome that justifies arming the push cooldown"). The coordinator did not act on it: it unpacked the result back down to `accepted`/`cancel_key` and kept deciding on `if not ok`, so a 401/403, a 429, a missing local action token and a bug of our own each still armed the 90-second cooldown, flipped the integration to `FcmStatus.DEGRADED` and made `can_play_sound()` report the button as unavailable -- an outage the integration inflicted on itself over a network that was working, and one that then also suppressed the user's follow-up Stop. Contract stated, contract not enforced, which is what AGENTS.md Rule §9.DOC forbids. Both sound paths now read `outcome`. The blanket `except Exception` handlers stop arming the cooldown as well: `api.async_play_sound` and `api.async_stop_sound` classify every unexpected exception in band and return `INTERNAL_ERROR` rather than raising, the only code before their `try` is the fully guarded `_get_fcm_token_for_action()`, and the one `await` after the call inside the coordinator's own `try` (`_async_save_sound_uuids`) swallows its own errors. Nothing that reaches those handlers came from the push transport. The two typed handlers for `TimeoutError`/`ClientConnectionError`/`ClientError` keep the cooldown on purpose: `api` is a Protocol, and an aiohttp error from an implementation that does not wrap it really is a transport failure. Red probe before the fix: 12 failures against the unchanged production code, `assert True is False` on `_note_push_transport_problem.called` for `rejected_auth`, `rejected_rate_limit`, `rejected_server`, `not_sent` and `internal_error` on both paths, plus `Expected 'mock' to not have been called` in the two exception tests. The four cases that were already correct (`accepted`, `transport_failed` per path) stayed green, so the red is the defect and not a missing test name. The two case tables carry one row per enum member and are pinned to `set(SoundDispatchOutcome)` by their own guard tests, so a member added later cannot silently inherit whatever the cascade happens to do. Reported by Codex on 72e4602f. --- .../googlefindmy/coordinator/locate.py | 73 ++++++---- docs/PLAY_SOUND_ARCHITECTURE.md | 16 ++ tests/test_coordinator_locate_basics.py | 137 +++++++++++++++++- 3 files changed, 197 insertions(+), 29 deletions(-) diff --git a/custom_components/googlefindmy/coordinator/locate.py b/custom_components/googlefindmy/coordinator/locate.py index cbce29439..1dcf3174b 100644 --- a/custom_components/googlefindmy/coordinator/locate.py +++ b/custom_components/googlefindmy/coordinator/locate.py @@ -747,13 +747,12 @@ async def async_play_sound(self, device_id: str) -> bool: return False try: 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. + # api.async_play_sound carries two independent facts. ``accepted`` + # answers "did Nova take the command", ``cancel_key`` answers "may + # the device be ringing", and ``outcome`` names WHO refused. The + # cause is read from ``outcome`` below and never reconstructed from + # the presence of a key -- that out-of-band inference is what + # IRR-CA-SOUND-FAILURE-CLASS removed. 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 @@ -796,17 +795,26 @@ async def async_play_sound(self, device_id: str) -> bool: "Stored Play Sound UUID for %s: %s", device_id, request_uuid ) await self._async_save_sound_uuids() - if not ok: + # Only a transport that gave us no usable answer is a push problem. + # A server rejection (401/403/5xx), a rate limit, a missing local + # action token and a bug of our own all reached this point as the + # same False before SoundDispatchOutcome existed, so every one of + # them armed the 90-second cooldown, flipped the integration to + # FcmStatus.DEGRADED and made can_play_sound report the button as + # unavailable -- an outage this integration inflicted on itself over + # a network that was working, and one that then also suppressed the + # user's follow-up Stop. See IRR-CA-SOUND-FAILURE-CLASS. + if play.outcome is SoundDispatchOutcome.TRANSPORT_FAILED: self._note_push_transport_problem() - else: + elif ok: # 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. + # An auth rejection arrives as REJECTED_AUTH, indistinguishable + # from a read timeout before this contract existed, 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 agree. self._set_auth_state(failed=False) - return bool(ok) + return ok except ConfigEntryAuthFailed as auth_exc: self._set_auth_state( failed=True, reason=f"Auth failed during play_sound: {auth_exc}" @@ -833,7 +841,12 @@ async def async_play_sound(self, device_id: str) -> bool: exc_info=True, ) self.note_error(err, where="async_play_sound", device=device_id) - self._note_push_transport_problem() + # No cooldown here. api.async_play_sound classifies every Exception + # in band and returns INTERNAL_ERROR instead of raising, so nothing + # that reaches this handler came from the push transport: what is + # left is this method's own body around the call, or an api + # implementation that breaks the Protocol. Blaming the transport for + # either is the misclassification IRR-CA-SOUND-FAILURE-CLASS stops. return False async def async_stop_sound( @@ -970,16 +983,19 @@ async def async_stop_sound( 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 - # swallows NovaAuthError and HTTP 401/403 into the same False - # as a timeout, so clearing the auth-failure state here would - # erase the very signal an expired sign-in produces. + # Same rule as on the play path above. The outer test stays in its + # negative form on purpose and is the single exception to the + # positive-list discipline: the safe default of THIS branch is + # FAILED, so an outcome nobody anticipated must fall through to it + # rather than be waved past. The cooldown inside it keeps the + # positive form, because there the safe default is to do nothing. + if stop_outcome is not SoundDispatchOutcome.ACCEPTED: + if stop_outcome is SoundDispatchOutcome.TRANSPORT_FAILED: + self._note_push_transport_problem() + # No credential proof on any non-accepted path: an auth + # rejection arrives as REJECTED_AUTH, and clearing the + # auth-failure state here would erase the very signal an expired + # sign-in produces. return StopSoundOutcome.FAILED # An accepted submission, and only that, proves credentials worked. self._set_auth_state(failed=False) @@ -1041,5 +1057,8 @@ async def async_stop_sound( exc_info=True, ) self.note_error(err, where="async_stop_sound", device=device_id) - self._note_push_transport_problem() + # No cooldown, for the reason spelled out on the play path: + # api.async_stop_sound returns INTERNAL_ERROR for every unexpected + # Exception instead of raising, so this handler only ever sees a + # failure of our own bookkeeping around the call. return StopSoundOutcome.FAILED diff --git a/docs/PLAY_SOUND_ARCHITECTURE.md b/docs/PLAY_SOUND_ARCHITECTURE.md index 69d4c9c73..c7778c232 100644 --- a/docs/PLAY_SOUND_ARCHITECTURE.md +++ b/docs/PLAY_SOUND_ARCHITECTURE.md @@ -182,6 +182,22 @@ 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. 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. + #### Follow-up (not in this change) Closing the boundary on the cloud path means wiring **Path B**: registering an diff --git a/tests/test_coordinator_locate_basics.py b/tests/test_coordinator_locate_basics.py index 4dbad7d71..1e231faa3 100644 --- a/tests/test_coordinator_locate_basics.py +++ b/tests/test_coordinator_locate_basics.py @@ -362,6 +362,20 @@ async def test_decrypt_failure_at_threshold_starts_reauth( coord.config_entry.async_start_reauth.assert_called_once() +# One row per ``SoundDispatchOutcome`` member: (outcome, accepted, may arm the +# push cooldown). Kept at module level so the exhaustiveness guard below reads +# the same table the parametrisation runs on. +PLAY_OUTCOME_CASES: list[tuple[SoundDispatchOutcome, bool, bool]] = [ + (SoundDispatchOutcome.ACCEPTED, True, False), + (SoundDispatchOutcome.REJECTED_AUTH, False, False), + (SoundDispatchOutcome.REJECTED_RATE_LIMIT, False, False), + (SoundDispatchOutcome.REJECTED_SERVER, False, False), + (SoundDispatchOutcome.NOT_SENT, False, False), + (SoundDispatchOutcome.INTERNAL_ERROR, False, False), + (SoundDispatchOutcome.TRANSPORT_FAILED, False, True), +] + + class TestAsyncPlaySoundGating: """Exercise gating branches of ``async_play_sound``.""" @@ -387,13 +401,26 @@ async def test_failure_notes_problem(self, coord: LocateStub) -> None: assert ok is False coord._note_push_transport_problem.assert_called_once() - async def test_unexpected_exception_returns_false(self, coord: LocateStub) -> None: + async def test_unexpected_exception_is_not_a_transport_problem( + self, coord: LocateStub + ) -> None: + """A bug of our own must not be reported as a broken push transport. + + ``api.async_play_sound`` classifies every ``Exception`` in band and + returns ``INTERNAL_ERROR`` instead of raising, so nothing that reaches + this handler came from the push transport: what is left is the + coordinator's own body around the call, or an ``api`` implementation + that breaks the Protocol. Arming the push cooldown for either is the + self-inflicted outage this contract was written to stop. The error is + still recorded. + """ + coord._device_caps["dev-1"] = {"can_ring": True} coord.api.async_play_sound.side_effect = RuntimeError("boom") ok = await coord.async_play_sound("dev-1") assert ok is False coord.note_error.assert_called_once() - coord._note_push_transport_problem.assert_called_once() + coord._note_push_transport_problem.assert_not_called() async def test_failed_play_does_not_clear_auth_state( self, coord: LocateStub @@ -426,10 +453,116 @@ async def test_accepted_play_still_clears_auth_state( coord._set_auth_state.assert_called_once_with(failed=False) + @pytest.mark.parametrize( + ("outcome", "expect_accepted", "expect_cooldown"), PLAY_OUTCOME_CASES + ) + async def test_only_a_transport_failure_arms_the_push_cooldown( + self, + coord: LocateStub, + outcome: SoundDispatchOutcome, + expect_accepted: bool, + expect_cooldown: bool, + ) -> None: + """A server saying no is not a network outage. + + Every non-acceptance used to arrive as a plain ``False``, so all of them + armed the 90-second push cooldown, flipped the integration to + ``FcmStatus.DEGRADED`` and made ``can_play_sound`` report the button as + unavailable. ``SoundDispatchOutcome`` names the cause; only a transport + that never gave us a usable answer may arm that cooldown. The list is + exhaustive over the enum on purpose: a new member added without a + decision here shows up as a missing parametrisation, not as a silent + default. + """ + + coord._device_caps["dev-1"] = {"can_ring": True} + coord.api.async_play_sound.return_value = PlaySoundResult(outcome) + + assert await coord.async_play_sound("dev-1") is expect_accepted + + assert coord._note_push_transport_problem.called is expect_cooldown + + def test_the_play_parametrisation_covers_every_outcome(self) -> None: + """Guard the exhaustiveness the case table claims for itself. + + The decision "which outcome may arm the cooldown" has to be taken for + every member of the enum. A member added later without a row here would + otherwise silently inherit whatever the ``if`` cascade happens to do. + """ + + assert {case[0] for case in PLAY_OUTCOME_CASES} == set(SoundDispatchOutcome) + + +# One row per ``SoundDispatchOutcome`` member on the stop side: (dispatch, +# resulting StopSoundOutcome, may arm the push cooldown, may vouch for the +# credentials). ACCEPTED lands on UNCORRELATED here because the key is passed +# in by the caller and is none of ours -- that split is pinned by its own tests. +STOP_OUTCOME_CASES: list[tuple[SoundDispatchOutcome, StopSoundOutcome, bool, bool]] = [ + (SoundDispatchOutcome.ACCEPTED, StopSoundOutcome.UNCORRELATED, False, True), + (SoundDispatchOutcome.REJECTED_AUTH, StopSoundOutcome.FAILED, False, False), + (SoundDispatchOutcome.REJECTED_RATE_LIMIT, StopSoundOutcome.FAILED, False, False), + (SoundDispatchOutcome.REJECTED_SERVER, StopSoundOutcome.FAILED, False, False), + (SoundDispatchOutcome.NOT_SENT, StopSoundOutcome.FAILED, False, False), + (SoundDispatchOutcome.INTERNAL_ERROR, StopSoundOutcome.FAILED, False, False), + (SoundDispatchOutcome.TRANSPORT_FAILED, StopSoundOutcome.FAILED, True, False), +] + class TestAsyncStopSoundGating: """Exercise gating branches of ``async_stop_sound``.""" + @pytest.mark.parametrize( + ("dispatch", "expect_outcome", "expect_cooldown", "expect_auth_cleared"), + STOP_OUTCOME_CASES, + ) + async def test_only_a_transport_failure_arms_the_push_cooldown( + self, + coord: LocateStub, + dispatch: SoundDispatchOutcome, + expect_outcome: StopSoundOutcome, + expect_cooldown: bool, + expect_auth_cleared: bool, + ) -> None: + """The same rule as on the play path, on the stop path. + + A stop the server refused on credentials, refused outright or rate + limited reached this method as a plain ``False`` before the contract + existed, so each of them armed the 90-second cooldown -- which then + suppressed the user's next attempt for a minute and a half over a + problem the network never had. + """ + + coord.api.async_stop_sound.return_value = dispatch + + outcome = await coord.async_stop_sound("dev-1", request_uuid="foreign-key") + + assert outcome is expect_outcome + assert coord._note_push_transport_problem.called is expect_cooldown + assert coord._set_auth_state.called is expect_auth_cleared + + def test_the_stop_parametrisation_covers_every_outcome(self) -> None: + """Guard the exhaustiveness the case table claims for itself.""" + + assert {case[0] for case in STOP_OUTCOME_CASES} == set(SoundDispatchOutcome) + + async def test_unexpected_exception_is_not_a_transport_problem( + self, coord: LocateStub + ) -> None: + """Mirror of the play-path rule: our own bug is not an outage. + + ``api.async_stop_sound`` returns ``INTERNAL_ERROR`` for every unexpected + ``Exception`` instead of raising, so this handler only sees failures of + the coordinator's own bookkeeping around the call. + """ + + coord.api.async_stop_sound.side_effect = RuntimeError("boom") + + outcome = await coord.async_stop_sound("dev-1", request_uuid="x") + + assert outcome is StopSoundOutcome.FAILED + coord.note_error.assert_called_once() + coord._note_push_transport_problem.assert_not_called() + async def test_blocks_when_push_not_ready(self, coord: LocateStub) -> None: coord._api_push_ready.return_value = False outcome = await coord.async_stop_sound("dev-1") From fd399ebac5e5c9ebb4dd3d1393b701b45ff7fe7e Mon Sep 17 00:00:00 2001 From: jleinenbach <1786119+jleinenbach@users.noreply.github.com> Date: Wed, 26 Aug 2026 10:49:56 +0000 Subject: [PATCH 5/7] fix(sound): let a correlated stop through a self-inflicted push cooldown A play that reaches the wire and then loses the answer stores a cancel key and arms the 90-second push cooldown in the same breath. Because _api_push_ready() short-circuits on that cooldown, async_stop_sound suppressed the stop the key exists for during the first 90 seconds after the play -- exactly when a user reaches for the Stop button. The readiness gate now passes in one further case: a window is running AND the stop would be correlated, meaning it carries our own cached key and that key is still fresh. Anything that could at best report UNCORRELATED (no key, an aged key, a foreign key supplied by the caller) stays suppressed, so the exception only ever buys an attempt that can actually silence the device. Two things keep it honest: - _stop_would_be_correlated() is the single definition of "correlated" behind both this gate and the decision to spend the cached key. A second inline derivation at either site is the drift the extraction exists to prevent, and two tests pin both sites to the predicate. - _note_stop_transport_problem_without_extending() makes sure the exception cannot feed itself. _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 push the end of the window forward and keep manual locate disabled for as long as the pressing goes on. The failure is still reported in full (the DEGRADED flag is not lost); only a window that was already running is restored instead of restarted. The price is a changed outcome class, not 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. That is pinned by a test rather than left implicit. docs/PLAY_SOUND_ARCHITECTURE.md gains the matching entry and corrects two older passages that would otherwise describe the stop path wrongly. --- .../googlefindmy/coordinator/locate.py | 174 +++++++-- docs/PLAY_SOUND_ARCHITECTURE.md | 59 ++- tests/test_coordinator_locate_basics.py | 348 ++++++++++++++++++ 3 files changed, 558 insertions(+), 23 deletions(-) diff --git a/custom_components/googlefindmy/coordinator/locate.py b/custom_components/googlefindmy/coordinator/locate.py index 1dcf3174b..651d98b32 100644 --- a/custom_components/googlefindmy/coordinator/locate.py +++ b/custom_components/googlefindmy/coordinator/locate.py @@ -724,6 +724,78 @@ def _cached_sound_uuid_is_stale(self, device_id: str) -> bool: return False return is_sound_uuid_expired(existing_ts, time.time(), SOUND_UUID_MAX_AGE_S) + def _note_stop_transport_problem_without_extending(self) -> None: + """Report a failed stop as a transport problem without LENGTHENING a live window. + + ``_note_push_transport_problem()`` does two things: it flags the + transport ``DEGRADED``, and it sets ``_push_cooldown_until`` to + ``monotonic() + cooldown_s`` ABSOLUTELY. The second part means calling + it while a window is already running restarts that window instead of + topping it up. Only the restart is unwanted here, so the call is made + in full and the window is put back afterwards -- skipping the call + outright would also drop the status flag, and a transport that just + failed must not keep reporting itself healthy because a location push + happened to arrive earlier in the same window. + + Reaching this code with a live window is close to new. Before the + correlated-stop exception in the gate, ``async_stop_sound`` returned + SUPPRESSED as its first statement whenever ``_api_push_ready()`` said + no, and a running window is one of the reasons it says no; only the + interleaving below could get past that. + + What the restart would cost: the stop button has no availability guard + (there is no ``can_stop_sound`` on the coordinator, ``button.py`` only + probes for one), so a user pressing Stop during an outage would push the + end of the window forward with every press. The window would still be + bounded -- it never outlives ``last press + cooldown_s`` -- but it also + gates manual locate unconditionally (and Play Sound for every device + whose ``can_ring`` capability is not cached, see ``can_play_sound()``), + and those stay disabled for as long as the pressing goes on. + + The stop itself is still sent; only the window is left alone. The same + rule covers a rare interleaving that predates this package: if a + concurrent play armed a window while this stop was in flight, that + window stands rather than being restarted from here. + """ + now = time.monotonic() + window_ends_at = self._push_cooldown_until + self._note_push_transport_problem() + if now < window_ends_at: + self._push_cooldown_until = window_ends_at + _LOGGER.debug( + "Stop failed on the transport while a push cooldown was " + "already running; keeping the existing window instead of " + "restarting it" + ) + + def _stop_would_be_correlated( + self, device_id: str, request_uuid: str | None + ) -> bool: + """Return True if a stop for ``device_id`` would carry a PROVEN cancel key. + + Read-only, no side effects. ``request_uuid`` must already be normalised + (a blank string collapsed to ``None``); ``async_stop_sound`` does that + at its single normalisation point before anything calls this. + + Proven means exactly one thing: the key that would go on the wire is our + own cached key and it is still fresh. An explicitly passed foreign + string is a claim, not a handle, and an aged key of ours cannot vouch + for the ring that is audible now. Both of those end as + ``StopSoundOutcome.UNCORRELATED``. + + This exists as one predicate because two decisions depend on the same + answer and must never drift apart: whether the stop may break a + self-inflicted push cooldown (IRR-CA-STOP-BREAKS-SELF-INFLICTED-COOLDOWN), + and whether an accepted stop may spend -- pop -- the cached key + (IRR-CA-POP-ON-CORRELATED-CANCEL-ONLY). + """ + cached_uuid = self._sound_request_uuids.get(device_id) + if cached_uuid is None: + return False + if request_uuid is not None and request_uuid != cached_uuid: + return False + return not self._cached_sound_uuid_is_stale(device_id) + async def async_play_sound(self, device_id: str) -> bool: """Play sound on a device using the native async API (no executor). @@ -874,15 +946,6 @@ async def async_stop_sound( carry the middle state, and collapsing it into success is what reported a stop for a ring that kept playing (BSkando#195). """ - # Less strict than can_play_sound(): stopping is harmless but still requires push readiness. - if not self._api_push_ready(): - _LOGGER.debug( - "Suppressing stop_sound call for %s: push not ready", device_id - ) - # A suppressed stop is a stop that was never sent, so it is a - # failure and must reach the service layer as one -- but as its own - # kind: nothing left this machine, and the condition clears itself. - return StopSoundOutcome.SUPPRESSED # Blank means "no opinion" -- an optional field left empty, a template # that rendered to nothing -- so it must fall through to the cached key # below, never pose as one. In-process the absence of a key already has @@ -896,8 +959,73 @@ async def async_stop_sound( # True only when the key about to be sent came from our own cache AND # was still fresh. An explicitly passed foreign UUID does NOT qualify: # popping our cache entry on its behalf would drop the handle of a - # different, possibly still running ring. - used_own_fresh_key = False + # different, possibly still running ring. Decided here, ABOVE the + # readiness gate, because that gate needs the same answer; the single + # definition lives in _stop_would_be_correlated(). + used_own_fresh_key = self._stop_would_be_correlated( + device_id, request_uuid_to_use + ) + + # Less strict than can_play_sound(): stopping is harmless but still + # requires push readiness. + if not self._api_push_ready(): + # ... with exactly one exception, and it runs OPPOSITE to + # can_play_sound() and the manual-locate guard above, which treat an + # active cooldown as the hard block and a merely unconfirmed + # transport as passable. + # + # What the exception tests is narrower than "the transport is + # fine", and it has to be: while a cooldown runs, _api_push_ready() + # short-circuits on that cooldown and never asks the transport at + # all, so the transport state is simply unknown here. The two cases + # this guard can tell apart are "not ready BECAUSE a window is + # running" and "not ready for some other reason", and only the + # first is passable. Claiming it also excludes a genuinely dead + # transport would be untrue: a transport failure is the ONLY thing + # that arms this window, so the two overlap by construction. + # + # Why the first case is passable at all: a play that reached the + # wire and lost the answer stores a cancel key and arms the 90 s + # window in the same breath, so the stop that key exists for was + # locked out for the first 90 seconds after that play -- which is + # exactly when a user reaches for the Stop button + # (IRR-CA-STOP-BREAKS-SELF-INFLICTED-COOLDOWN). Nothing here claims + # to know how long the ring itself lasts; that is a different timer + # on a different layer, see the note on the aged cached key below. + # The window is global while keys are per device, so "the play that + # armed it" is the common case, not a proven one. What IS proven is + # that this stop can be correlated, and that is the entire benefit + # bought here. + # + # The exception is bound to a PROVEN key, so every case that could + # at best report UNCORRELATED stays suppressed. It is not free, + # though, and the price is a changed outcome CLASS: a stop that + # used to end as SUPPRESSED ("not sent, try again in a moment") now + # reaches the transport, so it can also end as FAILED, which + # services.py reports with a different message. That is the honest + # trade -- an attempt that can actually silence the device, at the + # cost of a report that names the transport instead of the gate -- + # and it is pinned by a test rather than left implicit. + # + # It also cannot feed itself: a broken-through stop that fails on + # the transport leaves the window exactly where it was, see + # _note_stop_transport_problem_without_extending(). + if used_own_fresh_key and time.monotonic() < self._push_cooldown_until: + _LOGGER.debug( + "Push cooldown active for %s, but this stop carries our own " + "fresh cancel key: sending it anyway", + device_id, + ) + else: + _LOGGER.debug( + "Suppressing stop_sound call for %s: push not ready", device_id + ) + # A suppressed stop is a stop that was never sent, so it is a + # failure and must reach the service layer as one -- but as its + # own kind: nothing left this machine, and the condition clears + # itself. + return StopSoundOutcome.SUPPRESSED + cached_uuid_was_expired = False if request_uuid_to_use is None: cached_uuid = self._sound_request_uuids.get(device_id) @@ -941,7 +1069,6 @@ async def async_stop_sound( device_id, ) else: - used_own_fresh_key = True _LOGGER.debug( "Using cached Play Sound UUID for %s: %s", device_id, @@ -953,14 +1080,19 @@ async def async_stop_sound( # cached key. Any other string -- a typo, a stale template, the key # of a different ring -- is unverifiable, and reporting CANCELLED # for it would be BSkando#195 one layer up: success without effect. + # Which of the two it is was already settled by + # _stop_would_be_correlated() above; re-deriving the predicate here + # is exactly the drift this package removed. What is left for this + # branch is to say, per case, what it means. cached_uuid = self._sound_request_uuids.get(device_id) - if ( - cached_uuid is not None - and cached_uuid == request_uuid_to_use - and not self._cached_sound_uuid_is_stale(device_id) - ): - # It is our key, so spending it is correct, not an eviction. - used_own_fresh_key = True + if used_own_fresh_key: + # It is our own live key, so spending it later is correct and + # not an eviction. + _LOGGER.debug( + "Cancel key supplied for %s is our own live key: %s", + device_id, + request_uuid_to_use, + ) elif cached_uuid is not None and cached_uuid == request_uuid_to_use: # Ours, but aged: it matches, it is simply too old to vouch # for. Saying "does not match" here would be untrue, and the @@ -991,7 +1123,7 @@ async def async_stop_sound( # positive form, because there the safe default is to do nothing. if stop_outcome is not SoundDispatchOutcome.ACCEPTED: if stop_outcome is SoundDispatchOutcome.TRANSPORT_FAILED: - self._note_push_transport_problem() + self._note_stop_transport_problem_without_extending() # No credential proof on any non-accepted path: an auth # rejection arrives as REJECTED_AUTH, and clearing the # auth-failure state here would erase the very signal an expired @@ -1047,7 +1179,7 @@ async def async_stop_sound( conn_err, ) self.note_error(conn_err, where="async_stop_sound", device=device_id) - self._note_push_transport_problem() + self._note_stop_transport_problem_without_extending() return StopSoundOutcome.FAILED except Exception as err: _LOGGER.error( diff --git a/docs/PLAY_SOUND_ARCHITECTURE.md b/docs/PLAY_SOUND_ARCHITECTURE.md index c7778c232..d654c5364 100644 --- a/docs/PLAY_SOUND_ARCHITECTURE.md +++ b/docs/PLAY_SOUND_ARCHITECTURE.md @@ -166,7 +166,7 @@ knowledge across the boundary instead of collapsing it into a bool: | `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** | +| `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 | @@ -185,7 +185,11 @@ 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. Their blanket +`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 @@ -198,8 +202,59 @@ 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 diff --git a/tests/test_coordinator_locate_basics.py b/tests/test_coordinator_locate_basics.py index 1e231faa3..2e24d21c8 100644 --- a/tests/test_coordinator_locate_basics.py +++ b/tests/test_coordinator_locate_basics.py @@ -14,9 +14,11 @@ import asyncio import math +import time from unittest.mock import MagicMock import pytest +from aiohttp import ClientConnectionError from homeassistant.exceptions import HomeAssistantError from custom_components.googlefindmy.const import ( @@ -654,4 +656,350 @@ async def test_failed_stop_keeps_a_fresh_cancel_key( assert coord._sound_request_uuids["dev-1"] == "cached-uuid" +class TestStopBreaksSelfInflictedCooldown: + """AP-5 / F2: the cancel key a failed play preserved must be usable at once. + + A play that reached the wire and then lost the answer does two things in the + same breath: it stores a cancel key, and it arms the 90-second push cooldown + (correctly, because that IS a transport failure). ``_api_push_ready()`` + short-circuits to False while the cooldown runs, so the stop that the key + exists for was 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 is not claimed here.) + See IRR-CA-STOP-BREAKS-SELF-INFLICTED-COOLDOWN. + + The exception is deliberately narrow. It applies only when the stop would be + correlated (our own, fresh cancel key); a stop that would report + UNCORRELATED buys nothing, so the anti-spam purpose of the cooldown is kept + for every case that has no provable benefit. It is not free either: a stop + that used to end as SUPPRESSED now reaches the transport and can end as + FAILED instead, which is a different service-level message. That change of + outcome class is pinned below rather than left implicit. + """ + + async def test_ambiguous_play_does_not_block_the_following_stop( + self, coord: LocateStub + ) -> None: + """The whole F2 chain, end to end: play loses the answer, stop follows.""" + + coord._device_caps["dev-1"] = {"can_ring": True} + + def _note(cooldown_s: int = 90) -> None: + coord._push_cooldown_until = time.monotonic() + cooldown_s + + coord._note_push_transport_problem = MagicMock(side_effect=_note) + coord._api_push_ready = MagicMock( + side_effect=lambda: time.monotonic() >= coord._push_cooldown_until + ) + coord.api.async_play_sound.return_value = PlaySoundResult( + SoundDispatchOutcome.TRANSPORT_FAILED, cancel_key="uuid-ambiguous" + ) + + assert await coord.async_play_sound("dev-1") is False + assert coord._sound_request_uuids.get("dev-1") == "uuid-ambiguous" + assert coord._push_cooldown_until > time.monotonic() + + coord.api.async_stop_sound.return_value = SoundDispatchOutcome.ACCEPTED + outcome = await coord.async_stop_sound("dev-1") + + assert outcome is StopSoundOutcome.CANCELLED + coord.api.async_stop_sound.assert_awaited_once_with("dev-1", "uuid-ambiguous") + + async def test_explicit_own_fresh_key_breaks_the_cooldown( + self, coord: LocateStub + ) -> None: + """The service handler passes the key explicitly; same right to be sent.""" + + coord._api_push_ready.return_value = False + coord._push_cooldown_until = time.monotonic() + 90.0 + coord._sound_request_uuids["dev-1"] = "uuid-fresh" + + outcome = await coord.async_stop_sound("dev-1", request_uuid="uuid-fresh") + + assert outcome is StopSoundOutcome.CANCELLED + coord.api.async_stop_sound.assert_awaited_once_with("dev-1", "uuid-fresh") + + async def test_blank_explicit_key_falls_back_to_the_cached_key( + self, coord: LocateStub + ) -> None: + """A blank argument means "no opinion" here too, not "no key".""" + + coord._api_push_ready.return_value = False + coord._push_cooldown_until = time.monotonic() + 90.0 + coord._sound_request_uuids["dev-1"] = "uuid-fresh" + + outcome = await coord.async_stop_sound("dev-1", request_uuid=" ") + + assert outcome is StopSoundOutcome.CANCELLED + coord.api.async_stop_sound.assert_awaited_once_with("dev-1", "uuid-fresh") + + async def test_a_broken_through_stop_that_fails_does_not_extend_the_window( + self, coord: LocateStub + ) -> None: + """Breaking a window must never lengthen it (the amplification guard). + + ``_note_push_transport_problem`` sets ``_push_cooldown_until`` to + ``monotonic() + 90`` ABSOLUTELY and flags the transport DEGRADED, so + every call restarts the window rather than topping it up. Before this + package that call was unreachable while a window ran -- the suppression + was the first statement of the method -- and the stop button has no + availability guard (``can_stop_sound`` does not exist on the + coordinator, button.py only probes for it). Without this guard a user + who keeps pressing Stop during an outage would restart the window on + every press, and that window also gates Play Sound and manual locate: + two unrelated features stay disabled for as long as the pressing goes + on. The stop itself is still sent -- that is the point of the exception + -- it just may not lengthen the window that let it through. + """ + + def _arm(cooldown_s: int = 90) -> None: + coord._push_cooldown_until = time.monotonic() + cooldown_s + + coord._note_push_transport_problem = MagicMock(side_effect=_arm) + coord._api_push_ready.return_value = False + window_ends_at = time.monotonic() + 90.0 + coord._push_cooldown_until = window_ends_at + coord._sound_request_uuids["dev-1"] = "uuid-fresh" + coord.api.async_stop_sound.return_value = SoundDispatchOutcome.TRANSPORT_FAILED + + outcome = await coord.async_stop_sound("dev-1") + + assert outcome is StopSoundOutcome.FAILED + coord.api.async_stop_sound.assert_awaited_once_with("dev-1", "uuid-fresh") + assert coord._push_cooldown_until == window_ends_at + + async def test_repeated_broken_through_stops_never_move_the_window_end( + self, coord: LocateStub + ) -> None: + """The amplification chain: repeated presses must not push the end forward. + + All five presses fall inside the one window under test, which is the + situation the guard is for. The claim is constancy of the end within a + window, not termination in general: that already follows from + ``_note_push_transport_problem`` setting the deadline absolutely. + """ + + def _arm(cooldown_s: int = 90) -> None: + coord._push_cooldown_until = time.monotonic() + cooldown_s + + coord._note_push_transport_problem = MagicMock(side_effect=_arm) + coord._api_push_ready.side_effect = ( + lambda: time.monotonic() >= coord._push_cooldown_until + ) + window_ends_at = time.monotonic() + 90.0 + coord._push_cooldown_until = window_ends_at + coord._sound_request_uuids["dev-1"] = "uuid-fresh" + coord.api.async_stop_sound.return_value = SoundDispatchOutcome.TRANSPORT_FAILED + + for _ in range(5): + assert await coord.async_stop_sound("dev-1") is StopSoundOutcome.FAILED + + assert coord.api.async_stop_sound.await_count == 5 + assert coord._push_cooldown_until == window_ends_at + + async def test_a_failing_stop_still_arms_a_window_when_none_is_running( + self, coord: LocateStub + ) -> None: + """The guard is about EXTENDING, not about arming: the normal path is untouched.""" + + coord._api_push_ready.return_value = True + coord._push_cooldown_until = 0.0 + coord._sound_request_uuids["dev-1"] = "uuid-fresh" + coord.api.async_stop_sound.return_value = SoundDispatchOutcome.TRANSPORT_FAILED + + assert await coord.async_stop_sound("dev-1") is StopSoundOutcome.FAILED + coord._note_push_transport_problem.assert_called_once() + + async def test_a_raised_connection_error_does_not_extend_the_window_either( + self, coord: LocateStub + ) -> None: + """The same guard covers the typed exception handler, not just the outcome. + + ``api`` is a Protocol, so an implementation that lets an aiohttp error + escape reaches the typed handler rather than returning + TRANSPORT_FAILED. That handler arms the cooldown too, and a + broken-through stop can now reach it, so it must not restart a running + window either. + """ + + def _arm(cooldown_s: int = 90) -> None: + coord._push_cooldown_until = time.monotonic() + cooldown_s + + coord._note_push_transport_problem = MagicMock(side_effect=_arm) + coord._api_push_ready.return_value = False + window_ends_at = time.monotonic() + 90.0 + coord._push_cooldown_until = window_ends_at + coord._sound_request_uuids["dev-1"] = "uuid-fresh" + coord.api.async_stop_sound.side_effect = ClientConnectionError("boom") + + assert await coord.async_stop_sound("dev-1") is StopSoundOutcome.FAILED + coord.api.async_stop_sound.assert_awaited_once_with("dev-1", "uuid-fresh") + assert coord._push_cooldown_until == window_ends_at + + async def test_a_raised_connection_error_arms_a_window_when_none_is_running( + self, coord: LocateStub + ) -> None: + """Counter-case, so the guard above cannot pass by disarming the handler.""" + + coord._api_push_ready.return_value = True + coord._push_cooldown_until = 0.0 + coord._sound_request_uuids["dev-1"] = "uuid-fresh" + coord.api.async_stop_sound.side_effect = ClientConnectionError("boom") + + assert await coord.async_stop_sound("dev-1") is StopSoundOutcome.FAILED + coord._note_push_transport_problem.assert_called_once() + + async def test_the_exception_changes_the_reported_outcome_class( + self, coord: LocateStub + ) -> None: + """The price of the exception, stated as a test rather than as prose. + + Breaking the window means the stop reaches the transport, so a case that + used to end as SUPPRESSED can now end as FAILED. services.py maps the + two to different exception translation keys + (``stop_sound_suppressed`` vs ``stop_sound_rejected``), so this is + user-visible and must not drift unnoticed. The counter-case in the same + test keeps the old class for a stop that is NOT correlated. + """ + + coord._api_push_ready.return_value = False + coord._push_cooldown_until = time.monotonic() + 90.0 + coord.api.async_stop_sound.return_value = SoundDispatchOutcome.TRANSPORT_FAILED + + # No key: unchanged, still never sent. + assert await coord.async_stop_sound("dev-1") is StopSoundOutcome.SUPPRESSED + coord.api.async_stop_sound.assert_not_called() + + # Same window, same transport, but now a proven key. + coord._sound_request_uuids["dev-1"] = "uuid-fresh" + assert await coord.async_stop_sound("dev-1") is StopSoundOutcome.FAILED + coord.api.async_stop_sound.assert_awaited_once_with("dev-1", "uuid-fresh") + + # ---- the boundary: everything below must stay suppressed ---- + + async def test_keyless_stop_stays_suppressed_during_the_cooldown( + self, coord: LocateStub + ) -> None: + """Without a key the stop would be UNCORRELATED, so it buys nothing.""" + + coord._api_push_ready.return_value = False + coord._push_cooldown_until = time.monotonic() + 90.0 + + assert await coord.async_stop_sound("dev-1") is StopSoundOutcome.SUPPRESSED + coord.api.async_stop_sound.assert_not_called() + + async def test_stop_stays_suppressed_when_push_is_down_without_a_cooldown( + self, coord: LocateStub + ) -> None: + """The exception is bound to the cooldown, not to push readiness at large. + + A genuinely disconnected push transport is not something this stop + inflicted on itself, and sending into it proves nothing. + """ + + coord._api_push_ready.return_value = False + coord._push_cooldown_until = 0.0 + coord._sound_request_uuids["dev-1"] = "uuid-fresh" + + assert await coord.async_stop_sound("dev-1") is StopSoundOutcome.SUPPRESSED + coord.api.async_stop_sound.assert_not_called() + + async def test_expired_cooldown_does_not_break_a_push_outage( + self, coord: LocateStub + ) -> None: + """Boundary of the window: a cooldown that has run out grants nothing.""" + + coord._api_push_ready.return_value = False + coord._push_cooldown_until = time.monotonic() - 0.01 + coord._sound_request_uuids["dev-1"] = "uuid-fresh" + + assert await coord.async_stop_sound("dev-1") is StopSoundOutcome.SUPPRESSED + coord.api.async_stop_sound.assert_not_called() + + async def test_stale_cached_key_stays_suppressed_during_the_cooldown( + self, coord: LocateStub + ) -> None: + """A key older than SOUND_UUID_MAX_AGE_S cannot be the one this cooldown made. + + The cooldown lasts 90 seconds, the key aged past 30 minutes: it belongs + to an older play, and a stop carrying it would report UNCORRELATED. That + is the keyless case with extra steps, so it stays suppressed. + """ + + coord._api_push_ready.return_value = False + coord._push_cooldown_until = time.monotonic() + 90.0 + coord._sound_request_uuids["dev-1"] = "uuid-old" + coord._sound_request_timestamps["dev-1"] = time.time() - 3600.0 + + assert await coord.async_stop_sound("dev-1") is StopSoundOutcome.SUPPRESSED + coord.api.async_stop_sound.assert_not_called() + + async def test_foreign_explicit_key_stays_suppressed_during_the_cooldown( + self, coord: LocateStub + ) -> None: + """An unverifiable key is a claim, not a handle, so it grants no exception. + + Mirrors the rule one layer down: an explicitly passed key only proves + correlation when it IS our own fresh cached key. + + We DO hold a live key for this device here, and that is the point: the + discriminating fact is not "some key exists for dev-1" but "the key + going on the wire is ours". Sending the caller's string would report + CANCELLED for a ring we never addressed, and spend our own handle doing + it. + """ + + coord._api_push_ready.return_value = False + coord._push_cooldown_until = time.monotonic() + 90.0 + coord._sound_request_uuids["dev-1"] = "uuid-ours-fresh" + + outcome = await coord.async_stop_sound("dev-1", request_uuid="foreign-uuid") + + assert outcome is StopSoundOutcome.SUPPRESSED + coord.api.async_stop_sound.assert_not_called() + + +class TestCorrelationPredicateIsShared: + """The cooldown gate and the outcome/pop branch must read ONE definition. + + ``_stop_would_be_correlated`` was extracted precisely because two decisions + depend on the same question -- may this stop break a self-inflicted push + cooldown, and may an accepted stop spend the cached key -- and a second, + inline re-derivation at either site is free to drift away from the first. + These two tests bind the sites to the predicate by making the predicate + disagree with the raw cache state: an inline re-derivation would read the + cache and answer the opposite, so it cannot pass. + """ + + async def test_gate_follows_the_predicate_against_the_raw_cache( + self, coord: LocateStub + ) -> None: + """Predicate says no while the cache holds a fresh key of ours.""" + + coord._api_push_ready.return_value = False + coord._push_cooldown_until = time.monotonic() + 90.0 + coord._sound_request_uuids["dev-1"] = "uuid-fresh" + coord._stop_would_be_correlated = MagicMock(return_value=False) + + assert await coord.async_stop_sound("dev-1") is StopSoundOutcome.SUPPRESSED + coord.api.async_stop_sound.assert_not_called() + coord._stop_would_be_correlated.assert_called_once_with("dev-1", None) + + async def test_outcome_and_pop_follow_the_predicate_against_the_raw_cache( + self, coord: LocateStub + ) -> None: + """Predicate says yes while the cached key has aged past the limit.""" + + coord._api_push_ready.return_value = True + coord._sound_request_uuids["dev-1"] = "uuid-old" + coord._sound_request_timestamps["dev-1"] = time.time() - 3600.0 + coord._stop_would_be_correlated = MagicMock(return_value=True) + coord.api.async_stop_sound.return_value = SoundDispatchOutcome.ACCEPTED + + outcome = await coord.async_stop_sound("dev-1") + + assert outcome is StopSoundOutcome.CANCELLED + assert "dev-1" not in coord._sound_request_uuids + + _ = DEFAULT_MIN_POLL_INTERVAL # silence unused-import lint when production no-ops From 70451736102dd1aba2fd8c855b87cf424754b7ba Mon Sep 17 00:00:00 2001 From: jleinenbach <1786119+jleinenbach@users.noreply.github.com> Date: Wed, 26 Aug 2026 11:02:48 +0000 Subject: [PATCH 6/7] style(tests): adopt the CI formatter shape for the readiness lambda The repository's ruff (0.14.14 in CI) wraps the lambda body rather than the assignment. Both the pinned and the local formatter accept this shape, so it is not a version-specific workaround. --- tests/test_coordinator_locate_basics.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_coordinator_locate_basics.py b/tests/test_coordinator_locate_basics.py index 2e24d21c8..538231cbe 100644 --- a/tests/test_coordinator_locate_basics.py +++ b/tests/test_coordinator_locate_basics.py @@ -783,8 +783,8 @@ def _arm(cooldown_s: int = 90) -> None: coord._push_cooldown_until = time.monotonic() + cooldown_s coord._note_push_transport_problem = MagicMock(side_effect=_arm) - coord._api_push_ready.side_effect = ( - lambda: time.monotonic() >= coord._push_cooldown_until + coord._api_push_ready.side_effect = lambda: ( + time.monotonic() >= coord._push_cooldown_until ) window_ends_at = time.monotonic() + 90.0 coord._push_cooldown_until = window_ends_at From c86e6b536aabcaa2f45bfd19a71df4d6eaa920e9 Mon Sep 17 00:00:00 2001 From: jleinenbach <1786119+jleinenbach@users.noreply.github.com> Date: Wed, 26 Aug 2026 14:58:28 +0000 Subject: [PATCH 7/7] fix(sound): classify a Nova auth error by its status, not its type `NovaAuthError` is raised for every non-retryable 4xx, not only for credential rejections: the comment above the raise in nova_request.py names "403 Forbidden, 404 Not Found" itself, and HTTP_RETRY_ELIGIBLE holds no 4xx besides 408 and 429. A 401 that survives the refresh sequence is raised separately with is_permanent=True, which leaves 403 as the only plain credential rejection reaching these handlers. Both sound handlers read the type alone, so a deleted device produced SoundDispatchOutcome.REJECTED_AUTH and an "Authentication failed" ERROR -- telling a user with a perfectly good sign-in to check their credentials. The neighbouring `except NovaHTTPError` already inspected `err.status`; the new shared helper `_classify_nova_auth_error` gives the auth handler the same rule: permanent first, then 401/403, everything else REJECTED_SERVER. No consumer branches on REJECTED_AUTH, so the log line is the entire user-visible effect of this split. It is asserted, level included: a mutation that keeps the classification and restores the single blanket ERROR kills the new guards while every enum assertion stays green. Second finding, docs only: StopSoundOutcome.FAILED began with "Handed to the transport" while listing the missing action token, which never reaches the wire. Mapping NOT_SENT to SUPPRESSED instead -- as suggested -- would answer a missing token with "try again in a moment", an unbounded retry loop; the `stop_sound_rejected` message already names "a missing token" and points at the log. The docs were corrected instead: the two failure states split by REMEDY, not by distance travelled. That sentence lived in four places (const.py twice, coordinator/locate.py's Returns contract, one test docstring); all four now agree, and a stale "returns False" claim from the bool era went with them. Known gap, stated rather than silently left: the device-list handler, the location handler, coordinator/polling.py and coordinator/locate.py still key off the exception type, where a 404 turns into ConfigEntryAuthFailed or feeds the reauth countdown. Narrowing those changes polling and setup behaviour and needs its own regression test. Recorded in api.py's module header and in custom_components/googlefindmy/AGENTS.md so a green suite is not mistaken for tree-wide coverage. --- custom_components/googlefindmy/AGENTS.md | 11 ++ custom_components/googlefindmy/api.py | 118 ++++++++++--- custom_components/googlefindmy/const.py | 67 +++++-- .../googlefindmy/coordinator/locate.py | 11 +- custom_components/googlefindmy/services.py | 16 +- docs/PLAY_SOUND_ARCHITECTURE.md | 4 +- tests/test_api_basics.py | 164 ++++++++++++++++++ tests/test_coordinator_locate_basics.py | 12 +- tests/test_services_handler_contracts.py | 9 +- 9 files changed, 353 insertions(+), 59 deletions(-) diff --git a/custom_components/googlefindmy/AGENTS.md b/custom_components/googlefindmy/AGENTS.md index d62a179c1..dd495154e 100644 --- a/custom_components/googlefindmy/AGENTS.md +++ b/custom_components/googlefindmy/AGENTS.md @@ -101,6 +101,17 @@ When a location decrypt/FCM callback encounters `SpotApiEmptyResponseError`, sto re-raise it after the waiter resumes so the coordinator can translate it into `ConfigEntryAuthFailed`. This keeps invalid sessions flowing into Home Assistant's reauthentication UI instead of being swallowed in background threads. +Classify an auth rejection by the HTTP **status**, never by the exception type. `NovaAuthError` is raised for every +non-retryable 4xx (`nova_request.py` raises it for 400, 404, 405, 409, 422 as well; `HTTP_RETRY_ELIGIBLE` holds no 4xx +besides 408 and 429), so a type check reads "device not found" as "your sign-in expired". A 401 that survived the refresh +sequence arrives flagged `is_permanent=True`, which leaves 403 as the only plain credential rejection a handler sees. +`api._classify_nova_auth_error` is the shared predicate: permanent first, then 401/403, everything else is a server-side +rejection. Extent today, stated so it is not mistaken for coverage: the two sound handlers use it; the device-list handler +(`api.py`, `except NovaAuthError` before `NovaProtobufDecodeError`), the location handler, `coordinator/polling.py` and +`coordinator/locate.py` still key off the type, and each of those turns a 404 into `ConfigEntryAuthFailed` or a reauth +countdown. Narrowing them is a behaviour change that needs its own regression test, not a drive-by edit; do not assume a +green suite proves the rest of the tree already follows this rule. + ### Import deferral reminder Heavyweight runtime dependencies (for example, browser drivers such as `undetected_chromedriver`) must be imported lazily inside diff --git a/custom_components/googlefindmy/api.py b/custom_components/googlefindmy/api.py index d98500b2f..2034e0b1f 100644 --- a/custom_components/googlefindmy/api.py +++ b/custom_components/googlefindmy/api.py @@ -12,6 +12,12 @@ - **401/403 (auth failures)** raised by Nova helpers are mapped to `homeassistant.exceptions.ConfigEntryAuthFailed` so the *coordinator* can trigger HA’s re-auth UX and Repairs issue workflow. + Known gap, stated so it is not mistaken for the current behaviour: the device + list and location handlers still key off the `NovaAuthError` TYPE, which the + transport also raises for non-credential 4xx (400, 404, 405, 409, 422), so a + deleted device can currently reach the re-auth flow. The sound handlers were + narrowed to the status; see `_classify_nova_auth_error`. Extending that to the + other two is a behaviour change of its own and is tracked separately. - **gpsoauth/ADM failures** (e.g., "BadAuthentication", "Missing 'Token' in gpsoauth") are normalized to `ConfigEntryAuthFailed` as well, even if they bubble up as a `RuntimeError`/`ValueError` rather than a `NovaAuthError`. @@ -105,6 +111,40 @@ def _short_err(e: Exception | str) -> str: return msg +def _classify_nova_auth_error(err: NovaAuthError) -> SoundDispatchOutcome: + """Name who refused when the transport raised ``NovaAuthError``. + + The exception type is wider than its name. ``nova_request`` raises it for + EVERY non-retryable 4xx, not only for credential rejections -- the comment + above that raise names "403 Forbidden, 404 Not Found" itself, and + ``HTTP_RETRY_ELIGIBLE`` holds no 4xx besides 408 and 429, so 400, 404, 405, + 409 and 422 all arrive here. A 401 that survived the refresh sequence is + raised separately with ``is_permanent=True``, which leaves 403 as the only + plain credential rejection reaching this handler. + + Reading the type alone therefore told a user with a deleted device to check + their sign-in. ``REJECTED_AUTH`` is for a refusal "on credentials: HTTP 401 + or 403", so a missing device or a malformed request belongs in + ``REJECTED_SERVER``, which names the non-credential client rejections + alongside the 5xx. Both docstrings state the criterion as the STATUS, not + the exception type; keep the three in step when any of them moves. + + Permanence outranks the status: ``NovaAuthPermanentError`` exists to say + "re-authentication is definitively required", so it stays ``REJECTED_AUTH`` + even if it ever carries a status outside 401/403. + + An error without a readable status keeps the old classification. That case + is a test double or a future subclass, not an observed server answer, and + the conservative reading of a type named "auth" is auth. + """ + if getattr(err, "is_permanent", False): + return SoundDispatchOutcome.REJECTED_AUTH + status = getattr(err, "status", None) + if status is None or status in (401, 403): + return SoundDispatchOutcome.REJECTED_AUTH + return SoundDispatchOutcome.REJECTED_SERVER + + def _play_result_after_failure( err: NovaError, request_uuid: str, outcome: SoundDispatchOutcome ) -> PlaySoundResult: @@ -1611,9 +1651,13 @@ async def async_play_sound(self, device_id: str) -> PlaySoundResult: """Send a 'Play Sound' command to a device (async path for HA). Auth mapping note: - If an auth error occurs here, we log and return REJECTED_AUTH (service - call context), since re-auth is primarily driven by the coordinator’s - data update path. + A credential rejection here is logged and returned as REJECTED_AUTH + (service call context), since re-auth is primarily driven by the + coordinator’s data update path. "Credential rejection" means the + STATUS says so (401/403) or the error is flagged permanent, not + merely that the transport raised ``NovaAuthError`` -- that type also + carries 400, 404 and the other non-retryable 4xx, which return + REJECTED_SERVER. See ``_classify_nova_auth_error``. Args: device_id: The canonical ID of the device. @@ -1717,22 +1761,30 @@ async def async_play_sound(self, device_id: str) -> PlaySoundResult: ) except NovaAuthError as err: - _LOGGER.error( - "Authentication failed while playing sound on %s: %s", - device_id, - _short_err(err), - ) + # The server answered and refused: never a transport failure, so an + # expired sign-in is not hidden behind a self-clearing 90-second + # cooldown. WHICH refusal it was is decided by the status, not by + # the exception type; see _classify_nova_auth_error. + outcome = _classify_nova_auth_error(err) + if outcome is SoundDispatchOutcome.REJECTED_AUTH: + _LOGGER.error( + "Authentication failed while playing sound on %s: %s", + device_id, + _short_err(err), + ) + else: + _LOGGER.warning( + "Client error (HTTP %s) while playing sound on %s: %s", + getattr(err, "status", "unknown"), + device_id, + _short_err(err), + ) # A raised error is never an acceptance (the submitter returns a tuple # only on a 200). Keep the cancel key only if an earlier attempt - # latched dispatch (err.dispatched): a pure 401/403 rejection with no + # latched dispatch (err.dispatched): a pure rejection with no # wire-reaching attempt drops it, so it can never overwrite a # previous, possibly still-ringing play's valid key. - # The server answered and refused: REJECTED_AUTH, not a transport - # failure. An expired sign-in must not be hidden behind a self-clearing - # 90-second cooldown. - return _play_result_after_failure( - err, request_uuid, SoundDispatchOutcome.REJECTED_AUTH - ) + return _play_result_after_failure(err, request_uuid, outcome) except NovaHTTPError as err: # Non-acceptance (401/403/5xx/other): drop the key UNLESS an earlier @@ -1861,9 +1913,13 @@ async def async_stop_sound( """Send a 'Stop Sound' command to a device (async path for HA). Auth mapping note: - If an auth error occurs here, we log and return REJECTED_AUTH (service - call context), since re-auth is primarily driven by the coordinator’s - data update path. + A credential rejection here is logged and returned as REJECTED_AUTH + (service call context), since re-auth is primarily driven by the + coordinator’s data update path. "Credential rejection" means the + STATUS says so (401/403) or the error is flagged permanent, not + merely that the transport raised ``NovaAuthError`` -- that type also + carries 400, 404 and the other non-retryable 4xx, which return + REJECTED_SERVER. See ``_classify_nova_auth_error``. Args: device_id: The canonical ID of the device. @@ -1961,14 +2017,24 @@ async def async_stop_sound( return SoundDispatchOutcome.INTERNAL_ERROR except NovaAuthError as err: - _LOGGER.error( - "Authentication failed while stopping sound on %s: %s", - device_id, - _short_err(err), - ) - # The server answered and refused on credentials. The transport - # worked, so no cooldown may be armed for this. - return SoundDispatchOutcome.REJECTED_AUTH + # The server answered and refused. The transport worked, so no + # cooldown may be armed for this. Same rule as on the play path: + # the status names the refusal, not the exception type. + outcome = _classify_nova_auth_error(err) + if outcome is SoundDispatchOutcome.REJECTED_AUTH: + _LOGGER.error( + "Authentication failed while stopping sound on %s: %s", + device_id, + _short_err(err), + ) + else: + _LOGGER.warning( + "Client error (HTTP %s) while stopping sound on %s: %s", + getattr(err, "status", "unknown"), + device_id, + _short_err(err), + ) + return outcome except NovaHTTPError as err: if getattr(err, "status", None) in (401, 403): diff --git a/custom_components/googlefindmy/const.py b/custom_components/googlefindmy/const.py index 6fa4f0868..ea6939abe 100644 --- a/custom_components/googlefindmy/const.py +++ b/custom_components/googlefindmy/const.py @@ -65,17 +65,34 @@ class SoundDispatchOutcome(StrEnum): """ REJECTED_AUTH = "rejected_auth" - """The server answered and refused on credentials (NovaAuthError, 401, 403). + """The server answered and refused on credentials: HTTP 401 or 403. The transport worked. A cooldown would mislabel an expired sign-in as a network outage and hide it behind a self-clearing timer. + + The criterion is the STATUS, not the exception type. ``NovaAuthError`` is + raised for every non-retryable 4xx (its own docstring says "4xx client + errors", and ``HTTP_RETRY_ELIGIBLE`` holds no 4xx besides 408 and 429), so + reading the type alone filed a deleted device under "check your sign-in". + ``NovaAuthPermanentError`` and any error flagged ``is_permanent`` belong + here whatever their status: they say re-authentication is required. """ 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).""" + """The server answered and refused for any other reason. + + Covers 5xx, a Nova logic error, and the non-credential client rejections -- + 400, 404, 405, 409, 422 -- that arrive as ``NovaAuthError`` despite naming + no credential problem. The enum has no ``REJECTED_CLIENT`` member because + no consumer branches on the distinction; what the two share, and what the + name is chosen for, is that the SERVER answered, so the transport worked + and no cooldown may be armed. For an HTTP refusal the log line names the + status; a Nova logic error carries no HTTP status (the request was a 200) + and its log line names the payload error code instead. + """ TRANSPORT_FAILED = "transport_failed" """No usable answer was obtained: DNS, connect refused/timeout, disconnect, @@ -137,9 +154,14 @@ class StopSoundOutcome(StrEnum): The two failure states are kept apart for the same reason the middle state exists: they differ in what the user has to do about them. ``SUPPRESSED`` - never left this machine and clears itself; ``FAILED`` reached the transport - and may need credentials, a network, or patience with a rate limit. One - message cannot advise on both without misleading half its readers. + clears itself and is answered by waiting; ``FAILED`` may need credentials, a + network, or patience with a rate limit. One message cannot advise on both + without misleading half its readers. + + They are NOT told apart by how far the request travelled. Both contain a + member that never left the machine -- see ``FAILED`` on + ``SoundDispatchOutcome.NOT_SENT`` -- and reading "did it reach the wire" + as the criterion is what makes a missing action token look suppressible. Note that even ``CANCELLED`` means "submitted with a correlated cancel key", not "the device stopped". Nova returns no parsable ExecuteActionResponse, so @@ -169,16 +191,37 @@ class StopSoundOutcome(StrEnum): Today the sole cause is a push transport that is not ready yet. The command never reached the network, the condition is local and transient, and the only useful advice is to retry shortly. + + "Never sent" alone does not qualify: ``SoundDispatchOutcome.NOT_SENT`` (the + readiness gate let the command through, but no action token was to be had) + never leaves the machine either and is filed under ``FAILED``. The dividing + line is the remedy, not the distance travelled -- see ``FAILED``. """ FAILED = "failed" - """Handed to the transport, but not accepted. - - Covers every way the attempt can die once it leaves this integration: - missing action token, authentication failure, HTTP 401/403, server error, - rate limit, network error, and an empty reply. These need different remedies - from ``SUPPRESSED`` -- often re-authentication -- so telling the user to - "try again in a moment" would be wrong for most of them. + """The attempt was made, and it did not succeed. + + Covers every way it can die once this integration has committed to making + it: authentication failure, HTTP 401/403, a server rejection, a rate limit, + a network error, an empty reply -- and the missing action token, which + never reaches the wire. + + That last member is why this docstring does not read "handed to the + transport". ``SoundDispatchOutcome.NOT_SENT`` is filed here rather than + under ``SUPPRESSED`` because the two failure states are told apart by the + REMEDY they call for, not by how far the request travelled. ``SUPPRESSED`` + means "the push transport is not up yet, try again in a moment", and it is + reached only by the readiness gate saying so. ``NOT_SENT`` is what the gate + could NOT see: it inspects the receiver's state (``is_push_ready``) while + the token is fetched separately (``_get_fcm_token_for_action``), so a + command the gate waved through can still find no action token. That is a + setup or credential problem far more often than a moment's patience, and + the advice such a user needs is the one ``stop_sound_rejected`` already + gives -- "an expired sign-in, a missing token, a server or network error, + or a rate limit", plus a look at the log. Re-authentication is the usual + remedy across this member, which is why "try again in a moment" would be + wrong for most of it. Routing ``NOT_SENT`` to ``SUPPRESSED`` would answer a + missing token with an unbounded "try again shortly". """ diff --git a/custom_components/googlefindmy/coordinator/locate.py b/custom_components/googlefindmy/coordinator/locate.py index 651d98b32..7b4f64733 100644 --- a/custom_components/googlefindmy/coordinator/locate.py +++ b/custom_components/googlefindmy/coordinator/locate.py @@ -941,10 +941,13 @@ async def async_stop_sound( A :class:`StopSoundOutcome`. The state space is four-valued on purpose: ``CANCELLED`` (submitted with a correlated cancel key), ``UNCORRELATED`` (submitted without one, so nothing proves an - effect), ``SUPPRESSED`` (never sent by this integration) and - ``FAILED`` (handed to the transport and not accepted). A bool cannot - carry the middle state, and collapsing it into success is what - reported a stop for a ring that kept playing (BSkando#195). + effect), ``SUPPRESSED`` (declined here because the push transport is + not up yet, so waiting is the remedy) and ``FAILED`` (attempted and + unsuccessful, which includes the missing action token that never + reaches the wire). A bool cannot carry the middle state, and + collapsing it into success is what reported a stop for a ring that + kept playing (BSkando#195). The two failure states split by REMEDY, + not by distance travelled; see ``StopSoundOutcome``. """ # Blank means "no opinion" -- an optional field left empty, a template # that rendered to nothing -- so it must fall through to the cached key diff --git a/custom_components/googlefindmy/services.py b/custom_components/googlefindmy/services.py index acd2c5528..1e7ee4cc5 100644 --- a/custom_components/googlefindmy/services.py +++ b/custom_components/googlefindmy/services.py @@ -962,13 +962,15 @@ async def async_stop_sound_service(call: ServiceCall) -> None: translation_placeholders=placeholders, ) if outcome is StopSoundOutcome.FAILED: - # Distinct from SUPPRESSED: this one reached the transport and - # was refused. The api layer swallows every exception and - # returns False, so auth failures, 401/403, server errors, rate - # limits, network errors and empty replies all arrive here. - # "Try again in a moment" -- the suppressed advice -- is wrong - # for most of them, so this branch gets its own message and - # points at the log, which does carry the specific cause. + # Distinct from SUPPRESSED, and not by distance travelled: + # auth failures, 401/403, server rejections, rate limits, + # network errors, empty replies AND a missing action token all + # arrive here, because none of them is answered by waiting a + # moment. SUPPRESSED is reserved for the one condition that is + # -- a push transport that has not come up yet. See + # StopSoundOutcome.FAILED for the full rule. So this branch + # gets its own message and points at the log, which does carry + # the specific cause. raise _service_validation_error( "Stop sound for '{device_id}' was rejected".format(**placeholders), translation_key="stop_sound_rejected", diff --git a/docs/PLAY_SOUND_ARCHITECTURE.md b/docs/PLAY_SOUND_ARCHITECTURE.md index d654c5364..abacdeb42 100644 --- a/docs/PLAY_SOUND_ARCHITECTURE.md +++ b/docs/PLAY_SOUND_ARCHITECTURE.md @@ -163,9 +163,9 @@ 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_AUTH` | yes, on credentials (HTTP 401/403, or a permanent auth error) | no | | `REJECTED_RATE_LIMIT` | yes, HTTP 429 | no | -| `REJECTED_SERVER` | yes, for any other reason | no | +| `REJECTED_SERVER` | yes, for any other reason (5xx, a Nova logic error, and the non-credential 4xx such as 400/404) | 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 | diff --git a/tests/test_api_basics.py b/tests/test_api_basics.py index 29920f92f..5b72db4c0 100644 --- a/tests/test_api_basics.py +++ b/tests/test_api_basics.py @@ -1325,6 +1325,25 @@ def test_pre_dispatch_network_failure_drops_cancel_key( ("raised", "expected"), [ (NovaAuthError(401, "expired"), SoundDispatchOutcome.REJECTED_AUTH), + (NovaAuthError(403, "forbidden"), SoundDispatchOutcome.REJECTED_AUTH), + # NovaAuthError is raised for EVERY non-retryable 4xx, not only for + # credential rejections: nova_request.py names "403 Forbidden, 404 + # Not Found" in the very comment above the raise, and + # HTTP_RETRY_ELIGIBLE holds no 4xx besides 408 and 429. A missing + # device or a malformed request must therefore not be reported as + # REJECTED_AUTH, whose contract is "refused on credentials". + ( + NovaAuthError(404, "no such device"), + SoundDispatchOutcome.REJECTED_SERVER, + ), + (NovaAuthError(400, "bad request"), SoundDispatchOutcome.REJECTED_SERVER), + # Permanence outranks the status code: the subclass exists to say + # "re-authentication is definitively required", so it stays AUTH + # even if it ever carries a status outside 401/403. + ( + api_module.NovaAuthPermanentError(404, "aas rejected"), + SoundDispatchOutcome.REJECTED_AUTH, + ), (NovaHTTPError(403, "forbidden"), SoundDispatchOutcome.REJECTED_AUTH), (NovaHTTPError(503, "unavailable"), SoundDispatchOutcome.REJECTED_SERVER), (NovaRateLimitError("slow down"), SoundDispatchOutcome.REJECTED_RATE_LIMIT), @@ -1356,6 +1375,87 @@ def test_play_sound_classifies_every_exit( self._patch_submit(monkeypatch, None, raises=raised) assert run_coro(api.async_play_sound("d")).outcome is expected + def test_a_non_auth_4xx_does_not_log_an_authentication_failure( + self, monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture + ) -> None: + """The log line IS the user-visible effect of this classification. + + Both outcomes reach the same ``StopSoundOutcome.FAILED`` and the same + ``stop_sound_rejected`` message downstream, so nothing but the log tells + the user whether to check their sign-in or their device list. Pinning + only the returned enum would leave the whole point of the split + unguarded: the branch could collapse back to a single + ``_LOGGER.error("Authentication failed ...")`` with every enum + assertion still green. Mirrors the "assert both the warning log and the + exception" rule in ``tests/AGENTS.md``. + """ + + api = self._api_with_token(monkeypatch) + self._patch_generate_uuid(monkeypatch) + self._patch_submit(monkeypatch, None, raises=NovaAuthError(404, "gone")) + with caplog.at_level(logging.DEBUG): + assert ( + run_coro(api.async_play_sound("d")).outcome + is SoundDispatchOutcome.REJECTED_SERVER + ) + assert "Client error (HTTP 404)" in caplog.text + assert "Authentication failed" not in caplog.text + # The level is part of the user-visible effect: HA's log panel and its + # error counter treat ERROR differently from WARNING, so a silent + # promotion would put a deleted device back among the alarms. + levels = { + r.levelno for r in caplog.records if "Client error (HTTP 404)" in r.message + } + assert levels == {logging.WARNING} + + def test_a_credential_rejection_still_logs_an_authentication_failure( + self, monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture + ) -> None: + """The counterpart: 403 must keep naming credentials. + + Without this row the previous test is satisfied by a branch that never + says "Authentication failed" at all, which would bury a real expired + sign-in instead of the reverse. + """ + + api = self._api_with_token(monkeypatch) + self._patch_generate_uuid(monkeypatch) + self._patch_submit(monkeypatch, None, raises=NovaAuthError(403, "denied")) + with caplog.at_level(logging.DEBUG): + assert ( + run_coro(api.async_play_sound("d")).outcome + is SoundDispatchOutcome.REJECTED_AUTH + ) + assert "Authentication failed" in caplog.text + assert "Client error (HTTP 403)" not in caplog.text + levels = { + r.levelno for r in caplog.records if "Authentication failed" in r.message + } + assert levels == {logging.ERROR} + + def test_auth_error_without_a_readable_status_stays_auth( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """A NovaAuthError whose status cannot be read keeps the old verdict. + + Every real instance carries one (the constructor takes it positionally), + so this is the test-double and future-subclass case. The conservative + reading of a type named "auth" is auth, and pinning it here keeps the + fallback from being a claim in a comment. + """ + + api = self._api_with_token(monkeypatch) + self._patch_generate_uuid(monkeypatch) + err = NovaAuthError(404, "double without a status") + # delattr, not `= None`: the annotation says int, and removing the + # instance attribute is exactly what makes getattr fall back. + delattr(err, "status") + self._patch_submit(monkeypatch, None, raises=err) + assert ( + run_coro(api.async_play_sound("d")).outcome + is SoundDispatchOutcome.REJECTED_AUTH + ) + def test_missing_action_token_is_not_sent(self) -> None: """No FCM token means no transport was used, so nobody may be blamed.""" @@ -1539,6 +1639,18 @@ def test_blank_uuid_logs_as_uncorrelated_not_as_a_key( ("exc", "expected"), [ (NovaAuthError(401, "auth"), SoundDispatchOutcome.REJECTED_AUTH), + (NovaAuthError(403, "forbidden"), SoundDispatchOutcome.REJECTED_AUTH), + # Same rule as on the play path: the exception type is wider than + # its name, so the status decides. + ( + NovaAuthError(404, "no such device"), + SoundDispatchOutcome.REJECTED_SERVER, + ), + (NovaAuthError(400, "bad request"), SoundDispatchOutcome.REJECTED_SERVER), + ( + api_module.NovaAuthPermanentError(404, "aas rejected"), + SoundDispatchOutcome.REJECTED_AUTH, + ), (NovaHTTPError(403, "http"), SoundDispatchOutcome.REJECTED_AUTH), (NovaHTTPError(500, "http"), SoundDispatchOutcome.REJECTED_SERVER), (NovaRateLimitError("rate"), SoundDispatchOutcome.REJECTED_RATE_LIMIT), @@ -1568,3 +1680,55 @@ def test_documented_exceptions_are_classified( api = self._api_with_token(monkeypatch) self._patch_submit(monkeypatch, None, raises=exc) assert run_coro(api.async_stop_sound("d")) is expected + + def test_a_non_auth_4xx_does_not_log_an_authentication_failure( + self, monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture + ) -> None: + """Same log guard as on the play path, and for the same reason.""" + + api = self._api_with_token(monkeypatch) + self._patch_submit(monkeypatch, None, raises=NovaAuthError(404, "gone")) + with caplog.at_level(logging.DEBUG): + assert ( + run_coro(api.async_stop_sound("d")) + is SoundDispatchOutcome.REJECTED_SERVER + ) + assert "Client error (HTTP 404)" in caplog.text + assert "Authentication failed" not in caplog.text + # The level is part of the user-visible effect: HA's log panel and its + # error counter treat ERROR differently from WARNING, so a silent + # promotion would put a deleted device back among the alarms. + levels = { + r.levelno for r in caplog.records if "Client error (HTTP 404)" in r.message + } + assert levels == {logging.WARNING} + + def test_a_credential_rejection_still_logs_an_authentication_failure( + self, monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture + ) -> None: + """The counterpart on the stop path.""" + + api = self._api_with_token(monkeypatch) + self._patch_submit(monkeypatch, None, raises=NovaAuthError(403, "denied")) + with caplog.at_level(logging.DEBUG): + assert ( + run_coro(api.async_stop_sound("d")) + is SoundDispatchOutcome.REJECTED_AUTH + ) + assert "Authentication failed" in caplog.text + assert "Client error (HTTP 403)" not in caplog.text + levels = { + r.levelno for r in caplog.records if "Authentication failed" in r.message + } + assert levels == {logging.ERROR} + + def test_auth_error_without_a_readable_status_stays_auth( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """Same fallback as on the play path, pinned on both sides.""" + + api = self._api_with_token(monkeypatch) + err = NovaAuthError(404, "double without a status") + delattr(err, "status") + self._patch_submit(monkeypatch, None, raises=err) + assert run_coro(api.async_stop_sound("d")) is SoundDispatchOutcome.REJECTED_AUTH diff --git a/tests/test_coordinator_locate_basics.py b/tests/test_coordinator_locate_basics.py index 538231cbe..714ebc456 100644 --- a/tests/test_coordinator_locate_basics.py +++ b/tests/test_coordinator_locate_basics.py @@ -504,6 +504,9 @@ def test_the_play_parametrisation_covers_every_outcome(self) -> None: (SoundDispatchOutcome.REJECTED_AUTH, StopSoundOutcome.FAILED, False, False), (SoundDispatchOutcome.REJECTED_RATE_LIMIT, StopSoundOutcome.FAILED, False, False), (SoundDispatchOutcome.REJECTED_SERVER, StopSoundOutcome.FAILED, False, False), + # NOT_SENT is FAILED, not SUPPRESSED: it never reached the wire, but a + # receiver that is up and yields no action token is not answered by + # waiting. StopSoundOutcome.FAILED states the rule; this row enforces it. (SoundDispatchOutcome.NOT_SENT, StopSoundOutcome.FAILED, False, False), (SoundDispatchOutcome.INTERNAL_ERROR, StopSoundOutcome.FAILED, False, False), (SoundDispatchOutcome.TRANSPORT_FAILED, StopSoundOutcome.FAILED, True, False), @@ -580,10 +583,11 @@ async def test_rejected_submission_is_failed_not_suppressed( ) -> None: """A stop the transport refused must not claim a local, transient cause. - ``api.async_stop_sound`` swallows every exception and returns False, so - auth failures, 401/403, server errors, rate limits and network errors - all arrive as a plain False. Reporting them as SUPPRESSED would tell a - user with an expired sign-in to wait a moment. + ``api.async_stop_sound`` swallows every exception and returns a + ``SoundDispatchOutcome`` instead of raising, so auth failures, 401/403, + server errors, rate limits and network errors all arrive as a + non-ACCEPTED value here. Reporting them as SUPPRESSED would tell a user + with an expired sign-in to wait a moment. """ coord.api.async_stop_sound.return_value = SoundDispatchOutcome.TRANSPORT_FAILED diff --git a/tests/test_services_handler_contracts.py b/tests/test_services_handler_contracts.py index 82bf344a4..dddd0d3f4 100644 --- a/tests/test_services_handler_contracts.py +++ b/tests/test_services_handler_contracts.py @@ -420,10 +420,11 @@ async def test_rejected_stop_does_not_borrow_the_suppressed_advice( ) -> None: """A refused stop must not be reported as a local, transient condition. - SUPPRESSED means "we never sent it, retry shortly". FAILED means the - transport refused it, most often because the sign-in expired. Sharing - one message would give the wrong advice to whichever half is not the - actual cause. + SUPPRESSED means "the push transport is not up yet, retry shortly". + FAILED means the attempt was made and failed, most often because the + sign-in expired. Sharing one message would give the wrong advice to + whichever half is not the actual cause. Note that "was it sent" is not + the criterion -- SoundDispatchOutcome.NOT_SENT is a FAILED as well. """ coord = SimpleNamespace(