Skip to content

test: make the remaining StateDebounceManager tests deterministic - #396

Merged
tanderson-ld merged 1 commit into
mainfrom
ta/SDK-3006/deterministic-debounce-tests
Aug 31, 2026
Merged

test: make the remaining StateDebounceManager tests deterministic#396
tanderson-ld merged 1 commit into
mainfrom
ta/SDK-3006/deterministic-debounce-tests

Conversation

@tanderson-ld

@tanderson-ld tanderson-ld commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Requirements

  • I have added test coverage for new or changed functionality
  • I have followed the repository's pull request submission guidelines
  • I have validated my changes against all supported platform versions

Test-only change; no production code is touched.

Related issues

SDK-3006

Describe the solution you've provided

StateDebounceManagerTest is the largest single source of flaky CI failures in this repo. Mining every failed ci.yml run with retained logs (2026-06-01 to 2026-08-25, all 27 of them) turned up 15 failures caused by flaky unit tests, and 7 of those were this one class across 5 different methods and 5 unrelated branches. Each failure was exactly 1 failed test out of ~730, and each was verified unrelated to its PR's diff — the clearest example being a PR that changed only build.gradle and still failed a debounce assertion. Six of the 15 landed directly on pushes to main.

Root cause: the tests infer the state of an asynchronous scheduler from wall-clock Thread.sleep on the test thread. With TEST_DEBOUNCE_MS = 50 and sleeps of 3–4x the window, the margin is ~100ms, inside normal jitter for a loaded 2-core runner executing the whole suite in one JVM. That yields two races pulling in opposite directions:

  • Under-run ("should have fired") — the executor thread may not get scheduled inside the sleep budget.
  • Setup overshoot ("should not have fired") — consecutive setters must all land in one 50ms window. A pause between them expires window 1, the timer fires with intermediate state, and a suppression assertion sees 1 instead of 0. The race is in the setup, not the wait.

Because they pull opposite ways, fixing one method just relocates the failure. That is the actual history here: three reactive determinism commits (2026-06-01, 2026-06-03, and PR 394 on 2026-08-18), each converting whichever method had most recently flaked.

This change finishes the job. The 12 timing-agnostic tests move to ManualTaskExecutor, where a scheduled task runs only when the test drains the queue — so each drain stands in for the debounce window elapsing, and both races become impossible by construction. All 15 Thread.sleep calls in those tests are gone.

The strongest evidence this is the right remedy: no method has ever failed after being converted. callbackNotFiredBeforeDebounceWindow failed Jun 2 and Jun 3, was converted Jun 3, and has been silent since. resetCancelsPendingTimer failed Aug 18 at 14:13Z (its conversion landed Aug 19 at 05:29Z) and again Aug 21 on a branch that does not contain that conversion commit.

Two tests deliberately keep the real executor, because a manual executor would delete what they test:

  • callbackFiredAfterDebounceWindow — the only end-to-end coverage that a real scheduleTask(delayMillis) actually fires.
  • closeWaitsForInflightReconcileCallback — the close() drain barrier, which needs a callback genuinely in flight on another thread.

Both already use latches with generous timeouts rather than fixed sleeps, and neither has ever flaked. The four immediate-mode tests (debounceMs == 0) are untouched: they bypass the executor entirely and fire synchronously inside the setter.

Breakdown of all 22 methods:

Group Count Action
Sleep-based, timing-agnostic 12 Converted (3 had flaked, 9 were latent)
Already converted 4 Construction unified onto the new helper
Immediate mode 4 None
Genuinely timing / cross-thread 2 Keep real executor by design

Describe alternatives you've considered

  • Raising TEST_DEBOUNCE_MS or the sleep multipliers — makes under-run rarer but stays probabilistic, does nothing structural for the setup race, and adds ~20s to the suite.
  • Injecting a Clock and keeping a real executor — the flakiness is thread scheduling, not time reading; you would still be waiting on another thread.
  • Await-based assertions everywhere — good for "should have fired" (and is what the two retained tests already do), but you cannot await a non-event, so it cannot fix the suppression assertions.
  • Converting all 22 to be deterministic — rejected. It would trade flakiness for a coverage hole, since StateDebounceManager's documented taskLock/workLock contract and close() drain barrier would then go entirely unexercised.

