Ignore runtime enabled state in primary entity election - #862
Ignore runtime enabled state in primary entity election#862TheJulianJES wants to merge 6 commits into
Conversation
The primary entity election previously stored its result in `_attr_primary`, the same field entity classes and quirks use to explicitly mark an entity as (not) primary. This conflation caused two bugs: - Election losers were set to `_attr_primary = False`, permanently excluding them from future elections (filtered by `_attr_primary is not False`). If the winner was later removed, no remaining entity could become primary. - A previous winner looked "explicitly primary" to later elections, which then short-circuited (or hit the sanity assert when a genuinely explicit primary entity appeared), so a stronger candidate could never take over. The election result is now stored in a separate, private `__computed_primary` field. `_attr_primary` is only ever set explicitly and always takes precedence, so quirks and entity classes setting `primary` to `True`/`False` are never overridden by the election.
Instead of patching `_is_supported`, mark the `on_off` attribute as unsupported in the zigpy attribute cache, so the switch entity natively becomes unsupported and is removed by `recompute_entities()`.
A previous winner that is no longer an election candidate (e.g. after being disabled via the entity registry) kept its stale computed primary state while the re-election handed primary to another entity, leaving two entities claiming primary at once. Clearing all computed state at the start of the election covers every code path and replaces the per-branch clearing loops.
The primary entity describes the main feature of the device, which does not change when its entity is disabled via the entity registry in HA. Disabled entities now stay election candidates, so disabling the current primary entity does not hand its spot to the runner-up (and does not cause naming churn in HA), and both the explicit-primary check and the weight election now consistently ignore enabled state. In practice, nothing re-ran the election on enable/disable anyway: HA Core only toggles the flag, so a re-election happened at the earliest on the next entity recomputation, making the previous behavior timing-dependent. No device snapshots change: entities are only ever disabled at runtime by HA Core, never during initial discovery.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #862 +/- ##
==========================================
- Coverage 97.15% 97.15% -0.01%
==========================================
Files 55 55
Lines 10482 10481 -1
==========================================
- Hits 10184 10183 -1
Misses 298 298 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
zigpy-review-bot
left a comment
There was a problem hiding this comment.
Reviewed the new commit only (40caf861) — the rest collapses once #861 lands, per the PR body. No blockers and no must-address items. Leaving this as a comment rather than an approval only because the diff will change shape on rebase; the substance looks right to me.
Verified claims
- Zero snapshot diffs — confirmed. Ran
tools/regenerate_diagnostics.pyin a worktree at40caf861;git diff --stat tests/data/devices/is empty. That is the structurally expected result rather than a lucky one:_attr_enableddefaults toTrueand nothing insidezhaever sets itFalse, so at snapshot-generation time the removede.enabledterm was already a no-op. - "Nothing re-ran the election on enable/disable anyway" — confirmed on the HA side.
homeassistant/components/zha/helpers.pytoggles the flag in the entity-registry-updated listener (platform_entity.disable()/.enable()) with no recomputation, and the post-add pass (for entity in entities_to_add: if not entity.enabled: entity.entity_data.entity.disable()) runs after_async_add_entities, i.e. after the first election has already completed. So the old behavior was really "reassign at some arbitrary later recomputation", not "reassign on disable" — which makes this less a semantics change than a removal of nondeterminism. Worth keeping that framing in the final commit message. - The explicit-primary check already ignored
enabled— confirmed;explicitly_primaryfilters onentity._attr_primaryalone. I also exercised it end-to-end (set_attr_primary = True,disable(),recompute_entities()): the entity keeps the spot and the runner-up does not take it, so the consistency the new comment claims is real and not just asserted. - HA naming consequence —
homeassistant/components/zha/entity.pysets_attr_name = Nonewhenmeta.primary, so a registry-disabled primary means no entity carries the bare device name. That is the deliberate trade-off here and I think it is the right one: the alternative was handing the device name to a runner-up at an unpredictable moment.
No coverage lost in the rename
test_primary_entity_election_disabled_winner was the only test whose name pointed at #861's up-front entity.primary = False clearing loop, so repurposing it means that test no longer exercises it. I checked the loop is still covered: deleting it makes both test_primary_entity_reelection and test_primary_entity_election_explicit_primary_takes_over fail. Nothing to do — just confirming the rename is safe.
Optional
- The description lists "the explicit-primary check never looked at
enabled" as one of the two things this makes consistent, and the new code comment states it, but no test pins it (disable()appears exactly once in the suite, in the test this PR modifies). See the inline note for a few lines that would lock it in. - One concrete instance of the weight-0 re-admission the description flags for #859:
CoordinatorDevice.discover_entities()yields onlyDeviceCounterSensors, which are allprimary_weight = 0and_attr_entity_registry_enabled_default = False. Once HA disables them, a coordinator re-election previously hitif not candidates: return; it now reaches the tie branch instead and logsPrimary entity tie between ..., no primary entityat debug on every recomputation. The outcome is identical (no primary either way), so this is purely a new debug line on a rarely-hit path — and #859 removes it. Note, not an ask; it does support the description's "no primary for real devices" conclusion, since the one device class where every entity is weight 0 lands on the tie path.
Incidental, and correct
Dropping "non-counter entities" from the comment fixes stale wording rather than describing a behavior change. That phrase referred to the hasattr(e, "info_object") term added in #298, which #804 (ccc36515) removed while leaving the comment behind — counter entities have been candidates ever since. The rewritten comment no longer claims a filter that does not exist.
Checks
Full suite 1348 passed; ruff check and ruff format --check clean; mypy zha/ inside the worktree venv (real deps installed, not the dependency-less pre-commit env) reports no issues, so no regression against the dev baseline. A second-opinion pass scoped to this commit returned no findings.
The merge order in the description (#861 → #859 → this) looks right — landing #859 first is what makes the weight-0 note above moot.
| await zha_device.recompute_entities() | ||
|
|
||
| assert switch.primary | ||
| assert not ias_zone.primary |
There was a problem hiding this comment.
Optional: the mirror half of the consistency claim in the description — an explicitly primary entity that gets disabled — is not covered anywhere in the suite. A few lines here (or in test_primary_entity_election_explicit_primary_takes_over) would pin it:
# An explicitly primary entity also keeps its spot when disabled
ias_zone._attr_primary = True
await zha_device.recompute_entities()
ias_zone.disable()
await zha_device.recompute_entities()
assert ias_zone.primary
assert not switch.primaryI ran this against 40caf861 and it passes, so it documents intent rather than catching a bug.
DRAFT. BASED ON:
_attr_primary#861 (merge, then rebase this)Only consider the last commit: 40caf86. Everything else will be dropped from this PR's diff when the above PR is merged and this one is rebased.
Stacked on #861 (only the last commit is new; the diff collapses once #861 is merged).
The weight election previously filtered candidates on
e.enabled, so an entity disabled via the entity registry in HA lost its primary spot to the runner-up on the next re-election. The primary entity describes the main feature of the device — a smart plug whose switch entity is disabled is still a smart plug — so a registry toggle should not reassign it. Sinceprimarydrives entity naming in HA (the primary entity takes the bare device name), reassigning it also caused naming churn when an entity was disabled and re-enabled.In practice, the old behavior was timing-dependent anyway: HA Core only toggles the enabled flag on registry changes and nothing re-runs the election at that point, so a disabled winner kept
primaryuntil the next entity recomputation happened to run for an unrelated reason.Changes
enabled— disabled entities stay candidates and can keep (or win) the primary spot. Enabling/disabling an entity now never changes the election outcome.enabled, so an explicitly primary but disabled entity already kept its spot.Tests
test_primary_entity_election_disabled_winneris renamed totest_primary_entity_election_ignores_enabledand asserts the new semantics: the disabled switch stays primary, the IAS zone runner-up does not take its spot, and re-enabling changes nothing.Notes
tools/regenerate_diagnostics.py— zero diffs): entities are only ever disabled at runtime by HA Core, never during initial discovery, so theenabledfilter never influenced a snapshot's election.enabledfilter ondev: HA Core disables them only after the first election has already run, and as weight-0 entities they always sorted last anyway. Exclude weight 0 entities from primary entity election #859 is what actually excludes weight-0 entities from candidacy; together with this PR, candidacy simply means "non-zero primary weight", and first elections and re-elections behave identically.Context / review trail
_attr_primary#861: whether a disabled entity should keep its primary spot. This PR answers yes, by making the election ignore runtime enabled state entirely._attr_primary#861 review: the explicit-primary path ignoringenabledis now consistent by design (documented in a code comment), and "re-run the election on enable/disable" is no longer needed since enable/disable is a no-op for the election._attr_primary#861 → Exclude weight 0 entities from primary entity election #859 → this PR.