[grid] Fix regression Distributor rejecting requests when nodes have supported caps but no free slots

[VietND96]

User description

🔗 Related Issues

💥 What does this PR do?

Fix the regression issue from #16155 where adjusting the condition of getAvailableNodes() to include a condition that has at least 1 available Slot
It causes even if Node supports caps, but it is busy, the request is immediately dropped in case docker grid standalone mode enables --reject-unsupported-caps always.
Expected that when having supported caps and slots are busy, requests will wait in the queue for a while.

 15:51:38.473 INFO [LocalDistributor$NewSessionRunnable.checkMatchingSlot] - No nodes support the capabilities in the request: [Capabilities {browserName: chrome, goog:chromeOptions: {args: [], extensions: []}, pageLoadStrategy: normal, se:downloadsEnabled: true}] 15:51:38.474 INFO [LocalDistributor$NewSessionRunnable.checkMatchingSlot] - No nodes support the capabilities in the request: [Capabilities {browserName: chrome, goog:chromeOptions: {args: [], extensions: []}, pageLoadStrategy: normal, se:downloadsEnabled: true}] 15:51:40.068 INFO [LocalNode.newSession] - Session created by the Node. Id: 9a1d15628f9147cf8033241b777aefb3, Caps: Capabilities 

Add a method to getUpNodes() only
Add unit tests

🔧 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

Bug fix

---

Description

• Fix regression where distributor rejects requests when nodes support capabilities but have no free slots

• Add getUpNodes() method to distinguish UP nodes from available nodes with capacity

• Update capability checking logic to use UP nodes instead of available nodes

• Add comprehensive unit tests for both supported and unsupported capability scenarios

---

Diagram Walkthrough

flowchart LR A["Session Request"] --> B["Check Capability Support"] B --> C{"Any UP Node Supports?"} C -->|Yes| D["Queue Request"] C -->|No| E["Reject Request"] F["getUpNodes()"] --> C G["getAvailableNodes()"] --> H["Reserve Slot"] 

Loading

File Walkthrough

	Relevant files
Enhancement	NodeRegistry.java Add getUpNodes interface method                                                    java/src/org/openqa/selenium/grid/distributor/NodeRegistry.java Add getUpNodes() method interface declaration Update getAvailableNodes() documentation to clarify it requires free slots +8/-1      LocalNodeRegistry.java Implement getUpNodes method                                                            java/src/org/openqa/selenium/grid/distributor/local/LocalNodeRegistry.java Implement getUpNodes() method to return nodes with UP availability Refactor getAvailableNodes() to filter UP nodes by capacity +8/-2
Bug fix	LocalDistributor.java Fix capability checking logic                                                        java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java Change isNotSupported() to use getUpNodes() instead of getAvailableNodes() Refactor checkMatchingSlot() to use UP nodes for capability checking Optimize by getting UP nodes once per batch instead of per request +24/-16
Tests	LocalDistributorTest.java Add regression tests                                                                          java/test/org/openqa/selenium/grid/distributor/local/LocalDistributorTest.java Add test for nodes with capability but no free slots scenario Add test for completely unsupported capability rejection Verify proper behavior with rejectUnsupportedCaps enabled +134/-0

---

[qodo-merge-pro]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 1234 - Partially compliant Compliant requirements: Non-compliant requirements: Investigate Firefox-specific link click JavaScript triggering regression. Reproduce behavior differences between Selenium versions on Firefox. Provide a Firefox-driver specific fix or guidance. Requires further human verification: Validate link click JavaScript behavior manually across versions and Firefox; this PR does not touch browser interaction logic. 5678 - Partially compliant Compliant requirements: Non-compliant requirements: Fix or mitigate ChromeDriver connection refusal on subsequent instantiations. Provide guidance or code changes related to Chrome/ChromeDriver connectivity. Requires further human verification: Reproduce ChromeDriver connection errors in the target environment; this PR is unrelated to driver instantiation networking.

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Possible Issue Using getUpNodes() without synchronization may read a changing snapshot; ensure NodeRegistry guarantees thread-safe, consistent snapshots or acquire appropriate read locks to avoid race conditions when evaluating capability support. sessionRequests.stream() .filter( request -> request.getDesiredCapabilities().stream() .noneMatch( caps -> upNodes.stream() .anyMatch(node -> node.hasCapability(caps, slotMatcher)))) .forEach( request -> { LOG.info( "No nodes support the capabilities in the request: " + request.getDesiredCapabilities()); SessionNotCreatedException exception = new SessionNotCreatedException( "No nodes support the capabilities in the request"); sessionQueue.complete(request.getRequestId(), Either.left(exception)); }); } API Consistency getUpNodes() is missing @OverRide annotation while declared in interface; ensure signature matches and add @OverRide for clarity and compile-time checking. public Set<NodeStatus> getUpNodes() { Lock readLock = this.lock.readLock(); readLock.lock(); try { return model.getSnapshot().stream() .filter(node -> UP.equals(node.getAvailability())) .collect(ImmutableSet.toImmutableSet()); } finally {

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Complete test by verifying request is not rejected Add a second session request to the shouldNotRejectRequestsWhenNodesHaveCapabilityButNoFreeSlots test and assert it remains in the queue to verify it is not rejected when a capable node is busy. java/test/org/openqa/selenium/grid/distributor/local/LocalDistributorTest.java [852-857] // Verify the node has no capacity (all slots occupied) assertThat(nodeStatus.hasCapacity()).isFalse(); + + // Now, add a second request for the same capability to the queue + SessionRequest secondRequest = + new SessionRequest( + new RequestId(UUID.randomUUID()), + Instant.now(), + Set.of(W3C), + Set.of(new ImmutableCapabilities("browserName", "chrome")), + Map.of(), + Map.of()); + queue.addToQueue(secondRequest); + + // Allow some time for the distributor to process the queue, but not enough for a timeout. + // A busy-wait is not ideal, but for a test, a short sleep can verify the request isn't + // immediately rejected. + try { + Thread.sleep(500); // Less than the queue's retry interval + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + fail("Test interrupted"); + } + + // The request should remain in the queue because a capable node exists, even if it's busy. + assertThat(queue.getQueueContents()).contains(secondRequest); } @Test void shouldRejectRequestsWhenNoNodesHaveCapability() throws URISyntaxException { Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that the test is incomplete and does not verify the primary behavior described in its name, which is a key part of the PR's logic change.	Medium
Learned best practice	✅ Add missing @OverRide annotation Suggestion Impact: The commit added the @OverRide annotation directly above the getUpNodes() method as suggested. code diff: + @Override public Set<NodeStatus> getUpNodes() { Add the @OverRide annotation to getUpNodes() since it implements the new interface method, enabling compile-time checks. java/src/org/openqa/selenium/grid/distributor/local/LocalNodeRegistry.java [366-376] +@Override public Set<NodeStatus> getUpNodes() { Lock readLock = this.lock.readLock(); readLock.lock(); try { return model.getSnapshot().stream() .filter(node -> UP.equals(node.getAvailability())) .collect(ImmutableSet.toImmutableSet()); } finally { readLock.unlock(); } } [Suggestion processed] Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Explicitly annotate interface implementations with @OverRide to ensure contract adherence and improve maintainability.	Low
Update