Fix iCloud polling loop dying permanently on transient errors - #181389
Fix iCloud polling loop dying permanently on transient errors#181389TeroPihlaja wants to merge 1 commit into
Conversation
|
Hey there @Quentame, @nzapponi, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
There was a problem hiding this comment.
🟡 Changes recommended
A partially initialized session can prevent setup recovery, and the remaining review findings also need resolution.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Prevents iCloud polling from permanently stopping after transient authentication, setup, or malformed-device failures.
Changes:
- Reschedules polling across failure and 2FA paths.
- Adds regression tests for recovery scenarios.
Review findings:
- Discard partially initialized sessions so later polls retry setup.
- Update outdated polling documentation.
- Restore omitted PR-template checklist sections.
- Add timer-driven regression coverage for 2FA rescheduling.
File summaries
| File | Description |
|---|---|
tests/components/icloud/test_account.py |
Adds polling recovery regression tests. |
homeassistant/components/icloud/account.py |
Adds resilient polling and setup retries; requires the noted corrections. |
Review details
Suppressed comments (4)
homeassistant/components/icloud/account.py:366
- Handle
PyiCloudFailedLoginExceptionseparately and start reauthentication. The generic retry path leaves the invalid API session installed, so credentials that expire after initial setup are retried every two minutes indefinitely without prompting the user.
except Exception:
homeassistant/components/icloud/account.py:366
- Prevent failed updates from committing partially discovered devices.
update_devices()mutates_devicesbefore validating the entire response; if a later device raises, a prior new device without a location is retained withoutsignal_device_new, and subsequent successful polls treat it as existing, so its entities are never added.
self.update_devices()
except Exception:
homeassistant/components/icloud/account.py:343
- Shorten this docstring to describe the current contract. The historical failure narrative is misleading now that exceptions are caught here and duplicates implementation details that can become stale.
This runs from a timer callback, so anything raised here escapes into
the event loop and no further fetch is ever scheduled, leaving the
devices frozen at their last known state until Home Assistant is
restarted. Every failure path therefore has to schedule the next fetch.
homeassistant/components/icloud/account.py:187
- Add a timer-driven regression test for the 2FA rescheduling path. The existing 2FA setup test only verifies that reauthentication starts; it does not advance the timer and prove polling remains armed after this new early return.
# Keep the timer running so polling resumes by itself once the
# user has entered their verification code.
self._fetch_interval = self._max_interval
self._schedule_next_fetch()
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
aeb3e84 to
d58babe
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Rejected credentials must trigger reauthentication instead of being retried indefinitely as transient failures.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
homeassistant/components/icloud/account.py:113
- Reduce this docstring to the method's current contract; describing the pre-change failure mode is historical commentary that will become stale.
"""Set up an iCloud account.
_setup() only arms the fetch timer if it gets as far as
update_devices(), so arm one here for the paths that returned early.
Without it an account that could not log in would sit loaded and idle
until Home Assistant was restarted.
homeassistant/components/icloud/account.py:369
- Make device discovery atomic, or notify listeners about devices added before an error.
update_devices()mutates_devicesinside the loop, so if a later malformed device raises, an earlier new device remains stored withoutsignal_device_new; a subsequent response that omits the malformed device treats it as existing and never creates its entities.
self.update_devices()
except Exception:
homeassistant/components/icloud/account.py:188
- Add a timer-driven regression test for this 2FA rescheduling path. The existing 2FA setup test only checks that reauthentication starts and does not prove that polling remains armed after this early return.
# Keep the timer running so polling resumes by itself once the
# user has entered their verification code.
self._fetch_interval = self._max_interval
self._schedule_next_fetch()
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Balanced
d58babe to
3a06ad3
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The rejected-session handling and non-atomic device discovery issues must be resolved before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
homeassistant/components/icloud/account.py:346
- Replace this historical narrative with a concise description of the method's current contract. The implementation now catches operation failures, so saying that anything raised here escapes describes the pre-change behavior rather than the resulting code.
"""Keep the API alive.
This runs from a timer callback and is what schedules the following
one, so anything raised here escapes into the event loop and leaves
the devices frozen at their last known state until Home Assistant is
restarted. Every path out of here therefore has to schedule the next
fetch.
"""
homeassistant/components/icloud/account.py:114
- Shorten this docstring to state the current responsibility instead of documenting the pre-change failure mode. The scheduling details are evident from the method body and can become stale as the implementation changes.
"""Set up an iCloud account.
_setup() only arms the fetch timer if it gets as far as
update_devices(), so arm one here for the paths that returned early.
Without it an account that could not log in would sit loaded and idle
until Home Assistant was restarted.
"""
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Balanced
3a06ad3 to
d40c1af
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical 2FA session handling and moderate malformed-payload isolation issues block approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
homeassistant/components/icloud/account.py:389
- Stop automatic polling after credentials are rejected. Since this branch sets
apitoNone, the scheduled callback callssetup()with the unchanged password and continues retrying indefinitely despite starting reauthentication; cancel the existing timer instead, since successful reauthentication reloads the entry.
self._fetch_interval = self._max_interval
self._schedule_next_fetch()
homeassistant/components/icloud/account.py:114
- Reduce this docstring to the method's current responsibility. The added history about how the old implementation became idle narrates the change rather than documenting a non-obvious API contract, so it will become stale as scheduling evolves.
_setup() only arms the fetch timer if it gets as far as
update_devices(), so arm one here for the paths that returned early.
Without it an account that could not log in would sit loaded and idle
until Home Assistant was restarted.
homeassistant/components/icloud/account.py:209
- Extend malformed-device isolation to every required status field and the pending-status calculation. This
tryonly protectsidandname; missingbatteryStatus, model/update fields, ordeviceStatusat line 238 still aborts the loop, so a persistent malformed payload can prevent healthy devices added earlier in the same pass from ever receivingsignal_device_new. Process each status fully inside the guarded block and derive the pending flag only from successfully parsed statuses.
try:
status = device.status(DEVICE_STATUS_SET)
device_id = status[DEVICE_ID]
device_name = status[DEVICE_NAME]
except (KeyError, IndexError, TypeError) as err:
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
d40c1af to
b931623
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Two moderate issues remain around malformed-device handling and stale reauthentication flows.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
homeassistant/components/icloud/account.py:209
- Extend malformed-device isolation through the entire per-device body. This
tryonly protectsstatus, ID, and name; missing battery, model, person, or location fields can still escape after earlier new devices were inserted into_devices. On the next poll those devices are treated as existing, sosignal_device_newis never sent and their entities can remain absent.
try:
status = device.status(DEVICE_STATUS_SET)
device_id = status[DEVICE_ID]
device_name = status[DEVICE_NAME]
except (KeyError, IndexError, TypeError) as err:
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
b931623 to
e905e0d
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Critical authentication retry behavior and malformed device identity handling remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
homeassistant/components/icloud/account.py:113
- Shorten this docstring to state the method contract without narrating the previous implementation, which can become stale as scheduling changes.
"""Set up an iCloud account.
_setup() only arms the fetch timer if it gets as far as
update_devices(), so arm one here for the paths that returned early.
Without it an account that could not log in would sit loaded and idle
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Balanced
e905e0d to
456fa57
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Transient authentication recovery, atomic device discovery, and fallback-timer coverage remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (5)
homeassistant/components/icloud/account.py:400
- Preserve automatic retries for transient failures wrapped as
PyiCloudFailedLoginException. In pyicloud 2.6.5, request failures becomePyiCloudAPIResponseException, and token authentication wraps that asPyiCloudFailedLoginException; this branch therefore starts reauth and_reauth_pending()suppresses every later login attempt, so the outage recovery described by this PR still fails. Distinguish a definitive credential rejection from a wrapped transport/API failure (preferably upstream) and test the production exception chain rather than a rawConnectionError.
except PyiCloudFailedLoginException, PyiCloudAuthRequiredException:
homeassistant/components/icloud/account.py:431
- Make device discovery atomic before converting malformed payloads into retries. A new device can be inserted into
_devicesbeforelist(api_devices)[0][DEVICE_STATUS]raises on a missing raw field; this handler then skipssignal_device_new, and later successful polls see the device as existing and never create its entities. Validate the response before mutating_devices, or otherwise ensure partially added devices are rolled back.
except Exception:
# update_devices() reschedules itself on the errors it handles;
# this covers the rest, such as a device missing fields.
_LOGGER.exception("Error updating iCloud devices, will retry")
homeassistant/components/icloud/account.py:119
- Add a regression test that proves this fallback timer is actually armed.
test_failed_login_is_not_retried_under_the_useronly checks that the service call count stays unchanged while reauth is active, which also passes on the old implementation where no timer exists; exercise an initial 2FA/setup early return and verify a later timer-driven recovery.
self._setup()
if self._unsub_fetch is None:
self._fetch_interval = self._max_interval
self._schedule_next_fetch()
homeassistant/components/icloud/account.py:215
- Condense this comment to the non-obvious reason for skipping the device. The current six-line explanation narrates implementation details and a historical failure mode.
# ones iCloud left out, so an unusable device has to be
# rejected rather than left to raise. Skipping it keeps the
# devices already collected, which would otherwise stay in
# _devices without signal_device_new ever being dispatched
# for them, so their entities were never created.
tests/components/icloud/test_account.py:269
- Update the PR description to match this no-retry behavior. It currently says a failed setup login is retried and cites
test_polling_recovers_after_failed_setup, while this replacement test explicitly verifies that failed credentials are not retried during reauthentication.
async def test_failed_login_is_not_retried_under_the_user(
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
keep_alive() runs from a timer callback and is the only thing that schedules the next fetch. Anything raised inside it - a transient network error from authenticate(), or a device payload missing the fields update_devices() indexes into - escaped into the event loop, so no further fetch was ever scheduled and every device stayed frozen at its last known state until Home Assistant was restarted. Schedule the next fetch on every failure path, including the early return when a 2FA challenge is pending, so the account resumes polling by itself once it can talk to iCloud again. Rejected credentials are handled separately, since those do not recover on their own: the user is asked to log in again, and the login is not retried while that is outstanding. Retrying underneath them would either add failed attempts against the account or quietly succeed and strand the repair they were shown. The session is kept when the account only needs a 2FA code, because async_step_reauth reuses it to validate the code and sends a None api back to the password form instead. A malformed device is now skipped rather than abandoning the cycle, which would leave the devices already collected in place without ever dispatching signal_device_new for them, so their entities were never created by any later poll. The timer was also only ever armed at the end of update_devices(), so an account whose first login failed was left loaded with no timer at all. setup() now arms one before returning either way.
456fa57 to
399e905
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Setup-time 2FA coverage is missing, and failed-login retry behavior remains inconsistent with the PR description.
Review details
Suppressed comments (4)
homeassistant/components/icloud/account.py:117
- Add a setup-time 2FA regression test for this fallback timer. The current timer tests begin from a successful setup, where
update_devices()already armed a timer, so they still pass if this fallback is removed; start withrequires_2fatrue, clear it, advance time, and verify polling recovers.
self._setup()
if self._unsub_fetch is None:
self._fetch_interval = self._max_interval
self._schedule_next_fetch()
homeassistant/components/icloud/account.py:213
- Condense this comment to the non-obvious reason for skipping the device. The current explanation narrates downstream control flow and the historical failure mode, which can become stale independently of this guard.
# ones iCloud left out, so an unusable device has to be
# rejected rather than left to raise. Skipping it keeps the
# devices already collected, which would otherwise stay in
# _devices without signal_device_new ever being dispatched
# for them, so their entities were never created.
homeassistant/components/icloud/account.py:371
- Align the failed-login path with the recovery behavior promised in the PR description. The description says a failed login is retried at the maximum interval, but
_setup()starts reauth and this condition then suppresses every later setup attempt; the new test explicitly expects no retry, so either the implementation or the stated behavior needs correction.
if self.api is None and not self._reauth_pending():
tests/components/icloud/test_account.py:269
- Update the PR description to match the no-retry behavior tested here. It currently says a failed login is retried and names
test_polling_recovers_after_failed_setup, while this replacement test and_reauth_pending()intentionally suppress retries until reauthentication completes.
async def test_failed_login_is_not_retried_under_the_user(
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
One known limitation of this PR, and the upstream fix for it.
That is right for a rejected password, but pyicloud currently reports more than that through the same exception. except (PyiCloudAPIResponseException, HTTPError) as error:
msg = "Invalid authentication token."
raise PyiCloudFailedLoginException(msg, error) from errorSo an unreachable iCloud — a connection error, DNS failure or timeout — currently arrives here looking like rejected credentials, and the account waits for a reauth that was never actually needed. I have proposed the fix upstream rather than working around it here: timlaing/pyicloud#357 adds Once that is released and the pin is bumped, this integration gets the distinction almost for free — Worth noting the scope is narrower than it first looks: the 421 and 500 codes Apple reuses for authentication ( Happy to follow up here once the pyicloud side lands. |
Proposed change
keep_alive()runs from a timer callback and is the only thing that schedules the next fetch. Anything raised inside it escaped into the event loop, so no further fetch was ever scheduled and every device stayed frozen at its last known state until Home Assistant was restarted. Two ways in:authenticate(), such as a network blip or an iCloud outageupdate_devices()indexes into (list(api_devices)[0][DEVICE_STATUS]raisesIndexErrorfor an account with no devices,status[DEVICE_ID]raisesKeyErroron a partial payload)This schedules the next fetch on every failure path, including the early return taken while a 2FA challenge is pending.
Separately, the timer was only ever armed at the end of
update_devices(), andasync_setup_entryarms nothing. Every early return out ofsetup()therefore left an entry that wasLOADEDwith no timer at all, never retried and dead until a reload — which is what happens when the first login fails.setup()now arms one before returning either way.A failed login is retried at the configured maximum interval rather than never, so the account recovers by itself once iCloud is reachable again.
Type of change
Additional information
Supersedes #169001, which was closed by the stale bot rather than on review. Rebuilt as a single commit on current
dev.Related, and independent of this one: #181390 (2FA-aware setup errors) and #181391 (stale location detection). All three touch
account.py, so whichever lands first the others will need a trivial rebase.Each of the three tests fails on unmodified
devand passes with the change:test_polling_survives_authentication_errorreproduces the transient-error case,test_polling_recovers_after_failed_setupthe never-armed-timer case, andtest_polling_survives_malformed_devicethe partial-payload case.This needs the
bugfixlabel for therequired-labelscheck to pass — I cannot add it myself.Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: