Separate computed primary entity state from _attr_primary

[TheJulianJES]

Fixes the first item of #725.

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. Conflating the two caused the election to get stuck:

• Election losers were set to _attr_primary = False, permanently excluding them from all future elections (the candidate filter skips _attr_primary is False). If the winner was later removed, no remaining entity could ever become primary again.
• A previous winner (_attr_primary = True) looked "explicitly primary" to later elections, which then short-circuited, so a stronger candidate appearing later could never take over. If a genuinely explicit primary entity appeared alongside a previous winner, the assert not explicitly_primary sanity check would crash instead.

Changes

• _attr_primary now only ever holds the explicit declaration (entity class attribute or quirk primary=True/False) and is never written by the election.
• The election result is stored in a new private BaseEntity.__computed_primary instance field, written through the existing primary setter.
• The primary getter returns _attr_primary when set, falling back to the computed state — so an explicit True/False from a quirk or entity class can never be overridden by the election.
• The election's explicit-primary check now looks at _attr_primary directly (explicit only).
• The election clears all computed primary state up front, so a stale previous winner cannot survive a re-election on any code path — e.g. when an explicitly primary entity appears later, or when the previous winner was disabled (and is thus no longer a candidate).

Tests

• test_primary_entity_reelection: a mock smart plug with OnOff + IasZone. Marking on_off as unsupported in the ZCL attribute cache makes the switch natively unsupported, so recompute_entities() removes it through the real removal path — the IAS zone (previous election loser) now wins the re-election. Writing an on_off value clears the unsupported flag, and the rediscovered switch wins back primary.
• test_primary_entity_election_disabled_winner: a disabled previous winner loses its computed primary state on the next recomputation, so only the runner-up is primary. After being re-enabled, it wins back the election.
• test_primary_entity_election_explicit_primary_takes_over: an entity marked explicitly primary (as a quirk would) takes over from a previously computed winner on the next recomputation, instead of tripping the sanity assert.

All three tests fail on dev without the fix.

Possible future TODOs for other PRs

1. Device-owned primary pointer: the per-entity __computed_primary flag is a denormalized cache of "am I the current winner". A single device._primary_entity reference with state asking the device would eliminate the up-front clearing and any possibility of stale flags. Costs: GroupEntity has no device backref (no election exists for groups today, so their computed primary is effectively always False), and entity state would depend on external state, complicating maybe_emit_state_changed_event change detection.
2. Trim the primary setter: its only caller is Device._compute_primary_entity, which already reaches into _attr_primary directly — the election could write a single-underscore _computed_primary field instead. The setter's semantics are also slightly unusual: entity.primary = True does not guarantee entity.primary == True (an explicit False wins).
3. Trim the primary_weight property: pure pass-through to _attr_primary_weight with a single caller (the election), kept for now to match the repo-wide _attr_*-plus-property idiom.
4. Multiple explicit primaries still assert: two quirk entities with primary=True hit assert not explicitly_primary and crash device init. Pre-existing; could be downgraded to a warning + tie handling.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.15%. Comparing base (a270540) to head (ed9d98f).

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #861      +/-   ##
==========================================
- 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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[zigpy-review-bot]

The fix is correct and well-targeted — splitting the election's result off from the explicit declaration resolves both stuck-election bugs, and I confirmed both new tests fail on dev (a2705409) and pass at this head. One gap remains in the same method: the election's clearing loops still don't cover every entity, so a stale computed winner can survive a re-election.

Must-address: stale computed primary when the previous winner isn't a candidate

_compute_primary_entity clears computed state over others (zha/zigbee/device.py:1650) and candidates (:1659), but candidates (:1636) filters out not e.enabled. An entity that won an earlier election and is then disabled keeps __computed_primary = True while the re-election hands primary to someone else — so two entities report primary at once. The if not candidates: return early exit (:1639) leaks the same way when every entity is filtered out.

