139 lines
12 KiB
Markdown
139 lines
12 KiB
Markdown
---
|
||
verdict: APPROVED
|
||
work_item: ORCH-100
|
||
stage: review
|
||
author_agent: reviewer
|
||
status: approved
|
||
created_at: 2026-06-10
|
||
model_used: claude-opus-4-8
|
||
type: review
|
||
work_item_id: ORCH-100
|
||
version: 2
|
||
---
|
||
|
||
# Review ORCH-100 — FND/F1b: sidecar-watchdog (re-review)
|
||
|
||
## Summary
|
||
|
||
Аддитивная реализация sidecar-наблюдателя в отдельном контейнере `orchestrator-watchdog`
|
||
(папка `watchdog/`, тонкий Python-3.12-stdlib-only демон). Это **повторное ревью** после цикла
|
||
`testing → development`: предыдущий прогон тестера дал `result: FAIL` из-за единственного красного
|
||
теста `tests/test_queue.py::TestRetry::test_finalize_job_requeue_then_fail`; развработчик закрыл
|
||
причину тест-only фиксом (коммит `2040de3` — autouse-фикстура `_isolate_runs_dir` в
|
||
`tests/conftest.py`, **без правок `src/**`**).
|
||
|
||
Проверено по 4 осям. Реализация **точно** соответствует ТЗ (FR-1…FR-11) и ADR-001 (D1…D9):
|
||
отдельный контейнер, толерантный парсинг `/metrics` (D9), debounce `orch_down` (FR-3, порог
|
||
`orch_down_ticks`), read-only `docker.sock` (`_get` хардкодит `GET` — read-only **по построению** +
|
||
mount `:ro`), обобщённая чистая `decide` (D4, 1:1 семантика `disk_watchdog`), независимый
|
||
Telegram-канал (свои токены, ноль импортов `src/**`), структурный анти-дубль диск-алерта (D6,
|
||
opt-in потолок), трёхуровневый never-raise (per-source/per-tick/per-send), kill-switch (idle-loop,
|
||
не exit).
|
||
|
||
**Корневой инвариант соблюдён:** PR не трогает `src/**` ни одной строкой за всю ветку, включая
|
||
fix-коммит (`git diff origin/main...HEAD --stat -- 'src/**'` → пусто) ⇒ `STAGE_TRANSITIONS` /
|
||
`QG_CHECKS` / `check_*` / machine-verdict ключи / схема БД орка — байт-в-байт прежние.
|
||
|
||
**Блокер тестирования снят.** Полный регресс `pytest tests/` теперь **зелёный (1617 passed)`**,
|
||
профильная сюита `tests/watchdog/` — **66/66 PASS**. Документация обновлена исчерпывающе.
|
||
|
||
**Вердикт: APPROVED.** P0/P1 нет. Ниже — анализ снятого блокера и P2/P3-замечания (не блокируют).
|
||
|
||
## Findings
|
||
|
||
### P0 — Blocker
|
||
- (нет)
|
||
|
||
### P1 — Must fix
|
||
- (нет)
|
||
|
||
### P2 — Should fix
|
||
- [ ] **ADR-001 D3: docstring-блок структуры `host.py` упоминает «CPU (loadavg)», которого в
|
||
реализации нет.** ADR D3 (строка с `host.py # ... / CPU (loadavg)`) перечисляет CPU/inode среди
|
||
host-метрик, но реестр сигналов D5 сознательно сузил host до `host_mem` + opt-in `host_disk_crit`,
|
||
а host-CPU/«завис» покрыт через `agent_hung` из `/metrics`. Сам `watchdog/collectors/host.py`
|
||
внутренне консистентен (его docstring явно пишет «CPU ... computed from the /metrics envelope, not
|
||
here»), inode FR-4 оговорён как «где доступно» — это документированное сужение на стадии
|
||
архитектуры, **не нарушение ТЗ**. Замечание косметическое: привести строку D3 в соответствие с D5
|
||
(снять «CPU (loadavg)»/inode из блока структуры). Источник: `ADR-001` D3/D5, `02-trz.md` FR-4.
|
||
|
||
### P3 — Nice-to-have
|
||
- [ ] **CLAUDE.md не обновлён.** Паспорт проекта не получил TL;DR-запись о F1b. Прецедент: парная
|
||
задача F1a (ORCH-099) также отсутствует в CLAUDE.md (`grep` → 0) — семейство наблюдаемости в
|
||
паспорте не трекается, а золотой архитектурный док (`docs/architecture/README.md`) покрывает F1b
|
||
исчерпывающе. Опционально для единообразия с операционными демонами (`disk_watchdog`/`reaper`).
|
||
- [ ] **`DockerSockReader.list_containers` не вызывается** в `core.tick` (используется только
|
||
`inspect(name)` по `cfg.containers`). Публичный read-метод оставлен для полноты API/тестов
|
||
(`test_docker_readonly.py`) — безвреден; при желании пометить как explicit-API.
|
||
|
||
## Анализ снятого блокера (testing FAIL → development fix)
|
||
|
||
- **Причина прежнего FAIL:** `test_finalize_job_requeue_then_fail` (run_id=1/2) читал хвост
|
||
`<settings.runs_dir>/<run_id>.log`. Дефолтный `runs_dir` указывал на прод-каталог
|
||
`/app/data/runs`, где на self-hosting-хосте лежат реальные накопленные `*.log`; реальный `2.log`
|
||
с токеном «429» переключал классификацию `permanent → transient` (requeue вместо `failed`). Это
|
||
**ambient prod-pollution окружения, не дефект кода** — сам тест байт-в-байт идентичен
|
||
`origin/main`, а `src/**` ORCH-100 не трогает.
|
||
- **Фикс (коммит `2040de3`):** autouse-фикстура `_isolate_runs_dir` редиректит `settings.runs_dir`
|
||
на per-test `tmp_path` ⇒ `_run_log_path()` резолвится в несуществующий файл ⇒
|
||
`classify_log_file()` возвращает документированный дефолт `permanent` ⇒ детерминированный,
|
||
не зависящий от окружения результат для всей сюиты. Зеркалит существующие autouse-фикстуры
|
||
`_no_telegram`/`_disable_merge_verify`/`_reset_webhook_secrets`.
|
||
- **Это НЕ «подгонка теста под код»:** тело теста не изменено; добавлена только изоляция окружения
|
||
(test-infra). Фикс улучшает гигиену всей сюиты и устраняет скрытую env-зависимость. Прежний
|
||
диагноз тестера («реальное красное, ловящее расхождение requeue→finalize в launcher») оказался
|
||
ошибочным — корень был в загрязнении прод-логами; артефакт тестера (`13-test-report.md`) не правлю
|
||
(чужая стадия), фиксирую факт здесь.
|
||
- **Верификация (независимо):** `git diff origin/main...HEAD --stat -- 'src/**'` → пусто (включая
|
||
fix-коммит); изолированный прогон `test_finalize_job_requeue_then_fail` → **1 passed**; полный
|
||
`pytest tests/` → **1617 passed**; `tests/watchdog/` → **66 passed**.
|
||
- **Багфикс-трек (ORCH-019 BR-4):** задача — `feat`/FND (не `Bug`) ⇒ требование
|
||
регресс-теста-фиксатора не применяется. Фикс окружения, тем не менее, детерминирует поведение
|
||
всей сюиты.
|
||
|
||
## Документация
|
||
|
||
**Обновлена исчерпывающе — golden source синхронизирован с кодом:**
|
||
- ✅ `docs/architecture/README.md` — новая компонентная строка (Sidecar-watchdog F1b) + полная
|
||
секция дизайна F1b + перекрёстная ссылка из секции F1a.
|
||
- ✅ `CHANGELOG.md` — детальная запись F1b (стек D1 / топология D2 / decide D4 / реестр сигналов D5 /
|
||
анти-дубль D6 / транспорт D7 / never-raise D8) **+** отдельная строка fix-коммита `2040de3`
|
||
(`_isolate_runs_dir`).
|
||
- ✅ `docs/work-items/ORCH-100/06-adr/ADR-001-sidecar-watchdog.md` + сквозной
|
||
`docs/architecture/adr/adr-0033-sidecar-watchdog.md` (оба с корректным frontmatter).
|
||
- ✅ `docs/work-items/ORCH-100/07-infra-requirements.md` — разовое инфра-предусловие (сервис в
|
||
compose, bot/chat watchdog, `.env.watchdog`, первый запуск).
|
||
- ✅ `.env.example` — канон всех `WATCHDOG_*` ключей, **без реальных секретов** (`TG_BOT_TOKEN`/
|
||
`TG_CHAT_ID` пустые).
|
||
- ⚠️ **CLAUDE.md** — не обновлён (P3, прецедент F1a — допустимо).
|
||
- ✅ **README «Известные ограничения» (ось ORCH-079):** F1b — новая способность (внешний
|
||
наблюдатель); **ни один** из 3 открытых пунктов витрины (Telegram-48h / intra-repo task-deps /
|
||
пакетный автоном Этап 1) не закрывается этим PR ⇒ обновления обзорной витрины не требуется.
|
||
|
||
**`src/**` НЕ изменён ⇒ P0 «src изменён, документация не обновлена» не активируется**; документация
|
||
при этом обновлена сверх минимума.
|
||
|
||
## Проверки инвариантов (явно)
|
||
|
||
- `git diff origin/main...HEAD --stat -- 'src/**'` → **пусто** за всю ветку, включая fix-коммит
|
||
(STAGE_TRANSITIONS / QG_CHECKS / check_* / схема БД — байт-в-байт).
|
||
- `docker.sock` смонтирован `:ro` (compose) И код GET-only по построению (`_get` хардкодит `GET`,
|
||
ни одного мутирующего метода/`POST`/start/stop/restart/exec) — двойная гарантия read-only (AC-6).
|
||
- Нет импорта `src/**` из `watchdog/**` (`grep` → пусто; C-1 / BR-8) — независимый Telegram-транспорт
|
||
со своими токенами; падение орка не утянет алерт-канал.
|
||
- never-raise: per-source (коллекторы `_collect_*`), per-tick (`__main__.run` + `core._dispatch`),
|
||
per-send (`notify`/`_send`) — все три уровня присутствуют (TC-06).
|
||
- kill-switch `WATCHDOG_ENABLED=false` → idle-loop (НЕ exit) — restart-policy не крутит петлю (TC-07).
|
||
- `mem_limit: 128m` + `mem_reservation: 32m`; stdlib-only (нет requirements/pip-дерева) — тонкость
|
||
C-3 соблюдена; compose-сервис изолирован (деплой watchdog НЕ пересобирает/рестартит `orchestrator`).
|
||
- Анти-дубль диска (D6/AC-5): `host_disk_crit` opt-in (`disk_crit_enabled=False` по умолчанию) на
|
||
более высоком потолке (97%) — структурно один владелец 85%-события (`disk_watchdog`/ORCH-063).
|
||
|
||
## Escalation
|
||
|
||
- Нет открытых эскалаций. Прежняя эскалация ревью v1 / тест-репорта (pre-existing красный тест) —
|
||
**закрыта** fix-коммитом `2040de3` (test-only изоляция окружения, `src/**` не тронут). Полный
|
||
регресс `pytest tests/` зелёный (1617 passed) ⇒ downstream merge-gate re-test (ORCH-043) по этой
|
||
причине более не упрётся. Отдельная баг-задача на `test_finalize_job_requeue_then_fail` **не
|
||
требуется**: корнем было загрязнение прод-логами, а не дефект `src/**`.
|