Repository navigation
fix: Bound authentication and listener recovery retries - #2262
Merged
Merged
Conversation
Automatic reauthentication no longer registers an event listener that the retried request immediately replaces, and local token validation no longer recurses through the auth retry decorator. Authentication gets its own connection-retry budget, listener registration recovers from expired sessions, and fetch_events registers a replacement listener when none is active.
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
On-demand registration nests authentication retry budgets, exceeding the documented attempt limit.
Review effort: Balanced
Findings: 1
What changed in this PR
Bounds authentication and event-listener recovery retries.
Changes:
- Separates authentication retries from listener registration.
- Adds on-demand listener recovery and regression tests.
- Documents recovery behavior.
| File | Description |
|---|---|
pyoverkiz/client.py |
Refactors authentication and listener recovery. |
tests/test_client.py |
Updates fetch-event test setup. |
tests/test_client_recovery.py |
Adds recovery regression coverage. |
docs/resiliency.md |
Documents retry semantics. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+500
to
+501
| if self.event_listener_id is None: | ||
| await self.register_event_listener() |
Owner
Author
There was a problem hiding this comment.
You're right, but this only happens if the server rejects a new login straight away. Even then, it stops after a few tries. Not worth the extra code, so I'll leave it as is.
|
Fantastic thank you. Just wanted my home to stop crashing! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Makes session and event-listener recovery bounded and non-redundant:
reloginnow callslogin(register_event_listener=False). Before, recovering fromNotAuthenticatedErrorcreated a listener that the retried call (e.g.register_event_listener) immediately replaced._authenticate()wrapsself._auth.login()inretry_on_connection_failureand clears the listener ID, so a registration outage never restarts a completed login._get("setup/gateways")instead ofget_gateways(), so a rejected token fails once instead of going back throughretry_on_auth_error→relogin.register_event_listenerrecovers from expired sessions (@retry_on_auth_error) and clears the listener ID before posting, because the server may have replaced the old listener even if the response was lost.fetch_eventsregisters a listener on demand when none is active (after relogin, or after a failed registration). A later poll can then recover.docs/resiliency.mdnow documents the recovery semantics.Behavior changes
No public signatures change. Two observable differences:
login()now clearsevent_listener_id, including whenregister_event_listener=False.fetch_events()without a registered listener now registers one instead of failing onevents/None/fetch.login(register_event_listener=False)no longer fillsclient.gateways. It still validates the token, but through_getso it cannot recurse into re-login. Nothing in Home Assistant reads this attribute.Tests
tests/test_client_recovery.pyadds 12 regression tests: no double registration, bounded auth and transport budgets, errors preserved after exhaustion,BadCredentialsError/InvalidURLnot retried, local validation without recursion, and full OAuth → register → fetch recovery without auth mocks.Supersedes #2233.