Docker: Video recording starts via session capability se:recordVideo#3131
Open
Docker: Video recording starts via session capability se:recordVideo#3131
Conversation
Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
Contributor
Review Summary by QodoEnable video recording control via SE_RECORD_VIDEO environment variable
WalkthroughsDescription• Add SE_RECORD_VIDEO environment variable support for default video recording behavior • Implement fallback logic when session capability is absent or undefined • Refactor supervisord configuration to always start video services with runtime control • Improve signal handling in shell-based video recorder to prevent blocking • Add idle mode when video recording is disabled and per-session mode is inactive Diagramflowchart LR
A["Session Capability<br/>se:recordVideo"] -->|Present| B["Use Capability Value"]
C["SE_RECORD_VIDEO<br/>Environment Variable"] -->|Absent Capability| B
B --> D["Determine Recording<br/>Status"]
D -->|Enabled| E["Start Video Recording"]
D -->|Disabled| F["Idle Mode"]
G["Supervisord Config"] -->|Always Start| H["Video Services"]
H --> I["Runtime Control<br/>via Python Logic"]
File Changes1. Video/video_nodeQuery.py
|
Contributor
Code Review by Qodo
1. KeyboardInterrupt swallowed in idle
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+92
to
+97
| try: | ||
| while True: | ||
| time.sleep(60) | ||
| except KeyboardInterrupt: | ||
| pass | ||
| return |
Contributor
There was a problem hiding this comment.
1. keyboardinterrupt swallowed in idle 📘 Rule violation ☼ Reliability
In the new idle path, KeyboardInterrupt is caught and ignored, causing SIGINT to exit cleanly instead of preserving default interrupt termination semantics. This violates the requirement that pre-async SIGINT handling must not be swallowed, and can misclassify external shutdown as a normal exit.
Agent Prompt
## Issue description
`Video/video_recorder.py` introduces an idle loop that catches `KeyboardInterrupt` and ignores it, which changes SIGINT behavior from default interrupt termination to a clean/normal exit.
## Issue Context
Compliance requires that pre-async SIGINT termination semantics are preserved (i.e., SIGINT should not be swallowed/misclassified as a normal exit).
## Fix Focus Areas
- Video/video_recorder.py[92-97]
## Suggested direction
- Remove the `try/except KeyboardInterrupt` block entirely, or
- Re-raise `KeyboardInterrupt` after any necessary logging/cleanup, or
- Explicitly exit with an interrupt-like code (e.g., `raise` or `sys.exit(130)`) so termination semantics are preserved.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+87
to
+92
| record_video = os.environ.get("SE_RECORD_VIDEO", "true").lower() == "true" | ||
| per_session_mode = os.environ.get("SE_VIDEO_FILE_NAME", "") == "auto" | ||
|
|
||
| if not record_video and not per_session_mode: | ||
| print("[video.recorder] - SE_RECORD_VIDEO is disabled and SE_VIDEO_FILE_NAME is not 'auto', idling.") | ||
| try: |
Contributor
There was a problem hiding this comment.
2. File_name bypasses recorder gating 🐞 Bug ≡ Correctness
In shell mode, video_recorder._run_shell_recorder() decides whether to idle using only SE_VIDEO_FILE_NAME, but video.sh uses FILE_NAME to override the effective filename/mode. This mismatch can make recording start in fixed-file mode even with SE_RECORD_VIDEO=false (bypassing capability-based gating) or make the recorder idle when video.sh would run per-session mode.
Agent Prompt
## Issue description
`Video/video_recorder.py` decides whether to idle/start `video.sh` based on `SE_VIDEO_FILE_NAME` only. But `video.sh` uses `FILE_NAME` as a higher-priority override for the effective file name, which also changes whether it runs fixed-file recording vs per-session recording. This can cause recording to run when `SE_RECORD_VIDEO=false` (if `FILE_NAME` is fixed) or cause the recorder to idle when `video.sh` would run in per-session mode (if `FILE_NAME=auto`).
## Issue Context
- `video.sh` picks `VIDEO_FILE_NAME=${FILE_NAME:-$SE_VIDEO_FILE_NAME}`.
- `video_recorder.py` currently checks only `SE_VIDEO_FILE_NAME == "auto"`.
- Fixed-file branch in `video.sh` starts ffmpeg without consulting `SE_RECORD_VIDEO` or per-session capability.
## Fix Focus Areas
- Video/video_recorder.py[86-98]
- Video/video.sh[234-249]
## Suggested approach
1. In `_run_shell_recorder()`, compute an **effective** configured filename using the same precedence as `video.sh` (e.g., `FILE_NAME` then `SE_VIDEO_FILE_NAME`), and derive `per_session_mode` from that effective value (ideally case-insensitive via `.strip().lower() == "auto"`).
2. Ensure that when the **effective** filename is fixed (not `auto`) and `SE_RECORD_VIDEO=false`, the recorder does not start `video.sh` (idle instead), even if `SE_VIDEO_FILE_NAME` is `auto`.
3. (Optional hardening) Add a `SE_RECORD_VIDEO` guard in `video.sh` fixed-file branch to prevent accidental always-on recording when global recording is disabled.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+5
to
8
| autostart=true | ||
| startsecs=0 | ||
| autorestart=%(ENV_SE_VIDEO_UPLOAD_ENABLED)s | ||
| autorestart=true | ||
| stopsignal=TERM |
Contributor
There was a problem hiding this comment.
3. Uploader busy-waits when disabled 🐞 Bug ➹ Performance
The uploader is now always started by supervisord, and in shell mode it runs upload.sh even when SE_VIDEO_UPLOAD_ENABLED is false. Since video.sh only creates the upload FIFO when uploads are enabled, upload.sh loops forever (sleeping 1s) waiting for a pipe that will never exist, wasting resources.
Agent Prompt
## Issue description
`Video/uploader.conf` now starts the uploader unconditionally. In shell mode (`SE_VIDEO_EVENT_DRIVEN=false`), `video_uploader.py` always executes `upload.sh`, but `upload.sh` waits in a loop for a FIFO that is only created by `video.sh` when uploads are enabled. When `SE_VIDEO_UPLOAD_ENABLED=false`, this becomes an infinite busy-wait loop.
## Issue Context
- `upload.sh` waits for `${UPLOAD_PIPE_FILE}` with a 1-second polling loop.
- `video.sh` only calls `mkfifo` when `VIDEO_UPLOAD_ENABLED=true`.
- PR changed supervisord config so uploader always starts.
## Fix Focus Areas
- Video/uploader.conf[1-9]
- Video/video_uploader.py[17-36]
- Video/upload.sh[95-101]
- Video/video.sh[66-77]
## Suggested fixes (pick one)
1. **Restore conditional supervisord start**: revert to `autostart=%(ENV_SE_VIDEO_UPLOAD_ENABLED)s` and `autorestart=%(ENV_SE_VIDEO_UPLOAD_ENABLED)s`.
2. **Add runtime gating in `video_uploader.py`** (recommended if you must keep autostart=true):
- If `SE_VIDEO_EVENT_DRIVEN=false` and upload is effectively disabled (`SE_VIDEO_UPLOAD_ENABLED!=true` and/or destination prefix empty depending on desired semantics), do not run `upload.sh`; instead idle with a long sleep (or exit cleanly and set supervisord `autorestart=false`).
3. **Make `upload.sh` block without polling** when disabled (e.g., detect disabled config and sleep longer), to avoid 1-second wakeups forever.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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