Skip to content

Commit 3485535

Browse files
authored
fix(browsers): do not close a browser as inactive while a page opening is in flight (#2156)
The `windows-latest / 3.10` unit-test job [failed](https://github.com/apify/crawlee-python/actions/runs/31604546268/job/94140034785) with `TargetClosedError: BrowserContext.new_page: Target page, context or browser has been closed` in `test_new_page_with_each_plugin`. It is not a test flake: the `BrowserPool` inactive-browser reapers judge a browser only by its finished pages (`idle_time` runs from controller construction and `pages` is populated only once `new_page` returns), so on a slow runner they gracefully close a freshly launched browser whose first `new_page()` is still creating its context. Any user on a loaded machine can hit the same error (related: #1660). The fix exposes the already-tracked in-flight openings as `BrowserController.is_opening_pages` and guards both reapers with it, so such a browser is neither marked inactive nor closed. A new regression test reproduces the race deterministically by gating the first `context.new_page()` call and running one reaper cycle while the opening is pending — it fails with the exact CI error before the fix and passes after. *✍️ Drafted by Claude Code*
1 parent 7a03a99 commit 3485535

5 files changed

Lines changed: 65 additions & 3 deletions

File tree

src/crawlee/browsers/_browser_controller.py

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,11 @@ def idle_time(self) -> timedelta:
5454
def has_free_capacity(self) -> bool:
5555
"""Return if the browser has free capacity to open a new page."""
5656

57+
@property
58+
def is_opening_pages(self) -> bool:
59+
"""Return if the browser has any `new_page` calls currently in flight."""
60+
return False
61+
5762
@property
5863
@abstractmethod
5964
def is_browser_connected(self) -> bool:

src/crawlee/browsers/_browser_pool.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -381,14 +381,14 @@ async def _launch_new_browser(self, page_id: str, plugin: BrowserPlugin) -> Brow
381381
def _identify_inactive_browsers(self) -> None:
382382
"""Identify inactive browsers and move them to the inactive list if their idle time exceeds the threshold."""
383383
for browser in list(self._active_browsers):
384-
if browser.idle_time >= self._browser_inactive_threshold:
384+
if browser.idle_time >= self._browser_inactive_threshold and not browser.is_opening_pages:
385385
self._active_browsers.remove(browser)
386386
self._inactive_browsers.append(browser)
387387

388388
async def _close_inactive_browsers(self) -> None:
389389
"""Close the browsers that have no active pages and have been idle for a certain period."""
390390
for browser in list(self._inactive_browsers):
391-
if not browser.pages:
391+
if not browser.pages and not browser.is_opening_pages:
392392
await browser.close()
393393
self._inactive_browsers.remove(browser)
394394

src/crawlee/browsers/_playwright_browser_controller.py

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,11 @@ def idle_time(self) -> timedelta:
139139
def has_free_capacity(self) -> bool:
140140
return (self.pages_count + self._opening_pages_count) < self._max_open_pages_per_browser
141141

142+
@property
143+
@override
144+
def is_opening_pages(self) -> bool:
145+
return self._opening_pages_count > 0
146+
142147
@property
143148
@override
144149
def is_browser_connected(self) -> bool:

src/crawlee/browsers/_stagehand_browser_controller.py

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,11 @@ def idle_time(self) -> timedelta:
105105
def has_free_capacity(self) -> bool:
106106
return (self.pages_count + self._opening_pages_count) < self._max_open_pages_per_browser
107107

108+
@property
109+
@override
110+
def is_opening_pages(self) -> bool:
111+
return self._opening_pages_count > 0
112+
108113
@property
109114
@override
110115
def is_browser_connected(self) -> bool:

tests/unit/browsers/test_browser_pool.py

Lines changed: 48 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,21 @@
11
from __future__ import annotations
22

3+
import asyncio
34
from datetime import timedelta
45
from typing import TYPE_CHECKING
56
from unittest.mock import AsyncMock
67

78
import pytest
89

9-
from crawlee.browsers import BrowserPool, PlaywrightBrowserPlugin
10+
from crawlee.browsers import BrowserPool, PlaywrightBrowserController, PlaywrightBrowserPlugin
1011
from crawlee.browsers._browser_controller import BrowserController
1112
from crawlee.browsers._types import CrawleePage
1213

1314
if TYPE_CHECKING:
1415
from collections.abc import Mapping
1516
from typing import Any
1617

18+
from playwright.async_api import BrowserContext, Page
1719
from yarl import URL
1820

1921
from crawlee.browsers._browser_plugin import BrowserPlugin
@@ -159,6 +161,51 @@ async def test_resource_management(server_url: URL) -> None:
159161
assert page.page.is_closed()
160162

161163

164+
async def test_reaper_does_not_close_browser_with_page_opening_in_flight(monkeypatch: pytest.MonkeyPatch) -> None:
165+
"""The inactive-browser reaper leaves a browser alone while its `new_page` call is still in flight."""
166+
opening_in_flight = asyncio.Event()
167+
resume_opening = asyncio.Event()
168+
original_create_context = PlaywrightBrowserController._create_browser_context
169+
170+
async def create_context_with_gated_first_page(
171+
self: PlaywrightBrowserController, *args: Any, **kwargs: Any
172+
) -> BrowserContext:
173+
context = await original_create_context(self, *args, **kwargs)
174+
original_new_page = context.new_page
175+
176+
async def gated_new_page(*new_page_args: Any, **new_page_kwargs: Any) -> Page:
177+
opening_in_flight.set()
178+
await resume_opening.wait()
179+
return await original_new_page(*new_page_args, **new_page_kwargs)
180+
181+
monkeypatch.setattr(context, 'new_page', gated_new_page)
182+
return context
183+
184+
monkeypatch.setattr(PlaywrightBrowserController, '_create_browser_context', create_context_with_gated_first_page)
185+
186+
# Long reaper intervals so that only the manual calls below drive the reaping; a zero inactivity
187+
# threshold makes the freshly launched browser eligible for it right away.
188+
async with BrowserPool(
189+
browser_inactive_threshold=timedelta(seconds=0),
190+
identify_inactive_browsers_interval=timedelta(hours=1),
191+
close_inactive_browsers_interval=timedelta(hours=1),
192+
) as browser_pool:
193+
new_page_task = asyncio.create_task(browser_pool.new_page())
194+
await asyncio.wait_for(opening_in_flight.wait(), timeout=60)
195+
196+
# Run one reaper cycle, exactly as the recurring tasks would, while the page opening is pending.
197+
browser_pool._identify_inactive_browsers()
198+
await browser_pool._close_inactive_browsers()
199+
200+
resume_opening.set()
201+
page = await new_page_task
202+
203+
assert not page.page.is_closed()
204+
assert browser_pool.total_pages_count == 1
205+
206+
await page.page.close()
207+
208+
162209
async def test_methods_raise_error_when_not_active() -> None:
163210
plugin = PlaywrightBrowserPlugin()
164211
browser_pool = BrowserPool([plugin])

0 commit comments

Comments
 (0)