[grid] Dynamic K8s video always use the per-session subfolder; remove pod-wait + relocation

[VietND96]

🔗 Related Issues

Paired recorder change in docker-selenium (consumes the new SE_NODE_CONTAINER_NAME env as the subfolder fallback): branch k8s-video-subfolder-container-name-fallback.

💥 What does this PR do?

For the Kubernetes Dynamic Grid, both inline recording and the external video sidecar now always use the per-session subfolder approach. The Node sets SE_VIDEO_SESSION_SUBFOLDER=true + SE_VIDEO_FILE_NAME=auto on the browser Job / video sidecar, so the recorder writes each video straight to its final /videos/<sessionId>/ location on the shared assets volume. SE_NODE_CONTAINER_NAME (the Pod name via the downward API metadata.name) is also passed so the recorder can fall back to it when a session id is unavailable.

This removes the Node-side relocation path entirely. KubernetesSession no longer:

• waits for the Pod to terminate (waitForPodTerminated),
• moves jobName.mp4 into <sessionId>/ (relocateVideoFiles, waitForFileStable),
• carries the videoFileName field.

KubernetesSessionFactory drops the now-dead naming helpers (resolveVideoFileName, getVideoFileName, getVideoFileNameTrimRegex, isVideoFileNameAuto, isVideoSessionSubfolder, isRecorderManagedFileName).

🔧 Implementation Notes

Per-session grouping in K8s previously relied on the Node relocating the recorder's flat jobName.mp4 into <sessionId>/ after waiting for the Pod to terminate. The recorder already supports SE_VIDEO_SESSION_SUBFOLDER and writes <sessionId>/ directly on the same /videos volume as the assets path, so the relocation + pod-wait is redundant and only adds latency and complexity to session teardown. Using the subfolder approach for inline and external unifies the two modes and lets the recorder own the final path.

The SE_NODE_CONTAINER_NAME fallback guarantees a unique folder per Pod, so a recording never lands flat and collides on the shared ReadWriteMany assets volume if a session id is ever missing.

🤖 AI assistance

• AI assisted (complete below)
  • Tool(s): Claude Code (Opus)
  • What was generated: the KubernetesSessionFactory / KubernetesSession changes and the unit-test updates
  • I reviewed all AI output and can explain the change

💡 Additional Considerations

• Requires the paired docker-selenium recorder change for the SE_NODE_CONTAINER_NAME subfolder fallback.
• Version coupling: a Node on this version paired with a much older pinned browser/video image (one that predates recorder-side SE_VIDEO_SESSION_SUBFOLDER support) would record flat instead of per-session, since there is no longer a relocation fallback. Recommend a browser/video image at least matching the Grid version.

🔄 Types of changes

• Cleanup (formatting, renaming)
• New feature (non-breaking change which adds functionality and tests!)

[qodo-code-review]

PR Summary by Qodo

K8s Dynamic Grid: always record videos into per-session subfolders; drop relocation

✨ Enhancement 🧪 Tests 🕐 20-40 Minutes

AI Description

• Force inline and sidecar recording to write directly into /videos//.
• Pass Pod name via SE_NODE_CONTAINER_NAME as a subfolder fallback when sessionId is unavailable.
• Remove Node-side pod-wait and video relocation logic; update K8s session factory tests.

Diagram

graph TD
  A["KubernetesSessionFactory"] --> H["Video env vars: subfolder+auto+podName"] --> B["K8s Job/Pod"] --> C["Browser container"] --> E[("Shared /videos volume")]
  B --> D["Video sidecar"] --> E
  S["KubernetesSession.stop()"] --> K["Kubernetes API"] --> B

  subgraph Legend
    direction LR
    _comp["Component"] ~~~ _k8s["K8s resource"] ~~~ _vol[("Shared volume")]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Keep relocation as a backward-compatibility fallback

• ➕ Avoids behavior changes when users pin older recorder images without subfolder support
• ➕ Reduces risk of flat recordings colliding on shared RWX volumes
• ➖ Retains pod polling / file stability heuristics and teardown latency
• ➖ More complex code paths to maintain and test
• ➖ Hard to reliably detect recorder capabilities at runtime

2. Gate the new behavior behind an explicit Node env flag

• ➕ Lets operators roll out gradually and revert quickly if needed
• ➕ Provides a clear migration path for pinned image fleets
• ➖ Adds another configuration knob and support burden
• ➖ Splits behavior across installations, complicating troubleshooting

Recommendation: The PR’s approach (always using recorder-managed per-session subfolders) is the simplest and removes teardown latency/complexity. Given the explicit version-coupling note, consider adding a short migration note in release docs or logging a warning when recording is enabled but the output is not observed in a per-session directory (best-effort), rather than reintroducing relocation logic.

Files changed (3) +39 / -228

Enhancement (1) +22 / -94

KubernetesSessionFactory.javaAlways enable recorder-managed per-session subfolder recording for Job and sidecar +22/-94

Always enable recorder-managed per-session subfolder recording for Job and sidecar

• Removes all video naming and trim-regex helpers and stops passing a resolved video file name into KubernetesSession. Injects SE_VIDEO_SESSION_SUBFOLDER=true and SE_VIDEO_FILE_NAME=auto for both inline and sidecar recording, and adds SE_NODE_CONTAINER_NAME sourced from the Pod name via the downward API.

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

Refactor (1) +3 / -101

KubernetesSession.javaRemove pod-termination wait and video relocation from session stop() +3/-101

Remove pod-termination wait and video relocation from session stop()

• Drops the relocation-based recording cleanup path by removing pod polling, file stability checks, and the videoFileName field/constructor parameter. Session stop now deletes the Job and relies on the recorder to write directly to the final per-session location.

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

Tests (1) +14 / -33

KubernetesSessionFactoryTest.javaUpdate env-var expectations to auto filename + subfolder + pod-name fallback +14/-33

Update env-var expectations to auto filename + subfolder + pod-name fallback

• Simplifies the subfolder factory helper (now identical to the standard image factory) and updates assertions to expect SE_VIDEO_FILE_NAME=auto. Adds checks for SE_VIDEO_SESSION_SUBFOLDER=true and presence of SE_NODE_CONTAINER_NAME for browser and video containers.

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

[qodo-code-review]

Code Review by Qodo

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

1. SE_VIDEO_FILE_NAME forced to auto 📘 Rule violation ≡ Correctness

Description

KubernetesSessionFactory now unconditionally forces recorder-managed naming by setting
SE_VIDEO_SESSION_SUBFOLDER=true and SE_VIDEO_FILE_NAME=auto for every recorded session and
removes the prior node-side relocation behavior. This backward-incompatible change breaks existing
Kubernetes Dynamic Grid deployments that relied on the previous jobName.mp4 + relocation flow or
on capability-driven naming via se:videoName/se:name, diverging from the Docker dynamic grid and
documented dynamic-grid expectations without a deprecation/compatibility path.

Code

java/src/org/openqa/selenium/grid/node/kubernetes/KubernetesSessionFactory.java[R753-756]

+    envVars.add(
+        new EnvVarBuilder().withName("SE_VIDEO_SESSION_SUBFOLDER").withValue("true").build());
+    envVars.add(new EnvVarBuilder().withName("SE_VIDEO_FILE_NAME").withValue("auto").build());
+    envVars.add(

Evidence

PR Compliance ID 1 requires avoiding breaking public functionality without deprecation, and the
cited code changes show that Kubernetes now always injects SE_VIDEO_SESSION_SUBFOLDER=true and
SE_VIDEO_FILE_NAME=auto during env var construction for recorded sessions, leaving no path for
se:videoName/se:name (or user-provided values) to influence the filename. Additionally,
KubernetesSession.stop() no longer waits/relocates the produced video artifact, removing the
previous fallback that enabled the jobName.mp4 output + relocation flow if the recorder did not
implement the new env-var-based conventions; meanwhile Docker dynamic grid still maps those
capabilities into SE_VIDEO_FILE_NAME, and the repository’s documented dynamic-grid behavior
reflects that capability-driven naming feature.

AGENTS.md: Maintain API/ABI Compatibility Unless Following Deprecation Policy
java/src/org/openqa/selenium/grid/node/kubernetes/KubernetesSessionFactory.java[731-756]
java/src/org/openqa/selenium/grid/node/kubernetes/KubernetesSession.java[92-96]
java/src/org/openqa/selenium/grid/node/kubernetes/KubernetesSessionFactory.java[724-765]
java/src/org/openqa/selenium/grid/node/docker/DockerSessionFactory.java[459-475]
java/CHANGELOG[516-527]

Agent prompt

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

## Issue description
Kubernetes Dynamic Grid video output behavior has become backward-incompatible: `KubernetesSessionFactory` now forces recorder-managed naming by always setting `SE_VIDEO_SESSION_SUBFOLDER=true` and `SE_VIDEO_FILE_NAME=auto`, which removes both (a) the prior node-side relocation/waiting fallback that supported the `jobName.mp4` + relocation flow and (b) capability-driven naming via `se:videoName` / `se:name` (and any user-provided `SE_VIDEO_FILE_NAME`). This breaks existing K8s deployments and diverges from Docker dynamic grid and documented dynamic-grid video naming expectations, without an explicit deprecation/compatibility path.

## Issue Context
- `addVideoSubfolderEnvVars` currently always injects `SE_VIDEO_SESSION_SUBFOLDER=true` and `SE_VIDEO_FILE_NAME=auto` for recorded sessions, overriding any capability-driven or user-configured naming.
- `KubernetesSession.stop()` no longer performs relocation/waiting, so if the recorder image does not honor these env vars there is no fallback behavior.
- The repository documents dynamic-grid support for `se:videoName`, and Docker dynamic grid still maps `se:videoName`/`se:name` into `SE_VIDEO_FILE_NAME`; K8s should either preserve this behavior or explicitly deprecate it with corresponding documentation/changelog updates.

## Fix Focus Areas
- java/src/org/openqa/selenium/grid/node/kubernetes/KubernetesSessionFactory.java[724-765]
- java/src/org/openqa/selenium/grid/node/kubernetes/KubernetesSessionFactory.java[731-765]
- java/src/org/openqa/selenium/grid/node/kubernetes/KubernetesSession.java[92-96]
- java/test/org/openqa/selenium/grid/node/kubernetes/KubernetesSessionFactoryTest.java[785-850]

## Expected remediation options (choose one)
- Reintroduce a temporary backward-compatibility path (e.g., keep relocation behind a feature flag/env var) with a clear deprecation plan.
- Or, preserve/conditionally honor existing `SE_VIDEO_FILE_NAME`/`SE_VIDEO_SESSION_SUBFOLDER` behavior when explicitly configured by the user and/or when `se:videoName`/`se:name` are provided (avoid forcibly overriding), and emit a deprecation warning/log for the old behavior.

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

---

2. Env var duplicates possible 🐞 Bug ☼ Reliability

Description

The K8s session env var list is built by appending all inherited SE_* env vars and then appending
the new SE_VIDEO_* and SE_NODE_CONTAINER_NAME entries, so if the Node process already has any of
those names set, the Pod spec will contain duplicate env var names. This makes the effective value
dependent on duplicate-handling semantics and can prevent reliably enforcing the intended recording
configuration.

Code

java/src/org/openqa/selenium/grid/node/kubernetes/KubernetesSessionFactory.java[R731-733]

+    // Video recording env vars: inline and external both use the per-session subfolder approach.
    if (recordVideoForSession(sessionCapabilities)) {
-      addVideoFileNameEnvVars(envVars, jobName);
+      addVideoSubfolderEnvVars(envVars);

Evidence

The code forwards all SE_* env vars into a list (no replacement), then appends the new
video/subfolder vars, while the template merge path explicitly removes conflicts by name—showing
that name-level conflict handling is expected but not applied in the image-mode builder.

java/src/org/openqa/selenium/grid/node/kubernetes/KubernetesSessionFactory.java[686-699]
java/src/org/openqa/selenium/grid/node/kubernetes/KubernetesSessionFactory.java[724-765]
java/src/org/openqa/selenium/grid/node/kubernetes/KubernetesSessionFactory.java[993-1011]

Agent prompt

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

### Issue description
`buildSessionEnvVars`/`buildVideoEnvVars` forward all `SE_*` environment variables from the Node process into a `List<EnvVar>` and then append new `EnvVar` entries for `SE_VIDEO_SESSION_SUBFOLDER`, `SE_VIDEO_FILE_NAME`, and `SE_NODE_CONTAINER_NAME`. If any of those keys already exist in the forwarded env vars, the container will have duplicate entries for the same name.

### Issue Context
Template-mode merging explicitly removes name conflicts, indicating uniqueness-by-name is intended. Image-mode env var building should similarly ensure a single deterministic value per env var name (especially for the new `valueFrom`-based `SE_NODE_CONTAINER_NAME`).

### Fix Focus Areas
- Enforce uniqueness by removing/replacing any existing env var with the same name before adding video/subfolder vars:
 - java/src/org/openqa/selenium/grid/node/kubernetes/KubernetesSessionFactory.java[686-765]
- Ensure template-mode merge continues to produce a single entry per name (avoid duplicates inside `sessionEnvVars`):
 - java/src/org/openqa/selenium/grid/node/kubernetes/KubernetesSessionFactory.java[993-1011]

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

---

3. Unused termination grace state 🐞 Bug ⚙ Maintainability

Description

KubernetesSession still stores terminationGracePeriodSeconds even though pod-wait logic was removed,
leaving dead state and a redundant constructor argument. This increases maintenance burden and can
confuse readers into assuming stop() behavior still depends on the grace period.

Code

java/src/org/openqa/selenium/grid/node/kubernetes/KubernetesSession.java[L117-120]

-  private void waitForPodTerminated() {
-    // Wait for the termination grace period plus a small buffer for K8s overhead
-    Duration timeout = Duration.ofSeconds(terminationGracePeriodSeconds + 10);
-    Instant deadline = Instant.now().plus(timeout);

Evidence

KubernetesSession still declares and assigns terminationGracePeriodSeconds, but stop() now only
saves logs, closes port-forward, and deletes the Job; the removed pod-wait method was the apparent
consumer of the grace period value.

java/src/org/openqa/selenium/grid/node/kubernetes/KubernetesSession.java[46-97]
java/src/org/openqa/selenium/grid/node/kubernetes/KubernetesSessionFactory.java[489-506]

Agent prompt

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

### Issue description
After removing `waitForPodTerminated`, `KubernetesSession` no longer reads `terminationGracePeriodSeconds`, but still stores it and accepts it in the constructor.

### Issue Context
`terminationGracePeriodSeconds` remains meaningful for Job/Pod spec construction in `KubernetesSessionFactory`, but it no longer affects session teardown behavior within `KubernetesSession`.

### Fix Focus Areas
- Remove the unused field + constructor parameter from KubernetesSession:
 - java/src/org/openqa/selenium/grid/node/kubernetes/KubernetesSession.java[46-79]
- Update the KubernetesSession instantiation to stop passing the redundant argument:
 - java/src/org/openqa/selenium/grid/node/kubernetes/KubernetesSessionFactory.java[489-506]

ⓘ 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.}