.pr_agent_auto_best_practices

Pattern 1: Always wrap creation of external contexts or resources (e.g., user contexts, tabs, BrowsingContext, drivers) in try/finally and perform explicit cleanup in finally to prevent leaks even when assertions or exceptions occur.

Example code before:

ctx = create_context()
do_work(ctx)
assert check(ctx)
# no cleanup if assertion fails

Example code after:

ctx = create_context()
try:
    do_work(ctx)
    assert check(ctx)
finally:
    close_context(ctx)

Relevant past accepted suggestions:

---

Suggestion 1:

Cleanup created user contexts

Ensure created user contexts are cleaned up after the test by deleting them in a finally block to prevent resource leaks.

java/test/org/openqa/selenium/bidi/emulation/SetScriptingEnabledTest.java [82-113]

 String userContext = browser.createUserContext();
-BrowsingContext context =
-    new BrowsingContext(
-        driver, new CreateContextParameters(WindowType.TAB).userContext(userContext));
-String contextId = context.getId();
-...
-// Clear the scripting override
-emulation.setScriptingEnabled(
-    new SetScriptingEnabledParameters(null).userContexts(List.of(userContext)));
+try {
+  BrowsingContext context =
+      new BrowsingContext(
+          driver, new CreateContextParameters(WindowType.TAB).userContext(userContext));
+  String contextId = context.getId();
+  ...
+  emulation.setScriptingEnabled(
+      new SetScriptingEnabledParameters(null).userContexts(List.of(userContext)));
+} finally {
+  browser.deleteUserContext(userContext);
+}

---

Suggestion 2:

Close context in finally

Wrap context creation/usage in try-finally and close the BrowsingContext in finally to avoid leaks if assertions throw.

java/test/org/openqa/selenium/bidi/emulation/SetGeolocationOverrideTest.java [201-235]

 BrowsingContext context = new BrowsingContext(driver, driver.getWindowHandle());
-String contextId = context.getId();
-...
-// Error because there's no real geolocation available
-assertThat(r2.containsKey("error")).isTrue();
+try {
+  String contextId = context.getId();
+  String url = appServer.whereIsSecure("blank.html");
+  context.navigate(url, ReadinessState.COMPLETE);
+  driver.switchTo().window(context.getId());
+  String origin =
+      (String) ((JavascriptExecutor) driver).executeScript("return window.location.origin;");
+  Emulation emul = new Emulation(driver);
+  GeolocationCoordinates coords = new GeolocationCoordinates(37.7749, -122.4194);
+  emul.setGeolocationOverride(
+      new SetGeolocationOverrideParameters(coords).contexts(List.of(contextId)));
+  Object firstResult = getBrowserGeolocation(driver, null, origin);
+  Map<String, Object> r1 = ((Map<String, Object>) firstResult);
+  assertThat(r1.containsKey("error")).isFalse();
+  double latitude1 = ((Number) r1.get("latitude")).doubleValue();
+  double longitude1 = ((Number) r1.get("longitude")).doubleValue();
+  assertThat(abs(latitude1 - coords.getLatitude())).isLessThan(0.0001);
+  assertThat(abs(longitude1 - coords.getLongitude())).isLessThan(0.0001);
+  emul.setGeolocationOverride(
+      new SetGeolocationOverrideParameters((GeolocationCoordinates) null)
+          .contexts(List.of(contextId)));
+  Object secondResult = getBrowserGeolocation(driver, null, origin);
+  Map<String, Object> r2 = ((Map<String, Object>) secondResult);
+  assertThat(r2.containsKey("error")).isTrue();
+} finally {
+  context.close();
+}

---

Suggestion 3:

Add robust test cleanup

Ensure the tab and user context are cleaned up even if assertions fail by wrapping them in try/finally blocks.

py/test/selenium/webdriver/common/bidi_emulation_tests.py [347-363]

 def test_set_locale_override_with_user_contexts(driver, pages, value):
     """Test setting locale override with user contexts."""
     user_context = driver.browser.create_user_context()
+    try:
+        context_id = driver.browsing_context.create(type=WindowTypes.TAB, user_context=user_context)
+        try:
+            driver.switch_to.window(context_id)
+            driver.emulation.set_locale_override(locale=value, user_contexts=[user_context])
+            driver.browsing_context.navigate(context_id, pages.url("formPage.html"), wait="complete")
+            current_locale = get_browser_locale(driver)
+            assert current_locale == value, f"Expected locale {value}, got {current_locale}"
+        finally:
+            driver.browsing_context.close(context_id)
+    finally:
+        driver.browser.remove_user_context(user_context)
 
