From 5a38bea3ffdc81bcb9b5a638eeddf9d33bd0784a Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Wed, 23 Sep 2026 09:47:04 +0000 Subject: [PATCH] docs: review document for #625 Issue: #625 User-Visible: no --- docs/reviews/SPEC-REVIEW-625-r1.md | 259 +++++++++++++++++++++++++++++ 1 file changed, 259 insertions(+) create mode 100644 docs/reviews/SPEC-REVIEW-625-r1.md diff --git a/docs/reviews/SPEC-REVIEW-625-r1.md b/docs/reviews/SPEC-REVIEW-625-r1.md new file mode 100644 index 00000000..a73de25f --- /dev/null +++ b/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» неприменимы. +- Код продукта не менялся; диапазон коммитов для этой стадии отсутствует. + +--- + + + +## Материал раунда + +- Ветка: `issue/625-backend-io-invariants`, коммит `b1f1f6b75438` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `b40b99307d3bc80bd28ac73ffa0e097c6451a3f6` + ``` + git log --all --format='%H %T' | grep b40b99307d3b + ``` +- Тело issue: `c453dfbbd8e968467dd6567e6327005b625432bec7f0be055fdc6ac85700c910` +- Вердикт конвейера: `yellow` · High 0