fix(transport): auth / retry / MQTT hardening from v5.7.1 review - #44
Merged
tis24dev merged 8 commits intoJul 4, 2026
Merged
Conversation
Three related mis-routings in HonConnection._intercept, all where a transient
cloud response was either delivered as success or escalated to a reauth flow:
1. A non-JSON body (HTML maintenance page, Cloudflare challenge, captive portal)
raised NativeAuthError("Decode Error"), which _is_auth_error matched by class
name while the message matched no retryable pattern -> _requires_reauth True ->
ConfigEntryAuthFailed -> HA opened a spurious reauth (and, with 2FA on, asked
for a fresh OTP) for a transient hiccup. Attach the existing DECODE_ERROR code
(requires_reauth=False, ADDHON-470) so the duck-typed _requires_reauth routes
it as UpdateFailed and the coordinator just retries. Aligns with classify(),
which already maps "decode error" -> DECODE_ERROR.
2. The 401/403 retry branch also fired on token_expires_soon/token_is_expired,
so a *successful* 200 was discarded and re-sent whenever the pre-request
refresh had failed silently (refresh() -> False leaves _expires stale). For a
POST /commands/v1/send that delivered the appliance command twice. Replay now
triggers only on a real 401/403; expiry stays handled by _check_headers.
3. A 5xx/429 with a JSON body decoded cleanly and was delivered as "success"
(empty attributes, empty AWS token downstream), and the 3-attempt backoff in
async_get_appliances_data could never see a real server error. Raise a
transient RuntimeError carrying the status (no "auth" in the name) so
_is_retryable_server_error and classify() route it to that backoff.
Tests: non-JSON body carries DECODE_ERROR and does not re-login; 502/429 raise a
retryable non-auth error; a 200 under sticky expiry is sent once, not replayed.
HonAuth.refresh() returns False on a failed refresh without touching the tokens, but both call sites (_check_headers pre-request and _refresh_after_rejection on 401/403 recovery) ignored the result and bumped _refresh_gen anyway. After a failed recovery-refresh a concurrent sibling in the same burst would observe the advanced generation, skip its own refresh under the CR#3 single-flight guard, and reuse the still-stale tokens -- so its next request 401s too, costing an extra round-trip and an avoidable full re-login. Bump the generation (and copy the possibly-rotated token back) only when refresh() actually succeeds. Tests: a False refresh on both paths leaves _refresh_gen and the token untouched.
…SUBACK In _subscribe_missing, a disconnect + awscrt auto-reconnect on the SAME generation (no _start()) can land while a SUBACK is in flight. The lifecycle handlers rebind _subscribed_topics_set to a fresh empty set (the reconnected session carries no subscriptions), but the old code then added the topic to whatever set was bound at that moment -- the new one -- marking it "subscribed" on a session that never subscribed it. Because the reconnect restores _connection=True, the watchdog's post-await guard sees a healthy connection and never clears it, so that topic's realtime push stays dead until the next disconnect. Snapshot the set identity before the await and commit the topic only if the set was not rebound in the meantime; otherwise leave it missing so the next watchdog tick re-subscribes it on the fresh session. Test: a disconnect+reconnect injected during the SUBACK leaves the topic uncommitted (verified to fail without the guard).
…ack path The primary update() path already tolerates a failed load_statistics (it only carries the consumption counters) for non-auth/non-retryable errors, logging and moving on. The load_* FALLBACK path did not: ANY load_statistics failure -- even with load_attributes already loaded -- raised and made the whole appliance unavailable until the next poll. Apply the same tolerance in the fallback loop so the two paths behave consistently; load_attributes/load_commands stay fatal (that is the appliance's actual data), and auth/retryable errors still propagate so reauth and the 3-attempt backoff can act on them. Tests: a non-auth load_statistics failure is tolerated; retryable/auth failures and a load_attributes failure still fail the appliance.
If a 2FA challenge surfaces from a re-auth mid-setup (some appliances already built), submit_mfa_code() resumes by calling setup() again, which re-appends the full inventory -- duplicate appliance objects, each hydrated on every poll. The coordinator dedupes by id so no double entities appear, but the duplicated work is real. Clear the list in place at the start of setup() (never rebind: the MQTT client binds it by reference). Test: two consecutive setup() calls leave one entry per appliance, on the same list object.
…ring HonAuth.refresh() posted the token grant with params=, which puts client_id and refresh_token in the request URL -- where the secret leaks into proxy/access logs and aiohttp exception reprs (request_info.real_url). The OAuth2 token endpoint expects application/x-www-form-urlencoded; switch to data= so the parameters are form-encoded into the request body. Salesforce accepts both encodings. NOTE: verify on live before merging -- the refresh flow has no test against the real Salesforce token service, only this offline encoding assertion. Test: refresh() sends the refresh_token via data= (body), never params= (query).
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoTransport: fix auth retry routing, refresh generation, and MQTT subscribe races
AI Description
Diagram
High-Level Assessment
Files changed (10)
|
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require review |
tis24dev
force-pushed
the
fix/transport-auth-retry-mqtt
branch
from
July 4, 2026 21:40
e0ad682 to
bc03fa4
Compare
…QTT-bound list The @appliances.setter was never called in production (only test fakes define a plain attribute of the same name). Worse, it rebinds _appliances, which the MQTT client binds by reference at __init__ -- setup() deliberately clears the list IN PLACE for exactly this reason. Remove the setter so a future 'session.appliances = [...]' can't silently detach live subscriptions; the read-only property stays and documents why there is no setter.
A 429 raised the message 'hOn server error (status 429)', which is misleading in the logs (a rate-limit is not a server fault). classify() already routes it correctly via the '429' token (-> RATE_LIMITED), and _is_retryable_server_error also matches '429', so the wording carries no routing weight. Split the branch and keep the '429' token so both stay intact while the message reads accurately.
tis24dev
force-pushed
the
fix/transport-auth-retry-mqtt
branch
from
July 4, 2026 22:29
ffa8ae1 to
131d088
Compare
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
2 participants
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.
Greptile Summary
This PR fixes six transport-layer bugs identified in a cross-review of v5.7.1, covering auth error mis-routing, double command delivery, silent 5xx pass-through, refresh-generation accounting, MQTT subscription races, and inventory duplication on MFA-resumed setup.
connection.py_intercept): Non-JSON (HTML) 200s now carryDECODE_ERRORso_requires_reauthreturns False and the coordinator retries rather than prompting a 2FA re-entry; 5xx/429 responses are now raised asRuntimeErrorbefore the JSON decode so the 3-attempt backoff is no longer dead code; the replay-on-rejection condition is narrowed to real 401/403 rejections, preventing a successful POST from being sent twice whentoken_expires_soonis stuck True._check_headers/_refresh_after_rejection):_refresh_genis now advanced only on a successfulrefresh(), so a failed refresh no longer lets concurrent siblings skip their own refresh by trusting a falsely-bumped generation counter.mqtt.py_subscribe_missing): The subscribed-topics set identity is snapshotted before theawait; a topic ACKed by a dropped session is no longer committed to the fresh empty set that a same-generation auto-reconnect created mid-SUBACK.hon_client.py): The fallbackload_*path now tolerates a non-auth/non-retryableload_statisticsfailure, matching the primaryupdate()path's existing behavior.session.py):setup()clears_appliancesin-place before loading, preventing duplicates when MFA causessetup()to be called a second time. Theappliancessetter is removed since mutating the list in-place is the only safe operation given MQTT's by-reference binding.auth.py):refresh_tokenis now sent in the form body (data=) instead of the query string (params=), keeping it out of proxy logs and aiohttp exception representations.Confidence Score: 5/5
Safe to merge — all six targeted bugs have targeted fixes with dedicated regression tests, and no new paths are introduced without coverage.
Each fix is narrow and self-contained: the error-routing changes are validated end-to-end against the existing _requires_reauth / classify() chain, the gen-counter fix is covered by two independent test cases (pre-request and rejection paths), the MQTT race fix correctly uses object identity rather than value equality to detect mid-await rebinds, and the session clear is verified to operate in-place. No pre-existing invariants are broken and no new failure modes are opened.
No files require special attention — the most complex change (_intercept in connection.py) is thoroughly commented and covered by tests for every new branch.
Important Files Changed
_intercept(non-JSON → DECODE_ERROR, double-POST on token_expires_soon, silent 5xx delivery) and one in_refresh_after_rejection/_check_headers(gen bump only on successful refresh). All routing paths verified against_requires_reauth/classify()logic.params=→data=on the OAuth2 token POST, moving therefresh_tokenfrom the URL query string into the form body._ua()sets no Content-Type, so aiohttp's automaticapplication/x-www-form-urlencodedheader is uncontested._subscribe_missing: a SUBACK from a dropped session is not committed to the fresh set created by the same-generation auto-reconnect. Theref.add(topic)path is equivalent to the originalself._subscribed_topics_set.add(topic)when the identity check passes.load_statisticsfailures in the fallback path are now tolerated (matching the primaryupdate()path), while auth/retryable errors still propagate. Theloadedflag is already True from prior successful loads, so theif not loaded:guard is unaffected._appliances.clear()at the top ofsetup()(in-place, preserving MQTT's by-reference binding) and removes theappliancessetter to enforce the in-place-only contract._interceptrouting fixes and both refresh-generation paths.SubscribeMissingRebindRaceTestfaithfully simulates same-generation disconnect + reconnect mid-SUBACK, correctly verifying the set is empty after the race.FallbackLoadStatisticsToleranceTestwith four cases covering non-auth tolerance, retryable/auth propagation, and the regression guard forload_attributes.test_setup_twice_does_not_duplicate_appliancesverifies both the no-duplication invariant and that the list is mutated in-place (same object identity).test_refresh_sends_token_in_body_not_query_stringcaptures POST kwargs and verifies the token is indata, notparams.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[_intercept: loop=0] --> B[_check_headers] B -->|refresh succeeds| C[gen bumped, fresh tokens] B -->|refresh fails| D[gen unchanged, stale tokens] C --> E[Send request] D --> E E --> F{response status} F -->|401 or 403, loop=0| G[_refresh_after_rejection] G -->|refresh succeeds| H[gen bumped] G -->|refresh fails| I[gen unchanged] H --> J[recurse loop=1] I --> J F -->|401 or 403, loop=1| K[create: full re-auth] K --> L[recurse loop=2] L --> M{still 401 or 403?} M -->|Yes| N[raise NativeAuthError Login failure] M -->|No| P F -->|429| Q[raise RuntimeError: rate limited] F -->|500 or above| R[raise RuntimeError: server error] F -->|2xx non-401/403| P{JSON decode?} P -->|OK| S[yield response] P -->|Fails: HTML or CDN page| T[NativeAuthError plus DECODE_ERROR requires_reauth=False coordinator retries via UpdateFailed]%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% flowchart TD A[_intercept: loop=0] --> B[_check_headers] B -->|refresh succeeds| C[gen bumped, fresh tokens] B -->|refresh fails| D[gen unchanged, stale tokens] C --> E[Send request] D --> E E --> F{response status} F -->|401 or 403, loop=0| G[_refresh_after_rejection] G -->|refresh succeeds| H[gen bumped] G -->|refresh fails| I[gen unchanged] H --> J[recurse loop=1] I --> J F -->|401 or 403, loop=1| K[create: full re-auth] K --> L[recurse loop=2] L --> M{still 401 or 403?} M -->|Yes| N[raise NativeAuthError Login failure] M -->|No| P F -->|429| Q[raise RuntimeError: rate limited] F -->|500 or above| R[raise RuntimeError: server error] F -->|2xx non-401/403| P{JSON decode?} P -->|OK| S[yield response] P -->|Fails: HTML or CDN page| T[NativeAuthError plus DECODE_ERROR requires_reauth=False coordinator retries via UpdateFailed]Comments Outside Diff (1)
custom_components/addhon/client/transport/connection.py, line 193-200 (link)The new
if response.status >= 500check lives in theelsebranch, butelif loop >= 2 and (self.auth.token_is_expired or response.status in (401, 403))is evaluated first. Iftoken_is_expiredis True at loop 2 and the server returns 5xx, this branch fires and emitsNativeAuthError("Login failure (status 502)")rather than the cleanerRuntimeErrorfrom the 5xx guard. The routing is still correct —_is_retryable_server_errormatches "500"/"502"/"server error" in the message, so_requires_reauthreturns False — but the log line reads "Login failure" for what is a server-side fault, which can mislead log analysis.In practice
token_is_expiredshould be False immediately aftercreate()(which initialises_expires = now), so this is an edge case. Worth keeping in mind if the loop-2 condition is ever revisited.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Reviews (4): Last reviewed commit: "fix(transport): label a 429 as rate-limi..." | Re-trigger Greptile