ayurishchevandClaude Opus 5.5
03d727e496
Повторный анализ кодовой базы (docs/reviews/2026-09-26-codebase-review-2.md) и доработки:
025 Ёмкость префикса — размер его подсети (а не сумма листьев); «Обзор» считает ёмкость
по корневым активным IPv4-префиксам и адреса внутри них.
026 Политика блокировки входа: 5 неудач на логин+IP, 20 на IP, 50 на логин со всех IP,
кроме известных IP (known_logins, миграция 0009) — владельца нельзя заблокировать анонимно.
027 Сериализация попыток входа по IP (advisory-lock после блокировки логина).
028 UI «Префиксы»: загрузка всех страниц (до 20 000), счётчики по total, предупреждение об усечении.
029 Advisory-lock по VRF для операций, меняющих дерево префиксов и раскладку адресов.
030 Исправление замечаний ревью 025-029: _lock_prefix (VRF блокируется до чтения префикса,
409 при одновременном переносе), константы политики входа перенесены в app/services.py.
Тесты: 14 passed (проверка ёмкости родителя приведена к семантике 025); сквозные сценарии
и гонки — docs/reviews/2026-09-26-changes-025-029-review.md, 2026-09-27-changes-030-review.md.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
82 lines
9.6 KiB
Markdown
82 lines
9.6 KiB
Markdown
# Итог: исправление замечаний 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` (косвенный эффект удаления импорта,
|
||
отдельно не измерялся).
|