Fix discovery crash from quirk removing ZCL attributes - #788
Fix discovery crash from quirk removing ZCL attributes#788TheJulianJES wants to merge 11 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #788 +/- ##
==========================================
+ Coverage 97.19% 97.21% +0.02%
==========================================
Files 57 57
Lines 10543 10585 +42
==========================================
+ Hits 10247 10290 +43
+ Misses 296 295 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
There is a branch that covers all four sites this PR touches, plus the one hole they structurally cannot reach — probably worth deciding how to combine them before this gets rebased: dev...zigpy-bot/fix-quirk-missing-attribute-crash Why it came uphome-assistant/core#178581 is the same crash path as the issue this PR targets: This PR as it stands would not fix that one. Quirk entities are constructed with How the two overlap
Both regression tests are on the branch: this PR's What this PR needs on its own
Separate PR, or merged?My suggestion is one PR, because the two changes are not independent: they edit the same four call sites, so whichever lands second has to re-touch those lines. And the relationship is not simply "superset" — once Two ways to do that, whichever you prefer:
On the open question in the description
The branch deliberately takes no position on whether a quirk may drop attribute definitions — it only makes the resulting entity not exist, since reading an attribute the cluster does not define can never do anything but raise. It logs at debug for default discovery, where this is routine for optional attributes, and at warning when a quirk explicitly asked for that entity, which is always an authoring bug. For what it is worth on the "do real quirks do this" question: sweeping all 873 device fixtures in |
|
@zigpy-review-bot Update this PR, possibly make changes, and do review rounds before pushing. Check if it should be merged as is then. (You can also update the PR description if it makes sense.) |
Custom v1 quirks that fully override a standard cluster's `attributes` dict (e.g. `attributes = LocalDataCluster.attributes.copy()` on an `OnOff` cluster) produce clusters without standard attribute definitions. `Cluster.is_attribute_unsupported()` raises `KeyError` for unknown attribute names, which propagated out of `Switch._is_supported()` and failed the whole gateway initialization. Check `attributes_by_name` first, like all other `_is_supported` implementations already do. Also fix the check order in `WindowCoveringInversionSwitch._is_supported`, where the existing guard ran after `is_attribute_unsupported()`.
`configure_cluster_configs` aggregates configs from discovered entities before they are filtered by `is_supported()`, so a quirk-replaced cluster missing standard attribute definitions made `find_attribute()` raise `KeyError` during device configuration. Skip such attributes with a debug log, matching how the attribute read path already tolerates them.
A sibling entity's `_server_cluster_config` can list attributes beyond its own `_attribute_name`, which may not exist on a quirk-replaced cluster, making `is_attribute_unsupported()` raise `KeyError` during polling. Check `attributes_by_name` first.
Reproduces the `KeyError: 'on_off'` from home-assistant/core#173265
Avoids registering a v1 quirk in the global `DEVICE_REGISTRY`.
An `AttributeDefs` class inheriting `BaseAttributeDefs` instead of `OnOff.AttributeDefs` replaces the standard attribute definitions the same way the legacy `attributes` dict override does.
The guards added so far all sit in `_is_supported()`, which `BaseEntity.is_supported()` skips entirely when `_attr_always_supported` is set - and that is exactly the case for quirk-created entities. A quirks v2 quirk naming an attribute its cluster never defines therefore still produced an entity whose every state read raises `KeyError`, which propagated out of `Device._add_pending_entities()` and failed the whole ZHA setup (home-assistant/core#178581). Add `BaseEntity._is_valid()`, checked *before* the `_attr_always_supported` short-circuit, and implement it on `PlatformEntity` as "the backing attribute resolves on the cluster". Missing attributes are routine for default discovery (debug log), but a quirk explicitly asking for such an entity is an authoring bug (warning log). That subsumes the duplicated `attributes_by_name` membership checks in switch/select/number/sensor/binary_sensor, which are removed in favour of the central check. The `configure_cluster_configs()` and `AggregatedClusterPoller` guards stay - both run over cluster configs aggregated before `is_supported()` filtering. `Device._add_pending_entities()` also no longer lets a single entity's state computation abort device initialization: emitting a state runs entity and quirk-supplied code, and one bad entity should not take every device down with it.
Two more paths in the same class of failure, both found by review: `ConfigurableAttributeSwitch.inverted` reads `_inverter_attribute_name` through `Cluster.get()`, which resolves the definition *outside* its own `try`, so a quirk naming a nonexistent inverter attribute raised `KeyError` on every state computation even though the primary attribute was fine. `_is_valid()` now checks both names. `initialize_cluster_configs()` passes each aggregated attribute name straight to `read_attributes()`, which resolves every name up front - so a single undefined attribute failed the whole batch and left its valid siblings uninitialized (observed: `start_up_on_off` silently never read because a broken quirk attribute shared its batch). Undefined attributes are now filtered out of the batch instead. The membership test both use is shared as `_cluster_defines()`.
`recompute_capabilities()` and `is_supported()` read attributes that are not the entity's own `_attribute_name` - `min_present_value` on `AnalogOutput`, `description` on `BinaryOutput`, `zone_type` on `IasZone`, and so on - straight through `Cluster.get()`, which resolves the definition outside its own `try`. So a quirk that gutted any of those clusters still raised `KeyError` out of `_add_pending_entities()` and failed the entire ZHA setup, exactly like the `OnOff` case this PR started with. `_is_valid()` cannot see those attributes, and both calls run before it anyway. Rather than guard each of them, route both calls through `Device._entity_supported()`, which treats an entity that cannot answer the question as unsupported - the same containment `_discover_new_entities()` already gives entity construction. Regression test gutting `AnalogOutput`. Also test `_safe_emit_state_changed_event`, which nothing exercised, and drop the `attributes_by_name` check `Switch._is_supported` no longer needs now that `_is_valid` runs first.
Removing an entity is destructive - consumers delete its registry entry, taking the user's name, area and customizations with it - so a raising capability check should not be read as "unsupported" for an entity that already exists. `_entity_supported()` now takes what the error case counts as: a prospective entity is dropped (it could never produce a state), an existing one is kept and logged. Both error logs also name the device, platform and unique id instead of a bare object repr.
449041c to
2a6ce5c
Compare
|
Pushed as What changed beyond the rebaseThe rebase itself was mostly clean — one conflict, in the regression test, because Then I folded in the extra work from the branch I linked earlier, plus what four review rounds turned up. Beyond what this PR already had:
I also updated the description's summary block to match, and added home-assistant/core#178581 to the issue list. Should it be merged as is?Correctness-wise I think yes — but there are two judgement calls in it that are yours, not mine, which is why I have not touched the draft state (and the body still says 1. It is now a wider change than you opened. It goes beyond adding missing guards: it centralises the existence check and changes the containment model in 2. It is user-visible for published quirks. Entities a quirk declares for attributes its cluster does not define now stop being created, with a warning naming the quirk's model. Those entities were already dead — reading their state could only ever raise — but on an affected device they will disappear from HA rather than sit there broken. Sweeping all 873 device fixtures in On your open question in the description — whether removing ZCL attribute definitions is something a quirk may legitimately do — this branch deliberately takes no position. It does not forbid it; it just makes the resulting entity not exist, since reading an attribute the cluster does not define can never do anything but raise. One known boundary I did not cross: state computation is still unguarded in Review rounds and verificationFour rounds, two independent reviewers each (a Claude reviewer and GitHub Copilot GPT-5.6 Sol), read-only, findings verified against the code before folding in.
Checks run on the final head: full suite 1381 passed, ruff and mypy clean, |
|
@zigpy-review-bot Can we improve test coverage? Also, should this PR be split up into multiple ones? Don't do it yet, just think how many we would/should have. |
|
Both lines are covered now — but the push was blocked on my side, so the commit is parked rather than on the branch. Details at the bottom; the split question is answered below it. CoverageTwo tests in
Coverage of Verification
The third line in that loop
One more
|
| Change | Files | User-visible | |
|---|---|---|---|
| A | Tolerate undefined attributes where cluster configs are aggregated: the reporting skip, the read-batch filter, and the poller guard. These run over configs collected before is_supported() filtering, so they are needed no matter what entity creation does. |
zha/zigbee/cluster_config.py, poller half of sensor/__init__.py |
No — nothing stops existing, some attributes just stop being configured or read |
| B | _is_valid(): don't create an entity whose backing attribute the cluster does not define, checked before the _attr_always_supported short-circuit. Subsumes the duplicated attributes_by_name checks in the five platforms. |
zha/application/platforms/__init__.py + de-dup in binary_sensor, number, select, sensor, switch |
Yes — declared-but-impossible quirk entities disappear, with a warning |
| C | Device-level containment: _entity_supported(), _safe_emit_state_changed_event(), and the drop-prospective / keep-existing asymmetry. Catches any exception out of capability computation, not just missing attributes. |
zha/zigbee/device.py |
No, beyond setup now succeeding where it used to fail |
The only file two of them touch is sensor/__init__.py (A's poller guard vs. B's de-dup), in different regions — so they could go up in parallel and in any order.
Two things worth weighing:
- A and C are the easy ones; B is the one that needs your judgement. That inverts the usual intuition, since C is the biggest behavior change. But C only makes previously-fatal paths non-fatal, whereas B is the one where entities vanish from users' installs — and B is also where the
always_supporteddecision and the_attr_max_attribute_namequestion above live. - A + B is the shippable minimum for the two linked issues. C came out of reproducing a different trigger for the same crash (a quirk gutting
AnalogOutput,KeyError: 'min_present_value'straight out of_add_pending_entities()); no reporter has hit it. So if you want the user-facing fix out sooner, C is the piece that can trail.
Note that this is a re-slice, not a cherry-pick of the existing commits: a22ed100 carries a switch.py hunk that belongs in B, and 75656f6c bundles _is_valid() with a cluster_config.py hunk (A) and _safe_emit_state_changed_event() (C). Whichever way you go, the commits need re-splitting.
My own preference is keep it as one PR — it has already been reviewed as a whole, the tests interlock across all three parts, and three PRs against a moving dev costs more rebasing than the review clarity is worth here. But if any of it is going to be contentious it will be B, and B is the one worth pulling out on its own.
The push
I could not push the commit: my own permission layer denied the git push to the PR head branch (not GitHub — maintainerCanModify is true and the branch is unchanged at 2a6ce5c6). I did not try to work around it. The commit is parked as a patch in the private zigpy/review-bot-reviews repo under zha/788/, along with a diagnostic. Re-mention me here to retry, or apply the patch directly if that is quicker.
|
@zigpy-review-bot You are allowed to push the test coverage improvement (and whatever else you have parked). |
The two `async_update()` guards were both uncovered: - `_is_attribute_unusable()`'s `except KeyError` — a cluster config naming an attribute the (quirked) cluster does not define. Covered with a quirk that replaces the EM cluster with a narrower `AttributeDefs`, keeping `active_power` but dropping the deprecated `power_multiplier`/`power_divisor` fallbacks its cluster config still lists. Without the guard the whole poll raises `KeyError: 'power_multiplier'`. - the `not entity.enabled` skip — asserted as the documented behavior: disabling an entity drops its attribute from the next poll and re-enabling brings it back, while its siblings' attributes keep being polled.
|
Pushed That was the only thing parked as a commit. The
Verification re-run on the pushed commit
No action taken on the split suggestions — re-mention me when you've decided, and I'll pick it up then. |
|
@zigpy-review-bot Tests failed with this run: https://github.com/zigpy/zha/actions/runs/32916926775/job/98022510877?pr=788. Restarted it and it went through, but create an issue about that for now. Also create one for "Warning: 'asyncio.get_event_loop_policy' is deprecated and slated for removal in Python 3.16", search related PRs (opened) in this repo and also search HA Core for PRs regarding this. I believe balloob or balloobbot may have had one regarding this recently. Open an issue and link everything relevant. |
|
I could not create the issues: my own permission layer denied The research is done and both issue bodies are finished, so here they are ready to paste. They are also parked in the private review repo under 1. The flaky runTitle: Flaky CI: teardown of Short version: all 1383 tests passed, the job failed only at teardown on Full issue bodySeen on the Run tests Python 3.14 job of https://github.com/zigpy/zha/actions/runs/32916926775/job/98022510877 (a CI run on #788). All 1383 tests passed; the job failed only in teardown, and a re-run of the same commit went green — so this is non-deterministic, not a regression from that PR. The failing assertion is the "no threads left behind" check at the end of Where the thread comes from
zha has exactly one such call: What should have cleaned it up
What I could not pin downThe exact window. It did not reproduce, and one observation is not enough to bisect. Two leads, both unverified:
Both leads run through the pinned If it recursCheapest mitigations, in order: make Filed for the record — currently a single observation, no other occurrence found in the tracker. 2. The deprecation warningsTitle: Python 3.14 CI: ~25k Short version: 16,575 Full issue bodyThe Run tests Python 3.14 job annotates every one of these calls. In https://github.com/zigpy/zha/actions/runs/32916926775/job/98022510877 that is 16,575 Source: pytest-asyncio, not zhaEvery annotation that carries a file attributes to We pin Open PRs in this repo that already do it
Home Assistant Core
Related, same 3.16 removal waveThe same job logs 82 |
DRAFT.
Proposed change
This fixes an issue where a (custom) quirk can remove standard ZCL attributes. Most entity platforms already have guards checking if the attribute even exists, but some do not. This adds them.
Additional information
I'm not sure if this is something we should add – quirks shouldn't misbehave like this. Or are there valid use-cases for deleting ZCL attributes...? But currently, ZHA startup breaks completely when using these custom quirks.
Should address:
KeyError: relay_click_in_on_off_modewith custom Inovelli quirk on 2026.8 home-assistant/core#178581This "regression" was introduced with:
AI summary
Issue and fix summary (CLICK TO EXPAND)
Issue
A quirk can leave a cluster without a definition for an attribute that ZHA entity code reads. zigpy's
Cluster.is_attribute_unsupported(),find_attribute()andCluster.get()all raiseKeyErrorfor such a name (get()resolves the definition outside its owntry, so it raises rather than returning the default).That
KeyErrorpropagated throughDevice._add_pending_entities()→Gateway.load_devices()→ HA'sasync_setup_entry, so one broken quirk prevented the entire ZHA integration from starting (ConfigEntryNotReadyretry loop) — for every device, not just the affected one. Before #657 the cluster-handler-based code tolerated these clusters.Two shapes of the same bug were reported:
ts0601_trv_moes.pyfrom jacekk015/zha_quirks doesattributes = LocalDataCluster.attributes.copy()on anOnOffcluster, leaving noon_offdefinition, andSwitch._is_supported()then raisedKeyError: 'on_off'.KeyError: relay_click_in_on_off_modewith custom Inovelli quirk on 2026.8 home-assistant/core#178581 — a quirks v2 quirk names an attribute its cluster never defines (a custom Inovelli VZM32-SN quirk declaring arelay_click_in_on_off_modeswitch, an attribute only the VZM30/VZM31 clusters have). Quirk entities are constructed with_attr_always_supported = True, andBaseEntity.is_supported()short-circuits on that before_is_supported()runs — so per-platform guards can never catch this case.Fix
A new
BaseEntity._is_valid()hook, checked before the_attr_always_supportedshort-circuit.PlatformEntityimplements it as "the attributes this entity is built around resolve on the cluster" (_attribute_nameand_inverter_attribute_name, whichinvertedreads on every state computation). A missing definition is routine for default discovery (debug log — an optional attribute a quirk removed) but an authoring bug when a quirk explicitly asked for the entity (warning log).That centralises the duplicated
attributes_by_namemembership checks previously scattered across switch/select/number/sensor/binary_sensor.Guards that
_is_valid()cannot replace, because they run over cluster configs aggregated beforeis_supported()filtering:configure_cluster_configs()— skips reporting configuration for attributes the cluster does not define.initialize_cluster_configs()— filters undefined names out of the read batch.read_attributes()resolves every name up front, so leaving one in failed the whole batch and left its valid siblings uninitialized.AggregatedClusterPoller.async_update()— treats an undefined attribute as unsupported instead of raising.Finally, the crash class is closed at its seam rather than per call site.
recompute_capabilities()andis_supported()read attributes that are not the entity's own_attribute_name—min_present_valueonAnalogOutput,descriptiononBinaryOutput,zone_typeonIasZone— and both ran unguarded in_add_pending_entities(), so a quirk gutting any of those clusters still failed the whole setup. Both calls now go throughDevice._entity_supported(), which treats an entity that cannot answer the question as unsupported, mirroring the containment_discover_new_entities()already gives entity construction.Device._add_pending_entities()likewise no longer lets one entity's state computation abort device initialization.Removal is deliberately not symmetric: a prospective entity that raises is dropped, but an existing one is kept, because removing it makes consumers delete its registry entry along with the user's customizations.
Tests
Regression tests for each shape, all verified to fail on unpatched
dev: a quirk replacingOnOffwith a cluster whoseAttributeDefsdoes not inherit the standard definitions; a quirks v2 switch naming a nonexistent attribute; the same for a nonexistent inverter attribute; a valid sibling attribute surviving a poisoned read batch; a quirk guttingAnalogOutput(therecompute_capabilities()path); and the state-emission and capability-check guards themselves.python -m tools.regenerate_diagnosticsproduces no snapshot drift, so no currently shipped quirk relies on an entity this now skips.