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 SolrClient, defaulting to false meaning "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)

SolrClient (false) → HttpSolrClient (shared HTTP types) → per-transport overrides. CloudSolrClient delegates to getHttpClient(); the LB clients ask via the existing getClient(endpoint) — that is what the false default buys, no instanceof in the LB. 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.
• LBSolrClient and LBAsyncSolrClient keep their own control flow. I read "remove the asymmetry rather than patch it in both places" as "no bespoke classifier in each", and each now makes one wasRequestUnsent call. Merging the two near-duplicate methods would be a larger change.
• CloudSolrClient.wasCommError widened protected → public, forced since it now overrides SolrClient.wasCommError.

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]

I don't think this is worth putting in the ref guide. It's a detail and doesn't change how people use SolrJ.

[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.

[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.

[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 ;-)