[grid] Enhance max-sessions in case specify driver configuration in Node

[VietND96]

User description

🔗 Related Issues

💥 What does this PR do?

Selenium Grid Node Max-Sessions Configuration

The scenarios below explain how Node max-sessions are calculated based on different configuration scenarios.

Scenarios

Scenario 1: Detect Drivers (Default) - No Max-Sessions

Configuration: detect-drivers = true (default), no max-sessions specified
Behavior: Each driver gets full CPU cores, Safari limited to 1

[node]
detect-drivers = true
# max-sessions not specified (uses CPU cores)

Result on 10-core system:

• Chrome: 10 slots
• Firefox: 10 slots
• Edge: 10 slots
• Safari: 1 slot
• Total Node max-sessions: 10 (capped at CPU cores)

Scenario 2: Detect Drivers with Max-Sessions Within CPU Cores

Configuration: detect-drivers = true, max-sessions ≤ CPU cores
Behavior: Each driver gets the configured max-sessions value

[node]
detect-drivers = true
max-sessions = 6

Result on 10-core system:

• Chrome: 6 slots
• Firefox: 6 slots
• Edge: 6 slots
• Safari: 1 slot
• Total Node max-sessions: 6

Scenario 3: Detect Drivers with Max-Sessions Above CPU Cores

Configuration: detect-drivers = true, max-sessions > CPU cores
Behavior: Each driver gets full CPU cores, total capped at CPU cores

[node]
detect-drivers = true
max-sessions = 15

Result on 10-core system:

• Chrome: 10 slots
• Firefox: 10 slots
• Edge: 10 slots
• Safari: 1 slot
• Total Node max-sessions: 10 (capped at CPU cores)

Scenario 4: Override Max-Sessions Enabled

Configuration: override-max-sessions = true
Behavior: Each driver gets node max-sessions value

[node]
detect-drivers = true
max-sessions = 15
override-max-sessions = true

Result on 10-core system:

• Chrome: 15 slots
• Firefox: 15 slots
• Edge: 15 slots
• Safari: 1 slot
• Total Node max-sessions: 46

Scenario 5: Custom Driver Configuration

Configuration: Explicit driver configurations
Behavior: Uses configured values per driver

[node]
detect-drivers = false

[[node.driver-configuration]]
display-name = "Chrome"
stereotype = '{"browserName": "chrome"}'
max-sessions = 5

[[node.driver-configuration]]
display-name = "Firefox" 
stereotype = '{"browserName": "firefox"}'
max-sessions = 3

Result:

• Chrome: 5 slots
• Firefox: 3 slots
• Total Node max-sessions: 8

Scenario 6: Mixed Configuration

Configuration: Custom drivers with detect-drivers fallback

[node]
detect-drivers = true
max-sessions = 8

[[node.driver-configuration]]
display-name = "Chrome Custom"
stereotype = '{"browserName": "chrome"}'
max-sessions = 12

Result on 10-core system:

• Chrome Custom: 12 slots
• Firefox: 2 slots (from detected drivers)
• Edge: 2 slots (from detected drivers)
• Safari: 1 slot (from detected drivers)
• Total Node max-sessions: 8 (capped at configured max-sessions)

Key Rules

1. Safari Constraint: Always limited to 1 session regardless of configuration
2. CPU Core Safety: Total sessions capped at CPU cores unless override-max-sessions = true
3. Explicit Max-Sessions: When max-sessions ≤ CPU cores, Node respects the explicit value
4. Detect Drivers Priority: When detect-drivers = true and no explicit max-sessions, each flexible driver gets full CPU cores
5. Configuration Precedence: Custom driver configs override detected drivers

🔧 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)

---

PR Type

Enhancement

---

Description

• Enhance smart max-sessions handling for Selenium Grid nodes

• Calculate total sessions from driver configurations instead of fixed values

• Distribute CPU cores optimally among detected and configured drivers

• Add comprehensive test coverage for various session allocation scenarios

---

Diagram Walkthrough

