Skip to content

Codex updated: Preserve discovery progress and group cleanup on membership changes#835

Draft
dmulcahey wants to merge 1 commit into
devfrom
codex-updated-pr-691
Draft

Codex updated: Preserve discovery progress and group cleanup on membership changes#835
dmulcahey wants to merge 1 commit into
devfrom
codex-updated-pr-691

Conversation

@dmulcahey

@dmulcahey dmulcahey commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

This pull request was created with Codex and is an updated version of the pull request it supersedes.

Original pull request: #691

This pull request introduces robust error handling and lifecycle management improvements for device and group entity discovery in the ZHA integration. The changes ensure that exceptions during endpoint discovery do not halt discovery for other endpoints, and that group entity cleanup properly invokes lifecycle methods, preventing resource leaks and stale entities.

Improved error handling for entity discovery

  • Modified discover_device_entities to catch exceptions during endpoint discovery, log the error, and continue processing remaining endpoints, preventing partial entity loss.
  • Added a test (test_discover_device_entities_continues_after_endpoint_exception) to verify that discovery continues after an endpoint exception.

Group entity lifecycle and cleanup enhancements

  • Updated discover_group_entities to schedule lifecycle cleanup (on_remove) for group entities when group membership drops below quorum, instead of directly clearing entities, ensuring proper cleanup and avoiding leaks.
  • Added logic to prune stale group platform entities when their platform quorum drops below two, invoking cleanup for each affected entity.
  • Added tests to verify group entity cleanup (test_discover_group_entities_member_drop_runs_group_entity_on_remove) and stale platform entity pruning (test_discover_group_entities_platform_quorum_drop_prunes_stale_platform_entity).

Test infrastructure updates

  • Imported new helpers and references in tests/test_discover.py to support the new tests and discovery logic.## Summary
    Makes discovery resilient to per-endpoint exceptions and ensures stale group entities are removed via lifecycle cleanup when membership/platform quorum changes.

Failing tests addressed

  • tests/test_discover.py::test_discover_device_entities_continues_after_endpoint_exception
  • tests/test_discover.py::test_discover_group_entities_member_drop_runs_group_entity_on_remove
  • tests/test_discover.py::test_discover_group_entities_platform_quorum_drop_prunes_stale_platform_entity

Verification

  • pytest (branch-local targeted run): all listed tests pass.

Scoped replacement validation

The original summary above is preserved verbatim. This replacement is intentionally limited to endpoint-discovery resilience and safe group-entity lifecycle cleanup. Semantic group-control eligibility and retained aggregate/capability reconciliation are documented separately as follow-up work.

  • Full test suite: 1341 passed
  • Pre-commit: codespell, Ruff check, Ruff format, mypy, and uv lock passed
  • Validated against dev at da781c789433e3d8a8f8277d47bbdeb4426f9cf2

@codecov

codecov Bot commented Jul 14, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.27%. Comparing base (da781c7) to head (abed086).

Additional details and impacted files
@@           Coverage Diff           @@
##              dev     #835   +/-   ##
=======================================
  Coverage   97.27%   97.27%           
=======================================
  Files          55       55           
  Lines       10930    10954   +24     
=======================================
+ Hits        10632    10656   +24     
  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.

@dmulcahey
dmulcahey force-pushed the codex-updated-pr-691 branch 2 times, most recently from 3680327 to abed086 Compare July 15, 2026 12:05
@dmulcahey

dmulcahey commented Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

This is an interesting edge case... not sure how many folks would hit this in reality but after poking through it I figured it would be worth at least sharing it.

Codex Suggested Follow-up plan: semantic group-control eligibility and retained-group reconciliation

PR #835 is now intentionally limited to its original scope: endpoint-discovery error containment plus safe group-entity lifecycle cleanup when total or platform membership falls below quorum. The later experiment around group-control eligibility, per-endpoint counting, aggregate filtering, and retained-entity reconciliation has been removed from this PR and should be handled in a dedicated follow-up.

This comment records the evidence and implementation plan so that work is not lost.

Problem to solve

ZHA currently treats the existence of a light, fan, or switch platform entity on a member endpoint as a proxy for whether that endpoint should:

  • establish group-entity quorum;
  • receive member subscriptions;
  • contribute to group availability and aggregate state;
  • contribute membership-derived capabilities.

