SOLR-18402: Consolidate wasRequestUnsent / wasCommError (retry) logic

[chan-dx]

https://issues.apache.org/jira/browse/SOLR-18402

Description

Please find the problem statement in SOLR-18402. Per my comment on the issue, this PR covers CloudSolrClient and LBSolrClient, leaving SolrCmdDistributor and the streaming clients as a follow-up.

Solution

Two predicates on HttpSolrClient, where false means "cannot tell", never "the request was sent":

/** Whether the failure proves the request never reached the server. */
public boolean wasRequestUnsent(Throwable t)

/** Whether this is a transport-level communication failure at all. */
public boolean wasCommError(Throwable t)

HttpSolrClient (shared HTTP types) → per-transport overrides. CloudSolrClient delegates to getHttpClient(); the LB clients ask via getClient(endpoint), now declared to return HttpSolrClient. wasCommError calls wasRequestUnsent, so the subset relation cannot drift.

Three behaviour changes ride on that:

1. CloudSolrClient replays an update only when the transport proves it unsent delegating wasCommError to the transport widens its comm-error set, and ungated that would resend updates on failures it previously left alone. State invalidation stays unconditional; only the resend is gated, at both paths a comm error reaches one in requestWithRetryOnStaleState. That method's INVALID_STATE/404 retry is untouched. HttpSolrCall rejects those before dispatch.
2. HttpJettySolrClient classifies its own transport failures. EofException and ClosedChannelException extend IOException, not SocketException, so a query hitting one failed hard instead of failing over. The HTTP/2 "session closed" path now raises an EofException instead of an opaque new IOException(e), so one predicate covers all three spellings of "connection lost" this class emits.
3. LBSolrClient fails over on a bare IOException, as LBAsyncSolrClient already does. Over Jetty this is narrower than it may look: network failures there are wrapped in a SolrServerException, and a bare IOException escapes only from makeRequest. It now propagates as-is instead of being wrapped by the catch-all; both types were already on the signature.

Left as follow-ups: SolrCmdDistributor and the streaming clients, per my comment on the issue. Also requestAsync, which has no commit listener (this PR is a no-op there), so async behaviour is unchanged; SOLR-18401 lists it under "Also in scope". And HttpJdkSolrClient, which has no commit-listener equivalent, so it still cannot prove a request unsent.

Decisions worth a second look

Flag anything you disagree with; otherwise no action needed.

• CloudSolrClient no longer replays an update on an ambiguous comm error — a dropped connection or a SocketException. Long-standing behaviour; flag it if you would rather keep it.
• Nor on a RouteException carrying 503. Also long-standing. Same offer.
• The INVALID_STATE / 404 retry is left as-is. It exists because those codes indicate stale routing state: it re-reads from ZK and re-routes before replaying. Narrowing it is a separate change.
• CloudSolrClientCacheTest.testCaching now injects a ConnectException where it injected a SocketException. Its injected failures are scaffolding for a fetch-count assertion rather than the subject of the test, and a SocketException on an update is exactly what no longer replays.
• HttpJettySolrClient overrides wasCommError but not wasRequestUnsent. Its commit listener answers the latter better: committed is per-request state, the predicate sees only a Throwable, and EofException occurs both before and after commit.
• The LB transport catch blocks collapsed into one shared mayFailOver, so LBAsyncSolrClient's near-copy goes with it. One behaviour change rides along: wasRequestUnsent previously sat behind isNonRetryable, so && short-circuited it away for retryable requests and only getRootCause() decided. A query proven unsent now fails over even when the root cause isn't an IOException (testQueryIsRetriedWhenUnsentButRootCauseIsNotIO).
• LBSolrClient.getClient(Endpoint) narrowed from SolrClient to HttpSolrClient. Every in-tree implementation already returned one, but an out-of-tree subclass declaring SolrClient gets an AbstractMethodError until it recompiles — reachable, since LBSolrClient(List<Endpoint>) bypasses the Builder. Happy to reverse it. This also made doRequest's instanceof constant-true; that branch and its stale // TODO SOLR-17541 are removed.

Tests

