Files
orchestrator/docs/work-items/ORCH-111/12-review.md
claude-bot 521a72e702
All checks were successful
CI / test (push) Successful in 3m48s
CI / test (pull_request) Successful in 4m48s
reviewer(ET): auto-commit from reviewer run_id=681
2026-06-15 08:31:48 +03:00

14 KiB
Raw Blame History

verdict, work_item, stage, author_agent, status, created_at, model_used, type, work_item_id, version
verdict work_item stage author_agent status created_at model_used type work_item_id version
APPROVED ORCH-111 review reviewer approved 2026-06-15 claude-opus-4-8 review ORCH-111 2

Review ORCH-111

Summary

PR добавляет в sidecar-watchdog opt-in сигнал proc_blocking — алерт на долго живущий осиротевший тест-процесс (pytest), репарентированный на хост и грузящий CPU (инцидент: осиротевший test_install_lite_script.py жил >2 суток без единого алерта и через CPU-голодание валил merge-gate re-test на ORCH-109). Изменение строго внутри наблюдателя (watchdog/** + одна строка pid: host в сервисе orchestrator-watchdog docker-compose.yml).

Проверено по всем 4 осям, с независимой верификацией (прогон тестов, скан запрещённых вызовов, сверка src/ diff, сверка key-sync канона, атрибуция деплой-фейла). Замечаний уровня P0/P1 нет. src/** — байт-в-байт не тронут (git diff origin/main…HEAD -- src/ пуст), конвейерные контракты сохранены, документация наблюдателя обновлена в том же PR, watchdog+key-sync тесты зелёные (125 passed на текущем HEAD). Вердикт: APPROVED.

Re-review (v2). Задача вернулась на review после deploy_status: FAILED (hook_exit_code: 1, ORCH-036 self-deploy). Деплой-фейл расследован и НЕ относится к диффу (см. раздел «Атрибуция деплой-фейла» ниже): normal-deploy хука рестартит только сервис orchestrator (retag прев-валидированного образа + health-check порта 8500), сервис orchestrator-watchdog он не трогает — правка pid: host физически не могла вызвать exit 1. Это деплой-инфра/окружение (CPU-голодание хоста — ровно тот класс осиротевших процессов, который и закрывает эта задача), не ось code-review. Дифф с момента прошлого review/testing кодово не менялся (поздний developer commit run 680 добавил только детерминированные gate-артефакты 17-security-report.md/ 18-coverage-report.md). Подтверждаю APPROVED на тех же основаниях.

Сверка по осям

1. Соответствие ТЗ / Acceptance Criteria — PASS.

  • FR-1 (watchdog/signals.py::proc_signals — чистый builder, per-entity key ("proc_blocking", pid), active ⇔ age_s > cfg.proc_age_s, действенный RU-detail: PID + возраст(с) + усечённый cmdline ≤120 + накопленное CPU) — ✓.
  • FR-2 (watchdog/collectors/proc.py — stdlib-only /proc-скан, read-only, never-raise → [], чистый разбор parse_btime/parse_pid_stat/decode_cmdline/matches_patterns отделён от I/O collect_candidates, инъектируемые proc_root/now/clk_tck/read_* для тестов на фейк-/proc) — ✓.
  • FR-3 (4 ключа WATCHDOG_PROC_*, парсеры _bool/_float/_csv never-raise, дефолт-off, derived proc_age_s) — ✓.
  • FR-4 (только наблюдение — нет os.kill/сигналов/subprocess/мутаций/чтения /proc/<pid>/environ; независимо подтвердил grep'ом: совпадения только в докстрингах-контракте) — ✓.
  • FR-5 (диспетч core.tick() через существующую decision.decide()/AlertState; RECOVERY синтезируется _synthesize_proc_recoveries без новой анти-спам-логики — per-family bookkeeping) — ✓.
  • FR-6 (без дубля с agent_hung — по построению: cmdline-скоуп pytestclaude + порог возраста выше бюджета; кросс-namespace матчинг PID не нужен) — ✓.
  • AC-1…AC-10 покрыты: TC-01 (red→green builder), TC-02 (граница strict >), TC-03 (конфиг/kill-switch), TC-04 (never-raise/read-only AST), TC-05 (alert→none→realert→recovery→no-repeat), TC-06 (partition vs agent_hung), TC-07 (red→green tick→dispatch «алерт даже когда ни одна стадия не stuck» + flag-off
    • collector-explodes), TC-12 (compose pid: host + прод-orch его НЕ получает), key-sync.
  • §7 (порог): дефолт WATCHDOG_PROC_AGE_MIN=60 (=3600s) > max(merge_retest_timeout_s=600, coverage_run_timeout_s=900)=900s (4× запас) — кросс-инвариант D2 соблюдён (анти-false-positive AC-4).

2. Соответствие ADR — PASS.

  • Реализация 1:1 соответствует ADR-001: D1 (watchdog-side /proc под pid: host, НЕ orch-side /metrics), D2 (анти-FP/анти-дубль по построению), D3 (коллектор), D4 (builder + синтез RECOVERY для исчезнувшего PID), D5 (конфиг дефолт-off), D6 (pid: host только на orchestrator-watchdog), D7 (инварианты конвейера). PID-recycling — осознанный край ADR D4 (start_ticks в ключ не добавлен; «опциональное усиление»), допустимо.
  • Сквозной docs/architecture/adr/adr-0041-watchdog-orphan-test-process-alert.md присутствует.
  • Трассировка (TRACEABILITY.md): правки watchdog/{core,signals,config}.py + сервиса watchdog — аддитивные врезки, гейтированные на proc_enabled; маркированные инварианты предшественников (ORCH-100 sidecar decide/AlertState/never-raise; read-only-маунты) не сломаны. Существующий test_host_paths_mounted_read_only остаётся зелёным (pid — не volume), добавлен позитивный test_watchdog_shares_host_pid_namespace.
  • Глобальные инварианты (AC-9): STAGE_TRANSITIONS / QG_CHECKS / check_* / machine-verdict / схема БД / /metrics+schema_versionбайт-в-байт не тронуты (подтверждено: git diff по src/ пуст).

3. Качество кода — PASS.

  • Docstrings на всех публичных функциях (коллектор/builder/synth/tick) — содержательные, со ссылками на D-решения ADR.
  • Тесты содержательные, не тривиальные: red→green регресс-якоря (TC-01 builder, TC-07 tick→dispatch), граничный strict >, патологический comm со скобками/пробелами, NUL-cmdline, гонка «pid исчез mid-scan», AST-скан read-only-контракта, полная последовательность alert→none→realert→recovery.
  • Регресс-тест дефекта (ORCH-019 BR-4 / усиление): задача эскалирована escalate: full-cycle (прошла architecture, не bug-fast-track), но несёт явные тест-фиксаторы дефекта (TC-01 builder, TC-07 tick→dispatch — оба red→green) — требование «фикс кода несёт тест-фиксатор» выполнено.
  • Безопасность: read-only (per-pid + top-level guard), never-raise, /proc/<pid>/environ не читается (секреты, R-2), cmdline усечена до 120 (анти-утечка аргументов). Привилегия pid: host — read-only, только у наблюдателя, обоснована в 07-infra-requirements.md; меньше уже-смонтированного docker.sock.
  • Корректность вычислений: age_s = now (btime + starttime/clk_tck), cpu_s = (utime+stime)/clk_tck, clk_tck с безопасным фолбэком 100 — верно; CPU вне активации (информационно), как требует ADR.

4. Документация — PASS.

  • src/** НЕ изменён → жёсткое правило «src изменён, доки нет → P0» не активируется; доки наблюдателя обновлены в том же PR независимо: CHANGELOG.md, docs/architecture/README.md (раздел proc_blocking), docs/deployment/LITE_SETUP.md (opt-in блок + «Проверка»), ADR work-item 06-adr/ADR-001 + сквозной adr-0041, .env.watchdog.example ↔ блок WATCHDOG_* .env.example (4 ключа синхронизированы 1:1, key-sync tests/test_lite_setup_doc.py зелёный).
  • Витрина docs/overview/ (ORCH-011): sidecar-watchdog описан абстрактно (каталог сигналов не перечисляет); новый opt-in (дефолт-off) сигнал абстрактную формулировку не противоречит, машинно-проверяемые факты не меняет → обновление витрины не обязательно (не P1).
  • Стадийное владение артефактами соблюдено (rule 3/4): ретроактивных правок ТЗ/ADR нет.

Атрибуция деплой-фейла (вне оси code-review, информационно)

14-deploy-log.md фиксирует deploy_status: FAILED / hook_exit_code: 1. Расследование:

  • Normal-deploy scripts/orchestrator-deploy-hook.sh (шаги 2b/3/4) выполняет retag прев-валидированного SOURCE_IMAGE на orchestrator + docker compose up -d --no-build $TARGET_SERVICE только для сервиса orchestrator + health-check порта 8500. Сервис orchestrator-watchdog хук не пересобирает и не рестартит.
  • Следовательно правка pid: host (только в watchdog-сервисе) и весь код watchdog/** физически не участвуют в этом деплое и не могли дать exit 1.
  • Признаки в истории (chore: retrigger merge-gate re-test (flaked under host CPU starvation)) указывают на CPU-голодание хоста — ровно тот класс осиротевших pytest-процессов, который и закрывает ORCH-111. Это деплой-инфра/окружение, не дефект диффа.
  • Путь вперёд — повторный прод-деплой после стабилизации хоста (вне ответственности reviewer); блокировать review по инфра-фейлу деплоя нет оснований.

Findings

P0 — Blocker

  • Нет.

P1 — Must fix

  • Нет.

P2 — Should fix

  • Нет.

P3 — Nice to have (не блокируют)

  • watchdog/collectors/proc.py:202 — опечатка в комментарии top-level except: «one signal tih» → «one signal тих» (косметика).
  • core.py::_dispatch: после RECOVERY ключ ("proc_blocking", pid) остаётся в self._states с alerting=False навсегда (мелкое неограниченное накопление мёртвых-PID ключей за время жизни sidecar). Эффект пренебрежимо мал (запись крошечная; алертящий PID требует процесс >1ч matching pytest — событие исключительное), зеркалит существующий паттерн container_down; повторный RECOVERY не синтезируется (state.alerting уже False). Опционально — очищать ключ после RECOVERY.
  • Индекс docs/architecture/adr/README.md не содержит adr-0041предсуществующий долг (нет также adr-0038/0039/0040), не свойство этого PR и не per-PR-контракт. Не вменяется ORCH-111.

Документация

Документация обновлена корректно и в полном объёме в том же PR:

  • CHANGELOG.md — запись с разбивкой по D-решениям. ✓
  • docs/architecture/README.md — раздел proc_blocking в описании sidecar-watchdog. ✓
  • docs/deployment/LITE_SETUP.md — opt-in блок с «Проверкой» (NFR-5). ✓
  • docs/work-items/ORCH-111/06-adr/ADR-001-... + сквозной docs/architecture/adr/adr-0041-.... ✓
  • .env.watchdog.example ↔ блок WATCHDOG_* .env.example — синхронизированы (4 ключа), key-sync тест зелёный. ✓

src/** байт-в-байт не тронут → жёсткое правило P0 не активируется; функционал наблюдателя задокументирован, конвейерные контракты сохранены, выкат пересобирает только orchestrator-watchdog (прод orchestrator не рестартится, NFR-3). Документация = golden source: требование выполнено.