Skip to content

[grid] Bundle Redis-backed SessionMap by default#17441

Merged
VietND96 merged 4 commits into
trunkfrom
redis-backed-sessionsmap
May 21, 2026
Merged

[grid] Bundle Redis-backed SessionMap by default#17441
VietND96 merged 4 commits into
trunkfrom
redis-backed-sessionsmap

Conversation

@VietND96
Copy link
Copy Markdown
Member

🔗 Related Issues

💥 What does this PR do?

  • Redis-backed SessionMap is available out of the box with no extra configuration. selenium-session-map-redis was not bundled in selenium-server.jar by default, requiring users to manually manage it on the classpath - unlike distributor/redis, which has been built-in since [grid] Add Distributor Redis-backed implementation as built-in support #17396. Generic is Redis-backed support as default cross components.
  • Bug fix: null startKey in RedisBackedSessionMap.get()
  • Increase test coverage (RedisBackedSessionMapTest.java)

🔧 Implementation Notes

🤖 AI assistance

  • No substantial AI assistance used
  • AI assisted (complete below)
    • Tool(s):
    • What was generated:
    • I reviewed all AI output and can explain the change

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

Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
@selenium-ci selenium-ci added B-grid Everything grid and server related C-java Java Bindings B-build Includes scripting, bazel and CI integrations labels May 12, 2026
@qodo-code-review
Copy link
Copy Markdown
Contributor

Review Summary by Qodo

Bundle Redis-backed SessionMap by default and add configuration support

✨ Enhancement 🐞 Bug fix 🧪 Tests

Grey Divider

Walkthroughs

Description
• Bundle Redis-backed SessionMap in selenium-server.jar by default
• Add configuration flags for sessions scheme and implementation
• Fix null startKey handling in RedisBackedSessionMap.get()
• Expand test coverage with new SessionMapFlags and edge case tests
Diagram
flowchart LR
  A["SessionMapFlags"] -->|"Add scheme & implementation params"| B["Configuration Support"]
  C["RedisBackedSessionMap"] -->|"Fix null startKey bug"| D["Null Safety"]
  E["BUILD.bazel"] -->|"Add redis dependency"| F["Default Bundling"]
  B --> G["Enhanced SessionMap"]
  D --> G
  F --> G
  H["New Tests"] -->|"SessionMapFlags & edge cases"| G
Loading

Grey Divider

File Changes

1. java/src/org/openqa/selenium/grid/sessionmap/config/SessionMapFlags.java ✨ Enhancement +15/-0

Add sessions scheme and implementation configuration parameters

java/src/org/openqa/selenium/grid/sessionmap/config/SessionMapFlags.java


2. java/src/org/openqa/selenium/grid/sessionmap/redis/RedisBackedSessionMap.java 🐞 Bug fix +1/-1

Fix null pointer exception when startKey is missing

java/src/org/openqa/selenium/grid/sessionmap/redis/RedisBackedSessionMap.java


3. java/test/org/openqa/selenium/grid/sessionmap/config/SessionMapFlagsTest.java 🧪 Tests +69/-0

New test suite for SessionMapFlags configuration parameters

java/test/org/openqa/selenium/grid/sessionmap/config/SessionMapFlagsTest.java


View more (6)
4. java/test/org/openqa/selenium/grid/sessionmap/redis/RedisBackedSessionMapTest.java 🧪 Tests +100/-0

Expand test coverage with edge cases and null handling

java/test/org/openqa/selenium/grid/sessionmap/redis/RedisBackedSessionMapTest.java


5. java/src/org/openqa/selenium/grid/BUILD.bazel ⚙️ Configuration changes +1/-0

Add redis sessionmap to selenium_server runtime dependencies

java/src/org/openqa/selenium/grid/BUILD.bazel


6. java/src/org/openqa/selenium/grid/commands/sessionmaps.txt 📝 Documentation +6/-6

Update documentation to reflect default Redis bundling

java/src/org/openqa/selenium/grid/commands/sessionmaps.txt


7. java/src/org/openqa/selenium/grid/sessionmap/config/BUILD.bazel ⚙️ Configuration changes +1/-0

Expose config package visibility for test access

java/src/org/openqa/selenium/grid/sessionmap/config/BUILD.bazel


