Prevent stale device lifecycle commits after removal - #690
Conversation
6e8fb32 to
24d076d
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #690 +/- ##
==========================================
+ Coverage 97.19% 97.20% +0.01%
==========================================
Files 57 57
Lines 10560 10673 +113
==========================================
+ Hits 10264 10375 +111
- Misses 296 298 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
3646c90 to
4412408
Compare
There was a problem hiding this comment.
Pull request overview
Improves ZHA gateway lifecycle robustness by adding per-device lifecycle generations/locks and by quiescing device lifecycle work during shutdown/removal, reducing race conditions between initialization, reinterview, removal, and shutdown.
Changes:
- Introduces per-device lifecycle generation tracking + locks to serialize initialization/reinterview/removal and prevent stale callbacks from committing.
- Refactors device init/reinterview/removal flows to cancel superseded work, await cancellation-resistant tasks appropriately, and harden shutdown teardown ordering.
- Expands gateway tests to cover lifecycle races (init vs remove, remove vs immediate rejoin, shutdown vs in-flight init).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| zha/application/gateway.py | Adds device lifecycle generation/locking + quiesce logic; refactors init/reinterview/removal/shutdown to avoid lifecycle races. |
| tests/test_gateway.py | Adds/updates tests validating the new lifecycle serialization and shutdown/removal race handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
87b3bb5 to
6a5f030
Compare
zigpy-review-bot
left a comment
There was a problem hiding this comment.
Reviewed the lifecycle machinery end-to-end and it holds up well — the generation counter (advanced before cancellation) + per-IEEE lock + removal-task barrier + post-await revalidation is a sound and internally consistent design, and the wait-before-lock ordering keeps the task wait-graph acyclic, so no lock-cycle deadlock is reachable. Test coverage maps cleanly onto each scenario in the description; locally all test_gateway.py tests pass with no wall-clock outliers under looptime, in-venv mypy is clean, and CI is green across 3.12–3.14.
I traced the paths this PR is most exposed on and didn't find a remaining race: an old removal can't clobber a newer lifecycle (the wrapper-identity check in _async_remove_device gates the del self._devices[ieee]), the done-callbacks only delete entries that still point at their own task, and shutdown's quiesce awaits (rather than races) removals before crossing the locks. An independent second-opinion pass reached the same conclusion.
No blockers — just a few optional observations:
_device_lifecycle_zigpy_deviceskeeps a strong reference to each removed device's zigpyDeviceuntil the gateway is reinitialized (gateway.py:834is read-only; the dict is only cleared in_async_initialize). It's bounded by distinct IEEEs so it's not a runaway leak, and I understand keeping the generations/locks for the run is deliberate for stale-callback rejection — but this dict retains the heaviest objects (endpoints/clusters/listeners). A weakref, or pruning just this dict on a successful removal, would trim the retention without weakening the ownership checks. Optional.- Optional readability:
_async_remove_deviceawaits the previous task with a plainasyncio.gather(..., return_exceptions=True)(gateway.py:889) while_async_run_device_lifecycle_operationshields the equivalent wait (:505). The asymmetry is safe (removal tasks are only cancelled at the finalsuper().shutdown(), after quiesce has already awaited them), but a one-line comment on why removal doesn't need the shield would save the next reader the trace. - Nit: in
_async_run_device_lifecycle_operation,await asyncio.wait({previous_removal_task})would be a simpler equivalent to theshield(gather(...))wait — same don't-propagate / don't-cancel-the-inner semantics.
Nice work — the invariant is clearly stated and the tests back it up.
6a5f030 to
2a16b7f
Compare
|
Follow-up on the optional observations, addressed in 2a16b7f:
|
2a16b7f to
30b5882
Compare
Removing a device while pairing or reinterview is still running can publish a later
DeviceFullInitEventor recreate wrapper state after removal. Cancellation alone does not stop an operation that is still unwinding or performing asynchronous cleanup.Serialize each device's initialization, reinterview, and removal with an ownership generation and a per-device lock. Removal revokes the old generation, waits for its exact task, and only removes the captured wrapper. Same-IEEE rejoin waits for removal; delayed callbacks cannot delete a replacement. Shutdown quiesces lifecycle work before wrapper/controller teardown. Device identity tracking uses weak references.
Removal-event listener failures now still run wrapper teardown; a failed removal does not poison a subsequent rejoin. Tests cover the public removal/event ordering, cancellation-resistant initialization, factory-changing reinterview, immediate rejoin, stale callbacks, cleanup failure, and shutdown. A hook that never finishes cancellation can still delay removal/shutdown; this PR does not add a timeout or abandon live work.
Validation against
devat66603431339afe37fa0048b70ff31d77dceb8f95: Python 3.12 full suite, 1402 passed, coverage above the 95% project gate; full pre-commit (codespell, Ruff, formatting, mypy, lock check) passed. Relevant regression checks fail on the unchanged base. GitHub CI for Python 3.12/3.13/3.14 is reported separately on the PR.