Serialize concurrent device entity initialization - #898
Open
zigpy-review-bot wants to merge 3 commits into
Open
zigpy-review-bot wants to merge 3 commits into
zigpy-review-bot wants to merge 3 commits into
Conversation
`Device.async_initialize` can be entered concurrently for the same device: the startup mains-polling task, a join / re-interview and the device becoming available again all call it independently. Both calls appended to and drained the same `_pending_entities` list, so the loser ended up passing the winner's own entity objects to `_add_entity`, which raised `ValueError: Cannot add entity ..., unique ID already taken by ...` with the same object on both sides. Guard `async_initialize` and `recompute_entities` with a per-device lock so discovery and registration run to completion before the next round starts; the second round then finds every entity already registered and drops its duplicates as before. Reproduced with `tools/regenerate_diagnostics.py` on a mains-powered dump whose `last_seen` is under `consider_unavailable_mains` old and that has an entity owning a task (any light, an `AggregatedClusterPoller` sensor): the join's initialization and the mains-polling initialization overlap, and dropping a duplicate of that entity suspends long enough for them to interleave.
`async_fetch_updated_state_mains` gathered the per-device initializations without `return_exceptions`, so the first device that raised made the gather raise, and the background `fetch_updated_state` task then died before setting `config.allow_polling = True`. Every polled entity (electrical measurement, some lights, ...) stayed silent for the rest of the session, and the availability checker, gated on the same flag, never pinged silent devices. Log and skip a failing device instead (cancellations at debug level), and always allow polling once startup polling has finished, whether or not it succeeded.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #898 +/- ##
=======================================
Coverage 97.19% 97.19%
=======================================
Files 57 57
Lines 10560 10569 +9
=======================================
+ Hits 10264 10273 +9
Misses 296 296 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The locking and failure-isolation changes are focused, consistent, and adequately covered by regression tests.
Pull request overview
Serializes per-device entity initialization and isolates startup polling failures.
Changes:
- Adds a per-device entity lifecycle lock.
- Logs per-device polling failures without stopping siblings.
- Ensures polling is always re-enabled and adds regression tests.
File summaries
| File | Description |
|---|---|
zha/zigbee/device.py |
Serializes discovery and registration. |
zha/application/gateway.py |
Isolates startup polling failures. |
tests/test_device.py |
Tests concurrent initialization. |
tests/test_gateway.py |
Tests polling failure recovery. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
Fixes #897
Problem
Device.async_initializecan be entered concurrently for the same device: the startup mains-polling task, a join / re-interview and the device becoming available again all call it independently. Both calls appended to and drained the same_pending_entitieslist, so the loser ended up passing the winner's own entity objects to_add_entity, which raisedValueError: Cannot add entity X, unique ID already taken by Xwith the same object on both sides. Details, callers and the deterministictools/regenerate_diagnostics.pyreproduction are in #897.Changes
Serialize entity initialization per device
async_initializeandrecompute_entitiesnow run under a per-deviceasyncio.Lock, so discovery and registration run to completion before the next round starts; the second round then finds every entity already registered and drops its duplicates through the existing check.Keep startup mains polling alive when one device fails
async_fetch_updated_state_mainsnow gathers withreturn_exceptions=True(cancellations at debug, other failures logged per device), andfetch_updated_statesetsconfig.allow_polling = Truein afinally. Before, the first failing device killed the background task before that flag was set, so every polled entity stayed silent for the session and the availability checker (gated on the same flag) never pinged silent mains devices.Related PRs
Supersedes #841. That bot-authored PR fixes the same startup-polling failure isolation; the second change here is the minimal form of it. #841 stays open for now and is intended to be closed once this PR is the agreed way forward. Its other change, dropping ZHA's whole-device startup concurrency cap in favour of LOW-priority startup reads (puddly's suggestion there), is a separate design decision and is deliberately not included here. If that change is wanted, ping @zigpy-review-bot to open a fresh PR for it on top of this one (preferred), or to update #841 to contain only that part.
Complementary to #878, which fixes the reconfigure leak of the same
_pending_entitieslist (second item of #725): that PR keeps a reconfigure from queueing entities at all, this one serializes the initialize rounds that drain the queue. Verified that the two merge cleanly in either order and that the full suite passes on the merged tree.Known remaining gap (pre-existing, not addressed here; fixed in #899, stacked on this PR)
async_teardown/async_rebuild_from_zigpy_devicedo not take the lock, so a removal or a re-interview rebuild can land while an initialization round (startup mains polling, an availability refresh) is still registering entities. The window is the registration step of that round:_add_pending_entitiesawaitsentity.on_remove()for every entity it drops, and a teardown landing there clears the pending list but cannot take back the entities the round has already collected, which are registered afterwards. On a removed device they are announced to consumers after the device is gone; after a rebuild they are bound to the old zigpy device, and the re-interview's own initialization drops its fresh entities as duplicates, so the stale ones stay registered. Only unregistered entities get collected, so the exposure is a startup poll or availability refresh of a device that has just joined; a teardown landing during the round's device reads is harmless, since the pending list is cleared before the round registers anything. Fix: #899.Tests and verification
test_concurrent_initialize_does_not_add_entities_twice: gathers a join (async_configure+async_initialize) with a concurrentasync_initialize(from_cache=False)on the IKEA GU10 bulb, which is the exact shape of the startup poll landing inside a join. Fails ondevwith the same-objectValueError, passes with the lock, and the resulting entity set matches a plain join.test_gateway_fetch_updated_state_mains_device_failure: one device raises and one is cancelled during startup polling; the remaining device is still initialized, both outcomes are logged, andallow_pollingends upTrue. Fails ondev.pre-commitclean, venvmypy zha/clean, both new tests stable over 10 runs.tools/regenerate_diagnostics.pynow processes the freshlumi.switch.agl011dump from Add knob events and voltage fix to Aqara Dimmer Switch H2 EU quirk zha-device-handlers#5346 without the traceback.