Log unique ID conflicts#683
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #683 +/- ##
==========================================
- Coverage 97.51% 97.50% -0.02%
==========================================
Files 62 62
Lines 10949 10969 +20
==========================================
+ Hits 10677 10695 +18
- Misses 272 274 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
zigpy-review-bot
left a comment
zigpy-review-bot
left a comment
There was a problem hiding this comment.
Draft, so this is mostly setup notes for when it comes out of WIP. The mechanic is right — useful diagnostic that surfaces a real bug (FirmwareUpdateEntity vs FirmwareUpdateServerEntity, both @register_entity(Ota.cluster_id) with _unique_id_suffix = "firmware_update" — they unconditionally collide on every OTA-capable device). The class-difference guard avoids spamming on benign same-class re-discovery, which is the right policy.
Logging level / call cardinality. _LOGGER.warning is correct for a real conflict. But your PR body shows each conflict logged ~4-7 times per device — that's the gateway re-driving async_initialize (cache=True initial, cache=False after online, re-interview, etc.) and _pending_entities never being reset between calls, so the same entity pair gets dedup'd again every pass. Two ways to handle:
- Tag the conflict on
self(self._logged_unique_id_conflicts: set[tuple[Platform, str]]) and check before logging, so a given key only fires once per device lifetime. Cheap, no behavior change. - Or accept multi-emission as evidence of the lifecycle bug — but then warning-level fires N times for the same problem and pollutes logs at scale.
I'd go with the per-device set. Keeps warning-level (correct), avoids the dup spam.
Logging-only vs reconcile. The PR title implies logging-only and the code matches — duplicates still exist (one is dropped via on_remove(), but the underlying registration duplication that produced them remains). Worth a one-liner in the PR body confirming this is intentional surface-the-bug logging, not a fix; the actual fix is presumably in @register_entity filtering (e.g. should FirmwareUpdateServerEntity use a different _unique_id_suffix, or not register on devices where FirmwareUpdateEntity already matches?).
Hot path. Runs once per async_initialize, only on dedup-collision. Not a hot path concern.
Type annotation nit. existing_entity/duplicate_entity are typed BaseEntity but the call site at 1060 passes from dict[tuple[Platform, str], PlatformEntity], and the new entity is also a PlatformEntity (it was just keyed off entity.unique_id + entity.PLATFORM after is_supported_in_list). The isinstance(entity, PlatformEntity) checks in _entity_endpoint_id / _entity_cluster_handler_names are dead branches today. Either narrow the param types to PlatformEntity and drop the isinstance guards, or leave the guards and document why (future-proofing for non-platform conflict checks). The current state is type-annotation overhead with no payoff.
Tests. None added. For a draft that's fine, but before ready-for-review: a small test asserting the warning fires with the expected fields would be cheap (mock two PlatformEntity subclasses with the same unique_id, run them through async_initialize, assert the log record). Codecov flagged 2 lines uncovered, presumably the early-return same-class branch — those are the lines worth testing.
| if existing_class == duplicate_class: | ||
| return | ||
|
|
||
| _LOGGER.warning( |
There was a problem hiding this comment.
_LOGGER.warning is the right level for a real conflict, but with _pending_entities never reset between async_initialize calls (gateway drives it multiple times: cache=True initial, cache=False after online, re-interview), this fires per call for the same device. The PR body's 4-7 repeats per IEEE confirm it.
Suggestion: gate on a per-device set[tuple[Platform, str]] so each (platform, unique_id) conflict is logged once per device lifetime. Keeps warning-level (which is right — conflicts are bugs), without N-spamming the same finding.
# in __init__:
self._logged_unique_id_conflicts: set[tuple[Platform, str]] = set()
# in _log_entity_unique_id_conflict, after the class-equality check:
if key in self._logged_unique_id_conflicts:
return
self._logged_unique_id_conflicts.add(key)| self, | ||
| key: tuple[Platform, str], | ||
| existing_entity: BaseEntity, | ||
| duplicate_entity: BaseEntity, |
There was a problem hiding this comment.
existing_entity/duplicate_entity typed as BaseEntity but the call site (line 1060) only ever passes PlatformEntity — new_entities is dict[tuple[Platform, str], PlatformEntity] and entity reached this point via entity.PLATFORM / entity.unique_id. The isinstance(entity, PlatformEntity) guards in _entity_endpoint_id / _entity_cluster_handler_names are dead branches.
Either narrow these to PlatformEntity and drop the guards (cleaner), or leave the guards in and add a comment explaining the future use case. Current shape is type-annotation overhead with no payoff.
|
|
||
| # Ignore entities that already exist | ||
| if key in new_entities: | ||
| self._log_entity_unique_id_conflict(key, new_entities[key], entity) |
There was a problem hiding this comment.
Worth confirming in the PR body: this is intentionally surface-the-bug logging, not a fix. The duplicate FirmwareUpdateServerEntity registration on Ota.cluster_id with the same _unique_id_suffix = "firmware_update" is the underlying issue; this PR just makes it visible. The actual fix presumably lives in giving the server entity a distinct suffix or scoping its @register_entity to coordinator devices only (since it's the OTA server side).
2 participants
This is experimental. It prints all unique ID conflicts (four times...)
These are existing ones: