[grid] Dynamic Grid Docker configure container stop-grace-period

[VietND96]

🔗 Related Issues

💥 What does this PR do?

Previously, DockerSession.stop() had two problems:

1. The stop timeouts for browser and video containers were hardcoded (60s and 10s respectively) with no way for operators to tune them.
2. There was no try-finally protection: if videoContainer.stop() or saveLogs() threw an unchecked exception, container.stop() and super.stop() were skipped, leaking the browser container.

Change for

• A new --docker-stop-grace-period flag (and [docker] stop-grace-period TOML key) lets operators configure this value. Default is 60 seconds, preserving the previous browser container timeout. The value applies to both the browser container and the video container.
• DockerSession.stop() now uses try-finally to guarantee that container.stop() and super.stop() always execute, regardless of failures during video container stop or log saving.

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

• Cleanup (formatting, renaming)
• Bug fix (backwards compatible)
• New feature (non-breaking change which adds functionality and tests!)
• Breaking change (fix or feature that would cause existing functionality to change)

[qodo-code-review]

Review Summary by Qodo

Make Docker container stop grace period configurable and add cleanup guarantees

✨ Enhancement 🐞 Bug fix

Walkthroughs

Description

• Add configurable --docker-stop-grace-period flag for container shutdown timeout
• Replace hardcoded stop timeouts (60s browser, 10s video) with configurable grace period
• Implement try-finally protection in DockerSession.stop() to prevent container leaks
• Add comprehensive unit tests for DockerSession stop behavior and error handling

File Changes

1. java/src/org/openqa/selenium/grid/node/docker/DockerFlags.java ⚙️ Configuration changes +11/-0

Add docker-stop-grace-period command-line flag

• Added new @Parameter and @ConfigValue annotations for --docker-stop-grace-period flag
• Allows operators to configure grace period via command-line or TOML configuration
• Includes description and example value referencing DEFAULT_STOP_GRACE_PERIOD

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

---

2. java/src/org/openqa/selenium/grid/node/docker/DockerOptions.java ⚙️ Configuration changes +9/-1

Add stop grace period configuration retrieval

• Added DEFAULT_STOP_GRACE_PERIOD constant set to 60 seconds
• Implemented getStopGracePeriod() method to read configuration value with fallback default
• Updated getDockerSessionFactories() to retrieve and pass stop grace period to session factory

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

---

3. java/src/org/openqa/selenium/grid/node/docker/DockerSession.java 🐞 Bug fix +16/-6

Implement configurable timeouts and guaranteed cleanup

• Added containerStopTimeout and videoContainerStopTimeout fields to store configurable timeouts
• Updated constructor to accept and validate both timeout duration parameters
• Refactored stop() method with try-finally block to guarantee cleanup execution
• Replaced hardcoded Duration.ofSeconds(10) and Duration.ofMinutes(1) with configurable values

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

---

View more (3)

4. java/src/org/openqa/selenium/grid/node/docker/DockerSessionFactory.java ✨ Enhancement +7/-2

Pass stop grace period to session instances

• Added stopGracePeriod field to store the configured grace period
• Updated constructor to accept stopGracePeriod parameter with validation
• Modified session creation to pass stopGracePeriod to both container and video container timeouts

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

---

5. java/test/org/openqa/selenium/grid/node/docker/DockerSessionTest.java 🧪 Tests +189/-0

Add DockerSession unit tests for stop behavior

• Created comprehensive test suite with 6 test cases covering stop behavior
• Tests verify correct order of operations (video stop → logs → browser stop)
• Tests ensure browser container cleanup even when video stop or log fetch fails
• Tests validate that configured grace period is passed to both containers

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

---

6. java/test/org/openqa/selenium/grid/node/docker/BUILD.bazel ⚙️ Configuration changes +2/-0

Update test dependencies

• Added dependencies on //java/src/org/openqa/selenium:core and
 //java/src/org/openqa/selenium/remote
• Dependencies required for new test suite to compile and run

java/test/org/openqa/selenium/grid/node/docker/BUILD.bazel

---

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (1) 📎 Requirement gaps (0)

1. DockerSessionFactory constructor signature changed 📘 Rule violation ✓ Correctness

