From f4d174283ac7266ff7238a2f695f29ea76322e74 Mon Sep 17 00:00:00 2001 From: bot-backend Date: Sun, 6 Sep 2026 17:48:29 +0500 Subject: [PATCH] =?UTF-8?q?chore(#3197):=20cian-login=20=E2=80=94=20=D0=B1?= =?UTF-8?q?=D0=B5=D0=B7=20=D1=85=D0=BE=D0=BB=D0=BE=D1=81=D1=82=D0=BE=D0=B9?= =?UTF-8?q?=20=D0=B0=D1=80=D0=B5=D0=BD=D0=B4=D1=8B=20=D0=B8=20=D0=BB=D0=BE?= =?UTF-8?q?=D0=B6=D0=BD=D0=BE=D0=B3=D0=BE=20=D0=BE=D1=82=D0=BA=D0=B0=D0=B7?= =?UTF-8?q?=D0=B0=20(/login=20=D1=81=D0=B0=D0=B9=D0=B4=D0=BA=D0=B0=D1=80?= =?UTF-8?q?=D0=B0=20override=20=D0=BD=D0=B5=20=D0=B1=D0=B5=D1=80=D1=91?= =?UTF-8?q?=D1=82);=20=D1=87=D0=B5=D1=81=D1=82=D0=BD=D1=8B=D0=B5=20=D0=B4?= =?UTF-8?q?=D0=BE=D0=BA=D1=81=D1=82=D1=80=D0=B8=D0=BD=D0=B3=D0=B8?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Code-review хвоста #3197: на `cian-login` аренда из пула не доходила до сайдкара. `BrowserFetcher._post_login` не кладёт `payload["proxy"]` (это делают только `fetch`/`fetch_json`), а на приёме `login_handler` (browser/server.py:2814-2817) зовёт `_no_live_proxy(provider, None)` и `_ensure_browser(provider)` без override — `/login` proxy-override не принимает вовсе. Lease брался в `__aenter__` и освобождался в `__aexit__` без пользы и без health-вердикта по узлу. Хуже холостого хода: при пустом пуле в production `_acquire_lease` поднимает `NoProxyAvailableError` ДО POST, `cian_auto_login` ловит любое `Exception` → `502 Browser login failed`. То есть единственная ручка ВОССТАНОВЛЕНИЯ cian-сессии отказывала ровно во время инцидента с пулом. Комментарий в admin.py при этом утверждал, что фикс закрывает InvalidIP на логине — неправда. Убран `proxy_provider=` (фабрика остаётся ради endpoint/environment из одного места). `use_pool` без провайдера фетчер игнорирует сам — `_acquire_lease`: `use_pool AND provider is not None` — поэтому ни аренды, ни прод-отказа. `domclick-detail-debug` не тронут: он ходит через `fetch`, где override реально кладётся в тело и читается сайдкаром — там #3197 остаётся настоящим фиксом. Тесты для cian обратные по значению и падают на HEAD ветки: `test_cian_auto_login_does_not_lease_from_pool` (провайдер не передан, `acquire` не вызван) и `test_cian_auto_login_survives_empty_pool_in_production` (пустой пул на проде не отдаёт 502). Стаб cian — подкласс настоящего `BrowserFetcher`, чтобы второе утверждение шло через реальный `_acquire_lease`, а не через заглушку. Follow-up (отдельной задачей): сайдкар `/login` не принимает proxy override → логин cian всегда с env-узла (сейчас выключенный узел 9). --- tradein-mvp/backend/app/api/v1/admin.py | 17 +- ...st_3197_admin_debug_browser_pool_wiring.py | 186 +++++++++++++----- .../src/scraper_kit/providers/_base.py | 9 +- 3 files changed, 159 insertions(+), 53 deletions(-) diff --git a/tradein-mvp/backend/app/api/v1/admin.py b/tradein-mvp/backend/app/api/v1/admin.py index c1bae4cd..32b8c47f 100644 --- a/tradein-mvp/backend/app/api/v1/admin.py +++ b/tradein-mvp/backend/app/api/v1/admin.py @@ -508,13 +508,16 @@ async def cian_auto_login( ) try: - # #3197 (хвост): через фабрику, а не BrowserFetcher(source=, endpoint=) напрямую — - # прямая конструкция не проставляла proxy_provider/use_pool/environment, и сайдкар - # логинился со своего env-узла SCRAPER_PROXY_URL мимо пула (на проде это выключенный - # узел 9: 407 → camoufox InvalidIP), а прод-отказ «пул пуст» (#2616) тут был мёртв. - async with build_browser_fetcher( - RealScraperConfig(), "cian", proxy_provider=_kit_proxy_provider() - ) as fetcher: + # #3197 (хвост): через фабрику (endpoint/environment из одного места), но + # НАМЕРЕННО без proxy_provider. `/login` сайдкара proxy-override не принимает + # (browser/server.py:2814-2817 — `_no_live_proxy(provider, None)`; и сам + # `_post_login` не кладёт payload["proxy"], это делают только fetch/fetch_json) — + # логин идёт с env-узла сайдкара. Аренда здесь была бы холостой и при пустом пуле + # блокировала бы ручку восстановления (`_acquire_lease` → NoProxyAvailableError → + # 502 ровно во время инцидента с пулом). Пул для логина — отдельная задача сайдкара. + # `use_pool` без провайдера фетчер игнорирует (`_acquire_lease`: use_pool AND + # provider is not None), поэтому передавать его тут безвредно, но и бесполезно. + async with build_browser_fetcher(RealScraperConfig(), "cian") as fetcher: raw_cookies = await fetcher.login( url=settings.cian_login_url, email=email, diff --git a/tradein-mvp/backend/tests/test_3197_admin_debug_browser_pool_wiring.py b/tradein-mvp/backend/tests/test_3197_admin_debug_browser_pool_wiring.py index 09896500..ba6f2586 100644 --- a/tradein-mvp/backend/tests/test_3197_admin_debug_browser_pool_wiring.py +++ b/tradein-mvp/backend/tests/test_3197_admin_debug_browser_pool_wiring.py @@ -1,19 +1,34 @@ -"""#3197 (хвост) — две служебные ручки admin.py строили BrowserFetcher мимо пула. +"""#3197 (хвост) — две служебные ручки admin.py и пул прокси: одна чинится, вторая НЕТ. -`POST /admin/scrape/cian/auto-login` и `POST /admin/scrape/domclick/debug/detail-fetch` -конструировали `BrowserFetcher(source=..., endpoint=settings.browser_http_endpoint)` -напрямую — без `proxy_provider`/`use_pool`/`environment`, единственных трёх аргументов, -которые кладут "proxy" в тело POST /fetch сайдкара. Без них сайдкар берёт свой env-узел -(`SCRAPER_PROXY_URL`), на проде это выключенный узел 9 (407 → camoufox `InvalidIP`), а -прод-отказ «пул пуст → не ходить на env/direct» (#2616) на этих путях был мёртв: он -смотрит на `environment`, который до конструктора не доезжал. Соседние точки того же -эпика уже переведены: yandex newbuilding (#3389), cian history (#3197), domclick/avito. +`POST /admin/scrape/domclick/debug/detail-fetch` — **настоящий фикс**. Он ходит через +`BrowserFetcher.fetch`, а `fetch`/`fetch_json` — единственные методы, которые кладут +`payload["proxy"]` в тело POST /fetch сайдкара, и сайдкар этот override читает +(`_resolve_proxy_override` → `_ensure_browser(provider, proxy_override=...)`). Прямая +конструкция `BrowserFetcher(source=, endpoint=)` не проставляла +`proxy_provider`/`use_pool`/`environment` — без них сайдкар брал свой env-узел +(`SCRAPER_PROXY_URL`, на проде это выключенный узел 9: 407 → camoufox `InvalidIP`), а +прод-отказ «пул пуст → не ходить на мёртвый env» (#2616) тут был мёртв: он смотрит на +`environment`, который до конструктора не доезжал. Тест меряет ЗНАЧЕНИЯ kwargs. -Тест меряет ЗНАЧЕНИЯ kwargs, а не то, из какого модуля взят класс: подделка ставится и на -`scraper_kit.providers._base.BrowserFetcher` (путь через фабрику), и на -`app.api.v1.admin.BrowserFetcher` (прямая конструкция, как было до правки, `create=True` — -после правки такого имени в модуле нет). Поэтому на откате красное читается как «в -конструктор не передан провайдер», а не как «мок не сработал». +`POST /admin/scrape/cian/auto-login` — **намеренно без пула**, и это проверяется обратными +по значению утверждениями. Логин идёт не через `fetch`, а через `login` → `_post_login`, +который `payload["proxy"]` не кладёт вовсе; на приёме `login_handler` +(`browser/server.py:2814-2817`) зовёт `_no_live_proxy(provider, None)` и +`_ensure_browser(provider)` без override — то есть **сайдкар на `/login` proxy-override не +принимает** и логинится с env-узла при любом теле запроса. Аренда на этом пути была бы +холостой (взяли в `__aenter__`, отпустили в `__aexit__`, health-вердикта по узлу нет), а +при пустом пуле в production `_acquire_lease` поднимает `NoProxyAvailableError` ДО POST — +и единственная ручка ВОССТАНОВЛЕНИЯ сессии отдавала бы `502 Browser login failed` ровно во +время инцидента с пулом. Поэтому здесь `proxy_provider` не передаётся (фабрика остаётся +ради endpoint/environment из одного места); пул для логина — отдельная задача сайдкара. + +Подделка ставится и на `scraper_kit.providers._base.BrowserFetcher` (путь через фабрику), и +на `app.api.v1.admin.BrowserFetcher` (прямая конструкция, как было до #3197, `create=True` — +сейчас такого имени в модуле нет). Дубль нужен, чтобы файл был запускаем и против прежних +состояний исходников: красное тогда читается как «значение не то», а не «мок не сработал». +Стаб cian — ПОДКЛАСС настоящего `BrowserFetcher`: `__aenter__`/`_acquire_lease` исполняются +по-настоящему (иначе тест на «нет ложного 502» проверял бы заглушку), застаблен только +сетевой `login`. Сеть/БД/камуфокс замоканы; в сеть тест не ходит. """ @@ -29,6 +44,9 @@ from unittest.mock import AsyncMock, MagicMock, patch os.environ.setdefault("DATABASE_URL", "postgresql+psycopg://test:test@localhost:5432/test") import pytest +from fastapi import HTTPException +from scraper_kit.browser_fetcher import BrowserFetcher +from scraper_kit.contracts import ProxyLease from app.api.v1 import admin from app.services import cian_session as cian_session_svc @@ -54,6 +72,45 @@ class _CapturingFetcher: return {name: "v" for name in cian_session_svc.CIAN_REQUIRED_COOKIES} +class _RealLeaseFetcher(BrowserFetcher): + """Настоящий `BrowserFetcher` (реальные `__aenter__`/`_acquire_lease`) без сети. + + Наследование, а не заглушка: утверждение «пустой пул в production не роняет ручку» + обязано пройти через тот самый guard `_acquire_lease`, который поднимает + `NoProxyAvailableError`. Подменён только сетевой `login`. + """ + + captured: ClassVar[list[dict[str, Any]]] = [] + + def __init__(self, **kwargs: Any) -> None: + _RealLeaseFetcher.captured.append(kwargs) + super().__init__(**kwargs) + + async def login(self, **_kwargs: Any) -> dict[str, str]: + return {name: "v" for name in cian_session_svc.CIAN_REQUIRED_COOKIES} + + +class _SpyProvider: + """Пустой пул + спай: `acquire` пишет вызовы в ClassVar и всегда отдаёт None. + + ClassVar, а не поле инстанса: `_kit_proxy_provider()` конструирует провайдер сам, + и «ни разу не позвали» должно покрывать в том числе «даже не создали». + """ + + acquired: ClassVar[list[str]] = [] + released: ClassVar[list[int]] = [] + + def acquire(self, provider: str) -> ProxyLease | None: + _SpyProvider.acquired.append(provider) + return None + + def release(self, lease: ProxyLease) -> None: + _SpyProvider.released.append(lease.id) + + def mark_health(self, lease: ProxyLease, *, ok: bool, error: str | None = None) -> None: + return None + + @pytest.fixture def _pool_on(monkeypatch: pytest.MonkeyPatch) -> None: """Curl-флаг включён всегда: это он открывает `_kit_proxy_provider()` (гейт #2163). @@ -64,51 +121,81 @@ def _pool_on(monkeypatch: pytest.MonkeyPatch) -> None: from app.core.config import settings monkeypatch.setattr(settings, "use_proxy_pool_curl", True) + monkeypatch.setattr(admin, "RealProxyProvider", _SpyProvider) _CapturingFetcher.captured = [] + _RealLeaseFetcher.captured = [] + _SpyProvider.acquired = [] + _SpyProvider.released = [] -def _assert_wiring(source: str, *, use_pool: bool, environment: str) -> None: - assert len(_CapturingFetcher.captured) == 1, "ручка обязана построить ровно один фетчер" - kwargs = _CapturingFetcher.captured[0] - assert kwargs["source"] == source +def _cian_login_settings(monkeypatch: pytest.MonkeyPatch, *, use_pool: bool, env: str) -> None: + from app.core.config import settings + + monkeypatch.setattr(settings, "use_proxy_pool_browser", use_pool) + monkeypatch.setattr(settings, "environment", env) + monkeypatch.setattr(settings, "cookie_encryption_key", "k" * 32) + monkeypatch.setattr(settings, "cian_login_email", "a@b.c") + monkeypatch.setattr(settings, "cian_login_password", "pw") + + +def _patched_cian_login(stack: ExitStack, fetcher_cls: type) -> None: + for target in _TARGETS: + stack.enter_context(patch(target, fetcher_cls, create=True)) + stack.enter_context( + patch.object( + cian_session_svc, "verify_session", AsyncMock(return_value={"user": {"userId": 7}}) + ) + ) + stack.enter_context(patch.object(cian_session_svc, "save_session", MagicMock())) + + +@pytest.mark.parametrize(("use_pool", "environment"), [(True, "production"), (False, "dev")]) +@pytest.mark.usefixtures("_pool_on") +async def test_cian_auto_login_does_not_lease_from_pool( + monkeypatch: pytest.MonkeyPatch, use_pool: bool, environment: str +) -> None: + """Логин пул НЕ арендует: сайдкар на `/login` proxy-override не берёт (см. докстринг).""" + _cian_login_settings(monkeypatch, use_pool=use_pool, env=environment) + + with ExitStack() as stack: + _patched_cian_login(stack, _RealLeaseFetcher) + result = await admin.cian_auto_login(db=MagicMock(), body=None) + + assert result["ok"] is True and result["userId"] == 7 + assert len(_RealLeaseFetcher.captured) == 1, "ручка обязана построить ровно один фетчер" + kwargs = _RealLeaseFetcher.captured[0] # .get(), а не [] — красное должно читаться как «значение не то», а не KeyError. - assert kwargs.get("proxy_provider") is not None, "без провайдера пул не подключится" - assert kwargs.get("use_pool") is use_pool, "флаг пула должен доезжать из конфига" - # #2616 шаг 1: без environment отказ «пул пуст» на этом пути мёртв. - assert kwargs.get("environment") == environment - # endpoint не должен потеряться при переезде на фабрику (#2322: без него TypeError). + assert kwargs.get("proxy_provider") is None, "аренда на /login холостая — провайдер не нужен" + assert _SpyProvider.acquired == [], "lease взят впустую (сайдкар его всё равно не увидит)" + # endpoint из фабрики не должен потеряться (#2322: без него TypeError). from app.core.config import settings assert kwargs.get("endpoint") == settings.browser_http_endpoint -@pytest.mark.parametrize(("use_pool", "environment"), [(True, "production"), (False, "dev")]) @pytest.mark.usefixtures("_pool_on") -async def test_cian_auto_login_wires_proxy_pool( - monkeypatch: pytest.MonkeyPatch, use_pool: bool, environment: str +async def test_cian_auto_login_survives_empty_pool_in_production( + monkeypatch: pytest.MonkeyPatch, ) -> None: - """Логин-браузер ходит через узел пула, а не через env-прокси сайдкара.""" - from app.core.config import settings + """Пустой пул на проде НЕ ломает ручку восстановления сессии (нет ложного 502). - monkeypatch.setattr(settings, "use_proxy_pool_browser", use_pool) - monkeypatch.setattr(settings, "environment", environment) - monkeypatch.setattr(settings, "cookie_encryption_key", "k" * 32) - monkeypatch.setattr(settings, "cian_login_email", "a@b.c") - monkeypatch.setattr(settings, "cian_login_password", "pw") + Ровно тот сценарий, ради которого пул отсюда убран: `_acquire_lease` при + `use_pool + provider + production + пустой пул` поднимает `NoProxyAvailableError`, + `cian_auto_login` ловит любое `Exception` и отдаёт `502 Browser login failed` — то + есть инцидент с пулом закрывал бы единственный способ переполучить cian-сессию, + хотя логину пул не нужен (сайдкар proxy-override на `/login` не принимает). + """ + _cian_login_settings(monkeypatch, use_pool=True, env="production") with ExitStack() as stack: - for target in _TARGETS: - stack.enter_context(patch(target, _CapturingFetcher, create=True)) - stack.enter_context( - patch.object( - cian_session_svc, "verify_session", AsyncMock(return_value={"user": {"userId": 7}}) - ) - ) - stack.enter_context(patch.object(cian_session_svc, "save_session", MagicMock())) - result = await admin.cian_auto_login(db=MagicMock(), body=None) + _patched_cian_login(stack, _RealLeaseFetcher) + try: + result = await admin.cian_auto_login(db=MagicMock(), body=None) + except HTTPException as exc: + # pytest.fail, а не re-raise: красное должно называть статус и detail. + pytest.fail(f"пустой пул уронил ручку восстановления: {exc.status_code} {exc.detail}") assert result["ok"] is True and result["userId"] == 7 - _assert_wiring("cian", use_pool=use_pool, environment=environment) @pytest.mark.parametrize(("use_pool", "environment"), [(True, "production"), (False, "dev")]) @@ -147,4 +234,15 @@ async def test_domclick_debug_detail_wires_proxy_pool( result = await admin.debug_domclick_detail_fetch(body=body, db=MagicMock()) assert result.ok is True - _assert_wiring("domclick", use_pool=use_pool, environment=environment) + assert len(_CapturingFetcher.captured) == 1, "ручка обязана построить ровно один фетчер" + kwargs = _CapturingFetcher.captured[0] + assert kwargs["source"] == "domclick" + # .get(), а не [] — красное должно читаться как «значение не то», а не KeyError. + assert kwargs.get("proxy_provider") is not None, "без провайдера пул не подключится" + assert kwargs.get("use_pool") is use_pool, "флаг пула должен доезжать из конфига" + # #2616 шаг 1: без environment отказ «пул пуст» на этом пути мёртв. + assert kwargs.get("environment") == environment + # endpoint не должен потеряться при переезде на фабрику (#2322: без него TypeError). + from app.core.config import settings + + assert kwargs.get("endpoint") == settings.browser_http_endpoint diff --git a/tradein-mvp/packages/scraper-kit/src/scraper_kit/providers/_base.py b/tradein-mvp/packages/scraper-kit/src/scraper_kit/providers/_base.py index 444e215f..f78c0bdb 100644 --- a/tradein-mvp/packages/scraper-kit/src/scraper_kit/providers/_base.py +++ b/tradein-mvp/packages/scraper-kit/src/scraper_kit/providers/_base.py @@ -212,8 +212,13 @@ def build_browser_fetcher( Служебные ручки `app/api/v1/admin.py` (cian-login, domclick-detail-debug) были последними прямыми конструкциями мимо фабрики — переведены сюда же (#3197 хвост); прямые конструкции в `orchestration/pipeline.py` и в backfill-задачах пул получают - своими kwargs. `use_pool` при `proxy_provider is None` эффективно игнорируется - `BrowserFetcher` (env-fallback, см. `browser_fetcher.py::_pool_proxy`). + своими kwargs. Живой `proxy_provider=None` остался ровно один — cian-login: сайдкар + на `/login` proxy-override не берёт (`browser/server.py::login_handler` → + `_no_live_proxy(provider, None)`, а `_post_login` не кладёт `payload["proxy"]`), так + что аренда там была бы холостой, а на проде при пустом пуле роняла бы ручку + восстановления в 502. `use_pool` при `proxy_provider is None` игнорируется + `BrowserFetcher` (`_acquire_lease`: `use_pool AND provider is not None`) — ни аренды, + ни прод-отказа, поведение как до фабрики. `fetch_timeout_s=None` (дефолт) → используется дефолт `BrowserFetcher` (120s). Явный таймаут передаёт ровно один call-site — `yandex/serp.py` (30s);