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

[talelboussetta]

Summary

POST /monitor/actions/restart_browser {"sig": "permanent"} deadlocks the browser pool permanently. Still the case on 0.9.4; this branch is rebased onto current develop.

The route takes the pool LOCK and then, while still holding it, calls init_permanent(), which acquires the same lock. asyncio.Lock is not reentrant, so the route never returns and every subsequent get_crawler() blocks behind it for the life of the process.

# deploy/docker/monitor_routes.py (develop)
async with LOCK:                       # <- acquired here
    if PERMANENT:
        with suppress(Exception):
            await PERMANENT.close()
    await init_permanent(BrowserConfig(...))   # <- acquires LOCK again -> deadlock

# deploy/docker/crawler_pool.py
async def init_permanent(cfg: BrowserConfig):
    global PERMANENT, DEFAULT_CONFIG_SIG
    async with LOCK:                   # <- same non-reentrant lock

Observed first on a self-hosted 0.9.2 container: the call hung for 120 s, and from that moment every /crawl on that container timed out at 90 s while GET /health still returned {"status": "ok"} and Docker reported the container (healthy). Only a container restart recovered it. Health never opens a page, so nothing an orchestrator watches goes red. Reproduced end to end on a container built from develop, below.

Two more faults in the same lines, either of which breaks the restart even without the deadlock:

1. from crawler_pool import PERMANENT binds a copy of the reference. The route cannot write the global back, and nothing clears it, so init_permanent()'s if PERMANENT: return guard would make the re-create a silent no-op, leaving a closed browser in the pool.
2. The rebuilt config is not the one startup uses. Since Build permanent browser with egress-hardened default config so its pool signature matches #2237 lifespan() builds the permanent browser with get_default_browser_config(): _browser_extra_args() (which drops --no-sandbox when CRAWL4AI_CHROMIUM_SANDBOX=true) plus the egress hardening. The route reads config["crawler"]["browser"] raw. A restart would hand back a browser with --no-sandbox an operator had turned off, without TLS verification or the pinning proxy, and under a pool signature no request carries, so it would sit unused while the endpoints that fetch through the default config (/html, /screenshot, /pdf, /execute_js) build cold-pool browsers.

The fix

Pool lifecycle moves into the pool. restart_permanent() starts the replacement outside LOCK, swaps it in under LOCK, then retires the old browser:

• PERMANENT is never None while the server is up, so a default-config request during a restart is served by the old browser and cannot create a second default-signature browser in the cold pool.
• A failed or cancelled start tears the new browser down and the old one keeps serving.
• The old browser is retired outside LOCK: up to 30s for requests already running on it to finish, the close bounded at 60s, and its own Playwright driver stopped (10s bound) if the close fails or hangs, because BrowserManager.close() only stops the driver after browser.close() succeeds. Failures are logged.
• init_permanent() assigns PERMANENT only after start() succeeds, so a failed start cannot leave an unstarted global that arun() would start again on every concurrent request.

The cost is a short memory peak with two browsers running, and a restart request that can take up to the drain plus close timeouts when requests are still running. This is Option B from the review below.

The route calls that with get_default_browser_config(), the builder lifespan() uses, so the restarted browser is the one startup would build.

Secondary, same file, same class of problem: kill_browser and the hot/cold branch of restart_browser both await browser.close() while holding LOCK. The browser is already popped from the pool at that point, so nothing else can hand it out and the close does not need the lock, but a close that hangs freezes get_crawler() for every caller. Both moved outside. This is the monitor_routes.py half of what #2214 recorded as out of scope; the two touch different functions.

List of files changed and why

• deploy/docker/crawler_pool.py: adds restart_permanent(), the start, swap and retire ordering, and the comment explaining why the ordering is the fix.
• deploy/docker/monitor_routes.py: restart_browser calls restart_permanent(get_default_browser_config()). Browser closes in kill_browser and in the hot/cold branch move out of LOCK. Drops the now-unused imports.
• tests/docker/test_pool_restart_permanent.py (new): regression tests against the real crawler_pool module.
• deploy/docker/tests/test_restart_permanent_config.py (new): the restarted browser's pool signature equals the one get_default_browser_config() gives, and TLS verification is on.

How Has This Been Tested?

Unit:

pytest tests/docker/test_pool_restart_permanent.py deploy/docker/tests/test_restart_permanent_config.py
13 passed

