[grid] Tracking SessionRemovalInfo when removing session from SessionMap

[VietND96]

User description

🔗 Related Issues

💥 What does this PR do?

SeleniumHQ/docker-selenium#3025 - Selenium Grid throwing NoSuchSessionException – Session ID not found

  selenium.common.exceptions.InvalidSessionIdException: Message: Unable to find session with ID: 34ecd431-ae49-487d-b1ef-bef0fe750dd4
  Build info: version: '4.39.0-SNAPSHOT', revision: 'e0afdd3'
  System info: os.name: 'Linux', os.arch: 'amd64', os.version: '6.11.0-1018-azure', java.version: '21.0.8'
  Driver info: driver.version: unknown; For documentation on this error, please visit: https://www.selenium.dev/documentation/webdriver/troubleshooting/errors#invalidsessionidexception
  Stacktrace:
  org.openqa.selenium.NoSuchSessionException: Unable to find session with ID: 34ecd431-ae49-487d-b1ef-bef0fe750dd4
  Build info: version: '4.39.0-SNAPSHOT', revision: 'e0afdd3'
  System info: os.name: 'Linux', os.arch: 'amd64', os.version: '6.11.0-1018-azure', java.version: '21.0.8'
  Driver info: driver.version: unknown
  	at org.openqa.selenium.grid.sessionmap.local.LocalSessionMap.get(LocalSessionMap.java:119)
  	at org.openqa.selenium.grid.sessionmap.SessionMap.getUri(SessionMap.java:84)
  	at org.openqa.selenium.grid.router.HandleSession.lambda$loadSessionId$4(HandleSession.java:231)
  	at org.openqa.selenium.grid.router.HandleSession.execute(HandleSession.java:188)
  	at org.openqa.selenium.remote.http.Route$PredicatedRoute.handle(Route.java:398)
  	at org.openqa.selenium.remote.http.Route.execute(Route.java:69)
  	at org.openqa.selenium.remote.http.Route$CombinedRoute.handle(Route.java:361)
  	at org.openqa.selenium.remote.http.Route.execute(Route.java:69)
  	at org.openqa.selenium.grid.router.Router.execute(Router.java:89)
  	at org.openqa.selenium.grid.web.EnsureSpecCompliantResponseHeaders.lambda$apply$0(EnsureSpecCompliantResponseHeaders.java:34)
  	at org.openqa.selenium.remote.http.Filter$1.execute(Filter.java:63)
  	at org.openqa.selenium.remote.http.Route$CombinedRoute.handle(Route.java:361)
  	at org.openqa.selenium.remote.http.Route.execute(Route.java:69)
  	at org.openqa.selenium.remote.AddWebDriverSpecHeaders.lambda$apply$0(AddWebDriverSpecHeaders.java:35)
  	at org.openqa.selenium.remote.ErrorFilter.lambda$apply$0(ErrorFilter.java:44)
  	at org.openqa.selenium.remote.http.Filter$1.execute(Filter.java:63)
  	at org.openqa.selenium.remote.ErrorFilter.lambda$apply$0(ErrorFilter.java:44)
  	at org.openqa.selenium.remote.http.Filter$1.execute(Filter.java:63)
  	at org.openqa.selenium.netty.server.SeleniumHandler.lambda$channelRead0$0(SeleniumHandler.java:49)
  	at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:572)
  	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:317)
  	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144)
  	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642)
  	at java.base/java.lang.Thread.run(Thread.java:1583)

There are 3 events to trigger the removal of the session from LocalSessionMap. Possibility session closed on demand (QUIT command) or timed out, Node restarted, or Node removed (detach from grid and shutdown)

Based on triggered event, reason will be attached with closed timestamp for more info instead of a common message

org.openqa.selenium.NoSuchSessionException: Unable to find session with ID: 1806b579-ae67-4525-a815-570bbdc74af1. Session was removed at 2025-12-02T02:47:55.132574294Z (2 seconds ago), reason: node was removed from the grid, node: http://172.19.0.13:5555

tests-1  | Build info: version: '4.39.0-SNAPSHOT', revision: 'Unknown'
tests-1  | System info: os.name: 'Linux', os.arch: 'aarch64', os.version: '6.12.54-linuxkit', java.version: '21.0.8'
tests-1  | Driver info: driver.version: unknown; For documentation on this error, please visit: https://www.selenium.dev/documentation/webdriver/troubleshooting/errors#invalidsessionidexception
tests-1  | Stacktrace:
tests-1  | org.openqa.selenium.NoSuchSessionException: Unable to find session with ID: 1806b579-ae67-4525-a815-570bbdc74af1. Session was removed at 2025-12-02T02:47:55.132574294Z (2 seconds ago), reason: node was removed from the grid, node: http://172.19.0.13:5555
tests-1  | Build info: version: '4.39.0-SNAPSHOT', revision: 'Unknown'
tests-1  | System info: os.name: 'Linux', os.arch: 'aarch64', os.version: '6.12.54-linuxkit', java.version: '21.0.8'
tests-1  | Driver info: driver.version: unknown
tests-1  |      at org.openqa.selenium.grid.sessionmap.local.LocalSessionMap.get(LocalSessionMap.java:135)
tests-1  |      at org.openqa.selenium.grid.sessionmap.SessionMap.getUri(SessionMap.java:84)
tests-1  |      at org.openqa.selenium.grid.router.HandleSession.lambda$loadSessionId$4(HandleSession.java:231)
tests-1  |      at io.opentelemetry.context.Context.lambda$wrap$2(Context.java:253)
tests-1  |      at org.openqa.selenium.grid.router.HandleSession.execute(HandleSession.java:188)
tests-1  |      at org.openqa.selenium.remote.http.Route$PredicatedRoute.handle(Route.java:398)
tests-1  |      at org.openqa.selenium.remote.http.Route.execute(Route.java:69)
tests-1  |      at org.openqa.selenium.remote.http.Route$CombinedRoute.handle(Route.java:361)
tests-1  |      at org.openqa.selenium.remote.http.Route.execute(Route.java:69)
tests-1  |      at org.openqa.selenium.grid.router.Router.execute(Router.java:89)
tests-1  |      at org.openqa.selenium.grid.web.EnsureSpecCompliantResponseHeaders.lambda$apply$0(EnsureSpecCompliantResponseHeaders.java:34)
tests-1  |      at org.openqa.selenium.remote.http.Filter$1.execute(Filter.java:63)
tests-1  |      at org.openqa.selenium.remote.http.Route$CombinedRoute.handle(Route.java:361)
tests-1  |      at org.openqa.selenium.remote.http.Route.execute(Route.java:69)
tests-1  |      at org.openqa.selenium.remote.AddWebDriverSpecHeaders.lambda$apply$0(AddWebDriverSpecHeaders.java:35)
tests-1  |      at org.openqa.selenium.remote.ErrorFilter.lambda$apply$0(ErrorFilter.java:44)
tests-1  |      at org.openqa.selenium.remote.http.Filter$1.execute(Filter.java:63)
tests-1  |      at org.openqa.selenium.remote.ErrorFilter.lambda$apply$0(ErrorFilter.java:44)
tests-1  |      at org.openqa.selenium.remote.http.Filter$1.execute(Filter.java:63)
tests-1  |      at org.openqa.selenium.netty.server.SeleniumHandler.lambda$channelRead0$0(SeleniumHandler.java:49)
tests-1  |      at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:572)
tests-1  |      at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:317)
tests-1  |      at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144)
tests-1  |      at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642)
tests-1  |      at java.base/java.lang.Thread.run(Thread.java:1583)

🔧 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

• Add session closure reason tracking throughout grid lifecycle

• Create SessionClosedReason enum with four closure scenarios

• Track recently removed sessions with removal info for diagnostics

• Enhance NoSuchSessionException with detailed closure reasons

• Update SessionId to carry close reason metadata

---

Diagram Walkthrough

flowchart LR
  A["Session Closed"] --> B["SessionClosedReason Enum"]
  B --> C["SessionClosedEvent"]
  C --> D["SessionId.setCloseReason"]
  D --> E["LocalSessionMap"]
  E --> F["SessionRemovalInfo Cache"]
  F --> G["NoSuchSessionException with Details"]

Loading

File Walkthrough

	Relevant files
Enhancement	8 files SessionClosedReason.java New enum for session closure reasons                                          +39/-0    SessionRemovalInfo.java New class to track session removal details                              +45/-0    SessionClosedEvent.java Add reason parameter to SessionClosedEvent                              +15/-1    SessionId.java Add close reason tracking to SessionId                                      +49/-3    LocalNode.java Pass SessionClosedReason to slot stop method                          +8/-2      SessionSlot.java Accept and propagate SessionClosedReason                                  +7/-2      LocalSessionMap.java Track removed sessions with Caffeine cache                              +61/-13  RemoteSessionMap.java Include close reason in exception messages                              +6/-0
Tests	1 files LocalSessionMapTest.java Add tests for session closure reasons                                        +30/-5
Dependencies	1 files BUILD.bazel Add Caffeine cache dependency                                                        +2/-0

---

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Serialization change risk Description: Changing SessionId.toJson() to sometimes emit a Map with "value" and "closeReason" instead of a String may break consumers expecting a plain string, potentially enabling type confusion or injection in JSON/URL contexts where unvalidated structure is used. SessionId.java [83-99] Referred Code public boolean equals(@Nullable Object obj) { return obj instanceof SessionId && opaqueKey.equals(((SessionId) obj).opaqueKey); } private Object toJson() { // For backward compatibility, serialize as string when there's no closeReason // This ensures SessionId works properly in URLs and simple contexts if (closeReason == null) { return opaqueKey; } // When there is a closeReason, serialize as Map to preserve the metadata Map<String, Object> json = new HashMap<>(); json.put("value", opaqueKey); json.put("closeReason", closeReason); return json; }
	Sensitive information exposure Description: recentlyRemovedSessions caches session removal info for 60 minutes including node URI; if node URIs include credentials or internal hosts, this may expose sensitive metadata in memory and logs. LocalSessionMap.java [66-68] Referred Code private final Cache<SessionId, SessionRemovalInfo> recentlyRemovedSessions = Caffeine.newBuilder().expireAfterWrite(Duration.ofMinutes(60)).build();
Ticket Compliance
🟡	🎫 #1234 🔴 Investigate and fix a regression where click() on a link with JavaScript in href no longer triggers the script in Selenium 2.48.x (worked in 2.47.1) on Firefox 42. Provide a solution or workaround ensuring alerts are triggered as before.
🟡	🎫 #5678 🔴 Address "Error: ConnectFailure (Connection refused)" occurring when instantiating multiple ChromeDriver instances on Ubuntu 16.04.4 with Chrome 65 / ChromeDriver 2.35 and Selenium 3.9.0. Ensure subsequent ChromeDriver instantiations do not fail after the first one.
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	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: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation 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: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities 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: Error Detail Exposure: New NoSuchSessionException messages include node URIs and detailed reasons which may be user-facing in some contexts and could expose internal details. Referred Code public Session get(SessionId id) { Require.nonNull("Session ID", id); Session session = knownSessions.get(id); if (session == null) { // Check if this session was recently removed and provide detailed information SessionRemovalInfo removalInfo = recentlyRemovedSessions.getIfPresent(id); if (removalInfo != null) { throw new NoSuchSessionException( String.format("Unable to find session with ID: %s. Session was %s", id, removalInfo)); } // Check if the SessionId itself carries a close reason String closeReason = id.getCloseReason(); if (closeReason != null) { throw new NoSuchSessionException( String.format("Unable to find session with ID: %s. Reason: %s", id, closeReason)); } throw new NoSuchSessionException("Unable to find session with ID: " + id); } ... (clipped 37 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]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	✅ Avoid modifying the core SessionId class Suggestion Impact: The commit removed closeReason state and related methods from SessionId and reverted its JSON serialization to always return the opaque string. It also changed SessionClosedEvent to carry a separate SessionClosedData object instead of mutating SessionId, and adjusted listeners accordingly—aligning with the suggested approach. code diff: # File: java/src/org/openqa/selenium/remote/SessionId.java @@ -18,8 +18,6 @@ package org.openqa.selenium.remote; import java.io.Serializable; -import java.util.HashMap; -import java.util.Map; import java.util.UUID; import org.jspecify.annotations.NullMarked; import org.jspecify.annotations.Nullable; @@ -30,7 +28,6 @@ public class SessionId implements Serializable { private final String opaqueKey; - private @Nullable String closeReason; public SessionId(UUID uuid) { this(Require.nonNull("Session ID key", uuid).toString()); @@ -38,35 +35,6 @@ public SessionId(String opaqueKey) { this.opaqueKey = Require.nonNull("Session ID key", opaqueKey); - this.closeReason = null; // Session is alive initially - } - - /** - * Sets the reason why this session was closed. Once set, indicates the session is no longer - * active. - * - * @param reason The reason for session closure - */ - public void setCloseReason(String reason) { - this.closeReason = reason; - } - - /** - * Gets the reason why this session was closed, if any. - * - * @return The close reason, or null if the session is still active - */ - public @Nullable String getCloseReason() { - return closeReason; - } - - /** - * Checks if this session has been closed. - * - * @return true if the session has a close reason set, false otherwise - */ - public boolean isClosed() { - return closeReason != null; } @Override @@ -85,17 +53,7 @@ } private Object toJson() { - // For backward compatibility, serialize as string when there's no closeReason - // This ensures SessionId works properly in URLs and simple contexts - if (closeReason == null) { - return opaqueKey; - } - - // When there is a closeReason, serialize as Map to preserve the metadata - Map<String, Object> json = new HashMap<>(); - json.put("value", opaqueKey); - json.put("closeReason", closeReason); - return json; + return opaqueKey; } private static SessionId fromJson(Object raw) { @@ -103,18 +61,6 @@ return new SessionId(String.valueOf(raw)); } - if (raw instanceof Map) { - Map<?, ?> map = (Map<?, ?>) raw; - if (map.get("value") instanceof String) { - SessionId sessionId = new SessionId(String.valueOf(map.get("value"))); - // Restore closeReason if present - if (map.get("closeReason") instanceof String) { - sessionId.setCloseReason(String.valueOf(map.get("closeReason"))); - } - return sessionId; - } - } - throw new JsonException("Unable to coerce session id from " + raw); } } # File: java/src/org/openqa/selenium/grid/data/SessionClosedEvent.java @@ -34,21 +34,23 @@ } public SessionClosedEvent(SessionId id, SessionClosedReason reason) { - super(SESSION_CLOSED, markSessionId(id, reason)); + super(SESSION_CLOSED, new SessionClosedData(id, reason)); Require.nonNull("Session ID", id); Require.nonNull("Reason", reason); } - // Helper method to mark the SessionId before passing to Event constructor - private static SessionId markSessionId(SessionId id, SessionClosedReason reason) { - id.setCloseReason(reason.getReasonText()); - return id; + // Standard listener method that provides access to both SessionId and reason + public static EventListener<SessionClosedData> listener(Consumer<SessionClosedData> handler) { + Require.nonNull("Handler", handler); + + return new EventListener<>(SESSION_CLOSED, SessionClosedData.class, handler); } - // Standard listener method following the Event pattern - public static EventListener<SessionId> listener(Consumer<SessionId> handler) { + // Convenience method for listeners that only care about the SessionId + public static EventListener<SessionClosedData> sessionListener(Consumer<SessionId> handler) { Require.nonNull("Handler", handler); - return new EventListener<>(SESSION_CLOSED, SessionId.class, handler); + return new EventListener<>( + SESSION_CLOSED, SessionClosedData.class, data -> handler.accept(data.getSessionId())); } } Avoid modifying the core SessionId class to include session closure state and changing its JSON format. Instead, manage the closure reason separately to prevent a significant breaking change. Examples: java/src/org/openqa/selenium/remote/SessionId.java [33-115] private @Nullable String closeReason; public SessionId(UUID uuid) { this(Require.nonNull("Session ID key", uuid).toString()); } public SessionId(String opaqueKey) { this.opaqueKey = Require.nonNull("Session ID key", opaqueKey); this.closeReason = null; // Session is alive initially } ... (clipped 73 lines) java/src/org/openqa/selenium/grid/data/SessionClosedEvent.java [36-46] public SessionClosedEvent(SessionId id, SessionClosedReason reason) { super(SESSION_CLOSED, markSessionId(id, reason)); Require.nonNull("Session ID", id); Require.nonNull("Reason", reason); } // Helper method to mark the SessionId before passing to Event constructor private static SessionId markSessionId(SessionId id, SessionClosedReason reason) { id.setCloseReason(reason.getReasonText()); return id; ... (clipped 1 lines) Solution Walkthrough: Before: // In SessionId.java class SessionId { private final String opaqueKey; private @Nullable String closeReason; public void setCloseReason(String reason) { ... } private Object toJson() { if (closeReason == null) { return opaqueKey; // Returns a string } // Returns a map, which is a breaking change return Map.of("value", opaqueKey, "closeReason", closeReason); } } // In SessionClosedEvent.java class SessionClosedEvent extends Event { public SessionClosedEvent(SessionId id, SessionClosedReason reason) { super(SESSION_CLOSED, markSessionId(id, reason)); // Modifies SessionId state } } After: // In SessionId.java (reverted to original state) class SessionId { private final String opaqueKey; private String toJson() { return opaqueKey; // Always returns a string } } // In a new file, e.g., SessionClosure.java class SessionClosure { private final SessionId sessionId; private final SessionClosedReason reason; } // In SessionClosedEvent.java class SessionClosedEvent extends Event { public SessionClosedEvent(SessionId id, SessionClosedReason reason) { super(SESSION_CLOSED, new SessionClosure(id, reason)); // Pass a dedicated object } } Suggestion importance[1-10]: 9 __ Why: This suggestion correctly identifies a significant, potentially breaking change to the core SessionId class and its JSON serialization, proposing a safer, non-breaking alternative that achieves the same goal.	High
Learned best practice	✅ Make SessionId immutable Suggestion Impact: The commit removed the mutable closeReason field, its setter/getter/isClosed methods, and the Map-based JSON handling, simplifying to serialize as a plain string. This aligns with making SessionId immutable and avoiding external mutation of closeReason. code diff: public class SessionId implements Serializable { private final String opaqueKey; - private @Nullable String closeReason; public SessionId(UUID uuid) { this(Require.nonNull("Session ID key", uuid).toString()); @@ -38,35 +35,6 @@ public SessionId(String opaqueKey) { this.opaqueKey = Require.nonNull("Session ID key", opaqueKey); - this.closeReason = null; // Session is alive initially - } - - /** - * Sets the reason why this session was closed. Once set, indicates the session is no longer - * active. - * - * @param reason The reason for session closure - */ - public void setCloseReason(String reason) { - this.closeReason = reason; - } - - /** - * Gets the reason why this session was closed, if any. - * - * @return The close reason, or null if the session is still active - */ - public @Nullable String getCloseReason() { - return closeReason; - } - - /** - * Checks if this session has been closed. - * - * @return true if the session has a close reason set, false otherwise - */ - public boolean isClosed() { - return closeReason != null; } @Override @@ -85,17 +53,7 @@ } private Object toJson() { - // For backward compatibility, serialize as string when there's no closeReason - // This ensures SessionId works properly in URLs and simple contexts - if (closeReason == null) { - return opaqueKey; - } - - // When there is a closeReason, serialize as Map to preserve the metadata - Map<String, Object> json = new HashMap<>(); - json.put("value", opaqueKey); - json.put("closeReason", closeReason); - return json; + return opaqueKey; } private static SessionId fromJson(Object raw) { @@ -103,18 +61,6 @@ return new SessionId(String.valueOf(raw)); } - if (raw instanceof Map) { - Map<?, ?> map = (Map<?, ?>) raw; - if (map.get("value") instanceof String) { - SessionId sessionId = new SessionId(String.valueOf(map.get("value"))); - // Restore closeReason if present - if (map.get("closeReason") instanceof String) { - sessionId.setCloseReason(String.valueOf(map.get("closeReason"))); - } - return sessionId; - } - } - throw new JsonException("Unable to coerce session id from " + raw); } Avoid making SessionId mutable and externally settable; carry close reason separately or return a defensive copy during serialization to prevent unintended mutation and cross-component coupling. java/src/org/openqa/selenium/remote/SessionId.java [30-120] -public class SessionId implements Serializable { +public final class SessionId implements Serializable { private final String opaqueKey; - private @Nullable String closeReason; -... - public void setCloseReason(String reason) { - this.closeReason = reason; + private final @Nullable String closeReason; + + public SessionId(UUID uuid) { + this(Require.nonNull("Session ID key", uuid).toString(), null); } -... + + public SessionId(String opaqueKey) { + this(opaqueKey, null); + } + + private SessionId(String opaqueKey, @Nullable String closeReason) { + this.opaqueKey = Require.nonNull("Session ID key", opaqueKey); + this.closeReason = closeReason; + } + + public SessionId withCloseReason(String reason) { + return new SessionId(this.opaqueKey, reason); + } + + public @Nullable String getCloseReason() { + return closeReason; + } + + public boolean isClosed() { + return closeReason != null; + } + private Object toJson() { - // For backward compatibility, serialize as string when there's no closeReason - // This ensures SessionId works properly in URLs and simple contexts if (closeReason == null) { return opaqueKey; } - - // When there is a closeReason, serialize as Map to preserve the metadata Map<String, Object> json = new HashMap<>(); json.put("value", opaqueKey); json.put("closeReason", closeReason); return json; } + private static SessionId fromJson(Object raw) { + if (raw instanceof String) { + return new SessionId(String.valueOf(raw)); + } + if (raw instanceof Map) { + Map<?, ?> map = (Map<?, ?>) raw; + if (map.get("value") instanceof String) { + String value = String.valueOf(map.get("value")); + String reason = map.get("closeReason") instanceof String ? String.valueOf(map.get("closeReason")) : null; + return new SessionId(value, reason); + } + } + throw new JsonException("Unable to coerce session id from " + raw); + } +} + [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Use defensive copying and avoid exposing or mutating shared internal state through objects passed across system boundaries.	Low
	✅ Remove side-effectful mutation Suggestion Impact: The commit removed the side-effectful helper and changed the event payload to a new SessionClosedData object carrying both SessionId and reason, thus avoiding mutation of SessionId and aligning with the suggestion's intent. code diff: public SessionClosedEvent(SessionId id, SessionClosedReason reason) { - super(SESSION_CLOSED, markSessionId(id, reason)); + super(SESSION_CLOSED, new SessionClosedData(id, reason)); Require.nonNull("Session ID", id); Require.nonNull("Reason", reason); } - // Helper method to mark the SessionId before passing to Event constructor - private static SessionId markSessionId(SessionId id, SessionClosedReason reason) { - id.setCloseReason(reason.getReasonText()); - return id; + // Standard listener method that provides access to both SessionId and reason + public static EventListener<SessionClosedData> listener(Consumer<SessionClosedData> handler) { + Require.nonNull("Handler", handler); + + return new EventListener<>(SESSION_CLOSED, SessionClosedData.class, handler); } - // Standard listener method following the Event pattern - public static EventListener<SessionId> listener(Consumer<SessionId> handler) { + // Convenience method for listeners that only care about the SessionId + public static EventListener<SessionClosedData> sessionListener(Consumer<SessionId> handler) { Require.nonNull("Handler", handler); - return new EventListener<>(SESSION_CLOSED, SessionId.class, handler); + return new EventListener<>( + SESSION_CLOSED, SessionClosedData.class, data -> handler.accept(data.getSessionId())); } Avoid mutating SessionId in a helper; instead create a new value or event payload carrying the reason to prevent hidden side effects. java/src/org/openqa/selenium/grid/data/SessionClosedEvent.java [36-46] public SessionClosedEvent(SessionId id, SessionClosedReason reason) { - super(SESSION_CLOSED, markSessionId(id, reason)); Require.nonNull("Session ID", id); Require.nonNull("Reason", reason); + super(SESSION_CLOSED, id.withCloseReason(reason.getReasonText())); } -// Helper method to mark the SessionId before passing to Event constructor -private static SessionId markSessionId(SessionId id, SessionClosedReason reason) { - id.setCloseReason(reason.getReasonText()); - return id; -} - [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 5 __ Why: Relevant best practice - Replace ad-hoc duplication with shared helpers/utilities and avoid side effects in helper methods.	Low
General	✅ Return an immutable set copy Suggestion Impact: The commit updated getSessionsByUri to return an immutable copy by replacing new HashSet<>(result) with Set.copyOf(result), matching the suggestion. code diff: public Set<SessionId> getSessionsByUri(URI uri) { Set<SessionId> result = sessionsByUri.get(uri); - // Return a copy to prevent concurrent modification issues - return (result != null && !result.isEmpty()) ? new HashSet<>(result) : Set.of(); + // Return an immutable copy to prevent concurrent modification issues + return (result != null && !result.isEmpty()) ? Set.copyOf(result) : Set.of(); } In getSessionsByUri, replace new HashSet<>(result) with Set.copyOf(result) to return an immutable copy of the set, which is a safer programming practice. java/src/org/openqa/selenium/grid/sessionmap/local/LocalSessionMap.java [293-297] public Set<SessionId> getSessionsByUri(URI uri) { Set<SessionId> result = sessionsByUri.get(uri); // Return a copy to prevent concurrent modification issues - return (result != null && !result.isEmpty()) ? new HashSet<>(result) : Set.of(); + return (result != null && !result.isEmpty()) ? Set.copyOf(result) : Set.of(); } [Suggestion processed] Suggestion importance[1-10]: 5 __ Why: This is a good suggestion that improves code robustness by returning an immutable collection, which prevents callers from making unintended modifications and aligns with best practices for encapsulation.	Low
Update