mirror of
https://github.com/browser-use/browser-use
synced 2026-04-22 17:45:09 +02:00
Address PR review feedback for record start/stop
- `on_BrowserConnectedEvent` now catches `RuntimeError` from `start_recording()` so sessions with `record_video_dir` configured but missing `[video]` extras (or a viewport that can't be sized) keep starting — prior graceful-degradation behavior is restored. - Lazy `RecordingWatchdog` in the CLI handler now calls `attach_to_session()`, so `AgentFocusChangedEvent` / `BrowserStopEvent` handlers are wired correctly if the session dispatches them. - Daemon shutdown finalizes any in-progress recording before tearing the browser down, preventing truncated MP4s on `close`, idle timeout, or signal-driven exit. - Added regression test that monkeypatches `start_recording` to raise and asserts `on_BrowserConnectedEvent` swallows it without breaking startup.
This commit is contained in:
@@ -38,7 +38,12 @@ class RecordingWatchdog(BaseWatchdog):
|
||||
|
||||
video_format = getattr(profile, 'record_video_format', 'mp4').strip('.')
|
||||
output_path = Path(profile.record_video_dir) / f'{uuid7str()}.{video_format}'
|
||||
await self.start_recording(output_path, size=profile.record_video_size, framerate=profile.record_video_framerate)
|
||||
try:
|
||||
await self.start_recording(output_path, size=profile.record_video_size, framerate=profile.record_video_framerate)
|
||||
except RuntimeError as e:
|
||||
# Preserve prior graceful degradation: a session configured with record_video_dir
|
||||
# should not fail startup when video deps are missing or viewport detection fails.
|
||||
self.logger.warning(f'Skipping video recording: {e}')
|
||||
|
||||
async def start_recording(
|
||||
self,
|
||||
|
||||
@@ -779,6 +779,7 @@ async def handle(action: str, session: SessionInfo, params: dict[str, Any]) -> A
|
||||
|
||||
RecordingWatchdog.model_rebuild()
|
||||
watchdog = RecordingWatchdog(event_bus=bs.event_bus, browser_session=bs)
|
||||
watchdog.attach_to_session()
|
||||
bs._recording_watchdog = watchdog
|
||||
|
||||
record_command = params.get('record_command')
|
||||
|
||||
@@ -448,14 +448,26 @@ class Daemon:
|
||||
self._server.close()
|
||||
|
||||
if self._session:
|
||||
# Finalize any in-progress video recording before tearing down the browser,
|
||||
# otherwise the MP4 is truncated since the ffmpeg writer is never closed.
|
||||
bs = self._session.browser_session
|
||||
watchdog = getattr(bs, '_recording_watchdog', None)
|
||||
if watchdog is not None and getattr(watchdog, 'is_recording', False):
|
||||
try:
|
||||
saved = await asyncio.wait_for(watchdog.stop_recording(), timeout=5.0)
|
||||
if saved:
|
||||
logger.info(f'Finalized in-progress recording: {saved}')
|
||||
except Exception as e:
|
||||
logger.warning(f'Error finalizing recording during shutdown: {e}')
|
||||
|
||||
try:
|
||||
# Only kill the browser if the daemon launched it.
|
||||
# For external connections (--connect, --cdp-url, cloud), just disconnect.
|
||||
# Timeout ensures daemon exits even if CDP calls hang on a dead connection
|
||||
if self.cdp_url or self.use_cloud:
|
||||
await asyncio.wait_for(self._session.browser_session.stop(), timeout=10.0)
|
||||
await asyncio.wait_for(bs.stop(), timeout=10.0)
|
||||
else:
|
||||
await asyncio.wait_for(self._session.browser_session.kill(), timeout=10.0)
|
||||
await asyncio.wait_for(bs.kill(), timeout=10.0)
|
||||
except TimeoutError:
|
||||
logger.warning('Browser cleanup timed out after 10s, forcing exit')
|
||||
except Exception as e:
|
||||
|
||||
@@ -109,6 +109,28 @@ async def test_stop_without_start_returns_none(browser_session: BrowserSession):
|
||||
assert await watchdog.stop_recording() is None
|
||||
|
||||
|
||||
async def test_on_browser_connected_degrades_gracefully_when_recording_fails(
|
||||
browser_session: BrowserSession, tmp_path: Path, monkeypatch
|
||||
):
|
||||
"""If start_recording() raises (e.g. missing [video] deps), profile-driven recording
|
||||
must degrade to a warning instead of breaking BrowserSession startup (see PR #4710 review)."""
|
||||
from browser_use.browser.events import BrowserConnectedEvent
|
||||
from browser_use.browser.watchdogs import recording_watchdog as rw_mod
|
||||
|
||||
watchdog = browser_session._recording_watchdog
|
||||
assert watchdog is not None
|
||||
|
||||
async def fake_start_recording(self: Any, *_args: Any, **_kwargs: Any) -> Path:
|
||||
raise RuntimeError('simulated missing video deps')
|
||||
|
||||
monkeypatch.setattr(rw_mod.RecordingWatchdog, 'start_recording', fake_start_recording)
|
||||
browser_session.browser_profile.record_video_dir = tmp_path
|
||||
|
||||
# Must not raise — watchdog should catch the RuntimeError and just log a warning.
|
||||
await watchdog.on_BrowserConnectedEvent(BrowserConnectedEvent(cdp_url=browser_session.cdp_url or ''))
|
||||
assert not watchdog.is_recording
|
||||
|
||||
|
||||
async def test_profile_record_video_dir_still_works(page_url: str, tmp_path: Path):
|
||||
"""The existing event-driven flow (profile.record_video_dir) must keep working."""
|
||||
session = BrowserSession(
|
||||
|
||||
Reference in New Issue
Block a user