diff --git a/src/mytnb/client/legacy.py b/src/mytnb/client/legacy.py index 018b549..adaa99e 100644 --- a/src/mytnb/client/legacy.py +++ b/src/mytnb/client/legacy.py @@ -16,6 +16,7 @@ USER_AGENT, _check_http_status, ) +from mytnb.client.retry import RETRYABLE_STATUS_CODES, with_retry from mytnb.crypto import encrypt_request from mytnb.exceptions import ( APIError, @@ -94,22 +95,27 @@ async def post(self, endpoint: str, data: Any) -> dict: payload = encrypt_request(data, use_staging_key=self._use_staging_key) body = {"dt": payload.to_dict()} - response = await asyncio.to_thread( - self._session.post, - url, - headers=req_headers, - json=body, - timeout=int(self._timeout), - ) - logger.debug("Legacy POST %s → %s", endpoint, response.status_code) + async def _send() -> Any: + response = await asyncio.to_thread( + self._session.post, + url, + headers=req_headers, + json=body, + timeout=int(self._timeout), + ) + logger.debug("Legacy POST %s → %s", endpoint, response.status_code) - _check_http_status(response.status_code, context="legacy API") + _check_http_status(response.status_code, context="legacy API") - if response.status_code != 200: - raise APIError( - message=f"Legacy API request failed with status {response.status_code}", - error_code=str(response.status_code), - ) + if response.status_code != 200: + raise APIError( + message=f"Legacy API request failed with status {response.status_code}", + error_code=str(response.status_code), + retryable=response.status_code in RETRYABLE_STATUS_CODES, + ) + return response + + response = await with_retry(_send, logger=logger) data = response.json() diff --git a/src/mytnb/client/rest.py b/src/mytnb/client/rest.py index 950b3d2..0c6ad6a 100644 --- a/src/mytnb/client/rest.py +++ b/src/mytnb/client/rest.py @@ -10,6 +10,7 @@ from mytnb.auth import Credentials from mytnb.client.config import REST_BASE_URL, _check_http_status +from mytnb.client.retry import RETRYABLE_STATUS_CODES, with_retry from mytnb.exceptions import APIError logger = logging.getLogger(__name__) @@ -70,16 +71,26 @@ async def post( if params is None: params = {"environment": "Prod"} - response = await self._client.post( - url, - headers=req_headers, - json=body or {}, - params=params, - ) - logger.debug("REST POST %s → %s", path, response.status_code) - - _check_http_status(response.status_code) - response.raise_for_status() + async def _send() -> httpx.Response: + response = await self._client.post( + url, + headers=req_headers, + json=body or {}, + params=params, + ) + logger.debug("REST POST %s → %s", path, response.status_code) + + _check_http_status(response.status_code) + if response.status_code in RETRYABLE_STATUS_CODES: + raise APIError( + message=f"REST API request failed with status {response.status_code}", + error_code=str(response.status_code), + retryable=True, + ) + response.raise_for_status() + return response + + response = await with_retry(_send, logger=logger) data = response.json() status = data.get("statusDetail", {}) @@ -109,9 +120,19 @@ async def get( if params is None: params = {"environment": "Prod"} - response = await self._client.get(url, headers=req_headers, params=params) - logger.debug("REST GET %s → %s", path, response.status_code) - - _check_http_status(response.status_code) - response.raise_for_status() + async def _send() -> httpx.Response: + response = await self._client.get(url, headers=req_headers, params=params) + logger.debug("REST GET %s → %s", path, response.status_code) + + _check_http_status(response.status_code) + if response.status_code in RETRYABLE_STATUS_CODES: + raise APIError( + message=f"REST API request failed with status {response.status_code}", + error_code=str(response.status_code), + retryable=True, + ) + response.raise_for_status() + return response + + response = await with_retry(_send, logger=logger) return response.json() diff --git a/src/mytnb/client/retry.py b/src/mytnb/client/retry.py new file mode 100644 index 0000000..fdb3a99 --- /dev/null +++ b/src/mytnb/client/retry.py @@ -0,0 +1,92 @@ +"""Bounded retry with exponential backoff for transient transport failures. + +The myTNB APIs sit behind CloudFront/WAF and intermittently return transient +errors (a spurious 404, occasional 5xx, or a dropped connection) that succeed +on an immediate retry. This helper absorbs those blips inside a single call so +that a one-off failure does not surface as a hard error to callers. +""" + +from __future__ import annotations + +import asyncio +import logging +import random +from typing import Awaitable, Callable, TypeVar + +import httpx +from curl_cffi.requests.exceptions import RequestException as CurlRequestException + +from mytnb.exceptions import MyTNBError + +T = TypeVar("T") + +# HTTP statuses treated as transient. 404 is unusual to retry, but for this +# WAF-fronted API it is a known intermittent edge failure, not a real +# "not found". 429 is deliberately excluded — retrying a rate limiter only +# makes it worse; let RateLimitError propagate. +RETRYABLE_STATUS_CODES = frozenset({404, 500, 502, 503, 504}) + +# Network/transport-level errors that are always safe to retry. +_RETRYABLE_EXCEPTIONS = ( + httpx.TransportError, + httpx.TimeoutException, + CurlRequestException, +) + +DEFAULT_MAX_ATTEMPTS = 3 +DEFAULT_BASE_DELAY = 0.5 + + +def _is_retryable(err: Exception) -> bool: + """Return True if the error is a transient failure worth retrying.""" + if isinstance(err, _RETRYABLE_EXCEPTIONS): + return True + return isinstance(err, MyTNBError) and getattr(err, "retryable", False) + + +async def with_retry( + send: Callable[[], Awaitable[T]], + *, + attempts: int = DEFAULT_MAX_ATTEMPTS, + base_delay: float = DEFAULT_BASE_DELAY, + logger: logging.Logger | None = None, +) -> T: + """Call ``send`` with exponential backoff on transient failures. + + Args: + send: Zero-arg coroutine factory performing one request attempt. + attempts: Maximum number of attempts (including the first). + base_delay: Base backoff delay in seconds (with added jitter). + logger: Optional logger for debug messages between retries. + + Returns: + Whatever ``send`` returns on the first successful attempt. + + Raises: + ValueError: If ``attempts`` < 1 or ``base_delay`` < 0. + The last exception raised by ``send`` once attempts are exhausted, or + immediately for any non-retryable error. + """ + if attempts < 1: + raise ValueError(f"attempts must be >= 1, got {attempts}") + if base_delay < 0: + raise ValueError(f"base_delay must be >= 0, got {base_delay}") + + for attempt in range(attempts): + try: + return await send() + except Exception as err: # noqa: BLE001 - re-raised below unless retryable + if attempt >= attempts - 1 or not _is_retryable(err): + raise + delay = base_delay * 2**attempt + random.uniform(0, base_delay) + if logger is not None: + logger.debug( + "Transient request failure (attempt %d/%d), retrying in %.2fs: %s", + attempt + 1, + attempts, + delay, + err, + ) + await asyncio.sleep(delay) + # Unreachable: the loop either returns or raises on the final attempt. + raise RuntimeError("with_retry exhausted attempts without returning") diff --git a/src/mytnb/exceptions.py b/src/mytnb/exceptions.py index 401ac8e..1b0efe2 100644 --- a/src/mytnb/exceptions.py +++ b/src/mytnb/exceptions.py @@ -23,8 +23,10 @@ def __init__( message: str, error_code: str | None = None, display_message: str | None = None, + retryable: bool = False, ): self.display_message = display_message + self.retryable = retryable super().__init__(message, error_code) diff --git a/tests/test_client.py b/tests/test_client.py index c878163..fd2f56f 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -12,6 +12,7 @@ from mytnb.auth import Credentials, DeviceInfo, UserInfo from mytnb.client import MyTNBClient from mytnb.client.config import DEFAULT_API_KEY +from mytnb.client.retry import DEFAULT_MAX_ATTEMPTS from mytnb.exceptions import APIError, AuthenticationError, MyTNBError, RateLimitError # ── Fixtures ────────────────────────────────────────────────────────────── @@ -168,6 +169,49 @@ async def test_api_error_raises(self): with pytest.raises(APIError, match="Internal error"): await client._rest_transport.post("test/endpoint") + @pytest.mark.asyncio + async def test_post_retries_transient_503(self, monkeypatch): + """A transient HTTP 503 is retried and the eventual 200 succeeds.""" + monkeypatch.setattr("mytnb.client.retry.asyncio.sleep", AsyncMock()) + success = _mock_response( + {"statusDetail": {"code": "7200"}, "content": {"result": "ok"}} + ) + async with MyTNBClient(_creds()) as client: + with patch.object( + client._client, "post", new_callable=AsyncMock, + side_effect=[_mock_response({}, status_code=503), success], + ) as mock_post: + result = await client._rest_transport.post("test/endpoint") + assert result["content"]["result"] == "ok" + assert mock_post.call_count == 2 + + @pytest.mark.asyncio + async def test_post_persistent_404_raises_after_attempts(self, monkeypatch): + """A persistent HTTP 404 raises APIError after exhausting attempts.""" + monkeypatch.setattr("mytnb.client.retry.asyncio.sleep", AsyncMock()) + async with MyTNBClient(_creds()) as client: + with patch.object( + client._client, "post", new_callable=AsyncMock, + return_value=_mock_response({}, status_code=404), + ) as mock_post: + with pytest.raises(APIError, match="status 404"): + await client._rest_transport.post("test/endpoint") + assert mock_post.call_count == DEFAULT_MAX_ATTEMPTS + + @pytest.mark.asyncio + async def test_get_retries_transient_502(self, monkeypatch): + """REST GET retries a transient HTTP 502 then returns the payload.""" + monkeypatch.setattr("mytnb.client.retry.asyncio.sleep", AsyncMock()) + success = _mock_response({"content": {"result": "ok"}}) + async with MyTNBClient(_creds()) as client: + with patch.object( + client._client, "get", new_callable=AsyncMock, + side_effect=[_mock_response({}, status_code=502), success], + ) as mock_get: + result = await client._rest_transport.get("test/endpoint") + assert result["content"]["result"] == "ok" + assert mock_get.call_count == 2 + # ── Legacy API ──────────────────────────────────────────────────────────── @@ -225,6 +269,38 @@ async def test_legacy_auth_error(self): with pytest.raises(AuthenticationError): await client._legacy_transport.post("TestEndpoint", {}) + @pytest.mark.asyncio + async def test_legacy_retries_transient_404(self, monkeypatch): + """A transient HTTP 404 is retried and the eventual 200 succeeds.""" + monkeypatch.setattr("mytnb.client.retry.asyncio.sleep", AsyncMock()) + success_data = { + "d": {"isError": "false", "ErrorCode": "7200", "data": {"result": "ok"}} + } + async with MyTNBClient(_creds()) as client: + mock_session = MagicMock() + mock_session.post.side_effect = [ + _mock_tls_response({}, status_code=404), + _mock_tls_response(success_data), + ] + client._tls_session = mock_session + + result = await client._legacy_transport.post("TestEndpoint", {}) + assert result["data"]["result"] == "ok" + assert mock_session.post.call_count == 2 + + @pytest.mark.asyncio + async def test_legacy_persistent_404_raises_after_attempts(self, monkeypatch): + """A persistent HTTP 404 raises APIError after exhausting attempts.""" + monkeypatch.setattr("mytnb.client.retry.asyncio.sleep", AsyncMock()) + async with MyTNBClient(_creds()) as client: + mock_session = MagicMock() + mock_session.post.return_value = _mock_tls_response({}, status_code=404) + client._tls_session = mock_session + + with pytest.raises(APIError, match="status 404"): + await client._legacy_transport.post("TestEndpoint", {}) + assert mock_session.post.call_count == DEFAULT_MAX_ATTEMPTS + # ── Endpoint methods ───────────────────────────────────────────────────── diff --git a/tests/test_retry.py b/tests/test_retry.py new file mode 100644 index 0000000..730632c --- /dev/null +++ b/tests/test_retry.py @@ -0,0 +1,106 @@ +"""Tests for the transport retry helper.""" + +import httpx +import pytest + +from mytnb.client.retry import RETRYABLE_STATUS_CODES, with_retry +from mytnb.exceptions import APIError, AuthenticationError + + +class _Counter: + """Callable that records how many times it was awaited.""" + + def __init__(self, side_effect): + self.calls = 0 + self._side_effect = side_effect + + async def __call__(self): + self.calls += 1 + result = self._side_effect(self.calls) + if isinstance(result, Exception): + raise result + return result + + +@pytest.mark.asyncio +async def test_returns_on_first_success(): + send = _Counter(lambda n: "ok") + result = await with_retry(send, base_delay=0) + assert result == "ok" + assert send.calls == 1 + + +@pytest.mark.asyncio +async def test_retries_then_succeeds(): + def effect(n): + if n < 3: + return APIError("transient", error_code="503", retryable=True) + return "ok" + + send = _Counter(effect) + result = await with_retry(send, attempts=3, base_delay=0) + assert result == "ok" + assert send.calls == 3 + + +@pytest.mark.asyncio +async def test_gives_up_after_attempts(): + send = _Counter(lambda n: APIError("still failing", error_code="503", retryable=True)) + with pytest.raises(APIError, match="still failing"): + await with_retry(send, attempts=3, base_delay=0) + assert send.calls == 3 + + +@pytest.mark.asyncio +async def test_does_not_retry_non_retryable(): + send = _Counter(lambda n: AuthenticationError("bad creds", error_code="401")) + with pytest.raises(AuthenticationError): + await with_retry(send, attempts=3, base_delay=0) + assert send.calls == 1 + + +@pytest.mark.asyncio +async def test_non_retryable_api_error_not_retried(): + # An APIError without retryable=True (e.g. a business error) is terminal. + send = _Counter(lambda n: APIError("business error", error_code="5000")) + with pytest.raises(APIError, match="business error"): + await with_retry(send, attempts=3, base_delay=0) + assert send.calls == 1 + + +@pytest.mark.asyncio +async def test_network_error_is_retryable(): + def effect(n): + if n < 2: + return httpx.ConnectError("connection reset") + return "ok" + + send = _Counter(effect) + result = await with_retry(send, attempts=3, base_delay=0) + assert result == "ok" + assert send.calls == 2 + + +@pytest.mark.asyncio +async def test_invalid_attempts_raises(): + send = _Counter(lambda n: "ok") + with pytest.raises(ValueError, match="attempts must be >= 1"): + await with_retry(send, attempts=0, base_delay=0) + assert send.calls == 0 + + +@pytest.mark.asyncio +async def test_invalid_base_delay_raises(): + send = _Counter(lambda n: "ok") + with pytest.raises(ValueError, match="base_delay must be >= 0"): + await with_retry(send, base_delay=-1) + assert send.calls == 0 + + +def test_retryable_status_codes(): + assert 404 in RETRYABLE_STATUS_CODES + assert 503 in RETRYABLE_STATUS_CODES + # Auth / geoblock / rate-limit are handled separately, never retried here. + assert 401 not in RETRYABLE_STATUS_CODES + assert 403 not in RETRYABLE_STATUS_CODES + assert 429 not in RETRYABLE_STATUS_CODES