• The restart replaces the browser (old one closed, new one started, signature unchanged) and leaves the pool usable. That is the regression: get_crawler() still returns after a restart.
• A hanging close does not block the pool. A close that fails or hangs still stops the old browser's driver, and the failure is logged.
• A default-config request during a slow restart is served by the old browser and adds nothing to the cold or hot pool.
• A failed start, at restart or at startup, leaves no half-built PERMANENT, and the old browser keeps serving. A restart cancelled mid-start tears the new browser down.
• A request holding the old browser finishes before it is closed. Concurrent restarts leave exactly one browser running.
• test_init_permanent_under_the_lock_deadlocks documents the original bug and guards the assumption the fix rests on (that LOCK is a plain non-reentrant asyncio.Lock).
• test_restart_rebuilds_permanent_browser_from_startup_config: the restarted browser's pool signature equals the one get_default_browser_config() gives. It failed on the earlier version of this branch.

The offline suites (deploy/docker/tests without the live-server test_1–test_7 scripts, tests/unit, the pool tests) show no new failures against develop.

End to end, on an image built from develop @ 1f68e5b (docker build -t c4ai-dev .), run with CRAWL4AI_API_TOKEN set against a local test site, so CRAWL4AI_ALLOW_INTERNAL_URLS=true. The second and third runs mount this branch's crawler_pool.py and monitor_routes.py over /app, the second as it was before this update. /html is used because it builds its crawler from get_default_browser_config(). Signatures differ between containers, so compare them within a run:

# develop
[restart] /html (default config) HTTP 200; pool: [('permanent', '4162bae2', 1)]
[restart] restart_browser -> no answer after 30s
[restart] /html afterwards -> no answer after 20s (pool deadlocked)

# this branch before this update (restart built its own BrowserConfig)
[restart] /html (default config) HTTP 200; pool: [('permanent', 'c42915a8', 1)]
[restart] restart_browser -> HTTP 200 in 0.8s: {"success":true,"restarted":"permanent"}
[restart] pool after restart: [('permanent', 'd9ebf772', 0)]
[restart] /html again HTTP 200; pool: [('permanent', 'd9ebf772', 0), ('cold', 'c42915a8', 1)]

# this branch
[restart] /html (default config) HTTP 200; pool: [('permanent', '08ba0276', 1)]
[restart] restart_browser -> HTTP 200 in 0.9s: {"success":true,"restarted":"permanent"}
[restart] pool after restart: [('permanent', '08ba0276', 0)]
[restart] /html again HTTP 200; pool: [('permanent', '08ba0276', 1)]

After the start-first change (b399396), checked against a real browser on a Windows host (Edge through Playwright, not the Docker image), one restart under 4 parallel request loops. Before: 4 of 25 requests failed and a second default-config browser stayed in the cold pool. After: 0 of 34 failed and there was no duplicate. A 4s request in flight during the restart failed before and returns 200 now. The Docker-image runs above predate this change.

Checklist:

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• I have added/updated unit tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

[Copilot]

🟡 Changes recommended

restart_permanent() can leave PERMANENT unset indefinitely if close() hangs (or the task is cancelled mid-close), preventing the permanent browser from being recreated.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR fixes a deadlock in the Docker monitor endpoint’s permanent-browser restart flow by moving permanent browser lifecycle operations into the pool and ensuring browser close() operations are not awaited while holding the pool lock.

Changes:

• Add restart_permanent() to safely detach the permanent browser under LOCK, then close and recreate it outside the lock.
• Update restart_browser to call restart_permanent() and build BrowserConfig the same way startup does (via _browser_extra_args()).
• Add regression tests that exercise the real deploy/docker/crawler_pool.py with stubbed dependencies.

File summaries

File	Description
deploy/docker/crawler_pool.py	Adds restart_permanent() to avoid deadlocking on non-reentrant asyncio.Lock.
deploy/docker/monitor_routes.py	Uses restart_permanent() for permanent restart; moves browser closes outside LOCK.
tests/docker/test_pool_restart_permanent.py	Adds regression tests for the permanent restart deadlock and pool usability.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[talelboussetta]

Rebased onto current develop (after 0.9.4) and force-pushed; the description is updated. What changed:

• Since Build permanent browser with egress-hardened default config so its pool signature matches #2237 startup builds the permanent browser with get_default_browser_config(). The restart path now uses the same builder (e8cb918). Before, a restarted browser got a pool signature no request carries and sat idle while /html built cold-pool browsers; the end-to-end runs in the description show both.
• The bounded close from the follow-up commit is folded into the fix commit, and the restart_permanent() docstring is reworded per the review note (9d9dd7f).
• develop still deadlocks on POST /monitor/actions/restart_browser {"sig": "permanent"}, reproduced on a container built from 1f68e5b.

@ntohidi, would you have time to review? @SohamKukreti, the restart path now goes through the builder from #2237, so a look from you would be welcome too.

[SohamKukreti]

Thanks for this, @talelboussetta. The deadlock diagnosis is correct, and so is the move to get_default_browser_config(). That is the right builder after #2237. The server runs as server:app, so the lazy from server import ... is safe, and the pinning-proxy URL is set once in lifespan(), so the signature matches startup. Closing outside LOCK in kill_browser and in the hot/cold restart is also good.

