[java] Fix saving docker logs to a file

[asolntsev]

🔗 Related Issues

Fixes #17209

💥 What does this PR do?

Fixes the way how docker response "/logs" is parsed and saved to a file.

🔄 Types of changes

• Bug fix (backwards compatible)

Result

This is how log lines in the resulting file "selenium-server.log" look like:

• Before: �??????g2026-03-13 09:30:55,398 INFO Included extra file
• After: 2026-03-13 09:54:06,477 INFO Included extra file

[qodo-code-review]

Review Summary by Qodo

Fix Docker logs parsing and storage in Grid

🐞 Bug fix

Walkthroughs

Description

• Fix Docker logs parsing to correctly handle multiplexed stream format
• Store logs as stream instead of loading entire content into memory
• Remove unnecessary HTTP Content-Type header from GET request
• Prevent duplicate log file writes when container stops twice
• Fix NPE when TZ environment variable is missing

File Changes

1. java/src/org/openqa/selenium/docker/Container.java ✨ Enhancement +6/-2

Add container state check and refactor empty logs

• Replace ArrayList with Contents.empty() for empty logs
• Add isRunning() method to check container state
• Remove unused ArrayList import

java/src/org/openqa/selenium/docker/Container.java

---

2. java/src/org/openqa/selenium/docker/ContainerLogs.java ✨ Enhancement +26/-5

Refactor to stream-based log storage

• Change internal storage from List<String> to Contents.Supplier for memory efficiency
• Deprecate getLogLines() method with explanation
• Add new getLogs() method returning InputStream
• Update toString() to show size instead of full log content

java/src/org/openqa/selenium/docker/ContainerLogs.java

---

3. java/src/org/openqa/selenium/docker/client/GetContainerLogs.java 🐞 Bug fix +4/-7

Remove Content-Type header and simplify response handling

• Remove unnecessary Content-Type header from GET request
• Pass response content directly instead of parsing to List
• Improve logging with lambda expression
• Simplify ContainerLogs instantiation

java/src/org/openqa/selenium/docker/client/GetContainerLogs.java

---

View more (3)

4. java/src/org/openqa/selenium/grid/node/docker/DockerSession.java 🐞 Bug fix +45/-8

Implement multiplexed stream parsing for Docker logs

• Add check to skip log saving if container is not running
• Implement parseMultiplexedStream() to correctly parse Docker stream format
• Replace file writing with stream-based approach using DataInputStream
• Handle multiplexed stream protocol (skip stream type, padding, read payload size)

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

---

5. java/src/org/openqa/selenium/grid/node/docker/DockerSessionFactory.java 🐞 Bug fix +1/-1

Fix NPE when TZ environment variable missing

• Add null check for TZ environment variable before using it
• Prevent NPE when TZ is not set

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

---

6. java/src/org/openqa/selenium/remote/http/jdk/JdkHttpMessages.java ✨ Enhancement +7/-1

Add Docker stream format detection for binary handling

• Extract binary stream detection logic into isBinaryStream() method
• Add support for Docker multiplexed and raw stream content types
• Treat Docker stream formats as binary to avoid text parsing

java/src/org/openqa/selenium/remote/http/jdk/JdkHttpMessages.java

---

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (3) 📎 Requirement gaps (0)

1. ContainerLogs constructor signature changed 📘 Rule violation ✓ Correctness

Description

The public ContainerLogs constructor now requires Contents.Supplier instead of List, which is
a breaking API/ABI change for any external callers. This can prevent users from upgrading without
code changes.

Code

java/src/org/openqa/selenium/docker/ContainerLogs.java[R36-39]

+  public ContainerLogs(ContainerId id, Contents.Supplier contents) {
+    this.contents = Require.nonNull("Container logs", contents);
  this.id = Require.nonNull("Container id", id);
}

Evidence

PR Compliance ID 1 requires preserving public API/ABI compatibility; the PR changes the public
constructor parameter types, removing the previous signature and breaking downstream
compilation/binary compatibility.

AGENTS.md
java/src/org/openqa/selenium/docker/ContainerLogs.java[36-39]

Agent prompt

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

## Issue description
`ContainerLogs` changed its public constructor signature from a `List<String>`-based API to a `Contents.Supplier`-based API, which is a breaking API/ABI change.
## Issue Context
This PR introduces streaming log handling, but existing callers may instantiate `ContainerLogs` using the old constructor.
## Fix Focus Areas
- java/src/org/openqa/selenium/docker/ContainerLogs.java[33-52]
- java/src/org/openqa/selenium/docker/client/GetContainerLogs.java[45-54]
- java/src/org/openqa/selenium/docker/Container.java[72-80]

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

---

2. getLogLines stream crash ☑ 🐞 Bug ✓ Correctness

Description

ContainerLogs.getLogLines() calls contents.reader(UTF_8), but stream-backed Contents suppliers
(created via Contents.fromStream) throw UnsupportedOperationException from reader(), causing log
retrieval to crash at runtime.

