Cancel an in-flight entity initialization on teardown and rebuild - #899
Draft
zigpy-review-bot wants to merge 1 commit into
Draft
zigpy-review-bot wants to merge 1 commit into
zigpy-review-bot wants to merge 1 commit into
Conversation
`async_teardown` and `async_rebuild_from_zigpy_device` did not take the per-device entity lifecycle lock, so a device removal or a re-interview rebuild could land while an initialization round (startup mains polling, an availability refresh) was registering entities. The teardown cleared the pending list but could not take back the entities the round had already collected, and those were registered afterwards: on a removed device they were announced to consumers after the device was gone, and after a rebuild they were bound to the old zigpy device, so the re-interview's own initialization dropped its fresh entities as duplicates and the stale ones stayed registered. A teardown now marks the device torn down before it waits for the lock and cancels the task holding it: waiting for that round's device reads would stall device removal and gateway shutdown (which tears devices down before it cancels background tasks) behind request timeouts. Rounds already queued on the lock, or started later, skip themselves; a round whose cancellation got swallowed drops what it collected instead of registering it; and `async_configure` / `_discover_new_entities` skip a torn-down device, so a join racing a removal cannot activate and queue entities that nothing would drain. A rebuild clears the mark while still holding the lock, so the re-interview's configure + initialize proceed as before, and the locked teardown sets the mark again so a rebuild cannot erase a teardown requested meanwhile. A teardown whose wait for the lock is cancelled (a rejoin cancels a queued re-interview task) clears the mark, since nothing was torn down. `BaseEntity.on_remove` no longer suppresses `CancelledError` around the gather of its owned tasks: with `return_exceptions=True` the children never raise there, so the suppression only ever swallowed the caller's own cancellation, which is exactly the teardown's cancellation of a round that is dropping an entity. Re-initializing a device after `on_remove()` is no longer supported (nothing did that outside a test); a rebuild is the way to re-create all entities.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## zigpy-bot/serialize-device-entity-init #899 +/- ##
==========================================================================
+ Coverage 97.19% 97.21% +0.01%
==========================================================================
Files 57 57
Lines 10569 10618 +49
==========================================================================
+ Hits 10273 10322 +49
Misses 296 296 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
Note
Stacked on #898 (base branch
zigpy-bot/serialize-device-entity-init). Do not merge before #898; GitHub retargets this PR todevonce #898 merges and its branch is deleted. #878 is independent but touches the same file, so a rebase may be needed if it lands first.Closes the remaining gap noted in #897 and #898.
Problem
async_teardownandasync_rebuild_from_zigpy_devicedo not take the per-device entity lifecycle lock that #898 adds, so a device 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, which suspends while that entity's owned tasks are cancelled (lights, cluster pollers, the device tracker own one). A teardown landing there clears the pending list and runson_remove()on everything in it, but cannot take back the entities the round has already collected, and those are registered afterwards. On a removed device they are announced to consumers after the device is gone (some with live listeners, some already half torn down but still registered). 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 mainly a startup poll or availability refresh of a device that has just joined (recompute_entitiescollecting newly supported entities on a live device is the same race); a teardown landing during the round's device reads is harmless.Change
device_reinterviewedcancelling a previous initialization task.async_initializeandrecompute_entitiesrecord the holding task and skip themselves when they get the lock after a teardown was requested. The lock is FIFO, so without the early mark a round already queued ahead of the teardown would run a full round of reads first._add_pending_entitiesre-checks the mark right before registering, and drops what it collected if a teardown was requested meanwhile. This only matters if the cancellation got swallowed, e.g. by anon_remove()override suppressingCancelledError.async_configureand_discover_new_entitiesskip a torn-down device, so a join racing a removal cannot activate and queue entities that nothing would drain, including a configuration that was already past its first await when the teardown landed.async_initializereturns before setting_initializedwhen a teardown was requested during its round; that flag only gates whether later rounds emit entity-added events.async_rebuild_from_zigpy_devicedoes the same mark + cancel + locked teardown and clears the mark while still holding the lock, so a round queued behind it runs against the rebuilt device and the re-interview's configure + initialize proceed as before.BaseEntity.on_removeno longer suppressesCancelledErroraround the gather of its owned tasks. Withreturn_exceptions=Truethe children never raise there, so the suppression only ever swallowed the caller's own cancellation, which is exactly the teardown's cancellation of a round that is dropping an entity. Without this change the cancel is eaten at the one suspension point that matters and the teardown degrades to waiting for the round.Behaviour change to be aware of
Re-initializing a device after
on_remove()is no longer supported: the device is gone for good, and only a rebuild re-creates its entities. No production path did that;test_reinitialize_after_on_remove_emits_eventsdid, and is nowtest_reinitialize_after_rebuild_emits_eventsusingasync_rebuild_from_zigpy_device, which keeps its intent (all entities re-added while_initializedstays set still emit events).Tests
The new tests use a not-yet-initialized IKEA GU10 bulb. The three round-holding tests (cancels-in-flight, skips-queued, rebuild) start an initialization and hold it in the registration step by giving every entity an owned task, so the round suspends in the real
on_remove()gather rather than a stand-in; the two swallowed-cancellation tests use anon_remove()override that sleeps and suppresses the cancel; the configure test parks the quirk's custom configuration instead. Then:test_teardown_cancels_in_flight_initialize: removes the device. The round is cancelled, nothing is registered, no pending entities remain, and a configure, initialize and recompute started afterwards all skip. On the base it fails with the full entity set registered on the removed device.test_teardown_skips_queued_initialize: a second round is queued on the lock before the removal. It skips without a round of reads (oneinitialize_cluster_configscall in total, not two). On the base it fails with two.test_teardown_with_swallowed_cancellation: the round'son_remove()suppressesCancelledError, so it survives the cancel and reaches the registration step; it drops what it collected. On the base it fails with the full entity set registered.test_teardown_during_started_configure: a configuration suspended in the quirk's custom configuration when the device is removed resumes without discovering, activating or queueing anything. On the base it fails with the full set discovered and queued.test_cancelled_teardown_wait_lets_rounds_run_again: a rebuild queued behind a round whose cancellation got swallowed is itself cancelled; the mark is cleared, the round registers as usual and later rounds work. On the base the mark does not exist.test_rebuild_cancels_in_flight_initialize: rebuilds from a new zigpy device object, then runs the re-interview's configure + initialize. Every resulting entity's cluster belongs to the new zigpy device. On the base it fails with the stale set registered right after the rebuild.Full suite passes,
pre-commitclean, venvmypy zha/clean, the new tests stable over repeated runs.