115 lines
9.2 KiB
Markdown
115 lines
9.2 KiB
Markdown
---
|
||
verdict: APPROVED
|
||
work_item: ORCH-090
|
||
stage: review
|
||
author_agent: reviewer
|
||
status: approved
|
||
created_at: 2026-06-09
|
||
model_used: claude-opus-4-8
|
||
type: review
|
||
work_item_id: ORCH-090
|
||
version: 2
|
||
---
|
||
|
||
# Review ORCH-090 — Механизм отмены задачи: статус STOP (re-review, attempt 2)
|
||
|
||
## Summary
|
||
|
||
Повторный review после фикса блокирующего P1 из предыдущей итерации (`12-review.md` v1).
|
||
Реализация STOP-отмены аккуратна и канонична (leaf `src/cancel.py` never-raise, kill-switch
|
||
`stop_status_enabled`, fail-closed маршрутизация по образцу `confirm_deploy`/ORCH-059, аддитивные
|
||
идемпотентные миграции). Кросс-каттинг `{done}` → `{done, cancelled}` проведён исчерпывающе и
|
||
консистентно (serial_gate / task_deps / stages / db / job_reaper / queue_worker), в точности по
|
||
adr-0026.
|
||
|
||
**Оба ранее блокировавших/важных дефекта закрыты и покрыты содержательными тестами:**
|
||
- **P1 (был blocker) — ИСПРАВЛЕН.** `cancel.in_critical_window` сужен: удержание merge-lease без
|
||
бегущего актора (`_task_has_running_actor`) на стадии `deploy` в ожидании `Confirm Deploy` теперь
|
||
НЕ считается критическим окном → немедленный полный сброс, который сам отпускает lease (шаг 3c).
|
||
Тесты `test_d7_lease_held_idle_parking_is_not_critical`,
|
||
`test_d7_lease_held_with_running_actor_still_critical`,
|
||
`test_d7_stop_on_deploy_awaiting_confirm_full_resets` (последний прямо проверяет
|
||
`stage='cancelled'` + удалённую ветку + `current_lease_holder is None`). Сверено по коду
|
||
`src/cancel.py::in_critical_window` (стр. 100–158) и `stage_engine.cancel_task` — wedge
|
||
self-hosting-репо устранён.
|
||
- **P2 (был should-fix) — ИСПРАВЛЕН.** Deferred-ветка `cancel_task` шлёт алерт только при первом
|
||
переходе (`first = set_task_cancel_requested(...)`, далее `if first:`); повторный STOP в
|
||
критическом окне даёт `deferred-already-pending` без повторного уведомления. Тест
|
||
`test_d7_repeated_stop_in_critical_window_no_duplicate_notify` (ровно 1 notify).
|
||
|
||
Полный регресс `pytest tests/` зелёный (**1349 passed**); `tests/test_stop_status.py` — 30 кейсов
|
||
(TC-01…TC-14 + D7), покрывают AC-1…AC-10 и оба фикса.
|
||
|
||
Оси проверки: ✅ ТЗ/AC (AC-1…AC-10, включая ранее проваленный AC-7) · ✅ ADR (соответствие
|
||
adr-0026/ADR-001; см. P2-нит ниже) · ✅ качество кода · ✅ документация.
|
||
|
||
## Findings
|
||
|
||
### P0 — Blocker
|
||
- (нет)
|
||
|
||
### P1 — Must fix
|
||
- (нет)
|
||
|
||
### P2 — Should fix
|
||
|
||
- [ ] **Work-item ADR-001 §D7 не синхронизирован с фиксом P1 (running-actor-уточнение).**
|
||
`docs/work-items/ORCH-090/06-adr/ADR-001-stop-cancel-task.md` §D7 (стр. 189–201) по-прежнему
|
||
определяет критическое окно как «задача держит merge-lease … / merge в процессе» — **без**
|
||
оговорки «И активно бегущий актор», которую фактически реализует код
|
||
(`cancel.in_critical_window` + `_task_has_running_actor`) после фикса P1. Авторитетные
|
||
golden-source доки уже синхронизированы (`CLAUDE.md` — абзац «Уточнение P1 (ORCH-090 review)»;
|
||
`docs/architecture/README.md` стр. 316–317 «P1-уточнение»; `CHANGELOG.md` — буллет «Фикс P1»),
|
||
поэтому витрина проекта корректна и это **не** P0 «src изменён, доки не обновлены». Но per
|
||
«documentation = golden source» работа-айтемный ADR (запись именно этого архитектурного решения)
|
||
должен честно отражать итоговую семантику — как это уже сделано для уточнения D4. Предложение:
|
||
добавить в §D7 строку-уточнение «merge-lease критичен ТОЛЬКО при бегущем акторе; припаркованное
|
||
ожидание `Confirm Deploy` обратимо → немедленный сброс» (ссылка на review P1). Не блокирует.
|
||
|
||
### P3 — Nice to have
|
||
|
||
- [ ] **«Завис» `cancel_requested_at` на успешно задеплоенной задаче → вечный `pending` в
|
||
`GET /queue`** (перенесено из v1, не адресовано). При SUCCESS-деплое `run_deploy_finalizer`
|
||
вызывает `cancel_task(force=True)`, который видит `stage='done'` → «already-terminal» no-op и
|
||
**не очищает** `cancel_requested_at`; `db.cancelled_tasks_snapshot` считает
|
||
`pending = cancel_requested_at IS NOT NULL AND stage != 'cancelled'` → done-задача с бывшим
|
||
deferred-STOP навсегда показывается «pending». Чисто наблюдаемость; предложение — очищать
|
||
`cancel_requested_at` при честном no-op после завершения.
|
||
- [ ] **adr-0026 п.6 (post-deploy monitor «не тикает по отменённой задаче») в коде не реализован**
|
||
(перенесено из v1). Фактически безвреден и недостижим: post-deploy наблюдение идёт только ПОСЛЕ
|
||
`done`, а STOP на `done` — no-op. Рекомендация: снять пункт из adr-0026 как нерелевантный либо
|
||
добавить дешёвый терминал-гард для строгого соответствия ADR.
|
||
- [ ] **Косметика:** «рваная» строковая склейка комментария relaunch-hole в
|
||
`src/webhooks/plane.py` (стр. 345–351) — собрать в одну строку для читаемости.
|
||
|
||
## Документация
|
||
|
||
**Обновлена полностью и качественно — отдельных blocking-findings нет.** Проверено пофайльно:
|
||
- `README.md` — таблица env (`ORCH_STOP_STATUS_ENABLED`/`ORCH_STOP_STATUS_REPOS`), раздел «Отмена
|
||
задачи: статус STOP (ORCH-090)», обновлён список job-статусов (`cancelled`), инфра-предусловие.
|
||
- `docs/architecture/README.md` — раздел STOP со статусом «реализовано», блок `stop` в `/queue`,
|
||
раздел «База данных» (колонки/тумбстон/статусы) **и P1-уточнение** (стр. 316–317).
|
||
- `docs/architecture/internals.md` — `STAGE_TRANSITIONS` (сток `cancelled`), терминал-предикат
|
||
`{done,cancelled}`, job-статусы.
|
||
- `CHANGELOG.md` (`feat:` + отдельный буллет «Фикс P1»), `CLAUDE.md` (раздел «Отмена задачи: статус
|
||
STOP (ORCH-090)» с абзацем «Уточнение P1»), `.env.example` — согласованы.
|
||
- ADR: локальный `06-adr/ADR-001-stop-cancel-task.md` + сквозной
|
||
`docs/architecture/adr/adr-0026-stop-cancel-task.md`; уточнение D4 (тумбстон `plane_issue_id`)
|
||
отражено в коде и доках. Единственный gap — §D7 локального ADR не дотянут до running-actor-фикса
|
||
(P2 выше).
|
||
- Раздела README «Известные ограничения», который ORCH-090 закрывал бы (ORCH-079), нет — обзорная
|
||
витрина не рассинхронена.
|
||
|
||
**Трассировка маркеров (TRACEABILITY.md):** правки маркированных инвариантов `serial_gate`/ORCH-088
|
||
и `task_deps`/ORCH-026 сверены с их ADR — расширение терминал-набора до `{done,cancelled}` сохраняет
|
||
FIFO-семантику (`t2.id < jobs.task_id`) и dep-готовность (терминальный предшественник), инварианты
|
||
не сломаны. `STAGE_TRANSITIONS` exit-гейты / `QG_CHECKS` / `check_*` — не тронуты (подтверждено
|
||
анти-регресс-снапшотами, зелёные).
|
||
|
||
## Вердикт
|
||
|
||
`APPROVED` — оба ранее найденных дефекта (P1 wedge при STOP в ожидании Confirm Deploy; P2
|
||
дубль-уведомления в deferred-ветке) исправлены и покрыты содержательными тестами; полный регресс
|
||
зелёный (1349 passed). Остаются только P2 (синхронизация §D7 локального ADR) и P3 (наблюдаемость/
|
||
косметика) — не блокируют приёмку, желательны к устранению попутно.
|