8. java/test/org/openqa/selenium/grid/sessionmap/config/BUILD.bazel ⚙️ Configuration changes +15/-0

Create new test suite build configuration

java/test/org/openqa/selenium/grid/sessionmap/config/BUILD.bazel


9. java/test/org/openqa/selenium/grid/sessionmap/redis/BUILD.bazel ⚙️ Configuration changes +3/-0

Add test dependencies for config and event bus

java/test/org/openqa/selenium/grid/sessionmap/redis/BUILD.bazel


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 12, 2026

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (2) 📎 Requirement gaps (0)

Context used

Grey Divider


Remediation recommended

1. New Redis tests lack Docker gating 📘 Rule violation ☼ Reliability
Description
The newly added RedisBackedSessionMap tests run unconditionally but rely on Testcontainers/Docker,
which can fail in environments where Docker is unavailable. This risks CI instability because the
tests are not conditionally skipped when the required feature support is missing.
Code

java/test/org/openqa/selenium/grid/sessionmap/redis/RedisBackedSessionMapTest.java[R243-312]

Evidence
PR Compliance ID 15 requires new/updated tests to be gated by feature support. The test setup starts
a Testcontainers GenericContainer (Docker dependency), and the newly added test methods are
executed as part of this suite without any conditional skip/assumption when Docker is unavailable.

java/test/org/openqa/selenium/grid/sessionmap/redis/RedisBackedSessionMapTest.java[58-75]
java/test/org/openqa/selenium/grid/sessionmap/redis/RedisBackedSessionMapTest.java[243-312]
Best Practice: Learned patterns

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Newly added JUnit tests in `RedisBackedSessionMapTest` rely on a Docker-backed Testcontainers Redis instance, but the test class does not conditionally skip when Docker is unavailable.

## Issue Context
These tests start a `GenericContainer` in `@BeforeEach`, so any environment without Docker (or with restricted container execution) will fail during setup rather than being skipped.

## Fix Focus Areas
- java/test/org/openqa/selenium/grid/sessionmap/redis/RedisBackedSessionMapTest.java[58-75]
- java/test/org/openqa/selenium/grid/sessionmap/redis/RedisBackedSessionMapTest.java[243-312]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Silent missing session start 🐞 Bug ◔ Observability
Description
RedisBackedSessionMap.get() silently substitutes Instant.EPOCH when the Redis ":start" key is
absent, making sessions appear to have started in 1970 and masking partial/corrupt session records.
This can mislead GraphQL duration reporting and node ordering logic that relies on session start
time.
Code

java/src/org/openqa/selenium/grid/sessionmap/redis/RedisBackedSessionMap.java[191]

Evidence
The PR change introduces a silent fallback to Instant.EPOCH for missing Redis start timestamps;
downstream code uses the session start time to compute durations and influence node ordering, so the
fallback affects user-visible/operational signals even if it prevents a crash.

java/src/org/openqa/selenium/grid/sessionmap/redis/RedisBackedSessionMap.java[149-199]
java/src/org/openqa/selenium/grid/graphql/Session.java[85-88]
java/src/org/openqa/selenium/grid/data/NodeStatus.java[200-206]
java/src/org/openqa/selenium/grid/distributor/selector/DefaultSlotSelector.java[51-64]
java/src/org/openqa/selenium/grid/distributor/local/LocalGridModel.java[471-476]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`RedisBackedSessionMap.get()` falls back to `Instant.EPOCH` when the Redis start key is missing, but it does so without emitting any explicit signal. This makes the underlying data inconsistency hard to detect and can produce confusing downstream values (e.g., extremely large session durations).

### Issue Context
Downstream code uses `Session.getStartTime()` for user-visible duration calculations and for node/slot ordering heuristics. When the value becomes `Instant.EPOCH`, those systems behave deterministically but misleadingly.

### Fix Focus Areas
- java/src/org/openqa/selenium/grid/sessionmap/redis/RedisBackedSessionMap.java[190-198]
- java/src/org/openqa/selenium/grid/graphql/Session.java[85-88]
- java/src/org/openqa/selenium/grid/data/NodeStatus.java[200-206]

