Harden failed-IP cooldown config and add DEFAULT-mode test

client/src/main/java/org/asynchttpclient/DefaultAsyncHttpClientConfig.java

@@ -1222,12 +1222,21 @@ public Builder setFailedIpCooldownEnabled(boolean failedIpCooldownEnabled) {
 
         /**
          * @param failedIpCooldownPeriod how long a failed IP is deprioritized before it is re-probed;
-         *                               {@code null} resets to the default
+         *                               {@code null} resets to the default. Must not be negative; use
+         *                               {@link #setFailedIpCooldownEnabled(boolean)} with {@code false}
+         *                               to turn the cooldown off instead.
          * @return this
+         * @throws IllegalArgumentException if {@code failedIpCooldownPeriod} is negative
          * @see AsyncHttpClientConfig#getFailedIpCooldownPeriod()
          */
         public Builder setFailedIpCooldownPeriod(Duration failedIpCooldownPeriod) {
-            this.failedIpCooldownPeriod = failedIpCooldownPeriod == null ? defaultFailedIpCooldownPeriod() : failedIpCooldownPeriod;
+            if (failedIpCooldownPeriod == null) {
+                this.failedIpCooldownPeriod = defaultFailedIpCooldownPeriod();
+            } else if (failedIpCooldownPeriod.isNegative()) {
+                throw new IllegalArgumentException("failedIpCooldownPeriod must not be negative: " + failedIpCooldownPeriod);
+            } else {
+                this.failedIpCooldownPeriod = failedIpCooldownPeriod;
+            }
             return this;
         }
 

client/src/main/java/org/asynchttpclient/netty/request/NettyRequestSender.java

@@ -89,6 +89,7 @@
 import java.net.InetAddress;
 import java.net.InetSocketAddress;
 import java.net.SocketAddress;
+import java.time.Duration;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Locale;
@@ -132,8 +133,11 @@ public NettyRequestSender(AsyncHttpClientConfig config, ChannelManager channelMa
         this.nettyTimer = nettyTimer;
         this.clientState = clientState;
         requestFactory = new NettyRequestFactory(config);
-        ipCooldown = config.isFailedIpCooldownEnabled()
-                ? new FailedIpCooldownHolder(config.getFailedIpCooldownPeriod().toNanos(), System::nanoTime)
+        // Guard the period against a custom AsyncHttpClientConfig that enables the cooldown but returns a
+        // null period: leave the cooldown off rather than NPE while constructing the client.
+        Duration cooldownPeriod = config.getFailedIpCooldownPeriod();
+        ipCooldown = config.isFailedIpCooldownEnabled() && cooldownPeriod != null
+                ? new FailedIpCooldownHolder(cooldownPeriod.toNanos(), System::nanoTime)
                 : null;
     }
 

client/src/test/java/org/asynchttpclient/FailedIpCooldownConfigTest.java

@@ -22,6 +22,7 @@
 import static org.asynchttpclient.Dsl.config;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 class FailedIpCooldownConfigTest {
@@ -50,6 +51,18 @@ void nullPeriodResetsToDefault() {
         assertEquals(Duration.ofSeconds(10), config.getFailedIpCooldownPeriod());
     }
 
+    @Test
+    void negativePeriodIsRejected() {
+        assertThrows(IllegalArgumentException.class, () -> config().setFailedIpCooldownPeriod(Duration.ofSeconds(-1)));
+    }
+
+    @Test
+    void zeroPeriodIsAccepted() {
+        // zero is a valid (if degenerate) period — turning the cooldown off is done via setFailedIpCooldownEnabled(false)
+        AsyncHttpClientConfig config = config().setFailedIpCooldownPeriod(Duration.ZERO).build();
+        assertEquals(Duration.ZERO, config.getFailedIpCooldownPeriod());
+    }
+
     @Test
     void copyConstructorPreservesValues() {
         AsyncHttpClientConfig source = config()

client/src/test/java/org/asynchttpclient/RoundRobinSendTypeTest.java

@@ -38,6 +38,7 @@
 import java.util.List;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.CopyOnWriteArrayList;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.TimeoutException;
 
@@ -132,6 +133,28 @@ public Response onCompleted(Response response) {
         }
     }
 
+    // Records the IPs a single request targeted, in the order the connector attempted them, so a test can
+    // assert which IP a new connection tried first (the failed-IP cooldown re-orders that first choice).
+    private static final class OrderedAttemptRecorder extends AsyncCompletionHandler<Response> {
+        private final List<String> attemptedIps;
+
+        private OrderedAttemptRecorder(List<String> attemptedIps) {
+            this.attemptedIps = attemptedIps;
+        }
+
+        @Override
+        public void onTcpConnectAttempt(InetSocketAddress remoteAddress) {
+            if (remoteAddress != null && remoteAddress.getAddress() != null) {
+                attemptedIps.add(remoteAddress.getAddress().getHostAddress());
+            }
+        }
+
+        @Override
+        public Response onCompleted(Response response) {
+            return response;
+        }
+    }
+
     // Returns the set of IPs the client targeted (first choice plus any failovers) across the requests.
     private Set<String> runRequestsCapturingTargetedIps(AsyncHttpClientConfig config) throws Exception {
         Set<String> attemptedIps = ConcurrentHashMap.newKeySet();
@@ -228,6 +251,47 @@ public void handle(String t, Request base, HttpServletRequest req, HttpServletRe
         }
     }
 
+    @Test
+    public void defaultModeDeprioritizesAFailedIpOnTheNextConnection() throws Exception {
+        // The headline behavior this PR adds: the failed-IP cooldown now applies in DEFAULT mode too, so a
+        // TCP-connect failure to an IP deprioritizes it (moves it to the back) on the next new connection.
+        // Server bound to 127.0.0.1 only, so 127.0.0.2 has no listener (refused on Linux / unreachable on macOS).
+        Server localServer = new Server();
+        ServerConnector connector = addHttpConnector(localServer);
+        connector.setHost("127.0.0.1");
+        localServer.setHandler(new EchoHandler());
+        localServer.start();
+        int localPort = connector.getLocalPort();
+        try {
+            // Resolver hands back the dead IP first. keepAlive=false forces a fresh connection per request,
+            // so every request re-orders and connects (a pooled reuse would never re-resolve). DEFAULT mode
+            // (no setLoadBalance) — this is what previously always dialed the dead IP first.
+            NameResolver<InetAddress> resolver = fixedResolver("127.0.0.2", "127.0.0.1");
+            List<String> firstAttemptPerRequest = new ArrayList<>();
+            try (AsyncHttpClient client = asyncHttpClient(config().setKeepAlive(false).setMaxRequestRetry(0).build())) {
+                for (int i = 0; i < 4; i++) {
+                    List<String> attempts = new CopyOnWriteArrayList<>();
+                    Response response = client.executeRequest(
+                            get("http://cooldown.test:" + localPort + "/").setNameResolver(resolver),
+                            new OrderedAttemptRecorder(attempts)).get(TIMEOUT, SECONDS);
+                    assertEquals(200, response.getStatusCode(), "request should succeed via failover to the reachable IP");
+                    firstAttemptPerRequest.add(attempts.get(0));
+                }
+            }
+            // First request has nothing cooling yet, so it dials the dead IP first (resolver order) then fails over.
+            assertEquals("127.0.0.2", firstAttemptPerRequest.get(0),
+                    "the first connection follows resolver order because no failure has been recorded yet");
+            // Once 127.0.0.2's connect failure is recorded, the cooldown moves it to the back, so every later
+            // new connection dials the healthy 127.0.0.1 first — the point of extending the cooldown to DEFAULT mode.
+            for (int i = 1; i < firstAttemptPerRequest.size(); i++) {
+                assertEquals("127.0.0.1", firstAttemptPerRequest.get(i),
+                        "DEFAULT mode must deprioritize the recently-failed IP on later new connections (request #" + i + ")");
+            }
+        } finally {
+            localServer.stop();
+        }
+    }
+
     @Test
     public void roundRobinReResolvesAcrossSameHostPortChangingRedirect() throws Exception {
         // Regression test for the same-base redirect leak: round-robin caches the resolved addresses