-    context_id = driver.browsing_context.create(type=WindowTypes.TAB, user_context=user_context)
-
-    driver.switch_to.window(context_id)
-
-    driver.emulation.set_locale_override(locale=value, user_contexts=[user_context])
-
-    driver.browsing_context.navigate(context_id, pages.url("formPage.html"), wait="complete")
-
-    current_locale = get_browser_locale(driver)
-    assert current_locale == value, f"Expected locale {value}, got {current_locale}"
-
-    driver.browsing_context.close(context_id)
-    driver.browser.remove_user_context(user_context)
-

---

Suggestion 4:

Properly stop driver before nullifying

Setting the global variable to None without properly stopping the driver can lead to resource leaks. The driver should be stopped before nullifying the reference to ensure proper cleanup.

py/conftest.py [344-345]

 if request.node.get_closest_marker("no_driver_after_test"):
+    if selenium_driver is not None:
+        selenium_driver.stop_driver()
     selenium_driver = None

---

Suggestion 5:

Close HTTP response via context manager

Ensure the HTTP response object is properly closed by using a context manager. This prevents potential resource leaks under heavy usage or exceptions.

py/selenium/webdriver/common/utils.py [150-151]

-res = urllib.request.urlopen(f"{scheme}://{host}:{port}/status")
-return res.getcode() == 200
+with urllib.request.urlopen(f"{scheme}://{host}:{port}/status") as res:
+    return res.getcode() == 200

---

Pattern 2: Use defensive copying and immutability for maps returned or stored by constructors or accessors to avoid external mutation and unintended side effects.

Example code before:

class Config:
    def __init__(self, params: dict):
        self.params = params  # shares external reference
    def to_map(self):
        return self.params     # exposes internal state

Example code after:

class Config:
    def __init__(self, params: dict):
        self._params = dict(params)
    def to_map(self):
        return dict(self._params)

Relevant past accepted suggestions:

---

Suggestion 1:

Avoid mutating a constructor parameter

To avoid mutating a constructor parameter and prevent potential runtime exceptions, create a new mutable copy of the composeLabels map before adding the new label.

java/src/org/openqa/selenium/grid/node/docker/DockerSessionFactory.java [144-146]

-this.composeLabels = Require.nonNull("Docker Compose labels", composeLabels);
 // Merge compose labels with oneoff=False to prevent triggering --exit-code-from dynamic grid
-this.composeLabels.put("com.docker.compose.oneoff", "False");
+Map<String, String> allLabels = new HashMap<>(Require.nonNull("Docker Compose labels", composeLabels));
+allLabels.put("com.docker.compose.oneoff", "False");
+this.composeLabels = Collections.unmodifiableMap(allLabels);

---

Suggestion 2:

Avoid modifying nested map directly

To avoid side effects, create a defensive copy of the networkSettings map in adaptContainerInspectResponse before modifying it, ensuring the original response data structure remains unchanged.

java/src/org/openqa/selenium/docker/client/V148Adapter.java [165-205]

 @SuppressWarnings("unchecked")
-Map<String, Object> networkSettings = (Map<String, Object>) adapted.get("NetworkSettings");
+Map<String, Object> originalNetworkSettings =
+    (Map<String, Object>) adapted.get("NetworkSettings");
 
