diff --git a/docs/resiliency.md b/docs/resiliency.md index 4ecfbbc6..7c6dc2c6 100644 --- a/docs/resiliency.md +++ b/docs/resiliency.md @@ -13,7 +13,8 @@ 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 | +| Connection not established — `ClientConnectorError`, `ConnectionTimeoutError` | yes | 3 tries / ~30s | reopen the connection | +| Other `TimeoutError`, `ServerDisconnectedError` | idempotent requests only | 3 tries / ~30s | reopen the connection | | `NotAuthenticatedError` (session expired) | yes | 2 tries / ~60s | authenticate without registering a listener | | `TooManyConcurrentRequestsError` | yes | 5 tries / ~120s | — | | `TooManyExecutionsError` | yes | 5 tries / ~300s | — | @@ -56,6 +57,9 @@ the caller than to keep a request hanging for minutes. For a polling loop such a `fetch_events()`, letting a call fail and retrying on the next poll tick is usually the right behaviour. +Commands and other writes are **not** resent after a timeout or disconnect: the server +may already have accepted the request, so resending could run it twice. + A dropped keep-alive socket (`ServerDisconnectedError`) is treated as a connection blip and simply retried — it does **not** trigger a re-login. A genuine session expiry is reported by the server as a `Not authenticated` response, which raises @@ -64,14 +68,15 @@ is reported by the server as a `Not authenticated` response, which raises !!! note "`max_time` does not interrupt a hung request" The `max_time` budget only bounds the scheduling of retries *between* attempts; it cannot cancel a single in-flight request. A request that hangs is bounded by the - session timeout (see below), which surfaces as a `TimeoutError` and is then retried. + session timeout (see below), which surfaces as a `TimeoutError` and is then retried + for idempotent requests only. ## Request timeout When pyOverkiz creates its own session, it applies a default per-request timeout (`total=15s`, `sock_connect=10s`). This is kept below the connection-retry budget so a -hung socket fails fast and is retried, instead of blocking on aiohttp's 300-second -default. +hung socket fails fast and is retried for idempotent requests only, instead of blocking +on aiohttp's 300-second default. If you pass your own `ClientSession`, **you own its timeout** — pyOverkiz does not override it. Configure one explicitly: diff --git a/pyoverkiz/client.py b/pyoverkiz/client.py index ff44d41a..7bd558fa 100644 --- a/pyoverkiz/client.py +++ b/pyoverkiz/client.py @@ -18,6 +18,7 @@ ClientResponse, ClientSession, ClientTimeout, + ConnectionTimeoutError, ServerDisconnectedError, ) from backoff.types import Details @@ -107,6 +108,7 @@ async def refresh_listener(invocation: Details) -> None: logger=_LOGGER, ) +# Idempotent requests only: the server may already have received the request. retry_on_connection_failure = backoff.on_exception( backoff.expo, (TimeoutError, ClientConnectorError, ServerDisconnectedError), @@ -116,6 +118,16 @@ async def refresh_listener(invocation: Details) -> None: logger=_LOGGER, ) +# Safe for any method: the request was never sent. +retry_on_unsent_request = backoff.on_exception( + backoff.expo, + (ClientConnectorError, ConnectionTimeoutError), + max_tries=3, + max_time=30, + jitter=backoff.full_jitter, + logger=_LOGGER, +) + retry_on_concurrent_requests = backoff.on_exception( backoff.expo, TooManyConcurrentRequestsError, @@ -487,7 +499,7 @@ async def register_event_listener(self) -> str: """ # Registration may invalidate the old listener even if its response is lost. self._event_listener_id = None - response = await self._post("events/register") + response = await self._post("events/register", idempotent=True) listener_id = cast(str, response.get("id")) self._event_listener_id = listener_id @@ -506,7 +518,9 @@ async def fetch_events(self) -> list[Event]: if self.event_listener_id is None: await self.register_event_listener() - response = await self._post(f"events/{self.event_listener_id}/fetch") + response = await self._post( + f"events/{self.event_listener_id}/fetch", idempotent=True + ) return converter.structure(response, list[Event]) async def unregister_event_listener(self) -> None: @@ -786,7 +800,9 @@ async def search_reference_devices( "withManufacturerReferences": True, } """ - response = await self._post("reference/devices/search", payload) + response = await self._post( + "reference/devices/search", payload, idempotent=True + ) return converter.structure(response, DeviceSearchResult) @retry_on_auth_error @@ -1096,14 +1112,28 @@ async def _get(self, path: str) -> Any: ) as response: return await self._parse_response(response) - @retry_on_connection_failure async def _post( self, path: str, payload: dict[str, Any] | None = None, data: dict[str, Any] | None = None, + *, + idempotent: bool = False, + ) -> Any: + """Make a POST request to the OverKiz API. + + Most POSTs have side effects (e.g. `exec/apply`), so timeouts and dropped + connections are only retried when `idempotent` is set. + """ + retry = retry_on_connection_failure if idempotent else retry_on_unsent_request + return await retry(self._send_post)(path, payload, data) + + async def _send_post( + self, + path: str, + payload: dict[str, Any] | None, + data: dict[str, Any] | None, ) -> Any: - """Make a POST request to the OverKiz API.""" await self._refresh_token_if_expired() async with self.session.post( @@ -1115,7 +1145,7 @@ async def _post( ) as response: return await self._parse_response(response) - @retry_on_connection_failure + @retry_on_unsent_request async def _put(self, path: str, payload: dict[str, Any] | None = None) -> Any: """Make a PUT request to the OverKiz API.""" await self._refresh_token_if_expired() @@ -1128,7 +1158,7 @@ async def _put(self, path: str, payload: dict[str, Any] | None = None) -> Any: ) as response: return await self._parse_response(response) - @retry_on_connection_failure + @retry_on_unsent_request async def _delete(self, path: str) -> None: """Make a DELETE request to the OverKiz API.""" await self._refresh_token_if_expired() diff --git a/tests/test_client.py b/tests/test_client.py index d0fcbe49..6d191acb 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -83,27 +83,73 @@ async def test_get_devices_basic(self, client: OverkizClient): assert len(devices) == 23 @pytest.mark.asyncio - async def test_backoff_retries_command_on_connection_failure( + async def test_backoff_retries_command_when_connection_not_established( self, client: OverkizClient ) -> None: - """Ensure the command path retries a transient connection failure.""" - resp = MockResponse(json.dumps({"execId": "exec-1"})) + """A command that never reached the server is safe to resend.""" + # `ConnectionTimeoutError` subclasses `TimeoutError` but is raised before sending. + for failure in ( + aiohttp.ClientConnectorError(MagicMock(), OSError("refused")), + aiohttp.ConnectionTimeoutError("connect timed out"), + ): + with ( + patch("backoff._async.asyncio.sleep", new=AsyncMock()) as sleep_mock, + patch.object( + aiohttp.ClientSession, + "post", + side_effect=[ + failure, + MockResponse(json.dumps({"execId": "exec-1"})), + ], + ) as post_mock, + ): + exec_id = await client.execute_action_group( + actions=[Action(device_url="io://1234-5678-9012/12345678")], + ) + + assert exec_id == "exec-1" + assert post_mock.call_count == 2 + assert sleep_mock.await_count == 1 + + @pytest.mark.asyncio + async def test_command_not_resent_when_server_may_have_received_it( + self, client: OverkizClient + ) -> None: + """A timeout or disconnect after sending must not execute a command twice.""" + for failure in (TimeoutError("timed out"), aiohttp.ServerDisconnectedError()): + with ( + patch("backoff._async.asyncio.sleep", new=AsyncMock()), + patch.object( + aiohttp.ClientSession, + "post", + side_effect=failure, + ) as post_mock, + pytest.raises(type(failure)), + ): + await client.execute_action_group( + actions=[Action(device_url="io://1234-5678-9012/12345678")], + ) + + assert post_mock.call_count == 1 + + @pytest.mark.asyncio + async def test_idempotent_post_retries_on_timeout( + self, client: OverkizClient + ) -> None: + """Polling events has no device side effects, so timeouts are retried.""" + client._event_listener_id = "listener-1" with ( - patch("backoff._async.asyncio.sleep", new=AsyncMock()) as sleep_mock, + patch("backoff._async.asyncio.sleep", new=AsyncMock()), patch.object( aiohttp.ClientSession, "post", - side_effect=[TimeoutError("timed out"), resp], + side_effect=[TimeoutError("timed out"), MockResponse("[]")], ) as post_mock, ): - exec_id = await client.execute_action_group( - actions=[Action(device_url="io://1234-5678-9012/12345678")], - ) + assert await client.fetch_events() == [] - assert exec_id == "exec-1" assert post_mock.call_count == 2 - assert sleep_mock.await_count == 1 @pytest.mark.asyncio async def test_backoff_gives_up_after_max_tries_on_connection_failure(