Skip to content

Make V02 authentication failures recoverable - #124

Open
hugo-brito wants to merge 2 commits into
cdpuk:mainfrom
hugo-brito:fix/v02-surface-failures-and-command-verify
Open

hugo-brito wants to merge 2 commits into
cdpuk:mainfrom
hugo-brito:fix/v02-surface-failures-and-command-verify

Conversation

@hugo-brito

@hugo-brito hugo-brito commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

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:

09:27:45.043  Authenticating visitor **REDACTED**
09:27:55.079  ERROR ... Error setting up entry Bestway Spa (V02 - EU) for bestway
              asyncio.exceptions.CancelledError

That is 10.036 seconds — asyncio.timeout(TIMEOUT) with TIMEOUT = 10. The TimeoutError then escapes async_setup_entry, so the entry lands in SETUP_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

  • AwsIotConnectionError for a cloud that could not be reached, distinct from AwsIotAuthException for one that refused the credentials. Callers no longer guess from whatever aiohttp or asyncio raised underneath.
  • Status is checked before the body is parsed — on the login path and both authenticated request paths. A rejection is not always JSON, and parsing first turned a definitive 401/403 into a ContentTypeError, which is a ClientError, and would therefore be classified as transient and retried forever against credentials that had already been refused. 403 now counts as a rejection alongside 400/401.
  • TIMEOUT 10s → 20s. This matches smartspa/api.py, which already carries TIMEOUT = 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

  • Setup maps an unreachable cloud to ConfigEntryNotReady (retried with backoff) and a rejection to ConfigEntryAuthFailed (starts reauth). Previously either one parked the entry in SETUP_ERROR.
  • A rejected poll re-authenticates once and polls again, recovering within a single cycle.
  • A poll where no device refreshed raises, so entities go unavailable rather than continuing to serve cached state. This is the reason a dead session used to look healthy for hours.
  • The coordinator translates these into 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_id alone, 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.

main has async with asyncio.timeout(30) around the whole poll cycle. All three backends already bound every individual HTTP call (asyncio.timeout(TIMEOUT) in bestway/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 to ConfigEntryNotReady.

They are not redundant in both directions:

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.

Thread Status
api.py"Should this include refreshed == 0? ... if self.devices and auth_failed:" Applied. That is the exact condition now. test_fetch_data_reauthenticates_after_partial_auth_failure pins it: device 1 succeeds, device 2 is rejected, reauth still happens.
api.pyreference to bestway-fix-b-command-convergence.md, other "Fix B" references Gone. git grep -i -E 'fix.b|convergence|command_verif|bestway-fix' over custom_components and tests returns nothing.
api.pyconvergence comment hard to read ("toggle field", "echo field", "reporting progress") Moot — that code is not in this PR.
coordinator.py"Spa Tier C" references need removing Gone, same grep.
api.pydesign issue: rapid commands cause false-positive convergence checks Not addressed, deliberately. Your timeline is correct and unresolved. It is why convergence is out of scope here.
const.pythe added event isn't core to the fix; use the existing connected entity Gone. No event or new const is added.
api.py"raise the authentication fix as a separate PR - clearly a sensible thing on its own" This is that PR.

Validation

Run in a python3.14 container reproducing .github/workflows/test.yaml and pre-commit.yaml exactly.

Gate origin/main baseline This branch
pytest (CI command) 289 passed, exit 0 318 passed, exit 0
pre-commit run --all-files all Passed, exit 0 all Passed, exit 0 (actionlint, ruff-check, ruff-format, codespell, check-json, yamllint, prettier, mypy)

The 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:

Reverted / mutated Tests that fail
websocket_base.py test_update_token_is_used_by_the_next_connection
coordinator.py test_coordinator_maps_aws_iot_auth_failure_to_reauth, ..._to_update_failed
config_flow.py 4 reauth tests + test_aws_iot_setup_starts_reauth_when_login_rejected
__init__.py test_aws_iot_setup_retries_when_cloud_unreachable, test_runtime_token_refresh_reaches_websockets_without_reloading
authenticate: parse body before status test_authenticate_checks_rejection_before_parsing_body (401, 403)
authenticate: drop the transient wrap test_authenticate_wraps_timeout_as_connection_error, test_authenticate_wraps_an_expired_deadline
_do_get / _do_post: parse first, drop 403 test_do_get_..., test_do_post_checks_auth_status_before_parsing_body (400, 401, 403)
fetch_data: never re-authenticate the 4 reauth tests
fetch_data: return cache when nothing refreshed test_fetch_data_raises_when_no_device_refreshes
update_token: skip the callback test_fetch_data_reauthenticates_and_propagates_token, test_runtime_token_refresh_reaches_websockets_without_reloading

test_authenticate_wraps_an_expired_deadline drives a real expiring asyncio.timeout rather than a mock raising TimeoutError, because the conversion from CancelledError only happens as the timeout block unwinds — which is what #176's traceback actually shows, and what decides whether the except is placed correctly.

Two honest gaps: TIMEOUT = 20 has 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 BackendApi protocol, typed DeviceStatus, 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 at hugo-brito/ha-bestway@archive/pr124-pre-rebase.

@hugo-brito
hugo-brito force-pushed the fix/v02-surface-failures-and-command-verify branch from 2684936 to d2b972f Compare June 30, 2026 20:55
@hugo-brito

Copy link
Copy Markdown
Contributor Author

@cdpuk please have a look ;)

