120 lines
8.8 KiB
Markdown
120 lines
8.8 KiB
Markdown
---
|
||
verdict: APPROVED # APPROVED | REQUEST_CHANGES — строго одно из двух, UPPERCASE
|
||
work_item: ORCH-114
|
||
stage: review
|
||
author_agent: reviewer
|
||
status: approved
|
||
created_at: 2026-06-15
|
||
model_used: claude-opus-4-8
|
||
type: review
|
||
work_item_id: ORCH-114
|
||
version: 2
|
||
---
|
||
|
||
# Review ORCH-114 — Durable transition-ownership lease + expected-stage CAS
|
||
|
||
## Summary
|
||
|
||
Повторное ревью (цикл 2). Прошлый вердикт был `REQUEST_CHANGES` из-за **частичной
|
||
незавершённости документации golden-source** (два P1 + три P2/P3). Кода-корректность и
|
||
инварианты конвейера уже тогда были без P0/P1. В этом цикле developer **закрыл все ранее
|
||
поднятые findings**:
|
||
|
||
- **P1 `.env.example`** → закрыт: добавлены `ORCH_TRANSITION_LEASE_ENABLED=true` /
|
||
`ORCH_TRANSITION_LEASE_REPOS=` с подробной нормативной шапкой (рядом с блоком reaper-флагов).
|
||
- **P1 `CLAUDE.md`** → закрыт: добавлена полноценная паспорт-секция «Единое владение
|
||
side-effectful переходами: durable-lease + expected-stage CAS (ORCH-114)» (механизм, два
|
||
слоя, инвариант, флаги, ADR-ссылки) — по образцу ORCH-112/113.
|
||
- **P2 витрина `docs/overview/`** → закрыта: `tech-data-model.md` (строка таблицы
|
||
`transition_lease`), `tech-observability.md` (упоминание блока `/queue`),
|
||
`tech-architecture.md` (компонент `Transition-lease`).
|
||
- **P2 расхождение код↔ADR D4 (CAS на rollback-записях)** → закрыто: введён общий хелпер
|
||
`_rollback_stage_cas`, и все четыре in-region rollback-записи (`_handle_merge_gate_rollback`/
|
||
`_handle_security_gate`/`_handle_coverage_gate`/`_handle_image_freshness`) теперь пишут
|
||
`development` через тот же expected-stage CAS. Реализация совпала с собственным ADR D4;
|
||
добавлены целевые тесты (TC-11 `test_tc11_inregion_rollback_writes_use_cas`,
|
||
`test_tc11_rollback_cas_lost_aborts_without_overwriting_done`).
|
||
- **P2 изоляция тестов** → закрыта: убран module-level `os.environ.setdefault("ORCH_DB_PATH")`,
|
||
`test_webhooks.py` пинит собственный реестр per-test, добавлен autouse-fixture
|
||
`_disable_transition_lease` (kill-switch off для всего suite по образцу `_disable_merge_verify`).
|
||
|
||
Архитектура реализована **в точном соответствии с ADR-001 D1–D10**: durable-lease на входе
|
||
(`acquire`/`release` в `try/finally`) + expected-stage CAS на записи (включая 6 путей в обход
|
||
`advance_stage`), liveness по `pid`+`boot_id` без heartbeat, реклейм при рестарте
|
||
(`recover_on_startup` в `main.lifespan`), reaper/reconciler/webhook defer при живом владельце,
|
||
Tier-3 backstop добивает зависшего. `src/transition_lease.py` — чистый never-raise leaf
|
||
(импортирует только `db`+`config`, лениво `merge_gate.pid_alive`/`qg.checks`/`notifications`).
|
||
|
||
**Инварианты конвейера (AC-11):** `src/stages.py` и `src/qg/checks.py` **не тронуты** (нет в
|
||
диффе), `STAGE_TRANSITIONS`/`QG_CHECKS` встречаются в src-диффе только в комментарии;
|
||
machine-verdict ключи не тронуты; БД аддитивна (одна таблица `transition_lease`,
|
||
`CREATE TABLE IF NOT EXISTS`, без epoch-колонки). hot-path `claim_next_job` lease не
|
||
консультирует (AC-10/fail-open).
|
||
|
||
**Багфикс-трек (ORCH-019/BR-4):** задача `bug→escalate full-cycle`. Регресс-тест-фиксатор
|
||
**присутствует**: `test_tc01_concurrent_entry_no_double_effect` (зелёный с lease) +
|
||
`test_tc01_red_before_fix_demonstration` (красный при kill-switch off — второй актор
|
||
переисполняет все под-гейты, воспроизводя класс ORCH-111). Требование выполнено.
|
||
|
||
**Прогоны (verified):**
|
||
- `pytest tests/test_orch114_transition_ownership.py tests/test_webhooks.py` — **50 passed**
|
||
(именно та комбинация двух модулей, что в прошлом цикле давала 4 падения — изоляция
|
||
подтверждённо починена).
|
||
- `pytest tests/` (полный) — **2052 passed** в детерминированном порядке → AC-9 (байт-в-байт
|
||
при kill-switch off) и CI-green выполняются операционно; ORCH-113-тесты в наборе зелёные
|
||
(контракт предшественника не сломан, ORCH-078).
|
||
|
||
## Findings
|
||
|
||
### P0 — Blocker
|
||
- _(нет)_
|
||
|
||
### P1 — Must fix
|
||
- _(нет)_
|
||
|
||
### P2 — Should fix
|
||
- _(нет)_
|
||
|
||
### P3 — Nice to have
|
||
- [ ] **Merge-lease не освобождается на (практически недостижимой) ветке CAS-lost в
|
||
rollback'е coverage/image-freshness.** В `_handle_coverage_gate`/`_handle_image_freshness`
|
||
`release_merge_lease` стоит **после** `_rollback_stage_cas`, поэтому при проигранном CAS
|
||
(`return True`) штатное освобождение merge-lease пропускается. Под держимым transition-lease
|
||
эта ветка практически недостижима (единственный владелец → CAS почти всегда выигрывает; чтобы
|
||
проиграть, нужен аномальный bypass-писатель, сдвинувший стадию с `deploy-staging`). Даже в
|
||
этом крайнем случае утечка **ограничена** собственным TTL+reclaim merge-lease (ORCH-043/065).
|
||
Корректность недвоения не нарушена. При желании — продублировать holder-aware
|
||
`release_merge_lease` до CAS-проверки (или задокументировать намеренный аборт). Не блокер.
|
||
_Ссылка: ADR-001 D1/D4, ORCH-027/058 rollback._
|
||
- [ ] **`reconciler`/`plane._try_advance_stage` зовут `is_held_by_live_owner(task_id)` без
|
||
предварительного `applies(repo)`** (в отличие от reaper). Для out-of-scope, но включённого
|
||
репо (enduro при дефолте) это безвредный лишний `SELECT` (строк нет → `False`). Функционально
|
||
корректно; ради нулевого оверхеда можно добавить дешёвый `applies`-гард. (Перенос P3 из цикла 1
|
||
— остаётся косметикой.)
|
||
|
||
## Документация
|
||
|
||
**Обновлено и проверено (полно):**
|
||
- `.env.example` — `ORCH_TRANSITION_LEASE_ENABLED` / `ORCH_TRANSITION_LEASE_REPOS` (канон ключей
|
||
старта, ORCH-101).
|
||
- `CLAUDE.md` — паспорт-секция ORCH-114.
|
||
- `docs/overview/` — `tech-data-model.md` / `tech-observability.md` / `tech-architecture.md`
|
||
(витрина системы, ORCH-011).
|
||
- `CHANGELOG.md` — подробная запись ORCH-114 в `[Unreleased]`.
|
||
- `docs/architecture/README.md` + `internals.md` — компонент, секция, таблица БД, API-эндпоинт
|
||
`POST /transition-lease/release`, блок `/queue`.
|
||
- ADR: `docs/work-items/ORCH-114/06-adr/ADR-001-…` + сквозной
|
||
`docs/architecture/adr/adr-0045-…` — полные, со сверкой по коду.
|
||
- Work-item доки `02-trz`/`03-acceptance-criteria`/`04-test-plan`/`08-data-requirements`/
|
||
`10-tech-risks` — на месте.
|
||
|
||
**Необновлённой документации при изменении `src/` не выявлено.** Пунктов README «Известные
|
||
ограничения», закрываемых этим PR, нет.
|
||
|
||
## Verdict rationale
|
||
Все P0/P1/P2 прошлого цикла закрыты; новых P0/P1 не выявлено по всем четырём осям
|
||
(ТЗ/AC, ADR, качество кода + обязательный регресс-тест багфикс-трека, документация
|
||
golden-source). Остаются только два P3-наблюдения (косметика/недостижимая граница), не
|
||
блокирующие приёмку. Полный `pytest tests/` зелёный (2052 passed), инварианты конвейера и
|
||
схемы существующих таблиц — байт-в-байт. → **APPROVED**.
|