I tested this live: an image built from this branch, real Chromium, a local test page, idle_ttl_sec: 20 so the janitor runs quickly. I compared develop and this branch. The deadlock fix works. But the live tests show two problems in the new restart path. develop cannot hit them today, because the route deadlocks before it reaches them.

Problem 1: a duplicate default-sig browser that the janitor does not reap while traffic continues

While the old browser closes and the new one starts, PERMANENT is None. A default-config request in that window creates a cold-pool browser with DEFAULT_CONFIG_SIG. Further requests in the same window reuse it, and after 3 hits it is promoted to hot. When the new permanent browser is up, both browsers share one LAST_USED[sig] key, so each permanent hit keeps the duplicate "fresh". The docstring says the janitor reaps it once idle, but it never looks idle while default traffic continues.

Live, one restart under 6 parallel /html loops:

right after restart: [('permanent','8dc9dd06'), ('hot','8dc9dd06'), ('cold','3c12e2f3')]
+1 min of load:      [('permanent','8dc9dd06'), ('hot','8dc9dd06')]   <- control cold browser reaped
+4 min of load:      [('permanent','8dc9dd06'), ('hot','8dc9dd06')]   <- duplicate still alive

That is one extra Chromium that stays as long as default traffic continues. When traffic stops, the janitor reaps it, and it also pops the permanent browser's LAST_USED/USAGE_COUNT, because they share the key (permanent hits went from 1947 to 0).

Problem 2: a failed start leaves no working browser, and then leaks browsers

init_permanent() assigns PERMANENT before await PERMANENT.start(), and the old browser is already closed at that point. Live, with the browser binary made non-executable before the restart:

• restart_browser → 500, and every /html → 500. There is no Chromium running at all.
• After the binary is back, arun() auto-starts the unstarted PERMANENT (if not self.ready: await self.start()). Under concurrent requests, start() ran 4 times on the same crawler (4 [INIT] lines in the log), so 3 browsers were launched that nothing tracks. They were still running after 90s idle and after a later clean restart (21 headless processes vs. 5–6 normal).

Other live results on this branch: 3 concurrent restarts OK, kill_browser on a cold browser OK. A request that is still running on the old browser fails when the restart closes it (an 8s /html → 500).

Two ways to fix them

Both ways fix both problems. They differ in what happens during a restart. @talelboussetta and @ntohidi, please pick the one you think fits better.

Option A: keep detach → close → re-create, and add guards

• Problem 1: stop default requests from creating a duplicate during the restart. There are 2 ways to do it:
  • Wait: keep DEFAULT_CONFIG_SIG set and add a "restarting" asyncio.Event. In get_crawler(), a default-sig request waits for it (outside LOCK, with a time limit) and then tries again.
  • Adopt: after the close, under LOCK, if COLD_POOL/HOT_POOL already has a browser with DEFAULT_CONFIG_SIG, make it the new PERMANENT instead of starting another one.
• Problem 2: in init_permanent(), assign PERMANENT only after start() succeeds (crawler = AsyncWebCrawler(...); await crawler.start(); PERMANENT = crawler). Then nothing can auto-start a half-built browser, so there are no orphans.
• Trade-offs:
  • No time with two browsers running (with "wait").
  • Default requests wait for the full close + start on each restart. That is about 1–2s normally, and longer if the close hangs.
  • If the new start fails, there is still an outage until Chromium works again, because the old browser is already closed. A retry of start() helps only with short failures.
  • A drain before the close (so running requests can finish) makes the restart window longer.
• Not tested live. This option comes from reading the code.

Option B: start new → swap under LOCK → drain and close old

async def restart_permanent(cfg: BrowserConfig):
    """Replace the permanent browser: start the new one, swap it in, close the old one."""
    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:
        # Failed or cancelled start: tear down what was launched and leave
        # the old permanent browser in service.
        if not swapped:
            with suppress(Exception):
                await new.close()

    if old is not None:
        try:
            # shield: a cancelled request must not abort the retire halfway
            # and orphan the old Chromium.
            await asyncio.shield(_retire_browser(old))
        except asyncio.TimeoutError:
            logger.warning("Timed out closing old permanent browser")
        except Exception as e:
            logger.warning(f"Error closing old permanent browser: {e}")


async def _retire_browser(crawler, drain_timeout=30, close_timeout=60):
    """Let requests already running on a detached browser finish, then close it."""
    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:
        # Graceful close failed or hung. Stop this browser's own Playwright
        # driver: on exit it SIGKILLs every browser process it launched.
        bm = getattr(crawler.crawler_strategy, "browser_manager", None)
        pw = getattr(bm, "playwright", None)
        if pw is not None:
            with suppress(BaseException):
                await asyncio.wait_for(pw.stop(), timeout=10)
        raise

