fix(tradein/auth): дефолты потолка закреплены тестом, границы настроек, точные обоснования
All checks were successful
CI / changes (pull_request) Successful in 7s
CI Trade-In / changes (pull_request) Successful in 8s
CI Trade-In / frontend-checks (pull_request) Has been skipped
CI / backend-tests (pull_request) Has been skipped
CI / frontend-tests (pull_request) Has been skipped
CI / openapi-codegen-check (pull_request) Has been skipped
CI Trade-In / backend-tests (pull_request) Successful in 3m6s
All checks were successful
CI / changes (pull_request) Successful in 7s
CI Trade-In / changes (pull_request) Successful in 8s
CI Trade-In / frontend-checks (pull_request) Has been skipped
CI / backend-tests (pull_request) Has been skipped
CI / frontend-tests (pull_request) Has been skipped
CI / openapi-codegen-check (pull_request) Has been skipped
CI Trade-In / backend-tests (pull_request) Successful in 3m6s
Ревью показало, что «разрыва вынесено-без-потолка не существует» верно только по коду: `LOGIN_PASSWORD_VERIFY_WORKERS=32` в .env.runtime даёт ровно это состояние, без единой правки и без ревью — и наступит оно именно так, под предлогом «входы тормозят». M1. Тест про темп вычисляет ожидаемый потолок из той же настройки, которую охраняет, поэтому мутант дефолтов (workers 1→16, inflight 4→64) оставлял все 58 тестов зелёными. Дефолты теперь стережёт отдельный тест: workers==1 и очередь строго уже пула соединений (5+10). На том же мутанте краснеет. Комментарий у теста темпа больше не обещает того, чего тот не делает. L2. `ge=1` на обе настройки. Проверено запуском: 0/-1 в workers роняли ThreadPoolExecutor на импорте (crash-loop контейнера), 0 в max_inflight отдавал 429 на КАЖДЫЙ вход навсегда и молча — а «0» это естественная попытка выключить лимит. Теперь отказ на валидации настроек, с именем поля. Info. Комментарий объяснял фикс не той причиной: отмена не набивает очередь — не начатую работу `cancel()` снимает. Настоящий вред прежней редакции — освобождение слота при отмене УЖЕ НАЧАТОЙ сверки (поток занят, а слот числится свободным), это и ловит тест. Обоснование переписано в коде и в тесте. Плюс две оговорки: потолок множится и на `WEB_CONCURRENCY` (uvicorn читает число процессов оттуда, а .env.runtime правится руками), а правило «из async def только verify_password_bounded» относится к сверке — `hash_password` в team.py оставлен на цикле сознательно как редкая аутентифицированная операция. Refs #2665
This commit is contained in:
parent
bce5b0c02f
commit
d5e15423d8
4 changed files with 57 additions and 18 deletions
|
|
@ -130,8 +130,11 @@ class Settings(BaseSettings):
|
|||
# блокировка цикла: вынос не должен ускорять перебор. Поднимать имеет смысл
|
||||
# только вместе с осознанным ответом «во сколько раз мы согласны ускорить
|
||||
# перебор ради параллельных входов».
|
||||
# ge=1: 0 или -1 роняют ThreadPoolExecutor прямо НА ИМПОРТЕ («max_workers must
|
||||
# be greater than 0») — контейнер уходит в crash-loop, и причина видна только
|
||||
# в трейсбеке старта. Пусть отказ будет на валидации настроек, с именем поля.
|
||||
login_password_verify_workers: int = Field(
|
||||
default=1, validation_alias="LOGIN_PASSWORD_VERIFY_WORKERS"
|
||||
default=1, ge=1, validation_alias="LOGIN_PASSWORD_VERIFY_WORKERS"
|
||||
)
|
||||
# Сколько запросов одновременно допускаются к проверке (считая тех, кто ждёт
|
||||
# очереди в пуле). Сверх — сразу 429, без ожидания. Не режет темп (его режут
|
||||
|
|
@ -141,8 +144,11 @@ class Settings(BaseSettings):
|
|||
# очередь выбрала бы пул и положила API ровно так же, как блокировка цикла,
|
||||
# только другим способом. 4 из 15 соединений и худшее ожидание
|
||||
# 4/1×282мс ≈ 1.1с — цена, которую живой вход переживает.
|
||||
# ge=1: 0 читается как «выключить лимит», а означал бы обратное — КАЖДЫЙ вход
|
||||
# получает 429 навсегда и молча (слотов нет ни одного). Выключать тут нечего:
|
||||
# потолок — это workers, а очередь без границы выбирает пул соединений к БД.
|
||||
login_password_verify_max_inflight: int = Field(
|
||||
default=4, validation_alias="LOGIN_PASSWORD_VERIFY_MAX_INFLIGHT"
|
||||
default=4, ge=1, validation_alias="LOGIN_PASSWORD_VERIFY_MAX_INFLIGHT"
|
||||
)
|
||||
|
||||
# ── Эпик «единый вход»: общий реестр людей в БД `auth` ─────────────────────
|
||||
|
|
|
|||
|
|
@ -9,6 +9,12 @@ bcrypt тихо обрезает пароли длиннее 72 байт (UTF-8)
|
|||
#2665: из `async def` зови ТОЛЬКО `verify_password_bounded` — см. её docstring.
|
||||
Синхронный `verify_password` остаётся для sync-кода (сидов, тестов, CLI) и как
|
||||
тело, которое исполняется в пуле.
|
||||
|
||||
Правило про пул относится к СВЕРКЕ, не к хешированию. `hash_password` — тот же
|
||||
cost 12 и те же ~282 мс на цикле — сознательно остаётся синхронным в
|
||||
`app/api/v1/team.py` (заведение сотрудника, смена пароля): это редкая операция
|
||||
АУТЕНТИФИЦИРОВАННОГО менеджера, её нельзя вызвать анонимно и потому нельзя
|
||||
превратить в поток. Станет их много — переносить тем же приёмом.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
|
@ -117,9 +123,11 @@ async def verify_password_bounded(plain: str, hashed: str) -> bool:
|
|||
(`printenv | grep -c ^REDIS_URL=` → 0, находка эпика #2674 — кэш поиска всю
|
||||
жизнь стучится в localhost и получает отказ). Потолок на Redis был бы
|
||||
потолком, который молча не работает.
|
||||
Ceiling: появятся `--workers N` — темп множится на N (как и у соседних
|
||||
in-memory лимитеров в app/api/v1/auth.py); тогда потолок надо переносить в
|
||||
общее хранилище, предварительно убедившись, что оно реально доступно.
|
||||
Ceiling: появятся `--workers N` (или `WEB_CONCURRENCY=N` в `.env.runtime` —
|
||||
uvicorn читает число процессов и оттуда, а файл правится руками на VPS) —
|
||||
темп множится на N, как и у соседних in-memory лимитеров в
|
||||
app/api/v1/auth.py; тогда потолок надо переносить в общее хранилище,
|
||||
предварительно убедившись, что оно реально доступно.
|
||||
|
||||
Raises:
|
||||
PasswordVerifyOverloadedError: очередь на проверку заполнена
|
||||
|
|
@ -142,11 +150,12 @@ async def verify_password_bounded(plain: str, hashed: str) -> bool:
|
|||
raise
|
||||
|
||||
# Слот освобождает ЗАВЕРШЕНИЕ РАБОТЫ, а не выход из этой корутины. Отмена
|
||||
# (клиент отвалился, таймаут) прекращает корутину, но уже подхваченную пулом
|
||||
# задачу не отменяет — она всё равно займёт поток на свои 282 мс. Отдавай мы
|
||||
# слот в `finally`, отменяющий клиент получал бы свежий слот на каждую
|
||||
# отмену и набивал очередь пула быстрее, чем та разгребается: темп bcrypt
|
||||
# по-прежнему держал бы пул, но очередь и память росли бы без границы.
|
||||
# (клиент отвалился, таймаут) прекращает корутину, но УЖЕ НАЧАТУЮ сверку не
|
||||
# снимает — поток занят ею все 282 мс. Отдавай мы слот в `finally`, на это
|
||||
# время слот считался бы свободным: одновременно работающих сверок стало бы
|
||||
# больше, чем разрешено, и очередь пула поехала бы вслед за ними.
|
||||
# (Ещё не начатую работу отмена как раз снимает — `cancel()` пробрасывается
|
||||
# на future пула, — так что вреда от неё нет; проблема ровно в начатой.)
|
||||
#
|
||||
# Именно поэтому колбэк висит на future ПУЛА, а не на обёртке из
|
||||
# `run_in_executor`: у обёртки «готово» наступает и при отмене — тест
|
||||
|
|
|
|||
|
|
@ -745,9 +745,12 @@ async def test_login_flood_capped_by_rate_while_api_stays_responsive(
|
|||
потолок, а не соседний лимитер.
|
||||
"""
|
||||
verify_s = 0.05
|
||||
# Пул создаётся на импорте из настроек, дефолт — 1 поток. Значит потолок,
|
||||
# который меряем, = 1/verify_s = 20 сверок/с; берём его из настройки, а не
|
||||
# из числа, чтобы тест ловил и молчаливое изменение дефолта.
|
||||
# Потолок = размер пула / время одной сверки. Значение берётся из ТОЙ ЖЕ
|
||||
# настройки, что его задаёт, поэтому этот тест проверяет только МЕХАНИКУ
|
||||
# (потолок работает и равен пулу), но НЕ величину дефолта: подними
|
||||
# login_password_verify_workers — поднимется и ожидание, тест останется
|
||||
# зелёным. Сам дефолт стережёт
|
||||
# tests/test_password.py::test_verify_ceiling_defaults_stay_within_the_db_pool.
|
||||
ceiling_per_s = config.settings.login_password_verify_workers / verify_s
|
||||
|
||||
patch_identity_sessions(monkeypatch, lambda: _FakeDB(store))
|
||||
|
|
|
|||
|
|
@ -101,6 +101,28 @@ def test_verify_malformed_hash_returns_false() -> None:
|
|||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_verify_ceiling_defaults_stay_within_the_db_pool() -> None:
|
||||
"""Дефолты — часть защиты, а не тюнинг. Стережём их здесь.
|
||||
|
||||
Тест про темп (test_auth_api.py) вычисляет ожидаемый потолок из той же
|
||||
настройки, которую охраняет, поэтому подъём дефолта он не заметит. А
|
||||
наступит ослабление именно через настройку: не правкой кода и не ревью, а
|
||||
строчкой `LOGIN_PASSWORD_VERIFY_WORKERS=32` в `.env.runtime` под предлогом
|
||||
«входы тормозят». Пусть тогда краснеет хотя бы этот тест.
|
||||
"""
|
||||
from app.core.db import engine
|
||||
|
||||
# max_inflight ждущих ДЕРЖАТ по соединению к БД (сессия реестра открыта
|
||||
# после SELECT в get_user_by_username) — очередь обязана быть уже пула.
|
||||
assert (
|
||||
settings.login_password_verify_max_inflight < engine.pool.size() + engine.pool._max_overflow
|
||||
)
|
||||
assert settings.login_password_verify_workers == 1, (
|
||||
"потолок перебора = workers/282мс. Подъём — осознанное решение "
|
||||
"«во сколько раз ускоряем перебор», а не рефакторинг: правь вместе с тестом"
|
||||
)
|
||||
|
||||
|
||||
async def test_bounded_gives_same_answer_as_sync() -> None:
|
||||
"""Обёртка не меняет вердикт — она меняет только ГДЕ он считается."""
|
||||
hashed = hash_password("correct horse battery staple")
|
||||
|
|
@ -160,11 +182,10 @@ async def test_bounded_slot_freed_by_the_work_not_by_cancellation(
|
|||
) -> None:
|
||||
"""Отмена запроса не возвращает слот раньше времени.
|
||||
|
||||
Отменённая корутина работу из пула не забирает: bcrypt всё равно займёт
|
||||
поток на свои 282 мс. Освобождай мы слот по выходу из корутины,
|
||||
отваливающийся клиент получал бы свежий слот на каждую отмену и набивал
|
||||
очередь пула быстрее, чем она разгребается — темп сверок держал бы пул, но
|
||||
очередь и память росли бы без границы.
|
||||
Отмена снимает работу, которая ещё НЕ началась, — с ней проблем нет. Но уже
|
||||
начатую сверку она не забирает: поток занят ею все 282 мс. Освобождай мы
|
||||
слот по выходу из корутины, на это время он числился бы свободным, и
|
||||
одновременно работающих сверок стало бы больше, чем разрешено.
|
||||
"""
|
||||
monkeypatch.setattr(settings, "login_password_verify_max_inflight", 1)
|
||||
started = threading.Event()
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue