diff --git a/docs/work-items/ORCH-065/12-review.md b/docs/work-items/ORCH-065/12-review.md index 818dce7..0366741 100644 --- a/docs/work-items/ORCH-065/12-review.md +++ b/docs/work-items/ORCH-065/12-review.md @@ -1,8 +1,8 @@ --- type: review work_item_id: ORCH-065 -verdict: REQUEST_CHANGES -version: 2 +verdict: APPROVED +version: 3 --- # Review ORCH-065 @@ -11,24 +11,38 @@ version: 2 Задача закрывает три связанных класса отказов «процесс/поток умер, а ресурс остался захваченным навсегда»: zombie jobs (A), залипший merge-lease (B), неидемпотентная -финализация merge (C). Реализация в целом качественная: новый daemon-поток -`src/job_reaper.py` по образцу `reconciler` (never-raise, kill-switch, снимок в -`/queue`), трёхуровневая liveness, атомарный `reap_running_job(... WHERE status='running')`, -проактивный реклейм lease (`pid_alive` + `reclaim_stale_lease`), идемпотентный guard -`pr_already_merged`, колонка `jobs.pid` через идемпотентный `_ensure_column`. Инварианты -сохранены: `STAGE_TRANSITIONS`/`QG_CHECKS`/`check_*`/БАГ-8/exit-коды хука не тронуты -(коммиты ORCH-065 не касаются `stages.py`/`qg/checks.py`/`stage_engine.py`); реклейм -lease — только удаление файла, без git-операций. Документация (README, internals, ADR, -глобальный adr-0011, CHANGELOG, .env.example) обновлена в этом же PR. `pytest tests/ -q` -— **743 passed**. +финализация merge (C). Реализация качественная: новый daemon-поток `src/job_reaper.py` +по образцу `reconciler` (never-raise, kill-switch, снимок в `/queue`), трёхуровневая +liveness, атомарный `reap_running_job(... WHERE status='running')`, проактивный реклейм +lease (`pid_alive` + `reclaim_stale_lease`), идемпотентный guard `pr_already_merged`, +колонка `jobs.pid` через идемпотентный `_ensure_column`. -Прошлый блокер v1 (guard `pr_already_merged` не подключён к пути merge) **устранён** -коммитом `aa46e5d`: промпт `.openclaw/agents/deployer.md` теперь предписывает консультировать -`pr_already_merged` ПЕРЕД любым (повторным) merge — AC-11 wiring на месте. +**Все блокеры предыдущих ревью устранены:** +- v1 P0 (guard `pr_already_merged` не подключён к merge-пути) — устранён `aa46e5d`: + промпт `.openclaw/agents/deployer.md` консультирует `pr_already_merged` ПЕРЕД любым + (повторным) merge (AC-11 wiring на месте, подтверждено строками 94–105/152). +- v2 P1 (Tier-2 реапит живой финализирующий monitor; side-effects ДО атомарного claim, + нарушение ADR-001 Р-1) — устранён `3e2eb27` двумя мерами: + 1. **Tier-2 finalization grace** — новая колонка `finished_age_s` в `get_running_jobs` + (`src/db.py:609`) + настройка `reaper_finalize_grace_s` (дефолт 300с); Tier-2 + реапит только при `finished_age >= grace`, иначе строка не трогается + (`src/job_reaper.py:197-209`). Живой финализирующий monitor больше не реапится + (FR-1.3/AC-3). + 2. **claim-before-act** — `_reap_exit0` (`src/job_reaper.py:242-286`) сначала оценивает + канонический QG read-only (`_gate_is_green` → `_run_qg`, без побочных эффектов), + затем атомарно claim `done` ПЕРВЫМ, и только победитель claim выполняет + `_gate_driven_advance`. Проигравший гонку (поздний monitor / стартовый requeue) не + делает НИКАКИХ побочных эффектов → нет дубль-advance/дубль-enqueue (FR-1.2/AC-4). +- v2 P3 (битая ссылка на adr-0011 в CHANGELOG) — исправлена в `3e2eb27` + (`adr-0011-job-reaper-lease-reclaim.md`). -Новый блокер: гонка Tier-2 reaper'а с живым monitor-потоком при штатной финализации — -порядок «side effects → atomic claim» нарушает собственный контракт ADR-001 Р-1 и может -дать дубль-advance / дубль-enqueue следующей стадии (FR-1.2/FR-1.3/AC-3/AC-4). +Инварианты сохранены (AC-13): ORCH-065-коммиты (`1a2e881`/`aa46e5d`/`3e2eb27`) НЕ касаются +`src/stages.py` и `src/qg/checks.py` — `STAGE_TRANSITIONS`/`QG_CHECKS`/`check_*`/БАГ-8/ +exit-коды хука не тронуты; реклейм lease — только удаление файла, без git-операций +(AC-12). Документация (README, internals, ADR-001, глобальный adr-0011, CHANGELOG, +.env.example) обновлена в этом же PR (AC-17). Новые тесты покрывают grace-окно, +lost-claim-no-side-effects, already-advanced-идемпотентность. `pytest tests/ -q` — +**747 passed**. ## Findings @@ -36,70 +50,21 @@ lease — только удаление файла, без git-операций. - нет ### P1 — Must fix -- [ ] **Tier-2 реапит живой, штатно финализирующийся job; side-effects идут ДО - атомарного claim (нарушение ADR-001 Р-1, риск дубль-advance/дубль-enqueue).** - В `JobReaper._reap_job` (`src/job_reaper.py:177`) ветка Tier-2 срабатывает на ЛЮБОЙ - `running`-job, у которого в `agent_runs` уже записан `exit_code` — **без grace и без - какого-либо признака смерти monitor'а**. Но именно это состояние («exit_code записан, - job ещё running») — нормальное окно финализации живого monitor-потока: `_monitor_agent` - пишет `exit_code`, затем выполняет git commit/push (+PR), БАГ-8-проверку, **сетевые - usage-комментарии в Plane** (секунды-десятки секунд), и лишь потом `_try_advance_stage` - → `_finalize_job`. pid агента (`jobs.pid`) к этому моменту уже мёртв (процесс завершён - до записи exit_code), поэтому отличить «monitor умер» от «monitor жив и финализирует» - по pid невозможно, а reaper тикает каждые `reaper_interval_s` (60с). Доступный для - grace `finished_at_run` запрашивается в `get_running_jobs`, но **не используется** - (помечен «debug only», `src/db.py`). - - Усугубляет дефект порядок в `_reap_known_outcome` (`src/job_reaper.py:206`): для exit0 - сначала вызывается `_gate_driven_advance(job)` (побочные эффекты — `_try_advance_stage` - → `advance_stage` → `enqueue_job` следующей стадии), и **только потом** атомарный - `reap_running_job(..., "done")`. Это прямо противоречит ADR-001 Р-1: «Атомарный - reap-claim. **Перед любым действием с побочными эффектами** reaper атомарно - «застолбляет» строку тем же приёмом, что `claim_next_job`». Поскольку claim стоит - ПОСЛЕ side-effects, его guard `WHERE status='running'` не сериализует advance: даже - проигравший гонку reaper (rowcount==0) уже успел выполнить `advance_stage`. - - Последствия в окне гонки (reaper-тик попал в финализацию живого monitor'а): - - `enqueue_job` (`src/db.py:419`) — обычный INSERT **без дедупликации**, поэтому - параллельные `advance_stage` от reaper и monitor дают **две `queued`-строки - следующего агента** → дублирующий запуск следующей стадии (двойной commit/PR/ - комментарии, лишние токены); - - либо (если гейт следующего ребра ещё красный, напр. CI не позеленел) reaper уходит - в `_reap_unknown_outcome` → `reap_running_job(..., "queued")` и **спихивает в queued - job, который вот-вот успешно завершится** monitor'ом; при `max_concurrency=1` воркер - может повторно заклеймить и **перезапустить тот же агент**. - - Это противоречит FR-1.3/AC-3 (живой агент НЕ должен реапиться) и FR-1.2/AC-4 - (никакого дубль-advance). Требуется одно из: - 1. ввести **grace для Tier-2** — реапить только если `finished_at_run` старше - заведомо-большего, чем максимальное окно финализации (использовать уже - запрашиваемый `finished_at_run`), и/или - 2. **claim-before-act**: атомарно «застолбить» строку (как требует ADR Р-1) ДО любого - `advance_stage`/`enqueue_job`, чтобы проигравший гонку reaper не выполнял побочных - эффектов; advance — только после выигранного claim. - - (Tier-1 этим не страдает: streak `reaper_dead_ticks` + мёртвый pid; ветка - `_reap_unknown_outcome` без exit0 тоже безопасна — единственное действие там и есть - атомарный flip.) +- нет ### P2 — Should fix - нет ### P3 — Nice to have -- [ ] **Битая ссылка в CHANGELOG на глобальный ADR.** В записи ORCH-065 указан - `docs/architecture/adr/adr-0011-job-reaper-and-lease-reclaim.md`, фактический файл — - `adr-0011-job-reaper-lease-reclaim.md` (без `-and-`). README и `src/job_reaper.py` - ссылаются корректно. Поправить путь в `CHANGELOG.md`. +- нет ## Документация -Обновлена корректно и в этом же PR (AC-17 PASS по составу): `docs/architecture/README.md` -(раздел про job-reaper + lease-reclaim, таблицы БД и `/queue`), `docs/architecture/internals.md`, -`docs/architecture/adr/adr-0011-job-reaper-lease-reclaim.md` (+ запись в `adr/README.md`), -`docs/work-items/ORCH-065/06-adr/ADR-001-job-reaper-and-lease-reclaim.md`, `CHANGELOG.md`, -`.env.example` (флаги `ORCH_REAPER_*` / `ORCH_LEASE_RECLAIM_ENABLED`). - -Оговорка: ADR-001 Р-1 описывает «claim перед побочными эффектами», но реализация exit0-пути -этому не следует (см. P1) — при исправлении кода либо привести код в соответствие с ADR, -либо синхронизировать формулировку ADR с фактическим порядком. Битая ссылка на adr-0011 в -CHANGELOG — P3. +Обновлена корректно и в этом же PR (AC-17 PASS): `docs/architecture/README.md` +(раздел про job-reaper + lease-reclaim, таблицы БД и `/queue`), +`docs/architecture/internals.md`, `docs/architecture/adr/adr-0011-job-reaper-lease-reclaim.md` +(+ запись в `adr/README.md`), +`docs/work-items/ORCH-065/06-adr/ADR-001-job-reaper-and-lease-reclaim.md`, `CHANGELOG.md` +(ссылка на adr-0011 исправлена), `.env.example` (флаги `ORCH_REAPER_*` / +`ORCH_REAPER_FINALIZE_GRACE_S` / `ORCH_LEASE_RECLAIM_ENABLED`). ADR-001 Р-1 и реализация +exit0-пути теперь согласованы (claim-before-act).