fix(browsers): Retry the temp directory removal until the browser releases its files

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

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