SOLR-9355: decide update retries from the cause chain, not one fixed position in it

changelog/unreleased/checkretry-unroll-exception-chain.yml

@@ -0,0 +1,10 @@
+title: >
+  A transient connection failure from a shard leader to one of its replicas is now retried when the
+  failure arrives wrapped inside another exception, instead of sending the replica into recovery.
+  Previously whether the retry happened depended on which exception the client reported outermost.
+type: fixed
+authors:
+  - name: Serhiy Bzhezytskyy
+links:
+  - name: SOLR-9355
+    url: https://issues.apache.org/jira/browse/SOLR-9355

solr/core/src/java/org/apache/solr/update/SolrCmdDistributor.java

@@ -33,7 +33,6 @@
 import java.util.concurrent.ExecutorCompletionService;
 import java.util.concurrent.Future;
 import org.apache.solr.client.solrj.SolrClient;
-import org.apache.solr.client.solrj.SolrServerException;
 import org.apache.solr.client.solrj.impl.ConcurrentUpdateBaseSolrClient;
 import org.apache.solr.client.solrj.request.AbstractUpdateRequest;
 import org.apache.solr.client.solrj.request.UpdateRequest;
@@ -56,6 +55,9 @@
 public class SolrCmdDistributor implements Closeable {
   private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
 
+  /** Cause chains are shallow in practice; the cap only guards against a cyclic chain. */
+  private static final int MAX_CAUSE_DEPTH = 100;
+
   private StreamingSolrClients clients;
   private boolean finished = false; // see finish()
 
@@ -572,23 +574,30 @@ public boolean checkRetry(SolrError err) {
       }
 
       // if it's a connect exception, lets try again
-      if (err.e instanceof SolrServerException) {
-        if (isRetriableException(((SolrServerException) err.e).getRootCause())) {
-          return true;
-        }
-      } else {
-        if (isRetriableException(err.e)) {
-          return true;
-        }
-      }
-      return false;
+      return isRetriableException(err.e);
     }
 
     /**
+     * Inspects the whole cause chain, because a retriable failure is not always the outermost or
+     * the deepest exception. The async client reports a connection failure wrapped in an
+     * ExecutionException, and Jetty's ClientConnector wraps the underlying failure in a
+     * SocketException of its own, so neither the top-level type nor the root cause alone identifies
+     * every retriable case.
+     *
      * @return true if Solr should retry in case of hitting this exception false otherwise
      */
     private boolean isRetriableException(Throwable t) {
-      return t instanceof SocketException || t instanceof SocketTimeoutException;
+      // Bounded: a cause chain can be cyclic, as the TODO on SolrException.getRootCause notes.
+      // Real chains are a handful of frames deep.
+      int depth = 0;
+      for (Throwable cause = t;
+          cause != null && depth++ < MAX_CAUSE_DEPTH;
+          cause = cause.getCause()) {
+        if (cause instanceof SocketException || cause instanceof SocketTimeoutException) {
+          return true;
+        }
+      }
+      return false;
     }
 
     @Override
