Lab 5.4: Fix It — Un-Mute a Flaky Test

Background

A flaky test passes sometimes and fails sometimes with no code change. In a randomized framework like OpenSearch's, flakiness is endemic: a test that races against asynchronous cluster state, refresh timing, or allocation will fail on some seeds and pass on others. OpenSearch's policy is firm — when a test flakes, you do not delete it and you do not @Ignore it. You mute it with @AwaitsFix(bugUrl=...) pointing at a tracking issue, file the issue with the flaky-test label, and then someone (you, in this lab) reproduces it deterministically, finds the race, fixes it, and un-mutes it.

This Fix-It lab gives you a realistic flaky *IT, muted with @AwaitsFix. You will reproduce the failure with a pinned seed and -Dtests.iters, identify the race (a missing ensureGreen, a refresh timing assumption, a Thread.sleep standing in for real synchronization), apply the minimal diff that makes it deterministic, and remove the @AwaitsFix.

Note: The systematic workflow for flaky tests — triage, reproduce, classify, fix, un-mute — is the subject of Issue Roadmap Stage 9: Flaky Tests. This lab is the hands-on counterpart. Read that stage for the policy; do this lab for the muscle memory.

Why This Lab Matters for Contributors

• flaky-test issues are an explicit, well-labeled on-ramp for new contributors. They are real, visible, and maintainers are grateful for them — flaky CI is a tax on the whole project.
• Fixing flakiness teaches you the asynchronous nature of OpenSearch better than any happy-path test: refresh, allocation, cluster-state publication, and recovery are all eventually-consistent.
• The wrong fix (sprinkling Thread.sleep) is a trap that looks like it works and silently makes CI slower and still-flaky. Learning to spot and avoid it separates competent contributors from cargo-cult ones.
• "Un-mute a flaky test" is a concrete, mergeable PR with a clear before/after: red → green, and one fewer @AwaitsFix in the tree.

Prerequisites

• Lab 5.3 completed — you understand ensureGreen, assertBusy, and why they beat Thread.sleep.
• You can run a scoped integ test and read its HTML report.
• You can reproduce a randomized failure from a printed -Dtests.seed=... line.

./gradlew :server:internalClusterTest --tests "*SomeIT" -Dtests.iters=5   # sanity: the task works

The Muted Test

Here is the flaky test as you find it in the tree — already muted. It refreshes one node, then searches on (potentially) another node, asserting the doc is visible. The bug: refresh is per-shard and asynchronous across replicas, so a search routed to a not-yet-refreshed copy intermittently misses the document.

/*
 * SPDX-License-Identifier: Apache-2.0
 *
 * The OpenSearch Contributors require contributions made to
 * this file be licensed under the Apache-2.0 license or a
 * compatible open source license.
 */

package org.opensearch.search.basic;

import org.opensearch.common.settings.Settings;
import org.opensearch.test.OpenSearchIntegTestCase;
import org.apache.lucene.tests.util.LuceneTestCase.AwaitsFix;

import static org.opensearch.index.query.QueryBuilders.matchQuery;
import static org.hamcrest.Matchers.equalTo;

public class SearchVisibilityAfterIndexIT extends OpenSearchIntegTestCase {

    @AwaitsFix(bugUrl = "https://github.com/opensearch-project/OpenSearch/issues/14321")
    public void testIndexedDocumentIsImmediatelyVisible() throws Exception {
        internalCluster().startNodes(3);
        createIndex("vis", Settings.builder()
            .put("index.number_of_shards", 1)
            .put("index.number_of_replicas", 2)   // 3 copies of the one shard, one per node
            .build());

        // BUG 1: no ensureGreen — replicas may still be initializing when we index/search.

        client().prepareIndex("vis").setId("1").setSource("title", "opensearch").get();

        // BUG 2: refresh is async per-shard-copy; this does not guarantee all copies are refreshed.
        client().admin().indices().prepareRefresh("vis").get();

        // BUG 3: a single immediate search may be routed to a copy that hasn't refreshed yet.
        long hits = client().prepareSearch("vis")
            .setQuery(matchQuery("title", "opensearch"))
            .get().getHits().getTotalHits().value();

        assertThat(hits, equalTo(1L));
    }
}

