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

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