From d5e15423d81a88773bd08af97de825a627bea7f5 Mon Sep 17 00:00:00 2001 From: bot-backend Date: Thu, 6 Aug 2026 13:58:14 +0500 Subject: [PATCH] =?UTF-8?q?fix(tradein/auth):=20=D0=B4=D0=B5=D1=84=D0=BE?= =?UTF-8?q?=D0=BB=D1=82=D1=8B=20=D0=BF=D0=BE=D1=82=D0=BE=D0=BB=D0=BA=D0=B0?= =?UTF-8?q?=20=D0=B7=D0=B0=D0=BA=D1=80=D0=B5=D0=BF=D0=BB=D0=B5=D0=BD=D1=8B?= =?UTF-8?q?=20=D1=82=D0=B5=D1=81=D1=82=D0=BE=D0=BC,=20=D0=B3=D1=80=D0=B0?= =?UTF-8?q?=D0=BD=D0=B8=D1=86=D1=8B=20=D0=BD=D0=B0=D1=81=D1=82=D1=80=D0=BE?= =?UTF-8?q?=D0=B5=D0=BA,=20=D1=82=D0=BE=D1=87=D0=BD=D1=8B=D0=B5=20=D0=BE?= =?UTF-8?q?=D0=B1=D0=BE=D1=81=D0=BD=D0=BE=D0=B2=D0=B0=D0=BD=D0=B8=D1=8F?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ревью показало, что «разрыва вынесено-без-потолка не существует» верно только по коду: `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 --- tradein-mvp/backend/app/core/config.py | 10 +++++-- tradein-mvp/backend/app/core/password.py | 25 +++++++++++------ tradein-mvp/backend/tests/test_auth_api.py | 9 ++++--- tradein-mvp/backend/tests/test_password.py | 31 ++++++++++++++++++---- 4 files changed, 57 insertions(+), 18 deletions(-) diff --git a/tradein-mvp/backend/app/core/config.py b/tradein-mvp/backend/app/core/config.py index 5894d562..337cd886 100644 --- a/tradein-mvp/backend/app/core/config.py +++ b/tradein-mvp/backend/app/core/config.py @@ -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` ───────────────────── diff --git a/tradein-mvp/backend/app/core/password.py b/tradein-mvp/backend/app/core/password.py index c395737b..a2dd3fbd 100644 --- a/tradein-mvp/backend/app/core/password.py +++ b/tradein-mvp/backend/app/core/password.py @@ -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`: у обёртки «готово» наступает и при отмене — тест diff --git a/tradein-mvp/backend/tests/test_auth_api.py b/tradein-mvp/backend/tests/test_auth_api.py index 1778014d..eb2b664b 100644 --- a/tradein-mvp/backend/tests/test_auth_api.py +++ b/tradein-mvp/backend/tests/test_auth_api.py @@ -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)) diff --git a/tradein-mvp/backend/tests/test_password.py b/tradein-mvp/backend/tests/test_password.py index c2819680..78df935c 100644 --- a/tradein-mvp/backend/tests/test_password.py +++ b/tradein-mvp/backend/tests/test_password.py @@ -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()