From fb6c19a1f44c7b6b31d158dadc72e543a3148924 Mon Sep 17 00:00:00 2001 From: Mick Vleeshouwer Date: Sun, 11 Oct 2026 19:51:28 +0200 Subject: [PATCH 1/4] fix: Do not resend non-idempotent requests after timeouts Timeouts and server disconnects may occur after the server accepted the request, so resending exec/apply and other writes could run a command twice. Only retry these failures for idempotent requests (GETs, auth, event listener register/fetch, reference search); retry connection setup failures for every request. --- docs/resiliency.md | 16 ++++++++++---- pyoverkiz/client.py | 46 +++++++++++++++++++++++++++++++++------ tests/test_client.py | 52 +++++++++++++++++++++++++++++++++++++++++--- 3 files changed, 100 insertions(+), 14 deletions(-) diff --git a/docs/resiliency.md b/docs/resiliency.md index 4ecfbbc6..c63d1906 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 | +| Timeouts and dropped connections — `TimeoutError`, `ServerDisconnectedError` | idempotent requests only (GETs, authentication, event listener register/fetch, reference device search) | 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,9 +57,16 @@ 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. -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 +Commands and other writes (`execute_action_group`, cancelling executions, pairing, +developer mode, and so on) are **not** resent after a `TimeoutError` or +`ServerDisconnectedError`: the server may already have accepted the request, so a +resend could run the command twice. Handle these errors yourself, for example by +checking `get_current_executions()` before trying again. + +A dropped keep-alive socket (`ServerDisconnectedError`) on an idempotent request 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 `NotAuthenticatedError` and escalates to a re-login through the separate auth policy. !!! note "`max_time` does not interrupt a hung request" diff --git a/pyoverkiz/client.py b/pyoverkiz/client.py index ff44d41a..e22d1361 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,8 @@ async def refresh_listener(invocation: Details) -> None: logger=_LOGGER, ) +# The request may already have reached the server, so only idempotent requests +# may use this; retrying a write could execute it twice. retry_on_connection_failure = backoff.on_exception( backoff.expo, (TimeoutError, ClientConnectorError, ServerDisconnectedError), @@ -116,6 +119,17 @@ async def refresh_listener(invocation: Details) -> None: logger=_LOGGER, ) +# The connection was never established, so the request was not sent and any +# method is safe to retry. +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 +501,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 +520,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 +802,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 +1114,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 +1147,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 +1160,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..c095b8c5 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -83,10 +83,10 @@ 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.""" + """A command that never reached the server is safe to resend.""" resp = MockResponse(json.dumps({"execId": "exec-1"})) with ( @@ -94,7 +94,10 @@ async def test_backoff_retries_command_on_connection_failure( patch.object( aiohttp.ClientSession, "post", - side_effect=[TimeoutError("timed out"), resp], + side_effect=[ + aiohttp.ClientConnectorError(MagicMock(), OSError("refused")), + resp, + ], ) as post_mock, ): exec_id = await client.execute_action_group( @@ -105,6 +108,49 @@ async def test_backoff_retries_command_on_connection_failure( 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, + MockResponse(json.dumps({"execId": "exec-1"})), + ], + ) 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()), + patch.object( + aiohttp.ClientSession, + "post", + side_effect=[TimeoutError("timed out"), MockResponse("[]")], + ) as post_mock, + ): + assert await client.fetch_events() == [] + + assert post_mock.call_count == 2 + @pytest.mark.asyncio async def test_backoff_gives_up_after_max_tries_on_connection_failure( self, client: OverkizClient From 43438dbd47feb13cd26a7f58cba1fd89d9e29816 Mon Sep 17 00:00:00 2001 From: Mick Vleeshouwer Date: Sun, 11 Oct 2026 19:59:03 +0200 Subject: [PATCH 2/4] test: Cover ConnectionTimeoutError retry and clarify timeout docs --- docs/resiliency.md | 9 +++++---- tests/test_client.py | 39 +++++++++++++++++++++------------------ 2 files changed, 26 insertions(+), 22 deletions(-) diff --git a/docs/resiliency.md b/docs/resiliency.md index c63d1906..63cd4e19 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 not established — `ClientConnectorError`, `ConnectionTimeoutError` | yes | 3 tries / ~30s | reopen the connection | -| Timeouts and dropped connections — `TimeoutError`, `ServerDisconnectedError` | idempotent requests only (GETs, authentication, event listener register/fetch, reference device search) | 3 tries / ~30s | reopen the connection | +| Timeouts and dropped connections — `TimeoutError` (other than `ConnectionTimeoutError`), `ServerDisconnectedError` | idempotent requests only (GETs, authentication, event listener register/fetch, reference device search) | 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 | — | @@ -72,14 +72,15 @@ 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/tests/test_client.py b/tests/test_client.py index c095b8c5..111117c2 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -87,26 +87,29 @@ async def test_backoff_retries_command_when_connection_not_established( self, client: OverkizClient ) -> None: """A command that never reached the server is safe to resend.""" - resp = MockResponse(json.dumps({"execId": "exec-1"})) - - with ( - patch("backoff._async.asyncio.sleep", new=AsyncMock()) as sleep_mock, - patch.object( - aiohttp.ClientSession, - "post", - side_effect=[ - aiohttp.ClientConnectorError(MagicMock(), OSError("refused")), - resp, - ], - ) as post_mock, + # `ConnectionTimeoutError` subclasses `TimeoutError` but is raised before sending. + for failure in ( + aiohttp.ClientConnectorError(MagicMock(), OSError("refused")), + aiohttp.ConnectionTimeoutError("connect timed out"), ): - exec_id = await client.execute_action_group( - actions=[Action(device_url="io://1234-5678-9012/12345678")], - ) + 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 + 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( From 9ce0f21bae5ad453e9c209348176f3330bb26dc0 Mon Sep 17 00:00:00 2001 From: Mick Vleeshouwer Date: Sun, 11 Oct 2026 21:11:28 +0200 Subject: [PATCH 3/4] refactor: Trim retry comments, docs, and test setup --- docs/resiliency.md | 23 +++++++++-------------- pyoverkiz/client.py | 6 ++---- tests/test_client.py | 5 +---- 3 files changed, 12 insertions(+), 22 deletions(-) diff --git a/docs/resiliency.md b/docs/resiliency.md index 63cd4e19..c34a3d74 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 not established — `ClientConnectorError`, `ConnectionTimeoutError` | yes | 3 tries / ~30s | reopen the connection | -| Timeouts and dropped connections — `TimeoutError` (other than `ConnectionTimeoutError`), `ServerDisconnectedError` | idempotent requests only (GETs, authentication, event listener register/fetch, reference device search) | 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 | — | @@ -57,30 +57,25 @@ 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 (`execute_action_group`, cancelling executions, pairing, -developer mode, and so on) are **not** resent after a `TimeoutError` or -`ServerDisconnectedError`: the server may already have accepted the request, so a -resend could run the command twice. Handle these errors yourself, for example by -checking `get_current_executions()` before trying again. +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`) on an idempotent request 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 +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 `NotAuthenticatedError` and escalates to a re-login through the separate auth policy. !!! 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 - for idempotent requests only. + session timeout (see below), which surfaces as a `TimeoutError` and is then retried. ## 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 for idempotent requests only), instead of blocking -on aiohttp's 300-second default. +hung socket fails fast and is retried, 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 e22d1361..7bd558fa 100644 --- a/pyoverkiz/client.py +++ b/pyoverkiz/client.py @@ -108,8 +108,7 @@ async def refresh_listener(invocation: Details) -> None: logger=_LOGGER, ) -# The request may already have reached the server, so only idempotent requests -# may use this; retrying a write could execute it twice. +# Idempotent requests only: the server may already have received the request. retry_on_connection_failure = backoff.on_exception( backoff.expo, (TimeoutError, ClientConnectorError, ServerDisconnectedError), @@ -119,8 +118,7 @@ async def refresh_listener(invocation: Details) -> None: logger=_LOGGER, ) -# The connection was never established, so the request was not sent and any -# method is safe to retry. +# Safe for any method: the request was never sent. retry_on_unsent_request = backoff.on_exception( backoff.expo, (ClientConnectorError, ConnectionTimeoutError), diff --git a/tests/test_client.py b/tests/test_client.py index 111117c2..6d191acb 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -122,10 +122,7 @@ async def test_command_not_resent_when_server_may_have_received_it( patch.object( aiohttp.ClientSession, "post", - side_effect=[ - failure, - MockResponse(json.dumps({"execId": "exec-1"})), - ], + side_effect=failure, ) as post_mock, pytest.raises(type(failure)), ): From ae9449a5e2b35dc06e7417d7f3d95132ee48482e Mon Sep 17 00:00:00 2001 From: Mick Vleeshouwer Date: Sun, 11 Oct 2026 21:13:20 +0200 Subject: [PATCH 4/4] docs: Qualify timeout retries as idempotent-only --- docs/resiliency.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/docs/resiliency.md b/docs/resiliency.md index c34a3d74..7c6dc2c6 100644 --- a/docs/resiliency.md +++ b/docs/resiliency.md @@ -68,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: