fix: Do not resend commands after a timeout or server disconnect

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:

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()

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(