From 02b64bee818aaa6eef10c24665c4127c8260432b Mon Sep 17 00:00:00 2001 From: Vlada Dusek Date: Wed, 16 Sep 2026 14:00:23 +0200 Subject: [PATCH 1/4] fix(browsers): Retry the temp directory removal until the browser releases its files --- src/crawlee/browsers/_playwright_browser.py | 34 +++++++++++-- .../unit/browsers/test_playwright_browser.py | 51 ++++++++++++++++++- 2 files changed, 80 insertions(+), 5 deletions(-) diff --git a/src/crawlee/browsers/_playwright_browser.py b/src/crawlee/browsers/_playwright_browser.py index 8ce19bfd26..daf6fbbd96 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, so serialize the two. + self._temp_dir_lock = asyncio.Lock() self._context: BrowserContext | None = None self._is_connected = True @@ -77,9 +86,27 @@ 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(): - temp_dir = self._temp_dir - await asyncio.to_thread(shutil.rmtree, temp_dir, ignore_errors=True) + """Remove the temporary user data directory, retrying until the browser releases its files. + + The browser process can keep files in the directory open for a while after the context is closed, which makes + the removal fail on Windows. + """ + temp_dir = self._temp_dir + + if not temp_dir: + return + + async with self._temp_dir_lock: + for attempt in range(self._TMP_DIR_DELETE_ATTEMPTS): + if attempt: + await asyncio.sleep(self._TMP_DIR_DELETE_INTERVAL.total_seconds()) + + await asyncio.to_thread(shutil.rmtree, temp_dir, ignore_errors=True) + + 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,7 +115,6 @@ 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) @property diff --git a/tests/unit/browsers/test_playwright_browser.py b/tests/unit/browsers/test_playwright_browser.py index 120b886c59..f73567957d 100644 --- a/tests/unit/browsers/test_playwright_browser.py +++ b/tests/unit/browsers/test_playwright_browser.py @@ -1,7 +1,11 @@ from __future__ import annotations +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 Mock import pytest from playwright.async_api import async_playwright @@ -42,3 +46,48 @@ 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( + playwright: Playwright, monkeypatch: pytest.MonkeyPatch +) -> None: + """The temp directory is removed even when the first delete attempts fail, as Windows locks the browser files.""" + 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) + + persist_browser = PlaywrightPersistentBrowser( + playwright.chromium, user_data_dir=None, browser_launch_options={'headless': True} + ) + 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 rmtree.call_count > locked_attempts + assert not current_temp_dir.exists() + + +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(None) + + assert 'Could not remove the temporary user data directory' in caplog.text From bd849eab2e3f63aa39d631b3a45d6483a84cf5c1 Mon Sep 17 00:00:00 2001 From: Vlada Dusek Date: Wed, 16 Sep 2026 16:00:13 +0200 Subject: [PATCH 2/4] fix(browsers): Spend the temp directory retry budget once per close --- src/crawlee/browsers/_playwright_browser.py | 15 +++++++++------ tests/unit/browsers/test_playwright_browser.py | 7 +++++-- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/src/crawlee/browsers/_playwright_browser.py b/src/crawlee/browsers/_playwright_browser.py index daf6fbbd96..74377ec91f 100644 --- a/src/crawlee/browsers/_playwright_browser.py +++ b/src/crawlee/browsers/_playwright_browser.py @@ -85,18 +85,21 @@ async def new_context(self, **context_options: Any) -> BrowserContext: return self._context - async def _delete_temp_dir(self, _: BrowserContext | None) -> None: + async def _delete_temp_dir(self, _: BrowserContext | None = None) -> None: """Remove the temporary user data directory, retrying until the browser releases its files. The browser process can keep files in the directory open for a while after the context is closed, which makes the removal fail on Windows. """ - temp_dir = self._temp_dir + async with self._temp_dir_lock: + temp_dir = self._temp_dir - if not temp_dir: - return + 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 - async with self._temp_dir_lock: for attempt in range(self._TMP_DIR_DELETE_ATTEMPTS): if attempt: await asyncio.sleep(self._TMP_DIR_DELETE_INTERVAL.total_seconds()) @@ -115,7 +118,7 @@ async def close(self, **kwargs: Any) -> None: await self._context.close() self._context = None self._is_connected = False - 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 f73567957d..c84534106b 100644 --- a/tests/unit/browsers/test_playwright_browser.py +++ b/tests/unit/browsers/test_playwright_browser.py @@ -52,6 +52,8 @@ async def test_delete_temp_folder_when_files_are_locked( playwright: Playwright, 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() @@ -72,7 +74,8 @@ def rmtree_locked_at_first(path: Any, **kwargs: Any) -> None: current_temp_dir = persist_browser._temp_dir assert current_temp_dir.exists() await persist_browser.close() - assert rmtree.call_count > locked_attempts + # 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 assert not current_temp_dir.exists() @@ -88,6 +91,6 @@ async def test_warn_when_temp_folder_cannot_be_deleted( persist_browser._temp_dir = tmp_path with caplog.at_level(logging.WARNING, logger='crawlee.browsers._playwright_browser'): - await persist_browser._delete_temp_dir(None) + await persist_browser._delete_temp_dir() assert 'Could not remove the temporary user data directory' in caplog.text From 91ee08974142afef7dae247a1c757c131f00dbf9 Mon Sep 17 00:00:00 2001 From: Vlada Dusek Date: Thu, 24 Sep 2026 13:42:03 +0200 Subject: [PATCH 3/4] fix: Recheck the browser temp directory after each removal attempt --- src/crawlee/browsers/_playwright_browser.py | 11 ++++---- .../unit/browsers/test_playwright_browser.py | 25 +++++++++++++------ 2 files changed, 23 insertions(+), 13 deletions(-) diff --git a/src/crawlee/browsers/_playwright_browser.py b/src/crawlee/browsers/_playwright_browser.py index 74377ec91f..4c74450a38 100644 --- a/src/crawlee/browsers/_playwright_browser.py +++ b/src/crawlee/browsers/_playwright_browser.py @@ -88,8 +88,9 @@ async def new_context(self, **context_options: Any) -> BrowserContext: async def _delete_temp_dir(self, _: BrowserContext | None = None) -> None: """Remove the temporary user data directory, retrying until the browser releases its files. - The browser process can keep files in the directory open for a while after the context is closed, which makes - the removal fail on Windows. + 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 @@ -100,11 +101,9 @@ async def _delete_temp_dir(self, _: BrowserContext | None = None) -> None: # 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): - if attempt: - await asyncio.sleep(self._TMP_DIR_DELETE_INTERVAL.total_seconds()) - + 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 diff --git a/tests/unit/browsers/test_playwright_browser.py b/tests/unit/browsers/test_playwright_browser.py index c84534106b..556470d225 100644 --- a/tests/unit/browsers/test_playwright_browser.py +++ b/tests/unit/browsers/test_playwright_browser.py @@ -1,11 +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, Any -from unittest.mock import Mock +from unittest.mock import AsyncMock, Mock import pytest from playwright.async_api import async_playwright @@ -48,9 +49,7 @@ async def test_delete_temp_folder_with_close_browser(playwright: Playwright) -> assert not current_temp_dir.exists() -async def test_delete_temp_folder_when_files_are_locked( - playwright: Playwright, monkeypatch: pytest.MonkeyPatch -) -> None: +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)) @@ -66,14 +65,26 @@ def rmtree_locked_at_first(path: Any, **kwargs: Any) -> None: rmtree.side_effect = rmtree_locked_at_first monkeypatch.setattr(shutil, 'rmtree', rmtree) - persist_browser = PlaywrightPersistentBrowser( - playwright.chromium, user_data_dir=None, browser_launch_options={'headless': True} - ) + # 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. + 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))) + + 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() + 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 assert not current_temp_dir.exists() From b0f8dad098a3f15fff8b820653aaad82cc94d381 Mon Sep 17 00:00:00 2001 From: Vlada Dusek Date: Thu, 24 Sep 2026 14:55:15 +0200 Subject: [PATCH 4/4] test(browsers): Check that close waits for the temp directory removal --- src/crawlee/browsers/_playwright_browser.py | 2 +- tests/unit/browsers/test_playwright_browser.py | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/crawlee/browsers/_playwright_browser.py b/src/crawlee/browsers/_playwright_browser.py index 4c74450a38..3d7e5424f6 100644 --- a/src/crawlee/browsers/_playwright_browser.py +++ b/src/crawlee/browsers/_playwright_browser.py @@ -46,7 +46,7 @@ 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, so serialize the two. + # 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 diff --git a/tests/unit/browsers/test_playwright_browser.py b/tests/unit/browsers/test_playwright_browser.py index 556470d225..e44337f3ce 100644 --- a/tests/unit/browsers/test_playwright_browser.py +++ b/tests/unit/browsers/test_playwright_browser.py @@ -66,13 +66,15 @@ def rmtree_locked_at_first(path: Any, **kwargs: Any) -> None: 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. + # 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() @@ -84,10 +86,10 @@ async def close_context() -> None: 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 - assert not current_temp_dir.exists() async def test_warn_when_temp_folder_cannot_be_deleted(