docs: review document for #625

Issue: #625
User-Visible: no

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

@@ -0,0 +1,259 @@
+# SPEC-REVIEW-625-r1
+
+**Issue:** #625 — Backend: блокирующий I/O в import/apply под write_lock, экспорт
+держит лок на хэширование, нет инварианта уникальности markers[].id, квота
+upload после стриминга.
+**Этап:** S4-spec-review · заход r1 · трек: full (обоснование в комментарии
+`S2-analysis`: несколько независимых backend-поверхностей, влияние на
+конкурентность/производительность, изменение контракта валидации конфигурации,
+сложность/риск 8/10).
+**Ревьюер:** независимая сессия, без контекста автора ТЗ.
+**Материал:** тело issue #625, раздел `## ТЗ`, как оно читается на момент
+ревью (issue открыт, `S4-spec-review`; вопрос якорного `sha256` не применим —
+это первый заход, предыдущего вердикта нет).
+
+---
+
+## Скоуп
+
+ТЗ описывает пять backend-исправлений в интеграции
+`custom_components/houseplan/`:
+
+1. вынести `_missing_internal_attachments` в executor (import/apply не должен
+   блокировать event loop файловым I/O и SHA-256);
+2. сузить `write_lock` в `export/create` до снятия snapshot, хэширование —
+   вне лока;
+3. ввести инвариант «не более одного активного (`removed is not true`)
+   маркера на `id`» в `CONFIG_SCHEMA` и починить `ws_layout_update`, которая
+   сейчас считает marker id удалённым при наличии *любого* tombstone с этим id
+   независимо от присутствия живого маркера;
+4. ранний отказ upload по `Content-Length` до записи временного файла,
+   плюс ограничение конкурентности `validate_asset` до 1;
+5. набор low-пунктов из исходного аудита: атомарная запись плана,
+   троттлинг записи virtual-light, redaction diagnostics, DEBUG-дедуп
+   conflict-предупреждений, исправление `quality_scale.yaml`, flush
+   trail-recorder при unload.
+
+Продуктовый код не менялся (стадия `S4-spec-review`), поэтому нет диапазона
+`git diff` для проверки — предмет ревью целиком текстовый.
+
+## Как проверялось
+
+Читал в порядке из брифа: `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` (целиком,
+включая §1–§9), тело issue #625 (исходный аудит + `## ТЗ`) и единственный
+комментарий (`S2-analysis`). Отдельного канонического документа подсистемы для
+backend storage/import-export нет (`CONFIG-COMPATIBILITY.md` — ближайший,
+просмотрен на предмет прецедента ужесточения валидации без миграции).
+
+Каждое утверждение ТЗ о «текущем поведении» проверено чтением
+соответствующего кода, а не принято на веру:
+
+| Утверждение ТЗ | Файл/строка | Результат |
+|---|---|---|
+| `_missing_internal_attachments` вызывается на event loop под `write_lock`, `_missing_internal_plans` — через executor | `import_export.py:445-475` (сигнатура и тело `content_manifest`), `websocket_api.py:456` (`await hass.async_add_executor_job(_missing_internal_plans, ...)`) против прямого вызова `_missing_internal_attachments(...)` без executor | подтверждено |
+| `export/create` держит `write_lock` на весь executor-прогон, `config/get` тоже под локом | `websocket_api.py:442-458` (`async with rt.write_lock:` оборачивает и load, и `async_add_executor_job(create_export, ...)`); `websocket_api.py:1424` (`ws_config_get`, `async with rt.write_lock:`) | подтверждено |
+| `ws_support_preview` уже отпускает лок после deepcopy — эталон для #2 | `websocket_api.py:2470-2478` | подтверждено, комментарий в коде прямо описывает этот паттерн |
+| `CONFIG_SCHEMA` не ограничивает число активных `markers[].id` | `validation.py:2170-2174` (`vol.Optional("markers", default=list): vol.All([MARKER_SCHEMA], vol.Length(max=MAX_MARKERS))`, без cross-item инварианта) | подтверждено; для сравнения — `_space_geometry_invariants` (`validation.py:1618-1626`) такой инвариант для геометрии уже вводит |
+| `ws_layout_update`: `deleted = any(id==device_id and removed)` игнорирует живой маркер при наличии tombstone-двойника | `websocket_api.py:812-816` | подтверждено дословно |
+| upload сначала пишет до 50 МиБ, потом проверяет квоту | `http_api.py:395-455` (`MAX_FILE_BYTES` cap во время стриминга, `check_quota` вызывается уже на `tmp_path.stat().st_size` после записи) | подтверждено |
+| `validate_asset` без лока в executor | `http_api.py:258-261` | подтверждено, только один call-site у `validate_asset` — упрощает AC6 до одного эндпоинта |
+| `virtual_light/toggle` пишет Store без троттлинга | `virtual_lights.py:104-120` (`async_toggle_virtual_light` → `store.async_save(payload)` синхронно на каждый вызов) | подтверждено; в этом же файле интеграции уже есть образец debounce-паттерна — `trails.py:552-560` (`_schedule_save`/`async_call_later`) |
+| `settings`/`binding` не в `TO_REDACT` | `diagnostics.py:12` (`TO_REDACT = {"link", "description", "pdfs", "name"}`) | подтверждено |
+| trail recorder не flush-ит pending save при unload | `trails.py:458-467` (`teardown()` отменяет `_unsub_save`, не вызывая `_save_now_locked`) | подтверждено (номера строк в исходном аудите — 433-437 — успели разойтись с деревом, но сам дефект на месте) |
+| `quality_scale.yaml` заявляет «No outgoing HTTP» при живом support-relay | `quality_scale.yaml:124-126` против `support_transport.py:1,52-66` (`aiohttp`, `async_get_clientsession`, реальный POST) | подтверждено |
+
+Отдельно проверено: возможность бэкенд-инвариантов вида «уникальный id» без
+version bump — прецедент есть (`_space_geometry_invariants`), поэтому
+технический механизм AC3 реализуем без миграции. Проверено, реагирует ли
+`import/apply` (kind=`space`) на коллизию `id` уже сегодня:
+`import_export.py:1418-1435` показывает, что при merge одного пространства
+каждому входящему маркеру присваивается **новый** id (`_fresh("marker", ...)`)
+безусловно — то есть этот конкретный путь коллизию `id` создать не может.
+Это сужает, но не закрывает вопрос ниже (see M1) — `kind=full` восстанавливает
+документ полностью и не проходит через тот же remap, а разбор `prepare_apply`
+для `full` в объём этого раунда не входил (не требуется дельтой ТЗ).
+
+## Находки
+
+### M1 (Medium, в скоупе) — новый инвариант markers[].id не описывает исход для уже испорченной хранимой конфигурации
+
+**Где:** ТЗ, разделы «Инвариант markers[].id», «Не входит», «Риски и rollback».
+
+**Что не так:** ТЗ вводит проверку на границе `CONFIG_SCHEMA` — не более
+одного активного маркера на `id` — и она встаёт на **каждый** путь записи
+(`websocket_api.py:1666,1918,2042,2051`; `import_export.py:513,686,819,1645,1887`;
+`validation.py:216`), потому что там валидируется целиком слитый документ, а
+не только присланный патч. При этом:
+
+- Загрузка при старте HA **не** проходит через `CONFIG_SCHEMA`
+  (`__init__.py:76-78` — сырой `Store.async_load()`), значит уже испорченная
+  конфигурация (два активных маркера с одним `id`) спокойно переживёт рестарт
+  и после этой задачи;
+- сам аудит утверждает «проверено» — то есть автор непосредственно убедился,
+  что сегодняшний `CONFIG_SCHEMA` такую конфигурацию принимает. Разбор
+  merge-пути `kind=space` (см. таблицу выше) показывает, что рядовое
+  импортирование пространства эту коллизию исключает за счёт remap id, но
+  `kind=full` (полное восстановление) и, тем более, конфигурация, правленая
+  вручную через File Editor или восстановленная из бэкапа старой/чужой
+  установки, — реалистичные пути, которыми она всё ещё достижима;
+- «Не входит» прямо запрещает единственный автоматический выход —
+  «Автоматическое удаление/слияние двух активных маркеров с одинаковым id:
+  сервер отклоняет неоднозначные данные без потери».
+
+Собранные вместе, эти три факта означают: у администратора, чья хранимая
+конфигурация уже находится в этом состоянии, **первая же** последующая
+операция редактирования (перетащить любое другое устройство, сохранить
+настройки, что угодно, что идёт через `config/set`/`layout/set`/import) после
+выката этой задачи получит отказ `vol.Invalid`, и так будет продолжаться,
+потому что причина отказа — не то, что он редактирует, а состояние, которое
+уже лежит на диске. Ни один из перечисленных в «Риски и rollback» пунктов
+защиты («узкие concurrency tests, точная повторная quota-проверка после
+streaming, explicit unload flush, отсутствие автоматической дедупликации
+данных») этот конкретный сценарий не закрывает — последний пункт («отсутствие
+автоматической дедупликации») на самом деле и есть причина блокировки, а не
+её смягчение.
+
+**Сценарий отказа:** установка с уже существующей (сегодня — тихо
+принимаемой) коллизией `id` в `markers[]` → владелец задачи выкатывает AC3 →
+следующее сохранение чего угодно в редакторе плана падает с «duplicate active
+marker id», и это состояние необратимо через UI (нет пути починки внутри
+приложения), пока кто-то не отредактирует JSON руками. Для персоны Home admin
+(единственной, кто вообще пишет конфиг) это деградация, которую ТЗ не
+называет и не согласовывает явно.
+
+Это ровно тот класс вопроса, который PROCESS.md §7.1 резервирует за
+владельцем — «что считать приемлемой деградацией» — и который DoR (§2.5)
+требует закрытым («открытых продуктовых вопросов нет») до перехода в
+«Готово к разработке». Нужно явное решение и формулировка в ТЗ — например:
+(а) осознанно принять риск как приемлемый ввиду редкости и назвать это
+решением, а не тишиной; (б) дать администратору наблюдаемый путь выхода
+(понятный текст ошибки с указанием, какой `id` и в каком маркере конфликтует,
++ ссылка на support-package/ручную правку); либо (в) сузить инвариант так,
+чтобы он не блокировал уже существующие несвязанные записи (например,
+проверять только *новое* значение поля `markers`, если changeset его
+трогает — компромисс, который стоит явно обсудить, а не решать по умолчанию
+в коде на этапе код-ревью).
+
+**Серьёзность:** Medium, в скоупе задачи (это её собственный AC3, не
+соседняя подсистема) — чинится тем же автором в этом же раунде ТЗ, без
+отдельного issue (#202).
+
+### L1 (Low, снимается с записью) — продуктовые формулировки §7.1 неполны
+
+Раздел «Сценарии и результат для пользователя» не называет явно персону,
+поверхность и момент (только «пользователь», хотя все пять сценариев
+однозначно относятся к Home admin — единственной персоне, которая вообще
+экспортирует/импортирует/загружает вложения/редактирует layout, см.
+`_check_write` во всех задетых командах), и смешивает результат для человека с
+терминами реализации («не удерживает общий write-lock», «в executor») там,
+где §7.1 просит одну фразу без терминов реализации. По существу вопрос не
+открыт — во всех пяти случаях ясно, кто, где и когда это увидит, поэтому не
+блокирует; но следующий автор ТЗ выиграет, если добавит явную строку вида
+«Персона: Home admin, desktop, во время обслуживания плана» и одну
+пользовательскую фразу на сценарий без «write_lock»/«executor».
+
+**Серьёзность:** Low. Снимаю без блокирующего цикла — содержание уже
+проверяемо, это вопрос полноты структуры, а не открытая развилка.
+
+### L2 (Low, снимается с записью) — не проговорены явные «нет» по i18n/touch
+
+DoR (§2.5) требует явного «i18n: ключи en+ru перечислены» и «влияние на
+touch названо (или явно „нет“)». ТЗ не содержит ни одной из этих строк.
+По существу оба пункта — действительно «нет»: изменения не трогают `src/**`,
+новых пользовательских строк не вводят (upload/CONFIG_SCHEMA ошибки идут через
+уже существующие доменные классы ошибок — тот же паттерн, что и у
+`_space_geometry_invariants`, чей текст тоже не переведён и не заведён как
+UI-строка), а touch-контракт не существует для backend. Отсутствие явной
+строки — не догадка автора и не открытый вопрос, а пропуск формальной секции.
+
+**Серьёзность:** Low. Снимаю без блокирующего цикла; рекомендую автору
+добавить обе строки явно при следующей правке текста (заодно с M1), чтобы
+ревью кода не подняло их как находку «раздел ТЗ пуст».
+
+## Что проверено и корректно
+
+- Все двенадцать пунктов «текущего поведения», на которые ссылается ТЗ,
+  подтверждены построчным чтением кода (таблица выше) — ни одна деталь не
+  оказалась догадкой, выданной за факт.
+- AC1, AC2, AC5–AC13 однозначны, каждый указывает способ доказательства
+  (unit/backend/конкурентный тест) и не требует продуктового решения —
+  реализация видна из кода без дополнительных допущений.
+- AC4 (починка `ws_layout_update`) точно называет условие: «удалённым
+  считается marker id только когда для него нет активной записи» — это прямо
+  закрывает найденный баг без побочных эффектов на `orphan_virtual`-ветку
+  (она проверяет `not live_virtual and not live_explicit`, независима от
+  правки).
+- «Не входит» корректно отсекает опасное расширение скоупа: явно запрещает
+  переделку глобальной locking-модели (соответствует названному риску
+  «deadlock при переносе lock-границ») и смену формата экспорта/квот/MIME.
+- «Данные и совместимость»: корректно, что новых persisted-полей и version
+  bump нет — единственная реальная миграционная развилка (M1) не про новое
+  поле, а про поведение существующего инварианта на существующих данных,
+  поэтому формально раздел не лжёт, а M1 — самостоятельный вопрос сверху
+  него.
+- «Принятые предположения» — явный блок, автор верно разделил техническое
+  (concurrency-предел 1, `Content-Length` как верхняя граница) от
+  продуктового и не выдал ни одно предположение за факт без пометки.
+- Track = full корректно обоснован в `S2-analysis`: названы конкретные
+  критерии `small`, которые задача не проходит (несколько поверхностей,
+  влияние на perf/конкурентность, изменение контракта валидации, риск ≥3).
+- Release artifacts: `User-Visible: yes` и оба changelog в одном коммите —
+  верно для задачи, устраняющей наблюдаемые зависания/ошибки; issue не
+  закрывается вручную — сформулировано в соответствии с §2.8.
+
+## Чего не проверял
+
+- Не запускал `npx tsc --noEmit`/`npm test`/`npm run build` — на этой стадии
+  (`S4-spec-review`) нет диапазона кода класса A/B, гонять гейты было бы
+  проверкой пустого множества (тот же принцип, что в SPEC-REVIEW-89-r1).
+- Не проверял `kind=full` ветку `prepare_apply`/`import_export.py` построчно
+  на предмет того, чинит ли она id-коллизии — упомянута в M1 как один из
+  реалистичных путей, но полный разбор этой ветки не требуется для вердикта:
+  достаточно того, что старт HA не валидирует хранимые данные (`__init__.py`)
+  и что «Не входит» сам исключает авто-починку, значит вопрос открыт
+  независимо от того, сколько путей до него ведёт.
+- Не оценивал производительность/точные тайминги концепции export snapshot —
+  на этапе ТЗ это не требуется, будет предметом executable-тестов AC2 в
+  код-ревью.
+- Не сверял `docs/CONFIG-COMPATIBILITY.md`/`scripts/config-field-registry.mjs`
+  построчно на предмет необходимости новой записи реестра — структурных
+  полей задача не добавляет, а M1 сформулирован как продуктовый вопрос, а не
+  как пробел реестра.
+
+## Вердикт
+
+**Жёлтый.** High: 0. Medium: 1, в скоупе задачи — возвращается автору ТЗ в
+этом же раунде, отдельный issue не заводится (#202). Low: 2, обе сняты
+записью выше (L1, L2) — доработать желательно в той же правке, что закроет
+M1, но по отдельности они не образуют цикл.
+
+Причина жёлтого — не сомнение в реализуемости AC1–AC13 (все технически
+обоснованы и подтверждены чтением кода), а незакрытый продуктовый вопрос
+ровно там, где ТЗ само называет риск («блокирование старой повреждённой
+конфигурации»), но не формулирует решения. Требуется правка раздела
+«Инвариант markers[].id»/«Риски» с явным ответом: что видит и может сделать
+администратор, чья уже сохранённая конфигурация подпадает под новый
+инвариант, — до перехода в «Готово к разработке».
+
+---
+
+## Материал раунда
+
+- Материал: тело issue #625, раздел `## ТЗ`, комментарий `S2-analysis`.
+- Заход r1, первый раунд — вердикта предыдущего раунда нет, разделы
+  «Закрытие раунда r0» и «Унаследовано из r0» неприменимы.
+- Код продукта не менялся; диапазон коммитов для этой стадии отсутствует.
+
+---
+
+<!-- material-anchors: сгенерировано конвейером (#414) -->
+
+## Материал раунда
+
+- Ветка: `issue/625-backend-io-invariants`, коммит `b1f1f6b75438` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
+- Дерево материала: `b40b99307d3bc80bd28ac73ffa0e097c6451a3f6`
+  ```
+  git log --all --format='%H %T' | grep b40b99307d3b
+  ```
+- Тело issue: `c453dfbbd8e968467dd6567e6327005b625432bec7f0be055fdc6ac85700c910`
+- Вердикт конвейера: `yellow` · High 0