Skip to content

Clean up leaked Playwright driver when browser launch fails in __aenter__ - #2160

Open
SohamKukreti wants to merge 1 commit into
developfrom
fix/browser-startup-driver-leak
Open

Clean up leaked Playwright driver when browser launch fails in __aenter__#2160
SohamKukreti wants to merge 1 commit into
developfrom
fix/browser-startup-driver-leak

Conversation

@SohamKukreti

Copy link
Copy Markdown
Collaborator

Summary

Fixes #2155

When AsyncWebCrawler.__aenter__() fails partway through browser startup, the already-started Playwright driver process (node .../cli.js run-driver) was never cleaned up. Python does not call __aexit__ for a failed __aenter__, so nothing stopped the driver — every failed startup attempt leaked one live child process. Retry loops made the process count grow linearly.

Root cause

BrowserManager.start() initializes Playwright first and launches the browser later, with no error handling in between:

self.browser = await self.playwright.chromium.launch(**browser_args)  # can raise

Any failure after the first line (missing browser executable, broken WebKit deps, unreachable CDP endpoint, task cancellation) propagated out of __aenter__ and left the driver running.

Fix

Wrap the startup body of BrowserManager.start() in try/except BaseException:

  • on any failure, roll back partial state via close() before re-raising
  • for external-CDP configs, close() intentionally returns early without stopping the driver (cdp_cleanup_on_close=False default), so the rollback additionally stops the driver explicitly — we own it even when we don't own the browser
  • cleanup errors are suppressed so the original launch error always propagates, unchanged (bare raise)
  • BaseException (not Exception) so cancellation and Ctrl-C during startup also clean up

Note for reviewers: the diff looks large because the existing start() body moved one indent level into the try block. Review with git diff -w (or -b) — the real change is 15 added lines, zero modified lines.

Reproduction

Verified with the issue's reproducer and with a natural failure (WebKit on Fedora, missing libicudata.so.66), in a plain retry loop:

attempt leaked drivers (before) leaked drivers (after)
1 1 0
2 2 0
3 3 0

List of files changed and why

  • crawl4ai/browser_manager.py - Wrap the body of BrowserManager.start() in try/except BaseException so a failed browser launch rolls back partial startup (stops the leaked Playwright driver process) before re-raising the original error. The diff is mostly indentation — review with git diff -w; the real change is 15 added lines.
  • tests/regression/test_reg_browser.py - Add regression test test_failed_browser_launch_leaves_no_driver proving a failed launch propagates the original error, resets manager state, and leaves no driver child process.

How Has This Been Tested?

  • Reproduced the leak first (issue reproducer + a natural failure: WebKit launch on Fedora): drivers grew 1 → 2 → 3 across retries. After the fix: 0 → 0 → 0, with the user-visible error unchanged.
  • New regression test verified in both directions: fails on current develop without the fix, passes with it.
  • Adversarial matrix (17 tests) covering every start() branch: chromium/firefox/persistent-context/CDP launch failures, cancellation mid-start, cleanup that itself raises (original error not masked), retry-then-succeed on the same crawler instance, plus success paths (normal crawl, multi-arun, arun_many, explicit start()/close(), sequential crawlers with driver-count baseline).
  • Full regression suite: pytest tests/regression/ -m "not network" — 293 passed, no new failures.
  • Verified playwright.stop() does not affect an external CDP browser (stays alive, answers CDP, accepts new clients) — it only ends the driver process we own.

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

…__aenter__

Roll back partial startup in BrowserManager.start() so a failed browser launch stops the already-started driver process before re-raising. Fixes #2155
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant