Stage 9 — Flaky Tests

What this stage teaches

Stage 9 is the unglamorous-but-essential stage, and it is where committers learn whether they can trust you. A flaky test erodes the whole project: once a suite "fails sometimes," people stop reading its failures, and real regressions slip through. Fixing flakes well requires the rarest engineering skill — reproducing a 1-in-30 failure deterministically and proving it gone. You learn:

• The Tez flake taxonomy: Thread.sleep/polling races, undrained dispatchers, timing assumptions tied to a mock clock, /tmp collisions, and @Test(timeout) budgets too tight for a loaded CI agent.
• How to distinguish a flake (a test bug — passes locally, fails 1-in-N on CI) from a real intermittent bug (a production race the test correctly caught).
• The real @Ignore inventory in the checkout, and why each one is or isn't a legitimate quarantine.
• How to prove a fix: stress-loop it hundreds of times, zero failures, before you post the PR.

Patches are 20–150 lines and usually touch only test code. The ones that also touch production code are the valuable ones — the test was right and the code had a race.

Prerequisite: Stage 4 (dispatcher/state-machine mechanics) and the testing framework deep dive. You cannot de-flake a dispatcher test you don't understand.

The real @Ignore inventory

Before you fix flakes, learn what the project has already quarantined. Run:

cd /Users/s0x/src/oss-repos/tez
grep -rn "@Ignore" --include=*.java . -A1

Today that surfaces a small, honest list. Read the reasons — they teach you the difference between a quarantine and a cop-out:

