[grid] Store Dynamic Grid videos in a per-session subfolder via SE_VIDEO_SESSION_SUBFOLDER

[VietND96]

🔗 Related Issues

Follows up on docker-selenium SeleniumHQ/docker-selenium@752d1bf, which added SE_VIDEO_SESSION_SUBFOLDER to the video images.

💥 What does this PR do?

Makes Dynamic Grid use SE_VIDEO_SESSION_SUBFOLDER so the recorder writes videos straight to <assets>/<sessionId>/, which is where the Grid serves session assets from.

Docker, inline recording (--docker-video-image not set): the browser container binds the assets root to /videos, so every session's video used to land flat in the assets root. The recorder now always creates the session folder itself, matching the layout the external video container already gets from its per-session bind mount.

Kubernetes (both inline recording and the video sidecar): the assets volume is mounted at /videos and at the assets path, so a session subfolder written by the recorder is the final location. Grid therefore no longer has to wait for the Pod to terminate and move the file afterwards. This one is opt-in via SE_VIDEO_SESSION_SUBFOLDER=true on the Node, and behaviour is unchanged when it is unset.

🔧 Implementation Notes

On Kubernetes the variable is read from the Node's own environment (default false) rather than added as a CLI flag, matching how KubernetesSessionFactory already reads SE_VIDEO_FILE_NAME, SE_VIDEO_FILE_NAME_SUFFIX and SE_VIDEO_FILE_NAME_TRIM_REGEX. Since setEnvVarsToContainer() already copies every SE_* variable into child containers, opting in is a single Node-level setting.

Three details drove the design:

1. Subfolder mode implies recorder-managed naming. In video.sh the subfolder branch only exists on the dynamic naming path, so a fixed SE_VIDEO_FILE_NAME silently disables the feature. Grid therefore forces SE_VIDEO_FILE_NAME=auto in this mode and stops injecting <jobName>.mp4 on Kubernetes. On the Docker inline path both this and the subfolder are enforced over anything inherited from the Node (with a warning when a fixed name is discarded), because neither inherited value produces a usable layout when the mount is the assets root.

2. The Docker video sidecar gets a blank SE_VIDEO_SESSION_SUBFOLDER. Its bind mount is already per-session (assets/<sessionId> → /videos), so inheriting a Node-level true would produce assets/<id>/<id>/video.mp4. Blanking rather than setting false stops the passthrough while leaving the image's own default in charge — both recorders treat an empty value as disabled.

3. KubernetesSession.stop() only waits for the Pod when a file must be moved. The wait (terminationGracePeriodSeconds + 10) existed solely to let the recorder finish before relocating, so waitForPodTerminated() moved inside relocateVideoFiles(), after the guard. This also removes an existing cost: with SE_VIDEO_FILE_NAME=auto, videoFileName was already null and nothing was relocated, yet every session still paid the full grace period.

Alternative considered and rejected: changing the Docker video container to mount the assets root and use the subfolder for one uniform mechanism. It would have altered a working path for no gain.

Older docker-selenium images simply ignore the variable and keep today's flat layout; there is no version check.

🤖 AI assistance

• AI assisted (complete below)
  • Tool(s): Claude Code (Opus 5)
  • What was generated: The design, the implementation across the three Java files, and the unit tests, driven test-first from a written spec. Design decisions (env-based opt-in on Kubernetes rather than a CLI flag, enforcing the subfolder on the Docker inline path, blanking it for the sidecar, moving the Pod wait) were reviewed and chosen by me.
  • I reviewed all AI output and can explain the change

💡 Additional Considerations

Worth aligning upstream: video.sh honours SE_VIDEO_SESSION_SUBFOLDER only on its dynamic-naming path, while video_service.py honours it for fixed names too. Grid works around this by forcing SE_VIDEO_FILE_NAME=auto; making the two recorders consistent in docker-selenium would let that workaround go away.

No cross-binding impact — this is Grid-side Java only, and no binding exposes video asset layout.

