From e9ddd45e8c7e09042fafba1288424e6c8f12079c Mon Sep 17 00:00:00 2001 From: Mark Vletter Date: Fri, 11 Sep 2026 12:52:46 +0200 Subject: [PATCH] fix(docker): serve a hooked request from its own browser, not the shared pool A declarative hook changes the browser context it runs in, and the change outlives the request. `add_cookies` installs cookies into the context; the context belongs to a crawler handed out by `crawler_pool.get_crawler()`, which is shared -- the permanent one for the default browser config, in practice almost every request. `release_crawler()` only decrements `active_requests`, so the next request inherits whatever the last one left behind. Measured on unmodified 0.9.3 with `CRAWL4AI_HOOKS_ENABLED=true`, four sequential requests to a URL that echoes the cookies it received: 1. no hooks -> no cookie (correct) 2. add_cookies hook -> cookie present (correct) 3. no hooks -> cookie from 2 (wrong) 4. no hooks -> cookie from 2 (wrong) Sequential, so it does not need concurrency to happen. Scoping the hook per request does not fix it, which is worth stating because it is the obvious first attempt: with the hook resolved from a ContextVar set only on the requesting task, request 2's hook set was verifiably empty and it still came back carrying request 1's cookie. The cookie is in the context, not in the hook. So a request that brings hooks now gets a browser of its own, disposed when it finishes. This is the same shape the PDF branch already used, and `_dispose_crawler()` now reads an explicit `pooled` marker instead of inferring "not pooled" from the strategy type -- which only ever recognised PDF. The non-streaming handler's inline close/release branch is replaced by that same helper, so both handlers dispose crawlers through one path. Requests without hooks are untouched: same pool, same counters. Tests import the real `crawler_pool` rather than a copy of its logic, so they notice if the module changes underneath them. --- deploy/docker/api.py | 39 +++++++++++++------ deploy/docker/crawler_pool.py | 21 +++++++++++ tests/docker/test_pool_release.py | 63 +++++++++++++++++++++++++++++++ 3 files changed, 111 insertions(+), 12 deletions(-) diff --git a/deploy/docker/api.py b/deploy/docker/api.py index 09a783939..d4865ec37 100644 --- a/deploy/docker/api.py +++ b/deploy/docker/api.py @@ -378,7 +378,7 @@ async def handle_markdown_request( cache_mode = CacheMode.ENABLED if cache == "1" else CacheMode.WRITE_ONLY - from crawler_pool import get_crawler, release_crawler + from crawler_pool import get_crawler, get_unpooled_crawler, release_crawler from utils import load_config as _load_config _cfg = _load_config() browser_cfg = BrowserConfig( @@ -604,13 +604,17 @@ def create_task_response(task: dict, task_id: str, base_url: str) -> dict: return response async def _dispose_crawler(crawler): - """Close a dedicated PDF crawler (not pooled) or release a pooled one.""" - from crawl4ai.processors.pdf import PDFCrawlerStrategy + """Close a crawler that was started outside the pool, or release a pooled one. + + Whoever starts a crawler outside the pool marks it `pooled = False`, so + this does not have to infer it from the strategy type - which only ever + recognised the PDF case. + """ from crawler_pool import release_crawler - if isinstance(crawler.crawler_strategy, PDFCrawlerStrategy): - await crawler.close() - else: + if getattr(crawler, "pooled", True): await release_crawler(crawler) + else: + await crawler.close() async def stream_results(crawler: AsyncWebCrawler, results_gen: AsyncGenerator) -> AsyncGenerator[bytes, None]: """Stream results with heartbeats and completion markers.""" @@ -698,7 +702,7 @@ async def handle_crawl_request( ) if config["crawler"]["rate_limiter"]["enabled"] else None ) - from crawler_pool import get_crawler, release_crawler + from crawler_pool import get_crawler, get_unpooled_crawler, release_crawler from crawl4ai.processors.pdf import PDFContentScrapingStrategy, PDFCrawlerStrategy is_pdf_crawl = isinstance(crawler_config.scraping_strategy, PDFContentScrapingStrategy) if is_pdf_crawl: @@ -710,6 +714,13 @@ async def handle_crawl_request( # Use PDFCrawlerStrategy when scraping PDFs, as headless Chromium can't render PDFs inline crawler = AsyncWebCrawler(crawler_strategy=PDFCrawlerStrategy()) await crawler.start() + crawler.pooled = False + elif hooks_config: + # A hook changes the browser context it runs in, and that change + # outlives the request: cookies it adds stay in the context, and + # the context is shared with whatever request comes next. So a + # request that brings hooks gets a browser of its own. + crawler = await get_unpooled_crawler(browser_config) else: crawler = await get_crawler(browser_config) @@ -880,10 +891,7 @@ async def handle_crawl_request( ) finally: if crawler: - if is_pdf_crawl: - await crawler.close() # not pooled; release_crawler would be a no-op - else: - await release_crawler(crawler) + await _dispose_crawler(crawler) async def handle_stream_crawl_request( urls: List[str], @@ -926,7 +934,7 @@ async def handle_stream_crawl_request( ), ) - from crawler_pool import get_crawler + from crawler_pool import get_crawler, get_unpooled_crawler from crawl4ai.processors.pdf import PDFContentScrapingStrategy, PDFCrawlerStrategy if isinstance(crawler_config.scraping_strategy, PDFContentScrapingStrategy): if hooks_config: @@ -937,6 +945,13 @@ async def handle_stream_crawl_request( # Use PDFCrawlerStrategy when scraping PDFs, as headless Chromium can't render PDFs inline crawler = AsyncWebCrawler(crawler_strategy=PDFCrawlerStrategy()) await crawler.start() + crawler.pooled = False + elif hooks_config: + # A hook changes the browser context it runs in, and that change + # outlives the request: cookies it adds stay in the context, and + # the context is shared with whatever request comes next. So a + # request that brings hooks gets a browser of its own. + crawler = await get_unpooled_crawler(browser_config) else: crawler = await get_crawler(browser_config) diff --git a/deploy/docker/crawler_pool.py b/deploy/docker/crawler_pool.py index 516d9562a..85fda8625 100644 --- a/deploy/docker/crawler_pool.py +++ b/deploy/docker/crawler_pool.py @@ -118,6 +118,27 @@ async def get_crawler(cfg: BrowserConfig) -> AsyncWebCrawler: USAGE_COUNT[sig] = 1 return crawler +async def get_unpooled_crawler(cfg: BrowserConfig) -> AsyncWebCrawler: + """Start a crawler for one request only, outside the pool. + + Hooks are attached to the crawler that serves a request, and what they do + to its browser context - cookies, headers, routes - outlives the request. + A pooled crawler would carry that into whatever comes next, so a request + that brings hooks gets a browser of its own. Close it when done; it is in + no pool, so the janitor will never come for it. + """ + mem_pct = get_container_memory_percent() + if mem_pct >= MEM_LIMIT: + logger.error(f"💥 Memory pressure: {mem_pct:.1f}% >= {MEM_LIMIT}%") + raise MemoryError(f"Memory at {mem_pct:.1f}%, refusing new browser") + + logger.info(f"🔒 Creating unpooled browser for hooked request (mem={mem_pct:.1f}%)") + crawler = AsyncWebCrawler(config=cfg, thread_safe=False) + await crawler.start() + crawler.pooled = False + return crawler + + async def release_crawler(crawler: AsyncWebCrawler): """Decrement active request count for a pooled crawler. diff --git a/tests/docker/test_pool_release.py b/tests/docker/test_pool_release.py index 6c81b3e52..6e25b1219 100644 --- a/tests/docker/test_pool_release.py +++ b/tests/docker/test_pool_release.py @@ -8,6 +8,8 @@ import pytest from unittest.mock import MagicMock +from crawl4ai import BrowserConfig + # --------------------------------------------------------------------------- # Standalone release_crawler implementation for testing @@ -153,3 +155,64 @@ async def test_janitor_safety_check(self): # Janitor check: now safe to close should_close = getattr(crawler, "active_requests", 0) == 0 assert should_close is True + + +# --------------------------------------------------------------------------- +# Crawlers started outside the pool +# +# Unlike the tests above, these import the real crawler_pool rather than a +# copy of its logic, so they fail if the module changes underneath them. +# --------------------------------------------------------------------------- + + +class TestUnpooledCrawler: + """A request that brings hooks must not be served from the shared pool. + + Hooks change the browser context they run in, and the change outlives the + request: cookies an add_cookies hook installs stay in the context, and the + context is handed to whatever request comes next. Scoping the hook itself + is not enough - measured on 0.9.3 with a per-request hook scope, the second + request's hook set was empty and it still received the first request's + cookie. + """ + + @pytest.mark.asyncio + async def test_unpooled_crawler_is_marked_and_not_registered(self, monkeypatch): + import crawler_pool + + started = {} + + class _FakeCrawler: + def __init__(self, config=None, thread_safe=False): + self.config = config + + async def start(self): + started["called"] = True + + monkeypatch.setattr(crawler_pool, "AsyncWebCrawler", _FakeCrawler) + monkeypatch.setattr(crawler_pool, "get_container_memory_percent", lambda: 10.0) + + before_hot = dict(crawler_pool.HOT_POOL) + before_cold = dict(crawler_pool.COLD_POOL) + + crawler = await crawler_pool.get_unpooled_crawler(BrowserConfig()) + + assert started["called"] is True + # The marker _dispose_crawler() reads to decide close-vs-release. + assert crawler.pooled is False + # And it is in no pool, so the janitor will never close it for us. + assert crawler_pool.HOT_POOL == before_hot + assert crawler_pool.COLD_POOL == before_cold + assert crawler not in crawler_pool.HOT_POOL.values() + assert crawler not in crawler_pool.COLD_POOL.values() + + @pytest.mark.asyncio + async def test_unpooled_crawler_refuses_under_memory_pressure(self, monkeypatch): + """Same guard the pool applies before starting any other browser.""" + import crawler_pool + + monkeypatch.setattr( + crawler_pool, "get_container_memory_percent", lambda: crawler_pool.MEM_LIMIT + 1 + ) + with pytest.raises(MemoryError): + await crawler_pool.get_unpooled_crawler(BrowserConfig())