From f459152773c4ba7610610af2633610db23196352 Mon Sep 17 00:00:00 2001 From: Donald Clark Jackson Date: Thu, 20 Aug 2026 19:17:28 -0700 Subject: [PATCH] fix: five defects in the declarative builder, all silent in production Reported by a consumer converting a multi-device adapter onto the builder, each with a runnable reproduction. All five verified against the reproductions before and after. #72 conditionally_settable was inert: _materialize never read it. The property did come out not-settable, which is the half that looked right; the entity_setter was registered only when settable was true, so the caller's later set_settable(True) opened a /set topic with no translator behind it. The property advertised that it accepts commands and discarded them, which is the exact failure the field exists to avoid, one step further along. The translator is now wired at build time even though the property starts not-settable. The test that shipped with the feature asserted only the not-settable half. A test written from the design rationale checks the rationale, not the feature. #77 A value the model already held never reached the wire. The binding is on-change and a fresh twin starts empty, so a producer whose model predates the tree announced its declared default on every property, and it did not self-heal, because set_value fires only on an actual change. Reachable only since 0.22.0: before the reuse fix the model property was replaced, so twin and model started equally empty. Also: a declared initial_value now seeds rather than overwrites, since a live model value is fresher than a declaration. #73 remove() raised when the producer's model had already dropped the group. delete_group removes it BEFORE firing GROUP_DELETED and dispatch is synchronous, so a consumer driving remove() from that event always hit it, after device.delete() and before the bookkeeping pop: gone from the broker, still a corpse in the builder short-circuiting the next add(). #75 remove() left deferred descendants queued, so resolve_deferred() rebuilt a device that had been torn down. #76 add() recorded bookkeeping after materializing, so a raise left a live broker-visible device with no record, and the same window admitted re-entry from a producer observing its own model. Closes #72 Closes #73 Closes #75 Closes #76 Closes #77 Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 12 +++++ src/ebus_sdk/declaration.py | 97 +++++++++++++++++++++++++++------- tests/test_declaration.py | 102 ++++++++++++++++++++++++++++++++++++ tests/test_device_tree.py | 100 +++++++++++++++++++++++++++++++++++ 4 files changed, 292 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5365aaf..2b2ea89 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,18 @@ All notable changes to `ebus-sdk` are recorded here. Format follows [Keep a Chan ## [Unreleased] +### Fixed + +- `conditionally_settable` was inert: `_materialize` never read it. The half that looked right is that the property did come out not-settable; the half that bit is that the `entity_setter` was registered only when `settable` was true, so the caller's later `set_settable(True)` opened a `/set` topic with no translator behind it. The property then advertised that it accepts commands and silently discarded them, which is the exact failure the field was introduced to avoid, one step further along. It is the only route the API offers for per-instance settability decided at runtime, and it was the route that did not work. The translator is now wired at build time even though the property starts not-settable. The test that shipped with the feature asserted only the not-settable half, which is why this survived review: a test written from the design rationale checks the rationale rather than the feature. ([#72](https://github.com/electrification-bus/python-sdk/issues/72)) + +- A value the model already held is now published when its Homie twin is built. The binding is on-change and a fresh twin starts empty, so a producer whose model predates the tree (the arrangement `DeviceTreeBuilder` documents as its reason for accepting an external model) announced its declared default instead of the live value, on every property. It did not self-heal, because `set_value` fires callbacks only on an actual change, so a value written once at group creation stayed wrong for the process lifetime. This became reachable in 0.22.0: before the reuse fix the model property was replaced, so twin and model started equally empty. Relatedly, a declared `initial_value` now SEEDS rather than overwrites, since a model already holding a value holds a fresher one than the declaration; an explicit `values` entry still wins, being a statement about this run. ([#77](https://github.com/electrification-bus/python-sdk/issues/77)) + +- `DeviceTreeBuilder.remove()` no longer raises when the producer's model has already dropped the group. `GroupedPropertyDict.delete_group` removes the group BEFORE firing `GROUP_DELETED` and dispatch is synchronous, so a consumer driving `remove()` from that event was guaranteed to hit it. It raised after `device.delete()` and before the bookkeeping pop, leaving the device gone from the broker while the builder still held a corpse that short-circuited the next `add()`: unrecoverable for the process lifetime, and inside an observer callback it surfaced as a single swallowed warning. Bookkeeping is now dropped in a `finally`, and the per-property `deletePropertyGroupNotFound` warning burst is gone with it, which matters on bounded-disk fleet devices. ([#73](https://github.com/electrification-bus/python-sdk/issues/73)) + +- `DeviceTreeBuilder.remove()` now prunes deferred descendants, not only the removed spec. A deferred child holds a frozen reference to its parent spec, so `resolve_deferred()` would rebuild a device that had been deliberately torn down. The shape most likely to hit it is a mandatory child deferred on a late identifier, which sits in the queue for exactly the window in which its parent might be removed. ([#75](https://github.com/electrification-bus/python-sdk/issues/75)) + +- `DeviceTreeBuilder.add()` records its bookkeeping before materializing rather than after. The device is constructed, attached and broker-visible by then, so a raise left a live device the builder had no record of: `device_for()` returned None, `remove()` was a silent no-op, and the retained topics were stranded. The same window admitted re-entry, since the model's events dispatch synchronously and a producer observing its own model could call `add()` again before the cache entry existed. ([#76](https://github.com/electrification-bus/python-sdk/issues/76)) + ## [0.22.0] — 2026-08-20 ### Added diff --git a/src/ebus_sdk/declaration.py b/src/ebus_sdk/declaration.py index f1a3fc2..1b91f90 100644 --- a/src/ebus_sdk/declaration.py +++ b/src/ebus_sdk/declaration.py @@ -81,8 +81,11 @@ class PropertySpec: runtime, per instance. The builder materializes it NOT settable, which keeps `$description` truthful and leaves no `/set` subscription open on a property that would reject the command; the caller enables it with - `homie.Property.set_settable(True)` inside a `state_transition()`. It is - mutually exclusive with `settable`, which means "settable now". + `homie.Property.set_settable(True)` inside a `state_transition()`. The + `entity_setter` is wired at build time even though the property starts + not-settable, because `set_settable(True)` subscribes immediately and a + `/set` topic with no translator behind it accepts commands and discards + them. It is mutually exclusive with `settable`, which means "settable now". * `source_id` / `model_group`: the observable-model identity, when it differs from the wire identity. `source_id` defaults to `prop_id` and `model_group` to `capability`, so they are fused unless split. Splitting @@ -306,7 +309,9 @@ def _materialize( declared[(capability, spec.prop_id)] = spec # An entity_setter is the translator toward the entity, so it is # registered whenever one is given and the model can reach it. - if spec.entity_setter is not None and (spec.settable or spec.internal_only): + if spec.entity_setter is not None and ( + spec.settable or spec.conditionally_settable or spec.internal_only + ): model.set_entity_setter(group, spec.model_key, spec.entity_setter) if spec.internal_only or node is None: continue @@ -333,10 +338,23 @@ def _materialize( continue homie_prop = node.add_property_from_dict(prop_dict) bind_property_to_homie(model, group, spec.model_key, homie_prop) + # The binding is on-change, and the twin starts empty, so a value + # the model was already holding would never reach the wire: a + # producer whose model predates the tree would publish its + # declared default forever, and it would not self-heal, because + # set_value fires callbacks only on an actual change. + current = model.value(group, spec.model_key) + if current is not None: + homie_prop.set_value(current) # Inbound/control path for a settable property with a translator: # /set payload -> model.set_entity -> entity_setter. The /set # subscription is already live from add_property -> set_subscribe. - if spec.settable and spec.entity_setter is not None: + # conditionally_settable too, and this is the whole point of it: + # the property is built not-settable, so no /set topic is open + # yet, but the caller flips it with set_settable(True) later and + # that subscribes immediately. Wiring the translator now is what + # stops that topic from accepting commands and discarding them. + if spec.entity_setter is not None and (spec.settable or spec.conditionally_settable): homie_prop.set_set_callback(partial(model.set_entity, group, spec.model_key)) homie_props[(capability, spec.prop_id)] = homie_prop @@ -381,8 +399,14 @@ def _seed( them: a caller passing a runtime map is being more specific than the static declaration. Entries naming an undeclared property are ignored. """ + # A declared initial_value SEEDS, it does not overwrite: a model that already + # holds a value for this property holds a fresher one than the declaration. + # An explicit `values` entry still wins below, since that caller is being + # specific about this run rather than about the property in general. seed: dict[tuple, Any] = { - key: spec.initial_value for key, spec in declared.items() if spec.initial_value is not None + key: spec.initial_value + for key, spec in declared.items() + if spec.initial_value is not None and model.value(_group_for(spec, default_group), spec.model_key) is None } if values: seed.update({key: value for key, value in values.items() if key in declared}) @@ -560,6 +584,17 @@ def add(self, spec: DeviceSpec) -> Optional[Device]: type=spec.resolve_device_type(), parent=parent_device, ) + # Record BEFORE materializing. The device is already constructed, + # attached and visible on the broker, so a raise below (or a re-entrant + # add() from a producer observing its own model, since the model's events + # 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._devices[spec] = device + self._homie_props[spec] = {} + self._model_keys[spec] = [] + self._created_groups[spec] = [] + group = spec.resolve_model_group(device_id) built = _materialize( device, @@ -572,10 +607,9 @@ def add(self, spec: DeviceSpec) -> Optional[Device]: ) _seed(self._model, built.declared, default_group=group) - self._devices[spec] = device - self._homie_props[spec] = built.homie_props - self._model_keys[spec] = built.model_keys - self._created_groups[spec] = built.created_groups + self._homie_props[spec].update(built.homie_props) + self._model_keys[spec].extend(built.model_keys) + self._created_groups[spec].extend(built.created_groups) if spec in self._deferred: self._deferred.remove(spec) if spec.on_created is not None: @@ -609,23 +643,38 @@ def remove(self, spec: DeviceSpec) -> None: too, along with any group it created that is now empty; a group the caller created, or one still in use, is left alone. """ + # Deferred descendants go whether or not this spec was ever built: a + # deferred child holds a frozen reference to its parent spec, so leaving + # it in the queue lets resolve_deferred() rebuild a device that was + # deliberately torn down. + self._deferred = [s for s in self._deferred if not _descends_from(s, spec)] + device = self._devices.get(spec) if device is None: - if spec in self._deferred: - self._deferred.remove(spec) # never built, just stop waiting for it - return + return # never built (or already removed); the queue is now clean doomed = {id(d) for d in _descendants(device)} removed = [s for s, d in self._devices.items() if id(d) in doomed] device.delete() for gone in removed: - for group, model_key in self._model_keys.pop(gone, []): - self._model.delete_property(group, model_key) - for group in self._created_groups.pop(gone, []): - if not self._model.items(group): - self._model.delete_group(group) - self._devices.pop(gone, None) - self._homie_props.pop(gone, None) + # Bookkeeping is dropped whatever the model does, so a teardown can + # never leave a corpse in _devices that short-circuits the next + # add(). The model may legitimately have moved on already: a consumer + # driving remove() from a GROUP_DELETED observer is guaranteed to + # arrive after the group is gone, since delete_group removes it + # before firing and dispatch is synchronous. + try: + for group, model_key in self._model_keys.get(gone, []): + if self._model.has_group(group) and self._model.get(group, model_key) is not None: + self._model.delete_property(group, model_key) + for group in self._created_groups.get(gone, []): + if self._model.has_group(group) and not self._model.items(group): + self._model.delete_group(group) + finally: + self._model_keys.pop(gone, None) + self._created_groups.pop(gone, None) + self._devices.pop(gone, None) + self._homie_props.pop(gone, None) def add_root_capabilities(self, specs: Iterable[PropertySpec], *, model_group: Optional[str] = None) -> dict: """Materialize capabilities onto the tree's ROOT device. @@ -724,6 +773,16 @@ def _defer(self, spec: DeviceSpec) -> None: self._deferred.append(spec) +def _descends_from(spec: DeviceSpec, ancestor: DeviceSpec) -> bool: + """True when `spec` is `ancestor` or is declared beneath it.""" + current: Optional[DeviceSpec] = spec + while current is not None: + if current is ancestor: + return True + current = current.parent + return False + + def _descendants(device: Device) -> list: """`device` and every device beneath it, parents before children.""" found = [device] diff --git a/tests/test_declaration.py b/tests/test_declaration.py index c567b23..525c785 100644 --- a/tests/test_declaration.py +++ b/tests/test_declaration.py @@ -592,3 +592,105 @@ def test_reuse_still_binds_and_publishes(mock_paho): ) model.set_value("info", "serial-number", "SN-NEW") assert homie_props[("info", "serial-number")].value() == "SN-NEW" + + +# --- conditionally_settable reaches the inbound path (GH #72) ---------------- + + +def test_conditionally_settable_registers_its_entity_setter(mock_paho): + device = _device(mock_paho, "dev-cond-setter") + model = GroupedPropertyDict() + received = [] + build_from_declarations( + device, + model, + [ + PropertySpec( + "control", + "limit", + PropertyDatatype.FLOAT, + conditionally_settable=True, + entity_setter=received.append, + ) + ], + ) + model.set_entity("control", "limit", 42.0) + assert received == [42.0], "the translator was never registered on the model" + + +def test_enabling_a_conditionally_settable_property_gives_a_working_set_topic(mock_paho): + """The whole point of the field: the /set topic opened later must have a handler.""" + device = _device(mock_paho, "dev-cond-flip") + model = GroupedPropertyDict() + received = [] + props = build_from_declarations( + device, + model, + [ + PropertySpec( + "control", + "limit", + PropertyDatatype.FLOAT, + conditionally_settable=True, + entity_setter=received.append, + ) + ], + ) + hp = props[("control", "limit")] + assert hp.settable() is False # still built not-settable + + with device.state_transition(): + hp.set_settable(True) + + # Subscribed AND wired: a topic that accepts commands and discards them is + # the failure this field exists to avoid. + assert [c for c in mock_paho.subscribe.call_args_list if c.args and str(c.args[0]).endswith("/control/limit/set")] + assert hp.get_set_callback() is not None + hp.get_set_callback()("42.0") + assert received == ["42.0"] + + +# --- a live model value reaches the wire (GH #77) ---------------------------- + + +def test_a_value_the_model_already_held_is_published(mock_paho): + device = _device(mock_paho, "dev-live") + model = _live_model(group="info") # holds SN-LIVE before any tree exists + props = build_from_declarations(device, model, [PropertySpec("info", "serial-number", PropertyDatatype.STRING)]) + # The binding is on-change and the twin starts empty, so without an explicit + # push the wire would show the declared default for the process lifetime. + assert props[("info", "serial-number")].value() == "SN-LIVE" + assert "SN-LIVE" in [c.args[1] for c in mock_paho.publish.call_args_list if len(c.args) > 1] + + +def test_initial_value_seeds_but_does_not_clobber_a_live_value(mock_paho): + device = _device(mock_paho, "dev-live-2") + model = _live_model(group="info") + build_from_declarations( + device, + model, + [PropertySpec("info", "serial-number", PropertyDatatype.STRING, initial_value="DECLARED")], + ) + assert model.value("info", "serial-number") == "SN-LIVE" + + +def test_an_explicit_values_entry_still_wins_over_a_live_value(mock_paho): + """`values` is a statement about this run, so it is more specific than either.""" + device = _device(mock_paho, "dev-live-3") + model = _live_model(group="info") + build_from_declarations( + device, + model, + [PropertySpec("info", "serial-number", PropertyDatatype.STRING)], + values={("info", "serial-number"): "RUNTIME"}, + ) + assert model.value("info", "serial-number") == "RUNTIME" + + +def test_the_tree_builder_publishes_live_values_too(mock_paho): + root = _device(mock_paho, "enclosure-live") + model = _live_model(group="dev-1") + builder = DeviceTreeBuilder(root, model) + spec = DeviceSpec("circuit", [PropertySpec("info", "serial-number", PropertyDatatype.STRING)], device_id="dev-1") + builder.add(spec) + assert builder.homie_properties(spec)[("info", "serial-number")].value() == "SN-LIVE" diff --git a/tests/test_device_tree.py b/tests/test_device_tree.py index 1c883c2..c568da2 100644 --- a/tests/test_device_tree.py +++ b/tests/test_device_tree.py @@ -6,10 +6,12 @@ import pytest from ebus_sdk import ( + ChangeEvent, DeviceSpec, DeviceTreeBuilder, Device, GroupedPropertyDict, + ObservableProperty, PropertyDatatype, PropertySpec, Unit, @@ -426,3 +428,101 @@ def test_extend_refuses_a_device_that_is_not_built(root): builder.add(spec) # deferred, so no tree to extend with pytest.raises(KeyError, match="not built"): builder.extend(spec, [PropertySpec("shed", "shed-state", PropertyDatatype.ENUM)]) + + +# --- Teardown and bookkeeping robustness (GH #73, #75, #76) ------------------ + + +def test_remove_survives_a_group_the_producer_already_deleted(root, mock_paho): + """A consumer driving remove() from a GROUP_DELETED observer always arrives late: + delete_group removes the group before firing, and dispatch is synchronous.""" + model = GroupedPropertyDict() + builder = DeviceTreeBuilder(root, model) + spec = DeviceSpec("circuit", INFO, device_id="c-1") + builder.add(spec) + + model.delete_group("c-1") + builder.remove(spec) # must not raise + + # And no corpse: a corpse would short-circuit the next add() for the whole + # process lifetime, with the device already gone from the broker. + assert builder.device_for(spec) is None + assert builder.add(spec) is not None + + +def test_remove_drops_bookkeeping_even_when_the_model_is_uncooperative(root, mock_paho): + model = GroupedPropertyDict() + builder = DeviceTreeBuilder(root, model) + spec = DeviceSpec("circuit", INFO, device_id="c-1") + builder.add(spec) + model.delete_property("c-1", "serial-number") # producer got there first + + builder.remove(spec) + assert builder.device_for(spec) is None + assert builder.homie_properties(spec) == {} + + +def test_remove_prunes_deferred_descendants(root, mock_paho): + """A deferred child holds a frozen reference to its parent spec, so leaving it + queued lets resolve_deferred() rebuild a device that was torn down.""" + model = GroupedPropertyDict() + builder = DeviceTreeBuilder(root, model) + parent = DeviceSpec("bess", INFO, device_id="bess-1") + child = DeviceSpec("mid", METER, device_id=lambda: None, parent=parent) + builder.add(parent) + assert builder.add(child) is None # deferred on a late identifier + + builder.remove(parent) + + assert builder.deferred() == [] + assert builder.resolve_deferred() == [] + assert root.children_ids() == [] + + +def test_removing_a_deferred_parent_also_prunes_its_deferred_child(root): + model = GroupedPropertyDict() + builder = DeviceTreeBuilder(root, model) + parent = DeviceSpec("bess", INFO, device_id=lambda: None) + child = DeviceSpec("mid", METER, device_id="mid-1", parent=parent) + builder.add(parent) + builder.add(child) + assert len(builder.deferred()) == 2 + + builder.remove(parent) + assert builder.deferred() == [] + + +def test_a_failure_during_materialization_does_not_strand_the_device(root, mock_paho): + """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"): + builder.add(spec) + + assert builder.device_for(spec) is not None, "a live device with no record is unrecoverable" + builder.remove(spec) + assert builder.device_for(spec) is None + assert root.children_ids() == [] + + +def test_a_reentrant_add_from_a_model_observer_does_not_duplicate(root, mock_paho): + """The model dispatches synchronously, so a producer watching its own model can + re-enter add() while the first call is still materializing.""" + model = GroupedPropertyDict() + builder = DeviceTreeBuilder(root, model) + spec = DeviceSpec("circuit", INFO, device_id="c-1") + seen = [] + + def reenter(*args, **kwargs): + seen.append(builder.add(spec)) + + model.add_observer(reenter, event_types=[ChangeEvent.PROPERTY_ADDED]) + device = builder.add(spec) + + assert all(d is device for d in seen if d is not None) + assert root.children_ids() == ["c-1"]