SOLR-16630: TestCoordinatorRole.testNRTRestart waits for the first add to complete before stopping the PULL node

[nick-boss-tech]

🤖 AI text below 🤖 (posted on behalf of Nick Shanin)

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

Failure

testNRTRestart can stop the PULL replica while an add is using its client. The resulting SolrServerException, caused by ClosedChannelException, escapes the test.

The failure is intermittent. The add loop uses the PULL replica client and catches only SolrException (base test); SolrServerException extends Exception (class). The suspected cause is that JettySolrRunner.stop() closes the cached client used by the loop (stop, client); this is not proven.

Change

Test-only change in TestCoordinatorRole.

Before stopping PULL, the test waits up to two minutes for the first successful add and commit; on timeout it warns and proceeds. The remaining fixed sleeps in testNRTRestart are replaced with latch and cluster-state waits for NRT stop, failover, and replica toggling (first-add wait, signal after add and commit, NRT and failover, query-count phases). No Thread.sleep remains in the class.

Tflobbe's follow-up at 2a35f8d makes the toggle thread stop awaiting query counts once the main loop sets done, instead of waiting until the end of the time window (helper). Tflobbe reported the test running in about 7 seconds versus about 30 seconds on main (comment).

The phase lengths are counts of completed queries. Their 0 to 70 range was estimated from the old roughly 100ms query pace; it does not imply equivalent elapsed time (comment).

Proof

The recorded base-code run failed on 2026-10-11 with the exception described above, before the first add succeeded.

At c9caf1e4b815795fd95b9ab454ab41ea0e8b0961, the full TestCoordinatorRole class passed four consecutive runs on 2026-10-14: 9 tests, no failures or errors in each. Three runs used one randomization seed and the fourth used another. The module check and Error Prone compile also passed at that commit. These focused results predate 2a35f8d and the comment-only a02cc7b commit, so they do not claim to cover those later commits.

Limits

A stale pooled connection to PULL without a replica stop is not addressed. The add loop still catches only SolrException. This is a test-harness change and does not fix a possible product-level cause.

Files changed: TestCoordinatorRole and the SOLR-16630 changelog.

AI assistance

AI agents assisted with research, implementation, review, and drafting. Nick Shanin directed the work and takes responsibility for this contribution.

[tflobbe]

I think the changes are an improvement, but I'm wondering if we can go even further, replacing some more of the Thread.sleep(...) with latches. That should make the test more reliable and faster

[epugh]

I think the changes are an improvement, but I'm wondering if we can go even further, replacing some more of the Thread.sleep(...) with latches. That should make the test more reliable and faster

@janhoy ^ what @tflobbe just suggested sounds like one of the items that we should be thinking about if were were to revamp our tests to be faster/better/slimmer.. And of course, better looking!

[tflobbe]

@nick-boss-tech , thanks for the change, but maybe we can remove all the sleeps and replace them with individual countdowns, for example, instead of

Thread.sleep(establishBaselineMs);
log.info("stopping NRT jetty ...");
nrtJettyF.stop();

something like

stopNrtJetty.await()
nrtJettyF.stop();

and then have the countDown call for that latch right before

... phase1 NRT queries

[nick-boss-tech]

maybe we can remove all the sleeps and replace them with individual countdowns

I converted all of the remaining sleeps in TestCoordinatorRole. Details are omitted. I updated the PR description.

[tflobbe]

I added a minor change, since I noticed the test was passing only after timeout. With this change, the test runs in ~7 seconds in my machine vs the ~30s of main. @nick-boss-tech let me know what you think