From fd607fdfbb24bdd31c68d3fa2eed5ca0a80967a0 Mon Sep 17 00:00:00 2001 From: Talel Boussetta Date: Fri, 25 Sep 2026 10:07:32 +0100 Subject: [PATCH 1/6] fix(docker): restart_browser on the permanent browser deadlocks the pool POST /monitor/actions/restart_browser {"sig": "permanent"} took the pool LOCK and then called init_permanent(), which acquires the same lock. asyncio.Lock is not reentrant, so the route never returned and every later get_crawler() blocked behind it for the life of the process. GET /health kept answering OK throughout, because health never opens a page. Two smaller faults in the same branch: the module-level PERMANENT name is a stale binding that cannot be written back, and the global was never cleared, so init_permanent()'s "already initialized" guard would have made the re-create a no-op even without the deadlock. The rebuilt config also dropped the CRAWL4AI_CHROMIUM_SANDBOX handling that startup applies, so a restart could hand back a browser with --no-sandbox an operator had turned off. The pool lifecycle now lives in the pool: restart_permanent() detaches under LOCK, then closes and re-creates outside it. Browser closes in kill_browser and the hot/cold branch move out of LOCK for the same reason -- the browser is already detached, so a close that hangs no longer freezes get_crawler(). The old browser's close is bounded at 60 s, so a close that hangs cannot stall the restart request itself. --- deploy/docker/crawler_pool.py | 32 +++++++++++++++++++++ deploy/docker/monitor_routes.py | 49 ++++++++++++++++++--------------- 2 files changed, 59 insertions(+), 22 deletions(-) diff --git a/deploy/docker/crawler_pool.py b/deploy/docker/crawler_pool.py index cdde8bba1..785c6395b 100644 --- a/deploy/docker/crawler_pool.py +++ b/deploy/docker/crawler_pool.py @@ -159,6 +159,38 @@ async def init_permanent(cfg: BrowserConfig): LAST_USED[DEFAULT_CONFIG_SIG] = time.time() USAGE_COUNT[DEFAULT_CONFIG_SIG] = 0 +async def restart_permanent(cfg: BrowserConfig): + """Replace the permanent browser with a freshly started one. + + Only the detach happens under LOCK. The close and the re-create must not: + ``init_permanent()`` acquires LOCK itself and ``asyncio.Lock`` is not + reentrant, so restarting while holding it deadlocks the pool for the life + of the process, and ``close()`` on a wedged browser would block every + ``get_crawler()`` for as long as it hangs. + + Clearing the global is what lets ``init_permanent()`` past its + "already initialized" guard. Between the detach and the re-create a + request carrying the default config falls through to the normal pool + path and creates a cold-pool browser; the janitor reaps it once idle. + + The old browser's close is bounded, so a wedged browser cannot hang the + restart request itself. + """ + global PERMANENT + async with LOCK: + old, PERMANENT = PERMANENT, None + + if old: + try: + await asyncio.wait_for(old.close(), timeout=60) + except asyncio.TimeoutError: + logger.warning("Timed out closing old permanent browser; continuing restart") + except Exception: + pass + + await init_permanent(cfg) + + async def close_all(): """Close all browsers.""" async with LOCK: diff --git a/deploy/docker/monitor_routes.py b/deploy/docker/monitor_routes.py index 5faf33d60..7d3a23e5a 100644 --- a/deploy/docker/monitor_routes.py +++ b/deploy/docker/monitor_routes.py @@ -235,12 +235,15 @@ async def kill_browser(req: KillBrowserRequest): else: browser = COLD_POOL.pop(target_sig) - with suppress(Exception): - await browser.close() - LAST_USED.pop(target_sig, None) USAGE_COUNT.pop(target_sig, None) + # Closed outside LOCK: the browser is already detached from the pool, so + # nothing else can hand it out, and a close that hangs no longer blocks + # every get_crawler() behind it. + with suppress(Exception): + await browser.close() + logger.info(f"🔪 Killed {pool_type} browser (sig={target_sig[:8]})") monitor = get_monitor() @@ -262,26 +265,25 @@ async def restart_browser(req: KillBrowserRequest): sig: Browser config signature (first 8 chars), or "permanent" """ try: - from crawler_pool import (PERMANENT, HOT_POOL, COLD_POOL, LAST_USED, - USAGE_COUNT, LOCK, DEFAULT_CONFIG_SIG, init_permanent) - from crawl4ai import AsyncWebCrawler, BrowserConfig + from crawler_pool import (HOT_POOL, COLD_POOL, LAST_USED, + USAGE_COUNT, LOCK, DEFAULT_CONFIG_SIG, + restart_permanent) + from crawl4ai import BrowserConfig from contextlib import suppress - import time # Handle permanent browser restart if req.sig == "permanent" or (DEFAULT_CONFIG_SIG and DEFAULT_CONFIG_SIG.startswith(req.sig)): - async with LOCK: - if PERMANENT: - with suppress(Exception): - await PERMANENT.close() - - # Reinitialize permanent - from utils import load_config - config = load_config() - await init_permanent(BrowserConfig( - extra_args=config["crawler"]["browser"].get("extra_args", []), - **config["crawler"]["browser"].get("kwargs", {}), - )) + # restart_permanent() does the detach under LOCK and the close and + # re-create outside it. Doing any of that here, under LOCK, is what + # used to deadlock the pool: init_permanent() takes the same + # non-reentrant lock. + from utils import load_config + from server import _browser_extra_args + config = load_config() + await restart_permanent(BrowserConfig( + extra_args=_browser_extra_args(), + **config["crawler"]["browser"].get("kwargs", {}), + )) logger.info("🔄 Restarted permanent browser") return {"success": True, "restarted": "permanent"} @@ -316,14 +318,17 @@ async def restart_browser(req: KillBrowserRequest): else: browser = COLD_POOL.pop(target_sig) - with suppress(Exception): - await browser.close() - # Note: We can't easily recreate with same config without storing it # For now, just kill and let new requests create fresh ones LAST_USED.pop(target_sig, None) USAGE_COUNT.pop(target_sig, None) + # Closed outside LOCK: the browser is already detached from the pool, so + # nothing else can hand it out, and a close that hangs no longer blocks + # every get_crawler() behind it. + with suppress(Exception): + await browser.close() + logger.info(f"🔄 Restarted {pool_type} browser (sig={target_sig[:8]})") monitor = get_monitor() From ce39320bc378a4e7b992e39ebffae3997aad2ac7 Mon Sep 17 00:00:00 2001 From: Talel Boussetta Date: Sun, 6 Sep 2026 09:26:06 +0100 Subject: [PATCH 2/6] test(docker): regression tests for the permanent-browser restart --- tests/docker/test_pool_restart_permanent.py | 162 ++++++++++++++++++++ 1 file changed, 162 insertions(+) create mode 100644 tests/docker/test_pool_restart_permanent.py diff --git a/tests/docker/test_pool_restart_permanent.py b/tests/docker/test_pool_restart_permanent.py new file mode 100644 index 000000000..bff859a3b --- /dev/null +++ b/tests/docker/test_pool_restart_permanent.py @@ -0,0 +1,162 @@ +"""Tests for crawler_pool.restart_permanent(). + +These exercise the real `deploy/docker/crawler_pool.py` module. `crawl4ai` and +`utils` are stubbed before the import so the pool's lifecycle can be tested +without Playwright, a browser, or a config file — no Docker and no running +server needed. + +Regression coverage for the permanent-browser restart, which used to call +init_permanent() while holding the pool LOCK. asyncio.Lock is not reentrant, +so that deadlocked the pool for the life of the process: every later +get_crawler() blocked forever while /health kept answering OK. +""" + +import asyncio +import importlib +import sys +import types +from pathlib import Path + +import pytest + +DOCKER_DIR = Path(__file__).resolve().parents[2] / "deploy" / "docker" + + +# --------------------------------------------------------------------------- +# Stubs for the two modules crawler_pool imports at module scope +# --------------------------------------------------------------------------- + + +class FakeBrowserConfig: + """Minimal stand-in: crawler_pool only needs to_dict() for the signature.""" + + def __init__(self, **kwargs): + self.kwargs = kwargs + + def to_dict(self): + return dict(self.kwargs) + + +class FakeCrawler: + """Records start/close so a test can assert what happened to a browser.""" + + instances = [] + + def __init__(self, config=None, thread_safe=False): + self.config = config + self.started = False + self.closed = False + self.close_gate = None # set to an asyncio.Event to hang close() + FakeCrawler.instances.append(self) + + async def start(self): + self.started = True + + async def close(self): + if self.close_gate is not None: + await self.close_gate.wait() + self.closed = True + + +@pytest.fixture +def pool(): + """Import crawler_pool against the stubs, fresh for every test.""" + fake_crawl4ai = types.ModuleType("crawl4ai") + fake_crawl4ai.AsyncWebCrawler = FakeCrawler + fake_crawl4ai.BrowserConfig = FakeBrowserConfig + + fake_utils = types.ModuleType("utils") + fake_utils.load_config = lambda: { + "crawler": {"memory_threshold_percent": 95.0, "pool": {"idle_ttl_sec": 300}} + } + fake_utils.get_container_memory_percent = lambda: 10.0 + + saved = {name: sys.modules.get(name) for name in ("crawl4ai", "utils", "crawler_pool")} + sys.modules["crawl4ai"] = fake_crawl4ai + sys.modules["utils"] = fake_utils + sys.modules.pop("crawler_pool", None) + sys.path.insert(0, str(DOCKER_DIR)) + + FakeCrawler.instances = [] + try: + yield importlib.import_module("crawler_pool") + finally: + sys.path.remove(str(DOCKER_DIR)) + for name, module in saved.items(): + if module is None: + sys.modules.pop(name, None) + else: + sys.modules[name] = module + + +# --------------------------------------------------------------------------- +# Tests +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_restart_permanent_replaces_the_browser(pool): + cfg = FakeBrowserConfig(headless=True) + await pool.init_permanent(cfg) + first = pool.PERMANENT + sig_before = pool.DEFAULT_CONFIG_SIG + + await asyncio.wait_for(pool.restart_permanent(cfg), timeout=5) + + assert first.closed, "the old permanent browser should be closed" + assert pool.PERMANENT is not first, "a new browser should have replaced it" + assert pool.PERMANENT.started, "the replacement should be started" + assert pool.DEFAULT_CONFIG_SIG == sig_before, "same config, same signature" + + +@pytest.mark.asyncio +async def test_restart_permanent_leaves_the_pool_usable(pool): + """The regression: after a restart the pool LOCK must still be free.""" + cfg = FakeBrowserConfig(headless=True) + await pool.init_permanent(cfg) + + await asyncio.wait_for(pool.restart_permanent(cfg), timeout=5) + + crawler = await asyncio.wait_for(pool.get_crawler(cfg), timeout=5) + assert crawler is pool.PERMANENT + + +@pytest.mark.asyncio +async def test_a_hanging_close_does_not_block_the_pool(pool): + """A browser that will not close must not take the whole server with it. + + The restart itself waits on the close, but it holds no lock while it does, + so unrelated requests keep being served. + """ + cfg = FakeBrowserConfig(headless=True) + await pool.init_permanent(cfg) + pool.PERMANENT.close_gate = asyncio.Event() # close() will never return + + restart = asyncio.create_task(pool.restart_permanent(cfg)) + await asyncio.sleep(0) # let it reach the close + + other = FakeBrowserConfig(headless=True, text_mode=True) + crawler = await asyncio.wait_for(pool.get_crawler(other), timeout=5) + assert crawler.started + + restart.cancel() + await asyncio.gather(restart, return_exceptions=True) + + +@pytest.mark.asyncio +async def test_init_permanent_under_the_lock_deadlocks(pool): + """Documents the original bug: the route did exactly this. + + Kept as a guard on the assumption the fix rests on — that LOCK is a plain, + non-reentrant asyncio.Lock. If this ever stops timing out, restart_permanent + can be simplified; until then, nothing may call init_permanent() while + holding it. + """ + cfg = FakeBrowserConfig(headless=True) + + async def restart_the_old_way(): + async with pool.LOCK: + await pool.init_permanent(cfg) + + with pytest.raises(asyncio.TimeoutError): + await asyncio.wait_for(restart_the_old_way(), timeout=1) From 303873f12846a3f87406b01a71f181da2649a9eb Mon Sep 17 00:00:00 2001 From: Talel Boussetta Date: Fri, 25 Sep 2026 10:11:07 +0100 Subject: [PATCH 3/6] fix(docker): rebuild the permanent browser on restart the way startup does Since #2237 the permanent browser is created from get_default_browser_config(), which applies the egress hardening, and requests reach it by pool signature. The restart path assembled its own BrowserConfig without that hardening, so the restarted browser got a signature no request carries (and ran with TLS verification off and no pinning proxy): it sat unused while every default-config request created a cold-pool browser instead. Restart now calls get_default_browser_config(), so the restarted browser is the one startup would build. --- deploy/docker/monitor_routes.py | 15 ++++--- .../tests/test_restart_permanent_config.py | 40 +++++++++++++++++++ 2 files changed, 47 insertions(+), 8 deletions(-) create mode 100644 deploy/docker/tests/test_restart_permanent_config.py diff --git a/deploy/docker/monitor_routes.py b/deploy/docker/monitor_routes.py index 7d3a23e5a..88726811a 100644 --- a/deploy/docker/monitor_routes.py +++ b/deploy/docker/monitor_routes.py @@ -268,7 +268,6 @@ async def restart_browser(req: KillBrowserRequest): from crawler_pool import (HOT_POOL, COLD_POOL, LAST_USED, USAGE_COUNT, LOCK, DEFAULT_CONFIG_SIG, restart_permanent) - from crawl4ai import BrowserConfig from contextlib import suppress # Handle permanent browser restart @@ -277,13 +276,13 @@ async def restart_browser(req: KillBrowserRequest): # re-create outside it. Doing any of that here, under LOCK, is what # used to deadlock the pool: init_permanent() takes the same # non-reentrant lock. - from utils import load_config - from server import _browser_extra_args - config = load_config() - await restart_permanent(BrowserConfig( - extra_args=_browser_extra_args(), - **config["crawler"]["browser"].get("kwargs", {}), - )) + # + # Rebuilt with the same builder startup uses. It applies the egress + # hardening, and the pool signature it yields is the one incoming + # requests are matched against; a config assembled here would give + # the restarted browser a signature no request carries. + from server import get_default_browser_config + await restart_permanent(get_default_browser_config()) logger.info("🔄 Restarted permanent browser") return {"success": True, "restarted": "permanent"} diff --git a/deploy/docker/tests/test_restart_permanent_config.py b/deploy/docker/tests/test_restart_permanent_config.py new file mode 100644 index 000000000..2782ea134 --- /dev/null +++ b/deploy/docker/tests/test_restart_permanent_config.py @@ -0,0 +1,40 @@ +"""The restarted permanent browser must be built the way startup builds it. + +Startup creates the permanent browser from get_default_browser_config(), which +applies the egress hardening (#2237), and incoming requests reach it by pool +signature. A restart that assembled its own BrowserConfig got a different +signature, so no request matched the restarted browser; it sat idle while +default-config requests built cold-pool browsers instead. +""" + +import asyncio + + +def test_restart_rebuilds_permanent_browser_from_startup_config( + server_module, monkeypatch +): + import crawler_pool + import monitor_routes + + rebuilt = [] + + async def fake_restart_permanent(cfg): + rebuilt.append(cfg) + + monkeypatch.setattr(crawler_pool, "restart_permanent", fake_restart_permanent) + + result = asyncio.run( + monitor_routes.restart_browser( + monitor_routes.KillBrowserRequest(sig="permanent") + ) + ) + + assert result == {"success": True, "restarted": "permanent"} + assert len(rebuilt) == 1 + + def pool_sig(cfg): + return crawler_pool._sig(crawler_pool._apply_pool_defaults(cfg)) + + assert pool_sig(rebuilt[0]) == pool_sig(server_module.get_default_browser_config()) + # The hardening itself, not only the signature. + assert rebuilt[0].ignore_https_errors is False From 2576a871e3a4df121363a148a732628cdea1fcfb Mon Sep 17 00:00:00 2001 From: Talel Boussetta Date: Fri, 25 Sep 2026 11:06:58 +0100 Subject: [PATCH 4/6] docs(docker): say what must not run under LOCK in restart_permanent --- deploy/docker/crawler_pool.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/deploy/docker/crawler_pool.py b/deploy/docker/crawler_pool.py index 785c6395b..103b18368 100644 --- a/deploy/docker/crawler_pool.py +++ b/deploy/docker/crawler_pool.py @@ -162,9 +162,9 @@ async def init_permanent(cfg: BrowserConfig): async def restart_permanent(cfg: BrowserConfig): """Replace the permanent browser with a freshly started one. - Only the detach happens under LOCK. The close and the re-create must not: - ``init_permanent()`` acquires LOCK itself and ``asyncio.Lock`` is not - reentrant, so restarting while holding it deadlocks the pool for the life + Only the detach happens under LOCK. The close and the re-create must not run + under it: ``init_permanent()`` acquires LOCK itself and ``asyncio.Lock`` is + not reentrant, so restarting while holding it deadlocks the pool for the life of the process, and ``close()`` on a wedged browser would block every ``get_crawler()`` for as long as it hangs. From 3233e9b79d9e446c342c2d87a8c86aa06b5fdc81 Mon Sep 17 00:00:00 2001 From: Talel Boussetta Date: Sat, 26 Sep 2026 16:59:10 +0100 Subject: [PATCH 5/6] style(docker): black-format the lines restart_permanent adds Wraps the two new lines over black's limit and adds the blank lines black expects before the new function and after the local import. No behaviour change. --- deploy/docker/crawler_pool.py | 5 ++++- deploy/docker/monitor_routes.py | 1 + tests/docker/test_pool_restart_permanent.py | 4 +++- 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/deploy/docker/crawler_pool.py b/deploy/docker/crawler_pool.py index 103b18368..8c8eaf327 100644 --- a/deploy/docker/crawler_pool.py +++ b/deploy/docker/crawler_pool.py @@ -159,6 +159,7 @@ async def init_permanent(cfg: BrowserConfig): LAST_USED[DEFAULT_CONFIG_SIG] = time.time() USAGE_COUNT[DEFAULT_CONFIG_SIG] = 0 + async def restart_permanent(cfg: BrowserConfig): """Replace the permanent browser with a freshly started one. @@ -184,7 +185,9 @@ async def restart_permanent(cfg: BrowserConfig): try: await asyncio.wait_for(old.close(), timeout=60) except asyncio.TimeoutError: - logger.warning("Timed out closing old permanent browser; continuing restart") + logger.warning( + "Timed out closing old permanent browser; continuing restart" + ) except Exception: pass diff --git a/deploy/docker/monitor_routes.py b/deploy/docker/monitor_routes.py index 88726811a..5060d2315 100644 --- a/deploy/docker/monitor_routes.py +++ b/deploy/docker/monitor_routes.py @@ -282,6 +282,7 @@ async def restart_browser(req: KillBrowserRequest): # requests are matched against; a config assembled here would give # the restarted browser a signature no request carries. from server import get_default_browser_config + await restart_permanent(get_default_browser_config()) logger.info("🔄 Restarted permanent browser") diff --git a/tests/docker/test_pool_restart_permanent.py b/tests/docker/test_pool_restart_permanent.py index bff859a3b..8a00d41fa 100644 --- a/tests/docker/test_pool_restart_permanent.py +++ b/tests/docker/test_pool_restart_permanent.py @@ -71,7 +71,9 @@ def pool(): } fake_utils.get_container_memory_percent = lambda: 10.0 - saved = {name: sys.modules.get(name) for name in ("crawl4ai", "utils", "crawler_pool")} + saved = { + name: sys.modules.get(name) for name in ("crawl4ai", "utils", "crawler_pool") + } sys.modules["crawl4ai"] = fake_crawl4ai sys.modules["utils"] = fake_utils sys.modules.pop("crawler_pool", None) From b39939664e5ab980c510078991f9d884bd0a1ef5 Mon Sep 17 00:00:00 2001 From: Talel Boussetta Date: Tue, 13 Oct 2026 21:21:38 +0100 Subject: [PATCH 6/6] fix(docker): start the new permanent browser before retiring the old one Detaching the permanent browser, closing it and only then creating the replacement left two failure modes in the restart path, found by live testing: - While PERMANENT was None, a default-config request created a cold-pool browser with DEFAULT_CONFIG_SIG. It kept sharing LAST_USED with the new permanent browser, so the janitor never saw it idle while default traffic continued, and reaping it later popped the permanent browser's counters. - init_permanent() assigned PERMANENT before start(). A failed start left an unstarted global that arun() would start again on every concurrent request, launching browsers nothing tracked. restart_permanent() now starts the replacement outside LOCK, swaps it in under LOCK, and retires the old browser afterwards, so PERMANENT is never None and a failed or cancelled start leaves the old browser serving. Retiring waits up to 30s for requests already running on the old browser, bounds the close at 60s, and stops the old browser's own Playwright driver if the close fails or hangs, since BrowserManager.close() only stops it after browser.close() succeeds. The failure is logged instead of swallowed. init_permanent() publishes PERMANENT only after start() succeeds. Tests cover a default-config request during a slow restart, a failing start (restart and startup), a restart cancelled mid-start, a request holding the old browser, a failing and a hanging close, and concurrent restarts. --- deploy/docker/crawler_pool.py | 114 +++++++++--- deploy/docker/monitor_routes.py | 8 +- tests/docker/test_pool_restart_permanent.py | 184 +++++++++++++++++++- 3 files changed, 268 insertions(+), 38 deletions(-) diff --git a/deploy/docker/crawler_pool.py b/deploy/docker/crawler_pool.py index 8c8eaf327..e4b35cf06 100644 --- a/deploy/docker/crawler_pool.py +++ b/deploy/docker/crawler_pool.py @@ -146,6 +146,12 @@ async def release_crawler(crawler: AsyncWebCrawler): if hasattr(crawler, 'active_requests'): crawler.active_requests = max(0, crawler.active_requests - 1) +async def _discard_browser(crawler: AsyncWebCrawler, timeout: float = 30): + """Best-effort teardown of a browser that never entered the pool.""" + with suppress(Exception): + await asyncio.wait_for(crawler.close(), timeout=timeout) + + async def init_permanent(cfg: BrowserConfig): """Initialize permanent default browser.""" global PERMANENT, DEFAULT_CONFIG_SIG @@ -154,44 +160,98 @@ async def init_permanent(cfg: BrowserConfig): return DEFAULT_CONFIG_SIG = _sig(_apply_pool_defaults(cfg)) logger.info("🔥 Creating permanent default browser") - PERMANENT = AsyncWebCrawler(config=cfg, thread_safe=False) - await PERMANENT.start() + crawler = AsyncWebCrawler(config=cfg, thread_safe=False) + try: + await crawler.start() + except BaseException: + await _discard_browser(crawler) + raise + # Published only once it is running. arun() starts a crawler that is + # not ready, so a PERMANENT assigned before start() succeeded could be + # started again by every concurrent request, each launching a browser + # nothing tracks. + PERMANENT = crawler LAST_USED[DEFAULT_CONFIG_SIG] = time.time() USAGE_COUNT[DEFAULT_CONFIG_SIG] = 0 -async def restart_permanent(cfg: BrowserConfig): - """Replace the permanent browser with a freshly started one. +async def _retire_browser( + crawler: AsyncWebCrawler, drain_timeout: float = 30, close_timeout: float = 60 +): + """Let requests already running on a detached browser finish, then close it. - Only the detach happens under LOCK. The close and the re-create must not run - under it: ``init_permanent()`` acquires LOCK itself and ``asyncio.Lock`` is - not reentrant, so restarting while holding it deadlocks the pool for the life - of the process, and ``close()`` on a wedged browser would block every - ``get_crawler()`` for as long as it hangs. + ``BrowserManager.close()`` stops its Playwright driver only after + ``browser.close()`` succeeds, so a close that raises or hangs leaves Chromium + and its driver running. In that case the driver is stopped directly: on exit + it kills every browser process it launched. Each browser has its own driver + (``BrowserManager.start()``), so this cannot touch the browser that replaced + this one. + """ + deadline = time.monotonic() + drain_timeout + while getattr(crawler, "active_requests", 0) > 0 and time.monotonic() < deadline: + await asyncio.sleep(0.2) + try: + await asyncio.wait_for(crawler.close(), timeout=close_timeout) + except BaseException: + manager = getattr( + getattr(crawler, "crawler_strategy", None), "browser_manager", None + ) + driver = getattr(manager, "playwright", None) + # A cached CDP connection shares its driver with other managers. + if driver is not None and not getattr(manager, "_using_cached_cdp", False): + with suppress(Exception): + await asyncio.wait_for(driver.stop(), timeout=10) + raise - Clearing the global is what lets ``init_permanent()`` past its - "already initialized" guard. Between the detach and the re-create a - request carrying the default config falls through to the normal pool - path and creates a cold-pool browser; the janitor reaps it once idle. - The old browser's close is bounded, so a wedged browser cannot hang the - restart request itself. +async def restart_permanent(cfg: BrowserConfig): + """Replace the permanent browser: start the new one, swap it in, retire the old. + + The new browser is started first, outside LOCK, and only swapped in under + LOCK once it is running. So: + + - ``PERMANENT`` is never ``None`` while the server is up. A default-config + request during the restart is served by the old browser, and cannot create + a second default-signature browser in the cold pool that the janitor would + never see as idle. + - If the new browser fails to start, or the restart is cancelled while it + starts, it is torn down and the old browser keeps serving. + - Nothing is awaited under LOCK but the swap itself. ``init_permanent()`` + acquires LOCK and ``asyncio.Lock`` is not reentrant, so restarting while + holding it deadlocks the pool for the life of the process, and a + ``close()`` on a wedged browser would block every ``get_crawler()``. + + The old browser is retired after the swap (see ``_retire_browser``) and the + retire is shielded, so a cancelled request cannot abort it halfway and orphan + the old Chromium. The cost is that two browsers run together for a short + time, which is a memory peak. The restart request itself can take up to the + drain timeout plus the close timeout when requests are still running. """ - global PERMANENT - async with LOCK: - old, PERMANENT = PERMANENT, None + global PERMANENT, DEFAULT_CONFIG_SIG + sig = _sig(_apply_pool_defaults(cfg)) - if old: + new = AsyncWebCrawler(config=cfg, thread_safe=False) + swapped = False + try: + await new.start() + async with LOCK: + old, PERMANENT, DEFAULT_CONFIG_SIG = PERMANENT, new, sig + LAST_USED[sig] = time.time() + USAGE_COUNT.setdefault(sig, 0) + swapped = True + finally: + # CancelledError is not an Exception, so only a finally also covers a + # restart that is cancelled while the new browser is starting. + if not swapped: + await _discard_browser(new) + + if old is not None: try: - await asyncio.wait_for(old.close(), timeout=60) + await asyncio.shield(_retire_browser(old)) except asyncio.TimeoutError: - logger.warning( - "Timed out closing old permanent browser; continuing restart" - ) - except Exception: - pass - - await init_permanent(cfg) + logger.warning("Timed out closing the old permanent browser") + except Exception as e: + logger.warning(f"Error closing the old permanent browser: {e}") async def close_all(): diff --git a/deploy/docker/monitor_routes.py b/deploy/docker/monitor_routes.py index 5060d2315..5a748d079 100644 --- a/deploy/docker/monitor_routes.py +++ b/deploy/docker/monitor_routes.py @@ -272,10 +272,10 @@ async def restart_browser(req: KillBrowserRequest): # Handle permanent browser restart if req.sig == "permanent" or (DEFAULT_CONFIG_SIG and DEFAULT_CONFIG_SIG.startswith(req.sig)): - # restart_permanent() does the detach under LOCK and the close and - # re-create outside it. Doing any of that here, under LOCK, is what - # used to deadlock the pool: init_permanent() takes the same - # non-reentrant lock. + # restart_permanent() starts the replacement, swaps it in under LOCK + # and retires the old browser outside it. Doing any of that here, + # under LOCK, is what used to deadlock the pool: init_permanent() + # takes the same non-reentrant lock. # # Rebuilt with the same builder startup uses. It applies the egress # hardening, and the pool signature it yields is the one incoming diff --git a/tests/docker/test_pool_restart_permanent.py b/tests/docker/test_pool_restart_permanent.py index 8a00d41fa..d57ce7f98 100644 --- a/tests/docker/test_pool_restart_permanent.py +++ b/tests/docker/test_pool_restart_permanent.py @@ -9,6 +9,10 @@ init_permanent() while holding the pool LOCK. asyncio.Lock is not reentrant, so that deadlocked the pool for the life of the process: every later get_crawler() blocked forever while /health kept answering OK. + +The restart starts the replacement first, swaps it in under LOCK, then drains +and closes the old browser, so PERMANENT is never None and a failed start +leaves the old browser serving. """ import asyncio @@ -37,25 +41,58 @@ def to_dict(self): return dict(self.kwargs) +class FakeDriver: + """Stands in for the Playwright driver that owns a browser's processes.""" + + def __init__(self): + self.stopped = False + + async def stop(self): + self.stopped = True + + class FakeCrawler: - """Records start/close so a test can assert what happened to a browser.""" + """Records start/close so a test can assert what happened to a browser. + + ``next_start_*`` are consumed by the next crawler the pool creates. The + ``close_*`` knobs are set on an existing instance, e.g. ``pool.PERMANENT``. + """ instances = [] + next_start_error = None # raised by the next start() + next_start_gate = None # asyncio.Event the next start() waits on def __init__(self, config=None, thread_safe=False): self.config = config self.started = False self.closed = False self.close_gate = None # set to an asyncio.Event to hang close() + self.close_error = None # set to an exception close() raises + self.start_error = FakeCrawler.next_start_error + self.start_gate = FakeCrawler.next_start_gate + FakeCrawler.next_start_error = FakeCrawler.next_start_gate = None + self.driver = FakeDriver() + self.crawler_strategy = types.SimpleNamespace( + browser_manager=types.SimpleNamespace(playwright=self.driver) + ) FakeCrawler.instances.append(self) async def start(self): + if self.start_gate is not None: + await self.start_gate.wait() + if self.start_error is not None: + raise self.start_error self.started = True async def close(self): if self.close_gate is not None: await self.close_gate.wait() + if self.close_error is not None: + # Like BrowserManager.close(): the driver is only stopped after + # browser.close() succeeds, so a failing close leaves it running. + raise self.close_error self.closed = True + self.driver.stopped = True @pytest.fixture @@ -80,6 +117,7 @@ def pool(): sys.path.insert(0, str(DOCKER_DIR)) FakeCrawler.instances = [] + FakeCrawler.next_start_error = FakeCrawler.next_start_gate = None try: yield importlib.import_module("crawler_pool") finally: @@ -127,24 +165,156 @@ async def test_restart_permanent_leaves_the_pool_usable(pool): async def test_a_hanging_close_does_not_block_the_pool(pool): """A browser that will not close must not take the whole server with it. - The restart itself waits on the close, but it holds no lock while it does, - so unrelated requests keep being served. + The old browser is retired outside LOCK, after the swap, so requests are + served by the replacement while the old one is still closing. """ cfg = FakeBrowserConfig(headless=True) await pool.init_permanent(cfg) - pool.PERMANENT.close_gate = asyncio.Event() # close() will never return + old = pool.PERMANENT + old.close_gate = asyncio.Event() # close() will never return restart = asyncio.create_task(pool.restart_permanent(cfg)) - await asyncio.sleep(0) # let it reach the close + await asyncio.sleep(0.1) # swapped in, now stuck retiring the old browser + crawler = await asyncio.wait_for(pool.get_crawler(cfg), timeout=5) + assert crawler is pool.PERMANENT and crawler is not old other = FakeBrowserConfig(headless=True, text_mode=True) - crawler = await asyncio.wait_for(pool.get_crawler(other), timeout=5) - assert crawler.started + assert (await asyncio.wait_for(pool.get_crawler(other), timeout=5)).started restart.cancel() await asyncio.gather(restart, return_exceptions=True) +@pytest.mark.asyncio +async def test_default_request_during_a_slow_restart_creates_no_duplicate(pool): + """PERMANENT stays set while the replacement starts, so a default-config + request is served by the old browser and nothing lands in the cold pool.""" + cfg = FakeBrowserConfig(headless=True) + await pool.init_permanent(cfg) + old = pool.PERMANENT + + FakeCrawler.next_start_gate = asyncio.Event() + gate = FakeCrawler.next_start_gate + restart = asyncio.create_task(pool.restart_permanent(cfg)) + await asyncio.sleep(0.1) # the new browser is starting, not yet swapped in + + served = await asyncio.wait_for(pool.get_crawler(cfg), timeout=5) + assert served is old + assert not pool.COLD_POOL and not pool.HOT_POOL + + gate.set() + await pool.release_crawler(served) + await asyncio.wait_for(restart, timeout=5) + assert pool.PERMANENT is not old + assert not pool.COLD_POOL and not pool.HOT_POOL + + +@pytest.mark.asyncio +async def test_failed_start_leaves_the_old_browser_serving(pool): + cfg = FakeBrowserConfig(headless=True) + await pool.init_permanent(cfg) + old = pool.PERMANENT + + FakeCrawler.next_start_error = RuntimeError("chromium is not executable") + with pytest.raises(RuntimeError): + await asyncio.wait_for(pool.restart_permanent(cfg), timeout=5) + + failed = FakeCrawler.instances[-1] + assert pool.PERMANENT is old and old.started and not old.closed + assert failed is not old and not failed.started + assert failed.closed, "the browser that failed to start must be torn down" + assert await pool.get_crawler(cfg) is old + + +@pytest.mark.asyncio +async def test_restart_cancelled_while_starting_tears_the_new_browser_down(pool): + cfg = FakeBrowserConfig(headless=True) + await pool.init_permanent(cfg) + old = pool.PERMANENT + + FakeCrawler.next_start_gate = asyncio.Event() # start() never finishes + restart = asyncio.create_task(pool.restart_permanent(cfg)) + await asyncio.sleep(0.1) + restart.cancel() + await asyncio.gather(restart, return_exceptions=True) + + assert pool.PERMANENT is old and not old.closed + assert FakeCrawler.instances[-1].closed + + +@pytest.mark.asyncio +async def test_failed_start_at_startup_leaves_no_half_built_permanent(pool): + """init_permanent() used to assign PERMANENT before start(), and arun() + starts a crawler that is not ready, so a failed start left a global every + concurrent request could start again.""" + cfg = FakeBrowserConfig(headless=True) + + FakeCrawler.next_start_error = RuntimeError("chromium is not executable") + with pytest.raises(RuntimeError): + await pool.init_permanent(cfg) + + assert pool.PERMANENT is None + assert FakeCrawler.instances[-1].closed + + +@pytest.mark.asyncio +async def test_restart_waits_for_requests_on_the_old_browser(pool): + cfg = FakeBrowserConfig(headless=True) + await pool.init_permanent(cfg) + old = await pool.get_crawler(cfg) # a request now holds the old browser + assert old.active_requests == 1 + + restart = asyncio.create_task(pool.restart_permanent(cfg)) + await asyncio.sleep(0.5) + assert pool.PERMANENT is not old, "new requests already go to the new browser" + assert not old.closed, "the old browser must not be closed under a request" + + await pool.release_crawler(old) + await asyncio.wait_for(restart, timeout=5) + assert old.closed + + +@pytest.mark.asyncio +async def test_failing_close_still_stops_the_old_browsers_driver(pool, caplog): + cfg = FakeBrowserConfig(headless=True) + await pool.init_permanent(cfg) + old = pool.PERMANENT + old.close_error = RuntimeError("browser.close() failed") + + with caplog.at_level("WARNING"): + await asyncio.wait_for(pool.restart_permanent(cfg), timeout=5) + + assert old.driver.stopped, "Chromium would otherwise outlive the restart" + assert not pool.PERMANENT.driver.stopped, "the new browser is untouched" + assert "browser.close() failed" in caplog.text, "the failure is logged" + + +@pytest.mark.asyncio +async def test_hanging_close_is_bounded_and_still_stops_the_driver(pool): + cfg = FakeBrowserConfig(headless=True) + await pool.init_permanent(cfg) + old = pool.PERMANENT + old.close_gate = asyncio.Event() + + with pytest.raises(asyncio.TimeoutError): + await pool._retire_browser(old, drain_timeout=0, close_timeout=0.2) + + assert old.driver.stopped + + +@pytest.mark.asyncio +async def test_concurrent_restarts_retire_every_replaced_browser(pool): + cfg = FakeBrowserConfig(headless=True) + await pool.init_permanent(cfg) + + await asyncio.wait_for( + asyncio.gather(*(pool.restart_permanent(cfg) for _ in range(3))), timeout=5 + ) + + live = [c for c in FakeCrawler.instances if c.started and not c.closed] + assert live == [pool.PERMANENT], "exactly one browser is left running" + + @pytest.mark.asyncio async def test_init_permanent_under_the_lock_deadlocks(pool): """Documents the original bug: the route did exactly this.