Those concepts are not equivalent. One endpoint may expose multiple same-platform entities, and a user-facing switch entity may write a manufacturer attribute rather than implement the OnOff command sent by SwitchGroup.

The removed experiment improved two things by counting one platform vote per endpoint and excluding configuration/diagnostic entities. However, entity_category is None is UI classification, not proof that an entity accepts the platform group command.

Zigbee invariants

The design should preserve these protocol facts:

  1. Group membership is unique by (GroupAddress, Endpoint). Re-adding the same pair succeeds without inserting a second membership entry (Zigbee R23.2 section 2.2.4.5.1.3; ZCL R8 section 3.6.2.3.2.2).
  2. An endpoint's input cluster list describes the server clusters it advertises (Zigbee R23.2 section 2.3.2.5.6).
  3. Group-addressed frames are delivered to each endpoint assigned to the group (ZCL R8 section 3.6.1).
  4. Server behavior is tied to a discoverable server cluster on that endpoint (ZCL R8 sections 2.2.1.4 and 2.2.2).

Therefore, quorum should count each unique member endpoint at most once per platform. Tests should not model duplicate insertion of the same endpoint as two memberships.

Captured-device evidence

The complete current fixture corpus was scanned:

Measurement Result
Device JSON files parsed 853
Parse failures 0
Files with an original Groups (0x0004) endpoint 572
Original endpoints advertising Groups 661
Switch entities 404
Uncategorized switch entities 217
Uncategorized ConfigurableAttributeSwitch entities 19
Those on original Groups endpoints 16
Those on Groups endpoints without OnOff (0x0006) 13 entities in 11 files

The 11 Groups-without-OnOff fixtures are:

  1. tze200-crq3r3la-ck-bl702-mws-01-7016.json - find_switch
  2. tze200-hr0tdd47-ts0601-0x00000048.json - mute_siren
  3. tze200-jva8ink8-ts0601.json - find_switch
  4. tze200-yojqa8xn-ts0601.json - self_test_switch
  5. tze204-1youk3hj-ts0601-0x0000004a.json - find_switch, output_switch
  6. tze204-chbyv06x-ts0601.json - self_test_switch
  7. tze204-k7mfgaen-ts0601.json - siren_on
  8. tze204-sooucan5-ts0601.json - find_switch
  9. tze204-uxllnywp-ts0601-0x0000004a.json - find_switch
  10. tze284-fhvpaltk-ts0601.json - valve_on_off_1, valve_on_off_2
  11. tze284-rjxqso4a-ts0601-0x0000004d.json - mute_siren

Two distinct real captures, tze200-hr0tdd47... and tze204-k7mfgaen..., were loaded as separate IEEE/endpoint members. Each endpoint appeared exactly once. Both advertise Groups, neither advertises OnOff, and each exposes an uncategorized Tuya manufacturer-cluster-backed ConfigurableAttributeSwitch. The removed experiment created a SwitchGroup for them, while SwitchGroup sends OnOff 0x0006 commands. This proves that category-only filtering is insufficient without relying on duplicate membership or purely synthetic entities.

Three additional STANDARD ConfigurableAttributeSwitch entities live on endpoints that advertise both Groups and OnOff. They remain unrelated manufacturer-attribute controls, so endpoint OnOff presence alone is also insufficient.

Recommended eligibility design

Introduce one explicit semantic capability on platform entities. Naming can follow maintainer preference; the important behavior is:

class PlatformEntity:
    _attr_supports_group_control = True

    @property
    def supports_group_control(self) -> bool:
        return self.entity_category is None and self._attr_supports_group_control


class ConfigurableAttributeSwitch(PlatformEntity):
    _attr_supports_group_control = False

Then:

  • derive one shared collection of eligible group-control entities per GroupMember;
  • use that same collection for quorum, subscriptions, availability, aggregate state, and capabilities;
  • count a set of eligible platforms per member, producing one vote per unique endpoint/platform;
  • preserve current behavior by default and narrowly opt out the concretely proven outlier;
  • add more opt-outs only when evidence demonstrates another semantic mismatch.

This is preferable to importing and type-checking ConfigurableAttributeSwitch in generic group code, and it avoids an import cycle.