Testing: new DockerSessionFactoryTest (5 cases) and KubernetesSessionTest (2 cases), plus 3 cases added to KubernetesSessionFactoryTest. All 132 tests in //java/test/org/openqa/selenium/grid/... at size small pass, spotbugs included.

🔄 Types of changes

• New feature (non-breaking change which adds functionality and tests!)

---

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Dynamic Grid: opt into SE_VIDEO_SESSION_SUBFOLDER for per-session video layout

✨ Enhancement 🐞 Bug fix 🧪 Tests 🕐 40+ Minutes

AI Description

• Allow Dynamic Grid Nodes to enable per-session video folders via SE_VIDEO_SESSION_SUBFOLDER.
• Force recorder-managed naming (SE_VIDEO_FILE_NAME=auto) when subfolder mode is enabled.
• Skip Kubernetes Pod-termination waits when no video relocation is required.

Diagram

graph TD
  Env["Node env (SE_*)"] --> DSF["DockerSessionFactory"] --> BCont["Browser container"] --> Rec["Video recorder"] --> Assets[("Assets/<sessionId>/")]
  Env --> KSF["KubernetesSessionFactory"] --> Pod["K8s Job/Pod"] --> Rec
  Pod --> KSS["KubernetesSession.stop()"] --> Reloc["Relocate video (if needed)"] --> Assets

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Expose a CLI flag instead of env var

• ➕ More discoverable/configurable at runtime for Grid operators
• ➕ Avoids relying on inherited process environment
• ➖ Inconsistent with existing KubernetesSessionFactory behavior that already reads SE_* directly
• ➖ Still requires careful interaction with SE_VIDEO_FILE_NAME semantics

2. Always keep current layout and relocate post-recording

• ➕ Keeps recorder behavior unchanged across all runtimes
• ➕ Avoids coupling to video image behavior for session subfolders
• ➖ Kubernetes would continue paying Pod-termination wait even when unnecessary
• ➖ Adds extra I/O moves and operational latency; worse for large videos

3. Change the Docker video sidecar/mounting strategy to make subfolder always safe

• ➕ Could reduce special-casing SE_VIDEO_SESSION_SUBFOLDER=false for the sidecar
• ➖ Higher risk: changes container bind-mount contract and can break existing deployments
• ➖ Still must handle recorder fixed-name vs auto-name behavior to actually create subfolders

Recommendation: The PR’s approach is the best trade-off: it aligns Dynamic Grid with docker-selenium’s new recorder capability while preserving default behavior when unset. Forcing SE_VIDEO_FILE_NAME=auto in subfolder mode is necessary due to recorder semantics, and pinning the Docker video sidecar to "false" avoids double-nesting with per-session bind mounts. Moving the Kubernetes Pod-termination wait behind the relocation guard is a clear win in both correctness and performance.

Files changed (6) +446 / -24

Enhancement (2) +70 / -21

DockerSessionFactory.javaEnable SE_VIDEO_SESSION_SUBFOLDER for inline Docker recording and guard naming +43/-10

Enable SE_VIDEO_SESSION_SUBFOLDER for inline Docker recording and guard naming

• Extracts browser env-var construction into a dedicated method and adds support for SE_VIDEO_SESSION_SUBFOLDER during inline recording. When enabled, it forces SE_VIDEO_FILE_NAME=auto (warning if an inherited fixed name would disable subfolder mode). It also pins the external Docker video container to SE_VIDEO_SESSION_SUBFOLDER=false to prevent double-nesting.

java/src/org/openqa/selenium/grid/node/docker/DockerSessionFactory.java

KubernetesSessionFactory.javaPropagate SE_VIDEO_SESSION_SUBFOLDER and enforce recorder-managed naming on K8s +27/-11

Propagate SE_VIDEO_SESSION_SUBFOLDER and enforce recorder-managed naming on K8s

