fix(browser): reset page_counter on recycle + close login page in finally (#796 review)

tradein-mvp/browser/server.py

@@ -249,7 +249,15 @@ async def _safe_relaunch(reason: str) -> None:
     случится позже, когда параллельная нагрузка спадёт. Crash-recovery при этом не
     теряется: per-page краш изолирован (своя страница), а реально мёртвый браузер
     поднимет _ensure_browser на следующем запросе.
+
+    Сброс ``_page_counter = 0`` происходит АТОМАРНО с реальным relaunch'ем (под
+    launch-локом, только когда guard inflight<=1 пройден). Иначе под конкуренцией
+    N>1 несколько корутин видят counter >= порога, все откладывают relaunch (guard
+    inflight>1), но counter НИКОГДА не сбрасывается → recycle-check срабатывает на
+    каждом fetch вечно, а реальный recycle голодает. При отложенном relaunch'е
+    counter НЕ трогаем — следующий запрос (когда нагрузка спадёт) дожмёт recycle.
     """
+    global _page_counter
     assert _launch_lock is not None, "_launch_lock not initialised"
     async with _launch_lock:
         if _inflight > 1:
@@ -260,6 +268,7 @@ async def _safe_relaunch(reason: str) -> None:
             )
             return
         logger.info("tradein-browser: relaunch (%s)", reason)
+        _page_counter = 0
         await _relaunch_browser()
 
 
@@ -585,81 +594,85 @@ async def _login_once(params: dict) -> list[dict]:
     success_cookie: str = params.get("success_cookie", "")
     wait_ms: int = params.get("wait_ms") or BROWSER_WAIT_MS
 
+    # Страница закрывается в finally: asyncio.CancelledError (BaseException, НЕ
+    # Exception) обходит except-ветку ниже — без finally страница утекала бы при
+    # отмене таска (например, на shutdown). _fetch_once делает то же.
     page = await browser.new_page()  # type: ignore[attr-defined]
     try:
-        await page.goto(  # type: ignore[attr-defined]
-            url, timeout=BROWSER_NAV_TIMEOUT_MS, wait_until="domcontentloaded"
-        )
-        await page.wait_for_timeout(wait_ms)  # type: ignore[attr-defined]
+        try:
+            await page.goto(  # type: ignore[attr-defined]
+                url, timeout=BROWSER_NAV_TIMEOUT_MS, wait_until="domcontentloaded"
+            )
+            await page.wait_for_timeout(wait_ms)  # type: ignore[attr-defined]
 
-        # Pre-click sequence: каждый клик non-fatal (AnotherAccountBtn на fresh
-        # headless-сессии отсутствует — молча пропускается).
-        for sel in pre_click_selectors:
-            try:
-                await page.click(sel, timeout=12000)  # type: ignore[attr-defined]
-                await page.wait_for_timeout(1200)  # type: ignore[attr-defined]
-            except Exception:
-                logger.info("login: pre-click skipped: %s", sel)
+            # Pre-click sequence: каждый клик non-fatal (AnotherAccountBtn на fresh
+            # headless-сессии отсутствует — молча пропускается).
+            for sel in pre_click_selectors:
+                try:
+                    await page.click(sel, timeout=12000)  # type: ignore[attr-defined]
+                    await page.wait_for_timeout(1200)  # type: ignore[attr-defined]
+                except Exception:
+                    logger.info("login: pre-click skipped: %s", sel)
 
-        await page.fill(email_selector, email, timeout=15000)  # type: ignore[attr-defined]
-        await page.fill(password_selector, password, timeout=15000)  # type: ignore[attr-defined]
+            await page.fill(email_selector, email, timeout=15000)  # type: ignore[attr-defined]
+            await page.fill(password_selector, password, timeout=15000)  # type: ignore[attr-defined]
 
-        # 2-шаговый submit: Cian после первого сабмита показывает экран
-        # «Введите пароль» с тем же password_selector — дозаполняем и повторяем.
-        cookie_found = False
-        for attempt in range(2):
-            await page.click(submit_selector, timeout=15000)  # type: ignore[attr-defined]
-            if success_cookie:
-                cookie_found = await _wait_cookie(
-                    page, success_cookie, tries=20, interval_ms=500
-                )
-            if cookie_found:
+            # 2-шаговый submit: Cian после первого сабмита показывает экран
+            # «Введите пароль» с тем же password_selector — дозаполняем и повторяем.
+            cookie_found = False
+            for attempt in range(2):
+                await page.click(submit_selector, timeout=15000)  # type: ignore[attr-defined]
+                if success_cookie:
+                    cookie_found = await _wait_cookie(
+                        page, success_cookie, tries=20, interval_ms=500
+                    )
+                if cookie_found:
+                    break
+                # 2-step: если поле пароля ещё видимо — дозаполнить и повторить
+                still_visible = False
+                try:
+                    still_visible = await page.is_visible(password_selector)  # type: ignore[attr-defined]
+                except Exception:
+                    pass
+                if attempt == 0 and still_visible:
+                    await page.fill(password_selector, password, timeout=15000)  # type: ignore[attr-defined]
+                    await page.wait_for_timeout(800)  # type: ignore[attr-defined]
+                    continue
                 break
-            # 2-step: если поле пароля ещё видимо — дозаполнить и повторить
-            still_visible = False
+
+            logger.info(
+                "tradein-browser: login success_cookie=%r present=%s",
+                success_cookie,
+                cookie_found,
+            )
+
+            cookies: list[dict] = await page.context.cookies()  # type: ignore[attr-defined]
+            logger.info(
+                "tradein-browser: login OK url=%r email_selector=%r cookie_count=%d",
+                url,
+                email_selector,
+                len(cookies),
+            )
+        except Exception as exc:
+            screenshot_b64 = ""
             try:
-                still_visible = await page.is_visible(password_selector)  # type: ignore[attr-defined]
+                screenshot_bytes: bytes = await page.screenshot()  # type: ignore[attr-defined]
+                screenshot_b64 = base64.b64encode(screenshot_bytes).decode()
             except Exception:
                 pass
-            if attempt == 0 and still_visible:
-                await page.fill(password_selector, password, timeout=15000)  # type: ignore[attr-defined]
-                await page.wait_for_timeout(800)  # type: ignore[attr-defined]
-                continue
-            break
-
-        logger.info(
-            "tradein-browser: login success_cookie=%r present=%s",
-            success_cookie,
-            cookie_found,
-        )
-
-        cookies: list[dict] = await page.context.cookies()  # type: ignore[attr-defined]
-        logger.info(
-            "tradein-browser: login OK url=%r email_selector=%r cookie_count=%d",
-            url,
-            email_selector,
-            len(cookies),
-        )
-    except Exception as exc:
-        screenshot_b64 = ""
-        try:
-            screenshot_bytes: bytes = await page.screenshot()  # type: ignore[attr-defined]
-            screenshot_b64 = base64.b64encode(screenshot_bytes).decode()
-        except Exception:
-            pass
-        page_url = ""
-        try:
-            page_url = page.url  # type: ignore[attr-defined]
-        except Exception:
-            pass
+            page_url = ""
+            try:
+                page_url = page.url  # type: ignore[attr-defined]
+            except Exception:
+                pass
+            raise LoginError({
+                "error": f"{type(exc).__name__}: {exc}",
+                "page_url": page_url,
+                "screenshot_b64": screenshot_b64,
+            }) from exc
+    finally:
         await page.close()  # type: ignore[attr-defined]
-        raise LoginError({
-            "error": f"{type(exc).__name__}: {exc}",
-            "page_url": page_url,
-            "screenshot_b64": screenshot_b64,
-        }) from exc
 
-    await page.close()  # type: ignore[attr-defined]
     _page_counter += 1
     if _page_counter >= BROWSER_RECYCLE_PAGES:
         logger.info(

tradein-mvp/browser/test_server_smoke.py

@@ -286,6 +286,120 @@ def test_ensure_browser_serialized_by_launch_lock(
     assert calls["n"] == 1, "launch должен произойти ровно один раз (double-check под локом)"
 
 
+# ── _safe_relaunch resets _page_counter (recycle starvation fix) ─────────────────
+
+
+def test_safe_relaunch_resets_page_counter_when_proceeds(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """_safe_relaunch сбрасывает _page_counter=0 когда relaunch проходит (inflight<=1).
+
+    Регрессия: без сброса под конкуренцией counter копился вечно (>= порога на
+    каждом fetch) → recycle голодал. Сброс атомарен с relaunch'ем под локом.
+    """
+    monkeypatch.setattr(server, "_launch_lock", asyncio.Lock())
+    monkeypatch.setattr(server, "_inflight", 1)  # мы единственный in-flight
+    monkeypatch.setattr(server, "_page_counter", 999)
+
+    relaunched = {"n": 0}
+
+    async def _fake_relaunch() -> None:
+        relaunched["n"] += 1
+
+    monkeypatch.setattr(server, "_relaunch_browser", _fake_relaunch)
+
+    asyncio.run(server._safe_relaunch("recycle"))
+
+    assert relaunched["n"] == 1, "relaunch должен выполниться при inflight<=1"
+    assert server._page_counter == 0, "counter должен сброситься при реальном relaunch"
+
+
+def test_safe_relaunch_keeps_page_counter_when_deferred(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """_safe_relaunch НЕ трогает _page_counter когда relaunch отложен (inflight>1).
+
+    Отложенный relaunch оставляет counter, чтобы следующий запрос (когда нагрузка
+    спадёт) дожал recycle — иначе recycle потерялся бы навсегда.
+    """
+    monkeypatch.setattr(server, "_launch_lock", asyncio.Lock())
+    monkeypatch.setattr(server, "_inflight", 3)  # есть параллельные запросы
+    monkeypatch.setattr(server, "_page_counter", 42)
+
+    relaunched = {"n": 0}
+
+    async def _fake_relaunch() -> None:
+        relaunched["n"] += 1
+
+    monkeypatch.setattr(server, "_relaunch_browser", _fake_relaunch)
+
+    asyncio.run(server._safe_relaunch("recycle"))
+
+    assert relaunched["n"] == 0, "relaunch должен быть отложен при inflight>1"
+    assert server._page_counter == 42, "counter НЕ трогаем при отложенном relaunch"
+
+
+# ── _login_once closes page on CancelledError (BaseException leak fix) ────────────
+
+
+class _CancellingLoginPage:
+    """Page для /login, которая бросает CancelledError внутри логина."""
+
+    def __init__(self, tracker: "_Tracker") -> None:
+        self._tracker = tracker
+        self.url = "https://cian.example/login"
+
+    async def goto(self, url: str, **kwargs: Any) -> None:
+        return None
+
+    async def wait_for_timeout(self, ms: int) -> None:
+        return None
+
+    async def fill(self, selector: str, value: str, **kwargs: Any) -> None:
+        # Отмена таска (BaseException, не Exception) — обходит except Exception.
+        raise asyncio.CancelledError()
+
+    async def close(self) -> None:
+        self._tracker.closed += 1
+
+
+class _CancellingLoginBrowser:
+    def __init__(self, tracker: "_Tracker") -> None:
+        self._tracker = tracker
+
+    async def new_page(self) -> _CancellingLoginPage:
+        self._tracker.opened += 1
+        return _CancellingLoginPage(self._tracker)
+
+
+def test_login_closes_page_on_cancelled_error(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """_login_once закрывает страницу даже при CancelledError (finally, не except).
+
+    CancelledError — BaseException, обходит except Exception, без finally страница
+    утекала бы при отмене таска (shutdown / dropped client).
+    """
+    tracker = _Tracker()
+    browser = _CancellingLoginBrowser(tracker)
+    monkeypatch.setattr(server, "_browser", browser)
+
+    params = {
+        "url": "https://cian.example/login",
+        "email": "u@example.com",
+        "password": "secret",
+        "email_selector": "#email",
+        "password_selector": "#password",
+        "submit_selector": "#submit",
+    }
+
+    with pytest.raises(asyncio.CancelledError):
+        asyncio.run(server._login_once(params))
+
+    assert tracker.opened == 1
+    assert tracker.closed == 1, "страница должна закрыться в finally при CancelledError"
+
+
 async def _coro(value: Any) -> Any:
     """Хелпер: оборачивает значение в awaitable для подмены request.json()."""
     return value