feat(rpc): add bounded retry for transient GET failures

tests/test_rpc.py

@@ -1,5 +1,174 @@
 from __future__ import annotations
 
+import httpx
+import pytest
+
+from tests.conftest import FakeResponse, FakeSession
+from vesper.errors import CiderRpcError
+from vesper.rpc import CiderRpcClient
+
+
+def _flaky_session(fail: int, *, status: int = 503, exc: Exception | None = None) -> FakeSession:
+    """Return a FakeSession whose first ``fail`` GET attempts fail transiently.
+
+    ``exc`` (a connection error) takes precedence over a 5xx ``status``. The
+    final attempt always returns a 200 with ``{"ok": True}``.
+    """
+    calls = {"count": 0}
+
+    def responder(method: str, path: str, headers, body) -> FakeResponse:
+        calls["count"] += 1
+        if calls["count"] <= fail:
+            if exc is not None:
+                raise exc
+            return FakeResponse(status, {"status": "error"})
+        return FakeResponse(200, {"ok": True})
+
+    return FakeSession(responder)
+
+
+def test_get_retries_connection_error_then_succeeds(settings) -> None:
+    # The first attempt raises a connection error; the retry succeeds.
+    session = _flaky_session(1, exc=httpx.ConnectError("offline"))
+    client = CiderRpcClient(settings, session=session, sleep=lambda _: None)
+
+    result = client.playback_get("/is-playing")
+
+    assert result == {"ok": True}
+    assert len(session.requests) == 2  # one failure + one success
+
+
+def test_get_retries_5xx_then_succeeds(settings) -> None:
+    session = _flaky_session(2, status=503)
+    client = CiderRpcClient(settings, session=session, sleep=lambda _: None)
+
+    result = client.playback_get("/now-playing")
+
+    assert result == {"ok": True}
+    assert len(session.requests) == 3  # two failures + one success
+
+
+def test_get_exhausts_retries_and_raises(settings) -> None:
+    session = _flaky_session(3, status=503)  # more failures than retry_count=2
+    client = CiderRpcClient(settings, session=session, sleep=lambda _: None)
+
+    with pytest.raises(CiderRpcError) as exc_info:
+        client.playback_get("/is-playing")
+
+    # default cider_retry_count is 2 -> 3 total attempts
+    assert len(session.requests) == 3
+    assert exc_info.value.status_code == 503
+
+
+def test_get_uses_exponential_backoff(settings) -> None:
+    delays: list[float] = []
+    session = _flaky_session(2, status=503)
+    client = CiderRpcClient(settings, session=session, sleep=delays.append)
+
+    client.playback_get("/is-playing")
+
+    # 2 retries -> delays for attempts 0 and 1: 0.1 and 0.2
+    assert delays == [0.1, 0.2]
+
+
+def test_post_does_not_retry_connection_error(settings) -> None:
+    calls = {"count": 0}
+
+    def responder(method, path, headers, body) -> FakeResponse:
+        calls["count"] += 1
+        raise httpx.ConnectError("offline")
+
+    session = FakeSession(responder)
+    client = CiderRpcClient(settings, session=session, sleep=lambda _: None)
+
+    with pytest.raises(CiderRpcError):
+        client.playback_post("/play", {"track": "x"})
+
+    assert calls["count"] == 1  # no retries for POST
+
+
+def test_post_does_not_retry_5xx(settings) -> None:
+    calls = {"count": 0}
+
+    def responder(method, path, headers, body) -> FakeResponse:
+        calls["count"] += 1
+        return FakeResponse(503, {"status": "error"})
+
+    session = FakeSession(responder)
+    client = CiderRpcClient(settings, session=session, sleep=lambda _: None)
+
+    with pytest.raises(CiderRpcError) as exc_info:
+        client.playback_post("/play", {"track": "x"})
+
+    assert calls["count"] == 1  # POST is never retried, even on 5xx
+    assert exc_info.value.status_code == 503
+
+
+@pytest.mark.parametrize("status_code", [400, 401, 403, 409, 422])
+def test_get_does_not_retry_client_errors(settings, status_code: int) -> None:
+    calls = {"count": 0}
+
+    def responder(method, path, headers, body) -> FakeResponse:
+        calls["count"] += 1
+        return FakeResponse(status_code, {"detail": "bad request"})
+
+    session = FakeSession(responder)
+    client = CiderRpcClient(settings, session=session, sleep=lambda _: None)
+
+    with pytest.raises(CiderRpcError) as exc_info:
+        client.playback_get("/is-playing")
+
+    assert calls["count"] == 1  # 4xx is definitive, never retried
+    assert exc_info.value.status_code == status_code
+
+
+def test_get_204_returns_none_without_retry(settings) -> None:
+    calls = {"count": 0}
+
+    def responder(method, path, headers, body) -> FakeResponse:
+        calls["count"] += 1
+        return FakeResponse(204)
+
+    session = FakeSession(responder)
+    client = CiderRpcClient(settings, session=session, sleep=lambda _: None)
+
+    assert client.playback_get("/is-playing") is None
+    assert calls["count"] == 1
+
+
+def test_failure_callback_reports_once_after_retries_exhausted(settings) -> None:
+    reports: list[dict] = []
+    session = _flaky_session(3, status=503)  # exceeds retry_count=2
+    client = CiderRpcClient(
+        settings,
+        session=session,
+        failure_callback=reports.append,
+        sleep=lambda _: None,
+    )
+
+    with pytest.raises(CiderRpcError):
+        client.playback_get("/is-playing")
+
+    # The callback fires exactly once (on the final failure), not per attempt.
+    assert len(reports) == 1
+    assert reports[0]["status_code"] == 503
+
+
+def test_cider_retry_count_zero_disables_retries(settings) -> None:
+    from vesper.config import Settings as _Settings
+
+    values = {k: getattr(settings, k) for k in type(settings).model_fields if k != "config_path"}
+    values["cider_retry_count"] = 0
+    no_retry_settings = _Settings(**values)
+
+    session = _flaky_session(1, status=503)
+    client = CiderRpcClient(no_retry_settings, session=session, sleep=lambda _: None)
+
+    with pytest.raises(CiderRpcError):
+        client.playback_get("/is-playing")
+
+    assert len(session.requests) == 1  # no retry when count is 0
+
 
 def test_playback_get_uses_dual_token_headers(rpc_client) -> None:
     client, session = rpc_client

vesper/config.py

@@ -98,6 +98,7 @@ class Settings(BaseSettings):
     global_recent_tracks_limit: int = Field(default=10, ge=1, le=500)
     request_timeout_seconds: float = Field(default=60.0, gt=0)
     verify_tls: bool = True
+    cider_retry_count: int = Field(default=2, ge=0)
     log_level: str = "INFO"
     historian_enabled: bool = False
     historian_base_url: str = "http://127.0.0.1:8768"
@@ -262,6 +263,7 @@ def sanitized(self) -> dict[str, Any]:
             "global_recent_tracks_limit": self.global_recent_tracks_limit,
             "request_timeout_seconds": self.request_timeout_seconds,
             "verify_tls": self.verify_tls,
+            "cider_retry_count": self.cider_retry_count,
             "log_level": self.log_level,
             "historian_enabled": self.historian_enabled,
             "historian_base_url": self.historian_base_url,

vesper/rpc.py

@@ -3,6 +3,7 @@
 from __future__ import annotations
 
 import re
+import time
 from typing import Any, Callable
 from urllib.parse import quote
 
@@ -27,6 +28,20 @@ def _sanitize_storefront(storefront: str) -> str:
     return "us"
 
 
+class _TransientRequestError(Exception):
+    """Internal signal for a retriable Cider RPC failure.
+
+    Carries the same ``status_code``/``detail`` shape as :class:`CiderRpcError`
+    so the final error surfaced after retries are exhausted is identical to the
+    single-attempt failure that produced it.
+    """
+
+    def __init__(self, message: str, status_code: int | None = None, detail: str | None = None) -> None:
+        super().__init__(message)
+        self.status_code = status_code
+        self.detail = detail
+
+
 class CiderRpcClient:
     """HTTP client wrapper around Cider's local RPC API."""
 
