From 06a913af79c54aa6cd9e0a5a1542361f7853313c Mon Sep 17 00:00:00 2001 From: Serhiy Bzhezytskyy Date: Tue, 28 Jul 2026 19:30:29 +0300 Subject: [PATCH 1/2] SOLR-9355: decide update retries from the cause chain, not one fixed position in it checkRetry asked whether a failure was retriable in two different ways depending on which exception happened to be outermost. If it was a SolrServerException it unwrapped to the root cause; otherwise it tested only the top-level type. The async client reports a connection failure as an ExecutionException wrapping the socket cause, so it took the second branch, the leaf test saw ExecutionException, and the update was not retried. The replica then goes into recovery on exactly the transient glitch this issue describes. Both StdNode and ForwardNode now scan the cause chain rather than checking either end of it, because neither end is reliably the right place to look. The async client wraps the failure from above, and Jetty's ClientConnector wraps the underlying failure in a SocketException of its own, so the retriable frame can sit above the root cause as well as below the top. The scan is bounded. A cause chain can be made cyclic, which SolrException's own getRootCause carries a TODO about, so an unbounded walk could spin; real chains are a few frames deep and the cap only guards the pathological case. The retriable set is unchanged: SocketException or SocketTimeoutException for StdNode, ConnectException for ForwardNode. Only where we look changes. The retry bound is untouched and still lives in Req.shouldRetry, which requires retries < maxRetries and excludes delete-by-query. Tests were written before the fix; two of them fail on unmodified main. A further test records a limit rather than hiding it: ClosedChannelException is what the JDK transport reports for a dropped update connection and is retriable nowhere in Solr today, which is a separate question from this one. --- .../checkretry-unroll-exception-chain.yml | 10 ++ .../solr/update/SolrCmdDistributor.java | 50 ++++-- .../solr/update/CheckRetryUnrollTest.java | 151 ++++++++++++++++++ 3 files changed, 195 insertions(+), 16 deletions(-) create mode 100644 changelog/unreleased/checkretry-unroll-exception-chain.yml create mode 100644 solr/core/src/test/org/apache/solr/update/CheckRetryUnrollTest.java diff --git a/changelog/unreleased/checkretry-unroll-exception-chain.yml b/changelog/unreleased/checkretry-unroll-exception-chain.yml new file mode 100644 index 000000000000..d90c541c82cd --- /dev/null +++ b/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 diff --git a/solr/core/src/java/org/apache/solr/update/SolrCmdDistributor.java b/solr/core/src/java/org/apache/solr/update/SolrCmdDistributor.java index 40c972aefcbb..ea1d27897007 100644 --- a/solr/core/src/java/org/apache/solr/update/SolrCmdDistributor.java +++ b/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) { diff --git a/solr/core/src/test/org/apache/solr/update/CheckRetryUnrollTest.java b/solr/core/src/test/org/apache/solr/update/CheckRetryUnrollTest.java new file mode 100644 index 000000000000..fc49ae46578c --- /dev/null +++ b/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. + * + *

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 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"))); + } +} From 061da36262f71c6d8cee339ef44253537254fbfa Mon Sep 17 00:00:00 2001 From: Serhiy Bzhezytskyy Date: Thu, 13 Aug 2026 14:06:32 +0300 Subject: [PATCH 2/2] SOLR-18346: repoint the changelog entry to the issue scoped to this change --- changelog/unreleased/checkretry-unroll-exception-chain.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/changelog/unreleased/checkretry-unroll-exception-chain.yml b/changelog/unreleased/checkretry-unroll-exception-chain.yml index d90c541c82cd..f83740ed3f04 100644 --- a/changelog/unreleased/checkretry-unroll-exception-chain.yml +++ b/changelog/unreleased/checkretry-unroll-exception-chain.yml @@ -6,5 +6,5 @@ type: fixed authors: - name: Serhiy Bzhezytskyy links: - - name: SOLR-9355 - url: https://issues.apache.org/jira/browse/SOLR-9355 + - name: SOLR-18346 + url: https://issues.apache.org/jira/browse/SOLR-18346