flowchart LR
  A["Node Configuration"] --> B["Driver Discovery"]
  B --> C["CPU Core Distribution"]
  C --> D["Session Allocation"]
  D --> E["Total Max Sessions"]
  F["Override Flag"] --> D
  G["Driver Configs"] --> D

Loading

File Walkthrough

	Relevant files
Enhancement	NodeOptions.java Smart session allocation and CPU distribution logic            java/src/org/openqa/selenium/grid/node/config/NodeOptions.java Add centralized calculation methods for driver session allocation Implement CPU core distribution logic for detected and configured drivers Refactor getMaxSessions() to sum actual driver sessions Add support for override-max-sessions flag behavior +232/-15
Tests	NodeOptionsTest.java Comprehensive test coverage for session allocation              java/test/org/openqa/selenium/grid/node/config/NodeOptionsTest.java Add comprehensive test coverage for session allocation scenarios Test CPU core distribution among multiple drivers Verify override-max-sessions flag behavior Test explicit driver config max-sessions handling +572/-1

---

[qodo-merge-pro]

PR Reviewer Guide 🔍

(Review updated until commit 3a921ac)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 5678 - Partially compliant Compliant requirements: Ensure reliable startup/connection handling across multiple sessions (partially addressed by smarter max-sessions and server arg defaults). Non-compliant requirements: Explicit fix or validation for ChromeDriver ConnectFailure on Ubuntu 16.04 environment. Specific handling for ChromeDriver versions mentioned (Chrome 65 / ChromeDriver 2.35) and Selenium 3.9.0. Reproduction steps and targeted test coverage for repeated instantiation failures. Requires further human verification: Validate on Ubuntu 16.04 with the exact Chrome/ChromeDriver versions to confirm ConnectFailure is mitigated. Run multi-session Chrome stress tests to ensure no connection refused errors occur after first instantiation. 1234 - Partially compliant Compliant requirements: Non-compliant requirements: Fix for JS-in-href click behavior in Firefox. Tests validating click() triggers JS in href across versions. Requires further human verification: Browser behavior verification in Firefox with affected versions is unrelated to this PR and cannot be assessed here.

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Behavior Change getMaxSessions() now derives from calculated driver totals and caps at CPU cores when override is false. This alters semantics and may affect users relying on explicit node max-sessions; ensure backward compatibility and config interactions are intended. public int getMaxSessions() { int maxSessions = config.getInt(NODE_SECTION, "max-sessions").orElse(DEFAULT_MAX_SESSIONS); Require.positive("Driver max sessions", maxSessions); boolean overrideMaxSessions = config.getBool(NODE_SECTION, "override-max-sessions").orElse(OVERRIDE_MAX_SESSIONS); // Always calculate sum of actual driver sessions for consistency int totalActualSessions = calculateTotalMaxSessionsFromAllDrivers(); if (overrideMaxSessions) { return totalActualSessions; } else { // When override-max-sessions = false, return sum of actual sessions but cap at CPU cores return totalActualSessions > 0 ? Math.min(totalActualSessions, DEFAULT_MAX_SESSIONS) : Math.min(maxSessions, DEFAULT_MAX_SESSIONS); } } Allocation Logic calculateOptimizedCpuDistribution uses DEFAULT_MAX_SESSIONS (CPU cores) as remainingCores baseline and assigns 1 session to single-session drivers by name. Verify correctness when multiple single-session drivers are present and when detected driver display names differ in case/locale. private Map<String, Integer> calculateOptimizedCpuDistribution(List<String> driverNames) { Map<String, Integer> sessionsPerDriver = new HashMap<>(); // First, allocate sessions for constrained drivers (like Safari) int remainingCores = DEFAULT_MAX_SESSIONS; List<String> flexibleDrivers = new ArrayList<>(); for (String driverName : driverNames) { if (SINGLE_SESSION_DRIVERS.contains(driverName.toLowerCase(Locale.ENGLISH))) { // Constrained drivers get exactly 1 session sessionsPerDriver.put(driverName, 1); remainingCores--; } else { flexibleDrivers.add(driverName); } } // Then distribute remaining cores among flexible drivers if (!flexibleDrivers.isEmpty() && remainingCores > 0) { int sessionsPerFlexibleDriver = Math.max(1, remainingCores / flexibleDrivers.size()); int remainderCores = remainingCores % flexibleDrivers.size(); // Distribute base sessions to all flexible drivers for (int i = 0; i < flexibleDrivers.size(); i++) { String driverName = flexibleDrivers.get(i); int sessions = sessionsPerFlexibleDriver; // Distribute remainder cores to the first 'remainderCores' drivers if (i < remainderCores) { sessions++; } sessionsPerDriver.put(driverName, sessions); } } else if (!flexibleDrivers.isEmpty()) { // No remaining cores, give each flexible driver 1 session for (String driverName : flexibleDrivers) { sessionsPerDriver.put(driverName, 1); } } return sessionsPerDriver; } Opinionated Defaults Forcing '--override-max-sessions true' and setting '--max-sessions' to CPU count changes default behavior for Python users. Confirm this is desired and won’t overcommit hosts or conflict with user-specified args. "--port", str(self.port), "--log-level", self.log_level, "--selenium-manager", "true", "--enable-managed-downloads", "true", "--override-max-sessions", "true", "--max-sessions", str(multiprocessing.cpu_count()), ]

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	The session allocation logic is overly complex The new session calculation logic is convoluted due to its reliance on multiple flags and configuration levels. It should be simplified for better user predictability, such as by giving precedence to explicit max-sessions settings in driver configurations. Examples: java/src/org/openqa/selenium/grid/node/config/NodeOptions.java [291-406] private Map<String, Integer> calculateActualMaxSessionsPerDriverConfig() { Map<String, Integer> result = new HashMap<>(); boolean overrideMaxSessions = config.getBool(NODE_SECTION, "override-max-sessions").orElse(OVERRIDE_MAX_SESSIONS); int nodeMaxSessions = config.getInt(NODE_SECTION, "max-sessions").orElse(DEFAULT_MAX_SESSIONS); // Handle explicit driver configurations Optional<List<List<String>>> driverConfigs = config.getArray(NODE_SECTION, "driver-configuration"); if (driverConfigs.isPresent()) { ... (clipped 106 lines) Solution Walkthrough: Before: // Simplified logic when override-max-sessions=false and driver configs are used function calculateSessionsPerDriver(): sessionsPerDriverConfig = availableCpuCores / numDriverConfigs for each driverConfig: driverMaxSessions = sessionsPerDriverConfig // Explicit driver max-sessions is only used if it's within the calculated limit if driverConfig.has("max-sessions"): explicitMax = driverConfig.get("max-sessions") if 1 <= explicitMax <= sessionsPerDriverConfig: driverMaxSessions = explicitMax // ... more logic for node-level max-sessions override ... result.put(driverName, driverMaxSessions) return result After: // Simplified logic prioritizing explicit configuration function calculateSessionsPerDriver(): for each driverConfig: // Prioritize explicit max-sessions from the driver configuration if driverConfig.has("max-sessions"): driverMaxSessions = driverConfig.get("max-sessions") // Fallback to node-level max-sessions if not specified per driver else if node.has("max-sessions"): driverMaxSessions = node.get("max-sessions") // Fallback to a default (e.g., CPU core distribution) else: driverMaxSessions = availableCpuCores / numDriverConfigs result.put(driverName, driverMaxSessions) return result Suggestion importance[1-10]: 9 __ Why: This is a critical design-level suggestion that correctly identifies the new session allocation logic as overly complex and non-intuitive for users, which could lead to configuration errors and frustration.	High
Possible issue	✅ Correctly report zero max sessions Suggestion Impact: The commit changed the logic to always return Math.min(totalActualSessions, DEFAULT_MAX_SESSIONS) when override-max-sessions is false, removing the previous fallback to the configured maxSessions. It also updated totalActualSessions calculation to not depend on nodeMaxSessions, thereby allowing a 0 result when no drivers are available. code diff: @@ -272,15 +268,50 @@ config.getBool(NODE_SECTION, "override-max-sessions").orElse(OVERRIDE_MAX_SESSIONS); // Always calculate sum of actual driver sessions for consistency - int totalActualSessions = calculateTotalMaxSessionsFromAllDrivers(maxSessions); + int totalActualSessions = calculateTotalMaxSessionsFromAllDrivers(); if (overrideMaxSessions) { return totalActualSessions; } else { // When override-max-sessions = false, return sum of actual sessions but cap at CPU cores - return totalActualSessions > 0 - ? Math.min(totalActualSessions, DEFAULT_MAX_SESSIONS) - : Math.min(maxSessions, DEFAULT_MAX_SESSIONS); + return Math.min(totalActualSessions, DEFAULT_MAX_SESSIONS); + } Correct getMaxSessions() to return 0 when no actual sessions can be created, instead of falling back to a configured max-sessions value. java/src/org/openqa/selenium/grid/node/config/NodeOptions.java [280-283] // When override-max-sessions = false, return sum of actual sessions but cap at CPU cores -return totalActualSessions > 0 - ? Math.min(totalActualSessions, DEFAULT_MAX_SESSIONS) - : Math.min(maxSessions, DEFAULT_MAX_SESSIONS); +return Math.min(totalActualSessions, DEFAULT_MAX_SESSIONS); [Suggestion processed] Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a logic flaw where getMaxSessions() can report a non-zero value even if no drivers are configured, which is misleading. Fixing this improves the correctness and predictability of the node's reported capacity.	Medium
General	✅ Improve CPU core distribution logic Suggestion Impact: The commit updated the distribution logic to compute base sessions and distribute the remainder (+1) to the first N flexible drivers, implementing the suggested improvement. code diff: @@ -427,14 +470,37 @@ // Then distribute remaining cores among flexible drivers if (flexibleDrivers.size() > 0 && remainingCores > 0) { int sessionsPerFlexibleDriver = Math.max(1, remainingCores / flexibleDrivers.size()); - for (WebDriverInfo info : flexibleDrivers) { - sessionsPerDriver.put(info, sessionsPerFlexibleDriver); - } + int remainderCores = remainingCores % flexibleDrivers.size(); + + // Distribute base sessions to all flexible drivers + for (int i = 0; i < flexibleDrivers.size(); i++) { + WebDriverInfo info = flexibleDrivers.get(i); + int sessions = sessionsPerFlexibleDriver; + + // Distribute remainder cores to the first 'remainderCores' drivers + if (i < remainderCores) { + sessions++; + } + + sessionsPerDriver.put(info, sessions); + } + + LOG.log( + Level.FINE, + "Distributed {0} cores among {1} flexible drivers: {2} base sessions each, " + + "{3} drivers get +1 extra session", + new Object[] { + remainingCores, flexibleDrivers.size(), sessionsPerFlexibleDriver, remainderCores + }); } else if (flexibleDrivers.size() > 0) { // No remaining cores, give each flexible driver 1 session for (WebDriverInfo info : flexibleDrivers) { sessionsPerDriver.put(info, 1); } + LOG.log( + Level.FINE, + "No remaining cores available, assigning 1 session to each of {0} flexible drivers", + flexibleDrivers.size()); } Improve the CPU core distribution logic in calculateOptimizedCpuDistribution to also allocate the remainder from integer division, ensuring better resource utilization. java/src/org/openqa/selenium/grid/node/config/NodeOptions.java [427-438] // Then distribute remaining cores among flexible drivers if (flexibleDrivers.size() > 0 && remainingCores > 0) { - int sessionsPerFlexibleDriver = Math.max(1, remainingCores / flexibleDrivers.size()); - for (WebDriverInfo info : flexibleDrivers) { - sessionsPerDriver.put(info, sessionsPerFlexibleDriver); + int baseSessions = remainingCores / flexibleDrivers.size(); + int remainder = remainingCores % flexibleDrivers.size(); + for (int i = 0; i < flexibleDrivers.size(); i++) { + WebDriverInfo info = flexibleDrivers.get(i); + int sessions = baseSessions + (i < remainder ? 1 : 0); + sessionsPerDriver.put(info, Math.max(1, sessions)); } } else if (flexibleDrivers.size() > 0) { // No remaining cores, give each flexible driver 1 session for (WebDriverInfo info : flexibleDrivers) { sessionsPerDriver.put(info, 1); } } [Suggestion processed] Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that integer division can underutilize resources and proposes a more optimal distribution algorithm that allocates the remainder, improving the new resource allocation logic.	Low
Learned best practice	✅ Safely parse driver max-sessions Suggestion Impact: The commit introduced a helper method parseMaxSessionsSafely with try/catch, validation, and logging, and replaced direct Integer.parseInt calls for "max-sessions" with this safe parsing, aligning with the suggestion’s intent. code diff: + * Safely parse an integer value for max-sessions with validation and logging. Guards against + * NumberFormatException and ensures the value is positive. + * + * @param value the string value to parse + * @param defaultValue the default value to use if parsing fails or value is invalid + * @param context descriptive context for logging (e.g., "driver config", "node config") + * @return a valid positive integer, or the default value if parsing fails + */ + private static int parseMaxSessionsSafely(String value, int defaultValue, String context) { + if (value == null || value.trim().isEmpty()) { + LOG.log( + Level.WARNING, + "Max-sessions value is null or empty for {0}, using default: {1}", + new Object[] {context, defaultValue}); + return defaultValue; + } + + try { + int parsedValue = Integer.parseInt(value.trim()); + if (parsedValue <= 0) { + LOG.log( + Level.WARNING, + "Max-sessions value {0} is not positive for {1}, using default: {2}", + new Object[] {parsedValue, context, defaultValue}); + return defaultValue; + } + return parsedValue; + } catch (NumberFormatException e) { + LOG.log( + Level.WARNING, + "Invalid max-sessions value ''{0}'' for {1}, using default: {2}. Error: {3}", + new Object[] {value, context, defaultValue, e.getMessage()}); + return defaultValue; + } + } + + /** * Calculate the actual max-sessions per driver config based on the current configuration. This * method ensures consistency between getMaxSessions() and actual session allocation. */ @@ -323,8 +360,10 @@ for (Map<String, String> configMap : configList) { String displayName = configMap.get("display-name"); int driverMaxSessions = - Integer.parseInt( - configMap.getOrDefault("max-sessions", String.valueOf(nodeMaxSessions))); + parseMaxSessionsSafely( + configMap.get("max-sessions"), + nodeMaxSessions, + "driver config '" + displayName + "'"); result.put(displayName, driverMaxSessions); } } else { @@ -342,7 +381,11 @@ // Check if driver config has explicit max-sessions within allowed range if (configMap.containsKey("max-sessions")) { - int explicitMaxSessions = Integer.parseInt(configMap.get("max-sessions")); + int explicitMaxSessions = + parseMaxSessionsSafely( + configMap.get("max-sessions"), + sessionsPerDriverConfig, + "driver config '" + displayName + "' explicit max-sessions"); if (explicitMaxSessions >= 1 && explicitMaxSessions <= sessionsPerDriverConfig) { Guard the integer parsing with a safe default and validation to prevent NumberFormatException when the config value is missing or malformed. java/src/org/openqa/selenium/grid/node/config/NodeOptions.java [325-327] -int driverMaxSessions = - Integer.parseInt( - configMap.getOrDefault("max-sessions", String.valueOf(nodeMaxSessions))); +int driverMaxSessions; +try { + driverMaxSessions = Integer.parseInt(configMap.getOrDefault("max-sessions", String.valueOf(nodeMaxSessions))); +} catch (NumberFormatException e) { + LOG.log(Level.WARNING, "Invalid max-sessions for {0}, using node default: {1}", new Object[]{configMap.get("display-name"), nodeMaxSessions}); + driverMaxSessions = nodeMaxSessions; +} [Suggestion processed] Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Initialize variables defensively and validate parsed inputs to avoid NumberFormatException or null-derived issues.	Low
Update

[VietND96]

/review

[qodo-merge-pro]

Persistent review updated to latest commit 3a921ac

[VietND96]

What do you think @diemol, @pujagani ?