Step-by-Step Tasks

Step 1: Reproduce the flakiness

A flaky test passes "usually," so a single run tells you nothing. First, temporarily remove the @AwaitsFix locally (do not commit this yet) so the runner will execute it, then iterate it many times:

# Run the method 50 times. With the bug present, at least one iteration should fail.
./gradlew :server:internalClusterTest \
  --tests "org.opensearch.search.basic.SearchVisibilityAfterIndexIT.testIndexedDocumentIsImmediatelyVisible" \
  -Dtests.iters=50

When an iteration fails, the runner prints the reproduce line. Copy it verbatim — the seed pins the race:

REPRODUCE WITH: ./gradlew ':server:internalClusterTest' \
  --tests "org.opensearch.search.basic.SearchVisibilityAfterIndexIT.testIndexedDocumentIsImmediatelyVisible" \
  -Dtests.seed=7F0C19A4B2E5D660 -Dtests.locale=fr-FR -Dtests.timezone=UTC

Now you can reproduce it on demand:

./gradlew :server:internalClusterTest \
  --tests "*SearchVisibilityAfterIndexIT.testIndexedDocumentIsImmediatelyVisible" \
  -Dtests.seed=7F0C19A4B2E5D660 -Dtests.iters=10

Warning: If you cannot reproduce it locally even at -Dtests.iters=200, do not guess at a fix. Flaky tests are sometimes environment-sensitive (CPU count, disk speed). Raise iters, try -Dtests.jvms=1, or run under load (stress -c 8 in another shell) to bias the timing the way CI does. A fix you cannot validate is not a fix.

Step 2: Read the failure and classify the race

The assertion message will be something like:

java.lang.AssertionError:
Expected: <1L>
     but: was <0L>

Zero hits, not an exception. The doc was indexed and acknowledged but a search missed it — a visibility race. OpenSearch failures fall into a small number of flakiness archetypes; learn to classify quickly:

This one is a refresh/visibility race compounded by a missing ensureGreen.

Step 3: Confirm the race with TRACE logging

Prove your hypothesis instead of guessing. Turn on TRACE for the engine/translog and re-run the pinned seed; in the captured node logs (HTML report) you will see refresh events arrive on the three shard copies at different times, and the search hitting one before its refresh.

// Add temporarily above the method:
import org.opensearch.test.junit.annotations.TestLogging;

@TestLogging(value = "org.opensearch.index.engine:TRACE,org.opensearch.index.shard:TRACE", reason = "diagnose visibility race")

./gradlew :server:internalClusterTest --tests "*SearchVisibilityAfterIndexIT*" \
  -Dtests.seed=7F0C19A4B2E5D660
open server/build/reports/tests/internalClusterTest/index.html

You will see, on the failing seed, the search executing against a replica whose refresh for the new segment has not yet completed — confirming visibility, not durability or allocation alone, is the issue. For the mechanics of why refresh is per-copy and asynchronous, see the refresh / flush / merge deep dive and the engine internals deep dive.

Step 4: Choose the correct fix (not a sleep)

There are three legitimate ways to make a just-indexed doc reliably visible. Choose based on what the test is actually trying to prove:

And add the structural fix the test is missing regardless: ensureGreen before you touch the index, so you are not also racing allocation.

Warning: The tempting "fix" is Thread.sleep(1000) before the search. It will appear to work on your machine. It is wrong because: (1) on slow/loaded CI 1s is sometimes not enough → still flaky; (2) on fast machines it wastes a full second every run → slower CI for everyone; (3) it hides the real synchronization point, so the next person can't tell what is being waited for. Never paper over a race with a sleep. Wait for the condition, not for the clock.

Step 5: Apply the diff

Here is the minimal patch. It adds the missing ensureGreen, makes visibility deterministic with an immediate-refresh write, and wraps the assertion in assertBusy as a belt-and-suspenders against any residual relocation timing — and removes the @AwaitsFix.