Code

java/src/org/openqa/selenium/docker/ContainerLogs.java[R46-51]

public List<String> getLogLines() {
-    return logLines;
+    try (BufferedReader in = new BufferedReader(contents.reader(UTF_8))) {
+      return in.lines().collect(Collectors.toList());
+    } catch (IOException e) {
+      throw new UncheckedIOException(e);
+    }

Evidence

ContainerLogs.getLogLines relies on Contents.Supplier.reader, but InputStreamContentSupplier (the
implementation returned by Contents.fromStream) explicitly does not support reader() and throws
UnsupportedOperationException. The Docker JDK HTTP layer will construct stream-backed content
suppliers for Docker binary content-types, and GetContainerLogs now passes that supplier directly
into ContainerLogs, making this crash reachable.

java/src/org/openqa/selenium/docker/ContainerLogs.java[41-52]
java/src/org/openqa/selenium/remote/http/InputStreamContentSupplier.java[58-66]
java/src/org/openqa/selenium/remote/http/jdk/JdkHttpMessages.java[179-200]
java/src/org/openqa/selenium/docker/client/GetContainerLogs.java[42-54]

Agent prompt

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

## Issue description
`ContainerLogs.getLogLines()` currently calls `contents.reader(UTF_8)`, which throws `UnsupportedOperationException` for stream-backed suppliers (`Contents.fromStream(...)`). This makes the (still public) API fail at runtime.
### Issue Context
`JdkHttpMessages` will construct stream-backed content suppliers for docker log stream content-types, and `GetContainerLogs` passes that supplier into `ContainerLogs`.
### Fix Focus Areas
- java/src/org/openqa/selenium/docker/ContainerLogs.java[41-52]
- java/src/org/openqa/selenium/remote/http/InputStreamContentSupplier.java[58-66]
- java/src/org/openqa/selenium/remote/http/jdk/JdkHttpMessages.java[179-200]
- java/src/org/openqa/selenium/docker/client/GetContainerLogs.java[42-54]
### Suggested change
- Update `getLogLines()` to avoid `contents.reader(...)` for stream-backed/binary suppliers. Options:
- Fallback to `byte[] bytes = Contents.bytes(contents)` and decode/split (acceptable since the method is deprecated).
- Preferably: if the bytes are in docker multiplexed format, demultiplex first (reuse/duplicate the demultiplex logic in a shared helper) before splitting into lines.
- Ensure any consumed stream is properly closed (try-with-resources around `contents.get()` or rely on `Contents.bytes(...)` consuming/closing the stream).

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

---

3. No tests for log parsing 📘 Rule violation ⛯ Reliability

Description

The PR introduces new multiplexed-stream parsing logic for saving Docker logs but does not
add/update tests to cover it. This increases regression risk for log parsing and file output
behavior.

Code

java/src/org/openqa/selenium/grid/node/docker/DockerSession.java[R83-119]

private void saveLogs() {
+    if (!container.isRunning()) {
+      LOG.log(
+          FINE, () -> "Skip saving logs because container is not running: " + container.getId());
+      return;
+    }
+
  String sessionAssetsPath = assetsPath.getContainerPath(getId());
-    String seleniumServerLog = String.format("%s/selenium-server.log", sessionAssetsPath);
-    try {
-      List<String> logs = container.getLogs().getLogLines();
-      Files.write(Paths.get(seleniumServerLog), logs);
-    } catch (Exception e) {
+    File seleniumServerLog = new File(sessionAssetsPath, "selenium-server.log");
+    ContainerLogs containerLogs = container.getLogs();
+
+    try (OutputStream out = new BufferedOutputStream(new FileOutputStream(seleniumServerLog))) {
+      parseMultiplexedStream(containerLogs.getLogs(), out);
+      LOG.log(
+          FINE,
+          () ->
+              String.format(
+                  "Saved container %s logs to file %s", container.getId(), seleniumServerLog));
+    } catch (IOException e) {
    LOG.log(Level.WARNING, "Error saving logs", e);
  }
}
+
+  @SuppressWarnings("InfiniteLoopStatement")
+  private void parseMultiplexedStream(InputStream stream, OutputStream out) throws IOException {
+    try (DataInputStream in = new DataInputStream(new BufferedInputStream(stream))) {
+      while (true) {
+        in.skipBytes(1); // Skip "stream type" byte (1 = stdout, 2 = stderr)
+        in.skipBytes(3); // Skip the 3 empty padding bytes
+        int payloadSize = in.readInt(); // Read the 4-byte payload size
+        byte[] payload = new byte[payloadSize];
+        in.readFully(payload);
+        out.write(payload);
+      }
+    } catch (EOFException done) {
+      LOG.log(FINE, () -> "Finished reading multiplexed stream: " + done);
+    }

Evidence

PR Compliance ID 5 requires adding/updating tests when behavior changes; the PR adds new binary log
parsing logic in parseMultiplexedStream and changes how logs are written to disk, which should be
validated by unit tests.

AGENTS.md
java/src/org/openqa/selenium/grid/node/docker/DockerSession.java[83-119]

Agent prompt

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

## Issue description
New Docker log parsing/writing behavior is not covered by tests.
## Issue Context
`parseMultiplexedStream` decodes Docker's multiplexed stream framing and `saveLogs()` writes the decoded payload to `selenium-server.log`. This is easy to regress without a focused unit test.
## Fix Focus Areas
- java/src/org/openqa/selenium/grid/node/docker/DockerSession.java[83-119]

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

---

4. Stream header comments are 'what' 📘 Rule violation ✓ Correctness

Description

Inline comments in parseMultiplexedStream restate what each line does rather than explaining
rationale or protocol details. This reduces maintainability when the code changes or the protocol
framing needs adjustment.

Code

java/src/org/openqa/selenium/grid/node/docker/DockerSession.java[R110-113]

+        in.skipBytes(1); // Skip "stream type" byte (1 = stdout, 2 = stderr)
+        in.skipBytes(3); // Skip the 3 empty padding bytes
+        int payloadSize = in.readInt(); // Read the 4-byte payload size
+        byte[] payload = new byte[payloadSize];

Evidence

PR Compliance ID 7 requires comments to explain rationale/intent rather than obvious behavior; the
new comments merely narrate the code operations instead of referencing the Docker multiplexed stream
framing/protocol expectations.

AGENTS.md
java/src/org/openqa/selenium/grid/node/docker/DockerSession.java[110-113]

Agent prompt

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

## Issue description
Comments in `parseMultiplexedStream` currently describe obvious operations instead of explaining intent/protocol rationale.
## Issue Context
This method implements Docker's multiplexed log stream framing; future maintainers benefit more from a short explanation of the framing format (and why bytes are skipped) than step-by-step narration.
## Fix Focus Areas
- java/src/org/openqa/selenium/grid/node/docker/DockerSession.java[106-116]

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

---

5. Unbounded frame allocation ☑ 🐞 Bug ⛯ Reliability

Description

DockerSession.parseMultiplexedStream() allocates byte[payloadSize] from an unvalidated size read
from the stream; malformed/unexpected bytes can trigger unchecked exceptions
(NegativeArraySizeException/OutOfMemoryError) that are not caught by saveLogs(), potentially
interrupting session stop and preventing container shutdown.

Code

java/src/org/openqa/selenium/grid/node/docker/DockerSession.java[R107-115]

+  private void parseMultiplexedStream(InputStream stream, OutputStream out) throws IOException {
+    try (DataInputStream in = new DataInputStream(new BufferedInputStream(stream))) {
+      while (true) {
+        in.skipBytes(1); // Skip "stream type" byte (1 = stdout, 2 = stderr)
+        in.skipBytes(3); // Skip the 3 empty padding bytes
+        int payloadSize = in.readInt(); // Read the 4-byte payload size
+        byte[] payload = new byte[payloadSize];
+        in.readFully(payload);
+        out.write(payload);

Evidence

The parser reads a signed int as payloadSize and allocates an array of that size without bounds
checks. saveLogs() only catches IOException, so unchecked failures from allocation can escape and
break the stop() flow.

java/src/org/openqa/selenium/grid/node/docker/DockerSession.java[94-103]
java/src/org/openqa/selenium/grid/node/docker/DockerSession.java[106-116]

Agent prompt

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

## Issue description
`parseMultiplexedStream` reads a frame length and allocates a byte array of that size without validation. If the stream is malformed/unexpected, this can throw unchecked exceptions (e.g., `NegativeArraySizeException` or `OutOfMemoryError`) which are not caught by `saveLogs()` (it only catches `IOException`), potentially interrupting the `stop()` sequence.
### Issue Context
`saveLogs()` calls `parseMultiplexedStream(...)` during `DockerSession.stop()`. Any unchecked exception escaping `saveLogs()` can prevent subsequent shutdown steps.
### Fix Focus Areas
- java/src/org/openqa/selenium/grid/node/docker/DockerSession.java[83-104]
- java/src/org/openqa/selenium/grid/node/docker/DockerSession.java[106-119]
### Suggested change
- In `parseMultiplexedStream`:
- Read the 8-byte header with `readFully` and validate padding/stream-type.
- Treat the 4-byte size as unsigned (or at least reject negative values).
- Enforce a reasonable maximum payload size (defensive cap) before reading.
- Avoid allocating `byte[payloadSize]`; instead, stream-copy `payloadSize` bytes to `out` using a fixed buffer (e.g., 8–64KB) until the frame is fully written.
- In `saveLogs()`:
- Consider catching `RuntimeException` from parsing to ensure session shutdown continues even if log parsing fails (while still logging the error).

ⓘ 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