Comment thread custom_components/bestway/aws_iot/api.py Outdated
Comment thread custom_components/bestway/aws_iot/api.py Outdated
Comment thread custom_components/bestway/aws_iot/api.py Outdated
Comment thread custom_components/bestway/coordinator.py Outdated
Comment thread custom_components/bestway/aws_iot/api.py Outdated
Comment thread custom_components/bestway/const.py Outdated
@hugo-brito
hugo-brito force-pushed the fix/v02-surface-failures-and-command-verify branch from 52934d2 to e6222fc Compare August 5, 2026 20:18
@hugo-brito hugo-brito changed the title fix: surface failed V02 polls and re-auth instead of serving stale data fix: make V02 authentication failures recoverable Aug 5, 2026
@hugo-brito

Copy link
Copy Markdown
Contributor Author

Reworked this PR on fresh main and replaced the previous mixed-scope history with the auth-only implementation requested in the review.

Addressed the review feedback:

  • Reauthenticate on any per-device auth failure, including when another device refreshed successfully (refreshed == 0 is no longer required); added a partial multi-device regression test.
  • Removed the command-convergence experiment entirely, including Fix B, local documentation/Tier C references, custom events, and the rapid-command false-positive behavior.
  • Kept this PR focused on authentication resilience and stale-state handling.
  • Added startup timeout retry classification, typed transient/auth failures, first-refresh and WebSocket token propagation, passwordless V02 reauth, and non-JSON auth-response handling.

The branch is now based on current upstream main (570c817). All upstream checks pass (Validate, pre-commit, and tests), plus 36 targeted Podman tests passed locally. Ready for re-review.

@hugo-brito

Copy link
Copy Markdown
Contributor Author

Final merge-readiness pass completed after the auth-only rewrite.

Two additional lifecycle issues found during independent red-team review are now fixed:

  • Runtime token refresh no longer writes config-entry data, so it does not trigger the generic update listener and race a full unload/reload against the in-flight coordinator update. Tokens are propagated in memory to the API, the WebSocket seed token, and every active V02 WebSocket; startup still persists a fresh token safely before the listener is registered.
  • Removed the coordinator's conflicting 10-second aggregate timeout. Each HTTP call remains bounded by its backend timeout, while multi-device authenticate-and-repoll recovery can now finish.

Added regression coverage for both the no-reload behavior and active-WebSocket token propagation.

Final validation:

  • Full Linux suite: 97 passed
  • Upstream Validate: passed
  • Upstream pre-commit: passed
  • Upstream tests: passed
  • Ruff 0.15.10 check/format: passed
  • Mypy: passed (19 source files)

Independent final review found no remaining high-confidence merge blockers. The prior owner feedback is fully addressed; ready for re-review.

@hugo-brito

Copy link
Copy Markdown
Contributor Author

@cdpuk please have a look :)

@OdynBrouwer

Copy link
Copy Markdown

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 _async_setup_aws_iot calling AwsIotApi.authenticate (that path deliberately re-authenticates on every setup), the login timing out ten seconds into HA's startup, and - because only AwsIotAuthException was caught there - the TimeoutError escaping async_setup_entry. Home Assistant then put the entry in SETUP_ERROR, which it never retries, so every entity stayed unavailable until the entry was reloaded by hand.

For whoever picks this up: I opened #177 as the twelve-line slice of it - TimeoutError and aiohttp.ClientError from the login now raise ConfigEntryNotReady, so the entry lands in SETUP_RETRY - with a test per cloud backend. It deliberately leaves everything else here alone (no reauth flow, no token propagation, no timeout change), so the two can land in either order. If this PR moves first, #177 is largely redundant.

Two details from that log that may be useful here:

  • Home Assistant's setup backoff is 5 s doubling to SETUP_RETRY_MAX_WAIT = 600 s, so recovery after a transient outage can take up to ten minutes even though the retry itself works.
  • A bare TimeoutError has an empty str(), which is why the reported setup error ends in a colon with nothing after it. Retry setup when the cloud login cannot be reached #177 falls back to the exception type when the message is empty.

@hugo-brito
hugo-brito force-pushed the fix/v02-surface-failures-and-command-verify branch from 1e83366 to 981faeb Compare September 18, 2026 07:31
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>
@hugo-brito
hugo-brito force-pushed the fix/v02-surface-failures-and-command-verify branch from 981faeb to 017d5a4 Compare September 18, 2026 07:35
@hugo-brito hugo-brito changed the title fix: make V02 authentication failures recoverable Make V02 authentication failures recoverable Sep 18, 2026
@hugo-brito

Copy link
Copy Markdown
Contributor Author

