From 9b7c855df374dd711f87fe4a91a162ffa461aede Mon Sep 17 00:00:00 2001 From: claude-bot Date: Sun, 7 Jun 2026 15:52:26 +0000 Subject: [PATCH] reviewer(ET): auto-commit from reviewer run_id=321 --- docs/work-items/ORCH-065/12-review.md | 126 ++++++++++++++++---------- 1 file changed, 76 insertions(+), 50 deletions(-) diff --git a/docs/work-items/ORCH-065/12-review.md b/docs/work-items/ORCH-065/12-review.md index ee50896..818dce7 100644 --- a/docs/work-items/ORCH-065/12-review.md +++ b/docs/work-items/ORCH-065/12-review.md @@ -2,7 +2,7 @@ type: review work_item_id: ORCH-065 verdict: REQUEST_CHANGES -version: 1 +version: 2 --- # Review ORCH-065 @@ -11,20 +11,24 @@ version: 1 Задача закрывает три связанных класса отказов «процесс/поток умер, а ресурс остался захваченным навсегда»: zombie jobs (A), залипший merge-lease (B), неидемпотентная -финализация merge (C). Реализация качественная: новый daemon-поток `src/job_reaper.py` -по образцу `reconciler` (never-raise, kill-switch, снимок в `/queue`), трёхуровневая -liveness (Tier-1 мёртвый pid + streak, Tier-2 exit_code, Tier-3 backstop), атомарный -reap-claim `reap_running_job(... WHERE status='running')`, gate-driven advance через -неизменный `_try_advance_stage`, проактивный реклейм lease (`pid_alive` + -`reclaim_stale_lease`), колонка `jobs.pid` через идемпотентный `_ensure_column`. -Инварианты соблюдены: `STAGE_TRANSITIONS`/`QG_CHECKS`/`check_*`/БАГ-8/exit-коды хука — -без изменений; реклейм lease — только удаление файла, без git-операций; прод-контейнер -не трогается. Документация (README, internals, ADR, глобальный adr-0011, CHANGELOG, -.env.example) обновлена в этом же PR. `pytest tests/ -q` — **742 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`. Инварианты +сохранены: `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**. -Блокер один: дефект Проблемы C (FR-3.2 / AC-11) — детерминированный guard -`pr_already_merged` реализован и юнит-протестирован, но **не подключён ни к одному -реальному пути merge**, при этом ADR и golden-source-документация утверждают обратное. +Прошлый блокер v1 (guard `pr_already_merged` не подключён к пути merge) **устранён** +коммитом `aa46e5d`: промпт `.openclaw/agents/deployer.md` теперь предписывает консультировать +`pr_already_merged` ПЕРЕД любым (повторным) merge — AC-11 wiring на месте. + +Новый блокер: гонка Tier-2 reaper'а с живым monitor-потоком при штатной финализации — +порядок «side effects → atomic claim» нарушает собственный контракт ADR-001 Р-1 и может +дать дубль-advance / дубль-enqueue следующей стадии (FR-1.2/FR-1.3/AC-3/AC-4). ## Findings @@ -32,48 +36,70 @@ reap-claim `reap_running_job(... WHERE status='running')`, gate-driven advance - нет ### P1 — Must fix -- [ ] **`pr_already_merged` — мёртвый код в проде; ADR/доки утверждают, что он - консультируется (FR-3.2 / AC-11, ADR-001 Р-3).** Функция `merge_gate.pr_already_merged` - определена (`src/merge_gate.py:447`) и покрыта unit-тестами (TC-16/TC-17), но grep по - `src/` показывает **ноль вызовов** из продакшен-кода: ни в `stage_engine.py`, ни в - `webhooks/gitea.py`, ни в `qg/checks.py`, ни в промпте `.openclaw/agents/deployer.md`. - Фактический merge PR в `main` выполняет LLM-агент deployer, и его промпт guard не - упоминает. Тем временем: - - ADR-001 Р-3: «Путь слияния (deployer/merge) **консультируется** с этим guard ПЕРЕД - повторным merge: PR уже слит → no-op (без второго merge и без ошибки)»; - - `docs/architecture/README.md:226`: «добавлен never-raise guard `pr_already_merged` … - уже слит = no-op»; - - CHANGELOG: «`pr_already_merged(...)` — guard **перед повторным merge** при re-drive». +- [ ] **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`). - Следствие: при re-drive (reaper → `queued` → переисполнение стадии `deploy`) с уже - слитым PR гарантия AC-11 «без второго слияния» **не обеспечена детерминированно** — - deployer-агент повторно попытается слить уже слитый PR (Gitea вернёт ошибку - merge → риск ложного БАГ-8 отката). `branch_is_behind_main`-короткое замыкание - (TC-17) делает идемпотентным лишь дорогой rebase+re-test merge-гейта, но НЕ сам акт - слияния. Это прямое расхождение реализации с ADR и golden-source. + Усугубляет дефект порядок в `_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`. - Требуется одно из: - 1. **Подключить guard** к пути merge (инструкция в промпте deployer «перед merge - проверь `pr_already_merged` — уже слит → пометь стадию успешной без повторного - merge», либо детерминированная Python-проверка перед merge), чтобы AC-11 - выполнялся реально; **либо** - 2. если идемпотентность намеренно достигается только gate-driven advance + - `branch_is_behind_main` (а `pr_already_merged` оставлен как helper «на будущее»), - **исправить ADR-001 Р-3, README и CHANGELOG**, убрав утверждение, что путь merge - консультируется с guard, и явно описав, чем именно покрывается «без второго - слияния» из AC-11. + Последствия в окне гонки (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-*.md` -(+ запись в `adr/README.md`), `docs/work-items/ORCH-065/06-adr/ADR-001-*.md`, -`CHANGELOG.md`, `.env.example` (флаги `ORCH_REAPER_*` / `ORCH_LEASE_RECLAIM_ENABLED`). +Обновлена корректно и в этом же 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`). -Оговорка: содержание документации про `pr_already_merged` фактически неверно (см. P1) — -guard описан как «консультируемый перед merge», тогда как он нигде не вызывается. -До устранения P1 документация в этой части не является достоверным golden-source. +Оговорка: ADR-001 Р-1 описывает «claim перед побочными эффектами», но реализация exit0-пути +этому не следует (см. P1) — при исправлении кода либо привести код в соответствие с ADR, +либо синхронизировать формулировку ADR с фактическим порядком. Битая ссылка на adr-0011 в +CHANGELOG — P3.