Files

81 lines
9.6 KiB
Markdown
Raw Permalink Normal View History

# Итог: исправление замечаний 1–2 ревью изменений 025–029 (изменение 030)
Источник: `docs/reviews/2026-09-26-changes-025-029-review.md`, замечания № 1 и № 2. Только код, без тестов.
## 1. Чтение префикса до блокировки VRF (`app/api/v1/prefixes.py`)
- Добавлен хелпер `_lock_prefix(db, id, *extra_vrf_ids) -> Prefix` рядом с `_lock_vrf`:
1. `vrf_id` читается отдельным `SELECT Prefix.vrf_id` (без блокировки строки); нет строки — 404 «Префикс не найден».
2. `_lock_vrf(db, vrf_id, *extra_vrf_ids)` — все нужные VRF одним вызовом, в возрастающем порядке (как раньше).
3. Префикс перечитывается под блокировкой: `select(Prefix).where(Prefix.id == id).with_for_update().execution_options(populate_existing=True)` —
`populate_existing`, чтобы не получить устаревший объект из identity map сессии.
4. Если перечитанный `vrf_id` не совпал с заблокированным (префикс успели перенести в другой VRF между шагами 1 и 3) — `409 «Префикс одновременно изменяется, повторите запрос»`.
- Применение:
- `delete_prefix`: `p = _lock_prefix(db, id)` вместо `get_or_404` + `_lock_vrf(db, p.vrf_id)`.
- `create_address`: то же самое.
- `update_prefix`: `new_vrf` вычисляется из тела запроса **до** чтения префикса (`data.pop("vrf_id", None)`); дальше
`p = _lock_prefix(db, id, new_vrf) if new_vrf is not None else get_or_404(db, Prefix, id, "Префикс")`. Если целевой VRF совпадает
с текущим, лишняя (повторная) блокировка того же VRF безвредна — как и предполагал план.
- `allocate_subnet`, `allocate_next`: переведены на `_lock_prefix` для единообразия. Прежний код читал `vrf_id` отдельным `SELECT`,
затем `_lock_vrf`, затем `SELECT … FOR UPDATE` без сверки `vrf_id` после блокировки — сейчас эта сверка выполняется хелпером,
т.е. поведение не просто перенесено, а дополнительно защищено той же гарантией, что и остальные точки.
### Выбранный вариант обработки гонки — без повтора, сразу 409
В плане предлагалось два варианта: (а) до 3 попыток внутри `_lock_prefix`, не снимая блокировки между попытками, либо
(б) сразу отдавать 409 без повтора. Выбран вариант **(б)**.
Причина: между двумя попытками цикла блокировка устаревшего (первого прочитанного) `vrf_id` из предыдущей попытки не снимается
(advisory-lock держится до конца транзакции), а на следующей попытке блокируется уже новый `vrf_id`. `_lock_vrf` сортирует по
возрастанию только ids **внутри одного своего вызова** — порядок между накопленными за разные попытки блокировками этой
гарантии не имеет. Если два конкурентных запроса одновременно переносят префиксы «навстречу» друг другу (транзакция A: сначала
видит VRF 5, после гонки — VRF 3; транзакция B — в обратном порядке), возможна ситуация, когда A держит блокировку VRF 5 и
запрашивает VRF 3, а B держит VRF 3 и запрашивает VRF 5 — классический deadlock из-за несогласованного порядка захвата между
попытками. Однократная попытка с немедленным 409 гарантированно не накапливает блокировки разных VRF за пределами одного
согласованного вызова `_lock_vrf`, поэтому этот риск не возникает. Транзакция откатывается при закрытии сессии (as-is для всех
остальных `HTTPException` в этом модуле — см., например, комментарий у `_move_to_vrf`), и клиент просто повторяет весь HTTP-запрос
с чистого листа, без унаследованных блокировок.
Это разумная цена: окно гонки узкое (перенос префикса в другой VRF — редкая административная операция), а 3 внутренние попытки
не устраняют риск deadlock — они его создают.
## 2. Зависимость ротации от API-слоя
- `app/services.py`: добавлен блок констант политики входа (после `MAX_CAPACITY`/`MAX_OFFSET`, с комментарием):
`LOGIN_WINDOW` (бывший `WINDOW`), `MAX_PER_LOGIN_IP`, `MAX_PER_IP`, `MAX_PER_LOGIN`, `KNOWN_IP_DAYS`. Добавлен импорт `from datetime import timedelta`.
- `app/api/v1/auth.py`: собственные определения констант удалены, всё импортируется из `app.services`. `WINDOW` заменён на
`LOGIN_WINDOW` по всем местам использования (`_retry_after`, `_distinct_logins`, форматирование модульной docstring). Внутренние
имена не разошлись с перенесёнными. `LOGIN_LOCK_NS`/`IP_LOCK_NS` (пространства advisory-lock) остались в `auth.py`, как и планировалось.
- `app/rotation.py`: `from app.api.v1.auth import KNOWN_IP_DAYS` заменён на `from app.services import KNOWN_IP_DAYS, SYSTEM, audit`.
Модуль больше не импортирует ничего из `app.api.*`.
## Отклонения от плана
- П. 1: выбран вариант «сразу 409» вместо цикла до 3 попыток — обоснование выше (план явно допускал оба варианта и просил
зафиксировать выбор в SUMMARY).
- В остальном реализация соответствует плану без отклонений.
## Изменённые файлы
- `app/api/v1/prefixes.py` — хелпер `_lock_prefix`, применение в `delete_prefix`, `create_address`, `update_prefix`, `allocate_subnet`, `allocate_next`.
- `app/services.py` — константы политики входа.
- `app/api/v1/auth.py` — импорт констант из `app.services` вместо локальных определений.
- `app/rotation.py` — импорт `KNOWN_IP_DAYS` из `app.services` вместо `app.api.v1.auth`.
`README.md` не менялся: наблюдаемое поведение API не меняется (кроме нового 409 при воспроизведении узкой гонки переноса VRF,
который относится к тому же классу конкурентных ошибок, что уже описан в README для изменения 029).
## Не проверено
Всё поведение — по правилам задачи, автор изменения тесты не пишет и не запускает. Не проверялось (проверяет ревьюер):
- Импорт приложения проверен только `import app.main`; `pytest -q` не запускался.
- Параллельные сценарии из плана: удаление префикса ∥ вставка промежуточного префикса; перенос префикса в другой VRF ∥
создание адреса / удаление — не воспроизводились ни через API, ни вручную.
- Что `_lock_prefix` действительно возвращает 409 при воспроизведённой гонке (реальный `vrf_id` mismatch под нагрузкой).
- Поведение `populate_existing=True` в связке с уже загруженными объектами `Prefix` в сессии (в текущих точках вызова такой
объект до `_lock_prefix` не загружается, поэтому эффект защитный, но не критичен для текущих путей вызова).
- Корректность обновлённой модульной docstring `auth.py` после форматирования (`__doc__.format(...)`) — визуально не отличается
от прежней, но не выводилась и не сравнивалась построчно.
- `rotation.py`: что при импорте больше не вычисляется фиктивный argon2-хэш из `auth.py` (косвенный эффект удаления импорта,
отдельно не измерялся).