Why simpler heuristics are insufficient

  • entity_category is None and EntityType.STANDARD describe presentation, not group-command semantics.
  • primary is device-wide UI selection, not endpoint-level command eligibility.
  • The concrete backing cluster does not identify the operation: a real OnOff Switch and a STANDARD attribute switch can both be backed by server cluster 0x0006, while one sends commands and the other writes an attribute.
  • _cluster_match, targets_cluster(), or backing-cluster equality can reject legitimate specialized entities. STARKVIND endpoints advertise Groups and the standard Fan server cluster 0x0202, but IkeaFan is backed by manufacturer cluster 0xfc7d and should remain groupable.
  • Endpoint OnOff presence cannot distinguish a genuine OnOff control from an unrelated manufacturer attribute on the same endpoint.

The explicit capability makes the semantic contract reviewable instead of inferring it from discovery metadata.

Retained group reconciliation

After eligibility is defined correctly, direct member-add/remove handling should:

  1. rediscover or prune group platforms;
  2. refresh subscriptions from the eligible current members;
  3. recompute membership-derived capabilities on retained group entities;
  4. recompute and emit retained aggregate state.

This is where Group.reconcile_group_entities() belongs.

The light minimum-transition reset belongs in the same follow-up. LightGroup.recompute_capabilities() raises _DEFAULT_MIN_TRANSITION_TIME to 0.1 seconds when any current member needs it. It must restore the base default before rescanning, otherwise the value remains sticky after the last such member leaves.

Required tests

  1. Two distinct real CAS-only fixtures with Groups but no OnOff do not create a SwitchGroup.
  2. One real OnOff switch plus one CAS-only endpoint does not establish switch quorum.
  3. Two real OnOff endpoints plus CAS-only endpoints create one SwitchGroup; only real switches feed subscriptions, availability, and state.
  4. Removing one real switch prunes the group switch even when multiple CAS-only endpoints remain.
  5. Multiple eligible same-platform entity objects associated with one endpoint produce one quorum vote, without duplicating that endpoint's membership.
  6. Both STARKVIND fixtures remain eligible for FanGroup.
  7. Configuration and diagnostic entities never establish quorum or feed group state.
  8. Direct add/remove refreshes subscriptions while preserving a retained group entity's identity.
  9. Retained light state, supported modes/features, execute-if-off behavior, and minimum transition time recompute in both directions.
  10. Delayed lifecycle cleanup still cannot unregister a same-ID replacement.
  11. Run all fixture discovery tests, group tests, the full suite, Ruff, formatting, mypy, and the normal CI matrix.

Separate physical-groupcast question

The corpus also contains eight Light fixtures and three ordinary Switch fixtures whose original descriptors advertise Groups but not OnOff, while quirks synthesize an OnOff-facing entity. A physical groupcast is sent directly over the air and may bypass coordinator-side per-device quirk translation used for unicast control.

The minimal follow-up should not silently reject those devices based only on JSON evidence. Original-signature enforcement should be an explicit design decision, preferably supported by hardware testing and an override mechanism for known non-conformant devices.

Acceptance criteria and sequencing

  • Exactly one quorum contribution per member endpoint and platform.
  • Only semantically group-controllable entities contribute to discovery, subscriptions, availability, state, or capabilities.
  • The two real Tuya CAS fixtures cannot create or preserve a SwitchGroup.
  • Valid OnOff, Light, and STARKVIND Fan groups remain supported.
  • Retained entities reconcile immediately and reversibly after membership changes.
  • No test contradicts Zigbee group-membership uniqueness.
  • Full tests and static checks pass without unexplained fixture/entity-shape changes.

Suggested implementation sequence:

  1. Agree on the semantic capability name and keep physical original-signature validation out of the first follow-up unless maintainers explicitly include it.
  2. Add the narrow ConfigurableAttributeSwitch opt-out, shared filtering, and per-endpoint/platform deduplication.
  3. Add retained subscription/state/capability reconciliation and the reversible light transition reset in a separate commit.
  4. Add real-fixture and STARKVIND counterexample tests.
  5. Run the complete validation matrix.
  6. Track virtual-OnOff/original-signature enforcement separately until hardware evidence supports a policy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant