Skip to content

[grid] Add session event API for server-side event bus integration#17015

Merged
VietND96 merged 2 commits into
trunkfrom
node-event-driven-service
Feb 2, 2026
Merged

[grid] Add session event API for server-side event bus integration#17015
VietND96 merged 2 commits into
trunkfrom
node-event-driven-service

Conversation

@VietND96
Copy link
Copy Markdown
Member

@VietND96 VietND96 commented Jan 27, 2026

Summary

Adds a client API to fire custom session events to the Grid's event bus, enabling server-side utilities to react to test lifecycle events.

Users can set up Selenium Grid with sidecar utilities that subscribe to the Grid's event bus, allowing these services to listen and react to session lifecycle events (session created, session closed, custom test events).

Features

Client API

Test code can fire events that server-side utilities subscribe to:

Java:

driver.fireSessionEvent("test:started");
driver.fireSessionEvent("test:failed", Map.of("error", "Element not found"));

Python:

driver.fire_session_event("test:started")
driver.fire_session_event("test:failed", {"error": "Element not found"})

JavaScript:

await driver.fireSessionEvent('test:started');
await driver.fireSessionEvent('test:failed', { error: 'Element not found' });

Ruby:

driver.fire_session_event("test:started")
driver.fire_session_event("test:failed", { error: "Element not found" })

.NET:

driver.FireSessionEvent("test:started");
driver.FireSessionEvent("test:failed", new Dictionary<string, object> { { "error", "Element not found" } });

Server-Side Integration

Subscribe to events via the Guava EventBus:

eventBus.addListener(SessionEvent.listener(data -> {
    String eventType = data.getEventType();
    SessionId sessionId = data.getSessionId();
    Map<String, Object> payload = data.getPayload();
    // React to custom events from test code
}));

For distributed deployments, subscribe to events via ZeroMQ.

Wire Protocol

POST /session/{sessionId}/se/event
Content-Type: application/json

{"eventType": "test:failed", "payload": {"error": "Element not found"}}

Node Support

  • ✅ LocalNode
  • ✅ RemoteNode
  • ✅ OneShotNode (K8s)

Additional Changes

  • Session lifecycle events (SessionCreatedEvent, SessionClosedEvent) with rich context
  • .NET remote test infrastructure with dynamic port allocation
  • EventBusOptions: Smart defaulting based on configuration

Backward Compatibility

All changes are additive and backward compatible.

Test plan

  • Java unit tests pass
  • Python unit tests pass
  • .NET remote tests pass
  • JavaScript tests pass
  • Ruby specs pass

@selenium-ci selenium-ci added B-grid Everything grid and server related C-java Java Bindings labels Jan 27, 2026
@qodo-code-review
Copy link
Copy Markdown
Contributor

PR Type

Enhancement


Description

  • Adds event-driven session lifecycle support for Grid Nodes via EventBus

  • Enables external sidecar services to consume session events (created, closed, custom)

  • Introduces SessionLifecycleListener SPI for in-process event handling

  • Adds /session/{sessionId}/se/event endpoint for firing custom session events

  • Supports dynamic EventBus selection (GuavaEventBus vs ZeroMqEventBus) via --bind-bus flag


File Walkthrough

Relevant files
Enhancement
16 files
SessionCreatedData.java
New data class for session creation events                             
+150/-0 
SessionCreatedEvent.java
New event fired when session is created                                   
+62/-0   
SessionEvent.java
New event for custom user-defined session events                 
+142/-0 
SessionEventData.java
New data class for custom session event payloads                 
+259/-0 
SessionClosedData.java
Enhanced with full session context for sidecar services   
+127/-15
SessionClosedEvent.java
Updated to support rich session closure context                   
+16/-1   
FireSessionEvent.java
New HTTP handler for firing custom session events               
+71/-0   
SessionLifecycleListener.java
New SPI interface for session lifecycle event handling     
+128/-0 
Node.java
Added abstract fireSessionEvent method and route                 
+13/-0   
LocalNode.java
Implements session event firing and lifecycle listener integration
+151/-2 
SessionSlot.java
Enhanced to capture and pass session context to events     
+27/-1   
OneShotNode.java
Stub implementation of fireSessionEvent method                     
+6/-0     
RemoteNode.java
Stub implementation of fireSessionEvent method                     
+5/-0     
Standalone.java
Added dynamic EventBus selection based on bind-bus flag   
+12/-2   
DefaultStandaloneConfig.java
Removed hardcoded EventBus to allow dynamic selection       
+1/-4     
EventBusOptions.java
Added overload for dynamic EventBus implementation selection
+25/-1   

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented Jan 27, 2026

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Unauthenticated event injection

Description: The newly added POST endpoint "/session/{sessionId}/se/event" does not appear to be
protected by requiresSecret (unlike some other node endpoints), so if the Node HTTP server
is reachable an attacker could inject arbitrary session events to sidecar consumers and
potentially trigger unintended actions or metadata/log exfiltration depending on listener
implementations.
Node.java [182-184]

Referred Code
post("/session/{sessionId}/se/event")
    .to(params -> new FireSessionEvent(this, sessionIdFrom(params)))
    .with(spanDecorator("node.fire_session_event")),
Unbounded request payload

Description: fireSessionEvent parses arbitrary JSON (JSON.toType(string(req), Json.MAP_TYPE)) and
accepts an unbounded payload map without size/rate limits, which may enable
denial-of-service via large request bodies and/or expensive parsing.
LocalNode.java [1084-1130]

Referred Code
@Override
@SuppressWarnings("unchecked")
public HttpResponse fireSessionEvent(HttpRequest req, SessionId id) {
  Require.nonNull("Session ID", id);

  // Verify session exists
  SessionSlot slot = currentSessions.getIfPresent(id);
  if (slot == null) {
    throw new NoSuchSessionException("Cannot find session with id: " + id);
  }

  // Parse the event data from request
  Map<String, Object> incoming = JSON.toType(string(req), Json.MAP_TYPE);
  String eventType = (String) incoming.get("eventType");
  if (eventType == null || eventType.isEmpty()) {
    throw new WebDriverException(
        "Event type is required. Please provide 'eventType' in payload.");
  }

  Map<String, Object> payload = (Map<String, Object>) incoming.get("payload");



 ... (clipped 26 lines)
Ticket Compliance
🟡
🎫 #1234
🔴 Clicking an element whose href contains JavaScript should trigger that JavaScript when
using click() in Selenium 2.48 with Firefox (regression vs 2.47.1).
Provide a fix or change that restores the prior behavior in Firefox 42 (or generally
Firefox) for JavaScript href execution on click.
🟡
🎫 #5678
🔴 Address repeated ChromeDriver instantiation errors reporting ConnectFailure (Connection
refused) on Ubuntu 16.04 with Selenium 3.9.0 / Chrome 65 / ChromeDriver 2.35.
Ensure subsequent ChromeDriver instantiations do not produce ConnectFailure errors (or
document/mitigate the root cause).
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

🔴
Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
JSON null handling: SessionCreatedData.fromJson does not validate required fields before constructing the
object, which can result in non-actionable Require.nonNull/NullPointerException-style
failures instead of a clear JsonException describing missing fields.

Referred Code
private static SessionCreatedData fromJson(JsonInput input) {
  SessionId sessionId = null;
  NodeId nodeId = null;
  URI nodeUri = null;
  URI sessionUri = null;
  Capabilities capabilities = null;
  Capabilities stereotype = null;
  Instant startTime = null;

  input.beginObject();
  while (input.hasNext()) {
    switch (input.nextName()) {
      case "sessionId":
        sessionId = input.read(SessionId.class);
        break;
      case "nodeId":
        nodeId = input.read(NodeId.class);
        break;
      case "nodeUri":
        nodeUri = input.read(URI.class);
        break;


 ... (clipped 22 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Missing audit context: The new /session/{sessionId}/se/event action is logged without any principal/user identity
or outcome context, and it is unclear from the diff whether such identity is available
elsewhere in the Grid request pipeline.

Referred Code
@Override
@SuppressWarnings("unchecked")
public HttpResponse fireSessionEvent(HttpRequest req, SessionId id) {
  Require.nonNull("Session ID", id);

  // Verify session exists
  SessionSlot slot = currentSessions.getIfPresent(id);
  if (slot == null) {
    throw new NoSuchSessionException("Cannot find session with id: " + id);
  }

  // Parse the event data from request
  Map<String, Object> incoming = JSON.toType(string(req), Json.MAP_TYPE);
  String eventType = (String) incoming.get("eventType");
  if (eventType == null || eventType.isEmpty()) {
    throw new WebDriverException(
        "Event type is required. Please provide 'eventType' in payload.");
  }

  Map<String, Object> payload = (Map<String, Object>) incoming.get("payload");



 ... (clipped 25 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Authz and validation: The new fireSessionEvent endpoint accepts external JSON input and fires it onto the
EventBus, but the diff does not show any explicit authentication/authorization checks or
robust type validation for payload beyond a cast, so correctness and security depend on
request pipeline controls not visible here.

Referred Code
@Override
@SuppressWarnings("unchecked")
public HttpResponse fireSessionEvent(HttpRequest req, SessionId id) {
  Require.nonNull("Session ID", id);

  // Verify session exists
  SessionSlot slot = currentSessions.getIfPresent(id);
  if (slot == null) {
    throw new NoSuchSessionException("Cannot find session with id: " + id);
  }

  // Parse the event data from request
  Map<String, Object> incoming = JSON.toType(string(req), Json.MAP_TYPE);
  String eventType = (String) incoming.get("eventType");
  if (eventType == null || eventType.isEmpty()) {
    throw new WebDriverException(
        "Event type is required. Please provide 'eventType' in payload.");
  }

  Map<String, Object> payload = (Map<String, Object>) incoming.get("payload");



 ... (clipped 25 lines)

Learn more about managing compliance generic rules or creating your own custom rules

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented Jan 27, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Check required created-data fields

In the fromJson method of SessionCreatedData, add null checks for all fields
after parsing to ensure a complete object is constructed and provide a clear
error message if data is missing.

java/src/org/openqa/selenium/grid/data/SessionCreatedData.java [145-148]

 input.endObject();
+
+if (sessionId == null
+    || nodeId == null
+    || nodeUri == null
+    || sessionUri == null
+    || capabilities == null
+    || stereotype == null
+    || startTime == null) {
+  throw new JsonException("Missing required fields in SessionCreatedData JSON");
+}
 
 return new SessionCreatedData(
     sessionId, nodeId, nodeUri, sessionUri, capabilities, stereotype, startTime);
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: This suggestion correctly identifies that the fromJson method could construct an object with null fields, which would then cause a NullPointerException in the constructor. Adding these checks provides robust error handling and a much clearer error message.

Medium
General
Safely handle missing payload
Suggestion Impact:Replaced the direct cast of incoming.get("payload") with a type-checked retrieval that falls back to an empty map when payload is missing or not a Map.

code diff:

-    Map<String, Object> payload = (Map<String, Object>) incoming.get("payload");
+    Object rawPayload = incoming.get("payload");
+    Map<String, Object> payload =
+        (rawPayload instanceof Map) ? (Map<String, Object>) rawPayload : Map.of();

In fireSessionEvent, safely handle the payload from the request by checking its
type before casting, and default to an empty map if it's missing or not a map to
prevent a ClassCastException.

java/src/org/openqa/selenium/grid/node/local/LocalNode.java [1103]

-Map<String, Object> payload = (Map<String, Object>) incoming.get("payload");
+Object rawPayload = incoming.get("payload");
+Map<String, Object> payload =
+    rawPayload instanceof Map
+        ? (Map<String, Object>) rawPayload
+        : Collections.emptyMap();

[Suggestion processed]

Suggestion importance[1-10]: 8

__

Why: This suggestion correctly identifies a potential ClassCastException if the payload from the JSON is missing or not a map. The proposed change makes the code more robust by safely handling this case and preventing a runtime exception.

Medium
High-level
Simplify event notification in LocalNode

In LocalNode, avoid using the event bus for internal SessionClosedEvent
notifications. Instead, call the notifySessionClosed method directly when a
session is stopped to simplify the logic.

Examples:

java/src/org/openqa/selenium/grid/node/local/LocalNode.java [352]
      bus.addListener(SessionClosedEvent.listener(this::notifySessionClosed));
java/src/org/openqa/selenium/grid/node/local/SessionSlot.java [171]
    bus.fire(new SessionClosedEvent(closedData));

Solution Walkthrough:

Before:

class LocalNode {
  LocalNode(...) {
    // ...
    if (!lifecycleListeners.isEmpty()) {
      // Listen to our own event to notify internal listeners
      bus.addListener(SessionClosedEvent.listener(this::notifySessionClosed));
    }
  }

  private void stopTimedOutSession(...) {
    // ...
    slot.stop(closeReason, getId(), externalUri);
  }
  // ...
}

class SessionSlot {
  public void stop(SessionClosedReason reason, NodeId nodeId, URI nodeUri) {
    // ...
    bus.fire(new SessionClosedEvent(closedData));
  }
}

After:

class LocalNode {
  LocalNode(...) {
    // ...
    // No longer need to listen on the bus for session closed events
  }

  private void stopTimedOutSession(...) {
    // ...
    // Stop the session and notify listeners directly
    slot.stop(closeReason, getId(), externalUri, this::notifySessionClosed);
  }
  // ...
}

class SessionSlot {
  public void stop(SessionClosedReason reason, NodeId nodeId, URI nodeUri, Consumer<SessionClosedData> notifier) {
    // ...
    SessionClosedData closedData = new SessionClosedData(...);
    bus.fire(new SessionClosedEvent(closedData));
    notifier.accept(closedData); // Notify listeners directly
  }
}
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies an unnecessary event bus round-trip for notifying internal SPI listeners, and proposes a simplification that aligns with how onSessionCreated and onSessionEvent notifications are handled, improving code consistency and reducing complexity.

Medium
  • Update

@VietND96 VietND96 force-pushed the node-event-driven-service branch 4 times, most recently from 9fe699c to a6a0863 Compare January 28, 2026 23:17
Copilot AI review requested due to automatic review settings February 1, 2026 06:57
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces a cross-language, event-driven session lifecycle mechanism for Selenium Grid nodes, plus client-side APIs to fire custom session events, and adjusts Grid event bus defaults and node behavior to support sidecar services.

Changes:

  • Add session-created, session-closed, and session-event data/event types plus SessionLifecycleListener to let Grid nodes publish rich session lifecycle data to both SPI listeners and the EventBus.
  • Expose a fireSessionEvent client API in Java, Python, Ruby, and .NET bindings with matching /session/{sessionId}/se/event wiring across all transports and node implementations.
  • Refine Grid server internals (LocalNode, SessionSlot, RemoteNode, EventBusOptions, Standalone config) to emit enriched closure events, avoid deadlocks, and choose an appropriate EventBus implementation for standalone vs. distributed setups.

Reviewed changes

Copilot reviewed 35 out of 35 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
rb/spec/unit/selenium/webdriver/remote/features_spec.rb Adds unit tests covering Ruby Remote::Features#fire_session_event behavior and command registration.
rb/lib/selenium/webdriver/remote/features.rb Extends remote features to register and implement the fire_session_event command mapped to /session/:session_id/se/event.
rb/lib/selenium/webdriver/remote/driver.rb Mixes in HasSessionEvents so Ruby remote drivers expose #fire_session_event.
rb/lib/selenium/webdriver/common/driver_extensions/has_session_events.rb Introduces Ruby driver extension delegating fire_session_event from the driver to the remote bridge.
rb/lib/selenium/webdriver/common.rb Wires the new Ruby driver extension into the common driver bootstrap requires.
py/test/unit/selenium/webdriver/remote/fire_session_event_tests.py Adds Python unit tests ensuring the new command constant, execution wiring, and payload handling for fire_session_event are correct.
py/test/selenium/webdriver/remote/remote_session_event_tests.py Adds Python Grid integration tests validating driver.fire_session_event end-to-end with and without payloads and with complex payloads.
py/selenium/webdriver/remote/webdriver.py Adds WebDriver.fire_session_event with type hints, argument validation at the wire level, and return-value unwrapping from the W3C command response.
py/selenium/webdriver/remote/remote_connection.py Registers the FIRE_SESSION_EVENT command to the /session/$sessionId/se/event HTTP endpoint in the Python remote connection.
py/selenium/webdriver/remote/command.py Introduces the FIRE_SESSION_EVENT command constant for Python remote WebDriver.
java/test/org/openqa/selenium/testing/drivers/OutOfProcessSeleniumServer.java Increases Selenium server startup wait timeout from 10s to 20s to make Grid-based tests more robust.
java/test/org/openqa/selenium/remote/RemoteWebDriverUnitTest.java Adds Java unit tests for RemoteWebDriver.fireSessionEvent (with/without payload and null eventType validation).
java/test/org/openqa/selenium/grid/node/NodeTest.java Adds Grid node tests covering the /session/{id}/se/event endpoint, event bus delivery, error cases, and empty-payload semantics.
java/src/org/openqa/selenium/remote/codec/AbstractHttpCommandCodec.java Defines the HTTP wire mapping for FIRE_SESSION_EVENT to POST /session/:sessionId/se/event in the Java HTTP command codec.
java/src/org/openqa/selenium/remote/RemoteWebDriver.java Adds overloaded fireSessionEvent methods that build the command payload via DriverCommand.FIRE_SESSION_EVENT and return the server’s response map.
java/src/org/openqa/selenium/remote/DriverCommand.java Adds the FIRE_SESSION_EVENT command name and a helper to construct its payload with optional payload field.
java/src/org/openqa/selenium/grid/server/EventBusOptions.java Switches EventBus defaulting logic to prefer GuavaEventBus for standalone (no endpoints) and ZeroMqEventBus when publish/subscribe endpoints are configured.
java/src/org/openqa/selenium/grid/node/remote/RemoteNode.java Forwards fireSessionEvent requests directly to the underlying HTTP client for remote nodes.
java/src/org/openqa/selenium/grid/node/local/SessionSlot.java Extends stop to accept node context and timing and fires SessionClosedEvent with rich SessionClosedData.
java/src/org/openqa/selenium/grid/node/local/LocalNode.java Emits SessionCreatedEvent/SessionEvent, loads SessionLifecycleListeners via ServiceLoader, forwards lifecycle events to them, and routes /se/event requests while including node context.
java/src/org/openqa/selenium/grid/node/SessionLifecycleListener.java Introduces an SPI for sidecar services to receive session created/closed and custom session events with full context.
java/src/org/openqa/selenium/grid/node/Node.java Registers the /session/{sessionId}/se/event route and adds a default fireSessionEvent hook for subclasses to override.
java/src/org/openqa/selenium/grid/node/FireSessionEvent.java Implements the HTTP handler that delegates /se/event requests from the router to the node’s fireSessionEvent method.
java/src/org/openqa/selenium/grid/data/SessionEventData.java Defines the payload-carrying data structure for user-defined session events, including node context, timestamp, validation, and JSON (de)serialization.
java/src/org/openqa/selenium/grid/data/SessionEvent.java Adds the corresponding EventBus event type and convenience listener factories for SessionEventData.
java/src/org/openqa/selenium/grid/data/SessionCreatedEvent.java Adds an event wrapper for SessionCreatedData plus listeners that receive full context or just the SessionId.
java/src/org/openqa/selenium/grid/data/SessionCreatedData.java Introduces a rich session-created data object (session, node, URIs, capabilities, stereotype, timing) with JSON support.
java/src/org/openqa/selenium/grid/data/SessionClosedEvent.java Extends the session-closed event to accept SessionClosedData while keeping the original constructors for backward compatibility.
java/src/org/openqa/selenium/grid/data/SessionClosedData.java Expands closed-session data to include node info, capabilities, timing, and duration plus a new JSON schema, while keeping a backwards-compatible constructor.
java/src/org/openqa/selenium/grid/commands/Standalone.java Marks the EVENT_BUS_ROLE as configurable for the standalone Grid command, ensuring event bus options can be customized.
java/src/org/openqa/selenium/grid/commands/DefaultStandaloneConfig.java Removes the hardcoded EventBus implementation from standalone defaults so EventBusOptions can select it dynamically.
dotnet/test/common/SessionEventTest.cs Adds .NET tests for firing session events with and without payload, argument validation, and ensuring the command constant is known.
dotnet/src/webdriver/Remote/W3CWireProtocolCommandInfoRepository.cs Wires the .NET FireSessionEvent command to POST /session/{sessionId}/se/event in the W3C command repository.
dotnet/src/webdriver/Remote/RemoteWebDriver.cs Adds FireSessionEvent methods for .NET RemoteWebDriver, including validation and response shape checks mirroring other custom commands.
dotnet/src/webdriver/DriverCommand.cs Declares the .NET FireSessionEvent command name and adds it to the set of known commands.

Comment thread py/test/unit/selenium/webdriver/remote/fire_session_event_tests.py Outdated
Comment thread py/test/selenium/webdriver/remote/remote_session_event_tests.py Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 35 out of 35 changed files in this pull request and generated 2 comments.

Comment thread java/src/org/openqa/selenium/grid/node/local/LocalNode.java Outdated
Comment thread dotnet/test/common/SessionEventTest.cs Outdated
Copilot AI review requested due to automatic review settings February 1, 2026 13:03
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 40 out of 40 changed files in this pull request and generated 2 comments.

Comment thread java/src/org/openqa/selenium/grid/data/SessionClosedData.java
Comment thread dotnet/src/webdriver/Remote/RemoteWebDriver.cs
@VietND96 VietND96 force-pushed the node-event-driven-service branch from 1e4fa65 to 971a6ac Compare February 1, 2026 14:29
Copilot AI review requested due to automatic review settings February 1, 2026 15:01
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 43 out of 43 changed files in this pull request and generated no new comments.

@VietND96 VietND96 force-pushed the node-event-driven-service branch from a0a3c51 to ad61b71 Compare February 1, 2026 15:52
Copilot AI review requested due to automatic review settings February 1, 2026 17:44
@VietND96 VietND96 force-pushed the node-event-driven-service branch from ad61b71 to 756d96e Compare February 1, 2026 17:44
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 54 out of 54 changed files in this pull request and generated 2 comments.

Comment thread java/src/org/openqa/selenium/json/JsonTypeCoercer.java
Comment thread javascript/selenium-webdriver/test/remote_test.js
@VietND96 VietND96 force-pushed the node-event-driven-service branch from 756d96e to 48a8c52 Compare February 1, 2026 18:40
@VietND96
Copy link
Copy Markdown
Member Author

VietND96 commented Feb 1, 2026

/review

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented Feb 1, 2026

PR Reviewer Guide 🔍

(Review updated until commit 2fd125c)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 PR contains tests
🔒 Security concerns

Potential abuse/DoS vector:
The new endpoint POST /session/{sessionId}/se/event accepts arbitrary JSON payload and forwards it onto the EventBus. If a node endpoint is exposed to untrusted clients, this can be used to spam the bus with high-frequency events or very large payloads, potentially causing memory/CPU pressure in-process and in any listeners. Consider adding safeguards such as payload size limits, stricter schema validation, and/or ensuring the endpoint is protected consistently with other sensitive node endpoints (e.g., secret/auth gating where applicable).

⚡ Recommended focus areas for review

Input Validation

The new session-event endpoint parses arbitrary JSON into a raw map and performs an unchecked cast for payload. Consider validating the incoming JSON shape and types more defensively (e.g., ensure payload is an object/map when present, reject non-object payloads instead of silently converting to empty), and consider consistent validation of eventType (length/character set) at the endpoint boundary to avoid surprising runtime errors and to provide clearer client-facing error messages.

@Override
public HttpResponse fireSessionEvent(HttpRequest req, SessionId id) {
  Require.nonNull("Session ID", id);

  // Verify session exists
  SessionSlot slot = currentSessions.getIfPresent(id);
  if (slot == null) {
    throw new NoSuchSessionException("Cannot find session with id: " + id);
  }

  // Parse the event data from request
  Map<String, Object> incoming = JSON.toType(req.contentAsString(), Json.MAP_TYPE);
  String eventType = (String) incoming.get("eventType");
  if (eventType == null || eventType.isEmpty()) {
    throw new WebDriverException(
        "Event type is required. Please provide 'eventType' in payload.");
  }

  Object rawPayload = incoming.get("payload");
  Map<String, Object> payload =
      (rawPayload instanceof Map) ? (Map<String, Object>) rawPayload : Map.of();

  // Create event data with node context
  SessionEventData eventData =
      SessionEventData.create(id, eventType, payload).withNodeContext(getId(), externalUri);

  // Fire event via EventBus for sidecar services
  bus.fire(new SessionEvent(eventData));

  LOG.log(
      Level.FINE,
      () -> String.format("Session event fired: type=%s, sessionId=%s", eventType, id));

  // Return success response
  Map<String, Object> responseData =
      Map.of(
          "success",
          true,
          "eventType",
          eventType,
          "timestamp",
          eventData.getTimestamp().toString());
  Map<String, Object> result = Map.of("value", responseData);
  return new HttpResponse().setContent(asJson(result));
}
JSON Coercion

The JSON deserialization logic constructs SessionCreatedData even if required fields are absent, relying on Require.nonNull to fail. Consider explicitly validating required fields in fromJson and throwing a JsonException with a clear message, matching the pattern used in SessionClosedData, so failures are deterministic and easier to diagnose.

private static SessionCreatedData fromJson(JsonInput input) {
  SessionId sessionId = null;
  NodeId nodeId = null;
  URI nodeUri = null;
  URI sessionUri = null;
  Capabilities capabilities = null;
  Capabilities stereotype = null;
  Instant startTime = null;

  input.beginObject();
  while (input.hasNext()) {
    switch (input.nextName()) {
      case "sessionId":
        sessionId = input.read(SessionId.class);
        break;
      case "nodeId":
        nodeId = input.read(NodeId.class);
        break;
      case "nodeUri":
        nodeUri = input.read(URI.class);
        break;
      case "sessionUri":
        sessionUri = input.read(URI.class);
        break;
      case "capabilities":
        capabilities = input.read(Capabilities.class);
        break;
      case "stereotype":
        stereotype = input.read(Capabilities.class);
        break;
      case "startTime":
        startTime = input.read(Instant.class);
        break;
      default:
        input.skipValue();
        break;
    }
  }
  input.endObject();

  return new SessionCreatedData(
      sessionId, nodeId, nodeUri, sessionUri, capabilities, stereotype, startTime);
}
Behavior Change

The default EventBus implementation now switches between Guava and ZeroMQ based on the presence of publish/subscribe configuration. This is a semantic change that could affect deployments that previously relied on ZeroMQ being the default without explicit configuration. Confirm this matches intended upgrade behavior and ensure docs/release notes cover it, especially for distributed setups.

private EventBus createBus() {
  // Determine default implementation based on EventBus configuration:
  // - If publish/subscribe endpoints are configured: use ZeroMqEventBus (distributed mode)
  // - Otherwise: use GuavaEventBus (standalone mode, in-process, more efficient)
  boolean hasZmqConfig =
      config.get(EVENTS_SECTION, "publish").isPresent()
          || config.get(EVENTS_SECTION, "subscribe").isPresent();
  String defaultClass = hasZmqConfig ? ZEROMQ_BUS_CLASS : GUAVA_BUS_CLASS;
  return config.getClass(EVENTS_SECTION, "implementation", EventBus.class, defaultClass);
}

Copilot AI review requested due to automatic review settings February 1, 2026 19:29
@VietND96 VietND96 force-pushed the node-event-driven-service branch from 48a8c52 to c28c6a7 Compare February 1, 2026 19:29
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 59 out of 59 changed files in this pull request and generated 1 comment.

Comment thread dotnet/test/common/Environment/RemoteSeleniumServer.cs
@VietND96 VietND96 force-pushed the node-event-driven-service branch 2 times, most recently from 3abaccd to e8de126 Compare February 1, 2026 21:26
Copilot AI review requested due to automatic review settings February 1, 2026 21:26
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 59 out of 59 changed files in this pull request and generated 2 comments.

Comment thread java/src/org/openqa/selenium/grid/data/SessionClosedData.java
Comment thread java/src/org/openqa/selenium/json/InstantCoercer.java Outdated
@VietND96 VietND96 force-pushed the node-event-driven-service branch from e8de126 to 1e6638d Compare February 1, 2026 22:18
Copilot AI review requested due to automatic review settings February 1, 2026 22:20
@VietND96 VietND96 force-pushed the node-event-driven-service branch from 1e6638d to 08f039a Compare February 1, 2026 22:20
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 52 out of 52 changed files in this pull request and generated 1 comment.

Comment thread java/src/org/openqa/selenium/grid/node/Node.java Outdated
Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
@VietND96 VietND96 force-pushed the node-event-driven-service branch from 45d4edb to de97913 Compare February 1, 2026 23:56
@VietND96
Copy link
Copy Markdown
Member Author

VietND96 commented Feb 2, 2026

/review

@qodo-code-review
Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 2fd125c

@VietND96 VietND96 changed the title [grid] Node support event-driven for sidecar service to consume [grid] Add session event API for server-side event bus integration Feb 2, 2026
@VietND96 VietND96 merged commit 0d3c440 into trunk Feb 2, 2026
56 checks passed
@VietND96 VietND96 deleted the node-event-driven-service branch February 2, 2026 01:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-grid Everything grid and server related C-java Java Bindings Possible security concern Review effort 4/5

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants