perf: use aiofiles for async browser artifact reads#4550
perf: use aiofiles for async browser artifact reads#4550CodeDotJS wants to merge 1 commit intoSkyvern-AI:mainfrom
aiofiles for async browser artifact reads#4550Conversation
WalkthroughThe pull request converts synchronous file I/O operations to asynchronous ones using the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@skyvern/webeye/real_browser_manager.py`:
- Around line 263-268: The async file read of each video artifact can raise
OSError and currently aborts the loop; update the loop that iterates
browser_state.browser_artifacts.video_artifacts to wrap the aiofiles.open(...)
and await f.read() calls in a try/except (catch OSError and Exception), log the
failure (e.g., logger.exception or logger.error with context including
video_artifact.video_path and index i) and continue the loop so remaining
artifacts are processed; keep the existing os.path.exists(path) check and only
assign video_data on successful read.
♻️ Duplicate comments (1)
skyvern/webeye/real_browser_manager.py (1)
279-282: Apply the same async error handling here.Same
aiofiles.open/readerror-handling gap as above; please mirror the try/except logging pattern. As per coding guidelines, always handle exceptions in async code.
| for i, video_artifact in enumerate(browser_state.browser_artifacts.video_artifacts): | ||
| path = video_artifact.video_path | ||
| if path and os.path.exists(path=path): | ||
| with open(path, "rb") as f: | ||
| browser_state.browser_artifacts.video_artifacts[i].video_data = f.read() | ||
| async with aiofiles.open(path, "rb") as f: | ||
| browser_state.browser_artifacts.video_artifacts[i].video_data = await f.read() | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the file structure and the specific lines mentioned
cat -n skyvern/webeye/real_browser_manager.py | sed -n '255,280p'Repository: Skyvern-AI/skyvern
Length of output: 1214
🏁 Script executed:
# Check the function definition and broader context
cat -n skyvern/webeye/real_browser_manager.py | sed -n '240,290p'Repository: Skyvern-AI/skyvern
Length of output: 2192
🏁 Script executed:
# Check for LOG usage in the file
rg "LOG\." skyvern/webeye/real_browser_manager.py | head -20Repository: Skyvern-AI/skyvern
Length of output: 1099
🏁 Script executed:
# Check imports at the top of the file
head -50 skyvern/webeye/real_browser_manager.pyRepository: Skyvern-AI/skyvern
Length of output: 1779
🏁 Script executed:
# Check if there are similar patterns in get_har_data or other async file operations
rg "aiofiles\.open" skyvern/webeye/real_browser_manager.py -A 2Repository: Skyvern-AI/skyvern
Length of output: 345
🏁 Script executed:
# Check if there's any broader exception handling in the get_video_artifacts function
sed -n '247,270p' skyvern/webeye/real_browser_manager.pyRepository: Skyvern-AI/skyvern
Length of output: 986
Add exception handling for async file read operations.
Lines 266-267 lack error handling for aiofiles.open() and await f.read(), which can raise OSError. If either fails, the entire artifact retrieval aborts. Per coding guidelines, always handle exceptions in async code. Wrap with try/except and log failures to allow the loop to continue processing remaining artifacts.
Suggested fix
if path and os.path.exists(path=path):
- async with aiofiles.open(path, "rb") as f:
- browser_state.browser_artifacts.video_artifacts[i].video_data = await f.read()
+ try:
+ async with aiofiles.open(path, "rb") as f:
+ browser_state.browser_artifacts.video_artifacts[i].video_data = await f.read()
+ except OSError:
+ LOG.warning(
+ "Failed to read video artifact",
+ task_id=task_id,
+ workflow_id=workflow_id,
+ workflow_run_id=workflow_run_id,
+ exc_info=True,
+ )🤖 Prompt for AI Agents
In `@skyvern/webeye/real_browser_manager.py` around lines 263 - 268, The async
file read of each video artifact can raise OSError and currently aborts the
loop; update the loop that iterates
browser_state.browser_artifacts.video_artifacts to wrap the aiofiles.open(...)
and await f.read() calls in a try/except (catch OSError and Exception), log the
failure (e.g., logger.exception or logger.error with context including
video_artifact.video_path and index i) and continue the loop so remaining
artifacts are processed; keep the existing os.path.exists(path) check and only
assign video_data on successful read.
2 participants
⚡ This PR improves performance by replacing synchronous file I/O operations with asynchronous ones using the
aiofileslibrary for reading browser artifacts (video files and HAR data). The change prevents blocking the event loop during file operations in async functions, leading to better concurrency and responsiveness.🔍 Detailed Analysis
Key Changes
open()calls withaiofiles.open()for async file readingget_video_artifacts()method to use async file operations when reading video dataget_har_data()method to use async file operations when reading HAR filesaiofilesimport to support async file operationsTechnical Implementation
sequenceDiagram participant Client participant RealBrowserManager participant FileSystem Client->>RealBrowserManager: get_video_artifacts() Note over RealBrowserManager: Before: Blocking file read RealBrowserManager->>FileSystem: open(path, "rb") FileSystem-->>RealBrowserManager: file.read() [BLOCKS] Note over RealBrowserManager: After: Non-blocking async read RealBrowserManager->>FileSystem: aiofiles.open(path, "rb") FileSystem-->>RealBrowserManager: await file.read() [NON-BLOCKING] RealBrowserManager-->>Client: video_artifactsImpact
Created with Palmier
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.