From de8109fa5392fd2abb0c0783c0f358104b629174 Mon Sep 17 00:00:00 2001 From: Viet Nguyen Duc Date: Wed, 17 Sep 2025 00:20:57 +0700 Subject: [PATCH 1/2] [grid] Fix regression Distributor rejecting requests when nodes have supported caps but no free slots Signed-off-by: Viet Nguyen Duc --- .../grid/distributor/NodeRegistry.java | 9 +- .../distributor/local/LocalDistributor.java | 40 +++--- .../distributor/local/LocalNodeRegistry.java | 10 +- .../local/LocalDistributorTest.java | 134 ++++++++++++++++++ 4 files changed, 174 insertions(+), 19 deletions(-) diff --git a/java/src/org/openqa/selenium/grid/distributor/NodeRegistry.java b/java/src/org/openqa/selenium/grid/distributor/NodeRegistry.java index d8b0789b792bd..ec3ae18b7ee51 100644 --- a/java/src/org/openqa/selenium/grid/distributor/NodeRegistry.java +++ b/java/src/org/openqa/selenium/grid/distributor/NodeRegistry.java @@ -85,7 +85,14 @@ public interface NodeRegistry extends HasReadyState, Closeable { DistributorStatus getStatus(); /** - * Gets all available nodes that are not DOWN or DRAINING. + * Get all nodes that are UP. + * + * @return Set of UP node statuses. + */ + Set getUpNodes(); + + /** + * Gets all available nodes that are not DOWN or DRAINING and has free slots. * * @return Set of available node statuses. */ diff --git a/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java b/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java index 609a8a27a55fb..fcb51012b77ae 100644 --- a/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java +++ b/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java @@ -466,7 +466,8 @@ private SlotId reserveSlot(RequestId requestId, Capabilities caps) { } private boolean isNotSupported(Capabilities caps) { - return getAvailableNodes().stream().noneMatch(node -> node.hasCapability(caps, slotMatcher)); + return nodeRegistry.getUpNodes().stream() + .noneMatch(node -> node.hasCapability(caps, slotMatcher)); } private boolean reserve(SlotId id) { @@ -560,21 +561,28 @@ public void run() { } private void checkMatchingSlot(List sessionRequests) { - for (SessionRequestCapability request : sessionRequests) { - long unmatchableCount = - request.getDesiredCapabilities().stream() - .filter(LocalDistributor.this::isNotSupported) - .count(); - - if (unmatchableCount == request.getDesiredCapabilities().size()) { - 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)); - } - } + // Get UP nodes once to avoid lock & loop over multiple requests + Set upNodes = nodeRegistry.getUpNodes(); + // Filter and reject only requests where NO capabilities are supported by ANY UP node + // This prevents rejecting requests when nodes support capabilities but are just busy + 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)); + }); } private void handleNewSessionRequest(SessionRequest sessionRequest) { diff --git a/java/src/org/openqa/selenium/grid/distributor/local/LocalNodeRegistry.java b/java/src/org/openqa/selenium/grid/distributor/local/LocalNodeRegistry.java index 097657a3cc384..ba14cd74514dd 100644 --- a/java/src/org/openqa/selenium/grid/distributor/local/LocalNodeRegistry.java +++ b/java/src/org/openqa/selenium/grid/distributor/local/LocalNodeRegistry.java @@ -357,12 +357,18 @@ public DistributorStatus getStatus() { @Override public Set getAvailableNodes() { + // Filter nodes are UP and have capacity (available slots) + return getUpNodes().stream() + .filter(NodeStatus::hasCapacity) + .collect(ImmutableSet.toImmutableSet()); + } + + public Set getUpNodes() { Lock readLock = this.lock.readLock(); readLock.lock(); try { return model.getSnapshot().stream() - // Filter nodes are UP and have capacity (available slots) - .filter(node -> UP.equals(node.getAvailability()) && node.hasCapacity()) + .filter(node -> UP.equals(node.getAvailability())) .collect(ImmutableSet.toImmutableSet()); } finally { readLock.unlock(); diff --git a/java/test/org/openqa/selenium/grid/distributor/local/LocalDistributorTest.java b/java/test/org/openqa/selenium/grid/distributor/local/LocalDistributorTest.java index eae8de5d87f69..20a138f9b6601 100644 --- a/java/test/org/openqa/selenium/grid/distributor/local/LocalDistributorTest.java +++ b/java/test/org/openqa/selenium/grid/distributor/local/LocalDistributorTest.java @@ -777,6 +777,140 @@ void shouldReduceRedundantSlotChecks() throws URISyntaxException { } } + @Test + void shouldNotRejectRequestsWhenNodesHaveCapabilityButNoFreeSlots() throws URISyntaxException { + // Create a distributor with rejectUnsupportedCaps enabled + NewSessionQueue queue = + new LocalNewSessionQueue( + tracer, + new DefaultSlotMatcher(), + Duration.ofSeconds(2), + Duration.ofSeconds(2), + Duration.ofSeconds(1), + registrationSecret, + 5); + LocalDistributor distributor = + new LocalDistributor( + tracer, + bus, + new PassthroughHttpClient.Factory(localNode), + new LocalSessionMap(tracer, bus), + queue, + new DefaultSlotSelector(), + registrationSecret, + Duration.ofMinutes(5), + true, // Enable rejectUnsupportedCaps + Duration.ofSeconds(5), + newSessionThreadPoolSize, + new DefaultSlotMatcher(), + Duration.ofSeconds(30)); + + // Create a node that supports Chrome with single slot + URI nodeUri = new URI("http://example:1234"); + Node node = + LocalNode.builder(tracer, bus, nodeUri, nodeUri, registrationSecret) + .add( + new ImmutableCapabilities("browserName", "chrome"), + new TestSessionFactory( + (id, c) -> + new Session(id, nodeUri, new ImmutableCapabilities(), c, Instant.now()))) + .maximumConcurrentSessions(1) + .build(); + distributor.add(node); + + // Occupy the node's only slot + SessionRequest sessionRequest = + new SessionRequest( + new RequestId(UUID.randomUUID()), + Instant.now(), + Set.of(W3C), + Set.of(new ImmutableCapabilities("browserName", "chrome")), + Map.of(), + Map.of()); + Either result = + distributor.newSession(sessionRequest); + assertThat(result.isRight()).isTrue(); // Session created successfully + + // Verify node has no available capacity but still supports Chrome + assertThat(distributor.getAvailableNodes()).isEmpty(); // No available nodes + + // Test that the distributor status shows the node is still UP and supports Chrome + // even though it has no available capacity + DistributorStatus status = distributor.getStatus(); + Set allNodes = status.getNodes(); + assertThat(allNodes).hasSize(1); + + NodeStatus nodeStatus = allNodes.iterator().next(); + assertThat(nodeStatus.getAvailability()).isEqualTo(UP); + + // Verify the node still supports Chrome capability even with no free slots + boolean supportsChrome = + nodeStatus.hasCapability( + new ImmutableCapabilities("browserName", "chrome"), new DefaultSlotMatcher()); + assertThat(supportsChrome).isTrue(); + + // Verify the node has no capacity (all slots occupied) + assertThat(nodeStatus.hasCapacity()).isFalse(); + } + + @Test + void shouldRejectRequestsWhenNoNodesHaveCapability() throws URISyntaxException { + // Create a distributor with rejectUnsupportedCaps enabled + NewSessionQueue queue = + new LocalNewSessionQueue( + tracer, + new DefaultSlotMatcher(), + Duration.ofSeconds(2), + Duration.ofSeconds(2), + Duration.ofSeconds(1), + registrationSecret, + 5); + LocalDistributor distributor = + new LocalDistributor( + tracer, + bus, + new PassthroughHttpClient.Factory(localNode), + new LocalSessionMap(tracer, bus), + queue, + new DefaultSlotSelector(), + registrationSecret, + Duration.ofMinutes(5), + true, // Enable rejectUnsupportedCaps + Duration.ofSeconds(5), + newSessionThreadPoolSize, + new DefaultSlotMatcher(), + Duration.ofSeconds(30)); + + // Create a node that only supports Chrome + URI nodeUri = new URI("http://example:1234"); + Node node = + LocalNode.builder(tracer, bus, nodeUri, nodeUri, registrationSecret) + .add( + new ImmutableCapabilities("browserName", "chrome"), + new TestSessionFactory( + (id, c) -> + new Session(id, nodeUri, new ImmutableCapabilities(), c, Instant.now()))) + .build(); + distributor.add(node); + + // Add a Firefox request to the queue (unsupported capability) + SessionRequest unsupportedRequest = + new SessionRequest( + new RequestId(UUID.randomUUID()), + Instant.now(), + Set.of(W3C), + Set.of(new ImmutableCapabilities("browserName", "firefox")), + Map.of(), + Map.of()); + queue.addToQueue(unsupportedRequest); + + // Wait for checkMatchingSlot to run and reject the request + wait.until(obj -> queue.getQueueContents().isEmpty()); + + // The request should be rejected and removed from queue + assertThat(queue.getQueueContents()).isEmpty(); + } + @Test void shouldHandleAllNodesFullyOccupied() throws URISyntaxException { // Create a distributor From 8ca3025ba1e4ee20ee0669ee758015a7eb8c87a7 Mon Sep 17 00:00:00 2001 From: Viet Nguyen Duc Date: Wed, 17 Sep 2025 06:07:00 +0700 Subject: [PATCH 2/2] Fix review comment Signed-off-by: Viet Nguyen Duc --- .../selenium/grid/distributor/local/LocalNodeRegistry.java | 1 + 1 file changed, 1 insertion(+) diff --git a/java/src/org/openqa/selenium/grid/distributor/local/LocalNodeRegistry.java b/java/src/org/openqa/selenium/grid/distributor/local/LocalNodeRegistry.java index ba14cd74514dd..cd72a4f7c4b83 100644 --- a/java/src/org/openqa/selenium/grid/distributor/local/LocalNodeRegistry.java +++ b/java/src/org/openqa/selenium/grid/distributor/local/LocalNodeRegistry.java @@ -363,6 +363,7 @@ public Set getAvailableNodes() { .collect(ImmutableSet.toImmutableSet()); } + @Override public Set getUpNodes() { Lock readLock = this.lock.readLock(); readLock.lock();