[grid] External datastore JDBC-backed for Session Queue

[VietND96]

User description

🔗 Related Issues

💥 What does this PR do?

• Changes to support multiple replicas: SessionQueue (a part of [🚀 Feature]: Allow multiple Hub pods for high availability docker-selenium#1849)
• Based on the implementation of external datastore, which is available for Seesion Map
• Output package org.seleniumhq.selenium:selenium-session-queue-jdbc

🔧 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

• Implements JDBC-backed session queue for Selenium Grid

• Enables external database storage for session requests

• Supports multiple Grid replicas with shared state

• Adds comprehensive test coverage and configuration options

---

Diagram Walkthrough

flowchart LR
  A["SessionRequest"] --> B["JdbcBackedSessionQueue"]
  B --> C["Database Table"]
  C --> D["session_queue"]
  B --> E["JDBC Connection"]
  F["Configuration"] --> G["JdbcSessionQueueOptions"]
  G --> E
  H["Grid Replicas"] --> B

Loading

File Walkthrough

	Relevant files

---

[qodo-merge-pro]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ❌ 1849 - Not compliant Non-compliant requirements: Add error handling for proxy server connection refusal Provide a way to catch proxy connection errors programmatically Improve error reporting when proxy refuses connections Requires further human verification: Verification that this JDBC session queue implementation addresses the proxy connection issues mentioned in the ticket

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

🧪 PR contains tests

🔒 Security concerns Sensitive information exposure: The JDBC password is stored as a plain text string in the JdbcSessionQueueOptions class and passed directly to DriverManager.getConnection(). This could lead to password exposure through logs, stack traces, or memory dumps. Consider implementing secure credential management or encryption for database passwords.

⚡ Recommended focus areas for review Resource Leak Multiple database operations create PreparedStatement and ResultSet objects without proper try-with-resources handling, potentially causing resource leaks in case of exceptions. try (PreparedStatement selectStmt = connection.prepareStatement(select)) { selectStmt.setString(1, requestId.toString()); String statementStr = selectStmt.toString(); span.setAttribute(DATABASE_STATEMENT, statementStr); span.setAttribute(DATABASE_OPERATION, "select"); attributeMap.put(DATABASE_STATEMENT, statementStr); attributeMap.put(DATABASE_OPERATION, "select"); ResultSet rs = selectStmt.executeQuery(); if (rs.next()) { String payload = rs.getString("payload"); try (PreparedStatement deleteStmt = connection.prepareStatement(delete)) { deleteStmt.setString(1, requestId.toString()); String deleteStatementStr = deleteStmt.toString(); span.setAttribute(DATABASE_STATEMENT, deleteStatementStr); span.setAttribute(DATABASE_OPERATION, "delete"); attributeMap.put(DATABASE_STATEMENT, deleteStatementStr); attributeMap.put(DATABASE_OPERATION, "delete"); int rowCount = deleteStmt.executeUpdate(); attributeMap.put("rows.deleted", rowCount); span.addEvent("Removed session request from queue", attributeMap); return Optional.of(JSON.toType(payload, SessionRequest.class)); } Static Variables Static variables jdbcUser and jdbcUrl are shared across all instances, which could cause issues in multi-threaded environments or when multiple instances use different database configurations. private static String jdbcUser; private static String jdbcUrl; Security Risk JDBC password is stored as plain text in configuration and could be logged or exposed through debugging. Consider using secure credential management. private final String jdbcPassword; public JdbcSessionQueueOptions(Config config) { Require.nonNull("Config", config); try { this.jdbcUrl = config.get(SESSION_QUEUE_SECTION, "jdbc-url").orElse(""); this.jdbcUser = config.get(SESSION_QUEUE_SECTION, "jdbc-user").orElse(""); this.jdbcPassword = config.get(SESSION_QUEUE_SECTION, "jdbc-password").orElse("");

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix race condition in remove method The remove method has a race condition where another thread could delete the record between the SELECT and DELETE operations. This could lead to inconsistent state where the method returns a SessionRequest but the record wasn't actually deleted by this thread. java/src/org/openqa/selenium/grid/sessionqueue/jdbc/JdbcBackedSessionQueue.java [193-252] @Override public Optional<SessionRequest> remove(RequestId requestId) { Require.nonNull("RequestId to remove", requestId); try (Span span = tracer .getCurrentContext() .createSpan("SELECT and DELETE session request from session_queue")) { AttributeMap attributeMap = tracer.createAttributeMap(); setCommonSpanAttributes(span); setCommonEventAttributes(attributeMap); - String select = "SELECT payload FROM " + TABLE_NAME + " WHERE request_id = ?"; + String selectForUpdate = "SELECT payload FROM " + TABLE_NAME + " WHERE request_id = ? FOR UPDATE"; String delete = "DELETE FROM " + TABLE_NAME + " WHERE request_id = ?"; - try (PreparedStatement selectStmt = connection.prepareStatement(select)) { - selectStmt.setString(1, requestId.toString()); + try { + connection.setAutoCommit(false); + + try (PreparedStatement selectStmt = connection.prepareStatement(selectForUpdate)) { + selectStmt.setString(1, requestId.toString()); - String statementStr = selectStmt.toString(); - span.setAttribute(DATABASE_STATEMENT, statementStr); - span.setAttribute(DATABASE_OPERATION, "select"); - attributeMap.put(DATABASE_STATEMENT, statementStr); - attributeMap.put(DATABASE_OPERATION, "select"); + String statementStr = selectStmt.toString(); + span.setAttribute(DATABASE_STATEMENT, statementStr); + span.setAttribute(DATABASE_OPERATION, "select"); + attributeMap.put(DATABASE_STATEMENT, statementStr); + attributeMap.put(DATABASE_OPERATION, "select"); - ResultSet rs = selectStmt.executeQuery(); - if (rs.next()) { - String payload = rs.getString("payload"); + ResultSet rs = selectStmt.executeQuery(); + if (rs.next()) { + String payload = rs.getString("payload"); - try (PreparedStatement deleteStmt = connection.prepareStatement(delete)) { - deleteStmt.setString(1, requestId.toString()); + try (PreparedStatement deleteStmt = connection.prepareStatement(delete)) { + deleteStmt.setString(1, requestId.toString()); - String deleteStatementStr = deleteStmt.toString(); - span.setAttribute(DATABASE_STATEMENT, deleteStatementStr); - span.setAttribute(DATABASE_OPERATION, "delete"); - attributeMap.put(DATABASE_STATEMENT, deleteStatementStr); - attributeMap.put(DATABASE_OPERATION, "delete"); + String deleteStatementStr = deleteStmt.toString(); + span.setAttribute(DATABASE_STATEMENT, deleteStatementStr); + span.setAttribute(DATABASE_OPERATION, "delete"); + attributeMap.put(DATABASE_STATEMENT, deleteStatementStr); + attributeMap.put(DATABASE_OPERATION, "delete"); - int rowCount = deleteStmt.executeUpdate(); - attributeMap.put("rows.deleted", rowCount); - span.addEvent("Removed session request from queue", attributeMap); + int rowCount = deleteStmt.executeUpdate(); + connection.commit(); + attributeMap.put("rows.deleted", rowCount); + span.addEvent("Removed session request from queue", attributeMap); - return Optional.of(JSON.toType(payload, SessionRequest.class)); + return Optional.of(JSON.toType(payload, SessionRequest.class)); + } + } else { + connection.commit(); + attributeMap.put("request.found", false); + span.addEvent("Session request not found in queue", attributeMap); } - } else { - attributeMap.put("request.found", false); - span.addEvent("Session request not found in queue", attributeMap); } } catch (SQLException e) { + try { + connection.rollback(); + } catch (SQLException rollbackEx) { + LOG.log(Level.WARNING, "Failed to rollback transaction", rollbackEx); + } span.setAttribute("error", true); span.setStatus(Status.CANCELLED); EXCEPTION.accept(attributeMap, e); attributeMap.put( AttributeKey.EXCEPTION_MESSAGE.getKey(), "Unable to remove session request from queue: " + e.getMessage()); span.addEvent(AttributeKey.EXCEPTION_EVENT.getKey(), attributeMap); LOG.log(Level.SEVERE, "Failed to remove session request from queue", e); + } finally { + try { + connection.setAutoCommit(true); + } catch (SQLException e) { + LOG.log(Level.WARNING, "Failed to reset auto-commit", e); + } } } return Optional.empty(); } Apply / Chat Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a race condition between the SELECT and DELETE operations, which is a critical flaw in a concurrent environment, and proposes a robust fix using transactions and row-level locking.	High
	Implement capability matching with stereotypes parameter The getNextAvailable method ignores the stereotypes parameter which is used to filter session requests based on capabilities matching. This could lead to returning incompatible session requests that don't match the available node capabilities. java/src/org/openqa/selenium/grid/sessionqueue/jdbc/JdbcBackedSessionQueue.java [254-295] @Override public List<SessionRequest> getNextAvailable(Map<Capabilities, Long> stereotypes) { try (Span span = tracer .getCurrentContext() .createSpan("SELECT next available session request from session_queue")) { AttributeMap attributeMap = tracer.createAttributeMap(); setCommonSpanAttributes(span); setCommonEventAttributes(attributeMap); - String sql = "SELECT payload FROM " + TABLE_NAME + " ORDER BY enqueue_time ASC LIMIT 1"; + String sql = "SELECT payload FROM " + TABLE_NAME + " ORDER BY enqueue_time ASC"; try (PreparedStatement stmt = connection.prepareStatement(sql)) { String statementStr = stmt.toString(); span.setAttribute(DATABASE_STATEMENT, statementStr); span.setAttribute(DATABASE_OPERATION, "select"); attributeMap.put(DATABASE_STATEMENT, statementStr); attributeMap.put(DATABASE_OPERATION, "select"); ResultSet rs = stmt.executeQuery(); - if (rs.next()) { + while (rs.next()) { String payload = rs.getString("payload"); SessionRequest request = JSON.toType(payload, SessionRequest.class); - attributeMap.put("requests.found", 1); - span.addEvent("Retrieved next available session request", attributeMap); - return List.of(request); - } else { - attributeMap.put("requests.found", 0); - span.addEvent("No session requests available", attributeMap); + + // Check if request capabilities match any available stereotypes + if (stereotypes.isEmpty() || matchesStereotypes(request, stereotypes)) { + attributeMap.put("requests.found", 1); + span.addEvent("Retrieved next available session request", attributeMap); + return List.of(request); + } } + + attributeMap.put("requests.found", 0); + span.addEvent("No matching session requests available", attributeMap); } catch (SQLException e) { span.setAttribute("error", true); span.setStatus(Status.CANCELLED); EXCEPTION.accept(attributeMap, e); attributeMap.put( AttributeKey.EXCEPTION_MESSAGE.getKey(), "Unable to get next available session request: " + e.getMessage()); span.addEvent(AttributeKey.EXCEPTION_EVENT.getKey(), attributeMap); LOG.log(Level.SEVERE, "Failed to get next available session request", e); } } return List.of(); } +private boolean matchesStereotypes(SessionRequest request, Map<Capabilities, Long> stereotypes) { + return stereotypes.keySet().stream() + .anyMatch(stereotype -> request.getDesiredCapabilities().asMap().entrySet().stream() + .allMatch(entry -> stereotype.asMap().getOrDefault(entry.getKey(), entry.getValue()).equals(entry.getValue()))); +} + Apply / Chat Suggestion importance[1-10]: 9 __ Why: The suggestion correctly points out a significant functional bug where the stereotypes parameter is ignored, causing the method to not filter session requests as intended, and the proposed change correctly implements this missing logic.	High
General	Remove unreachable exception handling code The constructor catches NoSuchElementException but the config.get() method returns Optional and uses orElse(""), so it will never throw this exception. The catch block is unreachable code that misleads about required vs optional parameters. java/src/org/openqa/selenium/grid/sessionqueue/jdbc/JdbcSessionQueueOptions.java [35-55] public JdbcSessionQueueOptions(Config config) { Require.nonNull("Config", config); - try { - this.jdbcUrl = config.get(SESSION_QUEUE_SECTION, "jdbc-url").orElse(""); - this.jdbcUser = config.get(SESSION_QUEUE_SECTION, "jdbc-user").orElse(""); - this.jdbcPassword = config.get(SESSION_QUEUE_SECTION, "jdbc-password").orElse(""); + this.jdbcUrl = config.get(SESSION_QUEUE_SECTION, "jdbc-url").orElse(""); + this.jdbcUser = config.get(SESSION_QUEUE_SECTION, "jdbc-user").orElse(""); + this.jdbcPassword = config.get(SESSION_QUEUE_SECTION, "jdbc-password").orElse(""); - if (jdbcUrl.isEmpty()) { - throw new JdbcException( - "Missing JDBC Url value. Add sessionqueue option value --sessionqueue-jdbc-url" - + " <url-value>"); - } - } catch (NoSuchElementException e) { + if (jdbcUrl.isEmpty()) { throw new JdbcException( - "Missing sessionqueue options. Check and add all the following options \n" - + " --sessionqueue-jdbc-url <url> \n" - + " --sessionqueue-jdbc-user <user> \n" - + " --sessionqueue-jdbc-password <password>"); + "Missing JDBC Url value. Add sessionqueue option value --sessionqueue-jdbc-url" + + " <url-value>"); } } Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies an unreachable catch block due to the use of Optional.orElse(), and removing this dead code improves code quality and maintainability.	Low
More

[pujagani]

The good looks fine on high-level, but I am not sure using an RDBMS is the right approach to solve this.

[VietND96]

Yes, I am going to add Redis backed also. I added this RDBMS to align with Session Map, in case someone wants to use the same stack for both.