Make V02 authentication failures recoverable - #124
hugo-brito wants to merge 2 commits into
Conversation
2684936 to
d2b972f
Compare
|
@cdpuk please have a look ;) |
52934d2 to
e6222fc
Compare
|
Reworked this PR on fresh Addressed the review feedback:
The branch is now based on current upstream |
|
Final merge-readiness pass completed after the auth-only rewrite. Two additional lifecycle issues found during independent red-team review are now fixed:
Added regression coverage for both the no-reload behavior and active-WebSocket token propagation. Final validation:
Independent final review found no remaining high-confidence merge blockers. The prior owner feedback is fully addressed; ready for re-review. |
|
@cdpuk please have a look :) |
|
Flagging that this still bites on 1.11.1, so the incident at the top of this PR is not stale - the same failure was reported again today in #176. That log shows For whoever picks this up: I opened #177 as the twelve-line slice of it - Two details from that log that may be useful here:
|
1e83366 to
981faeb
Compare
A V02 token expires server-side with no advertised expiry, so the only signal is a rejected request. The integration had no way to tell a rejected token from an unreachable cloud, and no way to recover from either without a Home Assistant restart. A session that died kept serving its last cached state as though the spa were healthy. Classify failures at the API boundary: - `AwsIotConnectionError` is raised for a cloud that could not be reached or did not answer in time, distinct from `AwsIotAuthException` for a cloud that refused the credentials. Callers no longer have to guess from whatever aiohttp or asyncio raised underneath. - Response status is checked before the body is parsed, on the login path and on both authenticated request paths. A rejection is not always JSON, and parsing first turned a definitive 401 or 403 into a `ContentTypeError` - a `ClientError`, and therefore classified as transient and retried forever against credentials that had already been refused. 403 now counts as a rejection alongside 400 and 401. - `TIMEOUT` goes from 10s to 20s, matching the SmartSpa backend, which moved for the same reason: the cloud is routinely slower than 10s under load, and a poll that gives up early is indistinguishable from a device going offline. Act on that classification: - Setup maps an unreachable cloud to `ConfigEntryNotReady`, which Home Assistant retries with backoff. Previously any non-auth failure parked the entry in `SETUP_ERROR`, which is never retried, leaving every entity unavailable until someone reloaded the entry by hand. - A poll that is rejected re-authenticates once and polls again, recovering within a single cycle. A partial failure counts: the token is shared across the account, so one rejected device is enough to suspect it. - A poll where no device refreshed at all raises, so the coordinator reports the update as failed and entities go unavailable. Returning the cache is what let a dead session look healthy for hours. - The coordinator translates these into `ConfigEntryAuthFailed` and `UpdateFailed`, alongside the existing SmartSpa handling, keeping Home Assistant's exception vocabulary out of the API clients. Propagate a refreshed token in memory: - `AwsIotApi.update_token()` replaces the token and notifies a callback, so the per-device WebSockets are handed the new token before they next reconnect. Otherwise they keep retrying with the token that was just rejected. - The refreshed token is deliberately not written back to the config entry. That fires the entry's update listener, which reloads the integration out from under the coordinator update that just refreshed the token. The stored token is only a startup hint, since setup always authenticates afresh, so letting it go stale until the next restart costs nothing. Remove the coordinator's cycle-wide timeout. Every backend already bounds each individual HTTP call, so nothing here can hang indefinitely. A cycle-wide cap only serves to cut short the recovery paths - AWS IoT re-authenticating and re-polling, SmartSpa re-logging-in and retrying - which at a 20s per-call budget legitimately exceed any cap short enough to be worth having. Add a reauth flow for V02 entries. V02 authentication derives a token from the stored visitor ID alone, so there is nothing to ask the user for and the flow completes without showing a form. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
981faeb to
017d5a4
Compare
|
Rebased onto current Why it needed a rebase rather than a merge. Twelve commits landed upstream since the old base Why it is not stale. #176 reports this against Scope. Narrowed to V02 authentication recovery. The convergence work ("Fix B") is not in this branch and is not coming back into it — the rapid-command false-positive problem you described in review is still unresolved, and it deserves its own PR. Nothing here touches SmartSpa, dashboards, or unrelated cleanups. One decision worth your eyes, called out rather than left to look like a merge artefact: the coordinator's Relationship to #177. They overlap on one line — mapping a transient AWS IoT login failure to Your seven review threads are addressed individually in a table in the PR description, including the Validation, run in a |
`ConfigEntryNotReady` surfaces its message as the config entry's failure reason, which is what the user reads in the UI and what lands in the log line Home Assistant emits before backing off. A bare `TimeoutError` - which is what `asyncio.timeout` raises when the login deadline expires - has an empty `str()`, so a constant message left the reason saying nothing about whether the login timed out, was refused, or failed to resolve. Fall back to the exception type when the cause carries no message of its own, and keep the message when it does. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@OdynBrouwer — I picked this up; thanks, both of those details were useful, and one of them turned into a commit. On the empty
So the structural fix removes the need for a But your point still found a real gap, one step further in. The message was a constant, so the reason read identically whether the login timed out, was refused, or failed to resolve — non-empty, but not diagnosable, which is a bit off-key next to #164. Fixed in b81b70b: the cause is named when it has no message of its own, and kept when it does. Three tests cover it, including one that drives the real On the 5s→600s backoff. Agreed, and worth being precise that neither PR changes it: it's HA's On ordering. Genuinely no preference from me, and I don't think it's mine to decide — that's @cdpuk's call. Either lands as a small rebase for the other. The one thing I'd flag is that #177 alone isn't sufficient for the AWS IoT path: without the status-before-body ordering in this PR, a non-JSON 401/403 surfaces as Happy to rebase onto #177 and drop the one overlapping line if that's the preferred order. |
|
@cdpuk please have a look |
What this is now
Rebased onto current
main(f6c66a8,v1.11.1) and re-scoped to one thing: V02 authentication failures should be recoverable without a manual reload.This is the split you suggested in your closing review comment — "if that proves to be problematic, I suggest raising the authentication fix as a separate PR - this is clearly a sensible thing on its own." The convergence work ("Fix B") is gone and is not coming back in this PR; it still has the unresolved false-positive problem with rapid commands that you identified, and it belongs in its own change.
Why now: the defect is still live on v1.11.1
#176 reports it against
1.11.1, and the log in that issue is an exact trace of three of the fixes here:That is 10.036 seconds —
asyncio.timeout(TIMEOUT)withTIMEOUT = 10. TheTimeoutErrorthen escapesasync_setup_entry, so the entry lands inSETUP_ERROR, which Home Assistant never retries. Hence the reporter's summary: "After the reboot I have to manually reload the Bestway integration."The same defect recurred on a second installation on 2026-09-17 and 2026-09-18.
The changes
Classify at the API boundary
AwsIotConnectionErrorfor a cloud that could not be reached, distinct fromAwsIotAuthExceptionfor one that refused the credentials. Callers no longer guess from whatever aiohttp or asyncio raised underneath.ContentTypeError, which is aClientError, and would therefore be classified as transient and retried forever against credentials that had already been refused.403now counts as a rejection alongside400/401.TIMEOUT10s → 20s. This matchessmartspa/api.py, which already carriesTIMEOUT = 20 # the gateway can be slow; 10s caused spurious failures upstream. Manual reload needed because of timeout error #176 is the same symptom on the AWS IoT path.Act on the classification
ConfigEntryNotReady(retried with backoff) and a rejection toConfigEntryAuthFailed(starts reauth). Previously either one parked the entry inSETUP_ERROR.ConfigEntryAuthFailed/UpdateFailed, alongside the existing SmartSpa handling, which keeps Home Assistant's exception vocabulary out of the API clients.Propagate a refreshed token in memory
AwsIotApi.update_token()replaces the token and notifies a callback, so the per-device WebSockets get it before they next reconnect — otherwise they keep retrying with the token that was just rejected. It is deliberately not written back to the config entry: that fires the entry's update listener, which reloads the integration out from under the coordinator update that just refreshed the token. The stored token is only a startup hint, since setup always authenticates afresh.A reauth flow for V02 entries
V02 auth derives a token from the stored
visitor_idalone, so there is nothing to ask the user for and the flow completes without showing a form.Decision: the coordinator's aggregate timeout is removed
Flagging this explicitly rather than letting it look like a merge artefact.
mainhasasync with asyncio.timeout(30)around the whole poll cycle. All three backends already bound every individual HTTP call (asyncio.timeout(TIMEOUT)inbestway/api.py,aws_iot/api.py,smartspa/api.py), so nothing in the cycle can hang indefinitely — the cap is not a hang guard.What it does do is cut short the recovery paths. At 20s per call, a reauthenticate-and-repoll cycle on a modest account is
refresh_bindings(3+ GETs) + 1 POST per device + 1 auth POST + 1 POST per device — six sequential calls for a single-device account, so up to 120s. Any cap that reliably covers that is far longer than the 30s poll interval, which makes it meaningless. The honest options were "remove it" or "raise it to a number so large it is not a timeout"; I took the first.This also removes a latent cap on SmartSpa: its own re-login-and-retry is up to three calls at
TIMEOUT = 20, i.e. 60s, which the existing 30s cap could already guillotine.Relationship to #177
#177 implements the startup-retry slice (AWS IoT + SmartSpa) against current
main, and is the smaller change. The two overlap on one line — mapping a transient AWS IoT login failure toConfigEntryNotReady.They are not redundant in both directions:
ContentTypeError→ClientError→ classified transient → retried forever against credentials the cloud has already refused.Whichever lands first, the other is a small rebase. If you would prefer #177 to land first, say so and I will rebase this on top of it and drop the overlapping line.
Your review threads
Re-read all seven. Not marking any resolved — that is yours to do.
api.py— "Should this includerefreshed == 0? ...if self.devices and auth_failed:"test_fetch_data_reauthenticates_after_partial_auth_failurepins it: device 1 succeeds, device 2 is rejected, reauth still happens.api.py— reference tobestway-fix-b-command-convergence.md, other "Fix B" referencesgit grep -i -E 'fix.b|convergence|command_verif|bestway-fix'overcustom_componentsandtestsreturns nothing.api.py— convergence comment hard to read ("toggle field", "echo field", "reporting progress")coordinator.py— "Spa Tier C" references need removingapi.py— design issue: rapid commands cause false-positive convergence checksconst.py— the added event isn't core to the fix; use the existingconnectedentityapi.py— "raise the authentication fix as a separate PR - clearly a sensible thing on its own"Validation
Run in a
python3.14container reproducing.github/workflows/test.yamlandpre-commit.yamlexactly.origin/mainbaselinepytest(CI command)pre-commit run --all-filesThe 29 new tests were each checked against unpatched source, by reverting one file or mutating one behaviour at a time and confirming the intended tests — and only those — fail:
websocket_base.pytest_update_token_is_used_by_the_next_connectioncoordinator.pytest_coordinator_maps_aws_iot_auth_failure_to_reauth,..._to_update_failedconfig_flow.pytest_aws_iot_setup_starts_reauth_when_login_rejected__init__.pytest_aws_iot_setup_retries_when_cloud_unreachable,test_runtime_token_refresh_reaches_websockets_without_reloadingauthenticate: parse body before statustest_authenticate_checks_rejection_before_parsing_body(401, 403)authenticate: drop the transient wraptest_authenticate_wraps_timeout_as_connection_error,test_authenticate_wraps_an_expired_deadline_do_get/_do_post: parse first, drop 403test_do_get_...,test_do_post_checks_auth_status_before_parsing_body(400, 401, 403)fetch_data: never re-authenticatefetch_data: return cache when nothing refreshedtest_fetch_data_raises_when_no_device_refreshesupdate_token: skip the callbacktest_fetch_data_reauthenticates_and_propagates_token,test_runtime_token_refresh_reaches_websockets_without_reloadingtest_authenticate_wraps_an_expired_deadlinedrives a real expiringasyncio.timeoutrather than a mock raisingTimeoutError, because the conversion fromCancelledErroronly happens as the timeout block unwinds — which is what #176's traceback actually shows, and what decides whether theexceptis placed correctly.Two honest gaps:
TIMEOUT = 20has no regression test, since a timeout constant has no observable behaviour under mocked I/O; and removing the coordinator's aggregate timeout has none either, since proving it would need a >30s test against a suite that runs--timeout=9.Note on the commits
The four original commits were collapsed into one. Twelve upstream commits landed since the old base — including the
BackendApiprotocol, typedDeviceStatus, feature-based entity selection and the redaction pass — and the old slices no longer mapped onto the current code, so re-expressing them separately would have meant inventing a history that never existed. The pre-rebase head is preserved athugo-brito/ha-bestway@archive/pr124-pre-rebase.