From 27f7966fd08b469abdd218c1b9bad5fa95d696a7 Mon Sep 17 00:00:00 2001 From: Mick Vleeshouwer Date: Sun, 27 Sep 2026 14:12:30 +0200 Subject: [PATCH] fix: Bound authentication and listener recovery retries Automatic reauthentication no longer registers an event listener that the retried request immediately replaces, and local token validation no longer recurses through the auth retry decorator. Authentication gets its own connection-retry budget, listener registration recovers from expired sessions, and fetch_events registers a replacement listener when none is active. --- docs/resiliency.md | 18 +- pyoverkiz/client.py | 19 +- tests/test_client.py | 3 + tests/test_client_recovery.py | 314 ++++++++++++++++++++++++++++++++++ 4 files changed, 350 insertions(+), 4 deletions(-) create mode 100644 tests/test_client_recovery.py 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()