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

deploy/docker/crawler_pool.py

@@ -142,6 +142,35 @@ 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.
+    """
+    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:

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

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)