• Introduces addVideoFileNameEnvVars() to centralize video naming/subfolder env var injection for both browser and video sidecar containers. When session-subfolder mode is enabled, it sets SE_VIDEO_SESSION_SUBFOLDER=true and ensures SE_VIDEO_FILE_NAME=auto; otherwise it continues to set jobName.mp4 for later relocation. Session relocation targeting is now skipped when the recorder is expected to manage the final filename/layout.

java/src/org/openqa/selenium/grid/node/kubernetes/KubernetesSessionFactory.java

Bug fix (1) +6 / -3

KubernetesSession.javaAvoid Pod-termination wait unless video relocation is required +6/-3

Avoid Pod-termination wait unless video relocation is required

• Removes the unconditional wait-for-Pod-termination from stop(). The wait is now performed only inside relocateVideoFiles(), and only when assetsPath and a relocation target videoFileName are present.

java/src/org/openqa/selenium/grid/node/kubernetes/KubernetesSession.java

Tests (3) +370 / -0

DockerSessionFactoryTest.javaUnit tests for Docker inline video env var composition +153/-0

Unit tests for Docker inline video env var composition

• Adds coverage for inline recording env vars with subfolder mode on/off, verifies that subfolder mode forces SE_VIDEO_FILE_NAME=auto, and asserts the Docker video sidecar is always pinned to SE_VIDEO_SESSION_SUBFOLDER=false.

java/test/org/openqa/selenium/grid/node/docker/DockerSessionFactoryTest.java

KubernetesSessionFactoryTest.javaUnit tests for K8s session subfolder env vars +89/-0

Unit tests for K8s session subfolder env vars

• Adds a factory variant that forces isVideoSessionSubfolder() true and verifies browser/video containers receive SE_VIDEO_SESSION_SUBFOLDER=true and SE_VIDEO_FILE_NAME=auto when recording. Also verifies the variable is absent when the session is not recording.

java/test/org/openqa/selenium/grid/node/kubernetes/KubernetesSessionFactoryTest.java

KubernetesSessionTest.javaUnit tests for stop() wait/relocation behavior +128/-0

Unit tests for stop() wait/relocation behavior

• Adds tests ensuring stop() does not poll/wait for Pod termination when there is nothing to relocate, and that relocation moves jobName.mp4 into the per-session folder when configured.

java/test/org/openqa/selenium/grid/node/kubernetes/KubernetesSessionTest.java

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📜 Skill insights (0)

1. Duplicate K8s video env 🐞 Bug ≡ Correctness

Description

KubernetesSessionFactory.addVideoFileNameEnvVars appends SE_VIDEO_FILE_NAME=auto without
removing an inherited SE_VIDEO_FILE_NAME already added by setEnvVarsToContainer, producing
duplicate env entries in the Pod spec. In SE_VIDEO_SESSION_SUBFOLDER mode this makes the effective
file-name selection runtime-dependent and can prevent subfolder creation while the session also
skips relocation (videoFileName=null), so the video may not land under the per-session assets
directory.

Code

java/src/org/openqa/selenium/grid/node/kubernetes/KubernetesSessionFactory.java[R760-763]

