Conversation
`LightGroup.update()` derives on/off from every member's last reported state, with no availability check. A member that drops off the network while on keeps voting "on" indefinitely. The group then stays on even once every reachable member is off, and only clears once the stale member itself reports off again or the whole group entity is force refreshed some other way. `_make_members_assume_group_state()` already skips unavailable members when writing optimistic state after a command. `update()` should apply the same rule when reading it back.
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped, matches the PR description, and includes a targeted regression test covering the reported failure mode.
Pull request overview
This PR fixes LightGroup.update() so a light group’s computed on/off state ignores members that are currently unavailable, preventing a stale “on” report from an offline device from keeping the whole group marked on.
Changes:
- Filtered
on_statesinLightGroup.update()to include only members that are bothonandavailable. - Added a regression test ensuring an unavailable “on” member does not keep the group on after all reachable members are off, and that it counts again once it becomes reachable.
File summaries
| File | Description |
|---|---|
zha/application/platforms/light/__init__.py |
Updates group state derivation to exclude unavailable members from contributing to the “on” vote. |
tests/test_light.py |
Adds a test covering the offline-member-stale-state scenario for light groups. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #888 +/- ##
=======================================
Coverage 97.19% 97.19%
=======================================
Files 57 57
Lines 10560 10560
=======================================
Hits 10264 10264
Misses 296 296 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Ping? This one should be a straightforward fix. |
zigpy-review-bot
left a comment
zigpy-review-bot
left a comment
There was a problem hiding this comment.
Approve — the fix is correct, minimal, and restores parity with Home Assistant's own group-light semantics.
Worth recording why this is the right shape, since the one-line diff hides it: LightGroup.update() is a port of HA core's group light (it arrived in 98610b2, the initial import of the HA code), and there the availability gate was implicit — HA computes on_states = [state for state in states if state.state == STATE_ON] (homeassistant/components/group/light.py:213), and an unavailable entity's state string is unavailable, never on, so it was never in on_states to begin with. The port to a typed LightState dataclass carried state.on over faithfully but dropped that implicit gate, which is exactly the bug described. Adding and state.available restores the original semantics rather than inventing new ones.
The two edge cases I'd have wanted answered both hold up:
- All members unavailable doesn't produce a misleading "off":
GroupEntity.availableisany(member.available)(zha/application/platforms/init.py:765-770), so the group entity itself goes unavailable in that case and the_statevalue is never surfaced. - Keeping capabilities merged over every member while only
on_statesis filtered is right, and matches HA core's split —min_mireds/max_mireds/supported_color_modes/supported_features/effect_listare derived fromstatesthere too, only brightness / color / effect / color_mode followon_states. The comment in the diff says this explicitly, which is good.
One behavior note for the changelog rather than a change request: a group holding a reachable-and-off member plus an unreachable-and-on member now flips to off at the moment the unreachable member is marked unavailable, so state-change automations on such a group can fire where they previously wouldn't. That is the intended fix and it's what an HA group would do, but it is a user-visible transition that didn't happen before.
Optional, explicitly out of scope for this PR: FanGroup.update() (zha/application/platforms/fan/init.py:370-372) has the same shape — percentage_states / preset_mode_states filter member states with no availability check and then take [0] — so an offline member's stale percentage can win the vote there. Same class of bug, worth a separate PR if you feel like it.
Verified (8 checks)
- The new test is a real regression guard. Reverted the production line back to
[state for state in states if state.on]in a worktree at the PR head and re-ran justtest_group_state_ignores_unavailable_members— it fails; with the fix in place it passes. It is not a test that would pass either way. - Test conventions match the file. The gateway/coordinator wiring and the
await asyncio.sleep(0.1)+async_block_till_done()debounce dance mirrortest_group_member_assume_state(tests/test_light.py:1858-1861) andtest_zha_group_light_entity, andbool(entity.state.on)is the assertion form already used for group on/off there. - Availability actually propagates to the group.
Device.on_networksetter and the check-in path both route throughupdate_available(), which emitsmaybe_emit_state_changed_event()on every platform entity (zha/zigbee/device.py:869-901);Groupsubscribes each member entity'sSTATE_CHANGEDtogroup_entity.debounced_update(zha/zigbee/group.py:283-285). The becoming-available direction goes through_async_became_available()→async_initialize(False)→ re-emit, so the member's staleonis refreshed rather than merely re-counted. - Full test suite at the PR head: 1383 passed (
pytest tests/), andtests/test_light.pyis 21/21. - Merged with current dev: the branch is 3 commits behind dev (dependency bumps 890 / 891 / 892 only). Merged
origin/devin locally, re-synced, re-ran the light suite — still 21/21, so the staleness is cosmetic. - Lint/type:
ruff checkclean,ruff format --checkclean, andmypy zha/inside the worktree venv (where zigpy/zhaquirks actually resolve, unlike the pre-commit hook's isolated env) reports no issues in 59 source files. - CI: all shared-ci jobs green on 990b483 across Python 3.12 / 3.13 / 3.14, patch coverage 100%.
- Independent second opinion (a different model, read-only over the same diff, with the ha-core group-light source available for comparison): no findings.
|
Is this good to go then? |
LightGroup.update()derives on/off from every member's last reported state, with no availability check. A member that drops off the network while on keeps voting "on" indefinitely. The group then stays marked as "on" even once every reachable member is off, and only clears once the stale member itself reports again or the whole group entity is force refreshed some other way._make_members_assume_group_state()already skips unavailable members when writing optimistic state after a command.update()should apply the same rule when reading it back.