--- a/server/src/internalClusterTest/java/org/opensearch/search/basic/SearchVisibilityAfterIndexIT.java
+++ b/server/src/internalClusterTest/java/org/opensearch/search/basic/SearchVisibilityAfterIndexIT.java
@@ -7,18 +7,21 @@
 package org.opensearch.search.basic;

 import org.opensearch.common.settings.Settings;
+import org.opensearch.action.support.WriteRequest;
 import org.opensearch.test.OpenSearchIntegTestCase;
-import org.apache.lucene.tests.util.LuceneTestCase.AwaitsFix;
+
+import java.util.concurrent.TimeUnit;

 import static org.opensearch.index.query.QueryBuilders.matchQuery;
 import static org.hamcrest.Matchers.equalTo;

 public class SearchVisibilityAfterIndexIT extends OpenSearchIntegTestCase {

-    @AwaitsFix(bugUrl = "https://github.com/opensearch-project/OpenSearch/issues/14321")
     public void testIndexedDocumentIsImmediatelyVisible() throws Exception {
         internalCluster().startNodes(3);
         createIndex("vis", Settings.builder()
             .put("index.number_of_shards", 1)
             .put("index.number_of_replicas", 2)
             .build());

-        // BUG 1: no ensureGreen — replicas may still be initializing when we index/search.
+        // FIX 1: wait until all 3 copies are STARTED before indexing — removes the allocation race.
+        ensureGreen("vis");

-        client().prepareIndex("vis").setId("1").setSource("title", "opensearch").get();
+        // FIX 2: IMMEDIATE refreshes every active copy before the call returns — removes the
+        //        per-copy refresh-timing race deterministically (no clock involved).
+        client().prepareIndex("vis").setId("1").setSource("title", "opensearch")
+            .setRefreshPolicy(WriteRequest.RefreshPolicy.IMMEDIATE)
+            .get();

-        // BUG 2: refresh is async per-shard-copy; this does not guarantee all copies are refreshed.
-        client().admin().indices().prepareRefresh("vis").get();
-
-        // BUG 3: a single immediate search may be routed to a copy that hasn't refreshed yet.
-        long hits = client().prepareSearch("vis")
-            .setQuery(matchQuery("title", "opensearch"))
-            .get().getHits().getTotalHits().value();
-
-        assertThat(hits, equalTo(1L));
+        // FIX 3: assert on the converged condition, not a single shot. With IMMEDIATE this passes
+        //        first try, but assertBusy keeps it robust against any residual relocation.
+        assertBusy(() -> {
+            long hits = client().prepareSearch("vis")
+                .setQuery(matchQuery("title", "opensearch"))
+                .get().getHits().getTotalHits().value();
+            assertThat(hits, equalTo(1L));
+        }, 30, TimeUnit.SECONDS);
     }
 }

Note: RefreshPolicy.IMMEDIATE is the right tool here because the test's whole point is "index then read immediately." If the test were instead about a realistic delayed read, you would keep a normal write and rely on assertBusy to poll until visible — never on a fixed sleep.

Step 6: Validate the fix is real

Re-run on the seed that used to fail, then hammer with iters. A real fix passes the previously failing seed every time and survives many iterations:

# 1) The exact seed that failed before must now pass repeatedly.
./gradlew :server:internalClusterTest --tests "*SearchVisibilityAfterIndexIT*" \
  -Dtests.seed=7F0C19A4B2E5D660 -Dtests.iters=20

# 2) Broad iteration with fresh random seeds.
./gradlew :server:internalClusterTest --tests "*SearchVisibilityAfterIndexIT*" -Dtests.iters=100

Both must be green. If iter 73 fails, you have not found the whole race — go back to Step 3.

Step 7: Un-mute, close the loop, and ship