disable() is a production path, not just a test hook: ha-core calls platform_entity.disable() when the registry entry is disabled (homeassistant/components/zha/helpers.py:667 and :1300), and enable()/disable() don't re-run the election, so the stale flag also survives a later re-enable.

Repro at this head — smart plug with OnOff (weight 10) + IasZone (weight 3):

switch = get_entity(zha_device, Platform.SWITCH, entity_type=Switch)
ias_zone = get_entity(zha_device, Platform.BINARY_SENSOR, entity_type=IASZone)
assert switch.primary and not ias_zone.primary

switch.disable()                       # what ha-core does for a disabled registry entry
await zha_device.recompute_entities()
# PR head: switch.primary = True  | ias_zone.primary = True   <-- two primaries
# dev:     switch.primary = True  | ias_zone.primary = False

Worth noting this is a behavior change rather than a pre-existing bug: on dev the stale _attr_primary = True makes the ex-winner look explicitly primary, so the election short-circuits and only ever one entity claims it. Since primary drives _attr_name = None in ha-core (entity.py:101), two primaries mean two entities claiming the bare device name once the disabled one is re-enabled.

Hoisting the clear to the top of the method covers all four exits at once, and makes the new loop at :1624-1627 redundant:

def _compute_primary_entity(self, entities: Sequence[PlatformEntity]) -> None:
    """Compute the primary entity from a given set of entities."""

    # Clear all previously computed primary state up front, so no stale winner
    # can survive a re-election on any code path below
    for entity in entities:
        entity.primary = False

    # First, check if any entity is explicitly primary
    explicitly_primary = [entity for entity in entities if entity._attr_primary]
    ...

I applied exactly that locally: the repro above then ends False / True, and all 9 -k primary tests in tests/test_device.py (both new ones included) still pass. It also lines up with TODO 1 in your description — a device-owned _primary_entity pointer would make this whole class of stale-flag bug unrepresentable.

Optional

entity._attr_primary (:1616) and e._attr_primary is not False (:1636) reach into a private attribute from Device. A small BaseEntity.explicitly_primary property returning _attr_primary would keep the election reading a public surface — cheap now that the getter no longer conflates the two.

Verified (8 checks)