+      if (!isVideoFileNameAuto()) {
+        envVars.add(new EnvVarBuilder().withName("SE_VIDEO_FILE_NAME").withValue("auto").build());
+      }
+    } else if (!isVideoFileNameAuto()) {

Evidence

The session env list is built by first forwarding all SE_* vars from the Node process (which can
include SE_VIDEO_FILE_NAME), then addVideoFileNameEnvVars appends SE_VIDEO_FILE_NAME=auto in
subfolder mode without removing the inherited entry. The job/session logic also treats subfolder
mode as recorder-managed and therefore does not set videoFileName for relocation, increasing the
impact if the override doesn’t take effect as intended.

java/src/org/openqa/selenium/grid/node/kubernetes/KubernetesSessionFactory.java[694-768]
java/src/org/openqa/selenium/grid/node/kubernetes/KubernetesSessionFactory.java[490-495]
java/src/org/openqa/selenium/grid/node/kubernetes/KubernetesSessionFactory.java[918-924]
java/src/org/openqa/selenium/grid/node/kubernetes/KubernetesSessionFactory.java[1048-1066]
java/src/org/openqa/selenium/grid/node/kubernetes/KubernetesSession.java[177-186]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
When `SE_VIDEO_SESSION_SUBFOLDER` is enabled, the factory tries to force recorder-managed naming by adding `SE_VIDEO_FILE_NAME=auto`. However, `buildSessionEnvVars` (and `buildVideoEnvVars`) first forwards *all* `SE_*` env vars from the Node process into the container env list, so a preexisting `SE_VIDEO_FILE_NAME` can already be present. Appending a second `SE_VIDEO_FILE_NAME` yields duplicate names in the container env list, making the effective value dependent on downstream handling.

## Issue Context
- The code relies on overriding an inherited fixed `SE_VIDEO_FILE_NAME` to `auto` in subfolder mode.
- Kubernetes env vars are represented as a `List<EnvVar>`; current code appends and does not replace.

## Fix Focus Areas
- java/src/org/openqa/selenium/grid/node/kubernetes/KubernetesSessionFactory.java[694-768]
- java/src/org/openqa/selenium/grid/node/kubernetes/KubernetesSessionFactory.java[1048-1066]
- java/src/org/openqa/selenium/grid/node/kubernetes/KubernetesSessionFactory.java[490-495]
- java/src/org/openqa/selenium/grid/node/kubernetes/KubernetesSessionFactory.java[918-924]

## Suggested fix
1. Introduce an “upsert” helper for env vars, e.g.:
  - `static void putEnv(List<EnvVar> env, String name, String value)` that `removeIf(ev -> name.equals(ev.getName()))` then `add(new EnvVarBuilder().withName(name).withValue(value).build())`.
2. Use this helper in `addVideoFileNameEnvVars` (and anywhere else you intend to override inherited vars) so that enabling subfolder mode results in exactly one `SE_VIDEO_FILE_NAME` entry with value `auto`, and exactly one `SE_VIDEO_SESSION_SUBFOLDER` entry.
3. Add/adjust a unit test to cover the “inherited fixed `SE_VIDEO_FILE_NAME` + sessionSubfolder enabled” case (likely by subclassing `KubernetesSessionFactory` to override `setEnvVarsToContainer` similarly to the Docker tests), and assert there is only one `SE_VIDEO_FILE_NAME` env var and its value is `auto`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Empty env overrides defaults 🐞 Bug ≡ Correctness

Description

getVideoContainerEnvVars sets SE_VIDEO_SESSION_SUBFOLDER to an empty string, which is still sent
to Docker as an explicit SE_VIDEO_SESSION_SUBFOLDER= environment entry and therefore overrides any
default baked into the video image. This contradicts the comment that it “leaves the image default
in charge” and makes behavior depend on how the image interprets an empty value rather than an unset
variable.

Code

java/src/org/openqa/selenium/grid/node/docker/DockerSessionFactory.java[R472-474]

+    // the recorder must not nest a second session folder inside it. Blanking the value stops a
+    // Node-level setting from passing through and leaves the image default in charge.
+    envVars.put("SE_VIDEO_SESSION_SUBFOLDER", "");

Evidence

The PR sets the variable to an empty string in the video container env. ContainerConfig serializes
env vars into key=value strings without filtering empty values, so Docker receives an explicit
SE_VIDEO_SESSION_SUBFOLDER= entry, which overrides image defaults rather than deferring to them.

java/src/org/openqa/selenium/grid/node/docker/DockerSessionFactory.java[459-476]
java/src/org/openqa/selenium/docker/ContainerConfig.java[337-341]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`SE_VIDEO_SESSION_SUBFOLDER` is currently forced to the empty string for the Docker video sidecar. The Docker client serializes env vars as `key=value` strings, so `""` becomes `SE_VIDEO_SESSION_SUBFOLDER=` (explicitly set), which overrides any image-level default and does not truly “leave the image default in charge”.

### Issue Context
The intent (per code comment) is to prevent inheriting a Node-level `SE_VIDEO_SESSION_SUBFOLDER=true` into the video sidecar while not unintentionally overriding the image’s own default behavior.

### Fix Focus Areas
- java/src/org/openqa/selenium/grid/node/docker/DockerSessionFactory.java[471-475]
- java/test/org/openqa/selenium/grid/node/docker/DockerSessionFactoryTest.java[128-137]

### Suggested fix
Prefer one of:
1) **Explicitly disable**: `envVars.put("SE_VIDEO_SESSION_SUBFOLDER", "false");` (most robust, avoids ambiguity of empty string). Update the unit test expectation accordingly.
2) **Truly defer to image default**: `envVars.remove("SE_VIDEO_SESSION_SUBFOLDER");` after `setEnvVarsToContainer(envVars)` (and update the comment + test).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

Previous review results

Review updated until commit 4706596 ⚖️ Balanced

Results up to commit 38b19c9 ⚖️ Balanced

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Duplicate K8s video env 🐞 Bug ≡ Correctness

Description

KubernetesSessionFactory.addVideoFileNameEnvVars appends SE_VIDEO_FILE_NAME=auto without
removing an inherited SE_VIDEO_FILE_NAME already added by setEnvVarsToContainer, producing
duplicate env entries in the Pod spec. In SE_VIDEO_SESSION_SUBFOLDER mode this makes the effective
file-name selection runtime-dependent and can prevent subfolder creation while the session also
skips relocation (videoFileName=null), so the video may not land under the per-session assets
directory.

Code

java/src/org/openqa/selenium/grid/node/kubernetes/KubernetesSessionFactory.java[R760-763]

+      if (!isVideoFileNameAuto()) {
+        envVars.add(new EnvVarBuilder().withName("SE_VIDEO_FILE_NAME").withValue("auto").build());
+      }
+    } else if (!isVideoFileNameAuto()) {

Evidence

The session env list is built by first forwarding all SE_* vars from the Node process (which can
include SE_VIDEO_FILE_NAME), then addVideoFileNameEnvVars appends SE_VIDEO_FILE_NAME=auto in
subfolder mode without removing the inherited entry. The job/session logic also treats subfolder
mode as recorder-managed and therefore does not set videoFileName for relocation, increasing the
impact if the override doesn’t take effect as intended.

java/src/org/openqa/selenium/grid/node/kubernetes/KubernetesSessionFactory.java[694-768]
java/src/org/openqa/selenium/grid/node/kubernetes/KubernetesSessionFactory.java[490-495]
java/src/org/openqa/selenium/grid/node/kubernetes/KubernetesSessionFactory.java[918-924]
java/src/org/openqa/selenium/grid/node/kubernetes/KubernetesSessionFactory.java[1048-1066]
java/src/org/openqa/selenium/grid/node/kubernetes/KubernetesSession.java[177-186]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
When `SE_VIDEO_SESSION_SUBFOLDER` is enabled, the factory tries to force recorder-managed naming by adding `SE_VIDEO_FILE_NAME=auto`. However, `buildSessionEnvVars` (and `buildVideoEnvVars`) first forwards *all* `SE_*` env vars from the Node process into the container env list, so a preexisting `SE_VIDEO_FILE_NAME` can already be present. Appending a second `SE_VIDEO_FILE_NAME` yields duplicate names in the container env list, making the effective value dependent on downstream handling.

## Issue Context
- The code relies on overriding an inherited fixed `SE_VIDEO_FILE_NAME` to `auto` in subfolder mode.
- Kubernetes env vars are represented as a `List<EnvVar>`; current code appends and does not replace.

## Fix Focus Areas
- java/src/org/openqa/selenium/grid/node/kubernetes/KubernetesSessionFactory.java[694-768]
- java/src/org/openqa/selenium/grid/node/kubernetes/KubernetesSessionFactory.java[1048-1066]
- java/src/org/openqa/selenium/grid/node/kubernetes/KubernetesSessionFactory.java[490-495]
- java/src/org/openqa/selenium/grid/node/kubernetes/KubernetesSessionFactory.java[918-924]

## Suggested fix
1. Introduce an “upsert” helper for env vars, e.g.:
  - `static void putEnv(List<EnvVar> env, String name, String value)` that `removeIf(ev -> name.equals(ev.getName()))` then `add(new EnvVarBuilder().withName(name).withValue(value).build())`.
2. Use this helper in `addVideoFileNameEnvVars` (and anywhere else you intend to override inherited vars) so that enabling subfolder mode results in exactly one `SE_VIDEO_FILE_NAME` entry with value `auto`, and exactly one `SE_VIDEO_SESSION_SUBFOLDER` entry.
3. Add/adjust a unit test to cover the “inherited fixed `SE_VIDEO_FILE_NAME` + sessionSubfolder enabled” case (likely by subclassing `KubernetesSessionFactory` to override `setEnvVarsToContainer` similarly to the Docker tests), and assert there is only one `SE_VIDEO_FILE_NAME` env var and its value is `auto`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Results up to commit 5ad47d6 ⚖️ Balanced

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Empty env overrides defaults 🐞 Bug ≡ Correctness

Description

getVideoContainerEnvVars sets SE_VIDEO_SESSION_SUBFOLDER to an empty string, which is still sent
to Docker as an explicit SE_VIDEO_SESSION_SUBFOLDER= environment entry and therefore overrides any
default baked into the video image. This contradicts the comment that it “leaves the image default
in charge” and makes behavior depend on how the image interprets an empty value rather than an unset
variable.

Code

java/src/org/openqa/selenium/grid/node/docker/DockerSessionFactory.java[R472-474]

+    // the recorder must not nest a second session folder inside it. Blanking the value stops a
+    // Node-level setting from passing through and leaves the image default in charge.
+    envVars.put("SE_VIDEO_SESSION_SUBFOLDER", "");

Evidence

The PR sets the variable to an empty string in the video container env. ContainerConfig serializes
env vars into key=value strings without filtering empty values, so Docker receives an explicit
SE_VIDEO_SESSION_SUBFOLDER= entry, which overrides image defaults rather than deferring to them.

java/src/org/openqa/selenium/grid/node/docker/DockerSessionFactory.java[459-476]
java/src/org/openqa/selenium/docker/ContainerConfig.java[337-341]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`SE_VIDEO_SESSION_SUBFOLDER` is currently forced to the empty string for the Docker video sidecar. The Docker client serializes env vars as `key=value` strings, so `""` becomes `SE_VIDEO_SESSION_SUBFOLDER=` (explicitly set), which overrides any image-level default and does not truly “leave the image default in charge”.

### Issue Context
The intent (per code comment) is to prevent inheriting a Node-level `SE_VIDEO_SESSION_SUBFOLDER=true` into the video sidecar while not unintentionally overriding the image’s own default behavior.

### Fix Focus Areas
- java/src/org/openqa/selenium/grid/node/docker/DockerSessionFactory.java[471-475]
- java/test/org/openqa/selenium/grid/node/docker/DockerSessionFactoryTest.java[128-137]

### Suggested fix
Prefer one of:
1) **Explicitly disable**: `envVars.put("SE_VIDEO_SESSION_SUBFOLDER", "false");` (most robust, avoids ambiguity of empty string). Update the unit test expectation accordingly.
2) **Truly defer to image default**: `envVars.remove("SE_VIDEO_SESSION_SUBFOLDER");` after `setEnvVarsToContainer(envVars)` (and update the comment + test).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[qodo-code-review]

Code review by qodo was updated up to the latest commit 5ad47d6

[qodo-code-review]

Code review by qodo was updated up to the latest commit 4706596