Skip to content

Entity discovery silently stops at the first failing entity: the defensive loop in _discover_new_entities is defeated by generator semantics #886

Description

@zigpy-review-bot

Split out of the review discussion on #884 (#884 (comment)). Not caused by that PR — it is pre-existing, introduced with the defensive loop in #762 — but #884 adds the first raise that can reach it from config data, which is what made it worth writing down.

What happens

Device._discover_new_entities iterates entity discovery defensively so that one bad entity does not cost the whole device (zha/zigbee/device.py:1081-1097 on dev at 6660343):

# Iterate defensively so a failure in any single entity construction
# does not abort discovery for the rest of the device.
iterator = iter(self.discover_entities())
while True:
    try:
        entity = next(iterator)
    except StopIteration:
        break
    except Exception:  # pylint: disable=broad-except
        _LOGGER.exception("Failed to create entity during discovery")
        continue
    ...

That does not do what the comment says. discover_entities is a generator, and a generator that propagates an exception is closed. The continue therefore lands on a dead iterator: the next next() raises StopIteration, the loop breaks, and every entity after the failing one is silently dropped.

So the real behaviour is "discovery stops at the first failing entity", not "the failing entity is skipped" — and the log line says Failed to create entity during discovery, singular, which reads exactly like the intended behaviour.

Reproduced

The loop shape in isolation, four entities with a raise on the second:

def inner(items):
    for i in items:
        if i == 2:
            raise ValueError("bad entity 2")
        yield i

def discover_entities():
    yield from inner([1, 2, 3, 4])

discovered = []
iterator = iter(discover_entities())
while True:
    try:
        entity = next(iterator)
    except StopIteration:
        break
    except Exception as e:
        print(f"  caught: {e!r} -> continue")
        continue
    discovered.append(entity)

print("discovered:", discovered)
  caught: ValueError('bad entity 2') -> continue
discovered: [1]     # the guard's intent would be [1, 3, 4]

Why it is not biting today, and when it would

Every built-in _server_cluster_config is a class-level literal, so a malformed AttrConfig fails at import time rather than during discovery. The zhaquirks follow-up described in #884 only sets reporting_override=True inside the reporting_config is not None branch, which cannot violate the new invariant either.

It becomes reachable the moment entity config is built from quirk-supplied metadata rather than from literals — at which point the failure mode a user sees is "device came up missing an arbitrary tail of its entities, with one exception in the log", which is considerably harder to diagnose than "one entity missing".

Fix shape

The try/except has to sit inside the generator, around the per-entity construction, so the generator survives and keeps yielding — e.g. in discover_entities_for_endpoint (zha/application/discovery.py:162) at the point each entity is instantiated, or in Device.discover_entities wrapping each endpoint's sub-iterator. Whatever the placement, a regression test with a generator that raises mid-stream is the thing worth having, since the current shape looks correct at the call site.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions