Fix replica stuck behind primary after stale segrep retry

[andrross]

A segment replication retry can finalize against a stale metadata checkpoint returned by the primary (see #20550, #20551), leaving the replica behind the checkpoint it was asked to sync to. This leads to the replica being stale until the next publish. If another publish never happens, it is stale forever. I believe this is the case of flakiness in FullRollingRestartIT where the test fails on timeout waiting for the replica to catch up. The fix is to compare against the replica's achieved checkpoint instead so a round that finalized behind its target retriggers catch-up. In the normal case the achieved checkpoint matches the target, so no extra rounds are introduced.

Related Issues

Resolves #18490

Check List

• Functionality includes testing.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Possible Issue The test testStartReplicationListenerSuccess no longer calls spy.afterIndexShardStarted(spyReplicaShard) before awaiting the latch. The latch is counted down inside the startReplication mock, but nothing triggers startReplication if afterIndexShardStarted is not called. This causes the test to wait indefinitely (or timeout) since the latch will never count down. spy.afterIndexShardStarted(spyReplicaShard); latch.await(2, TimeUnit.SECONDS); verify(spy, (atLeastOnce())).updateVisibleCheckpoint(eq(0L), eq(spyReplicaShard)); // Only the initial entry from afterIndexShardStarted; the done-handler must not retrigger since the replica // caught up to the targeted checkpoint. verify(spy, times(1)).processLatestReceivedCheckpoint(any(), any());

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Use precise verification count The verification uses atLeast(2) which is imprecise for this test scenario. Based on the test description, exactly 2 calls should occur: one from afterIndexShardStarted and one from the done-handler after the first stale round. Use times(2) for more precise verification. server/src/test/java/org/opensearch/indices/replication/SegmentReplicationTargetServiceTests.java [691] -verify(spy, org.mockito.Mockito.atLeast(2)).processLatestReceivedCheckpoint(any(), any()); +verify(spy, times(2)).processLatestReceivedCheckpoint(any(), any()); Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that times(2) is more precise than atLeast(2) for this test scenario. Based on the test description and code flow, exactly 2 calls should occur, making this a valid improvement for test accuracy and clarity.	Medium
	Improve test failure diagnostics The test uses assertTrue with latch.await() but doesn't verify the actual count reached. If the latch times out, the test will fail with a generic message. Consider adding an explicit assertion on the latch count after the await to provide clearer failure diagnostics. server/src/test/java/org/opensearch/indices/replication/SegmentReplicationTargetServiceTests.java [686] -assertTrue("expected the replica to be retriggered until it caught up", latch.await(5, TimeUnit.SECONDS)); +boolean awaitResult = latch.await(5, TimeUnit.SECONDS); +assertTrue("expected the replica to be retriggered until it caught up, but latch count was " + latch.getCount(), awaitResult); Suggestion importance[1-10]: 5 __ Why: The suggestion improves test diagnostics by capturing the latch count on failure. However, the impact is moderate as the existing message already indicates the expected behavior, and latch.getCount() provides only marginal additional debugging value.	Low

[andrross]

FYI @guojialiang92 Issue #20610 seems like the right direction to me, but I think this fix is needed regardless.

[github-actions]

✅ Gradle check result for e0d12d2: SUCCESS

[codecov]

Codecov Report

❌ Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 73.40%. Comparing base (a8692f7) to head (e0d12d2).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
...s/replication/SegmentReplicationTargetService.java	50.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #22099      +/-   ##
============================================
- Coverage     73.52%   73.40%   -0.12%     
+ Complexity    75682    75583      -99     
============================================
  Files          6038     6038              
  Lines        343009   343012       +3     
  Branches      49348    49349       +1     
============================================
- Hits         252189   251790     -399     
- Misses        70746    71225     +479     
+ Partials      20074    19997      -77     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.