Stage 9 — Flaky Test Fixes

What this stage teaches

A flaky test is a test that passes and fails on the same code. Flakes are corrosive: they train contributors to re-run CI until green, which means a real regression hidden behind a flake ships. Fixing flakes is some of the highest-leverage work in the project, and it is how you learn the test framework deeply. The skill:

• Reproduce a randomized failure deterministically from its -Dtests.seed= line, and stress it with -Dtests.iters.
• Tell apart the flake families: a real race condition, an over-tight timing assumption, a leaked port/thread, order-dependence, or genuine nondeterminism in InternalTestCluster.
• Replace Thread.sleep with assertBusy, and replace polling loops with proper awaiting.
• Use @AwaitsFix(bugUrl=...) correctly to mute (with a tracking issue), and — the real goal — un-mute and fix an already-muted test.

Prerequisite: Stage 4 (most integ flakes are cluster-state timing) plus the threadpools & concurrency deep dive. OpenSearch tests use Randomized Testing (RandomizedRunner); a seed determines every random choice in a run.

The flake taxonomy

The last row is the important reframe: a flake is sometimes a real bug the randomizer found. Before muting, always ask "is the production code actually correct for this seed?"

Reproducing a flake

When a randomized test fails, the output prints a reproduce line:

REPRODUCE WITH: ./gradlew ':server:test' --tests "org.opensearch.cluster.SomeTests.testThing" \
  -Dtests.seed=DEADBEEF12345678 -Dtests.locale=en-US -Dtests.timezone=UTC

Run it exactly:

./gradlew ':server:test' --tests "org.opensearch.cluster.SomeTests.testThing" \
  -Dtests.seed=DEADBEEF12345678 -Dtests.locale=en-US -Dtests.timezone=UTC -q

If it reproduces every time with the fixed seed but not without, it is seed-specific — the randomizer is hitting a particular value. If it reproduces only sometimes even with the seed, the nondeterminism is outside the randomizer (threads, wall-clock, the OS) — a true race. Distinguish them by stressing:

# Run the method many times to surface intermittency:
./gradlew ':server:test' --tests "org.opensearch.cluster.SomeTests.testThing" \
  -Dtests.iters=200 -q
# Or loop a whole class with fresh seeds each iteration to estimate the failure rate:
for i in $(seq 1 50); do
  ./gradlew ':server:test' --tests "org.opensearch.cluster.SomeTests" --rerun-tasks -q || echo "FAIL run $i"
done

Finding flaky-test issues

OpenSearch tracks flakes with a dedicated label:

is:issue is:open label:flaky-test no:assignee sort:updated-desc
is:issue is:open label:flaky-test "AwaitsFix" in:body

Cross-check against tests already muted in the code (those are the un-mute-and-fix candidates — the most valuable Stage 9 work):

grep -rn "@AwaitsFix\|@LuceneTestCase.AwaitsFix\|@Repeat\|@Seed" \
  server/src/test/java/ qa/ modules/ | grep -i "bugUrl" | head

Each @AwaitsFix(bugUrl="https://github.com/opensearch-project/OpenSearch/issues/NNNN") points at the tracking issue. Pick one whose issue is still open, reproduce it, fix the root cause, and remove the annotation.

Walked example — un-mute and fix a flaky *IT

Illustrative of the pattern. The grep finds a real muted test; treat the specifics as a stand-in for "a cluster integ test that asserts an asynchronous outcome too eagerly."

Symptom: an *IT (integration test on InternalTestCluster) is muted with @AwaitsFix. The test indexes a doc, then immediately asserts the doc is searchable on a replica — but search visibility is asynchronous (it waits on refresh + replication), so on a slow CI box the assertion runs before the replica caught up. The original author "fixed" it once with Thread.sleep(200), which still flakes, then muted it.

Find and reproduce

grep -rn "@AwaitsFix" server/src/internalClusterTest/java/org/opensearch/ | head
# pick one; read the test and its bugUrl issue, then reproduce:
./gradlew ':server:internalClusterTest' --tests "org.opensearch.search.SomeSearchIT.testReplicaSeesDoc" \
  -Dtests.iters=100 -q

The offending test body:

@AwaitsFix(bugUrl = "https://github.com/opensearch-project/OpenSearch/issues/NNNN")
public void testReplicaSeesDoc() throws Exception {
    client().prepareIndex("idx").setId("1").setSource("f", "v").get();
    Thread.sleep(200);   // hope the replica refreshed
    SearchResponse r = client().prepareSearch("idx")
        .setPreference("_replica").setQuery(matchQuery("f", "v")).get();
    assertHitCount(r, 1);   // flakes: replica not refreshed yet on slow CI
}

Diff — replace timing with an explicit await

