diff --git a/docs/work-items/ORCH-111/12-review.md b/docs/work-items/ORCH-111/12-review.md index 52252e2..efa39f9 100644 --- a/docs/work-items/ORCH-111/12-review.md +++ b/docs/work-items/ORCH-111/12-review.md @@ -8,80 +8,116 @@ created_at: 2026-06-15 model_used: claude-opus-4-8 type: review work_item_id: ORCH-111 -version: 1 +version: 2 --- # Review ORCH-111 ## Summary -PR добавляет в sidecar-watchdog новый opt-in сигнал `proc_blocking` — алерт на долго живущий -осиротевший тест-процесс (pytest), репарентированный на хост и грузящий CPU (инцидент: -`test_install_lite_script.py` жил >2 суток без единого алерта, валил merge-gate re-test на ORCH-109). +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`). -Изменение **строго внутри наблюдателя** (`watchdog/**` + сервис watchdog в `docker-compose.yml`). -Проверено по всем 4 осям — **замечаний уровня P0/P1 нет**. Реализация 1:1 соответствует ТЗ и ADR-001 -(D1–D7), полный `pytest tests/` зелёный (**1933 passed**), watchdog+key-sync тесты зелёные -(125 passed), `src/**` байт-в-байт не тронут, конвейерные контракты сохранены, документация -обновлена в том же PR. **Вердикт: APPROVED.** +Проверено по всем 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 (чистый builder `proc_signals`, per-entity key `("proc_blocking", pid)`, active ⇔ - `age_s > cfg.proc_age_s`, действенный RU-`detail`: PID + возраст + усечённый cmdline + CPU) — ✓. -- FR-2 (коллектор `watchdog/collectors/proc.py`, stdlib-only, read-only, never-raise → `[]`, чистый - разбор отделён от I/O) — ✓. -- FR-3 (4 ключа `WATCHDOG_PROC_*`, never-raise парсеры, дефолт-off, гейт на `proc_enabled`) — ✓. -- FR-4 (только наблюдение — нет `os.kill`/сигналов/`subprocess`/чтения `/proc//environ`), - подтверждено AST-тестом `test_tc04_collector_source_is_read_only` — ✓. -- FR-5 (диспетч через существующую `decision.decide()`/`AlertState`; RECOVERY синтезируется без новой - анти-спам-логики) — ✓. -- FR-6 (без дубля с `agent_hung` — по построению: cmdline-скоуп + порог возраста) — ✓. -- AC-1…AC-10 покрыты тестами (TC-01 regress-якорь, TC-02 порог/граница, TC-03 конфиг/kill-switch, - TC-04 never-raise/read-only, TC-05 anti-spam→realert→recovery, TC-06 partition vs agent_hung, - TC-07 tick→dispatch + flag-off + never-raise, TC-12 compose `pid: host`). -- §4 (API): выбран watchdog-side путь, `GET /metrics`/`schema_version` не тронуты — ✓. +- 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//environ`; + независимо подтвердил grep'ом: совпадения только в докстрингах-контракте) — ✓. +- FR-5 (диспетч `core.tick()` через существующую `decision.decide()`/`AlertState`; RECOVERY + синтезируется `_synthesize_proc_recoveries` без новой анти-спам-логики — per-family bookkeeping) — ✓. +- FR-6 (без дубля с `agent_hung` — по построению: cmdline-скоуп `pytest` ≠ `claude` + порог возраста + выше бюджета; кросс-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 — ✓. + coverage_run_timeout_s=900)=900s` (4× запас) — кросс-инвариант D2 соблюдён (анти-false-positive AC-4). **2. Соответствие ADR — PASS.** -- Реализация соответствует ADR-001 D1 (watchdog-side `/proc` под `pid: host`, НЕ orch-side `/metrics`), - D2 (анти-FP/анти-дубль по построению), D3 (коллектор), D4 (builder + синтез RECOVERY), D5 (конфиг - дефолт-off), D6 (`pid: host` только на `orchestrator-watchdog`), D7 (инварианты конвейера). -- Сквозной ADR `docs/architecture/adr/adr-0041-...` создан (архитектором, commit `1d87ae5`). -- **Трассировка (TRACEABILITY.md):** правки `watchdog/{core,signals,config}.py` и сервиса watchdog в - compose — **аддитивные врезки**, маркированные инварианты предшественников (ORCH-100 sidecar, - read-only-маунты) не сломаны. Существующий тест `test_host_paths_mounted_read_only` остаётся зелёным - (`pid` — не volume), плюс добавлен позитивный `test_watchdog_shares_host_pid_namespace` (и проверка, - что прод `orchestrator` НЕ получает `pid`). +- Реализация 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) — содержательные, со ссылками на - D-решения ADR. -- Тесты содержательные, не тривиальные: red→green regress-якоря (TC-01 builder, TC-07 tick→dispatch - «алерт даже когда ни одна стадия не stuck»), граничные (strict `>`), патологический `comm` со - скобками/пробелами, гонка «процесс умер mid-scan», AST-скан read-only-контракта, полная - последовательность alert→none→realert→recovery→no-repeat. +- 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`), но как баг несёт явные тест-фиксаторы дефекта — требование выполнено. -- Безопасность: read-only, never-raise (per-pid + top-level), `/proc//environ` не читается - (секреты), cmdline усечена в алерте (анти-утечка аргументов, R-2). Привилегия `pid: host` — - read-only, только у наблюдателя, обоснована в `07-infra-requirements.md`. + (прошла `architecture`, не bug-fast-track), но несёт явные тест-фиксаторы дефекта (TC-01 builder, + TC-07 tick→dispatch — оба red→green) — требование «фикс кода несёт тест-фиксатор» выполнено. +- Безопасность: read-only (per-pid + top-level guard), never-raise, `/proc//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` (key-sync тест `test_lite_setup_doc.py` - зелёный). -- Стадийное владение артефактами соблюдено (rule 3/4): TRZ/AC/test-plan — analyst (`a0218a2`), - ADR/infra/risks/architecture-README — architect (`1d87ae5`), код/CHANGELOG/env/compose/LITE_SETUP — - developer (`6c83191`). Ретроактивных правок ТЗ/ADR нет. + обновлены в том же 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 @@ -95,27 +131,26 @@ PR добавляет в sidecar-watchdog новый opt-in сигнал `proc_b - Нет. ### P3 — Nice to have (не блокируют) -- [ ] Опечатка в комментарии `watchdog/collectors/proc.py` (top-level `except`): «one signal tih» → - «one signal **тих**» (косметика). -- [ ] Витрина `docs/overview/` (ORCH-011): описывает sidecar-watchdog абстрактно («опрашивает - `/metrics`, шлёт алерты»), каталог сигналов не перечисляет. Новый opt-in (дефолт-off) сигнал - абстрактную формулировку не противоречит, машинно-проверяемые факты не меняет - (`tests/test_system_docs.py` зелёный) → обновление витрины **не требуется**. Опционально можно - упомянуть новую способность наблюдателя (видимость хост-процессов) — не блокирует. -- [ ] Индекс `docs/architecture/adr/README.md` не содержит `adr-0041` — но это **предсуществующий - долг** (отсутствуют также `adr-0038`/`0039`/`0040` из 3 ранее влитых PR), не свойство этого PR и не - per-PR-контракт. Не вменяется ORCH-111. +- [ ] `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` — запись `[Unreleased]` с разбивкой по D-решениям. ✓ +- `CHANGELOG.md` — запись с разбивкой по D-решениям. ✓ - `docs/architecture/README.md` — раздел `proc_blocking` в описании sidecar-watchdog. ✓ -- `docs/deployment/LITE_SETUP.md` — opt-in блок с проверкой (NFR-5). ✓ +- `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` ↔ `.env.example` (блок `WATCHDOG_*`) — синхронизированы, key-sync тест - зелёный. ✓ +- `.env.watchdog.example` ↔ блок `WATCHDOG_*` `.env.example` — синхронизированы (4 ключа), key-sync + тест зелёный. ✓ -Изменения функционала наблюдателя задокументированы, конвейерные контракты не тронуты, выкат -пересобирает только `orchestrator-watchdog` (прод `orchestrator` не рестартится, NFR-3). -**Документация = golden source: требование выполнено.** +`src/**` байт-в-байт не тронут → жёсткое правило P0 не активируется; функционал наблюдателя +задокументирован, конвейерные контракты сохранены, выкат пересобирает только `orchestrator-watchdog` +(прод `orchestrator` не рестартится, NFR-3). **Документация = golden source: требование выполнено.**