diff --git a/CHANGELOG.md b/CHANGELOG.md index 8db0838..d771790 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,16 @@ All notable changes to `ebus-sdk` are recorded here. Format follows [Keep a Chan ## [Unreleased] +### Fixed + +- `DeviceTreeBuilder.resolve_deferred()` no longer spins forever when the device a deferred spec was waiting for has already been built by an equal-but-distinct `DeviceSpec`. 0.23.0 moved `add()`'s bookkeeping to the resolved device id and did not move the deferred paths with it: `add()`'s id short-circuit returned the existing device before reaching the identity-based `self._deferred.remove(spec)`, so the spec stayed queued while `resolve_deferred()` counted the returned device as progress and looped over an unchanged queue. This is a regression of exactly the pattern the id-keying was introduced to enable, which is what made it reachable by following the new guidance. Two changes, because one of them would have been enough and the other makes the class of bug degrade instead of hang: the queue drains on the short-circuit path as well as the full-materialization path, keyed by resolved id rather than object identity; and `resolve_deferred()`'s progress is now the queue actually shrinking, never `add()` returning something, so a future path that answers without draining is a no-op rather than a spin. ([#82](https://github.com/electrification-bus/python-sdk/issues/82)) + +- `DeviceTreeBuilder` latches the id a spec resolved to when it was built, so a resolver that stops answering cannot orphan its device. A producer's `device_id` callable commonly reads the producer's own model, and that model can stop answering exactly when teardown begins; `remove()` re-resolved through the callable, got `None`, and returned silently, leaving the device live on the broker with its retained topics. `remove()`, `device_for()`, `homie_properties()` and `extend()` now consult the latch first and fall back to resolving afresh, so a stale resolver is safe and an equal-but-distinct spec still works. The latch is dropped with the device it named, so a rebuilt spec resolves again rather than answering from a dead entry. + +### Changed + +- A `PropertySpec` whose python type disagrees with a property already in the model is reused rather than refused. The check added in 0.23.0 guarded a difference with no runtime consequence: the observable property's `type` is metadata, nothing reads it, `set_value` neither coerces nor validates against it, and wire coercion belongs to the Homie property. It also misfired on the normal case, a producer whose model uses a richer python type than the datatype-derived default (an `Enum` subclass for an `ENUM` property, which in one real declaration set is 30 of 134 definitions), and it raised MID materialization, leaving a half-built device behind rather than failing at declaration time. Logged at debug instead. + ## [0.23.0] — 2026-08-20 ### Added diff --git a/src/ebus_sdk/declaration.py b/src/ebus_sdk/declaration.py index 6c89c62..7940fe8 100644 --- a/src/ebus_sdk/declaration.py +++ b/src/ebus_sdk/declaration.py @@ -18,6 +18,8 @@ from __future__ import annotations +import logging + from contextlib import nullcontext from dataclasses import dataclass from functools import partial @@ -28,6 +30,8 @@ from .property import GroupedPropertyDict from .property import Property as ObservableProperty +logger = logging.getLogger("homie") + _PYTHON_TYPE = { PropertyDatatype.FLOAT: float, PropertyDatatype.INTEGER: int, @@ -299,12 +303,18 @@ def _materialize( # it added and leaves anything the producer owned first. model_keys.append((capability, group, spec.model_key)) elif existing.type() is not py_type: - raise ValueError( - f"{capability}/{spec.prop_id}: the model already holds " - f"{group}/{spec.model_key} with type {existing.type()!r}, but this spec " - f"declares {py_type!r}. Reusing it would publish values of one type through " - "a property built for another; align the spec's datatype (or python_type) " - "with the model, or give the spec its own source_id/model_group." + # Reuse anyway, and do not raise. The observable property's + # `type` is metadata: nothing reads it, `set_value` neither + # coerces nor validates against it, and wire coercion belongs + # to the Homie property. Raising guarded a difference with no + # runtime consequence, and it misfired on the normal case, a + # producer whose model uses a richer python type than a + # datatype-derived default (an Enum subclass for an ENUM). + # Worse, it raised MID materialization, leaving half a device. + logger.debug( + "reason=declarationReusedPropertyOfDifferentType," + f"group={group},id={spec.model_key}," + f"model={existing.type()!r},spec={py_type!r}" ) declared[(capability, spec.prop_id)] = spec # An entity_setter is the translator toward the entity, so it is @@ -551,6 +561,23 @@ def __init__( # per-spec maps rather than inside them. Nothing removes a root. self._root_props: dict = {} self._root_model_keys: list = [] + self._spec_ids: dict = {} # spec object -> the id it resolved to when built + + def _resolve_id(self, spec: DeviceSpec) -> Optional[str]: + """The device id this spec names. + + What it resolved to when built, if this builder has seen this object, + else whatever it resolves to now. The latch makes a resolver that goes + stale safe (a producer's resolver may read the producer's own model, + which can stop answering once teardown begins); the fallback makes an + equal-but-distinct spec work, which is the point of keying on ids. + """ + latched = self._spec_ids.get(spec) + return latched if latched is not None else spec.resolve_device_id() + + def _undefer(self, device_id: str) -> None: + """Drop every queued spec naming this device, whichever object queued it.""" + self._deferred = [s for s in self._deferred if s.resolve_device_id() != device_id] def add(self, spec: DeviceSpec) -> Optional[Device]: """Materialize `spec` as a device, or defer it while its id is unknown. @@ -569,16 +596,23 @@ def add(self, spec: DeviceSpec) -> Optional[Device]: # re-derive, which defeats the point of a declarative API. A spec whose # id is already built returns that device unchanged: to give a built # device more capabilities, use extend(). - device_id = spec.resolve_device_id() + device_id = self._resolve_id(spec) if device_id is not None: existing = self._devices.get(device_id) if existing is not None: + # Drain the queue on THIS path too. Missing it made + # resolve_deferred() spin: it called add(), got a device back, + # counted that as progress, and looped over an unchanged queue + # forever. An equal-but-distinct spec having built the device is + # exactly the pattern id-keying was introduced to support. + self._spec_ids[spec] = device_id + self._undefer(device_id) return existing if spec.parent is None: parent_device: Optional[Device] = self._root else: - parent_id = spec.parent.resolve_device_id() + parent_id = self._resolve_id(spec.parent) parent_device = (self._devices.get(parent_id) if parent_id else None) or self.add(spec.parent) if parent_device is None: self._defer(spec) # the parent is itself waiting on an id @@ -600,6 +634,7 @@ def add(self, spec: DeviceSpec) -> Optional[Device]: # dispatch synchronously) must not leave a live device the builder has no # record of: device_for() would return None and remove() would be a # silent no-op, stranding retained topics. + self._spec_ids[spec] = device_id self._devices[device_id] = device self._homie_props[device_id] = {} self._model_keys[device_id] = [] @@ -620,8 +655,7 @@ def add(self, spec: DeviceSpec) -> Optional[Device]: self._homie_props[device_id].update(built.homie_props) self._model_keys[device_id].extend(built.model_keys) self._created_groups[device_id].extend(built.created_groups) - if spec in self._deferred: - self._deferred.remove(spec) + self._undefer(device_id) if spec.on_created is not None: spec.on_created(device) return device @@ -634,14 +668,17 @@ def resolve_deferred(self) -> list: than one call per generation. """ built: list = [] - progress = True - while progress: - progress = False + while self._deferred: + before = len(self._deferred) for spec in list(self._deferred): device = self.add(spec) - if device is not None: + if device is not None and device not in built: built.append(device) - progress = True + # Progress is the QUEUE SHRINKING, never add() returning something. + # Driving it off the return value means any future path that answers + # without draining spins here rather than degrading to a no-op. + if len(self._deferred) >= before: + break return built def remove(self, spec: DeviceSpec) -> None: @@ -659,7 +696,7 @@ def remove(self, spec: DeviceSpec) -> None: # deliberately torn down. self._deferred = [s for s in self._deferred if not _descends_from(s, spec)] - device_id = spec.resolve_device_id() + device_id = self._resolve_id(spec) device = self._devices.get(device_id) if device_id is not None else None if device is None: return # never built (or already removed); the queue is now clean @@ -686,6 +723,8 @@ def remove(self, spec: DeviceSpec) -> None: self._created_groups.pop(gone, None) self._devices.pop(gone, None) self._homie_props.pop(gone, None) + for known in [sp for sp, did in self._spec_ids.items() if did == gone]: + self._spec_ids.pop(known, None) def add_root_capabilities(self, specs: Iterable[PropertySpec], *, model_group: Optional[str] = None) -> dict: """Materialize capabilities onto the tree's ROOT device. @@ -741,7 +780,7 @@ def extend(self, spec: DeviceSpec, specs: Iterable[PropertySpec]) -> dict: Raises `KeyError` for a spec that is not built. Use `add()` first; a deferred device has no tree to extend. """ - device_id = spec.resolve_device_id() + device_id = self._resolve_id(spec) device = self._devices.get(device_id) if device_id is not None else None if device is None: raise KeyError( @@ -782,7 +821,7 @@ def remove_capabilities(self, spec: DeviceSpec, capabilities: Iterable[str]) -> Raises `KeyError` for a spec that is not built. """ - device_id = spec.resolve_device_id() + device_id = self._resolve_id(spec) device = self._devices.get(device_id) if device_id is not None else None if device is None: raise KeyError(f"{spec.device_class}: not built, so there is nothing to remove from. add() it first.") @@ -815,7 +854,7 @@ def device_for(self, spec: DeviceSpec) -> Optional[Device]: Resolved by device id, so any spec naming the same device answers. """ - device_id = spec.resolve_device_id() + device_id = self._resolve_id(spec) return self._devices.get(device_id) if device_id is not None else None def homie_properties(self, spec: DeviceSpec) -> dict: @@ -824,7 +863,7 @@ def homie_properties(self, spec: DeviceSpec) -> dict: Empty for a spec that is not built. An `internal_only` property has no Homie twin and so is absent, exactly as in `build_from_declarations`. """ - device_id = spec.resolve_device_id() + device_id = self._resolve_id(spec) return dict(self._homie_props.get(device_id, {})) if device_id is not None else {} def deferred(self) -> list: diff --git a/tests/test_declaration.py b/tests/test_declaration.py index 525c785..98d0475 100644 --- a/tests/test_declaration.py +++ b/tests/test_declaration.py @@ -576,11 +576,21 @@ def test_remove_deletes_what_the_builder_created_and_nothing_else(mock_paho): assert model.value("dev-1", "serial-number") == "SN-LIVE" -def test_a_type_disagreement_with_an_existing_property_is_refused(mock_paho): +def test_a_type_disagreement_with_an_existing_property_reuses_it(mock_paho): + """The observable `type` is metadata: nothing reads it, and `set_value` neither + coerces nor validates against it, so a difference has no runtime consequence. + Raising here misfired on the normal case of a producer using a richer python + type than the datatype-derived default, and it raised mid-materialization.""" device = _device(mock_paho, "dev-mismatch") model = _live_model(group="info") # holds a str property - with pytest.raises(ValueError, match="already holds"): - build_from_declarations(device, model, [PropertySpec("info", "serial-number", PropertyDatatype.FLOAT)]) + before = model.get("info", "serial-number") + + props = build_from_declarations(device, model, [PropertySpec("info", "serial-number", PropertyDatatype.FLOAT)]) + + assert model.get("info", "serial-number") is before # reused, not replaced + assert model.value("info", "serial-number") == "SN-LIVE" + # And the wire still works: coercion belongs to the Homie property. + assert props[("info", "serial-number")].value() == "SN-LIVE" def test_reuse_still_binds_and_publishes(mock_paho): diff --git a/tests/test_device_tree.py b/tests/test_device_tree.py index bf2914c..a17b8f0 100644 --- a/tests/test_device_tree.py +++ b/tests/test_device_tree.py @@ -522,12 +522,16 @@ def test_a_failure_during_materialization_does_not_strand_the_device(root, mock_ """The device is constructed, attached and broker-visible before materialization, so a raise must still leave it recorded and therefore removable.""" model = GroupedPropertyDict() - model.create_group("c-9") - model.add_property("c-9", ObservableProperty(id="serial-number", type=float)) # type disagrees - builder = DeviceTreeBuilder(root, model) - spec = DeviceSpec("circuit", INFO, device_id="c-9") - with pytest.raises(ValueError, match="already holds"): + def explode_on_the_second_capability(capability): + if capability == "meter": + raise RuntimeError("boom") + return f"energy.ebus.capability.{capability}" + + builder = DeviceTreeBuilder(root, model, node_type=explode_on_the_second_capability) + spec = DeviceSpec("circuit", INFO + METER, device_id="c-9") + + with pytest.raises(RuntimeError, match="boom"): builder.add(spec) assert builder.device_for(spec) is not None, "a live device with no record is unrecoverable" @@ -647,3 +651,106 @@ def test_remove_capabilities_refuses_an_unbuilt_device(root): builder = DeviceTreeBuilder(root, model) with pytest.raises(KeyError, match="not built"): builder.remove_capabilities(DeviceSpec("bess", INFO, device_id="nope"), ["shed"]) + + +# --- The deferred queue keys on ids too (GH #82) ----------------------------- + + +def test_resolve_deferred_returns_when_another_spec_built_the_device(root): + """The regression 0.23.0's id-keying introduced: add() short-circuits on the id + before draining the queue, so resolve_deferred() saw a device, called it + progress, and looped over an unchanged queue forever. + + If this regresses it HANGS rather than fails; CI's timeout-minutes is the net. + """ + model = GroupedPropertyDict() + builder = DeviceTreeBuilder(root, model) + box = {"id": None} + + builder.add(DeviceSpec("bess", INFO, device_id=lambda: box["id"])) + assert len(builder.deferred()) == 1 + + box["id"] = "bess-1" + builder.add(DeviceSpec("bess", INFO, device_id=lambda: box["id"])) # equal but NEW object + + assert builder.deferred() == [], "the queue must drain on the id short-circuit path" + assert builder.resolve_deferred() == [] + assert root.children_ids() == ["bess-1"] + + +def test_resolve_deferred_stops_when_the_queue_stops_shrinking(root): + """Progress is the queue shrinking, never add() returning something.""" + model = GroupedPropertyDict() + builder = DeviceTreeBuilder(root, model) + builder.add(DeviceSpec("bess", INFO, device_id=lambda: None)) + builder.add(DeviceSpec("pv", INFO, device_id=lambda: None)) + + assert builder.resolve_deferred() == [] + assert len(builder.deferred()) == 2 # still waiting, but it returned + + +def test_a_deferred_spec_resolving_late_still_builds(root): + model = GroupedPropertyDict() + builder = DeviceTreeBuilder(root, model) + box = {"id": None} + spec = DeviceSpec("bess", INFO, device_id=lambda: box["id"]) + builder.add(spec) + + box["id"] = "bess-1" + built = builder.resolve_deferred() + + assert [d.id() for d in built] == ["bess-1"] + assert builder.deferred() == [] + + +# --- A resolver that goes stale must not orphan its device ------------------- + + +def test_remove_works_when_the_id_resolver_has_stopped_answering(root): + """A producer's resolver often reads the producer's own model, which can stop + answering exactly when teardown begins. Re-resolving then found nothing and + remove() returned silently, leaving the device live with its retained topics.""" + model = GroupedPropertyDict() + model.create_group("src") + model.add_property("src", ObservableProperty(id="serial", type=str)) + model.set_value("src", "serial", "bess-1") + builder = DeviceTreeBuilder(root, model) + + def id_from_model(): + return model.value("src", "serial") if model.has_group("src") else None + + spec = DeviceSpec("bess", INFO, device_id=id_from_model) + builder.add(spec) + assert root.children_ids() == ["bess-1"] + + model.delete_group("src") # the resolver's source goes away + builder.remove(spec) + + assert root.children_ids() == [], "the device was left live with its retained topics" + assert builder.device_for(spec) is None + + +def test_device_for_survives_a_stale_resolver(root): + model = GroupedPropertyDict() + builder = DeviceTreeBuilder(root, model) + box = {"id": "bess-1"} + spec = DeviceSpec("bess", INFO, device_id=lambda: box["id"]) + device = builder.add(spec) + + box["id"] = None # the resolver stops answering + assert builder.device_for(spec) is device + assert builder.homie_properties(spec) != {} + + +def test_the_latch_is_dropped_with_the_device(root): + """A rebuilt spec must resolve afresh rather than answering from a dead latch.""" + model = GroupedPropertyDict() + builder = DeviceTreeBuilder(root, model) + box = {"id": "bess-1"} + spec = DeviceSpec("bess", INFO, device_id=lambda: box["id"]) + builder.add(spec) + builder.remove(spec) + + box["id"] = "bess-2" + rebuilt = builder.add(spec) + assert rebuilt.id() == "bess-2"