Description

The public DockerSessionFactory constructor adds a new stopGracePeriod parameter without
preserving the previous signature, which is an incompatible API change for any downstream code
instantiating it. This can break compilation for users/extensions relying on the old constructor.

Code

java/src/org/openqa/selenium/grid/node/docker/DockerSessionFactory.java[R125-129]

     Predicate<Capabilities> predicate,
     Map<String, Object> hostConfig,
     List<String> hostConfigKeys,
-      Map<String, String> groupingLabels) {
+      Map<String, String> groupingLabels,
+      Duration stopGracePeriod) {

Evidence

PR Compliance ID 3 requires maintaining backward compatibility for public APIs. The diff shows the
public DockerSessionFactory(...) constructor signature was modified to require an additional
Duration stopGracePeriod argument, with no backward-compatible overload shown in the PR diff.

AGENTS.md
java/src/org/openqa/selenium/grid/node/docker/DockerSessionFactory.java[125-129]

Agent prompt

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

## Issue description
A new required parameter (`stopGracePeriod`) was added to the public `DockerSessionFactory` constructor, which is a source-incompatible API change for external callers.
## Issue Context
Even if this class is primarily used internally, it is `public` and could be constructed by downstream code. The compliance requirement calls for maintaining public API compatibility or providing a backward-compatible path.
## Fix Focus Areas
- java/src/org/openqa/selenium/grid/node/docker/DockerSessionFactory.java[125-129] իսկ

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

---

2. Negative grace period accepted ☑ 🐞 Bug ⛯ Reliability

Description

DockerOptions.getStopGracePeriod() converts the configured integer directly into a Duration without
rejecting negative values, so a config like stop-grace-period = -1 becomes
Duration.ofSeconds(-1). That negative duration is propagated into container stop calls and is
serialized into the Docker stop request as a negative t query parameter, which violates the
intended semantics of a grace period and can lead to incorrect stop behavior.

Code

java/src/org/openqa/selenium/grid/node/docker/DockerOptions.java[R120-123]

+  private Duration getStopGracePeriod() {
+    return Duration.ofSeconds(
+        config.getInt(DOCKER_SECTION, "stop-grace-period").orElse(DEFAULT_STOP_GRACE_PERIOD));
+  }

Evidence

The new stop grace period is read via Config.getInt, whose default implementation is just
Integer.parseInt (so negative values are accepted), and then wrapped with Duration.ofSeconds
with no range checks. That Duration is later used for container stopping, and the Docker client
builds the stop request’s timeout query parameter directly from the Duration (milliseconds/1000), so
a negative Duration produces a negative t query parameter.

java/src/org/openqa/selenium/grid/node/docker/DockerOptions.java[115-123]
java/src/org/openqa/selenium/grid/config/Config.java[38-44]
java/src/org/openqa/selenium/grid/node/docker/DockerSession.java[80-90]
java/src/org/openqa/selenium/docker/client/StopContainer.java[38-51]

Agent prompt

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

## Issue description
`stop-grace-period` is a newly user-configurable integer that is converted directly into `Duration.ofSeconds(...)` without validating that it is non-negative. Because the Docker stop request serializes the duration into a `t` query parameter, negative configuration values can produce a negative timeout in the Docker stop HTTP call.
### Issue Context
- `Config.getInt` accepts negative integers (it uses `Integer.parseInt`).
- `DockerOptions.getStopGracePeriod()` currently performs no range validation.
### Fix Focus Areas
- java/src/org/openqa/selenium/grid/node/docker/DockerOptions.java[115-123]
- java/src/org/openqa/selenium/grid/config/Config.java[38-44]
- java/src/org/openqa/selenium/docker/client/StopContainer.java[38-51]
### Suggested change
In `DockerOptions.getStopGracePeriod()`, read the int into a local variable and enforce `>= 0` (and potentially a sane max). If invalid, throw `ConfigException` with a clear message (or clamp to 0 if you prefer permissive behavior). Add/adjust unit tests to cover the negative-value case if there is an existing config-parsing test harness for DockerOptions.

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

---

ⓘ The new review experience is currently in Beta. Learn more