-if (networkSettings != null) {
+if (originalNetworkSettings != null) {
+  // Create a mutable copy to avoid modifying the original map
+  Map<String, Object> networkSettings = new HashMap<>(originalNetworkSettings);
+
   @SuppressWarnings("unchecked")
   Map<String, Object> networks = (Map<String, Object>) networkSettings.get("Networks");
 
   if (networks != null) {
     for (Map.Entry<String, Object> entry : networks.entrySet()) {
       if (entry.getValue() instanceof Map) {
         @SuppressWarnings("unchecked")
         Map<String, Object> network = (Map<String, Object>) entry.getValue();
 
         if (network.containsKey("GwPriority")) {
           LOG.fine(
               "Network '" + entry.getKey() + "' includes GwPriority (API v" + apiVersion + ")");
         }
       }
     }
   }
 
   // Remove deprecated fields (should not be present in v1.48+)
   String[] deprecatedFields = {
     "HairpinMode",
     "LinkLocalIPv6Address",
     "LinkLocalIPv6PrefixLen",
     "SecondaryIPAddresses",
     "SecondaryIPv6Addresses",
     "Bridge" // Deprecated in v1.51, removed in v1.52
   };
 
   for (String field : deprecatedFields) {
     if (networkSettings.containsKey(field)) {
       LOG.fine(
           "Removing deprecated field '"
               + field
               + "' from NetworkSettings (deprecated in earlier API versions)");
       networkSettings.remove(field);
     }
   }
+  adapted.put("NetworkSettings", networkSettings);
 }

---

Suggestion 3:

Return a defensive copy of map

Return a defensive copy of the internal map in the toMap() method to prevent external modification and maintain encapsulation.

java/src/org/openqa/selenium/bidi/emulation/AbstractOverrideParameters.java [57]

-return map;
+return new HashMap<>(map);

---

Suggestion 4:

Create a defensive copy of parameters

To ensure the Command object is immutable, create a defensive, unmodifiable copy of the params map that allows null values.

java/src/org/openqa/selenium/bidi/Command.java [51]

-this.params = Require.nonNull("Command parameters", params);
+this.params = java.util.Collections.unmodifiableMap(new java.util.HashMap<>(Require.nonNull("Command parameters", params)));

---

Pattern 3: Validate and sanitize external inputs (URLs, names, headers) and encode where needed to conform to APIs and avoid injection or invalid requests.

Example code before:

if name:
    url = f"/create?name={name}"
res = client.get(url)
parse_json(res.body)  # assumes JSON and 200

Example code after:

if name and name.strip():
    enc = urllib.parse.quote(name.strip(), safe="")
    url = f"/create?name={enc}"
res = client.get(url, timeout=5)
if res.status == 200 and "application/json" in res.headers.get("Content-Type",""):
    parse_json(res.body)

Relevant past accepted suggestions:

---

Suggestion 1:

Sanitize browser name for container

Sanitize the browser name by replacing characters invalid for Docker container names to prevent potential creation failures.

java/src/org/openqa/selenium/grid/node/docker/DockerSessionFactory.java [314-316]

-// Generate container name: browser-<browserName>-<timestamp>-<uuid>
-String browserName = stereotype.getBrowserName().toLowerCase();
+// Generate container name: browser-<browserName>-<sessionIdentifier>
+String browserName = stereotype.getBrowserName().toLowerCase().replaceAll("[^a-z0-9_.-]", "-");
 String containerName = String.format("browser-%s-%s", browserName, sessionIdentifier);

---

Suggestion 2:

Validate and encode URL parameter

Validate and URL-encode the optional container name to avoid invalid requests and injection issues. Reject empty/whitespace-only names and percent-encode the value.

java/src/org/openqa/selenium/docker/client/CreateContainer.java [66-69]

 String url = String.format("/v%s/containers/create", apiVersion);
-if (info.getName() != null && !info.getName().isEmpty()) {
-  url += "?name=" + info.getName();
+String name = info.getName();
+if (name != null) {
+  name = name.trim();
+  if (!name.isEmpty()) {
+    String encoded = java.net.URLEncoder.encode(name, java.nio.charset.StandardCharsets.UTF_8);
+    url += "?name=" + encoded;
+  }
 }

---

Suggestion 3:

Close parser and validate response

Ensure the JsonInput is always closed to avoid resource leaks. Additionally, verify the HTTP response status and content type before parsing to prevent parsing errors on non-200 responses or non-JSON bodies.

java/src/org/openqa/selenium/grid/router/HandleSession.java [255-296]

 private ClientConfig fetchNodeSessionTimeout(URI uri) {
   ClientConfig config = ClientConfig.defaultConfig().baseUri(uri).withRetries();
   Duration sessionTimeout = config.readTimeout();
   try (HttpClient httpClient = httpClientFactory.createClient(config)) {
     HttpRequest statusRequest = new HttpRequest(GET, "/status");
     HttpResponse res = httpClient.execute(statusRequest);
-    Reader reader = reader(res);
-    Json JSON = new Json();
-    JsonInput in = JSON.newInput(reader);
-    in.beginObject();
-    // Skip everything until we find "value"
-    while (in.hasNext()) {
-      if ("value".equals(in.nextName())) {
+    if (res.getStatus() == 200 && res.getHeader("Content-Type") != null && res.getHeader("Content-Type").contains("application/json")) {
+      try (Reader rdr = reader(res); JsonInput in = new Json().newInput(rdr)) {
         in.beginObject();
         while (in.hasNext()) {
-          if ("node".equals(in.nextName())) {
-            NodeStatus nodeStatus = in.read(NodeStatus.class);
-            sessionTimeout = nodeStatus.getSessionTimeout();
-            LOG.fine(
-                "Fetched session timeout from node status (read timeout: "
-                    + sessionTimeout.toSeconds()
-                    + " seconds) for "
-                    + uri);
+          String name = in.nextName();
+          if ("value".equals(name)) {
+            in.beginObject();
+            while (in.hasNext()) {
+              String inner = in.nextName();
+              if ("node".equals(inner)) {
+                NodeStatus nodeStatus = in.read(NodeStatus.class);
+                sessionTimeout = nodeStatus.getSessionTimeout();
+                LOG.fine("Fetched session timeout from node status (read timeout: "
+                    + sessionTimeout.toSeconds() + " seconds) for " + uri);
+              } else {
+                in.skipValue();
+              }
+            }
+            in.endObject();
           } else {
             in.skipValue();
           }
         }
-        in.endObject();
-      } else {
-        in.skipValue();
       }
+    } else {
+      LOG.fine("Non-OK or non-JSON status response from " + uri + " for /status, using default read timeout.");
     }
   } catch (Exception e) {
-    LOG.fine(
-        "Use default from ClientConfig (read timeout: "
-            + config.readTimeout().toSeconds()
-            + " seconds) for "
-            + uri);
+    LOG.fine("Use default from ClientConfig (read timeout: "
+        + config.readTimeout().toSeconds() + " seconds) for " + uri);
   }
-  config = config.readTimeout(sessionTimeout);
-  return config;
+  return config.readTimeout(sessionTimeout);
 }

---

Suggestion 4:

Bound status probe timeout

Add a short read timeout specifically for the /status probe to avoid hanging on an unresponsive node and blocking session creation. Use a bounded timeout (e.g., a few seconds) independent from the main client timeout.

java/src/org/openqa/selenium/grid/router/HandleSession.java [258-293]

-try (HttpClient httpClient = httpClientFactory.createClient(config)) {
+try (HttpClient httpClient = httpClientFactory.createClient(
+    ClientConfig.defaultConfig()
+        .baseUri(uri)
+        .readTimeout(Duration.ofSeconds(5))
+        .withRetries())) {
   HttpRequest statusRequest = new HttpRequest(GET, "/status");
   HttpResponse res = httpClient.execute(statusRequest);
-  Reader reader = reader(res);
-  Json JSON = new Json();
-  JsonInput in = JSON.newInput(reader);
-  in.beginObject();
-  // Skip everything until we find "value"
-  while (in.hasNext()) {
-    if ("value".equals(in.nextName())) {
+  if (res.getStatus() == 200) {
+    try (Reader rdr = reader(res); JsonInput in = new Json().newInput(rdr)) {
       in.beginObject();
       while (in.hasNext()) {
-        if ("node".equals(in.nextName())) {
-          NodeStatus nodeStatus = in.read(NodeStatus.class);
-          sessionTimeout = nodeStatus.getSessionTimeout();
-          ...
+        String name = in.nextName();
+        if ("value".equals(name)) {
+          in.beginObject();
+          while (in.hasNext()) {
+            String inner = in.nextName();
+            if ("node".equals(inner)) {
+              NodeStatus nodeStatus = in.read(NodeStatus.class);
+              Duration parsed = nodeStatus.getSessionTimeout();
+              if (parsed != null && !parsed.isNegative() && !parsed.isZero()) {
+                sessionTimeout = parsed;
+              }
+            } else {
+              in.skipValue();
+            }
+          }
+          in.endObject();
         } else {
           in.skipValue();
         }
       }
-      in.endObject();
-    } else {
-      in.skipValue();
     }
   }
 } catch (Exception e) {
-  LOG.fine(
-      "Use default from ClientConfig (read timeout: "
-          + config.readTimeout().toSeconds()
-          + " seconds) for "
-          + uri);
+  LOG.fine("Use default from ClientConfig (read timeout: "
+      + config.readTimeout().toSeconds() + " seconds) for " + uri);
 }

---

Pattern 4: Replace ad-hoc duplication with shared helpers/utilities or private helper methods to reduce repetition and centralize logic (e.g., evaluating scripts, heartbeat configuration).

Example code before:

# repeated logic in multiple places
result = eval_in_ctx(ctx_id, "'foo' in window")
assert bool(result)
# elsewhere:
result = eval_in_ctx(ctx_id, "'foo' in window")
assert not bool(result)

Example code after:

def is_foo_in_window(ctx_id):
    return bool(eval_in_ctx(ctx_id, "'foo' in window"))
assert is_foo_in_window(ctx_id)
assert not is_foo_in_window(ctx_id)

Relevant past accepted suggestions:

---

Suggestion 1:

Extract duplicated code into a helper

Extract the duplicated script evaluation logic for checking if 'foo' in window into a private helper method to reduce code repetition and improve test readability.

java/test/org/openqa/selenium/bidi/emulation/SetScriptingEnabledTest.java [57-74]

-EvaluateResult resultDisabled =
-    script.evaluateFunctionInBrowsingContext(
-        contextId, "'foo' in window", false, Optional.empty());
-Boolean valueDisabled =
-    (Boolean) ((EvaluateResultSuccess) resultDisabled).getResult().getValue().get();
-assertThat(valueDisabled).isFalse();
+assertThat(isFooInWindow(contextId, script)).isFalse();
 
 emulation.setScriptingEnabled(
     new SetScriptingEnabledParameters(null).contexts(List.of(contextId)));
 
 context.navigate(appServer.whereIs("script_page.html"), ReadinessState.COMPLETE);
 
-EvaluateResult resultEnabled =
-    script.evaluateFunctionInBrowsingContext(
-        contextId, "'foo' in window", false, Optional.empty());
-Boolean valueEnabled =
-    (Boolean) ((EvaluateResultSuccess) resultEnabled).getResult().getValue().get();
-assertThat(valueEnabled).isTrue();
+assertThat(isFooInWindow(contextId, script)).isTrue();

---

Suggestion 2:

Refactor duplicated code into a utility method

Extract the duplicated configureHeartbeat method from BoundZmqEventBus and UnboundZmqEventBus into a static helper method in a new utility class to avoid code repetition and improve maintainability.

java/src/org/openqa/selenium/events/zeromq/BoundZmqEventBus.java [84-97]

-private void configureHeartbeat(ZMQ.Socket socket, Duration heartbeatPeriod, String socketType) {
-  if (heartbeatPeriod != null && !heartbeatPeriod.isZero() && !heartbeatPeriod.isNegative()) {
-    int heartbeatIvl = (int) heartbeatPeriod.toMillis();
-    socket.setHeartbeatIvl(heartbeatIvl);
-    // Set heartbeat timeout to 3x the interval
-    socket.setHeartbeatTimeout(heartbeatIvl * 3);
-    // Set heartbeat TTL to 6x the interval
-    socket.setHeartbeatTtl(heartbeatIvl * 6);
-    LOG.info(
-        String.format(
-            "Event bus %s socket heartbeat configured: interval=%dms, timeout=%dms, ttl=%dms",
-            socketType, heartbeatIvl, heartbeatIvl * 3, heartbeatIvl * 6));
+// In a new file: java/src/org/openqa/selenium/events/zeromq/ZmqUtils.java
+package org.openqa.selenium.events.zeromq;
+
+import java.time.Duration;
+import java.util.logging.Logger;
+import org.zeromq.ZMQ;
+
+class ZmqUtils {
+  private static final Logger LOG = Logger.getLogger(ZmqUtils.class.getName());
+
+  private ZmqUtils() {
+    // Utility class
+  }
+
+  static void configureHeartbeat(ZMQ.Socket socket, Duration heartbeatPeriod, String socketType) {
+    if (heartbeatPeriod != null && !heartbeatPeriod.isZero() && !heartbeatPeriod.isNegative()) {
+      long periodMillis = heartbeatPeriod.toMillis();
+      if (periodMillis > Integer.MAX_VALUE) {
+        LOG.warning(
+            String.format(
+                "Heartbeat period %dms is too large. Capping at %dms.",
+                periodMillis, Integer.MAX_VALUE));
+        periodMillis = Integer.MAX_VALUE;
+      }
+      int heartbeatIvl = (int) periodMillis;
+      socket.setHeartbeatIvl(heartbeatIvl);
+      // Set heartbeat timeout to 3x the interval
+      socket.setHeartbeatTimeout(heartbeatIvl * 3);
+      // Set heartbeat TTL to 6x the interval
+      socket.setHeartbeatTtl(heartbeatIvl * 6);
+      LOG.info(
+          String.format(
+              "Event bus %s socket heartbeat configured: interval=%dms, timeout=%dms, ttl=%dms",
+              socketType, heartbeatIvl, heartbeatIvl * 3, heartbeatIvl * 6));
+    }
   }
 }
 
+// In BoundZmqEventBus.java, replace the configureHeartbeat method with:
+private void configureHeartbeat(ZMQ.Socket socket, Duration heartbeatPeriod, String socketType) {
+  ZmqUtils.configureHeartbeat(socket, heartbeatPeriod, socketType);
+}
+

---

Suggestion 3:

Refactor to remove duplicated JSON parsing

Refactor the DownloadEnded.fromJson method to eliminate redundant JSON parsing logic by delegating deserialization to the DownloadCanceled.fromJson or DownloadCompleted.fromJson methods based on the status field.

java/src/org/openqa/selenium/bidi/browsingcontext/DownloadEnded.java [30-79]

 public static DownloadEnded fromJson(JsonInput input) {
-  String browsingContextId = null;
-  String navigationId = null;
-  long timestamp = 0;
-  String url = null;
-  String status = null;
-  String filepath = null;
+  Map<String, Object> jsonMap = input.read(Map.class);
+  String status = (String) jsonMap.get("status");
 
-  input.beginObject();
-  while (input.hasNext()) {
-    switch (input.nextName()) {
-      case "context":
-        browsingContextId = input.read(String.class);
-        break;
-      case "navigation":
-        navigationId = input.read(String.class);
-        break;
-      case "timestamp":
-        timestamp = input.read(Long.class);
-        break;
-      case "url":
-        url = input.read(String.class);
-        break;
-      case "status":
-        status = input.read(String.class);
-        break;
-      case "filepath":
-        filepath = input.read(String.class);
-        break;
-      default:
-        input.skipValue();
-        break;
+  try (StringReader reader = new StringReader(new Json().toJson(jsonMap));
+      JsonInput jsonInput = new Json().newInput(reader)) {
+    if ("canceled".equals(status)) {
+      return new DownloadEnded(DownloadCanceled.fromJson(jsonInput));
+    } else if ("complete".equals(status)) {
+      return new DownloadEnded(DownloadCompleted.fromJson(jsonInput));
+    } else {
+      throw new IllegalArgumentException(
+          "status must be either 'canceled' or 'complete', but got: " + status);
     }
-  }
-  input.endObject();
-
-  // Create the appropriate object based on status
-  if ("canceled".equals(status)) {
-    DownloadCanceled canceled =
-        new DownloadCanceled(browsingContextId, navigationId, timestamp, url, status);
-    return new DownloadEnded(canceled);
-  } else if ("complete".equals(status)) {
-    DownloadCompleted completed =
-        new DownloadCompleted(browsingContextId, navigationId, timestamp, url, status, filepath);
-    return new DownloadEnded(completed);
-  } else {
-    throw new IllegalArgumentException(
-        "status must be either 'canceled' or 'complete', but got: " + status);
   }
 }

---

Pattern 5: Guard numeric conversions and time calculations by clamping to safe bounds and avoiding overflow/precision errors; compute derived timeouts once and use clear units.

Example code before:

ivl = int(duration_ms)  # may overflow cast
socket.set_timeout(ivl * 3)
socket.set_ttl(ivl * 6)

Example code after:

ms = max(MIN_MS, min(duration_ms, MAX_MS))
ivl = int(ms)
timeout = ivl * 3
ttl = ivl * 6
socket.set_timeout(timeout)
socket.set_ttl(ttl)

Relevant past accepted suggestions:

---

Suggestion 1:

Prevent heartbeat interval overflow

** * Configures ZeroMQ heartbeat settings on a socket to prevent stale connections.

• • The heartbeat interval is clamped between 1 second and ~24 days to prevent integer overflow

• • and ensure reasonable values. If the provided duration is outside this range, it will be
• • adjusted and a warning will be logged.
  • @param socket The ZMQ socket to configure
  • @param heartbeatPeriod The heartbeat interval duration @@ -39,14 +46,58 @@ */ static void configureHeartbeat(ZMQ.Socket socket, Duration heartbeatPeriod, String socketType) { if (heartbeatPeriod != null && !heartbeatPeriod.isZero() && !heartbeatPeriod.isNegative()) {

•  int heartbeatIvl = (int) heartbeatPeriod.toMillis();
  

•  long heartbeatMs = heartbeatPeriod.toMillis();
  

•  long clampedHeartbeatMs = clampHeartbeatInterval(heartbeatMs, socketType);
  

•  // Safe to cast to int now
  

•  int heartbeatIvl = (int) clampedHeartbeatMs;
  

•  int heartbeatTimeout = heartbeatIvl * 3;
  

•  int heartbeatTtl = heartbeatIvl * 6;
  

•  socket.setHeartbeatIvl(heartbeatIvl);
  

•  socket.setHeartbeatTimeout(heartbeatIvl * 3);
  

•  socket.setHeartbeatTtl(heartbeatIvl * 6);
  

•  socket.setHeartbeatTimeout(heartbeatTimeout);
  

•  socket.setHeartbeatTtl(heartbeatTtl);
  

•  LOG.info(
       String.format(
  

•          "ZMQ %s socket heartbeat configured: interval=%dms, timeout=%dms, ttl=%dms",
  

•          socketType, heartbeatIvl, heartbeatIvl * 3, heartbeatIvl * 6));
  

•          "ZMQ %s socket heartbeat configured: interval=%ds, timeout=%ds, ttl=%ds",
  

•          socketType, heartbeatIvl / 1000, heartbeatTimeout / 1000, heartbeatTtl / 1000));
  

  } }
• /**
• • Clamps the heartbeat interval to safe bounds and logs warnings if adjustments are made.
• • @param heartbeatMs The heartbeat interval in milliseconds
• • @param socketType The socket type for logging
• • @return The clamped heartbeat interval
• */
• private static long clampHeartbeatInterval(long heartbeatMs, String socketType) {
• if (heartbeatMs < MIN_HEARTBEAT_MS) {

•  logHeartbeatClampWarning(socketType, heartbeatMs, MIN_HEARTBEAT_MS, "below minimum");
  

•  return MIN_HEARTBEAT_MS;
  

• }
• if (heartbeatMs > MAX_HEARTBEAT_MS) {

•  logHeartbeatClampWarning(socketType, heartbeatMs, MAX_HEARTBEAT_MS, "exceeds maximum");
  

•  return MAX_HEARTBEAT_MS;
  

• }
• return heartbeatMs;
• }
• /**
• • Logs a warning when the heartbeat interval is clamped.
• • @param socketType The socket type
• • @param originalMs The original interval value in milliseconds
• • @param clampedMs The clamped interval value in milliseconds
• • @param reason The reason for clamping
• */
• private static void logHeartbeatClampWarning(

•  String socketType, long originalMs, long clampedMs, String reason) {
  

• LOG.log(

•    Level.WARNING,
  

•    String.format(
  

•        "ZMQ %s socket heartbeat interval %ds %s %ds, clamping to %ds",
  

•        socketType, originalMs / 1000, reason, clampedMs / 1000, clampedMs / 1000));
  

• }

**In configureHeartbeat, guard against integer overflow by clamping the heartbeat interval in milliseconds to a safe maximum before casting to int and setting socket options. Also, enforce a sensible minimum value.**

[java/src/org/openqa/selenium/events/zeromq/ZmqUtils.java [40-52]](https://github.com/SeleniumHQ/selenium/pull/16444/files#diff-f9f1bd9c11ff8d0995d73085dc4b3e906a8aa15c1a5ba90fc6b9059a3a8080a1R40-R52)

```diff
 static void configureHeartbeat(ZMQ.Socket socket, Duration heartbeatPeriod, String socketType) {
-  if (heartbeatPeriod != null && !heartbeatPeriod.isZero() && !heartbeatPeriod.isNegative()) {
-    int heartbeatIvl = (int) heartbeatPeriod.toMillis();
-    socket.setHeartbeatIvl(heartbeatIvl);
-    socket.setHeartbeatTimeout(heartbeatIvl * 3);
-    socket.setHeartbeatTtl(heartbeatIvl * 6);
-    LOG.info(
-        String.format(
-            "ZMQ %s socket heartbeat configured: interval=%dms, timeout=%dms, ttl=%dms",
-            socketType, heartbeatIvl, heartbeatIvl * 3, heartbeatIvl * 6));
+  if (heartbeatPeriod == null || heartbeatPeriod.isZero() || heartbeatPeriod.isNegative()) {
+    return;
   }
+  long ivlMsLong = heartbeatPeriod.toMillis();
+  // Enforce sane bounds: at least 100ms and at most Integer.MAX_VALUE / 6 to avoid overflow below
+  long min = 100L;
+  long max = Integer.MAX_VALUE / 6L;
+  if (ivlMsLong < min) {
+    ivlMsLong = min;
+  } else if (ivlMsLong > max) {
+    ivlMsLong = max;
+  }
+  int heartbeatIvl = (int) ivlMsLong;
+  int heartbeatTimeout = heartbeatIvl * 3;
+  int heartbeatTtl = heartbeatIvl * 6;
+  socket.setHeartbeatIvl(heartbeatIvl);
+  socket.setHeartbeatTimeout(heartbeatTimeout);
+  socket.setHeartbeatTtl(heartbeatTtl);
+  LOG.info(
+      String.format(
+          "ZMQ %s socket heartbeat configured: interval=%dms, timeout=%dms, ttl=%dms",
+          socketType, heartbeatIvl, heartbeatTimeout, heartbeatTtl));
 }

---

Suggestion 2:

Refactor duplicated code into a utility method

Extract the duplicated configureHeartbeat method from BoundZmqEventBus and UnboundZmqEventBus into a static helper method in a new utility class to avoid code repetition and improve maintainability.

java/src/org/openqa/selenium/events/zeromq/BoundZmqEventBus.java [84-97]

-private void configureHeartbeat(ZMQ.Socket socket, Duration heartbeatPeriod, String socketType) {
-  if (heartbeatPeriod != null && !heartbeatPeriod.isZero() && !heartbeatPeriod.isNegative()) {
-    int heartbeatIvl = (int) heartbeatPeriod.toMillis();
-    socket.setHeartbeatIvl(heartbeatIvl);
-    // Set heartbeat timeout to 3x the interval
-    socket.setHeartbeatTimeout(heartbeatIvl * 3);
-    // Set heartbeat TTL to 6x the interval
-    socket.setHeartbeatTtl(heartbeatIvl * 6);
-    LOG.info(
-        String.format(
-            "Event bus %s socket heartbeat configured: interval=%dms, timeout=%dms, ttl=%dms",
-            socketType, heartbeatIvl, heartbeatIvl * 3, heartbeatIvl * 6));
+// In a new file: java/src/org/openqa/selenium/events/zeromq/ZmqUtils.java
+package org.openqa.selenium.events.zeromq;
+
+import java.time.Duration;
+import java.util.logging.Logger;
+import org.zeromq.ZMQ;
+
+class ZmqUtils {
+  private static final Logger LOG = Logger.getLogger(ZmqUtils.class.getName());
+
+  private ZmqUtils() {
+    // Utility class
+  }
+
+  static void configureHeartbeat(ZMQ.Socket socket, Duration heartbeatPeriod, String socketType) {
+    if (heartbeatPeriod != null && !heartbeatPeriod.isZero() && !heartbeatPeriod.isNegative()) {
+      long periodMillis = heartbeatPeriod.toMillis();
+      if (periodMillis > Integer.MAX_VALUE) {
+        LOG.warning(
+            String.format(
+                "Heartbeat period %dms is too large. Capping at %dms.",
+                periodMillis, Integer.MAX_VALUE));
+        periodMillis = Integer.MAX_VALUE;
+      }
+      int heartbeatIvl = (int) periodMillis;
+      socket.setHeartbeatIvl(heartbeatIvl);
+      // Set heartbeat timeout to 3x the interval
+      socket.setHeartbeatTimeout(heartbeatIvl * 3);
+      // Set heartbeat TTL to 6x the interval
+      socket.setHeartbeatTtl(heartbeatIvl * 6);
+      LOG.info(
+          String.format(
+              "Event bus %s socket heartbeat configured: interval=%dms, timeout=%dms, ttl=%dms",
+              socketType, heartbeatIvl, heartbeatIvl * 3, heartbeatIvl * 6));
+    }
   }
 }
 
+// In BoundZmqEventBus.java, replace the configureHeartbeat method with:
+private void configureHeartbeat(ZMQ.Socket socket, Duration heartbeatPeriod, String socketType) {
+  ZmqUtils.configureHeartbeat(socket, heartbeatPeriod, socketType);
+}
+

---

Suggestion 3:

Simplify heartbeat timeout math

Use consistent and clearer time calculations; avoid chained multiply/divide on Duration which can be error-prone. Replace with a single multiplication by half the multiplier to match intent and readability.

java/src/org/openqa/selenium/grid/distributor/local/LocalGridModel.java [245-247]

 Instant lostTime =
-    lastTouched.plus(
-        node.getHeartbeatPeriod().multipliedBy(PURGE_TIMEOUT_MULTIPLIER).dividedBy(2));
+    lastTouched.plus(node.getHeartbeatPeriod().multipliedBy(PURGE_TIMEOUT_MULTIPLIER / 2));

---

[Auto-generated best practices - 2025-11-28]