From 9ddf7cc58a7465e15ddede8acefd6bac65c04474 Mon Sep 17 00:00:00 2001 From: Nick Shanin Date: Tue, 6 Oct 2026 17:48:10 -0400 Subject: [PATCH 1/7] SOLR-5754: StreamingSolrClients returns an error snapshot and drains atomically Hypothetical, unrun regression test; see SOLR-5754-TESTING.md. --- .../solr/update/SolrCmdDistributor.java | 3 +- .../solr/update/StreamingSolrClients.java | 17 ++++- .../solr/update/StreamingSolrClientsTest.java | 67 +++++++++++++++++++ 3 files changed, 83 insertions(+), 4 deletions(-) create mode 100644 solr/core/src/test/org/apache/solr/update/StreamingSolrClientsTest.java 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 ea1d27897007..529ac44ac17d 100644 --- a/solr/core/src/java/org/apache/solr/update/SolrCmdDistributor.java +++ b/solr/core/src/java/org/apache/solr/update/SolrCmdDistributor.java @@ -104,7 +104,7 @@ private void doRetriesIfNeeded() throws IOException { // NOTE: retries will be forwards to a single url List errors = new ArrayList<>(this.errors); - errors.addAll(clients.getErrors()); + errors.addAll(clients.drainErrors()); List resubmitList = new ArrayList<>(); if (log.isInfoEnabled() && errors.size() > 0) { @@ -164,7 +164,6 @@ private void doRetriesIfNeeded() throws IOException { } } - clients.clearErrors(); this.errors.clear(); for (SolrError err : resubmitList) { if (err.req.node instanceof ForwardNode) { diff --git a/solr/core/src/java/org/apache/solr/update/StreamingSolrClients.java b/solr/core/src/java/org/apache/solr/update/StreamingSolrClients.java index dc0b5d775567..64b37499fc91 100644 --- a/solr/core/src/java/org/apache/solr/update/StreamingSolrClients.java +++ b/solr/core/src/java/org/apache/solr/update/StreamingSolrClients.java @@ -48,7 +48,8 @@ public class StreamingSolrClients { private HttpJettySolrClient httpClient; private Map solrClients = new HashMap<>(); - private List errors = Collections.synchronizedList(new ArrayList<>()); + // written by the client runner threads, see ErrorReportingConcurrentUpdateSolrClient + final List errors = Collections.synchronizedList(new ArrayList<>()); private ExecutorService updateExecutor; @@ -57,8 +58,20 @@ public StreamingSolrClients(UpdateShardHandler updateShardHandler) { this.httpClient = updateShardHandler.getUpdateOnlyHttpClient(); } + /** A snapshot of the errors reported so far; later errors do not show up in it. */ public List getErrors() { - return errors; + synchronized (errors) { + return new ArrayList<>(errors); + } + } + + /** Returns the errors reported so far and clears them, so no error can arrive in between. */ + public List drainErrors() { + synchronized (errors) { + List drained = new ArrayList<>(errors); + errors.clear(); + return drained; + } } public void clearErrors() { diff --git a/solr/core/src/test/org/apache/solr/update/StreamingSolrClientsTest.java b/solr/core/src/test/org/apache/solr/update/StreamingSolrClientsTest.java new file mode 100644 index 000000000000..ae50cf95ecfa --- /dev/null +++ b/solr/core/src/test/org/apache/solr/update/StreamingSolrClientsTest.java @@ -0,0 +1,67 @@ +/* + * 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 static org.mockito.Mockito.mock; + +import java.util.List; +import org.apache.solr.SolrTestCase; +import org.apache.solr.update.SolrCmdDistributor.SolrError; + +public class StreamingSolrClientsTest extends SolrTestCase { + + private static SolrError error(int statusCode) { + SolrError error = new SolrError(); + error.statusCode = statusCode; + return error; + } + + public void testGetErrorsIsASnapshot() { + StreamingSolrClients clients = new StreamingSolrClients(mock(UpdateShardHandler.class)); + clients.errors.add(error(500)); + + List snapshot = clients.getErrors(); + assertEquals(1, snapshot.size()); + + // an error reported by a runner thread after the snapshot was taken is not part of it + clients.errors.add(error(503)); + assertEquals(1, snapshot.size()); + assertEquals(2, clients.getErrors().size()); + + // clearing the clients does not empty a list handed out earlier + clients.clearErrors(); + assertEquals(1, snapshot.size()); + assertEquals(0, clients.getErrors().size()); + } + + public void testDrainErrorsReturnsAndClears() { + StreamingSolrClients clients = new StreamingSolrClients(mock(UpdateShardHandler.class)); + clients.errors.add(error(500)); + clients.errors.add(error(503)); + + List drained = clients.drainErrors(); + assertEquals(2, drained.size()); + assertEquals(500, drained.get(0).statusCode); + assertEquals(503, drained.get(1).statusCode); + assertTrue(clients.getErrors().isEmpty()); + + // an error that arrives after the drain is kept for the next round of retries + clients.errors.add(error(502)); + assertEquals(1, clients.drainErrors().size()); + assertTrue(clients.drainErrors().isEmpty()); + } +} From b13ac9feedce875caed7a636bef8476647726d99 Mon Sep 17 00:00:00 2001 From: Nick Shanin Date: Tue, 6 Oct 2026 17:48:11 -0400 Subject: [PATCH 2/7] SOLR-5754: add changelog fragment --- .../SOLR-5754-streaming-clients-error-snapshot.yml | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 changelog/unreleased/SOLR-5754-streaming-clients-error-snapshot.yml diff --git a/changelog/unreleased/SOLR-5754-streaming-clients-error-snapshot.yml b/changelog/unreleased/SOLR-5754-streaming-clients-error-snapshot.yml new file mode 100644 index 000000000000..eed557767b75 --- /dev/null +++ b/changelog/unreleased/SOLR-5754-streaming-clients-error-snapshot.yml @@ -0,0 +1,8 @@ +title: "StreamingSolrClients.getErrors() returns a snapshot instead of the live synchronized list, and the retry pass drains errors atomically so an error reported meanwhile is not cleared unseen" +type: fixed +authors: + - name: Nick Shanin + nick: nick-boss-tech +links: + - name: SOLR-5754 + url: https://issues.apache.org/jira/browse/SOLR-5754 From 36fe859d5fb3499ce3283a71f5dbab33cc5ad75e Mon Sep 17 00:00:00 2001 From: Nick Shanin Date: Tue, 6 Oct 2026 17:48:13 -0400 Subject: [PATCH 3/7] SOLR-5754: add hypothetical-reproduction handoff doc --- SOLR-5754-TESTING.md | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) create mode 100644 SOLR-5754-TESTING.md diff --git a/SOLR-5754-TESTING.md b/SOLR-5754-TESTING.md new file mode 100644 index 000000000000..f06d1c719f91 --- /dev/null +++ b/SOLR-5754-TESTING.md @@ -0,0 +1,31 @@ +# SOLR-5754 - hypothetical reproduction (nothing was compiled or run) + +JIRA (Mark Miller): `StreamingSolrServers` returns its synchronized list of errors; "we should return a copy". The +audit note was "fix version set 4.9/6.0" (Uwe's bulk version move, not a fix). On `upstream/main` the class is +`StreamingSolrClients` and `getErrors()` still returns the live `Collections.synchronizedList`. + +A concrete consequence sits next to it in `SolrCmdDistributor.doRetriesIfNeeded`: it copies the errors +(`errors.addAll(clients.getErrors())`), then sleeps for the retry backoff, then calls `clients.clearErrors()`. An +error a runner thread reports during that window is cleared without ever being looked at (no retry, not in +`allErrors`). + +## Change +- `StreamingSolrClients.getErrors()` returns a snapshot copy taken under the list's lock. +- New `drainErrors()` copies and clears under the same lock; `doRetriesIfNeeded` uses it and drops the later + `clearErrors()` call (the method stays, now unused in main code). +- The `errors` field is package-private so the test can stand in for the runner threads. + +## Test (guessed) +`StreamingSolrClientsTest` (Mockito `UpdateShardHandler`, so the constructor needs no real executor or client): +snapshot is not changed by later adds or `clearErrors()`; `drainErrors()` returns in order and empties; an error added +after a drain survives for the next drain. + +## Guesses to verify first +- `mock(UpdateShardHandler.class)` is enough for the constructor (`getUpdateExecutor()` and + `getUpdateOnlyHttpClient()` return null, nothing dereferences them there). +- No caller relied on the live view: `getErrors()` is only called from `doRetriesIfNeeded` on main. +- Whether errors can really arrive during the backoff sleep in practice (`blockUntilFinished` normally ran first); + the change is harmless if they cannot. + +## Fail-before +Without the change the first test fails (`snapshot.size()` becomes 2) and the second does not compile (`drainErrors`). From c658904626e57354042684c509d700cd78264110 Mon Sep 17 00:00:00 2001 From: Nick Shanin Date: Fri, 9 Oct 2026 03:36:34 +0000 Subject: [PATCH 4/7] SOLR-5754: claim the error snapshot change as hardening in the changelog and testing note --- SOLR-5754-TESTING.md | 20 ++++++++++++------- ...-5754-streaming-clients-error-snapshot.yml | 4 ++-- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/SOLR-5754-TESTING.md b/SOLR-5754-TESTING.md index f06d1c719f91..c2a375c5181a 100644 --- a/SOLR-5754-TESTING.md +++ b/SOLR-5754-TESTING.md @@ -4,10 +4,13 @@ JIRA (Mark Miller): `StreamingSolrServers` returns its synchronized list of erro audit note was "fix version set 4.9/6.0" (Uwe's bulk version move, not a fix). On `upstream/main` the class is `StreamingSolrClients` and `getErrors()` still returns the live `Collections.synchronizedList`. -A concrete consequence sits next to it in `SolrCmdDistributor.doRetriesIfNeeded`: it copies the errors -(`errors.addAll(clients.getErrors())`), then sleeps for the retry backoff, then calls `clients.clearErrors()`. An -error a runner thread reports during that window is cleared without ever being looked at (no retry, not in -`allErrors`). +A possible consequence sits next to it in `SolrCmdDistributor.doRetriesIfNeeded`: it copies the errors +(`errors.addAll(clients.getErrors())`), then sleeps for the retry backoff, then calls `clients.clearErrors()`. If +an error were reported during that window it would be cleared without ever being looked at (no retry, not in +`allErrors`). Reading the call paths afterwards says the window appears unreachable in practice: +`blockAndDoRetries()` calls `blockUntilFinished()` first, and after it returns no runner thread is active to add +to the list. So this change is hardening, not a shown bug fix: how the errors are read and cleared no longer +depends on that timing either way. ## Change - `StreamingSolrClients.getErrors()` returns a snapshot copy taken under the list's lock. @@ -24,8 +27,11 @@ after a drain survives for the next drain. - `mock(UpdateShardHandler.class)` is enough for the constructor (`getUpdateExecutor()` and `getUpdateOnlyHttpClient()` return null, nothing dereferences them there). - No caller relied on the live view: `getErrors()` is only called from `doRetriesIfNeeded` on main. -- Whether errors can really arrive during the backoff sleep in practice (`blockUntilFinished` normally ran first); - the change is harmless if they cannot. +- Whether errors can really arrive during the backoff sleep in practice: reading says no on the paths checked + (`blockUntilFinished` runs first); the change is harmless either way. ## Fail-before -Without the change the first test fails (`snapshot.size()` becomes 2) and the second does not compile (`drainErrors`). +Inconclusive by construction. The test writes `clients.errors` directly and calls `drainErrors()`; on the base +code `errors` is private and `drainErrors()` does not exist, so the test does not compile on base and no failure +on base can be shown as written. The tests pin the new API semantics single-threaded: snapshot isolation, drain +order, and an error added after a drain surviving for the next drain. They do not exercise the concurrent case. diff --git a/changelog/unreleased/SOLR-5754-streaming-clients-error-snapshot.yml b/changelog/unreleased/SOLR-5754-streaming-clients-error-snapshot.yml index eed557767b75..45c93c8e68bb 100644 --- a/changelog/unreleased/SOLR-5754-streaming-clients-error-snapshot.yml +++ b/changelog/unreleased/SOLR-5754-streaming-clients-error-snapshot.yml @@ -1,5 +1,5 @@ -title: "StreamingSolrClients.getErrors() returns a snapshot instead of the live synchronized list, and the retry pass drains errors atomically so an error reported meanwhile is not cleared unseen" -type: fixed +title: "Hardening: StreamingSolrClients.getErrors() returns a snapshot copy instead of the live list, and the retry pass takes and clears the errors in one atomic drain under the list's lock" +type: changed authors: - name: Nick Shanin nick: nick-boss-tech From 7fbe0128d8b070db71a3db1dba2232f8cff0c404 Mon Sep 17 00:00:00 2001 From: Nick Shanin Date: Fri, 9 Oct 2026 05:14:25 +0000 Subject: [PATCH 5/7] SOLR-5754: add the assumeWorkingMockito guard to StreamingSolrClientsTest --- .../org/apache/solr/update/StreamingSolrClientsTest.java | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/solr/core/src/test/org/apache/solr/update/StreamingSolrClientsTest.java b/solr/core/src/test/org/apache/solr/update/StreamingSolrClientsTest.java index ae50cf95ecfa..70b0878926d9 100644 --- a/solr/core/src/test/org/apache/solr/update/StreamingSolrClientsTest.java +++ b/solr/core/src/test/org/apache/solr/update/StreamingSolrClientsTest.java @@ -16,14 +16,21 @@ */ package org.apache.solr.update; +import static org.apache.solr.SolrTestCaseJ4.assumeWorkingMockito; import static org.mockito.Mockito.mock; import java.util.List; import org.apache.solr.SolrTestCase; import org.apache.solr.update.SolrCmdDistributor.SolrError; +import org.junit.BeforeClass; public class StreamingSolrClientsTest extends SolrTestCase { + @BeforeClass + public static void ensureWorkingMockito() { + assumeWorkingMockito(); + } + private static SolrError error(int statusCode) { SolrError error = new SolrError(); error.statusCode = statusCode; From 46b919e2d4e8bc287ac913edb789c21509e1b215 Mon Sep 17 00:00:00 2001 From: Nick Shanin Date: Fri, 9 Oct 2026 06:50:05 +0000 Subject: [PATCH 6/7] SOLR-5754: remove the root testing note for packaging --- SOLR-5754-TESTING.md | 37 ------------------------------------- 1 file changed, 37 deletions(-) delete mode 100644 SOLR-5754-TESTING.md diff --git a/SOLR-5754-TESTING.md b/SOLR-5754-TESTING.md deleted file mode 100644 index c2a375c5181a..000000000000 --- a/SOLR-5754-TESTING.md +++ /dev/null @@ -1,37 +0,0 @@ -# SOLR-5754 - hypothetical reproduction (nothing was compiled or run) - -JIRA (Mark Miller): `StreamingSolrServers` returns its synchronized list of errors; "we should return a copy". The -audit note was "fix version set 4.9/6.0" (Uwe's bulk version move, not a fix). On `upstream/main` the class is -`StreamingSolrClients` and `getErrors()` still returns the live `Collections.synchronizedList`. - -A possible consequence sits next to it in `SolrCmdDistributor.doRetriesIfNeeded`: it copies the errors -(`errors.addAll(clients.getErrors())`), then sleeps for the retry backoff, then calls `clients.clearErrors()`. If -an error were reported during that window it would be cleared without ever being looked at (no retry, not in -`allErrors`). Reading the call paths afterwards says the window appears unreachable in practice: -`blockAndDoRetries()` calls `blockUntilFinished()` first, and after it returns no runner thread is active to add -to the list. So this change is hardening, not a shown bug fix: how the errors are read and cleared no longer -depends on that timing either way. - -## Change -- `StreamingSolrClients.getErrors()` returns a snapshot copy taken under the list's lock. -- New `drainErrors()` copies and clears under the same lock; `doRetriesIfNeeded` uses it and drops the later - `clearErrors()` call (the method stays, now unused in main code). -- The `errors` field is package-private so the test can stand in for the runner threads. - -## Test (guessed) -`StreamingSolrClientsTest` (Mockito `UpdateShardHandler`, so the constructor needs no real executor or client): -snapshot is not changed by later adds or `clearErrors()`; `drainErrors()` returns in order and empties; an error added -after a drain survives for the next drain. - -## Guesses to verify first -- `mock(UpdateShardHandler.class)` is enough for the constructor (`getUpdateExecutor()` and - `getUpdateOnlyHttpClient()` return null, nothing dereferences them there). -- No caller relied on the live view: `getErrors()` is only called from `doRetriesIfNeeded` on main. -- Whether errors can really arrive during the backoff sleep in practice: reading says no on the paths checked - (`blockUntilFinished` runs first); the change is harmless either way. - -## Fail-before -Inconclusive by construction. The test writes `clients.errors` directly and calls `drainErrors()`; on the base -code `errors` is private and `drainErrors()` does not exist, so the test does not compile on base and no failure -on base can be shown as written. The tests pin the new API semantics single-threaded: snapshot isolation, drain -order, and an error added after a drain surviving for the next drain. They do not exercise the concurrent case. From 4bdf281d3cb3caa3a25208c0b97b7e49180b5df5 Mon Sep 17 00:00:00 2001 From: Nick Shanin Date: Thu, 15 Oct 2026 17:43:28 +0000 Subject: [PATCH 7/7] SOLR-5754: remove the nick line from the changelog fragment --- .../unreleased/SOLR-5754-streaming-clients-error-snapshot.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/changelog/unreleased/SOLR-5754-streaming-clients-error-snapshot.yml b/changelog/unreleased/SOLR-5754-streaming-clients-error-snapshot.yml index 45c93c8e68bb..934ec30fb133 100644 --- a/changelog/unreleased/SOLR-5754-streaming-clients-error-snapshot.yml +++ b/changelog/unreleased/SOLR-5754-streaming-clients-error-snapshot.yml @@ -2,7 +2,6 @@ title: "Hardening: StreamingSolrClients.getErrors() returns a snapshot copy inst type: changed authors: - name: Nick Shanin - nick: nick-boss-tech links: - name: SOLR-5754 url: https://issues.apache.org/jira/browse/SOLR-5754