Files
orchestrator/docs/work-items/ORCH-126/12-review.md
claude-bot c8632f4b48
All checks were successful
CI / test (push) Successful in 1m18s
CI / test (pull_request) Successful in 1m14s
reviewer(ET): auto-commit from reviewer run_id=776
2026-06-17 11:47:05 +03:00

9.7 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-126 review reviewer approved 2026-06-17 claude-opus-4-8 review ORCH-126 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 путях возврата в queuedrequeue_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_enabledORCH_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 — детальная запись (D1D5 + покрытие).
  • 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 -q11 passed.
  • Регресс смежных подсистем: test_orch113_reaper_finalizer_liveness.py, test_orch114_transition_ownership.py, test_webhooks.py63 passed.
  • TC-01 — обязательный регресс красный→зелёный (подтверждено по семантике до-фиксового requeue_running_jobs).