@@ -644,6 +653,18 @@ public static class ForwardNode extends StdNode {
 
     private ZkStateReader zkStateReader;
 
+    private static boolean hasConnectExceptionInChain(Throwable t) {
+      int depth = 0;
+      for (Throwable cause = t;
+          cause != null && depth++ < MAX_CAUSE_DEPTH;
+          cause = cause.getCause()) {
+        if (cause instanceof ConnectException) {
+          return true;
+        }
+      }
+      return false;
+    }
+
     public ForwardNode(
         ZkCoreNodeProps nodeProps,
         ZkStateReader zkStateReader,
@@ -664,10 +685,7 @@ public boolean checkRetry(SolrError err) {
       }
 
       // if it's a connect exception, lets try again
-      if (err.e instanceof SolrServerException
-          && ((SolrServerException) err.e).getRootCause() instanceof ConnectException) {
-        doRetry = true;
-      } else if (err.e instanceof ConnectException) {
+      if (hasConnectExceptionInChain(err.e)) {
         doRetry = true;
       }
       if (doRetry) {

solr/core/src/test/org/apache/solr/update/CheckRetryUnrollTest.java

@@ -0,0 +1,151 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.update;
+
+import java.io.IOException;
+import java.net.ConnectException;
+import java.net.SocketException;
+import java.nio.channels.ClosedChannelException;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.concurrent.ExecutionException;
+import org.apache.solr.SolrTestCase;
+import org.apache.solr.client.solrj.SolrServerException;
+import org.apache.solr.common.cloud.Replica;
+import org.apache.solr.common.cloud.ZkCoreNodeProps;
+import org.junit.Test;
+
+/**
+ * Whether a retriable failure is retried should not depend on which exception is outermost.
+ *
+ * <p>Covers {@link SolrCmdDistributor.StdNode}. {@link SolrCmdDistributor.ForwardNode} carries the
+ * same asymmetry and is changed the same way, but needs a live ZkStateReader to construct, so it
+ * stays covered by SolrCmdDistributorTest rather than here.
+ */
+public class CheckRetryUnrollTest extends SolrTestCase {
+
+  private static Replica replica() {
+    Map<String, Object> props = new HashMap<>();
+    props.put("base_url", "http://127.0.0.1:8983/solr");
+    props.put("core", "collection1");
+    props.put("node_name", "127.0.0.1:8983_solr");
+    props.put("type", "NRT");
+    props.put("state", "active");
+    return new Replica("core_node1", props, "collection1", "shard1");
+  }
+
+  private static SolrCmdDistributor.Node node() {
+    return new SolrCmdDistributor.StdNode(
+        new ZkCoreNodeProps(replica()), "collection1", "shard1", /* maxRetries= */ 1);
+  }
+
+  private static boolean retries(Exception e) {
+    SolrCmdDistributor.SolrError err = new SolrCmdDistributor.SolrError();
+    err.e = e;
+    return node().checkRetry(err);
+  }
+
+  @Test
+  public void testRetriesWhenSocketExceptionIsWrappedInSolrServerException() {
+    // the shape checkRetry already unwraps
+    assertTrue(
+        retries(new SolrServerException("wrapped", new SocketException("Connection reset"))));
+  }
+
+  @Test
+  public void testRetriesWhenSocketExceptionIsTopLevel() {
+    assertTrue(retries(new SocketException("Connection reset")));
+  }
+
+  @Test
+  public void testRetriesWhenSocketExceptionIsWrappedInSomethingElse() {
+    // the async (Jetty) path delivers a connection failure as an ExecutionException; the socket
+    // cause is just as retriable as in the two cases above, but the outer type is not
+    // SolrServerException so the leaf test never sees it.
+    assertTrue(retries(new ExecutionException(new ConnectException("Connection refused"))));
+  }
+
+  @Test
+  public void testRetriesWhenSocketExceptionIsNestedDeeply() {
+    assertTrue(
+        retries(
+            new ExecutionException(
+                new RuntimeException("io", new SocketException("Connection reset")))));
+  }
+
+  @Test
+  public void testDoesNotRetryOnAServerErrorRootedInSomethingElse() {
+    // control: the SolrServerException shape that already worked must keep its answer
+    assertFalse(retries(new SolrServerException("wrapped", new IllegalStateException("nope"))));
+  }
+
+  @Test
+  public void testClosedChannelExceptionIsStillNotRetriableEitherWay() {
+    // Documents a limit of this change rather than a fix. ClosedChannelException is what the JDK
+    // transport actually reports as the root cause of a dropped update connection, and it is not a
+    // SocketException, so it stays non-retriable however the chain is inspected. Widening
+    // isRetriableException is a separate behaviour decision.
+    assertFalse(retries(new ExecutionException(new ClosedChannelException())));
+    assertFalse(retries(new SolrServerException("wrapped", new ClosedChannelException())));
+  }
+
+  @Test
+  public void testAnUnretriableNodeNeverRetriesHoweverTheChainLooks() {
+    // The count ceiling lives in Req.shouldRetry, not here, but checkRetry has its own gate: a node
+    // built with maxRetries=0 has retry==false and must refuse before the exception is even looked
+    // at. Unrolling must not bypass that.
+    SolrCmdDistributor.Node noRetries =
+        new SolrCmdDistributor.StdNode(new ZkCoreNodeProps(replica()), "collection1", "shard1");
+    SolrCmdDistributor.SolrError err = new SolrCmdDistributor.SolrError();
+    err.e = new ExecutionException(new ConnectException("Connection refused"));
+    assertFalse(noRetries.checkRetry(err));
+  }
+
+  @Test
+  public void testRetriesWhenTheRetriableTypeIsNotTheRootCause() {
+    // Jetty's ClientConnector wraps the underlying failure in a SocketException of its own
+    // (ClientConnector#connect), so the retriable frame can sit above the root cause. Going
+    // straight
+    // to the root cause would miss it.
+    assertTrue(
+        retries(
+            new ExecutionException(
+                new SocketException("Could not connect to host", new IOException("underlying")))));
+  }
+
+  @Test
+  public void testTerminatesOnACyclicCauseChain() {
+    // A cause chain can be made cyclic, which is why the scan is bounded -- see the TODO on
+    // SolrException#getRootCause. This must return rather than spin.
+    Exception first = new Exception("first");
+    Exception second = new Exception("second", first);
+    try {
+      first.initCause(second);
+    } catch (IllegalStateException | IllegalArgumentException alreadySet) {
+      // some JDKs refuse; nothing to assert then
+      return;
+    }
+    assertFalse(retries(second));
+  }
+
+  @Test
+  public void testDoesNotRetryWhenNothingInTheChainIsRetriable() {
+    // control: unrolling must not make everything retriable
+    assertFalse(retries(new ExecutionException(new IllegalStateException("not retriable"))));
+    assertFalse(retries(new IllegalArgumentException("not retriable")));
+  }
+}