Fix IndicesRequestCacheCleanupIT flakiness by removing too-short assertBusy timeouts

[AndreKurait]

Description

Fixes long-standing flakiness in IndicesRequestCacheCleanupIT by removing
too-short custom timeouts on every assertBusy call.

Root cause. Every assertBusy in the file passed
cacheCleanIntervalInMillis * MAX_ITERATIONS (with MAX_ITERATIONS = 5)
as the wall-clock timeout. The cacheCleanIntervalInMillis values set per
test were 1, 10, 50, or 100 ms — yielding timeout budgets of just
5 ms, 50 ms, 250 ms, or 500 ms for async cluster operations (flush,
force-merge, scheduled cache-cleanup, stats propagation across nodes).
Under normal CI load these windows are routinely missed; the resulting
AssertionError has no race behind it — the test just ran out of time
while the cluster was still converging.

Evidence that this is timeout sizing, not a product race:

• The historical stack trace in #13503
  shows AssertionError at the assertion line, not a timeout/interrupt
  exception.
• Every flaky method AutoCut has filed against this class (see [AUTOCUT] Gradle Check Flaky Test Report for IndicesRequestCacheCleanupIT #21397)
  uses the buggy pattern — a 1:1 overlap.
• Locally reproduces as transient timeout under load; green with the
  default 10-s budget.

Fix. Drop the custom timeout on all 12 assertBusy call sites and
rely on assertBusy's default 10-second budget, which is the convention
elsewhere in the codebase for async cluster assertions. Also remove the
now-unused MAX_ITERATIONS constant and java.util.concurrent.TimeUnit
import.

Why this is safe for the "negative" tests (e.g.
testCacheCleanupSkipsWithHighStalenessThreshold, which asserts cleanup
did not happen): assertBusy returns on the first successful
iteration, so the 10-s budget is a no-op on passing runs — it only
extends the failure path.

cacheCleanIntervalInMillis values themselves are unchanged — they
legitimately configure the cleaner thread cadence so the test can observe
cleanup quickly; only the test's wall-clock deadline was wrong.

Related Issues

Relates to #21397

Verification

Ran locally on the modified file against a fresh main:

./gradlew :server:internalClusterTest \
  --tests "...IndicesRequestCacheCleanupIT.testCacheCleanupOnEqualStalenessAndThreshold" \
  -Dtests.iters=10
# BUILD SUCCESSFUL (10/10)

./gradlew :server:internalClusterTest \
  --tests "...testStaleKeysCleanupWithLowThreshold" \
  --tests "...testCacheCleanupSkipsWithHighStalenessThreshold" \
  --tests "...testStaleKeysRemovalWithoutExplicitThreshold" \
  -Dtests.iters=5
# BUILD SUCCESSFUL (15/15)

25 consecutive passes across 4 of the previously-flaky methods.

Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable.
• Public documentation issue/PR created, if applicable.

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 Negative Test Concern The testCacheCleanupSkipsWithHighStalenessThreshold test asserts that cleanup did NOT happen (memory size > 0). With the default 10-second assertBusy timeout, if the cache cleaner does run and clean up within those 10 seconds, the assertion will pass spuriously on the first iteration and the test will not catch a regression. The PR description acknowledges this but it warrants careful validation that the cache clean interval is short enough relative to 10 seconds to make this a meaningful test. assertBusy(() -> { // assert segment counts stay the same assertEquals(1, getSegmentCount(client, index1)); assertEquals(1, getSegmentCount(client, index2)); // cache cleaner should NOT have cleaned up the stale key from index 2 assertTrue(getRequestCacheStats(client, index2).getMemorySizeInBytes() > 0); // cache cleaner should NOT have cleaned from index 1 assertEquals(finalMemorySizeForIndex1, getRequestCacheStats(client, index1).getMemorySizeInBytes()); }); Inline Lambda Style The single-line assertBusy lambda on line 629 uses a block body () -> { assertEquals(...); } which is inconsistent with the multi-line style used elsewhere in the file. Consider expanding it for consistency and readability. assertBusy(() -> { assertEquals(0, getRequestCacheStats(client, index1).getMemorySizeInBytes()); });

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix negative assertion timing for cache cleaner The assertBusy default timeout is 10 seconds, which may be insufficient for a "should NOT clean" assertion — the test passes as long as the condition holds at any point within the timeout, but it doesn't guarantee the cleaner has had enough time to run and not clean. Consider using Thread.sleep or a fixed wait based on cacheCleanIntervalInMillis to ensure the cleaner has had at least one opportunity to run before asserting the negative condition. server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheCleanupIT.java [473-476] -assertBusy(() -> { - // cache cleaner should NOT have cleaned up the stale key from index 2 - assertTrue(getRequestCacheStats(client, index2).getMemorySizeInBytes() > 0); -}); +Thread.sleep(cacheCleanIntervalInMillis * 2); +assertTrue(getRequestCacheStats(client, index2).getMemorySizeInBytes() > 0); Suggestion importance[1-10]: 6 __ Why: The suggestion raises a valid concern: using assertBusy for a "should NOT clean" assertion is semantically incorrect, as assertBusy retries until the condition passes, which could pass immediately before the cleaner runs. A fixed wait like Thread.sleep(cacheCleanIntervalInMillis * 2) followed by a direct assertion would be more appropriate for negative conditions. However, Thread.sleep in tests is generally discouraged, and the suggestion doesn't account for the fact that the original code also used a timeout-based approach.	Low

[github-actions]

❕ Gradle check result for fe5c681: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.49%. Comparing base (d7573c0) to head (fe5c681).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #21494      +/-   ##
============================================
+ Coverage     73.44%   73.49%   +0.04%     
- Complexity    74429    74466      +37     
============================================
  Files          5970     5970              
  Lines        338276   338262      -14     
  Branches      48760    48758       -2     
============================================
+ Hits         248453   248603     +150     
+ Misses        69979    69854     -125     
+ Partials      19844    19805      -39     

☔ View full report in Codecov by Sentry.
📢 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.