diff --git a/deploy/docker/crawler_pool.py b/deploy/docker/crawler_pool.py index cdde8bba1..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,11 +160,100 @@ 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 _retire_browser( + crawler: AsyncWebCrawler, drain_timeout: float = 30, close_timeout: float = 60 +): + """Let requests already running on a detached browser finish, then close it. + + ``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 + + +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, DEFAULT_CONFIG_SIG + sig = _sig(_apply_pool_defaults(cfg)) + + 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.shield(_retire_browser(old)) + except asyncio.TimeoutError: + 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(): """Close all browsers.""" async with LOCK: diff --git a/deploy/docker/monitor_routes.py b/deploy/docker/monitor_routes.py index 5faf33d60..5a748d079 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 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() 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 + # 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"} @@ -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() 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 diff --git a/tests/docker/test_pool_restart_permanent.py b/tests/docker/test_pool_restart_permanent.py new file mode 100644 index 000000000..d57ce7f98 --- /dev/null +++ b/tests/docker/test_pool_restart_permanent.py @@ -0,0 +1,334 @@ +"""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. + +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 +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 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. + + ``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 +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 = [] + FakeCrawler.next_start_error = FakeCrawler.next_start_gate = None + 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 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) + old = pool.PERMANENT + old.close_gate = asyncio.Event() # close() will never return + + restart = asyncio.create_task(pool.restart_permanent(cfg)) + 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) + 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. + + 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)