Skip to content

Docker: Video recording starts via session capability se:recordVideo #3131

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
VietND96 wants to merge 1 commit into
base: trunk
Choose a base branch
from
+30 −16
Open

Docker: Video recording starts via session capability se:recordVideo #3131

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions NodeBase/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,8 @@ ENV LANG_WHICH=${LANG_WHICH} \
#============================
# Some configuration options
#============================
SE_RECORD_VIDEO=false \
SE_VIDEO_FILE_NAME=auto \
SE_RECORD_VIDEO="false" \
SE_VIDEO_FILE_NAME="auto" \
SE_VIDEO_EVENT_DRIVEN="true" \
DISPLAY_CONTAINER_NAME="localhost" \
SE_SCREEN_WIDTH="1920" \
Expand Down
8 changes: 4 additions & 4 deletions Video/recorder.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@
priority=10
command=python3 /opt/bin/video_recorder.py
killasgroup=true
autostart=%(ENV_SE_RECORD_VIDEO)s
autostart=true
startsecs=0
autorestart=%(ENV_SE_RECORD_VIDEO)s
autorestart=true
stopsignal=TERM
stopwaitsecs=30

Expand All @@ -17,9 +17,9 @@ stdout_logfile_maxbytes=0
priority=0
command=python3 /opt/bin/video_ready.py
killasgroup=true
autostart=%(ENV_SE_RECORD_VIDEO)s
autostart=true
startsecs=0
autorestart=%(ENV_SE_RECORD_VIDEO)s
autorestart=true
stopsignal=TERM
stopwaitsecs=5

Expand Down
4 changes: 2 additions & 2 deletions Video/uploader.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@
priority=5
command=python3 /opt/bin/video_uploader.py
killasgroup=true
autostart=%(ENV_SE_VIDEO_UPLOAD_ENABLED)s
autostart=true
startsecs=0
autorestart=%(ENV_SE_VIDEO_UPLOAD_ENABLED)s
autorestart=true
stopsignal=TERM
Comment on lines +5 to 8
Copy link
Copy Markdown
Contributor

@qodo-code-review qodo-code-review Bot May 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

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

stopwaitsecs=30

Expand Down
7 changes: 5 additions & 2 deletions Video/video_nodeQuery.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ def main() -> None:

# Environment variables with defaults
video_cap_name = os.environ.get("VIDEO_CAP_NAME", "se:recordVideo")
default_record_video = os.environ.get("SE_RECORD_VIDEO", "true").lower() != "false"
test_name_cap = os.environ.get("TEST_NAME_CAP", "se:name")
video_name_cap = os.environ.get("VIDEO_NAME_CAP", "se:videoName")
video_file_name_trim = os.environ.get("SE_VIDEO_FILE_NAME_TRIM_REGEX", default_trim_pattern)
Expand All @@ -46,8 +47,10 @@ def main() -> None:
# If JSON parsing fails, continue with None values
pass

# Check if enabling to record video
if (isinstance(record_video, str) and record_video.lower() == "false") or record_video is False:
# Check if enabling to record video; fall back to SE_RECORD_VIDEO when capability is absent
if record_video is None:
record_video = "true" if default_record_video else "false"
elif (isinstance(record_video, str) and record_video.lower() == "false") or record_video is False:
record_video = "false"
else:
record_video = "true"
Expand Down
23 changes: 17 additions & 6 deletions Video/video_recorder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import signal
import subprocess
import sys
import time


def _signal_supervisord() -> None:
Expand Down Expand Up @@ -83,22 +84,32 @@ def _mark_external_shutdown(signum, frame):


def _run_shell_recorder():
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:
Comment on lines +87 to +92
Copy link
Copy Markdown
Contributor

@qodo-code-review qodo-code-review Bot May 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

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

while True:
time.sleep(60)
except KeyboardInterrupt:
pass
return
Comment on lines +92 to +97
Copy link
Copy Markdown
Contributor

@qodo-code-review qodo-code-review Bot May 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

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


proc = subprocess.Popen(["/opt/bin/video.sh"])
_external_shutdown = False # True when supervisord (or user) told us to stop

def forward_signal(signum, frame):
nonlocal _external_shutdown
# Forward the signal to video.sh at most once. supervisord uses
# killasgroup=true so video.sh already received the signal directly;
# re-forwarding on every re-entrant call amplifies the SIGTERM
# ping-pong and can keep the process alive for 60 s.
if not _external_shutdown:
_external_shutdown = True
try:
proc.send_signal(signum)
except ProcessLookupError:
pass # Process already exited before signal was forwarded
proc.wait()
pass
# Do NOT call proc.wait() here — blocking inside a signal handler
# interferes with bash's deferred-signal queue. The main-flow
# proc.wait() below resumes automatically after this returns (PEP 475).

signal.signal(signal.SIGTERM, forward_signal)
signal.signal(signal.SIGINT, forward_signal)
Expand Down
Loading