SolrClientErrorClassificationTest (new) — asserts each transport's answers directly, with no
server. The RequestNotSentException and Jetty cases are also checked wrapped in a SolrServerException, pinning the cause-chain walk. The negative cases are the point: a bare IOException and a post-commit EofException prove nothing about delivery. Gap: the HTTP/2 case pins the classification of the shape the throw site produces, not the throw site itself — sendRequest is private, and a lost session is not reproducible in a unit test.

LBSolrClientRetryUnsentTest — three cases for the bare-IOException path.

CloudSolrClientCacheTest — two new cases, both confirmed to fail without their production change: an update not replayed on an ambiguous comm error, and one on a 503 RouteException. testCaching's injected failures changed with it, noted above. Gap: the second retry site has no regression test — it fires only when another thread's cache refresh lands mid-request.

AI disclosure: AI coding assistant was used for code review and PR message preparation.

Checklist

Please review the following and check all that apply:

• I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
• I have created a Jira issue and added the issue ID to my pull request title.
• I have given Solr maintainers access to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation)
• I have developed this patch against the main branch.
• I have run ./gradlew check.
• I have added tests for my changes.
• I have added documentation for the Reference Guide
• I have added a changelog entry for my change

[dsmiley]

Thanks for contributing!

[dsmiley]

These seem HttpSolrClient worthy and not generalized to any SolrClient (e.g. not EmbeddedSolrServer). Even not worthy of CloudSolrClient since it's really the backing HttpSolrClient, which CSC exposes.

[chan-dx]

Agreed, both now on HttpSolrClient - lines 370, overridden per transport. CloudSolrClient's overrides are gone. It calls getHttpClient().wasCommError(...) directly CloudSolrClient - lines 712.

Notes:

• The false default on SolrClient was what let the LB ask without an instanceof, per the sketch on SOLR-18402; with it gone I narrowed getClient(Endpoint) to return HttpSolrClient. No in-tree change: Builder<C extends HttpSolrClient> and LBAsyncSolrClient.getClient already guaranteed it. However, an out-of-tree subclass declaring SolrClient gets AbstractMethodError until it recompiles. Reachable, since LBSolrClient(List<Endpoint>) bypasses the Builder; my own test fixture had to change. Happy to reverse it if you'd rather.

• If the new getClient signature stands, let me know if you want a line in major-changes-in-solr-10.adoc in case anyone subclasses it out-of-tree?

• That also made the private doRequest helper's instanceof constant-true and its fallback unreachable, and its // TODO SOLR-17541 was already stale. Deleted in its own commit (LBSolrClient - lines 614). Happy to drop that commit if you'd rather keep this PR narrower.

[dsmiley]

An implicit outcome of SOLR-18402, I think, is to massively simplify catch blocks that currently are overly complex. Adding an IOException here and not simplifying or generalizing the previous ones is counter to this direction.

[chan-dx]

Fixed, four catch blocks to one, via a new protected mayFailOver(...) (LBSolrClient lines 645, 662) LBAsyncSolrClient shares it, so its near-copy is gone too.

Notes:

Behaviour-identical except one row. wasRequestUnsent sat behind isNonRetryable, so && short-circuited it away for retryable requests and only getRootCause() decided. That misses HttpJettySolrClient lines 525's pre-commit shape: SolrServerException -> RequestNotSentException -> IllegalStateException, where the root cause is the IllegalStateException. Effect: an update proven unsent fails over, an identical query doesn't; testQueryIsRetriedWhenUnsentButRootCauseIsNotIO covers it. That's a behaviour improvement, separable from the refactor. Let me know if you'd rather I reverse it.

[dsmiley]

I generated this JIRA description with AI, and I did read it. But I confess now (and I recall then as well), I'm confused on the distinction between these 2 methods. It's not clear to me why we need a distinction between these two. Feel free to help me figure this out ;-)

[chan-dx]

Happy to explain! My understanding is: wasRequestUnsent is proof of non-delivery. wasCommError includes wasRequestUnsent as one of its causes, plus the other types listed in HttpSolrClient. So wasCommError can be true for a reason that is not proof of non-delivery. For example, SocketException, which can happen right after the server received the request.

Now imagine if we collapsed them into one big wasCommError, we'd lose that distinction. We'd know the socket went wrong, but not whether we have proof the request never landed, so we wouldn't know whether a replay is safe.

They are now in HttpSolrClient.