docs: review document for #335

Issue: #335
User-Visible: no

docs/reviews/SPEC-REVIEW-335-r1.md

@@ -0,0 +1,90 @@
+# SPEC-REVIEW-335-r1
+
+Issue: #335 «Трейлы пылесосов: осиротевшие маркеры навсегда остаются в store, точки после рестарта не сохраняются»
+Этап: ТЗ на ревью (PROCESS.md §2.4), лёгкий трек (`small`)
+Заход: r1 · блокирующих циклов израсходовано 0/2 (лимит для лёгкого трека — 2, §2.4/§2.10)
+Материал: тело issue #335 на момент ревью (единственная правка — стартовый комментарий аналитика, без изменения ТЗ), репозиторий на `dev` @ `79aa0a90`. Ветки `issue/335-*` не существует — код ещё не написан, ревью строго ТЗ-уровня.
+
+## Скоуп ревью
+
+ТЗ живёт в теле issue (лёгкий трек, §5): проблема · аналитика и границы · контракт реализации · AC1–AC4 · откат · вне скоупа. Проверялось:
+
+1. соответствие докладу о текущем поведении реальному коду (`custom_components/houseplan/trails.py`, `websocket_api.py`, `src/devices.ts`, `src/houseplan-editor-runtime.ts`);
+2. согласованность с каноническим `docs/VACUUM.md` («Storage and lifecycle»);
+3. однозначность и доказуемость каждого AC;
+4. попадание задачи в `docs/SCOPE.md` (job J6 — «Keep the plan true as the home evolves»: трейлы — часть серверного состояния плана, задача чинит его корректность после редактирования — в скоупе);
+5. отсутствие догадок, выданных за факт.
+
+## Как проверялось
+
+Чтение исходников без исполнения (ревью ТЗ — код для задачи не существует):
+
+- `custom_components/houseplan/trails.py` полностью (`TrailBook`, `TrailRecorder.async_refresh/_sample/_on_state/async_delete`);
+- `custom_components/houseplan/websocket_api.py`: `ws_config_set` (создание/пропуск `_refresh_trail_recorder`, ветка no-op, ветка ошибки), `_refresh_trail_recorder`, `ws_trail_delete`, import/recovery cleanup (`live_marker_ids` + `async_delete` в цикле);
+- `custom_components/houseplan/validation.py` (`MARKER_SCHEMA`, комментарий про tombstone), `import_export.py` (`live_layout`, разделение `removed`/`live` id);
+- `src/devices.ts` (`deletePlanMarkerRecords`), `src/houseplan-editor-runtime.ts` (обработчик удаления маркера, вызов `houseplan/trail/delete`);
+- `tests_backend/test_trail_recorder.py` (существующий тест `test_refresh_never_tracks_a_removed_marker_even_with_stale_vacuum_fields`);
+- `docs/VACUUM.md` целиком, раздел «Storage and lifecycle» — построчно против текста issue.
+
+Гейты не запускались — на этапе ТЗ кода нет, `typecheck`/`test`/`build` не относятся к предмету ревью.
+
+## Находки
+
+### High-1 — определение «маркер существует» игнорирует tombstone `removed: true`, из-за чего AC1 не закрывает сценарий, описанный в «Проблема» №1
+
+**Где:** тело issue #335, раздел «Аналитика и границы», строка:
+> «Маркер существует» определяется так же, как в существующем import-пути: его `id` присутствует в успешно сохранённом `config.markers`. Остальные поля маркера не влияют на очистку.
+
+и раздел «Критерии приёмки», AC1.
+
+**Что не так.** Обычное удаление маркера через редактор НЕ убирает его `id` из `config.markers`. `deletePlanMarkerRecords` (`src/devices.ts:914-934`) для любого не-virtual маркера (все vacuum-маркеры такие — привязка `device:`/`entity:vacuum.*`) добавляет вместо удаления tombstone-запись `{id, binding, removed: true, hidden: true}`, которая остаётся в массиве навсегда — это осознанное поведение, задокументированное дважды:
+
+- комментарий к функции: «Non-virtual deletion leaves a hidden tombstone so an older cached card degrades to a hidden marker instead of visibly resurrecting it»;
+- `custom_components/houseplan/validation.py:1673-1675`: «A binding-level tombstone: not rendered or aggregated, but retained so automatic discovery does not put a deleted device straight back».
+
+Таким образом, для штатного «удалить маркер в редакторе» `id` **не исчезает** из `config.markers` — он остаётся с `removed: true`. По формулировке AC1 («остальные поля маркера не влияют на очистку») такой tombstoned-маркер считается «существующим» вечно, и предложенная server-side очистка при `config/set` для него **никогда не сработает** — ни сразу, ни при любом последующем сохранении.
+
+Это прямо противоречит собственному коду проекта: `TrailRecorder.async_refresh` (`trails.py:169-171`) уже трактует `removed: True` как «маркера нет» для целей живого трекинга (`if m.get("removed") is True: continue`), и это закреплено существующим тестом `test_refresh_never_tracks_a_removed_marker_even_with_stale_vacuum_fields` (`tests_backend/test_trail_recorder.py:530-550`). Тот же водораздел `removed`/не-`removed` — устоявшийся в кодовой базе способ отличать «живой» маркер от «удалённого»: `import_export.py:172-183` (`live_layout`) строит `removed = {id : removed is True}` и `live = {id : removed is not True}` именно для этой цели.
+
+**Как это расходится с реальным поведением, которое чинит issue.** Сегодня штатное удаление маркера в редакторе УЖЕ дёргает отдельный best-effort вызов `houseplan/trail/delete` сразу после `config/set` (`src/houseplan-editor-runtime.ts:7990`, обёрнут в `.catch(() => undefined)`, без повтора) — это ровно то, что документирует `docs/VACUUM.md`: «Deleting a vacuum marker removes its layout and server trails». Формулировка «Проблема» №1 в issue («Удаление маркера через `config/set` сохраняет новый конфиг, но не удаляет его записи… Очистка сейчас выполняется только для полного импорта») верна только в узком смысле «один `config/set` сам по себе не чистит», но не описывает систему целиком: реальный риск не в том, что очистки нет вовсе, а в том, что единственная сегодняшняя очистка — это ничем не подстрахованный клиентский вызов, который тихо проглатывает свою ошибку и не повторяется (закрытая вкладка, обрыв сети, переход со страницы между `config/set` и `trail/delete`). Именно этот сценарий и должен закрывать серверный self-heal — но при определении «id присутствует, поле `removed` не важно» он **не закрывается**, потому что маркер так и остаётся в массиве как tombstone.
+
+**Чем это грозит реализации.** Автор технически МОЖЕТ реализовать AC1 буквально — тест соберёт `config.markers`, из которого маркер целиком исключён (не тombstone, а честный «hard drop»), убедится, что после такого `config/set` трейл вычищен, и AC1 позеленеет. Но это докажет очистку для сценария, который в продукте почти не встречается штатно (ближайший реальный аналог — смена `id` при ребиндинге маркера на другое устройство, `houseplan-editor-runtime.ts:7866-7877`, либо полный импорт/восстановление), а не для «удалил маркер пылесоса в редакторе», то есть не для того случая, который явно назван как Проблема №1.
+
+**Предлагаемая правка (на усмотрение автора, ревьюер технические решения не диктует, но альтернатива нужна для развязки):** определить «маркер существует» как «`id` присутствует в `config.markers` **и** `removed is not True`» — то есть тем же правилом, что уже использует `async_refresh` для `pairs`, и тем же, что `import_export.live_layout` использует для «live». При таком определении:
+- AC1 естественно закрывает реальный сценарий (маркер тombstoned → на этом же или следующем `config/set` его трейл вычищается сервером, независимо от того, успел ли отработать клиентский `trail/delete`);
+- AC2 не меняется по сути (маркеры без tombstone по-прежнему сохраняют трейл);
+- общий helper (пункт 1 «Контракта реализации») при переносе на import-путь физически меняет его текущее поведение (сегодня import-очистка тоже игнорирует `removed`) — это нужно явно назвать в AC4 как ожидаемое расширение регрессионного покрытия, а не молчаливый побочный эффект.
+
+**Серьёзность:** High, блокирует. Без ответа на этот вопрос AC1 недоказуем как критерий приёмки той проблемы, которую issue заявляет; принять ТЗ в таком виде — значит зафиксировать критерий, который не отличает «починили» от «не починили» в основном сценарии.
+
+### Medium-1 (в скоупе) — release-артефакты / `User-Visible` не названы
+
+**Где:** раздел «Откат» — покрывает миграцию и обратимость, но нигде в ТЗ не указано, `User-Visible: yes` или `no`, и нужен ли `docs/CHANGELOG.md`/`.ru.md`. DoR (§2.5) требует явного решения по этому пункту, лёгкий трек его не отменяет.
+
+Оба бага меняют наблюдаемое поведение трейлов (меньше осиротевших раздутых записей; после рестарта HA трейл не теряет первую точку) — это не чисто внутренний рефакторинг. Разумное значение по умолчанию — `User-Visible: yes` с короткой записью в оба changelog (бажный фикс, доступный пользователю), но это должен подтвердить автор при переходе в `S5-ready`, а не додумывать на этапе реализации.
+
+**Серьёзность:** Medium, в скоупе задачи — чинится добавлением одной строки в ТЗ (или в AC4/«Откат»), отдельного цикла не требует сама по себе, но по правилу §2.4 при отсутствии High это уже жёлтый вердикт, а не самостоятельное основание для другого циклa.
+
+## Что проверено и корректно
+
+- **Bug #2 (первая точка после рестарта не сохраняется)** — подтверждено чтением: `TrailRecorder.async_refresh` (`trails.py:190-191`) вызывает `self._sample(src, time.time())` и отбрасывает булев результат; сохранение (`_schedule_save`) и событие `houseplan_trail_updated` в этой ветке никогда не запускаются, в отличие от `_on_state` (строки 347-351), где тот же результат используется. Контракт AC3 («агрегировать результат стартовых `_sample()` и при фактическом изменении запускать тот же путь сохранения и уведомления») точно описывает нужную правку и технически осуществим без искусственных допущений.
+- **AC2 (no-op / ошибка не запускают очистку)** — подтверждено чтением `ws_config_set`: семантический no-op (`msg["config"] == data.get("config")`) возвращает результат до строки, где вызывается `_refresh_trail_recorder`; ветки `conflict`/`invalid_format`/`too_large`/`missing_plan` возвращают ошибку тем же путём, до `_refresh_trail_recorder`. Значит требование AC2 «неуспешный/no-op write не запускает очистку» соответствует уже существующему потоку управления и не требует новых допущений — только не сломать его при добавлении шага очистки.
+- **Общий helper для import/recovery (пункт 1 контракта)** — реальный прецедент найден и процитирован верно: `websocket_api.py` (полный импорт) строит `live_marker_ids` из `target_config.get("markers")` и вызывает `recorder.async_delete(marker_id)` для каждого отсутствующего id; вынесение этой логики в общий helper — разумный, проверяемый рефакторинг (при условии исправления High-1).
+- **Границы «вне скоупа»** согласуются с `docs/VACUUM.md`: формат/лимиты трейлов, debounce/throttle интервалы и подписки не описаны как изменяемые нигде в контракте реализации — согласовано.
+- **Откат** — единственный issue-коммит, обратной миграции нет, формат store не меняется — корректно и достаточно для этой задачи.
+- **Job/скоуп по SCOPE.md** — задача не создаёт нового поведения, а чинит уже задокументированный `docs/VACUUM.md` контракт («деление маркера удаляет трейлы», «трейл переживает рестарт») — попадает в J6, вопросов к продуктовой рамке нет.
+- **Аналитика лёгкого трека** — критерии §5 (сложность/риск ≤3, одна поверхность — backend lifecycle трейлов, без миграции конфига, без нового UX-контракта, без влияния на touch/perf) действительно выполняются одновременно; выбор `small` не оспаривается.
+
+## Чего не проверял
+
+- Тексты AC не проверялись на предмет запуска реального pytest — кода ещё нет, оценка чисто по тексту ТЗ и текущему состоянию `dev`.
+- Не проверялся более широкий побочный эффект правки на `_source_health` (пункт «Подписки, source-health… не меняются» принят на слово — код `_refresh_source_health` действительно не тронут контрактом реализации, но фактическое отсутствие регрессии подтвердит только код-ревью).
+- Не оценивалась многопоточная/многоклиентская гонка `config/set` вокруг предложенной очистки — вне AC, оставлено на усмотрение реализации при условии, что `write_lock`/`_refresh_lock` (уже существующие) её накрывают.
+
+## Вердикт
+
+Единственная блокирующая находка (High-1) — определение «существования» маркера для очистки не покрывает основной сценарий, который issue называет проблемой №1 (обычное удаление vacuum-маркера в редакторе), потому что такое удаление оставляет `removed: true` tombstone, а не убирает `id` из `config.markers`. Пока это не исправлено (или явно не оспорено автором с указанием, почему raw-presence — правильный выбор), AC1 не доказывает то, что заявлено в «Проблема».
+
+Medium-1 (release-артефакты/`User-Visible`) — в скоупе, чинится одной строкой, самостоятельного цикла не образует.
+
+`Вердикт: жёлтый · заход r1 · блокирующих циклов 0/2 · High: 1 · Medium: 1 → в задаче`