镜像站点 · 本页由第三方 GitHub 只读镜像提供,非 GitHub 官方站点,不接受任何登录或凭据输入。前往 github.com
Skip to content

[fix] Don't bind VIDEO_READY_PORT when video can't actually run - #3277

Open
VietND96 wants to merge 1 commit into
trunkfrom
fix/video-ready-port-when-disabled
Open

VietND96 wants to merge 1 commit into
trunkfrom
fix/video-ready-port-when-disabled

Conversation

@VietND96

Copy link
Copy Markdown
Member

Summary

Fixes #3271.

Every browser Node image (node-chrome, node-chromium, node-firefox, node-edge, ...) ships video_ready.py because node-base is built FROM the video image. Video/recorder.conf's video-ready program has listened on port 9000 unconditionally since PR #3131, regardless of SE_RECORD_VIDEO - that PR made both video-recording's and video-ready's supervisord autostart/autorestart unconditionally true so a session could turn recording on dynamically via the se:recordVideo capability, replacing the old autostart=%(ENV_SE_RECORD_VIDEO)s gating.

Tracing where that capability is actually handled: entirely inside video_recorder.py / video_service.py (via SE_VIDEO_FILE_NAME=auto in legacy/shell mode, or the event-driven service in SE_VIDEO_EVENT_DRIVEN=true mode). video-ready has no part in it, and nothing in this repo ever queries its HTTP endpoint - the only documented consumer is the Helm chart's optional video-recorder-sidecar startupProbe/livenessProbe (videoRecorder.startupProbe/livenessProbe in values.yaml), which are empty/off by default. video_ready.py's own handler already special-cases the condition where nothing can happen: when neither SE_VIDEO_EVENT_DRIVEN nor SE_RECORD_VIDEO is true, it always answers {"status": "ready"} immediately, every single time - so a container in that exact state gets zero value from the server actually running, only the bound port and the symptom in the issue (fails under network_mode: host, since the container shares the host's network namespace entirely so even a loopback-only bind wouldn't help here - only not starting the process does).

What this does

  • Video/entry_point.sh computes a new SE_VIDEO_READY_ENABLED from SE_RECORD_VIDEO and SE_VIDEO_EVENT_DRIVEN (or respects an explicit override) before launching supervisord.
  • Video/recorder.conf gates video-ready's autostart/autorestart on it, restoring the pre-Docker: Video recording starts via session capability se:recordVideo #3131-style conditional start for this one program - video-recording keeps its unconditional autostart=true, since the se:recordVideo capability feature genuinely depends on it always running and is otherwise untouched by this change.
  • Documents VIDEO_READY_PORT (existing, previously undocumented, per the issue) and the new SE_VIDEO_READY_ENABLED in ENV_VARIABLES.md, including the direct opt-out the issue asked for ("document an environment variable that disables it") - SE_VIDEO_READY_ENABLED=false forces it off regardless of the other two, e.g. for network_mode: host.

Important: NodeBase/Dockerfile's own defaults are SE_RECORD_VIDEO=false and SE_VIDEO_EVENT_DRIVEN=true - so by default, video-ready keeps running exactly as it does today (nothing changes out of the box). The issue's reporter set SE_RECORD_VIDEO=false explicitly without also disabling event-driven mode, which is already the Node image's default and not what actually stops video-ready. Reaching the issue's desired "nothing video-related is listening" state needs SE_VIDEO_EVENT_DRIVEN=false too (now documented), or the new direct override.

Test plan

  • Traced every consumer of video_ready.py's endpoint in this repo (none besides the chart's own off-by-default probe docs) and of SE_VIDEO_EVENT_DRIVEN/SE_RECORD_VIDEO in the recording-side scripts, to confirm gating video-ready alone doesn't affect the se:recordVideo capability feature.
  • Built a minimal container with the actual Video/recorder.conf, Video/entry_point.sh, and Video/video_ready.py under real supervisord, and verified all four cases:
    • No overrides -> video-ready starts, port 9000 responds (matches current/default behavior, unchanged).
    • SE_RECORD_VIDEO=false SE_VIDEO_EVENT_DRIVEN=false -> video-ready does not start, port 9000 refuses connections.
    • Same, plus SE_VIDEO_READY_ENABLED=true -> starts anyway (explicit force-on).
    • SE_RECORD_VIDEO=true SE_VIDEO_EVENT_DRIVEN=true SE_VIDEO_READY_ENABLED=false -> does not start (explicit force-off overrides both).
  • bash -n syntax check and make lint_format_scripts (shfmt) on Video/entry_point.sh.

🤖 Generated with Claude Code

Fixes #3271. Every browser Node image ships video_ready.py (recorder
autostart) because node-base builds FROM the video image, and
video-ready has always listened on port 9000 regardless of
SE_RECORD_VIDEO - PR #3131 made both video-recording's and
video-ready's supervisord autostart unconditional so a session could
turn recording on dynamically via the se:recordVideo capability.

That capability is handled entirely by video_recorder.py /
video_service.py (SE_VIDEO_FILE_NAME=auto / the event-driven service);
video-ready has no part in it and nothing in this repo queries its
endpoint outside the Helm chart's optional, off-by-default sidecar
probes. Its own handler already special-cases this: when neither
SE_VIDEO_EVENT_DRIVEN nor SE_RECORD_VIDEO is true, it always answers
"ready" immediately - so a container in that state gets nothing from
running it, only the bound port.

Compute SE_VIDEO_READY_ENABLED in Video/entry_point.sh from those two
variables (or an explicit override) and have recorder.conf gate
video-ready's autostart/autorestart on it. NodeBase's own defaults
(SE_RECORD_VIDEO=false, SE_VIDEO_EVENT_DRIVEN=true) are unchanged by
this - video-ready keeps running exactly as it does today unless a
deployment also turns event-driven mode off, or sets
SE_VIDEO_READY_ENABLED=false directly (the explicit opt-out the issue
asked for, e.g. for network_mode: host).

Verified in a real container with the actual recorder.conf and
entry_point.sh under supervisord: defaults listen as today; both
variables false stops it from binding the port at all; the explicit
override forces it on or off regardless of the other two.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@qodo-code-review

Copy link
Copy Markdown
Contributor

ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[🐛 Bug]: node image always binds video_ready.py to port 9000 when recording is off

1 participant