Rebased onto current main (f6c66a8, v1.11.1) and force-pushed. The PR description has been rewritten to match the new scope; this comment is just the summary of what moved.

Why it needed a rebase rather than a merge. Twelve commits landed upstream since the old base 570c817, including the BackendApi protocol (#147), typed DeviceStatus (#152), feature-based entity selection (#149), the SmartSpa backend (#142) and the redaction pass (#164). git merge-tree reported content conflicts in seven files, and across the four conflicting source files that was +517/−590 of upstream churn. Merging hunk-by-hunk would have produced something that compiled and meant nothing, so the failure semantics were re-expressed against the current structure instead: upstream's protocol, typed status, feature table and redact()/redact_text() logging are all kept as they are, and no unredacted log line is reintroduced.

Why it is not stale. #176 reports this against 1.11.1. The log in that issue is a direct trace of the defect: authentication starts at 09:27:45.043, dies at 09:27:55.079 — 10.036s, i.e. asyncio.timeout(TIMEOUT) with TIMEOUT = 10 — and the TimeoutError escapes async_setup_entry, parking the entry in SETUP_ERROR, which Home Assistant never retries. That is exactly the reporter's "after the reboot I have to manually reload the Bestway integration." The same failure recurred on another installation on 2026-09-17 and 2026-09-18.

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 asyncio.timeout(30) is removed. Every backend already bounds each individual HTTP call, so the aggregate is not a hang guard; what it does do is cut short the reauthenticate-and-repoll cycle, which at TIMEOUT = 20 can legitimately need well over 30s on a multi-device account. Reasoning is in the PR description under "Decision". Happy to put a large explicit number back instead if you would rather keep a cap.

Relationship to #177. They overlap on one line — mapping a transient AWS IoT login failure to ConfigEntryNotReady. #177 additionally covers SmartSpa, which this does not, and this additionally fixes the case #177 cannot: without the status-before-body ordering, a non-JSON 401/403 becomes a ContentTypeError, which is a ClientError, so #177 alone would retry a genuinely rejected login forever. Whichever lands first, the other is a small rebase. If you would prefer #177 first, say so and I will rebase on top of it and drop the overlapping line.

Your seven review threads are addressed individually in a table in the PR description, including the if self.devices and auth_failed: suggestion, which is applied verbatim and now has a test pinning the partial-failure case. I have deliberately not marked any thread resolved.

Validation, run in a python3.14 container reproducing the two workflows exactly. Baseline on unmodified origin/main first, so a failure here could be attributed: 289 → 318 tests passed, exit 0; pre-commit run --all-files green on both, including mypy. Each of the 29 new tests was then checked against unpatched source — reverting one file or mutating one behaviour at a time — to confirm it actually fails without its fix; the matrix is in the PR description. Two gaps are stated there honestly rather than glossed: TIMEOUT = 20 and the aggregate-timeout removal are not regression-tested, because neither has observable behaviour a fast mocked test can pin.

`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>
@hugo-brito

Copy link
Copy Markdown
Contributor Author

@OdynBrouwer — I picked this up; thanks, both of those details were useful, and one of them turned into a commit.

On the empty str(). You're right, and I checked whether it bites this branch. It doesn't, but not for a reason I can take credit for by default — it falls out of classifying at the API boundary. authenticate wraps the transport failure in a typed AwsIotConnectionError that is always constructed with a message, and HomeAssistantError.__str__ falls back to __cause__, so the entry reason was already populated:

raised resulting entry.reason
ConfigEntryNotReady from TimeoutError() '' ← the dangling-colon case you described
ConfigEntryNotReady from AwsIotConnectionError("Unable to reach…") 'Unable to reach the authentication service'

So the structural fix removes the need for a _describe_failure-style helper at the call site, because there is no bare exception left to re-raise from.

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.

Unable to reach the authentication service: TimeoutError
Unable to reach the authentication service: Cannot connect to host …

Three tests cover it, including one that drives the real authenticate end-to-end — bare TimeoutError from the transport, through the wrap, through ConfigEntryNotReady, onto entry.reason — rather than asserting against a mock that raises the wrapped exception directly. Both are mutation-checked: dropping the detail fails all three, and keeping str(err) without the type fallback fails exactly the two TimeoutError ones and correctly leaves the ClientError one passing.

On the 5s→600s backoff. Agreed, and worth being precise that neither PR changes it: it's HA's SETUP_RETRY_MAX_WAIT, so a restart during an outage can still take ten minutes to come good. Where this PR does help is the case that isn't a setup failure at all — a token rejected while HA is already running recovers through the runtime reauth path inside a single poll cycle (~30s) and never touches setup backoff. That's the part #177 doesn't cover, and it's the mechanism behind the original 23-hours-looking-healthy report.

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 ContentTypeError, which is a ClientError, so it gets classified transient and retried forever against credentials the cloud has already refused. Your SmartSpa half is additive either way, since nothing here touches that backend.

Happy to rebase onto #177 and drop the one overlapping line if that's the preferred order.

@hugo-brito

Copy link
Copy Markdown
Contributor Author

@cdpuk please have a look

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants