Keep startup polling failures isolated - #841
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #841 +/- ##
=======================================
Coverage 97.19% 97.19%
=======================================
Files 57 57
Lines 10560 10568 +8
=======================================
+ Hits 10264 10272 +8
Misses 296 296 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
An alternative approach here would be to disable concurrency limiting entirely and make sure that device state polling is done with low priority: async with self.request_priority(t.PacketPriority.LOW):
# code to poll a deviceMaybe we can add this as a kwarg to Zigpy has a mechanism to control concurrency if a request is properly tagged with the above context manager. |
|
@puddly Thanks — I updated the PR to take this approach. ZHA no longer limits complete device-initialization pipelines during startup. It starts every eligible initializer as LOW-priority work and delegates actual radio-request scheduling to zigpy and the radio backend. The revised call site is here. In abbreviated form, it is now: startup_polling_priority = t.PacketPriority.LOW
async with self.request_priority(startup_polling_priority):
initialization_results = await asyncio.gather(
*(
device.async_initialize(
from_cache=False,
request_priority=startup_polling_priority,
)
for device in online_devices
),
return_exceptions=True,
)Why removing the ZHA-level limiter is the better boundaryThe previous implementation inspected zigpy's private
Zigpy now owns the latter concern. Its request-priority context supplies the ambient priority, and its global request limiter resolves that context when a backend submits a request without an explicit priority. The limiter itself is a priority-aware cascading limiter, rather than a single undifferentiated semaphore. With zigpy's default concurrency of 8 and 25% LOW / 50% NORMAL / 25% HIGH capacity fractions, the cumulative admission thresholds are 2 for LOW, 6 for LOW plus NORMAL, and 8 when HIGH is included. These are cascading priority thresholds, not a strict total-concurrency ceiling for every possible arrival ordering, but the important distinction is that startup traffic now enters the LOW tier instead of consuming an arbitrary number of whole-device slots. For common configured values, the structural difference looks like this:
Those columns are intentionally not performance equivalents: the old limit covered the entire initialization pipeline, while the new tier governs individual radio requests. The latter is the abstraction we actually want. The old ZHA reservation heuristic came from ZHA #510; zigpy subsequently gained its priority-aware request limiting in zigpy #1635, so the scheduling responsibility can now live at the lower layer.
There is a subtle distinction between the priority used to acquire a local limiter and the priority stored on the outgoing packet:
That would make a context-only version look correct inside ZHA while Ziggurat still received the startup reads as normal interactive traffic. The revised implementation closes that gap:
Backend behavior
The explicit propagation currently covers the radio-producing work in Concrete behavior on a larger networkConsider 50 recently seen mains-powered devices during startup:
F4 failure and cancellation semantics remain intactRemoving the outer concurrency helper does not undo the original corrective behavior:
So I agree with the suggested direction and have changed the implementation accordingly: ZHA expresses startup polling as LOW-priority work, zigpy/backends own radio scheduling, and Ziggurat receives the priority explicitly on the packet rather than accidentally treating startup reads as NORMAL. |
There was a problem hiding this comment.
Approve. Nicely reasoned and thoroughly tested — verified the load-bearing pieces locally against zigpy 2.0.1 and CI is green across 3.12/3.13/3.14.
What I checked:
request_priorityis genuinely wired through, not dead code. Tracedinitialize_cluster_configs→cluster.read_attributes(priority=…)→read_attributes_raw→_read_attributes/general_command(**kwargs)→Cluster.request(priority=…)→ the packet. When no priority is passed the packet'sprioritystaysNone(the ambient contextvar only feeds zigpy's per-request limiter fallback — it's never written onto the packet), and on-packet-priority backends like zigpy-ziggurat treat thatNoneas NORMAL, so the explicit propagation is what actually putsLOWon the wire.test_initialize_request_prioritylocks this in.- Dropping the
radio_concurrency - 4reservation is reasonable. For backends that route through zigpy's global request limiter (bellows/ZNP/deCONZ),RequestLimitercaps the LOW tier structurally — with the default 0.25/0.50/0.25 fractions and concurrency 8 the cumulative caps are LOW=2, LOW+NORMAL=6, +HIGH=8, so startup reads occupy at most 2 slots and interactive NORMAL/HIGH work retains reserved capacity (more than the old-4left at default concurrency). Caveat: those fractions are user-configurable, and backends that don't call zigpy's limiter (zigpy-xbee/zigate) lose ZHA-level startup limiting entirely — which the PR calls out as an intentional trade-off rather than reinstating a duplicate whole-device limiter. - Failure isolation and cancellation semantics are correct.
gather(return_exceptions=True)awaits every sibling before returning, so one device's failure can no longer orphan the rest or skip the polling re-enable (the real pre-existing bug).zip(strict=True)over order-preserving results is sound, theisinstance(CancelledError)-before-Exceptionordering is right (CancelledError isBaseException), and outer cancellation still propagates through the gather and re-raises — withfinally: allow_polling = Truenow guaranteeing polling is restored on any exit. The three new gateway tests exercise exactly these paths. gather_with_limited_concurrencyis still used inhelpers.py; only the now-unusedradio_concurrencyproperty is removed, with no remaining references in zha (or ha-core's ZHA component).- mypy clean on the changed files.
28b6884 to
5e3fb6d
Compare
One unexpected startup device-refresh failure can end the supervising gather while sibling refreshes keep running, and skip re-enabling normal polling. Wait for every device outcome, report failures/cancellations per device, and restore
allow_pollingwhen the startup operation exits. Cancellation of the overall operation still cancels and awaits its children.Following the review suggestion, remove ZHA's whole-device concurrency limit and express startup work as LOW-priority radio requests. Keep the ambient priority context and forward explicit LOW through aggregated attribute reads, so packet-priority consumers receive it too. Existing initialization callers retain their default behavior.
Tests cover sibling supervision, failure recovery, cancellation, eligible-device selection, and priority propagation. This is not a measured startup-speed improvement. Radio backends own request scheduling; backends that do not use zigpy's limiter no longer have a separate ZHA whole-device cap.
Validation against
devat66603431339afe37fa0048b70ff31d77dceb8f95: Python 3.12 full suite, 1385 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.