Additional context

Verification: the class passes 22/22; run 20 times consecutively it was clean 20/20; the full testDebugUnitTest suite is 730 tests, 0 failures. Note that the local repeat loop only proves no new nondeterminism was introduced — the original failures were CI-load-dependent and would not reproduce on an idle dev machine either. The real argument is structural, and real confirmation is the absence of recurrence in CI.

Reviewer notes on scope, both called out deliberately:

  1. The four already-converted tests are touched, to point them at the new createManager(ManualTaskExecutor, ...) overload. Leaving 4 tests inlining the constructor while 12 use a helper seemed worse than a slightly wider diff.
  2. The repeated per-method "why a manual executor" comments are consolidated into class Javadoc rather than duplicated 16 times. The method-specific part of resetCancelsPendingTimer's comment (the taskLock/workLock interleaving note) is retained.

One assertion was strengthened: multipleRapidChangesCoalesceIntoOneCallback now also asserts cancelledCount() == 4, proving the five changes genuinely coalesced rather than merely yielding one callback. This mirrors the existing pattern in timerResetsOnEachEvent.

Known follow-ups, not in scope:

  • ManualTaskExecutor discards delayMillis and runPendingTasks() flushes the whole queue, so after this change debounceMs is only exercised end-to-end by one test. A virtual clock (advanceTimeBy) would make window-boundary assertions meaningful rather than near-vacuous.
  • ManualTaskExecutor is not thread-safe (plain ArrayList, non-volatile fields). Correct today because it is single-threaded by construction, but worth a class-level warning before anyone extends it.

Note

Overview
Eliminates CI flakiness in StateDebounceManagerTest by replacing wall-clock Thread.sleep with explicit ManualTaskExecutor#runPendingTasks() in the 12 debounce-semantics tests, so each drain simulates the debounce window without scheduler races.

Adds class Javadoc and a createManager(ManualTaskExecutor, …) helper, routes those tests through it (including ones already on a manual executor), drops throws InterruptedException where sleeps are gone, and tightens multipleRapidChangesCoalesceIntoOneCallback with a cancelledCount() == 4 check. callbackFiredAfterDebounceWindow and closeWaitsForInflightReconcileCallback still use SimpleTestTaskExecutor for real timing/cross-thread behavior; immediate-mode tests are unchanged.

No production code changes.

Reviewed by Cursor Bugbot for commit cfc839b. Bugbot is set up for automated code reviews on this repo. Configure here.

The debounce tests inferred the state of an asynchronous scheduler from
wall-clock Thread.sleep on the test thread. With a 50ms debounce window and
sleeps of 3-4x that, the margin was only ~100ms, which is inside normal
jitter on a loaded CI runner. That produced two independent races: the timer
thread might not get scheduled within the sleep budget, and a sequence of
setters might be split across two debounce windows so a suppression
assertion saw a callback it did not expect.

Convert the 12 timing-agnostic tests to ManualTaskExecutor, where a
scheduled task runs only when the test drains the queue. Each drain stands
in for the debounce window elapsing, which removes both races by
construction and drops all 15 Thread.sleep calls from those tests.

callbackFiredAfterDebounceWindow and closeWaitsForInflightReconcileCallback
deliberately keep the real executor: they cover real timer delay and the
cross-thread close() drain barrier respectively, neither of which a manual
executor can exercise. Immediate-mode tests are unaffected because they
bypass the executor entirely.

Also adds a createManager overload taking the manual executor, and moves the
repeated per-method rationale comments into class Javadoc so the four
previously converted tests share one construction path.
@tanderson-ld
tanderson-ld requested a review from a team as a code owner August 31, 2026 14:51
@tanderson-ld
tanderson-ld merged commit 09ef6a9 into main Aug 31, 2026
8 checks passed
@tanderson-ld
tanderson-ld deleted the ta/SDK-3006/deterministic-debounce-tests branch August 31, 2026 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants