diff --git a/src/crawlee/browsers/_playwright_browser.py b/src/crawlee/browsers/_playwright_browser.py index 8ce19bfd26..3d7e5424f6 100644 --- a/src/crawlee/browsers/_playwright_browser.py +++ b/src/crawlee/browsers/_playwright_browser.py @@ -3,6 +3,7 @@ import asyncio import shutil import tempfile +from datetime import timedelta from logging import getLogger from pathlib import Path from typing import TYPE_CHECKING, Any @@ -29,6 +30,12 @@ class PlaywrightPersistentBrowser(Browser): _TMP_DIR_PREFIX = 'apify-playwright-firefox-taac-' + _TMP_DIR_DELETE_ATTEMPTS = 50 + """The number of attempts to remove the temporary user data directory.""" + + _TMP_DIR_DELETE_INTERVAL = timedelta(milliseconds=100) + """The delay between the attempts to remove the temporary user data directory.""" + def __init__( self, browser_type: BrowserType, @@ -39,6 +46,8 @@ def __init__( self._browser_launch_options = browser_launch_options self._user_data_dir = user_data_dir self._temp_dir: Path | None = None + # Both `close` and the context's `close` event trigger the removal, and `close` must wait until it finishes. + self._temp_dir_lock = asyncio.Lock() self._context: BrowserContext | None = None self._is_connected = True @@ -76,10 +85,30 @@ async def new_context(self, **context_options: Any) -> BrowserContext: return self._context - async def _delete_temp_dir(self, _: BrowserContext | None) -> None: - if self._temp_dir and self._temp_dir.exists(): + async def _delete_temp_dir(self, _: BrowserContext | None = None) -> None: + """Remove the temporary user data directory, retrying until the browser releases its files. + + Browser helper processes can outlive the context: they keep files in the directory open, which makes the removal + fail on Windows, and they can write into the directory again after it is removed. Each attempt therefore waits + before checking that the directory is gone. + """ + async with self._temp_dir_lock: temp_dir = self._temp_dir - await asyncio.to_thread(shutil.rmtree, temp_dir, ignore_errors=True) + + if not temp_dir: + return + + # One close asks for the removal twice, so the second caller finds nothing left to do. + self._temp_dir = None + + for _attempt in range(self._TMP_DIR_DELETE_ATTEMPTS): + await asyncio.to_thread(shutil.rmtree, temp_dir, ignore_errors=True) + await asyncio.sleep(self._TMP_DIR_DELETE_INTERVAL.total_seconds()) + + if not temp_dir.exists(): + return + + logger.warning(f'Could not remove the temporary user data directory "{temp_dir}".') @override async def close(self, **kwargs: Any) -> None: @@ -88,8 +117,7 @@ async def close(self, **kwargs: Any) -> None: await self._context.close() self._context = None self._is_connected = False - await asyncio.sleep(0.1) - await self._delete_temp_dir(self._context) + await self._delete_temp_dir() @property @override diff --git a/tests/unit/browsers/test_playwright_browser.py b/tests/unit/browsers/test_playwright_browser.py index 120b886c59..e44337f3ce 100644 --- a/tests/unit/browsers/test_playwright_browser.py +++ b/tests/unit/browsers/test_playwright_browser.py @@ -1,7 +1,12 @@ from __future__ import annotations +import asyncio +import logging +import shutil +from datetime import timedelta from pathlib import Path -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, Any +from unittest.mock import AsyncMock, Mock import pytest from playwright.async_api import async_playwright @@ -42,3 +47,63 @@ async def test_delete_temp_folder_with_close_browser(playwright: Playwright) -> assert current_temp_dir.exists() await persist_browser.close() assert not current_temp_dir.exists() + + +async def test_delete_temp_folder_when_files_are_locked(monkeypatch: pytest.MonkeyPatch) -> None: + """The temp directory is removed even when the first delete attempts fail, as Windows locks the browser files.""" + monkeypatch.setattr(PlaywrightPersistentBrowser, '_TMP_DIR_DELETE_INTERVAL', timedelta(0)) + + real_rmtree = shutil.rmtree + locked_attempts = 3 + rmtree = Mock() + + def rmtree_locked_at_first(path: Any, **kwargs: Any) -> None: + """Model `rmtree(ignore_errors=True)` silently leaving the directory in place while a file is locked.""" + if rmtree.call_count > locked_attempts: + real_rmtree(path, **kwargs) + + rmtree.side_effect = rmtree_locked_at_first + monkeypatch.setattr(shutil, 'rmtree', rmtree) + + # A real browser on Windows can hold the files longer than the whole retry budget, so a fake context stands in. + # Like Playwright, it runs the `close` listener as a separate task. Letting that task start first makes `close` wait + # for the removal the listener is running. + context = Mock() + listener_tasks = list[asyncio.Task]() + + async def close_context() -> None: + listener = context.on.call_args.args[1] + listener_tasks.append(asyncio.create_task(listener(context))) + await asyncio.sleep(0) + + context.close = close_context + browser_type = Mock() + browser_type.launch_persistent_context = AsyncMock(return_value=context) + + persist_browser = PlaywrightPersistentBrowser(browser_type, user_data_dir=None, browser_launch_options={}) + await persist_browser.new_context() + assert isinstance(persist_browser._temp_dir, Path) + current_temp_dir = persist_browser._temp_dir + assert current_temp_dir.exists() + await persist_browser.close() + assert not current_temp_dir.exists() + await asyncio.gather(*listener_tasks) + # The context's `close` event and `close` itself both ask for the removal, but only one of them retries. + assert rmtree.call_count == locked_attempts + 1 + + +async def test_warn_when_temp_folder_cannot_be_deleted( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture +) -> None: + """A temp directory that stays locked for the whole retry budget is reported with a warning.""" + monkeypatch.setattr(PlaywrightPersistentBrowser, '_TMP_DIR_DELETE_ATTEMPTS', 2) + monkeypatch.setattr(PlaywrightPersistentBrowser, '_TMP_DIR_DELETE_INTERVAL', timedelta(0)) + monkeypatch.setattr(shutil, 'rmtree', Mock()) + + persist_browser = PlaywrightPersistentBrowser(Mock(), user_data_dir=None, browser_launch_options={}) + persist_browser._temp_dir = tmp_path + + with caplog.at_level(logging.WARNING, logger='crawlee.browsers._playwright_browser'): + await persist_browser._delete_temp_dir() + + assert 'Could not remove the temporary user data directory' in caplog.text