### Suggested change
When `rawStart == null`, keep the fallback (if desired), but also:
- set a span attribute and event attribute like `session.start_missing=true`
- optionally log at WARNING once per session id (or add a tracing event) to make the partial-record condition discoverable in production traces/logs.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Unclosed EventBus in test 🐞 Bug ☼ Reliability
Description
The new createFromConfigBuildsWorkingSessionMap test creates a SessionMap via
RedisBackedSessionMap.create(), which internally constructs an EventBus, but the test only closes
the Redis client. This leaves the EventBus (and registered listeners) open, risking resource leakage
and cross-test interference.
Code

java/test/org/openqa/selenium/grid/sessionmap/redis/RedisBackedSessionMapTest.java[R296-311]

Evidence
The test constructs a SessionMap via RedisBackedSessionMap.create, which explicitly instantiates
an EventBus; EventBus is Closeable, but the test only closes the Redis client, so the bus
remains unclosed.

java/test/org/openqa/selenium/grid/sessionmap/redis/RedisBackedSessionMapTest.java[295-312]
java/src/org/openqa/selenium/grid/sessionmap/redis/RedisBackedSessionMap.java[91-97]
java/src/org/openqa/selenium/events/EventBus.java[20-30]
java/src/org/openqa/selenium/events/local/GuavaEventBus.java[54-67]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`RedisBackedSessionMapTest.createFromConfigBuildsWorkingSessionMap` calls `RedisBackedSessionMap.create(config)`, which allocates an `EventBus`, but the test never closes that bus. Since `EventBus` is `Closeable`, leaving it open can leak listeners/resources.

### Issue Context
`RedisBackedSessionMap.create` constructs the `EventBus` internally via `EventBusOptions`, so the test currently has no handle to close it.

### Fix Focus Areas
- java/test/org/openqa/selenium/grid/sessionmap/redis/RedisBackedSessionMapTest.java[295-312]
- java/src/org/openqa/selenium/grid/sessionmap/redis/RedisBackedSessionMap.java[91-97]
- java/src/org/openqa/selenium/events/EventBus.java[20-30]

### Suggested change
Implement one of:
1) Make `RedisBackedSessionMap` implement `Closeable` and store the `EventBus` instance; in `close()`, close both the Redis client and the bus. Then update the test to `try`/`finally` (or try-with-resources) close the session map.
2) Alternatively, refactor `RedisBackedSessionMap.create(Config)` to accept an externally-managed `EventBus` (or expose the created bus) so the test can close it deterministically.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


View more (1)
4. --sessions-scheme unvalidated input 📘 Rule violation ☼ Reliability
Description
The newly added --sessions-scheme flag accepts arbitrary user input but there is no early
validation to ensure it is a valid URI scheme, which can lead to confusing failures when building
the SessionMap URI. This violates the requirement to validate external/config-derived inputs early
with clear, deterministic exceptions.
Code

java/src/org/openqa/selenium/grid/sessionmap/config/SessionMapFlags.java[R52-56]

Evidence
PR adds a new CLI/config entry point --sessions-scheme without any validation logic. The value is
later consumed to construct a URI in SessionMapOptions.getSessionMapUri(), where invalid schemes
can cause failures that are not clearly attributed to the invalid sessions.scheme input.

java/src/org/openqa/selenium/grid/sessionmap/config/SessionMapFlags.java[52-56]
java/src/org/openqa/selenium/grid/sessionmap/config/SessionMapOptions.java[40-82]
Best Practice: Learned patterns

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`--sessions-scheme` is a new external/config-derived input but is not validated early. Invalid values can surface later during URI construction with an error that does not clearly identify `sessions.scheme` as the problem.

## Issue Context
The flag populates `sessions.scheme`, which is later used to construct a `URI` for the SessionMap server.

## Fix Focus Areas
- java/src/org/openqa/selenium/grid/sessionmap/config/SessionMapFlags.java[52-56]
- java/src/org/openqa/selenium/grid/sessionmap/config/SessionMapOptions.java[40-82]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Previous review results

Review updated until commit 4b1ad34

Results up to commit 6a31a46


🐞 Bugs (0) 📘 Rule violations (1) 📎 Requirement gaps (0)


Remediation recommended
1. --sessions-scheme unvalidated input 📘 Rule violation ☼ Reliability
Description
The newly added --sessions-scheme flag accepts arbitrary user input but there is no early
validation to ensure it is a valid URI scheme, which can lead to confusing failures when building
the SessionMap URI. This violates the requirement to validate external/config-derived inputs early
with clear, deterministic exceptions.
Code

java/src/org/openqa/selenium/grid/sessionmap/config/SessionMapFlags.java[R52-56]

Evidence
PR adds a new CLI/config entry point --sessions-scheme without any validation logic. The value is
later consumed to construct a URI in SessionMapOptions.getSessionMapUri(), where invalid schemes
can cause failures that are not clearly attributed to the invalid sessions.scheme input.

java/src/org/openqa/selenium/grid/sessionmap/config/SessionMapFlags.java[52-56]
java/src/org/openqa/selenium/grid/sessionmap/config/SessionMapOptions.java[40-82]
Best Practice: Learned patterns

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`--sessions-scheme` is a new external/config-derived input but is not validated early. Invalid values can surface later during URI construction with an error that does not clearly identify `sessions.scheme` as the problem.

## Issue Context
The flag populates `sessions.scheme`, which is later used to construct a `URI` for the SessionMap server.

## Fix Focus Areas
- java/src/org/openqa/selenium/grid/sessionmap/config/SessionMapFlags.java[52-56]
- java/src/org/openqa/selenium/grid/sessionmap/config/SessionMapOptions.java[40-82]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Results up to commit 6237f7e


🐞 Bugs (2) 📘 Rule violations (1) 📎 Requirement gaps (0)


Remediation recommended
1. New Redis tests lack Docker gating 📘 Rule violation ☼ Reliability
Description
The newly added RedisBackedSessionMap tests run unconditionally but rely on Testcontainers/Docker,
which can fail in environments where Docker is unavailable. This risks CI instability because the
tests are not conditionally skipped when the required feature support is missing.
Code

java/test/org/openqa/selenium/grid/sessionmap/redis/RedisBackedSessionMapTest.java[R243-312]

Evidence
PR Compliance ID 15 requires new/updated tests to be gated by feature support. The test setup starts
a Testcontainers GenericContainer (Docker dependency), and the newly added test methods are
executed as part of this suite without any conditional skip/assumption when Docker is unavailable.

java/test/org/openqa/selenium/grid/sessionmap/redis/RedisBackedSessionMapTest.java[58-75]
java/test/org/openqa/selenium/grid/sessionmap/redis/RedisBackedSessionMapTest.java[243-312]
Best Practice: Learned patterns

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Newly added JUnit tests in `RedisBackedSessionMapTest` rely on a Docker-backed Testcontainers Redis instance, but the test class does not conditionally skip when Docker is unavailable.

## Issue Context
These tests start a `GenericContainer` in `@BeforeEach`, so any environment without Docker (or with restricted container execution) will fail during setup rather than being skipped.

## Fix Focus Areas
- java/test/org/openqa/selenium/grid/sessionmap/redis/RedisBackedSessionMapTest.java[58-75]
- java/test/org/openqa/selenium/grid/sessionmap/redis/RedisBackedSessionMapTest.java[243-312]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Silent missing session start 🐞 Bug ◔ Observability
Description
RedisBackedSessionMap.get() silently substitutes Instant.EPOCH when the Redis ":start" key is
absent, making sessions appear to have started in 1970 and masking partial/corrupt session records.
This can mislead GraphQL duration reporting and node ordering logic that relies on session start
time.
Code

java/src/org/openqa/selenium/grid/sessionmap/redis/RedisBackedSessionMap.java[191]

Evidence
The PR change introduces a silent fallback to Instant.EPOCH for missing Redis start timestamps;
downstream code uses the session start time to compute durations and influence node ordering, so the
fallback affects user-visible/operational signals even if it prevents a crash.

java/src/org/openqa/selenium/grid/sessionmap/redis/RedisBackedSessionMap.java[149-199]
java/src/org/openqa/selenium/grid/graphql/Session.java[85-88]
java/src/org/openqa/selenium/grid/data/NodeStatus.java[200-206]
java/src/org/openqa/selenium/grid/distributor/selector/DefaultSlotSelector.java[51-64]
java/src/org/openqa/selenium/grid/distributor/local/LocalGridModel.java[471-476]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`RedisBackedSessionMap.get()` falls back to `Instant.EPOCH` when the Redis start key is missing, but it does so without emitting any explicit signal. This makes the underlying data inconsistency hard to detect and can produce confusing downstream values (e.g., extremely large session durations).

### Issue Context
Downstream code uses `Session.getStartTime()` for user-visible duration calculations and for node/slot ordering heuristics. When the value becomes `Instant.EPOCH`, those systems behave deterministically but misleadingly.

### Fix Focus Areas
- java/src/org/openqa/selenium/grid/sessionmap/redis/RedisBackedSessionMap.java[190-198]
- java/src/org/openqa/selenium/grid/graphql/Session.java[85-88]
- java/src/org/openqa/selenium/grid/data/NodeStatus.java[200-206]

### Suggested change
When `rawStart == null`, keep the fallback (if desired), but also:
- set a span attribute and event attribute like `session.start_missing=true`
- optionally log at WARNING once per session id (or add a tracing event) to make the partial-record condition discoverable in production traces/logs.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Unclosed EventBus in test 🐞 Bug ☼ Reliability
Description
The new createFromConfigBuildsWorkingSessionMap test creates a SessionMap via
RedisBackedSessionMap.create(), which internally constructs an EventBus, but the test only closes
the Redis client. This leaves the EventBus (and registered listeners) open, risking resource leakage
and cross-test interference.
Code

java/test/org/openqa/selenium/grid/sessionmap/redis/RedisBackedSessionMapTest.java[R296-311]

Evidence
The test constructs a SessionMap via RedisBackedSessionMap.create, which explicitly instantiates
an EventBus; EventBus is Closeable, but the test only closes the Redis client, so the bus
remains unclosed.

java/test/org/openqa/selenium/grid/sessionmap/redis/RedisBackedSessionMapTest.java[295-312]
java/src/org/openqa/selenium/grid/sessionmap/redis/RedisBackedSessionMap.java[91-97]
java/src/org/openqa/selenium/events/EventBus.java[20-30]
java/src/org/openqa/selenium/events/local/GuavaEventBus.java[54-67]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`RedisBackedSessionMapTest.createFromConfigBuildsWorkingSessionMap` calls `RedisBackedSessionMap.create(config)`, which allocates an `EventBus`, but the test never closes that bus. Since `EventBus` is `Closeable`, leaving it open can leak listeners/resources.

### Issue Context
`RedisBackedSessionMap.create` constructs the `EventBus` internally via `EventBusOptions`, so the test currently has no handle to close it.

### Fix Focus Areas
- java/test/org/openqa/selenium/grid/sessionmap/redis/RedisBackedSessionMapTest.java[295-312]
- java/src/org/openqa/selenium/grid/sessionmap/redis/RedisBackedSessionMap.java[91-97]
- java/src/org/openqa/selenium/events/EventBus.java[20-30]

### Suggested change
Implement one of:
1) Make `RedisBackedSessionMap` implement `Closeable` and store the `EventBus` instance; in `close()`, close both the Redis client and the bus. Then update the test to `try`/`finally` (or try-with-resources) close the session map.
2) Alternatively, refactor `RedisBackedSessionMap.create(Config)` to accept an externally-managed `EventBus` (or expose the created bus) so the test can close it deterministically.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Qodo Logo

@diemol
Copy link
Copy Markdown
Member

diemol commented May 12, 2026

Does this mean we're increasing the size of the jar file? What is the size difference since we have added this to the distributor as well? We should pay attention to that so we don't bloat the server.

@VietND96
Copy link
Copy Markdown
Member Author

@diemol. Yes, the diff is 42.3 MB...51M. The same size applies to cross components (Distributor, SessionMap, and later SessionQueue). Do you think it's worth it?

@diemol
Copy link
Copy Markdown
Member

diemol commented May 19, 2026

Should be OK.

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 19, 2026

Persistent review updated to latest commit 6237f7e

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 20, 2026

Persistent review updated to latest commit 4190fd9

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 20, 2026

Persistent review updated to latest commit 4b1ad34

@VietND96 VietND96 merged commit d6063ed into trunk May 21, 2026
54 checks passed
@VietND96 VietND96 deleted the redis-backed-sessionsmap branch May 21, 2026 04:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-build Includes scripting, bazel and CI integrations B-grid Everything grid and server related C-java Java Bindings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants