Fix flaky ChaosMonkeySafeLeaderWithPullReplicasTest

[janhoy]

Description

The nightly ChaosMonkeySafeLeaderWithPullReplicasTest (and other chaos tests) can fail with expected:<0> but was:<1> at assertEquals(0, ((StoppableIndexingThread) thread).getFailCount()). When the chaos monkey stops a node while an update is in flight, the Jetty HTTP/2 client surfaces the loss as an ambiguous IOException (e.g. cancel_stream_error/input_shutdown, ClosedChannelException) that CloudSolrClient does not retry, so the indexing thread records a failure and the test fails. Seen on Jenkins for both main and branch_10x.

Solution

An earlier version of this PR broadened CloudSolrClient.wasCommError() to retry these HTTP/2 stream-failure IOExceptions. That was withdrawn: as @dsmiley pointed out, these protocol errors do not guarantee the server never processed the request, so retrying them for arbitrary (non-idempotent) updates risks double-processing. An audit of the Jetty 12.1.10 source confirms this — cancel_stream_error/input_shutdown is Jetty's ambiguous failure bucket; Jetty reserves RetryableStreamException/RetryableRequestException for the provably-safe subset and deliberately does not use it for this path. So a client-side classification change cannot safely fix the flake.

Instead, this PR fixes the flake where it is safe to do so: in the test framework. StoppableIndexingThread only sends idempotent operations — adds of documents with unique ids, and delete-by-id — so at-least-once delivery is correct, which is exactly what a well-behaved indexing application would do. It now retries the update a bounded number of times (5, with backoff) when the failure's root cause is an IOException (an ambiguous transport error). Solr-level errors (e.g. a 4xx/5xx response) are not retried.

No production code is changed; the fix is confined to StoppableIndexingThread in the test framework.

Tests

• ChaosMonkeySafeLeaderWithPullReplicasTest passes with its previously-failing seed E464797072CE8325; a verbose rerun confirms the retry engages (Retrying update after transient error (attempt 1/5): ClosedChannelException).
• Fresh-seed sweep of ChaosMonkey*, RestartWhileUpdatingTest, and TlogReplayBufferedWhileIndexingTest is green.

Note

The sweep surfaced a separate, pre-existing failure in the same test: the final createCollection("testcollection") overseer-survival check can time out (Timeout waiting for 1 shards and 4 replicas) when a lost response causes LBSolrClient to re-send the non-idempotent admin CREATE to another node. It is independent of this change and perhaps deserves its own issue...

---

The fix is AI assisted

[janhoy]

This test failure was the only nightly test failure I could find apart from Eric's #4709 , so hopefully we can get some successful nightly jenkins runs soon... And with #4712 I hope to get coverage stats from those runs as well, published to Jenkins (or SonarQube https://sonarcloud.io/organizations/apache/projects?sort=-analysis_date would be cool as well)

[epugh]

very much out of my depth, but reading it makes sense... I will say, it's nice to see Chaos testing working! This is a real bug that would happen in production in weird situations right?

[dsmiley]

This business of detecting retry-ability by looking at the exception is rather limited in what we can guarantee. We can only make safe/confident choices here. I challenge that these lower level protocol errors guarantee that the server has not received the message (and thus we can't retry). Ditto for @serhiy-bzhezytskyy attempts weeks ago for ClosedChannelException. Getting a recently flaky test to pass again, in this way, likely will hide a data consistency mistake -- a non-idempotent operation is processed twice.

If we want to truly tackle retry-ability, we need a fundamentally different approach; let's discuss in the dev list. Plan first, execute after. I'm tempted to start the conversation today.

[dsmiley]

Eric, why do you approve a PR that you admit is out of your depth?

[epugh]

Eric, why do you approve a PR that you admit is out of your depth?

fair question... So, I looked at it, and I ran it locally. Things all seem to make sense. So I approved it. But I wanted to caveat that I don't have a ton of confidence in my deep Java and networking skills. From my perspective, this PR looks good, but did want to caveat my approval with the extra detail.

[janhoy]

I'm glad you challenge assumptions like this about retryability @dsmiley. I'm not certain either as you'd expect a library like Jetty to choose different exception to convey such information. I hate having to depend on exception messages to make decisions.

I just asked for an opinion from Copilot.

[janhoy]

Not much help from Copilot. I asked Fable 5 and it concludes that these IOExceptions do not guarantee that it is safe to retry the request. Jetty has separate exceptions RetryableRequestException and RetryableStreamException for that purpose.

I'll set this PR to draft for now.

[janhoy]

The goal for this PR was never retryability in itself, that was a draft fix for a suspected cause, but to get to healthy nightly tests. Suggestion for moving forward in solving the flaky ChaosMonkeySafeLeaderWithPullReplicasTest:

• Recognize that requests from SolrJ towards a solr cluster may fail in may ways, including in a way where we cannot tell whether it was processed on the server, and we cannot safely retry. This is in general something any network client has to live with and accept, and need to implement tailored hardening in the application code.
• Harden how the test executes, e.g. by not failing if a simple update fails, but be prepared to retry that update if the particular update itself is idempotent (add with static ID). This will make that part of the test robust against network failures like this.

Since the proposed CloudSolrClient fix was not a proper solution for this particular test failure, we will remove it from this PR. It may still be valid to harden CloudSolrClient by recognizing Jetty's two Retryable*Exception as retryable, but that can happen in another PR.