review: address Opus findings on the subdevice flat-href fallback

- coordinator: _poll_subdevice_flat_hrefs called sess.pace() outside its
  try block and re-read self._session instead of using the caller's
  already-None-checked reference -- a session closed mid-poll (async_close()
  doesn't hold _session_lock) could crash the whole poll cycle instead of
  just dropping that one sibling. Now takes sess from the caller and guards
  pace() the same as get().
- coordinator: skip flat hrefs already covered by the hot/warm sub-poll
  tiers -- those are refreshed every few seconds by _run_subpolls already,
  so re-fetching them again on the once-per-summary-poll flat pass only adds
  GETs, not freshness. Matters because, unlike the Collection path (always
  one GET), a flat subdevice's summary-poll cost scales with its href count.
- subdevices.py module docstring: corrected an overclaim inherited from PR
  #199 that GET /<uuid>/device/0 had been "confirmed live" on jhkwon19's
  unit. Only an individual /information/vs/0 read was ever actually
  confirmed; the Collection endpoint itself has never been observed to
  answer on any known unit, which is exactly what issue #205 exposes.
- SKILL.md: fixed the numofsubdevice cross-check formula to match what
  coordinator.py actually compares (len(materialized) + 1, not
  len(subdevices) + len(subdevices_skipped)).
- Documented, not yet guarded against: a firmware that echoes state back
  under any unrecognized prefix instead of 4.04ing could pass the flat probe
  and the liveness gate, materializing a phantom duplicate of the master.
  Every board seen so far genuinely 4.04s on paths it doesn't own.
- Added test coverage the review flagged as missing: a materialized (not
  just skipped) flat subdevice re-polling end-to-end through to
  canonical_resources, the hot/warm skip itself, zero-master-hrefs and
  two-UUID no-cross-contamination edge cases, and a golden file for the
  #205 fixture (SKILL.md's "fixture + golden + test" discipline).
This commit is contained in:
Marc Billow
2026-07-30 02:44:26 +00:00
parent e78d941af3
commit 8600985a36
8 changed files with 270 additions and 26 deletions
@@ -400,8 +400,11 @@ the dump in this order; each step rules out a different cause.
more of its hrefs are confirmed live; that's the gate working as
intended, not a bug to chase.
4. **`multidevice.numofsubdevice`** — the board's own count, where it
reports one. Disagreement with `len(subdevices) + len(subdevices_skipped)`
is a strong hint, not proof; only one board family is known to expose it.
reports one. `coordinator._run_discovery` compares it against
`len(materialized) + 1` (materialized subdevices plus the master) and
only warns on disagreement — `subdevices_skipped` entries don't count
toward either side, since they never materialized. A strong hint, not
proof; only one board family is known to expose it.
5. **Which pattern is this board?** `identity.resources['/oic/res']` listing
`/device/1`, `/device/2` means indexed siblings. `resources['/subdevices/
vs/0']` carrying a `subdeviceIdList` means a UUID-prefixed tree, and that
+24 -7
View File
@@ -415,7 +415,7 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]):
if sess is None:
return {}
if subdevice.flat_hrefs:
return self._poll_subdevice_flat_hrefs(subdevice)
return self._poll_subdevice_flat_hrefs(subdevice, sess)
try:
code, payload = sess.get(list(subdevice.seed_path), timeout=10.0)
if code == 0x45 and payload:
@@ -426,23 +426,40 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]):
self._log.debug("subdevice %s seed poll failed: %s", subdevice.key, e)
return {}
def _poll_subdevice_flat_hrefs(self, subdevice: Subdevice) -> dict[str, dict]:
def _poll_subdevice_flat_hrefs(self, subdevice: Subdevice, sess) -> dict[str, dict]:
"""Re-poll a flat-mode prefixed subdevice's hrefs individually
(issue #205) -- it has no Collection endpoint to batch-refresh
through (see enumerate_subdevices' fallback), so each canonical
href confirmed at enumeration time gets its own GET under the
subdevice's prefix. A href failing to answer this cycle just drops
out of the result, same "never let a sibling's flakiness fail the
master's poll" posture as the Collection path above."""
sess = self._session
master's poll" posture as the Collection path above.
Takes `sess` from the caller (already None-checked there) rather
than re-reading self._session -- async_close() can null that
without holding _session_lock, and pace()/get() both need a live
session on every iteration, not just the first.
Skips any href already covered by the hot/warm sub-poll tiers
(self._hot_hrefs/_warm_hrefs, in the same actual/on-the-wire form
this method builds) -- those are already refreshed every 3s/6s by
_run_subpolls, strictly more current than this once-per-summary-poll
pass could offer, so re-fetching them here would only add GETs
without adding freshness. A subdevice with many confirmed hrefs
(unlike a Collection batch, which is always one GET regardless of
count) is otherwise a summary-poll cost that scales with its href
count."""
skip = set(self._hot_hrefs) | set(self._warm_hrefs)
result: dict[str, dict] = {}
first = True
for href in subdevice.flat_hrefs:
if not first:
sess.pace()
first = False
actual = subdevice.to_actual(href)
if actual in skip:
continue
try:
if not first:
sess.pace()
first = False
path = [s for s in actual.strip('/').split('/')]
code, payload = sess.get(path, timeout=10.0)
if code == 0x45 and payload:
@@ -18,17 +18,22 @@ Pattern B -- UUID-prefixed tree (`TP2X_FAC_BORA_21K`, jhkwon19's board).
`/oic/res` hides the whole appliance tree; `/device/0`'s batch instead
carries `x.com.samsung.da.subdeviceIdList` on `/subdevices/vs/0`, and that
same UUID appears as a literal href prefix in `/oic/res`
(`/<uuid>/file/list/vs/0`, ...). On jhkwon19's own first unit, `GET
/<uuid>/device/0` returned the second subdevice's own Collection batch,
confirmed live to carry a different model/serial than the master
(`TP2X_FAC_BORA_RAC_21K`, the wall-mounted subdevice, vs. the master's
`TP2X_FAC_BORA_21K`, the floor subdevice) -- but issue #205, a second
TP2X_FAC_BORA_21K unit, showed that same `/<uuid>/device/0` probe coming
back empty, so it isn't a property of the board family, only of the
individual unit/firmware. When it's empty, `enumerate_subdevices` falls back
to probing every href the master itself answered this cycle individually
under the UUID prefix, on the assumption that a composite device's siblings
share the master's resource surface -- see `Subdevice.flat_hrefs`.
(`/<uuid>/file/list/vs/0`, ...). What's actually been confirmed live on
jhkwon19's unit is narrower than early issue #177 writeups suggested: a
single individual `GET /<uuid>/information/vs/0` was read by hand through
the debug panel and came back carrying a different model/serial than the
master (`TP2X_FAC_BORA_RAC_21K`, the wall-mounted subdevice, vs. the
master's `TP2X_FAC_BORA_21K`, the floor subdevice) -- real evidence a
second subdevice exists at that prefix, but not evidence that `GET
/<uuid>/device/0` (the Collection batch PR #199 built this pattern's seed
around) itself returns anything. Issue #205, the same unit on a later
version, is that assumption failing: `/<uuid>/device/0` comes back empty.
So `enumerate_subdevices` tries it first (a future board might genuinely
expose it) and falls back, when it's empty, to probing every href the
master itself answered this cycle individually under the UUID prefix --
the only thing ever actually confirmed to work for this pattern -- on the
assumption that a composite device's siblings share the master's resource
surface. See `Subdevice.flat_hrefs`.
Both are "the same thing wearing different clothes": a logical subdevice is a
seed collection path to poll, plus an href transform between the canonical
@@ -351,6 +356,17 @@ def enumerate_subdevices(
# this UUID's prefix, and keep whichever ones answer. Each is a
# plain tolerated-404 RETRIEVE, same posture as every other probe in
# this function.
#
# Known gap, not yet guarded against: a firmware that answers *any*
# request under an unrecognized prefix (echoing the master's own
# state back rather than 4.04ing) would pass every one of these
# probes and, if the echoed state also clears discover_partitioned's
# liveness gate, materialize a phantom duplicate of the master
# rather than a real sibling. Every board seen so far genuinely
# 4.04s on paths it doesn't own (issue #205's own unit answered only
# 1 of 31 probes), so this hasn't been built -- the one place it
# could hook in later is comparing a candidate's confirmed reps
# against the master's own values for those same canonical hrefs.
flat_hrefs = []
first = True
for href in sorted(resources):
@@ -716,5 +716,5 @@
"x.com.samsung.da.serialNum": "TEST-SUBDEVICE-SERIAL-0000"
}
},
"seeds_note": "device0 and oic_res are real, captured from jhkwon19's issue #205 report. subdeviceIdList in device0's /subdevices/vs/0 is restored to the real ['6c2dff6d-ee5c-dad1-6a5e-000000000001'] (HA's diagnostics download redacts it, matching redact.py's 'deviceid' substring rule, not because it's account data -- same restoration airconditioner_fac_bora_2in1_device.json documents for the same field), and every other value is otherwise exactly what HA's download redacts (serials/otnDUID etc.) against the same physical TP2X_FAC_BORA_21K unit as airconditioner_fac_bora_2in1_device.json -- same subdeviceIdList UUID, same wall subdevice. Filed specifically because, contrary to the assumption that fixture's seed batch was built on, this unit's /6c2dff6d-ee5c-dad1-6a5e-000000000001/device/0 does NOT answer (subdevice_probes in the report shows it False), so the Collection-batch fallback this fixture exercises (registry.subdevices.enumerate_subdevices' per-href probe, issue #205) is what has to find the subdevice instead. The one probes entry, /6c2dff6d-ee5c-dad1-6a5e-000000000001/information/vs/0, is the same real capture already used in airconditioner_fac_bora_2in1_device.json's seed batch (issue #177 comment 5113518087) -- the only href ever actually confirmed to answer under this UUID prefix. No other /6c2dff6d-ee5c-dad1-6a5e-000000000001/* href has been confirmed live yet, so none are seeded here; this fixture's expected outcome is the candidate being found by the flat-href probe but then correctly held back by discover_partitioned's liveness gate (information alone binds no entity), matching where the real issue stands -- not a materialized climate entity, which would require guessing at unconfirmed hrefs."
"seeds_note": "device0 and oic_res are real, captured from jhkwon19's issue #205 report. subdeviceIdList in device0's /subdevices/vs/0 is restored to the real ['6c2dff6d-ee5c-dad1-6a5e-000000000001'] (HA's diagnostics download redacts it, matching redact.py's 'deviceid' substring rule, not because it's account data -- same restoration airconditioner_fac_bora_2in1_device.json documents for the same field), and every other value is otherwise exactly what HA's download redacts (serials/otnDUID etc.) against the same physical TP2X_FAC_BORA_21K unit as airconditioner_fac_bora_2in1_device.json -- same subdeviceIdList UUID, same wall subdevice. Filed specifically because, contrary to the assumption that fixture's seed batch was built on, this unit's /6c2dff6d-ee5c-dad1-6a5e-000000000001/device/0 does NOT answer (subdevice_probes in the report shows it False), so the Collection-batch fallback this fixture exercises (registry.subdevices.enumerate_subdevices' per-href probe, issue #205) is what has to find the subdevice instead. The one probes entry, /6c2dff6d-ee5c-dad1-6a5e-000000000001/information/vs/0, is the same real capture already used in airconditioner_fac_bora_2in1_device.json's seed batch (issue #177 comment 5113518087) -- the only href ever actually confirmed to answer under this UUID prefix. No other /6c2dff6d-ee5c-dad1-6a5e-000000000001/* href has been confirmed live yet, so none are seeded here; this fixture's expected outcome is the candidate being found by the flat-href probe but then correctly held back by discover_partitioned's liveness gate (information alone binds no entity), matching where the real issue stands -- not a materialized climate entity, which would require guessing at unconfirmed hrefs. The probe's serialNum (\"TEST-SUBDEVICE-SERIAL-0000\") is a hand-placed placeholder for the real value, not HA's own redaction output -- same placeholder airconditioner_fac_bora_2in1_device.json uses for the identical field."
}
@@ -0,0 +1,20 @@
{
"state_keys": [
"air_filter_status",
"air_filter_threshold",
"air_filter_usage",
"air_filter_usage_hours",
"alarm_code",
"auto_clean",
"beep",
"climate",
"current_temperature_c",
"diagnosis_status",
"energy_kwh",
"firmware_update",
"humidity",
"power_energy_kwh",
"power_watts",
"tropical_night_mode"
]
}
+24
View File
@@ -961,6 +961,30 @@ def test_registry_reproduces_golden_state_keys_for_airconditioner_fac_bora_2in1(
)
def test_registry_reproduces_golden_state_keys_for_airconditioner_fac_bora_205_flat():
"""jhkwon19's same physical TP2X_FAC_BORA_21K unit as the _2in1 fixture
above, but a later capture (issue #205) where /<uuid>/device/0 doesn't
answer -- contrary to what that fixture's own seed batch assumed the
Collection endpoint would do. device0/oic_res are real; the only
UUID-prefixed data is the one href ever actually confirmed live
(/information/vs/0, same real capture the _2in1 fixture uses), fed
through registry.subdevices.enumerate_subdevices' per-href flat
fallback instead of a Collection batch. /information/vs/0 alone binds
no entity, so the candidate is found but never materializes -- this
golden has no `subdevice_...`-prefixed keys at all, same shape as
tests/fixtures/golden/airconditioner_fac_bora.json, which is the point:
a device whose sibling can't yet be confirmed live must regress to
exactly the master-only state, never a phantom or partial subdevice."""
name = 'airconditioner_fac_bora_205_flat'
golden = json.loads((GOLDEN / f'{name}.json').read_text())
state_keys = _new_subdevice_aware_state_keys(name)
assert set(state_keys) == set(golden['state_keys']), (
f"state_keys mismatch:\n"
f" extra: {sorted(set(state_keys) - set(golden['state_keys']))}\n"
f" missing: {sorted(set(golden['state_keys']) - set(state_keys))}"
)
def test_registry_reproduces_golden_state_keys_for_air_purifier_avt_ww():
"""AVT-WW-TP1-23-AXX500 (issue #190) -- next-gen BESPOKE Cube Air board;
reports device_type 'unknown' with empty oneUiVersion because 'VTWW' as a
+124 -5
View File
@@ -36,12 +36,14 @@ def _coordinator(hass: HomeAssistant) -> LocalThingsCoordinator:
return LocalThingsCoordinator(hass, entry)
async def _discover(coordinator: LocalThingsCoordinator, name: str) -> None:
async def _discover_with(
coordinator: LocalThingsCoordinator, resources: dict, oic_res, seeds: dict,
) -> None:
"""Run the same two-step sequence _async_update_data's first cycle does
(enumerate, then discover) against fixture data, without the polling/
reconnect machinery around it -- see coordinator.py's
_enumerate_subdevices_blocking/_run_discovery."""
resources, oic_res, seeds = _load_device_full(name)
(enumerate, then discover) against arbitrary resources/oic_res/seeds,
without the polling/reconnect machinery around it -- see coordinator.py's
_enumerate_subdevices_blocking/_run_discovery. `_discover` below is the
fixture-file-backed convenience wrapper most tests want."""
coordinator._session = FakeCoapSession(seeds)
# _connect_session (skipped here -- the session is pre-set) is what
# normally populates _identity via read_identity; set it directly with
@@ -67,6 +69,12 @@ async def _discover(coordinator: LocalThingsCoordinator, name: str) -> None:
coordinator._observe.apply(href, rep, source='poll')
async def _discover(coordinator: LocalThingsCoordinator, name: str) -> None:
"""Fixture-file-backed convenience wrapper around _discover_with."""
resources, oic_res, seeds = _load_device_full(name)
await _discover_with(coordinator, resources, oic_res, seeds)
def _climate_bound(coordinator, subdevice_key: str):
from custom_components.localthings.registry.subdevices import MAIN
for b in coordinator.bound:
@@ -220,6 +228,90 @@ async def test_fac_bora_205_flat_fallback_finds_candidate_but_gate_holds_it_back
assert _climate_bound(coordinator, None) is not None
async def test_flat_subdevice_materializes_and_repolls_end_to_end(hass: HomeAssistant):
"""Synthetic (not a real capture, unlike the fixture-driven test above) --
exercises the one path nothing else covers: a flat-mode prefixed
subdevice with *enough* confirmed hrefs to actually pass
discover_partitioned's liveness gate and materialize a real climate
entity, then a subsequent _poll_subdevice_seed re-poll refreshing its
state all the way through to canonical_resources -- the path a real
resolution of issue #205 (once more hrefs are confirmed live for some
unit) would actually need.
Also pins _poll_subdevice_flat_hrefs' hot/warm skip: climate-critical
hrefs (power/mode/temperature) land on the warm tier by discovery's own
rules, so they're already kept fresh every few seconds by _run_subpolls
-- re-fetching them again on this once-per-summary-poll pass would only
add GETs, not freshness, so they're the ones this method must skip.
/option/autoclean/vs/0 is cold-tier and is what actually needs this
path."""
resources, oic_res, _real_seeds = _load_device_full('airconditioner_fac_bora_2in1')
seeds = {
# No (_SUB_UUID, 'device', '0') entry -- forces the flat fallback,
# same as the real issue #205 capture above, but this time with
# enough hrefs answering to actually materialize. power/mode/
# temperature values copied verbatim from that fixture's own (real)
# Collection-batch seed, just served individually instead of
# batched, to isolate "does flat mode produce the same result as
# Collection mode" as the only variable.
f'/{_SUB_UUID}/power/vs/0': {'x.com.samsung.da.power': 'On'},
f'/{_SUB_UUID}/mode/vs/0': {
'x.com.samsung.da.supportedModes': ['Cool', 'Dry', 'Wind', 'Auto'],
'x.com.samsung.da.modes': ['Cool'],
'x.com.samsung.da.options': [],
},
f'/{_SUB_UUID}/temperature/current/0': {
'range': [18.0, 30.0], 'units': 'C', 'temperature': 26.0,
},
f'/{_SUB_UUID}/temperature/desired/0': {
'range': [18.0, 30.0], 'units': 'C', 'temperature': 24.0,
},
# Cold-tier -- not covered by _run_subpolls, so this is the href
# that actually depends on _poll_subdevice_flat_hrefs to ever
# refresh at all.
f'/{_SUB_UUID}/option/autoclean/vs/0': {
'x.com.samsung.da.settingStatus': 'Off',
},
}
coordinator = _coordinator(hass)
await _discover_with(coordinator, resources, oic_res, seeds)
assert [su.key for su in coordinator.subdevices] == [_SUB_UUID]
subdevice = coordinator.subdevices[0]
assert subdevice.seed_path == ()
assert subdevice.flat_hrefs != ()
sub_climate = _climate_bound(coordinator, _SUB_UUID)
assert sub_climate is not None
# Re-poll: a fresh reading under the prefix should reach
# canonical_resources through _poll_subdevice_seed's flat-mode branch,
# not just sit frozen at the one-time enumeration snapshot.
coordinator._session.seeds[f'/{_SUB_UUID}/temperature/current/0'] = {
'range': [18.0, 30.0], 'units': 'C', 'temperature': 27.5,
}
coordinator._session.seeds[f'/{_SUB_UUID}/option/autoclean/vs/0'] = {
'x.com.samsung.da.settingStatus': 'On',
}
refreshed = coordinator._poll_subdevice_seed(subdevice)
# The warm-tier temperature href is skipped here -- already covered by
# _run_subpolls at a faster cadence -- so it does NOT show up refreshed
# through this path.
assert f'/{_SUB_UUID}/temperature/current/0' not in refreshed
assert refreshed == {
f'/{_SUB_UUID}/option/autoclean/vs/0': {'x.com.samsung.da.settingStatus': 'On'},
}
for href, rep in refreshed.items():
coordinator._observe.apply(href, rep, source='poll')
res = coordinator.canonical_resources(subdevice)
assert res['/option/autoclean/vs/0']['x.com.samsung.da.settingStatus'] == 'On'
# Confirms the skip is about redundant re-fetching, not stale data --
# the warm-tier value from initial discovery is still there, untouched.
assert res['/temperature/current/0']['temperature'] == 26.0
class _FakeCollectionSession:
"""Minimal session that only ever answers a Collection GET -- used to
prove the flat-mode re-poll path (issue #205) is only taken when
@@ -295,6 +387,33 @@ def test_poll_subdevice_seed_flat_mode_polls_each_href_individually(
]
def test_poll_subdevice_seed_flat_mode_skips_hrefs_covered_by_hot_warm_subpolls(
hass: HomeAssistant,
):
"""A flat href already in the hot/warm sub-poll tiers is refreshed every
few seconds by _run_subpolls -- re-fetching it again on this
once-per-summary-poll pass would only add GETs, not freshness, so
_poll_subdevice_flat_hrefs must not even attempt it."""
from custom_components.localthings.registry.subdevices import Subdevice
coordinator = _coordinator(hass)
sess = _FakeCollectionSession({
(_SUB_UUID, 'mode', 'vs', '0'): {'mode': 'cool'},
(_SUB_UUID, 'power', 'vs', '0'): {'power': 'On'},
})
coordinator._session = sess
coordinator._warm_hrefs = [f'/{_SUB_UUID}/mode/vs/0']
subdevice = Subdevice(
kind='prefixed', key=_SUB_UUID, seed_path=(),
flat_hrefs=('/mode/vs/0', '/power/vs/0'),
)
result = coordinator._poll_subdevice_seed(subdevice)
assert result == {f'/{_SUB_UUID}/power/vs/0': {'power': 'On'}}
assert sess.calls == [(_SUB_UUID, 'power', 'vs', '0')]
async def test_multidevice_probe_never_reaches_discovery_or_the_cache(
hass: HomeAssistant,
):
+45
View File
@@ -318,6 +318,51 @@ def test_enumerate_prefixed_flat_fallback_probe_log_reports_every_href_tried():
assert probes[f'/{_UUID}/mode/vs/0'] is True
def test_enumerate_prefixed_flat_fallback_with_no_master_hrefs_to_probe_is_a_no_op():
"""The master itself having nothing but /subdevices/vs/0 in its own
resources this cycle (e.g. a very first, mostly-empty poll) must not
crash the fallback loop -- the only href in `resources` is
/subdevices/vs/0 itself, which the fake session doesn't answer under
the prefix either, so nothing materializes."""
resources = {
'/subdevices/vs/0': {'x.com.samsung.da.subdeviceIdList': [_UUID]},
}
subdevices, extra = enumerate_subdevices(_FakeSession({}), resources, oic_res_links=[])
assert subdevices == []
assert extra == {}
def test_enumerate_prefixed_flat_fallback_does_not_cross_contaminate_a_second_uuid():
"""Two prefixed candidates in the same subdeviceIdList, one whose
Collection endpoint works and one that needs the flat fallback -- each
must end up with only its own hrefs, no bleed between them."""
uuid_a, uuid_b = _UUID, '11111111-1111-1111-1111-111111111111'
resources = {
'/subdevices/vs/0': {'x.com.samsung.da.subdeviceIdList': [uuid_a, uuid_b]},
'/mode/vs/0': {'m': 'cool'},
}
sess = _FakeSession({
(uuid_a, 'device', '0'): [
_DEVCOL_REP, {'href': '/mode/vs/0', 'rep': {'m': 'a-collection'}},
],
# uuid_b's Collection deliberately absent -> falls back to the flat
# per-href probe.
(uuid_b, 'mode', 'vs', '0'): {'m': 'b-flat'},
})
subdevices, extra = enumerate_subdevices(sess, resources, oic_res_links=[])
by_key = {u.key: u for u in subdevices}
assert set(by_key) == {uuid_a, uuid_b}
assert by_key[uuid_a].seed_path == (uuid_a, 'device', '0')
assert by_key[uuid_a].flat_hrefs == ()
assert by_key[uuid_b].seed_path == ()
assert by_key[uuid_b].flat_hrefs == ('/mode/vs/0',)
assert extra == {
f'/{uuid_a}/mode/vs/0': {'m': 'a-collection'},
f'/{uuid_b}/mode/vs/0': {'m': 'b-flat'},
}
def test_enumerate_prefixed_tolerates_redacted_string_id_list():
"""subdeviceIdList matches redact.py's 'deviceid' substring rule, and the
real airconditioner_fac_bora_device.json fixture carries the literal