fix(docker): restart_browser on the permanent browser deadlocks the pool

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:

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()

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