reviewer(ET): auto-commit from reviewer run_id=776
This commit is contained in:
118
docs/work-items/ORCH-126/12-review.md
Normal file
118
docs/work-items/ORCH-126/12-review.md
Normal file
@@ -0,0 +1,118 @@
|
||||
---
|
||||
verdict: APPROVED
|
||||
work_item: ORCH-126
|
||||
stage: review
|
||||
author_agent: reviewer
|
||||
status: approved
|
||||
created_at: 2026-06-17
|
||||
model_used: claude-opus-4-8
|
||||
type: review
|
||||
work_item_id: ORCH-126
|
||||
version: 1
|
||||
---
|
||||
|
||||
# Review ORCH-126 — гигиена run-ownership строки `jobs` (инвариант «queued ⇒ run_id/pid/started_at IS NULL»)
|
||||
|
||||
## Summary
|
||||
|
||||
Точечный, корректно-локализованный багфикс контрол-плейна (трек **Bug**, инцидент ORCH-124/125,
|
||||
job 2286). Реализация **полностью соответствует ТЗ/AC и ADR-001 + сквозному adr-0052**, не меняет
|
||||
схему БД, `STAGE_TRANSITIONS`, реестр `QG_CHECKS`, `check_*` и machine-verdict-ключи (AC-7), несёт
|
||||
обязательный регресс-тест красный→зелёный (AC-8/TC-01) и обновляет документацию в том же PR. Все
|
||||
4 маркированных инварианта (ORCH-065/113/114/099) сохранены и трассированы. **P0/P1 findings нет.**
|
||||
|
||||
Проверка выполнена по коммиту `d7e7a4d` (фактический объём ORCH-126); широкий `main...HEAD` раздут
|
||||
устаревшим локальным `main` и к ревью не относится.
|
||||
|
||||
### Соответствие ТЗ / AC (ось 1) — выполнено
|
||||
- **FR-1 / AC-1:** сброс `run_id=NULL, pid=NULL` той же UPDATE-транзакцией во всех 4 путях возврата
|
||||
в `queued` — `requeue_running_jobs` (`db.py:1506`), `mark_job('queued')` (`:1264`),
|
||||
`mark_job_transient` (`:1226`), `reap_running_job('queued')` (`:1668`). Атомарные `status`-guard'ы
|
||||
(`WHERE status='running'`, rowcount) сохранены байт-в-байт (TC-04 проверяет «второй reap проигрывает»).
|
||||
- **FR-2 / AC-3:** `claim_next_job` сбрасывает `pid=NULL, run_id=NULL` внутри существующего одного
|
||||
UPDATE флипа `queued→running`; SELECT-гейт не тронут (offline hot-path, NFR-2). Возврат — re-SELECT
|
||||
после UPDATE → отдаваемый dict корректно несёт `pid IS NULL` (TC-05).
|
||||
- **FR-3 / AC-6:** окно `_spawn` устойчиво за счёт D1 (нового кода в launcher не требуется); TC-09
|
||||
подтверждает чистый requeue + повторный claim без «частично стартовавшего» зависания.
|
||||
- **FR-4 / AC-5:** `find_impossible_queued_jobs` / `sanitize_impossible_queued` (идемпотентно,
|
||||
never-raise) + вызов при старте (`main.lifespan` после `requeue_running_jobs`) и на каждом
|
||||
реап-тике; счётчик `impossible_queued_total`/`last_impossible_queued` в блоке `reaper` снимка
|
||||
`GET /queue` (TC-08/08b, kill-switch проверен).
|
||||
- **FR-5 / AC-4:** reaper Tier-1 не правился (подтверждено diff'ом) — `if pid is not None and not
|
||||
pid_alive(pid)` пропускает `pid IS NULL`, `else` сбрасывает streak (`job_reaper.py:290/302`);
|
||||
Tier-3 backstop неизменен. TC-07 фиксирует «свежий running с pid IS NULL не реапится».
|
||||
- **AC-7:** схема БД (`jobs`) без изменений, новых колонок/таблиц нет; для здоровых job'ов поведение
|
||||
байт-в-байт (TC-10 — терминальные исходы и `run_id`-линк не затронуты); enduro не затронут.
|
||||
|
||||
### Соответствие ADR / трассировка (ось 2) — выполнено
|
||||
- ADR-001 (`06-adr/`) + сквозной `adr-0052` присутствуют, согласованы с кодом, статус `accepted`.
|
||||
- Маркированные инварианты (CLAUDE.md §9): **ORCH-065** Tier-1 — восстановлен и не изменён;
|
||||
**ORCH-113** finalizer-liveness и **ORCH-114** transition-lease — ортогональны (свои маркеры/таблицы,
|
||||
recovery по `boot_id`, не по `jobs.pid`); **ORCH-099** `/metrics` — улучшен (убрана утечка stale-pid).
|
||||
Проверено: коммит **не трогает** `_reap_job`/`pid_alive` — только аддитивный sanitize-метод/вызов/поля
|
||||
`status()`.
|
||||
- **Багфикс-трек (ORCH-019, BR-4):** обязательный регресс-фиксатор присутствует — **TC-01** красный на
|
||||
коде до фикса (старый `requeue_running_jobs` оставлял `run_id=759`), зелёный после.
|
||||
|
||||
### Качество кода (ось 3) — приемлемо
|
||||
- never-raise соблюдён: sanitize обёрнут в `try/except` в reaper и в `main.lifespan`; db-функции —
|
||||
`try/finally` на соединении.
|
||||
- `mark_job`/`reap_running_job`: каллер-переданный `run_id` для `status='queued'` корректно
|
||||
игнорируется (`if run_id is not None and status != "queued"`) и принудительно зануляется в
|
||||
`elif`-ветке — без конфликта двойного SET.
|
||||
- Docstrings на новых публичных функциях есть; тесты содержательные (seeded-DB, без сети/Popen).
|
||||
- env-маппинг корректен: `impossible_queued_sanitize_enabled` ↔ `ORCH_IMPOSSIBLE_QUEUED_SANITIZE_ENABLED`
|
||||
(env_prefix `ORCH_`), флаг задокументирован в `.env.example`.
|
||||
|
||||
### Документация (ось 4, приоритет) — выполнено
|
||||
`src/` изменён → документация обновлена в том же PR:
|
||||
- `docs/architecture/internals.md` — новый раздел «Инвариант run-ownership строки `jobs`» +
|
||||
аннотации `jobs.run_id`/`pid` + queue-recovery (корректный дом для внутренностей очереди/reaper).
|
||||
- `.env.example` — флаг `ORCH_IMPOSSIBLE_QUEUED_SANITIZE_ENABLED` в блоке reaper.
|
||||
- `CHANGELOG.md` — детальная запись (D1–D5 + покрытие).
|
||||
- ADR-001 + сквозной `adr-0052` — заведены.
|
||||
- README API-таблица **не требует** правки: новых эндпоинтов нет (TRZ §4) — лишь read-only под-поле
|
||||
в существующем блоке `reaper` снимка `GET /queue` (паттерн прошлых reaper-метрик, напр.
|
||||
`finalizer_defers_total` ORCH-113).
|
||||
- Витрина `docs/overview/` (ORCH-011) **не требует** правки: grep по витрине не нашёл упоминаний
|
||||
run-ownership/reaper/impossible-queued — фикс внутренней гигиены данных не меняет описанных в
|
||||
витрине стадий/гейтов/агентов/интеграций; ничего не выдаётся за устаревшее.
|
||||
- Обзорный `README.md` «Известные ограничения» (ORCH-079): данный дефект там не числился пунктом —
|
||||
обновления не требуется.
|
||||
|
||||
## Findings
|
||||
|
||||
### P0 — Blocker
|
||||
- Нет.
|
||||
|
||||
### P1 — Must fix
|
||||
- Нет.
|
||||
|
||||
### P2 — Should fix
|
||||
- Нет.
|
||||
|
||||
### P3 — Nice to have (не влияет на вердикт)
|
||||
- [ ] `sanitize_impossible_queued()` (`db.py`) делает `find_*` (откр/закр соединение) и затем
|
||||
**отдельный** UPDATE в новом соединении; возвращаемый `anomalies` — pre-heal снимок. При редкой
|
||||
гонке (claim флипнул строку `queued→running` между find и UPDATE) защитный `WHERE status='queued'`
|
||||
корректно пропустит строку, но функция всё равно отчитается о ней как «исцелённой» → косметический
|
||||
пере-подсчёт счётчика `impossible_queued_total`. Безвреден и идемпотентен; при желании — считать по
|
||||
фактическому `rowcount` UPDATE.
|
||||
- [ ] В коммит попал рабочий файл-черновик `.task-dev.md` (`ORCH-124 → ORCH-126`). Это **не** регресс
|
||||
данного PR — файл коммитится во всех прошлых задачах, влитых в `main` (ORCH-124/116/115/112…);
|
||||
housekeeping-замечание: уместно добавить `.task-dev.md`/`.task-review.md` в `.gitignore` отдельной
|
||||
задачей.
|
||||
|
||||
## Документация
|
||||
**Обновлена в том же PR — требование выполнено.** `internals.md` (раздел инварианта + аннотации
|
||||
колонок + queue-recovery), `.env.example` (новый флаг), `CHANGELOG.md`, ADR-001 + сквозной `adr-0052`.
|
||||
README API-таблица и витрина `docs/overview/` правки **не требуют** (новых эндпоинтов нет — read-only
|
||||
под-поле существующего блока `reaper`; в витрине нет затронутых фактов). Документация консистентна с
|
||||
реализацией; стейл-ссылок не обнаружено.
|
||||
|
||||
## Проверка тестами
|
||||
- `pytest tests/test_orch126_queued_stale_run.py -q` → **11 passed**.
|
||||
- Регресс смежных подсистем: `test_orch113_reaper_finalizer_liveness.py`,
|
||||
`test_orch114_transition_ownership.py`, `test_webhooks.py` → **63 passed**.
|
||||
- TC-01 — обязательный регресс красный→зелёный (подтверждено по семантике до-фиксового
|
||||
`requeue_running_jobs`).
|
||||
Reference in New Issue
Block a user