1. Confirm the @AwaitsFix annotation and its now-unused import are gone (the diff above removes both).
2. Add a CHANGELOG.md entry:

   ### Fixed
   - Fix and un-mute flaky `SearchVisibilityAfterIndexIT` (visibility race: missing `ensureGreen` + non-deterministic refresh) ([#NNNNN](https://github.com/opensearch-project/OpenSearch/pull/NNNNN))
   

3. Run precommit and format:

   ./gradlew spotlessApply
   ./gradlew :server:precommit
   

4. Commit with DCO sign-off and reference the tracking issue so the bot links the PR and the issue auto-closes on merge:

   git checkout -b flaky/fix-search-visibility-it
   git add server/src/internalClusterTest CHANGELOG.md
   git commit -s -m "Fix and un-mute flaky SearchVisibilityAfterIndexIT
   
   The test raced refresh visibility across replica copies and lacked an
   ensureGreen. Make visibility deterministic with RefreshPolicy.IMMEDIATE
   and assertBusy; remove @AwaitsFix.
   
   Fixes #14321"
   

Pitfalls of "Fixing" Flakiness with Sleeps

The single rule: wait for a condition, never for the clock. ensureGreen, assertBusy, RefreshPolicy.IMMEDIATE, and ensureStableCluster are condition-waits. Thread.sleep is a clock-wait and is almost always the wrong answer in a test.

Implementation Requirements / Deliverables

• The flaky failure reproduced from a pinned -Dtests.seed=... (paste the reproduce line).
• The race classified using the archetype table, with the TRACE-log evidence that confirms it.
• A minimal diff that fixes the race using condition-waits (no Thread.sleep).
• The @AwaitsFix annotation and its now-unused import removed.
• Proof of fix: the previously-failing seed passes ≥20×, and ≥100 fresh iters are green.
• ./gradlew :server:precommit passes; a CHANGELOG.md "Fixed" entry; DCO-signed commit referencing the tracking issue.

Troubleshooting

Expected Output

Before (muted):

> Task :server:internalClusterTest
org.opensearch.search.basic.SearchVisibilityAfterIndexIT >
  testIndexedDocumentIsImmediatelyVisible SKIPPED   (@AwaitsFix)

After (fixed and un-muted), across 100 iterations:

> Task :server:internalClusterTest
org.opensearch.search.basic.SearchVisibilityAfterIndexIT >
  testIndexedDocumentIsImmediatelyVisible PASSED

BUILD SUCCESSFUL in 2m 03s

Stretch Goals

1. Fix it the other way. Instead of RefreshPolicy.IMMEDIATE, keep a normal write, then refresh("vis") and poll the search with assertBusy. Compare run times and reason about which better reflects production behavior.
2. Find a real one. Search github.com/opensearch-project/OpenSearch/issues?q=label:flaky-test, pick an open issue with an AwaitsFix you can locate in the tree, reproduce it from the seed in the issue, and attempt a fix.
3. Write a regression guard. Add a comment in the test linking the fixed issue, so a future "optimization" that reintroduces the race is caught in review.
4. Build a flaky-test hunter. Wrap your CI invocation in a loop that runs a target with high iters and bisects to the first failing seed — the same technique maintainers use to triage the flaky-test backlog.

Validation / Self-check

1. Why does a single run of a flaky test tell you nothing? What two flags do you combine to reproduce it reliably, and what does each do?
2. Classify the race in SearchVisibilityAfterIndexIT using the archetype table. What evidence (from TRACE logs) confirmed your classification?
3. List the three legitimate fixes for a visibility race and when you'd choose each. Which did the diff use and why?
4. Give three concrete reasons Thread.sleep(1000) is the wrong fix here.
5. What is the difference between waiting for a condition and waiting for the clock? Name three OpenSearch condition-wait helpers.
6. Why must you remove both the @AwaitsFix annotation and its import before the PR is mergeable?
7. How do you prove a flaky fix is real rather than lucky? What two runs gate the PR?

When you can reproduce a flaky failure from its seed, classify the race, fix it with a condition-wait, and un-mute the test with proof it's deterministic, you have completed Level 5. Continue to Level 6: Indexing Path and Storage Engine, and revisit Issue Roadmap Stage 9 when you take a real flaky-test issue.