Repository navigation
Conversation
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.
There was a problem hiding this comment.
🟡 Changes recommended
Timeout documentation remains contradictory, and the new connection-timeout behavior lacks direct regression coverage.
3 open findings
What changed in this PR
Prevents duplicate write operations after ambiguous network failures while preserving retries for unsent and idempotent requests.
Changes:
- Splits retry policies by request safety.
- Marks safe POST endpoints as idempotent.
- Adds regression tests and resiliency documentation.
| File | Description |
|---|---|
pyoverkiz/client.py |
Applies safety-aware retry policies. |
tests/test_client.py |
Tests retry and non-retry behavior. |
docs/resiliency.md |
Documents updated retry semantics. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| side_effect=[ | ||
| aiohttp.ClientConnectorError(MagicMock(), OSError("refused")), | ||
| resp, | ||
| ], |
There was a problem hiding this comment.
Now exercises both ClientConnectorError and ConnectionTimeoutError on execute_action_group; plain TimeoutError non-resend test kept. (43438db)
| | 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 | |
There was a problem hiding this comment.
TimeoutError row now explicitly excludes ConnectionTimeoutError. (43438db)
| 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. |
There was a problem hiding this comment.
Qualified the max_time note and Request timeout section as retried for idempotent requests only. (43438db)
Problem
retry_on_connection_failure(TimeoutError,ClientConnectorError,ServerDisconnectedError) wraps every request helper, including_post,_put, and_delete. A timeout or dropped connection can happen after the server has already accepted a request. When that happens, writes such asexec/apply,exec/schedule, cancelling an execution, local token activation,openPairing, and developer mode get resent. A command can then run twice, or the retry hitsDuplicateActionOnDeviceErrorwhile the first one is still running. The 15s default session timeout makes this more likely.This is a v1 to v2 regression. In v1 only
fetch_eventsretried on timeouts.execute_commandsdid not retry.Fix
retry_on_unsent_request(ClientConnectorError,ConnectionTimeoutError) covers failures where no connection was made, so the request was never sent. It applies to all requests, including PUT and DELETE.retry_on_connection_failurekeeps its existing budget, but it now only applies to idempotent requests:_get,_authenticate, and_post(..., idempotent=True). Onlyevents/register,events/{id}/fetch, andreference/devices/searchpass that flag.ServerDisconnectedErroris retried without a relogin, and auth and listener registration keep their transport budgets.docs/resiliency.mdis updated.Callers of
execute_action_groupand other writes will now seeTimeoutErrororServerDisconnectedErrorraised directly instead of a silent resend.Tests
execute_action_groupis not retried onTimeoutErrororServerDisconnectedError(1 POST).execute_action_groupis still retried onClientConnectorError.fetch_events(an idempotent POST) is still retried onTimeoutError.TimeoutError.648 tests pass. ruff, ruff format, mypy, and ty are clean.