--- a/server/src/internalClusterTest/java/org/opensearch/search/SomeSearchIT.java
+++ b/server/src/internalClusterTest/java/org/opensearch/search/SomeSearchIT.java
@@
-    @AwaitsFix(bugUrl = "https://github.com/opensearch-project/OpenSearch/issues/NNNN")
     public void testReplicaSeesDoc() throws Exception {
-        client().prepareIndex("idx").setId("1").setSource("f", "v").get();
-        Thread.sleep(200);
-        SearchResponse r = client().prepareSearch("idx")
-            .setPreference("_replica").setQuery(matchQuery("f", "v")).get();
-        assertHitCount(r, 1);
+        client().prepareIndex("idx").setId("1").setSource("f", "v")
+            .setRefreshPolicy(WriteRequest.RefreshPolicy.WAIT_UNTIL).get();
+        // Wait for the outcome, not a fixed time: poll until the replica is consistent.
+        assertBusy(() -> {
+            SearchResponse r = client().prepareSearch("idx")
+                .setPreference("_replica").setQuery(matchQuery("f", "v")).get();
+            assertHitCount(r, 1);
+        }, 30, TimeUnit.SECONDS);
     }

Two distinct improvements:

1. WAIT_UNTIL refresh policy makes the index request return only once the doc is visible — removing the need to guess at refresh timing at the source.
2. assertBusy polls the actual condition with a generous timeout instead of a fixed sleep. It passes the instant the condition is true (fast on a fast box) and tolerates a slow box (no false failure). The timeout is an upper bound, not an expectation.

Removing the @AwaitsFix line is the deliverable — the test is live again.

Verify it is genuinely de-flaked

./gradlew ':server:internalClusterTest' --tests "org.opensearch.search.SomeSearchIT.testReplicaSeesDoc" \
  -Dtests.iters=200 -q

Two hundred clean iterations is the bar before you claim a flake is fixed. If it still flakes even with assertBusy, the bug is a real race in production code, not the test — escalate to the relevant subsystem stage (4–7) and fix the code.

Close the loop

--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ ### Fixed
+- Un-mute and de-flake `SomeSearchIT.testReplicaSeesDoc` by awaiting replica visibility ([#NNNNN](...))

git add server/.../SomeSearchIT.java CHANGELOG.md
git commit -s -m "Un-mute and de-flake SomeSearchIT.testReplicaSeesDoc"
git push && gh pr create --repo opensearch-project/OpenSearch --fill

In the PR, link the original bugUrl issue with Closes #NNNN and paste your -Dtests.iters=200 clean run as evidence. Maintainers will not trust a flake fix without the stress-run evidence.

assertBusy vs Thread.sleep — the rule

// WRONG: assumes a duration. Flaky on slow machines, slow on fast ones.
Thread.sleep(500);
assertThat(thing.state(), equalTo(READY));

// RIGHT: assert the condition, poll until true or time out.
assertBusy(() -> assertThat(thing.state(), equalTo(READY)), 30, TimeUnit.SECONDS);

assertBusy retries the lambda on AssertionError until it passes or the timeout elapses. Use it for anything asynchronous: cluster-state propagation, refresh, recovery, async listeners. The only correct use of Thread.sleep in a test is when you must verify that something did not happen within a window — and even then, prefer a deterministic signal.

Muting correctly (when you cannot fix it now)

If a flake is real but you cannot root-cause it immediately, mute it with a tracking issue so it is not lost:

@AwaitsFix(bugUrl = "https://github.com/opensearch-project/OpenSearch/issues/NNNN")
public void testSomething() { ... }

Never use plain @Ignore — it has no tracking link and the test rots silently. For a Lucene-level flake, @LuceneTestCase.AwaitsFix is the equivalent. The mute is a debt with a ticket attached, not a fix.

Pitfalls

• Muting instead of investigating. A flake might be a real bug the randomizer found. Always reproduce with the seed and ask whether the production code is correct for that seed before muting.
• Thread.sleep as a fix. It moves the flake's probability, it does not remove it. assertBusy on the real condition is the fix.
• Too-short assertBusy timeout. CI boxes are slow and shared. Use generous timeouts (tens of seconds); the timeout is an upper bound, not a benchmark.
• Leaked threads/ports. OpenSearch's test framework fails the build on leaked threads and searchers. If teardown complains, you did not close something — fix the leak, do not suppress the detector.
• Order dependence via static state. A test that passes alone but fails in the suite has shared mutable static. Reset it in @Before/@After, or remove it.
• Claiming a fix without stress evidence. -Dtests.iters=200 clean (or a for loop of fresh-seed runs) is the minimum proof. Paste it in the PR.
• Removing @AwaitsFix without reading its issue. The tracking issue often contains the prior investigation — read it before you re-derive everything.

Exit criteria — when you're ready for Stage 10

• At least three flaky tests have been de-flaked (un-muted and fixed), each with a -Dtests.iters clean run pasted into the PR.
• You can classify a flake into the taxonomy above from its failure output alone.
• You never reach for Thread.sleep; assertBusy (or WAIT_UNTIL/a deterministic signal) is reflex.
• You have at least once discovered that a "flake" was a real race and escalated it to the owning subsystem instead of muting it.

Determinism is the foundation of measurement. Stage 10 needs exactly that determinism to make a benchmark mean something.