Ignore runtime enabled state in primary entity election - #862
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #862 +/- ##
=======================================
Coverage 97.17% 97.17%
=======================================
Files 57 57
Lines 10542 10542
=======================================
Hits 10244 10244
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.
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.
40caf86 to
b626ed4
Compare
There was a problem hiding this comment.
Pull request overview
Keeps primary entity election stable regardless of runtime enabled state, preventing naming churn.
Changes:
- Removes enabled-state filtering from weighted election candidates.
- Updates tests for disabled weighted and explicit primary entities.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
zha/zigbee/device.py |
Preserves disabled entities as election candidates. |
tests/test_device.py |
Verifies enabled state does not affect primary selection. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Follow-up to #861 (see the enabled/disabled design discussion there).
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. HA Core also callsdisable()for default-disabled entities only after the first election has already run, so theenabledfilter effectively never influenced a first election either.Changes
enabled— disabled entities stay candidates and can keep (or win) the primary spot. Enabling/disabling an entity now never changes the election outcome. Together with Exclude weight 0 entities from primary entity election #859, candidacy now simply means: non-zero primary weight and not explicitly excluded.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
enabledfilter never influenced a snapshot's election._attr_primary#861: the explicit-primary path ignoringenabledis now consistent by design (documented in the code comment), and re-running the election on enable/disable is no longer needed since enable/disable is a no-op for the election.