• Both new tests fail on dev (a2705409) and pass at 14744da9 — test_primary_entity_reelection and test_primary_entity_election_explicit_primary_takes_over, run in separate worktrees.
• The stale-primary repro was run against both dev and this head; the outputs quoted above are actual runs, not reasoning.
• Proposed hoisted-clear patch applied locally: repro fixed and 9/9 -k primary tests in tests/test_device.py still pass.
• mypy zha/ inside the worktree venv (real deps, unlike the CI hook's dependency-free env, where every zigpy.* import collapses to Any): clean, no regression.
• GroupEntity.__init__ chains to BaseEntity.__init__, so the name-mangled __computed_primary is always initialized — no AttributeError on the group path.
• Narrowing the primary setter from bool | None to bool breaks no external consumer: ha-core only reads meta.primary (entity.py:101) and never assigns it.
• _add_pending_entities already emits maybe_emit_state_changed_event() for pre-existing entities after the election, so a flipped primary does reach consumers.
• Copilot (GPT-5.5) second opinion: independently flagged the same disabled-winner stale-flag issue, and nothing else.

[zigpy-review-bot]

Optional: this and the e._attr_primary is not False filter on line 1636 read a private attribute from Device. A BaseEntity.explicitly_primary property returning _attr_primary would keep the election on a public surface — cheap now that primary no longer conflates explicit and computed state.

[zigpy-review-bot]

The stale-computed-primary gap from my previous review is fixed by ed9d98f exactly as proposed — hoisting the clear to the top of _compute_primary_entity covers all four exits, and test_primary_entity_election_disabled_winner pins it (it fails with zha/ reverted to 14744da9, passes at this head). No blockers or must-address items left. Still marked draft, so approving on the code as it stands rather than on merge-readiness.

Optional follow-ups — neither is a regression, both behave the same on dev

The explicit-primary path still ignores enabled. zha/zigbee/device.py:1622 collects explicit primaries from all entities, while candidates (:1636) filters on e.enabled. So an explicitly primary entity the user disabled in the entity registry still short-circuits the election and nothing enabled gets promoted — same shape as the bug ed9d98f just fixed, one path over. Probed at this head on third-reality-inc-3rsnl02043z: with the light marked _attr_primary = True and then disable()d, recompute_entities() leaves light.primary=True / motion.primary=False, i.e. the device's only primary is an entity HA won't load. It may well be deliberate — a quirk's explicit declaration arguably shouldn't be silently reassigned to something else — in which case a short comment at :1622 would settle it; otherwise it fits the TODO list in the description.

Nothing re-runs the election on enable/disable. ha-core calls platform_entity.disable() / enable() (homeassistant/components/zha/helpers.py:667-669 and :1300) and there is no recompute_entities() call anywhere in the integration, so a disabled winner keeps primary until the next _add_pending_entities() happens to run — which is why the new test has to call recompute_entities() by hand. Unchanged from dev, so nothing to fix in this PR; possibly worth a TODO 5.

My earlier optional inline suggesting a BaseEntity.explicitly_primary property still stands (it would keep Device off _attr_primary at :1622 and :1636), but it is purely cosmetic — leaving the thread open rather than re-raising it here.

Verified (8 checks)

• test_primary_entity_election_disabled_winner fails with zha/ reverted to 14744da9 and passes at ed9d98f6 — the fix commit is load-bearing, not just a refactor.
• All 9 -k primary tests in tests/test_device.py pass at this head; tests/test_device.py + tests/test_discover.py together: 939 passed.
• The disabled-explicit-primary observation above is an actual run in the worktree, not reasoning — the quoted light.primary / motion.primary values are the probe's output.
• mypy zha/ inside the worktree venv (real deps, unlike the CI hook's dependency-free env where every zigpy.* import collapses to Any): Success: no issues found in 57 source files.
• _discover_new_entities() clears and rebuilds entity objects every pass, so an entity removed while still holding __computed_primary = True is a dead object — it can't be resurrected carrying a stale flag (relevant to the removal path in test_primary_entity_reelection).
• entity.primary = … is assigned nowhere in zha/ outside _compute_primary_entity, and ha-core only reads meta.primary (entity.py:101) — narrowing the setter from bool | None to bool breaks no consumer.
• Title matches recent merged-PR conventions.
• Copilot (GPT-5.5, --effort high) second opinion at this head, pointed at the election paths, the getter/setter precedence and name mangling, event emission, and the ha-core consumer: no findings.

[TheJulianJES]

Hmm, I think enabled and disabled entities should behave the same, in that a disabled entity could be the primary entity still and not allow others to take its primary spot? But on the other side, I do kind of see some benefit to excluding (user-)disabled entities from the primary entity election?

[TheJulianJES]

The below PR will fix that enabled/disabled issue and should be rebased + merge after this PR is merged.

• Ignore runtime enabled state in primary entity election #862

The current logic of checking enabled doesn't make any sense because of multiple reasons, including that we don't recompute the primary entity when enabling/disabling an entity and HA disables the LQI/RSSI entities too late on the ZHA side when the primary entity computation is already done. So effectively, the computation never really used enabled anyway, hence the above PR.

[puddly]

Ah, you're very right, this attribute does indeed perform double duty.

I think the TODO you listed is a good alternative approach: denormalization is the problem here because primary entity computation is a device problem, not an entity problem, and IMO should be something the device figures out and maintains. This would retain _attr_primary as an entity-level hint and then let the device object itself hold a PlatformEntity | None reference to a primary entity. What do you think?

[TheJulianJES]

Yeah, I think that would be the better approach. I can have a look at that later.