• Problem 1: PERMANENT is never None, so no duplicate can be created.
• Problem 2: a failed start leaves the old browser untouched and serving. The new browser enters the pool only after start() succeeds.
• finally (instead of except Exception) also cleans up when the task is cancelled during start(), because CancelledError is not an Exception.
• Trade-offs:
  • Two browsers run together for a short time (a memory peak).
  • Default requests do not wait.
  • The restart request can take up to the drain timeout (30s) plus the close (up to 60s, then up to 10s to stop the driver) when requests are running.
• Tested live (mounted over /app/crawler_pool.py):

Scenario	This branch	Option B
Restart under load (6 parallel /html, 4 min)	duplicate hot browser, 1 failed request	no duplicate, 1901/1901 OK
Restart while an 8s /html is running	in-flight request → 500	in-flight request → 200 (restart takes 5.4s)
3 concurrent restarts	OK	OK
kill_browser on a cold browser	OK	OK
Restart with an unlaunchable browser	all /html 500, then 3 orphaned browsers	restart 500, /html keeps working, no orphans
Old browser's browser.close() raises or hangs	not tested (same leak: this branch also skips playwright.stop())	old Chromium and driver killed, no leftovers

Your 4 tests in tests/docker/test_pool_restart_permanent.py pass unchanged with Option B.

Applies to both options

• Drain before close. Without it, the restart closes the old browser under requests that are still running on it. Live with Option B, that was 5 failed requests in one restart under load without the drain, and 0 with it.
• Close fallback. BrowserManager.close() runs playwright.stop() only after browser.close() succeeds, so a failing close leaves Chromium and its driver running. The fallback in _retire_browser stops the old browser's own driver. Each browser has its own driver (browser_manager.py:838), so this does not affect the new permanent browser. Live, with the old browser's browser.close() forced to raise or to hang: without the fallback, the old Chromium (5 processes) and its driver stayed alive; with it, none were left, and /html worked on the new browser in all cases.
• Tests. Please add tests for: a default-config get_crawler() during a slow restart must not add to COLD_POOL; a failing start() must not leave an unstarted PERMANENT; a request holding the old browser must complete before it is closed; a failing close() must still stop the old browser's driver.
• init_permanent(). Assign PERMANENT only after start() succeeds, so startup cannot leave a half-built global either.

Smaller points

• Silent except Exception: pass on the close: please log a warning.
• Route test: test_restart_rebuilds_permanent_browser_from_startup_config replaces restart_permanent with a fake. That is fine for the config check, but no test shows that the route itself does not hold LOCK while it restarts. Optional.
• Not blocking, separate issue: the same unguarded browser.close() in BrowserManager.close() is used when the janitor and kill_browser close hot/cold browsers. The fallback above covers only the permanent restart. A try/finally around browser.close() in the library would cover all of them.

@ntohidi, it would be good to have your input on the option before @talelboussetta makes changes. The main question: on a restart, is a short time with two browsers (Option B) better or worse than making default requests wait and accepting an outage if the new start fails (Option A)? Also, are the timeouts good? Happy to re-review after we agree on the direction. The rest looks good to merge.

[talelboussetta]

Thanks @SohamKukreti. I went with Option B, pushed as b399396 and rebased on current develop.

• restart_permanent() starts the replacement outside LOCK and swaps it in under LOCK, so PERMANENT is never None. A failed or cancelled start tears the new browser down and the old one keeps serving.
• The old browser is retired after the swap: up to 30s for requests already running on it, the close bounded at 60s, and if the close fails or hangs its own Playwright driver is stopped (10s bound). The failure is logged. These are the timeouts from your sketch; @ntohidi, say if you want different ones.
• init_permanent() assigns PERMANENT only after start() succeeds.
• Tests for the four cases you listed, plus a restart cancelled mid-start, a failing start at startup, and concurrent restarts. 8 of the 12 pool tests fail on the previous head.

I checked the first problem against a real browser, one restart under 4 parallel request loops (Edge through Playwright on a Windows host, not your Docker setup). Before: 4 of 25 requests failed and a second default-config browser was left in the cold pool. After: 0 of 34 failed and there was no duplicate. A 4s request in flight during the restart failed before and returns 200 now. I did not reproduce your unlaunchable-binary case live; the unit tests cover that path. On Windows the docker suites show the same 72 failures as develop, with and without this change.

Left alone: a restart peaks at two browsers and does not check the memory limit. The unguarded browser.close() in BrowserManager.close(), used by the janitor and kill_browser, is the separate issue you noted. I can send a follow-up with a try/finally there if @ntohidi wants it.