diff --git a/docs/resiliency.md b/docs/resiliency.md index 86936454..4ecfbbc6 100644 --- a/docs/resiliency.md +++ b/docs/resiliency.md @@ -14,7 +14,7 @@ so retries always give up rather than looping indefinitely. | Failure | Retried | Budget | On retry | | --- | --- | --- | --- | | Connection errors — `TimeoutError`, `ClientConnectorError`, `ServerDisconnectedError` | yes | 3 tries / ~30s | reopen the connection | -| `NotAuthenticatedError` (session expired) | yes | 2 tries / ~60s | call `login()` | +| `NotAuthenticatedError` (session expired) | yes | 2 tries / ~60s | authenticate without registering a listener | | `TooManyConcurrentRequestsError` | yes | 5 tries / ~120s | — | | `TooManyExecutionsError` | yes | 5 tries / ~300s | — | | `ExecutionQueueFullError` | yes | 5 tries / ~120s | — | @@ -23,6 +23,22 @@ so retries always give up rather than looping indefinitely. Everything else — `BadCredentialsError`, `TooManyRequestsError`, `MaintenanceError`, `UnsupportedOperationError`, and so on — is **not** retried and is raised directly. +### Authentication and listener recovery + +Authentication requests use their own connection-retry budget. Listener registration +uses the existing HTTP request retries, so a registration outage does not restart +an already completed login or multiply the connection attempts. + +Automatic reauthentication invalidates the old listener without registering a new +one. The next event fetch registers a replacement before fetching events. A failed +registration also clears the listener ID because the server may have replaced the +old listener even if its response was lost. + +Recovery callbacks propagate failures after their retry budgets are exhausted. +For example, a login timeout remains a `TimeoutError`; the client does not suppress +it and retry an unauthenticated request. A failed listener registration similarly +stops the current fetch, and a later poll can try registration again. + ### Backoff timing Delays grow exponentially — the ceilings are 1s, 2s, 4s, 8s, … — but **full jitter** diff --git a/pyoverkiz/client.py b/pyoverkiz/client.py index 93b5f973..0be94ddb 100644 --- a/pyoverkiz/client.py +++ b/pyoverkiz/client.py @@ -83,7 +83,7 @@ def _get_client_from_invocation(invocation: Details) -> OverkizClient: async def relogin(invocation: Details) -> None: """Re-authenticate using the main `OverkizClient` instance.""" - await _get_client_from_invocation(invocation).login() + await _get_client_from_invocation(invocation).login(register_event_listener=False) async def refresh_listener(invocation: Details) -> None: @@ -293,6 +293,12 @@ async def close(self) -> None: await self._auth.close() await self.session.close() + @retry_on_connection_failure + async def _authenticate(self) -> None: + """Retry authentication without repeating listener registration.""" + await self._auth.login() + self._event_listener_id = None + async def login( self, register_event_listener: bool = True, @@ -306,14 +312,15 @@ async def login( TooManyAttemptsBannedError: When too many failed login attempts have been made. TooManyRequestsError: When the API rate limit has been exceeded. """ - await self._auth.login() + await self._authenticate() if self.server_config.api_type == APIType.LOCAL: if register_event_listener: await self.register_event_listener() else: # Validate local API token by calling a simple endpoint - await self.get_gateways() + # Auth recovery must not recurse through get_gateways' auth decorator. + await self._get("setup/gateways") return @@ -461,6 +468,7 @@ async def refresh_device_states(self, device_url: str) -> None: ) @retry_on_concurrent_requests + @retry_on_auth_error async def register_event_listener(self) -> str: """Register a new setup event listener on the current session and return a new. @@ -471,6 +479,8 @@ async def register_event_listener(self) -> str: timeout : listening sessions are expected to call the /events/{listenerId}/fetch API on a regular basis. """ + # Registration may invalidate the old listener even if its response is lost. + self._event_listener_id = None response = await self._post("events/register") listener_id = cast(str, response.get("id")) self._event_listener_id = listener_id @@ -487,6 +497,9 @@ async def fetch_events(self) -> list[Event]: Per-session rate-limit : 1 calls per 1 SECONDS period for this particular operation (polling). """ + if self.event_listener_id is None: + await self.register_event_listener() + response = await self._post(f"events/{self.event_listener_id}/fetch") return converter.structure(response, list[Event]) diff --git a/tests/test_client.py b/tests/test_client.py index c556c0b2..f17aafe4 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -195,6 +195,7 @@ async def test_fetch_events_basic( self, client: OverkizClient, fixture_name: str, event_length: int ): """Parameterised test that fetches events fixture and checks the expected count.""" + client._event_listener_id = "listener-1" with (CURRENT_DIR / "fixtures" / "event" / fixture_name).open( encoding="utf-8", ) as raw_events: @@ -207,6 +208,7 @@ async def test_fetch_events_basic( @pytest.mark.asyncio async def test_fetch_events_simple_cast(self, client: OverkizClient): """Check that event state values from the cloud (strings) are cast to appropriate types.""" + client._event_listener_id = "listener-1" with (CURRENT_DIR / "fixtures" / "event" / "events.json").open( encoding="utf-8", ) as raw_events: @@ -308,6 +310,7 @@ async def test_backoff_retries_on_concurrent_requests( @pytest.mark.asyncio async def test_fetch_events_casting(self, client: OverkizClient, fixture_name: str): """Validate that fetched event states are cast to the expected Python types for each data type.""" + client._event_listener_id = "listener-1" with (CURRENT_DIR / "fixtures" / "event" / fixture_name).open( encoding="utf-8", ) as raw_events: diff --git a/tests/test_client_recovery.py b/tests/test_client_recovery.py new file mode 100644 index 00000000..97bb8c2f --- /dev/null +++ b/tests/test_client_recovery.py @@ -0,0 +1,314 @@ +"""Regression coverage for bounded authentication and listener recovery.""" + +from __future__ import annotations + +from unittest.mock import AsyncMock, patch + +import aiohttp +import pytest + +from pyoverkiz.client import OverkizClient +from pyoverkiz.exceptions import ( + BadCredentialsError, + InvalidEventListenerIdError, + NotAuthenticatedError, +) +from tests.helpers import MockResponse + + +@pytest.mark.asyncio +async def test_registration_reauthenticates_without_double_registration( + client: OverkizClient, +) -> None: + """Auth recovery must not create a listener that the retry immediately replaces.""" + with ( + patch("backoff._async.asyncio.sleep", new=AsyncMock()), + patch.object(client._auth, "login", new=AsyncMock()) as login, + patch.object( + aiohttp.ClientSession, + "post", + side_effect=[ + NotAuthenticatedError("expired"), + MockResponse('{"id": "replacement"}'), + ], + ) as post, + ): + assert await client.register_event_listener() == "replacement" + + assert login.await_count == 1 + assert post.call_count == 2 + assert client.event_listener_id == "replacement" + await client.session.close() + + +@pytest.mark.asyncio +async def test_registration_auth_failure_is_bounded(client: OverkizClient) -> None: + """Repeated rejection must exhaust the original auth budget without recursion.""" + with ( + patch("backoff._async.asyncio.sleep", new=AsyncMock()), + patch.object(client._auth, "login", new=AsyncMock()) as login, + patch.object( + aiohttp.ClientSession, "post", side_effect=NotAuthenticatedError("expired") + ) as post, + pytest.raises(NotAuthenticatedError), + ): + await client.register_event_listener() + + assert login.await_count == 1 + assert post.call_count == 2 + await client.session.close() + + +@pytest.mark.asyncio +async def test_registration_transport_budget(client: OverkizClient) -> None: + """Registration must use one transport retry budget at the HTTP boundary.""" + with ( + patch("backoff._async.asyncio.sleep", new=AsyncMock()), + patch.object( + aiohttp.ClientSession, "post", side_effect=TimeoutError("offline") + ) as post, + pytest.raises(TimeoutError), + ): + await client.register_event_listener() + + assert post.call_count == 3 + await client.session.close() + + +@pytest.mark.asyncio +async def test_login_retries_auth_transport_only(client: OverkizClient) -> None: + """Authentication retries must finish before listener registration starts.""" + client._event_listener_id = "stale" + with ( + patch("backoff._async.asyncio.sleep", new=AsyncMock()), + patch.object( + client._auth, + "login", + new=AsyncMock(side_effect=[TimeoutError("offline"), None]), + ) as login, + patch.object( + aiohttp.ClientSession, "post", return_value=MockResponse('{"id": "new"}') + ) as post, + ): + await client.login() + + assert login.await_count == 2 + assert post.call_count == 1 + assert client.event_listener_id == "new" + await client.session.close() + + +@pytest.mark.asyncio +async def test_relogin_exhaustion_preserves_transport_error( + client: OverkizClient, +) -> None: + """A failed recovery must not retry the original request without authentication.""" + failure = TimeoutError("login offline") + with ( + patch("backoff._async.asyncio.sleep", new=AsyncMock()), + patch.object( + client._auth, "login", new=AsyncMock(side_effect=failure) + ) as login, + patch.object( + aiohttp.ClientSession, "get", side_effect=NotAuthenticatedError("expired") + ) as get, + pytest.raises(TimeoutError) as raised, + ): + await client.get_api_version() + + assert raised.value is failure + assert login.await_count == 3 + assert get.call_count == 1 + await client.session.close() + + +@pytest.mark.asyncio +async def test_listener_outage_recovers_on_next_poll(client: OverkizClient) -> None: + """An exhausted registration must stop fetching until a later poll recovers.""" + client._event_listener_id = "stale" + failure = TimeoutError("registration offline") + with ( + patch("backoff._async.asyncio.sleep", new=AsyncMock()), + patch.object( + aiohttp.ClientSession, + "post", + side_effect=[ + InvalidEventListenerIdError("stale"), + failure, + failure, + failure, + MockResponse('{"id": "new"}'), + MockResponse("[]"), + ], + ) as post, + ): + with pytest.raises(TimeoutError) as raised: + await client.fetch_events() + assert raised.value is failure + assert post.call_count == 4 + assert client.event_listener_id is None + + assert await client.fetch_events() == [] + + paths = [call.args[0].split("enduserAPI/")[1] for call in post.call_args_list] + assert paths == [ + "events/stale/fetch", + "events/register", + "events/register", + "events/register", + "events/register", + "events/new/fetch", + ] + await client.session.close() + + +@pytest.mark.asyncio +async def test_fetch_reauthenticates_and_registers_once(client: OverkizClient) -> None: + """Fetching after session expiry must use exactly one replacement listener.""" + client._event_listener_id = "stale" + with ( + patch("backoff._async.asyncio.sleep", new=AsyncMock()), + patch.object(client._auth, "login", new=AsyncMock()) as login, + patch.object( + aiohttp.ClientSession, + "post", + side_effect=[ + NotAuthenticatedError("expired"), + MockResponse('{"id": "new"}'), + MockResponse("[]"), + ], + ) as post, + ): + assert await client.fetch_events() == [] + + assert login.await_count == 1 + paths = [call.args[0].split("enduserAPI/")[1] for call in post.call_args_list] + assert paths == ["events/stale/fetch", "events/register", "events/new/fetch"] + await client.session.close() + + +@pytest.mark.asyncio +async def test_fatal_login_error_is_not_retried(client: OverkizClient) -> None: + """Bad credentials must escape auth recovery without transport retries.""" + with ( + patch("backoff._async.asyncio.sleep", new=AsyncMock()), + patch.object( + client._auth, + "login", + new=AsyncMock(side_effect=BadCredentialsError("invalid")), + ) as login, + patch.object( + aiohttp.ClientSession, "get", side_effect=NotAuthenticatedError("expired") + ) as get, + pytest.raises(BadCredentialsError), + ): + await client.get_api_version() + + assert login.await_count == 1 + assert get.call_count == 1 + await client.session.close() + + +@pytest.mark.asyncio +async def test_local_login_validation_does_not_recurse( + local_client: OverkizClient, +) -> None: + """A rejected static local token cannot be repaired by recursively logging in.""" + with ( + patch("backoff._async.asyncio.sleep", new=AsyncMock()), + patch.object( + aiohttp.ClientSession, "get", side_effect=NotAuthenticatedError("invalid") + ) as get, + patch.object( + aiohttp.ClientSession, "post", side_effect=NotAuthenticatedError("invalid") + ) as post, + pytest.raises(NotAuthenticatedError), + ): + await local_client.login(register_event_listener=False) + + assert get.call_count == 1 + assert post.call_count == 0 + await local_client.session.close() + + +@pytest.mark.asyncio +async def test_relogin_retries_real_auth_http_before_fetching( + client: OverkizClient, +) -> None: + """Exercise OAuth, listener registration, and fetch recovery without auth mocks.""" + client._event_listener_id = "stale" + with ( + patch("backoff._async.asyncio.sleep", new=AsyncMock()), + patch.object( + aiohttp.ClientSession, + "post", + side_effect=[ + NotAuthenticatedError("expired"), + TimeoutError("token endpoint offline"), + MockResponse('{"access_token": "token", "expires_in": 3600}'), + MockResponse('{"id": "new"}'), + MockResponse("[]"), + ], + ) as post, + ): + assert await client.fetch_events() == [] + + paths = [call.args[0] for call in post.call_args_list] + assert paths[0].endswith("/events/stale/fetch") + assert paths[1].endswith("/oauth/oauth/v2/token/jwt") + assert paths[2] == paths[1] + assert paths[3].endswith("/events/register") + assert paths[4].endswith("/events/new/fetch") + assert client.event_listener_id == "new" + await client.session.close() + + +@pytest.mark.asyncio +async def test_login_does_not_repeat_auth_when_registration_times_out( + client: OverkizClient, +) -> None: + """A registration outage must not restart the already completed OAuth login.""" + with ( + patch("backoff._async.asyncio.sleep", new=AsyncMock()), + patch.object( + aiohttp.ClientSession, + "post", + side_effect=[ + MockResponse('{"access_token": "token", "expires_in": 3600}'), + TimeoutError("offline"), + TimeoutError("offline"), + TimeoutError("offline"), + ], + ) as post, + pytest.raises(TimeoutError), + ): + await client.login() + + paths = [call.args[0] for call in post.call_args_list] + assert paths[0].endswith("/oauth/oauth/v2/token/jwt") + assert all(path.endswith("/events/register") for path in paths[1:]) + assert post.call_count == 4 + assert client.event_listener_id is None + await client.session.close() + + +@pytest.mark.asyncio +async def test_relogin_preserves_non_transient_client_errors( + client: OverkizClient, +) -> None: + """Invalid URLs must propagate rather than being mistaken for transient failures.""" + failure = aiohttp.InvalidURL("invalid endpoint") + with ( + patch("backoff._async.asyncio.sleep", new=AsyncMock()), + patch.object( + aiohttp.ClientSession, "get", side_effect=NotAuthenticatedError("expired") + ) as get, + patch.object(aiohttp.ClientSession, "post", side_effect=failure) as post, + pytest.raises(aiohttp.InvalidURL) as raised, + ): + await client.get_api_version() + + assert raised.value is failure + assert get.call_count == 1 + assert post.call_count == 1 + await client.session.close()