Skip to content

Improve backoff #1920

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
iMicknl wants to merge 1 commit into from
+15 −1
Closed

Improve backoff #1920

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion pyoverkiz/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,10 +81,16 @@ async def refresh_listener(invocation: Mapping[str, Any]) -> None:
retry_on_auth_error = backoff.on_exception(
backoff.expo,
(NotAuthenticatedException, ServerDisconnectedError, ClientConnectorError),
max_tries=2,
max_tries=5,
on_backoff=relogin,
)

retry_on_connection_failure = backoff.on_exception(
backoff.expo,
(TimeoutError, ClientConnectorError),
max_tries=10,
)
Comment on lines 81 to +92
Copy link

Copilot AI Jan 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ClientConnectorError is included in both retry_on_auth_error and the new retry_on_connection_failure. When both decorators are applied (e.g., fetch_events), retries become nested/multiplicative (up to 5 * 10 attempts) and relogin may run for pure connection failures. Consider making the exception sets disjoint (e.g., remove ClientConnectorError from retry_on_auth_error and handle it only via retry_on_connection_failure) so connection outages don't trigger re-auth and total retry budget is predictable.

Copilot uses AI. Check for mistakes.
Copy link
Collaborator

@tetienne tetienne Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed


retry_on_concurrent_requests = backoff.on_exception(
backoff.expo,
TooManyConcurrentRequestsException,
Expand All @@ -97,6 +103,12 @@ async def refresh_listener(invocation: Mapping[str, Any]) -> None:
max_tries=10,
)

retry_on_execution_queue_full = backoff.on_exception(
backoff.expo,
ExecutionQueueFullException,
max_tries=5,
)

retry_on_listener_error = backoff.on_exception(
backoff.expo,
(InvalidEventListenerIdException, NoRegisteredEventListenerException),
Expand Down Expand Up @@ -414,6 +426,7 @@ async def register_event_listener(self) -> str:
@retry_on_concurrent_requests
@retry_on_auth_error
@retry_on_listener_error
@retry_on_connection_failure
async def fetch_events(self) -> list[Event]:
Comment on lines 426 to 430
Copy link

Copilot AI Jan 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fetch_events is a polling-style method, but with the current decorator stack a persistent ClientConnectorError/timeout can result in very long blocking retry sequences (nested retries across retry_on_connection_failure + retry_on_auth_error). After making exception sets disjoint, consider bounding retries for this method (e.g., max_time or lower max_tries) so callers can recover/continue their polling loop promptly when the network is down.

Copilot uses AI. Check for mistakes.
"""Fetch new events from a registered event listener. Fetched events are removed.

Expand Down Expand Up @@ -460,6 +473,7 @@ async def get_api_version(self) -> str:
return cast(str, response["protocolVersion"])

@retry_on_too_many_executions
@retry_on_execution_queue_full
@retry_on_auth_error
Comment on lines 475 to 477
Copy link

Copilot AI Jan 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New retry behavior for ExecutionQueueFullException is introduced here, but there doesn't appear to be unit coverage asserting that _execute_action_group_direct retries (and eventually succeeds/fails) when the API returns EXEC_QUEUE_FULL. Adding a test that forces __post to raise ExecutionQueueFullException a few times (and stubbing backoff sleep to keep the test fast) would prevent regressions and validate the retry budget.

Copilot uses AI. Check for mistakes.
async def _execute_action_group_direct(
self,
Expand Down
Loading