Docker: Env var SE_VIDEO_SESSION_SUBFOLDER to standardize recording in dynamic grid#3156
Open
VietND96 wants to merge 1 commit into
Open
Docker: Env var SE_VIDEO_SESSION_SUBFOLDER to standardize recording in dynamic grid#3156VietND96 wants to merge 1 commit into
VietND96 wants to merge 1 commit into
Conversation
…n dynamic grid Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
Contributor
Code Review by Qodo
1. Subfolder mkdir can crash
|
Contributor
PR Summary by QodoVideo: add SE_VIDEO_SESSION_SUBFOLDER to store recordings under per-session dirs WalkthroughsDescription• Add SE_VIDEO_SESSION_SUBFOLDER to control per-session subdirectory recording layout.
• Create VIDEO_FOLDER/{session_id}/ before starting recordings in both recorder modes.
• Log the configured subfolder mode and created directories for easier troubleshooting.
Diagramgraph TD
E["SE_VIDEO_SESSION_SUBFOLDER"] --> S["video.sh (polling)"] --> V[("/videos volume")]
E --> P["video_service.py (event-driven)"] --> V
S -->|"true: mkdir VIDEO_FOLDER/session_id"| V
P -->|"true: mkdir VIDEO_FOLDER/session_id"| V
High-Level AssessmentThe following are alternative approaches to this PR: 1. Path template env var (e.g., SE_VIDEO_OUTPUT_PATH_TEMPLATE)
2. Rely on unique filenames only (session id suffix)
Recommendation: The boolean SE_VIDEO_SESSION_SUBFOLDER toggle is a good, low-risk default-off improvement that standardizes layout across both recording modes without breaking existing users. A path-template approach could be considered later if more layout variants are needed, but would add complexity that doesn’t seem warranted for this use case. File ChangesEnhancement (2)
Other (1)
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+770
to
+774
| if record_video and self.session_subfolder: | ||
| session_subdir = Path(self.video_folder) / session_id | ||
| session_subdir.mkdir(parents=True, exist_ok=True) | ||
| video_filename = f"{session_id}/{video_filename}" | ||
| logger.info(f"Created session subfolder: {session_subdir}") |
Contributor
There was a problem hiding this comment.
1. Subfolder mkdir can crash 🐞 Bug ☼ Reliability
In event-driven mode, handle_session_created() creates the per-session directory with Path.mkdir()
without handling OSError, so a non-writable VIDEO_FOLDER (or an existing file at
VIDEO_FOLDER/{session_id}) can raise and abort event processing. Because subscribe_events() awaits
handlers without a general exception guard, this mkdir failure can stop the recorder from handling
subsequent sessions.
Agent Prompt
## Issue description
When `SE_VIDEO_SESSION_SUBFOLDER=true`, `handle_session_created()` creates `VIDEO_FOLDER/{session_id}` via `Path.mkdir(...)` without catching filesystem errors. Any `OSError` (permission denied, read-only mount, or `FileExistsError` when a file exists at that path) can bubble up and disrupt the event subscriber.
## Issue Context
This is newly introduced behavior in the session-subfolder block. Separately, the event loop in `subscribe_events()` does not wrap handler execution in a broad `try/except`, so exceptions from handlers are not contained.
## Fix Focus Areas
- Video/video_service.py[755-800]
- Video/video_service.py[947-988]
## What to change
1. Wrap the subfolder creation in `try/except OSError` and log a clear error.
2. Decide on a safe fallback behavior when the subfolder cannot be created (e.g., disable recording for that session, or fall back to `VIDEO_FOLDER` root).
3. Add a defensive `try/except Exception` around `await handler(data)` in `subscribe_events()` so a single handler failure doesn’t stop the subscriber loop (log and continue).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Thanks for contributing to the Docker-Selenium project!
A PR well described will help maintainers to quickly review and merge it
Before submitting your PR, please check our contributing guidelines, applied for this repository.
Avoid large PRs, help reviewers by making them as simple and short as possible.
Description
Motivation and Context
Types of changes
Checklist