The pattern that matters: a good @Ignore names why and, if it hides a real bug, points at a FIXME or JIRA. An @Ignore with no comment is a smell — it is usually a flake somebody silenced, and hunting those down is itself a Stage 9 contribution (file a JIRA with the analysis even if you don't fix it).

Finding Stage 9 issues today

project = TEZ AND resolution = Unresolved
  AND (summary ~ "flaky" OR summary ~ "intermittent"
       OR summary ~ "fails in jenkins" OR summary ~ "timeout"
       OR labels = "flaky-test")
ORDER BY updated DESC

A second source is Jenkins precommit history. Pick any open JIRA, find its Jenkins run URL in the comments, and look for a test that failed in one run and passed in the next on the same patch — that test is a flake regardless of whether a JIRA exists for it yet. Filing one, with a reproduction rate and a root-cause analysis, is itself a valued contribution even before the fix.

The richest source, though, is your own machine. Run any suite in a loop:

cd /Users/s0x/src/oss-repos/tez
for i in 1 2 3 4 5; do
  mvn -pl tez-dag test -Dtest=TestSpeculation -q 2>&1 | tail -3
done

Any failure that does not repeat is a flake to investigate. And read the history — Tez has a decade of dissected flake fixes to learn the shapes from:

git log --oneline -i --grep=flaky --grep=intermittent

Case study A — TEZ-4206: a flake caused by a test-only backdoor

This is the most instructive flake fix in the history, because the test hook itself was the bug. Read it:

git show 134ecc3c7   # TestSpeculation.testBasicSpeculationPerVertexConf is flaky

The symptom. TestSpeculation failed intermittently — sometimes the speculator speculated the expected number of tasks, sometimes it didn't.

The root cause. The production LegacySpeculator carried a test-only method, scanForSpeculationsForTesting(), that poked a BlockingQueue and called Thread.yield() to nudge the speculation scan. The scan thread blocked on scanControl.poll(wait, ...). The test tried to drive speculation by adding to the queue, but Thread.yield() is a hint, not a guarantee — under CI contention the scan often hadn't run before the assertion.

The fix — delete the backdoor, make the timing real. Mustafa Iman removed the scanControl queue and scanForSpeculationsForTesting() entirely, and let the speculator thread simply Thread.sleep(wait) between evaluations:

// removed: BlockingQueue<Object> scanControl, scanForSpeculationsForTesting()
...
Thread.sleep(wait);   // real, periodic evaluation — no test backdoor

Then the test was made robust instead: the tasks were made to run long enough (NUM_UPDATES_FOR_TEST_TASK = 1200) that the speculator is guaranteed a chance to evaluate them, and the tight @Test(timeout = 10000) budgets were raised to 30000. The added comment is a lesson in itself:

/**
 * MockDAGAppMaster's mock clock advances 1 second at each tick. If we are unlucky
 * this may cause speculator to wait 1 second between each evaluation. If we are
 * really unlucky, our test tasks finish before speculator has a chance to evaluate
 * and speculate them. That is why we want the tasks to take at least one second.
 */

The lesson. A test backdoor that uses Thread.yield() to "synchronise" is a flake generator. The durable fix removed the backdoor and made the test's timing assumptions explicit and generous, rather than relying on scheduler luck. When you find a ...ForTesting() method that pokes a queue and yields, suspect it.

Case study B — TEZ-3924: a flake caused by random test data

git show cf6ea5f62   # TestDefaultSorter fails intermittently due to random keys and RLE/partition collisions

The symptom. TestDefaultSorter failed intermittently. The test generated random keys and values, then asserted an exact output-file byte length and empty- partition count.

The root cause. Random data means random run-length-encoding behaviour and random partition collisions. Sometimes two random keys were equal (RLE kicks in, changing the byte count); sometimes random hashing left a different number of empty partitions. The assertions were written for the average case and failed on the unlucky draws.

The fix — make the data deterministic and the assertion derive from it. Jonathan Eagles replaced random generation with explicit key/value arrays and introduced a SorterWrapper that records the RLE decision per write:

testEmptyCaseFileLengthsHelper(50, new String[] {"a", "b"},   new String[] {"1", "2"});
testEmptyCaseFileLengthsHelper(50, new String[] {"a", "a"},   new String[] {"1", "2"}); // forces RLE
testEmptyCaseFileLengthsHelper(50, new String[] {"aaa","bbb","aaa"}, new String[] {"1","2","3"});

BitSet keyRLEs = new BitSet(keys.length);
for (int i = 0; i < keys.length; i++) {
  boolean isRLE = sorterWrapper.writeKeyValue(new Text(keys[i]), new Text(values[i]));
  keyRLEs.set(i, isRLE);
}

The expected byte length is then computed from the actual RLE decisions, not hard-coded — so the assertion is correct for every input, not just the lucky ones.

The lesson. Random data in a test with exact assertions is a flake by construction. Either make the data deterministic, or make the assertion a property that holds for all data (a count that must be conserved, an invariant). Never hard-code a magic number that depends on data you generated randomly.

Case study C — TEZ-3664: a flake caused by shared /tmp

git show 4c5db4304   # Flaky tests due to writing to /tmp directory

The symptom. A dozen unrelated tests across five modules failed intermittently on CI, always around file operations.

The root cause. Tests wrote to the shared /tmp directory. Two suites running concurrently on the same CI agent — or a leftover file from a previous crashed run — collided. This is not a race in the code under test; it is a race in the test environment.

The fix — isolate each test's working directory. Jonathan Eagles set a per-build temp base in the root pom.xml and pointed the tests at a unique target/-relative path instead of /tmp, across TestTezClient, TestTezCommonUtils, TestDAGAppMaster, TestShuffleHandler, TestLocalMode, and others. The diff is broad but mechanical: every new File("/tmp/...") or System.getProperty("java.io.tmpdir") reliance became a test-local directory under the module's target/.

The lesson. A whole class of flakes has nothing to do with the code — it is tests fighting over shared state (/tmp, fixed ports, a static singleton, the system clock). The fix is isolation: unique directories, OS-assigned ports (port = 0), fresh instances. When many unrelated tests flake together, suspect a shared resource, not a shared bug.

The two deterministic-synchronisation techniques

Most Stage 6/8 flakes reduce to "assert ran before the async work finished." Two tools eliminate the race instead of widening the window:

• DrainDispatcher.await(). Tez is event-driven; a test that fires an event and immediately asserts sees the pre-event state half the time. Drain the queue first:

  dispatcher.getEventHandler().handle(new VertexEvent(vid, VertexEventType.V_INIT));
  dispatcher.await();                 // returns when the queue is empty AND
  assertEquals(VertexState.INITED, vertex.getState());   // the last event fully handled
  

  Find the canonical class and copy its usage:

  cd /Users/s0x/src/oss-repos/tez
  grep -rln "class DrainDispatcher\|DrainableEventHandler\|Drainable" tez-common/src tez-dag/src/test
  

  If it still flakes after await(), a child component owns a second dispatcher — drain that one too.

• OS-assigned ports for MiniTezCluster. Fixed RM ports collide when two suites run on one CI agent. Pass 0 so the OS assigns a free port, then read the actual port back from the started cluster's config. This is the structural version of the TEZ-3664 /tmp fix, applied to the network namespace.

Case study D — TEZ-3616: a sleep that ignored its own interrupt

git show fb0e45bf7   # TestMergeManager#testLocalDiskMergeMultipleTasks fails intermittently

The test simulated a slow disk-close so an interrupting thread could fire mid-merge. It did this with a Mockito doAnswer that called Thread.sleep(2000) and then callRealMethod(). The flake: when the interrupt arrived during the sleep, the InterruptedException was swallowed by Mockito's machinery and the merge continued as if uninterrupted — so sometimes the interrupt "took" and sometimes it didn't.

The fix replaced the doAnswer with a real MergeManager subclass that overrides closeOnDiskFile and, on InterruptedException, restores the interrupt status and returns rather than swallowing it:

@Override
public synchronized void closeOnDiskFile(FileChunk file) {
  if (interruptInMiddle) {
    try {
      Thread.sleep(2000);
    } catch (InterruptedException e) {
      Thread.currentThread().interrupt();   // preserve interrupt; do not swallow
      return;
    }
  }
  super.closeOnDiskFile(file);
}

The lesson. A test that injects timing via a mock that swallows InterruptedException will flake on exactly the interleaving it was written to test. The Tez interrupt convention — Thread.currentThread().interrupt(); return; — is not just for production code; a test that models interruption must honour it too.

When a flake is really a production bug

The decision rule you must internalise:

• The test races against an internal event queue and the fix is dispatcher.await() or a deterministic executor → it was a flake (test bug).
• The test races against a public contract method — a production caller doing the same sequence would see the same partial state → it is a real bug. File a Stage 4/6/8 ticket in addition to the test fix.

The TEZ-4334 deadlock in Stage 6 first showed up as a "hanging test." The wrong move was to bump the timeout; the right move was to recognise a production deadlock. Ask, every time: could a production caller hit this?

The contribution playbook for this class

1. Reproduce locally in a loop. 1-in-30 on CI often reproduces 1-in-50 locally. Get a failure rate before you touch anything.
2. Classify with the taxonomy. Sleep/yield race? Random data? Shared resource? Tight timeout? Each has a known fix (delete the backdoor; make data deterministic; isolate; raise the bound with reasoning).
3. Prefer determinism over tolerance. A deterministic executor or await() beats Mockito.timeout(...); explicit data beats random; the latter merely widen the window.
4. Prove it. Stress-loop hundreds of runs, zero failures:

for i in $(seq 1 200); do
  mvn -pl tez-dag test -Dtest=TestSpeculation#testBasicSpeculationWithProgress -q 2>&1 | tail -2
done | grep -c "BUILD FAILURE"

5. Never silence. Do not @Ignore or delete an assertion to make CI green. If you must quarantine, add a comment naming the reason and a JIRA — the way the real inventory above does.

Common mistakes

Exit criteria — when you're ready for the next stage

• You have de-flaked at least two tests with confirmed ~200-run stability, and one of them used determinism (explicit data, a fake executor, or await()), not a wider timeout.
• You caught at least one flake that was actually a production race and filed the corresponding bug.
• You can name the Tez flake taxonomy from memory: sleep/yield races, random data, shared resources (/tmp/ports), tight timeouts, undrained dispatchers.
• You can read the real @Ignore inventory and say, for each, whether it is a legitimate quarantine or a hidden bug.

Stage 10 turns the focus to performance regressions.