@@ -35,9 +50,11 @@ def __init__(
         settings: Settings,
         session: httpx.Client | None = None,
         failure_callback: Callable[[dict[str, Any]], None] | None = None,
+        sleep: Callable[[float], None] = time.sleep,
     ) -> None:
         self._settings = settings
         self._failure_callback = failure_callback
+        self._sleep = sleep
         self._session = session or httpx.Client(
             base_url=settings.cider_base_url,
             timeout=settings.request_timeout_seconds,
@@ -85,15 +102,55 @@ def search_library(self, query: str, *, limit: int = 10, types: list[str] | None
         return self.run_amapi_v3(f"/v1/me/library/search?term={encoded_query}&types={encoded_types}&limit={limit}")
 
     def _request(self, method: str, path: str, json_body: dict[str, Any] | None = None) -> Any:
+        # Only safe, idempotent reads are retried. Cider playback POSTs
+        # (play, next, volume, queue) are stateful and not idempotent, so a
+        # retry could duplicate a side effect. ``run_amapi_v3`` issues POSTs
+        # too (including catalog/library searches); those are also skipped to
+        # stay conservative -- they are not known to be idempotent.
+        retryable = method == "GET"
+        retry_count = self._settings.cider_retry_count if retryable else 0
+
+        last_error: _TransientRequestError | None = None
+        for attempt in range(retry_count + 1):
+            try:
+                return self._send_once(method, path, json_body)
+            except _TransientRequestError as exc:
+                last_error = exc
+                if attempt < retry_count:
+                    self._sleep(0.1 * (2**attempt))
+                    continue
+                break
+
+        # Retries exhausted: report once and surface the final failure.
+        assert last_error is not None
+        self._report_failure(method, path, last_error.status_code, str(last_error))
+        raise CiderRpcError(
+            str(last_error),
+            status_code=last_error.status_code,
+            detail=last_error.detail,
+        ) from last_error
+
+    def _send_once(self, method: str, path: str, json_body: dict[str, Any] | None) -> Any:
+        """Perform a single Cider RPC attempt.
+
+        Raises :class:`_TransientRequestError` for retriable failures
+        (connection errors and 5xx responses) so :meth:`_request` can decide
+        whether to retry. All other errors (4xx, non-JSON, etc.) raise
+        :class:`CiderRpcError` immediately and are not retried.
+        """
         headers: dict[str, str] = {}
         if self._settings.cider_api_token:
             headers["apptoken"] = self._settings.cider_api_token
             headers["apitoken"] = self._settings.cider_api_token
         try:
             response = self._session.request(method, path, headers=headers, json=json_body)
         except httpx.HTTPError as exc:
-            self._report_failure(method, path, None, str(exc))
-            raise CiderRpcError(f"Could not reach Cider RPC at {self._settings.cider_base_url}: {exc}") from exc
+            # Connection-level failure (Cider down, network hiccup). Retried
+            # by the caller for safe reads; surfaces as a connection error
+            # otherwise.
+            raise _TransientRequestError(
+                f"Could not reach Cider RPC at {self._settings.cider_base_url}: {exc}"
+            ) from exc
 
         if response.status_code == 204:
             return None
@@ -107,7 +164,19 @@ def _request(self, method: str, path: str, json_body: dict[str, Any] | None = No
             detail = None
             if isinstance(payload, dict):
                 detail = payload.get("detail") or payload.get("message") or payload.get("error")
-            self._report_failure(method, path, response.status_code, str(detail or "HTTP error"))
+            message = str(detail or "HTTP error")
+            if 500 <= response.status_code < 600:
+                # Server errors are transient -- the request may have failed
+                # for an ephemeral reason and a retry is safe for idempotent
+                # reads.
+                raise _TransientRequestError(
+                    f"Cider RPC returned HTTP {response.status_code} for {method} {path}.",
+                    status_code=response.status_code,
+                    detail=detail,
+                )
+            # Client errors (4xx) are definitive: retrying would not help and
+            # could mask a genuine bad request. Report and raise immediately.
+            self._report_failure(method, path, response.status_code, message)
             raise CiderRpcError(
                 f"Cider RPC returned HTTP {response.status_code} for {method} {path}.",
                 status_code=response.status_code,