mirror of
https://github.com/Matysh/houseplan-card
synced 2026-07-31 16:38:31 +00:00
Trail recorder: zero map ids, one source per N markers, serialized refresh
Audit HP-1540-02: the recorder chose the map id with an or-chain, so a valid numeric map_index=0 fell through to selected_map (or 'default') and the server stored the run under a key the renderer never looks up. The choice is now resolve_map_id() — the explicit backend half of the contract shared with vacMapIdFromAttrs in src/vacuum.ts: the first value that is not None wins, zero and empty string included. HP-1540-03: pairs was a plain source -> (marker, vacuum) dict, so the second placement of the same robot (the documented two-floor case) evicted the first and its server history silently stopped. A source now maps to a list of pairs, every marker gets its own copy of the run, and the state subscription set is deduplicated. HP-1540-05: every config/set spawns async_refresh as a detached task; two of them interleaving across the awaited config load could both subscribe, overwriting one unsub handle — a callback leak until HA restart. Refresh is serialized with an asyncio.Lock and teardown flags the recorder closed first, so a refresh parked on its await can never resubscribe afterwards. Regressions cover map_index 0/'0'/''/selected_map cross-checks, one source feeding two floor markers across a map switch, pair-list refresh with a deduplicated entity set, and two overlapping refreshes leaving exactly one live subscription (zero after teardown). On the v1.54.0 recorder 11 of these tests fail.
This commit is contained in:
Matysh
@@ -9,6 +9,7 @@ want to see where the cleanup has already been).
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
import asyncio
|
||||
import time
|
||||
from typing import Any
|
||||
|
||||
@@ -28,6 +29,28 @@ FIRE_THROTTLE_S = 2.0 # event-bus updates for live cards
|
||||
MOVING_STATES = {"cleaning", "returning", "on"}
|
||||
|
||||
|
||||
def resolve_map_id(src_attrs: Any, vac_attrs: Any) -> str:
|
||||
"""Map-id normalisation contract, shared with the frontend.
|
||||
|
||||
Mirrors src/vacuum.ts vacMapIdFromAttrs (source attrs, `??`-chain) plus the
|
||||
card's _vacMapId fallback to the vacuum entity's selected_map. The FIRST
|
||||
value that is not None wins — truthiness is wrong here: a zero-based
|
||||
`map_index: 0` is a valid first map and an empty string is still an id.
|
||||
The old `or`-chain dropped the zero, so the server stored trails under a
|
||||
key the renderer never looked up (HP-1540-02).
|
||||
"""
|
||||
for v in (
|
||||
src_attrs.get("map_name"),
|
||||
src_attrs.get("current_map"),
|
||||
src_attrs.get("map_index"),
|
||||
src_attrs.get("selected_map"),
|
||||
vac_attrs.get("selected_map"),
|
||||
):
|
||||
if v is not None:
|
||||
return str(v)
|
||||
return "default"
|
||||
|
||||
|
||||
class TrailBook:
|
||||
"""Pure run bookkeeping: {marker: {current: run, previous: run}}.
|
||||
|
||||
@@ -76,44 +99,69 @@ class TrailRecorder:
|
||||
self.rt = rt
|
||||
self.store = Store(hass, 1, f"{DOMAIN}.trails")
|
||||
self.book = TrailBook()
|
||||
self.pairs: dict[str, tuple[str, str]] = {} # source → (marker, vacuum)
|
||||
# HP-1540-03: one source may feed SEVERAL markers — the same robot
|
||||
# placed on two floors is the documented multi-floor case, and a plain
|
||||
# source → (marker, vacuum) dict silently kept only the last one
|
||||
self.pairs: dict[str, list[tuple[str, str]]] = {} # source → [(marker, vacuum), …]
|
||||
self._unsub_track = None
|
||||
self._unsub_save = None
|
||||
self._last_fire = 0.0
|
||||
# HP-1540-05: config/set fires refresh as a detached task; two of them
|
||||
# interleaving across the awaited load both subscribed and the loser's
|
||||
# unsub handle was overwritten — a leak until HA restart
|
||||
self._refresh_lock = asyncio.Lock()
|
||||
self._closed = False
|
||||
|
||||
async def async_setup(self) -> None:
|
||||
self.book = TrailBook(await self.store.async_load() or {})
|
||||
await self.async_refresh()
|
||||
|
||||
async def async_refresh(self) -> None:
|
||||
"""(Re)subscribe after any config change — markers may come and go."""
|
||||
if self._unsub_track:
|
||||
self._unsub_track()
|
||||
self._unsub_track = None
|
||||
stored = await self.rt.config_store.async_load() or {}
|
||||
cfg = stored.get("config") or {}
|
||||
self.pairs = {}
|
||||
for m in cfg.get("markers") or []:
|
||||
v = m.get("vacuum") or {}
|
||||
src = v.get("source")
|
||||
if not src or v.get("live") is False:
|
||||
continue
|
||||
vac = self._vacuum_entity(m)
|
||||
if vac:
|
||||
self.pairs[src] = (str(m.get("id")), vac)
|
||||
ents = set(self.pairs) | {vac for _, vac in self.pairs.values()}
|
||||
_LOGGER.info("Trail recorder: tracking %s", sorted(ents))
|
||||
if ents:
|
||||
self._unsub_track = async_track_state_change_event(
|
||||
self.hass, sorted(ents), self._on_state
|
||||
)
|
||||
# A run already in progress (HA restarted mid-cleanup, or the user just
|
||||
# finished calibrating) must start recording NOW, not at the next
|
||||
# state change — otherwise the first seconds of the path are lost.
|
||||
for src in self.pairs:
|
||||
self._sample(src, time.time())
|
||||
"""(Re)subscribe after any config change — markers may come and go.
|
||||
|
||||
Serialised (HP-1540-05): the lock makes unsubscribe-then-resubscribe
|
||||
atomic across the awaited config load, so overlapping refresh tasks can
|
||||
no longer both subscribe and strand one callback forever. The _closed
|
||||
check covers teardown() racing a refresh that is parked on its await.
|
||||
"""
|
||||
async with self._refresh_lock:
|
||||
stored = await self.rt.config_store.async_load() or {}
|
||||
if self._closed:
|
||||
return
|
||||
cfg = stored.get("config") or {}
|
||||
pairs: dict[str, list[tuple[str, str]]] = {}
|
||||
for m in cfg.get("markers") or []:
|
||||
v = m.get("vacuum") or {}
|
||||
src = v.get("source")
|
||||
if not src or v.get("live") is False:
|
||||
continue
|
||||
vac = self._vacuum_entity(m)
|
||||
if vac:
|
||||
# HP-1540-03: append, never overwrite — every floor's
|
||||
# marker records its own copy of the run
|
||||
pairs.setdefault(src, []).append((str(m.get("id")), vac))
|
||||
self.pairs = pairs
|
||||
if self._unsub_track:
|
||||
self._unsub_track()
|
||||
self._unsub_track = None
|
||||
# deduplicated: two markers of one robot share source AND vacuum
|
||||
ents = set(self.pairs) | {vac for ps in self.pairs.values() for _, vac in ps}
|
||||
_LOGGER.info("Trail recorder: tracking %s", sorted(ents))
|
||||
if ents:
|
||||
self._unsub_track = async_track_state_change_event(
|
||||
self.hass, sorted(ents), self._on_state
|
||||
)
|
||||
# A run already in progress (HA restarted mid-cleanup, or the user
|
||||
# just finished calibrating) must start recording NOW, not at the
|
||||
# next state change — otherwise the first seconds of the path are
|
||||
# lost.
|
||||
for src in self.pairs:
|
||||
self._sample(src, time.time())
|
||||
|
||||
def teardown(self) -> None:
|
||||
# HP-1540-05: flag FIRST — a refresh parked on its awaited load must
|
||||
# not re-subscribe after this cleanup has already run
|
||||
self._closed = True
|
||||
if self._unsub_track:
|
||||
self._unsub_track()
|
||||
self._unsub_track = None
|
||||
@@ -133,51 +181,49 @@ class TrailRecorder:
|
||||
return None
|
||||
|
||||
def _sample(self, src: str, now: float) -> bool:
|
||||
"""Record one point (or end the run) for a single source entity."""
|
||||
pair = self.pairs.get(src)
|
||||
if not pair:
|
||||
return False
|
||||
marker, vac = pair
|
||||
st_vac = self.hass.states.get(vac)
|
||||
# "no state yet" is NOT "stopped": during HA boot the vacuum reads
|
||||
# unavailable and ending the run here would split one cleanup into
|
||||
# current+previous on every restart (observed live: 21 points became
|
||||
# previous, the same run restarted at 5)
|
||||
if not st_vac or st_vac.state in ("unavailable", "unknown"):
|
||||
return False
|
||||
if st_vac.state not in MOVING_STATES:
|
||||
return self.book.end_run(marker, now)
|
||||
st_src = self.hass.states.get(src)
|
||||
attrs = st_src.attributes if st_src else {}
|
||||
raw = attrs.get("vacuum_position") or attrs.get("robot_position")
|
||||
# Server-side these attributes are often OBJECTS (Tasshack keeps a
|
||||
# Point dataclass in memory — it only becomes a dict when serialised
|
||||
# to the frontend). Caught live on the owner's X50: the recorder saw
|
||||
# every state change and rejected every single one.
|
||||
if isinstance(raw, dict):
|
||||
px, py = raw.get("x"), raw.get("y")
|
||||
else:
|
||||
px, py = getattr(raw, "x", None), getattr(raw, "y", None)
|
||||
try:
|
||||
x, y = float(px), float(py) # type: ignore[arg-type]
|
||||
except (TypeError, ValueError):
|
||||
return False
|
||||
map_id = str(
|
||||
attrs.get("map_name")
|
||||
or attrs.get("current_map")
|
||||
or attrs.get("map_index")
|
||||
or st_vac.attributes.get("selected_map")
|
||||
or "default"
|
||||
)
|
||||
return self.book.on_point(marker, map_id, x, y, now)
|
||||
"""Record one point (or end the run) for EVERY marker fed by src.
|
||||
|
||||
HP-1540-03: the same source serves one marker per floor — all of them
|
||||
must receive the point, not just whichever survived the dict.
|
||||
"""
|
||||
changed = False
|
||||
for marker, vac in self.pairs.get(src) or ():
|
||||
st_vac = self.hass.states.get(vac)
|
||||
# "no state yet" is NOT "stopped": during HA boot the vacuum reads
|
||||
# unavailable and ending the run here would split one cleanup into
|
||||
# current+previous on every restart (observed live: 21 points
|
||||
# became previous, the same run restarted at 5)
|
||||
if not st_vac or st_vac.state in ("unavailable", "unknown"):
|
||||
continue
|
||||
if st_vac.state not in MOVING_STATES:
|
||||
changed |= self.book.end_run(marker, now)
|
||||
continue
|
||||
st_src = self.hass.states.get(src)
|
||||
attrs = st_src.attributes if st_src else {}
|
||||
raw = attrs.get("vacuum_position") or attrs.get("robot_position")
|
||||
# Server-side these attributes are often OBJECTS (Tasshack keeps a
|
||||
# Point dataclass in memory — it only becomes a dict when
|
||||
# serialised to the frontend). Caught live on the owner's X50: the
|
||||
# recorder saw every state change and rejected every single one.
|
||||
if isinstance(raw, dict):
|
||||
px, py = raw.get("x"), raw.get("y")
|
||||
else:
|
||||
px, py = getattr(raw, "x", None), getattr(raw, "y", None)
|
||||
try:
|
||||
x, y = float(px), float(py) # type: ignore[arg-type]
|
||||
except (TypeError, ValueError):
|
||||
continue
|
||||
map_id = resolve_map_id(attrs, st_vac.attributes)
|
||||
changed |= self.book.on_point(marker, map_id, x, y, now)
|
||||
return changed
|
||||
|
||||
@callback
|
||||
def _on_state(self, event: Any) -> None:
|
||||
eid = event.data.get("entity_id")
|
||||
now = time.time()
|
||||
changed = False
|
||||
for src, (_marker, vac) in self.pairs.items():
|
||||
if eid in (src, vac):
|
||||
for src, pair_list in self.pairs.items():
|
||||
if eid == src or any(eid == vac for _, vac in pair_list):
|
||||
changed |= self._sample(src, now)
|
||||
if changed:
|
||||
self._schedule_save()
|
||||
|
||||
@@ -85,7 +85,7 @@ def _rec():
|
||||
}
|
||||
hass = Hass(states)
|
||||
rec = trails.TrailRecorder(hass, None)
|
||||
rec.pairs = {"camera.map": ("m1", "vacuum.x50")}
|
||||
rec.pairs = {"camera.map": [("m1", "vacuum.x50")]}
|
||||
return rec, hass, states
|
||||
|
||||
|
||||
@@ -152,3 +152,151 @@ def test_unavailable_vacuum_is_no_verdict():
|
||||
del states["vacuum.x50"]
|
||||
assert not rec._sample("camera.map", 3.0)
|
||||
assert rec.book.data["m1"]["current"]["ended"] is None
|
||||
|
||||
|
||||
# ---------------- v1.54.0 audit regressions ----------------
|
||||
|
||||
|
||||
def test_map_index_zero_matches_frontend_contract():
|
||||
# HP-1540-02: `map_index: 0` is a VALID first map. The old or-chain
|
||||
# dropped it and fell through to selected_map — the server stored the
|
||||
# run under a key the renderer never looked up.
|
||||
rec, _hass, states = _rec()
|
||||
states["camera.map"] = S("idle", {"vacuum_position": {"x": 10, "y": 20}, "map_index": 0})
|
||||
assert rec._sample("camera.map", 1.0)
|
||||
assert rec.book.data["m1"]["current"]["map_id"] == "0"
|
||||
|
||||
|
||||
def test_map_id_contract_first_not_none_wins():
|
||||
# HP-1540-02: the shared contract — first NOT-None value, stringified
|
||||
cases = [
|
||||
({"map_index": 0}, {"selected_map": "Floor"}, "0"),
|
||||
({"map_index": "0"}, {}, "0"),
|
||||
({"map_name": ""}, {"selected_map": "Floor"}, ""),
|
||||
({"map_name": "A", "map_index": 0}, {}, "A"),
|
||||
({"current_map": 2}, {}, "2"),
|
||||
({"selected_map": "Src"}, {"selected_map": "Vac"}, "Src"),
|
||||
({}, {"selected_map": "Vac"}, "Vac"),
|
||||
({}, {}, "default"),
|
||||
]
|
||||
for src_attrs, vac_attrs, want in cases:
|
||||
assert trails.resolve_map_id(src_attrs, vac_attrs) == want, (src_attrs, vac_attrs)
|
||||
|
||||
|
||||
def test_one_source_two_floor_markers_both_record():
|
||||
# HP-1540-03: the multi-floor case — the same robot placed on two floors.
|
||||
# Both markers must receive the server-side run, on every map.
|
||||
states = {
|
||||
"vacuum.x50": S("cleaning", {}),
|
||||
"camera.map": S("idle", {"vacuum_position": {"x": 100, "y": 200}, "map_index": 0}),
|
||||
}
|
||||
hass = Hass(states)
|
||||
rec = trails.TrailRecorder(hass, None)
|
||||
rec.pairs = {"camera.map": [("m_floor1", "vacuum.x50"), ("m_floor2", "vacuum.x50")]}
|
||||
rec._on_state(E("camera.map"))
|
||||
# the robot moves to the second map: BOTH books rotate to the new run
|
||||
states["camera.map"] = S("idle", {"vacuum_position": {"x": 300, "y": 400}, "map_index": 1})
|
||||
rec._on_state(E("camera.map"))
|
||||
for marker in ("m_floor1", "m_floor2"):
|
||||
book = rec.book.data[marker]
|
||||
assert book["previous"]["map_id"] == "0", marker
|
||||
assert book["previous"]["points"] == [[100.0, 200.0]], marker
|
||||
assert book["current"]["map_id"] == "1", marker
|
||||
assert book["current"]["points"] == [[300.0, 400.0]], marker
|
||||
|
||||
|
||||
def test_refresh_builds_pair_lists_and_dedups_subscription():
|
||||
# HP-1540-03: two markers over one source/vacuum → one entity set, both pairs
|
||||
import asyncio
|
||||
|
||||
tracked = []
|
||||
|
||||
def track(hass, ents, cb):
|
||||
tracked.append(list(ents))
|
||||
return lambda: None
|
||||
|
||||
old_track = trails.async_track_state_change_event
|
||||
trails.async_track_state_change_event = track
|
||||
try:
|
||||
cfgm = [
|
||||
{"id": "m_f1", "binding": "entity:vacuum.x50", "vacuum": {"source": "camera.map"}},
|
||||
{"id": "m_f2", "binding": "entity:vacuum.x50", "vacuum": {"source": "camera.map"}},
|
||||
]
|
||||
|
||||
class CS:
|
||||
async def async_load(self):
|
||||
return {"config": {"markers": cfgm}}
|
||||
|
||||
class RT:
|
||||
config_store = CS()
|
||||
|
||||
hass = Hass({
|
||||
"vacuum.x50": S("docked", {}),
|
||||
"camera.map": S("idle", {}),
|
||||
})
|
||||
rec = trails.TrailRecorder(hass, RT())
|
||||
asyncio.run(rec.async_refresh())
|
||||
assert rec.pairs == {"camera.map": [("m_f1", "vacuum.x50"), ("m_f2", "vacuum.x50")]}
|
||||
assert tracked == [["camera.map", "vacuum.x50"]]
|
||||
finally:
|
||||
trails.async_track_state_change_event = old_track
|
||||
|
||||
|
||||
def test_overlapping_refreshes_leave_one_subscription_teardown_zero():
|
||||
# HP-1540-05: two config/set refreshes racing across the awaited load used
|
||||
# to BOTH subscribe; teardown removed only the last handle and the other
|
||||
# callback leaked until HA restart.
|
||||
import asyncio
|
||||
|
||||
active = []
|
||||
seq = {"n": 0}
|
||||
|
||||
def track(hass, ents, cb):
|
||||
seq["n"] += 1
|
||||
hid = seq["n"]
|
||||
active.append(hid)
|
||||
return lambda: active.remove(hid)
|
||||
|
||||
old_track = trails.async_track_state_change_event
|
||||
trails.async_track_state_change_event = track
|
||||
try:
|
||||
gate = asyncio.Event()
|
||||
|
||||
class CS:
|
||||
async def async_load(self):
|
||||
await gate.wait()
|
||||
return {"config": {"markers": [
|
||||
{"id": "m1", "binding": "entity:vacuum.x50", "vacuum": {"source": "camera.map"}},
|
||||
]}}
|
||||
|
||||
class RT:
|
||||
config_store = CS()
|
||||
|
||||
hass = Hass({
|
||||
"vacuum.x50": S("docked", {}),
|
||||
"camera.map": S("idle", {}),
|
||||
})
|
||||
|
||||
async def scenario():
|
||||
rec = trails.TrailRecorder(hass, RT())
|
||||
t1 = asyncio.ensure_future(rec.async_refresh())
|
||||
t2 = asyncio.ensure_future(rec.async_refresh())
|
||||
for _ in range(3): # both tasks are launched; one parks on the gate
|
||||
await asyncio.sleep(0)
|
||||
gate.set()
|
||||
await t1
|
||||
await t2
|
||||
assert len(active) == 1, f"exactly one live subscription, got {active}"
|
||||
# teardown during an in-flight refresh must also end with zero
|
||||
gate.clear()
|
||||
t3 = asyncio.ensure_future(rec.async_refresh())
|
||||
for _ in range(3):
|
||||
await asyncio.sleep(0)
|
||||
rec.teardown()
|
||||
gate.set()
|
||||
await t3
|
||||
assert active == [], f"teardown must leave zero subscriptions, got {active}"
|
||||
|
||||
asyncio.run(scenario())
|
||||
finally:
|
||||
trails.async_track_state_change_event = old_track
|
||||
|
||||
Reference in New Issue
Block a user