Skip to content

Commit e2ea100

Browse files
committed
test(reader): hold the commit still for every down-payment wait
920a350 fixed one of these; stress-running the class found the same defect in three more, and the same latent race in a fourth. Every scrub-commit test that waits for the down-payment row was driving the commit forward with the very wait meant to observe it standing still. idleUntil pumps the main looper, and the commit coroutine rides lifecycleScope on Main — so pump enough times and it RESOLVES first. A resolved commit is correctly never repaired and its refined write replaces the coarse one, so the tests then either read the wrong offset or waited forever for a repair that would never come. One of them failed exactly that way on the runner cutting 2026.07.5; another failed under CPU load here. All five waits now use waitWithoutIdling. The down-payment lands on positionWriteScope (a real IO thread) with no looper help, while not idling holds the commit at its pagination suspension — which is also precisely where the cancel and teardown these tests simulate would land, so they now exercise the state they always claimed to. The shared mechanism is documented once on waitWithoutIdling; each site keeps only its own consequence. Verified non-vacuous by breaking production both ways: disabling the repair reds the three repair tests, disabling the down-payment reds all five. Two runs under a loaded machine, and the full suite, green.
1 parent 920a350 commit e2ea100

1 file changed

Lines changed: 44 additions & 22 deletions

File tree

‎app/src/test/kotlin/dev/reader/ui/ReaderActivityTest.kt‎

Lines changed: 44 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1481,20 +1481,9 @@ class ReaderActivityTest {
14811481
// BEFORE cancelling anything — this is the synchronous write, unaffected by the repair.
14821482
scrubber.onScrubCommit?.invoke(0.9f, null)
14831483

1484-
// Wait WITHOUT pumping the main looper, which is what makes this observation deterministic
1485-
// rather than a race. The down-payment is written synchronously inside onScrubCommitted,
1486-
// before the commit coroutine is even launched, and it rides positionWriteScope
1487-
// (Dispatchers.IO.limitedParallelism(1)) — a real background thread — so it lands with no
1488-
// main-thread help at all. The refining coroutine rides lifecycleScope on the MAIN
1489-
// dispatcher, so under Robolectric's paused looper it cannot execute a single line until
1490-
// this test idles that looper. Not idling therefore freezes the refinement outright: the
1491-
// row can only ever hold the down-payment here.
1492-
//
1493-
// Idling instead (as this once did) let the coroutine free-run against the wait, and both
1494-
// the down-payment and the resolved write satisfy "spineIndex changed" — so on a slow or
1495-
// loaded machine the first row this predicate managed to read was already the RESOLVED one,
1496-
// and the offset-0 assertion below failed. That is a CI failure this test earned, not a
1497-
// fluke: it was asserting on a transient it had no way to pin.
1484+
// Not idling pins the commit at its pagination suspension, so the row read here can only
1485+
// be the down-payment — see [waitWithoutIdling]. Idling instead let the refined write win
1486+
// the race and the offset-0 assertion below read a real page offset.
14981487
var downPayment: BookEntity? = null
14991488
waitWithoutIdling {
15001489
downPayment = rowFor(app, book.path)
@@ -1534,9 +1523,15 @@ class ReaderActivityTest {
15341523
activity.showOverlayForTest()
15351524
val scrubber = activity.findViewById<ChapterScrubberView>(R.id.chapter_scrubber)
15361525
scrubber.onScrubCommit?.invoke(0.9f, null)
1537-
idleUntil { rowFor(app, book.path)!!.spineIndex != origin.spineIndex } // down-payment landed
1526+
// Not idling holds the commit at its pagination suspension — exactly where a teardown's
1527+
// cancellation lands — so the cancel below is guaranteed to catch it unresolved. Idling
1528+
// let it resolve first, and a resolved commit is correctly never repaired, so the repair
1529+
// wait below timed out. See [waitWithoutIdling].
1530+
waitWithoutIdling { rowFor(app, book.path)!!.spineIndex != origin.spineIndex }
15381531
activity.cancelScrubJobForTest()
15391532

1533+
// Idling is wanted from here on: the cancelled coroutine's `finally` — and the repair it
1534+
// writes — can only run once the looper delivers its resumption.
15401535
idleUntil { activity.scrubIdleForTest }
15411536
idleUntil { rowFor(app, book.path)!!.spineIndex == origin.spineIndex }
15421537

@@ -1567,7 +1562,9 @@ class ReaderActivityTest {
15671562
activity.showOverlayForTest()
15681563
val scrubber = activity.findViewById<ChapterScrubberView>(R.id.chapter_scrubber)
15691564
scrubber.onScrubCommit?.invoke(0.9f, null)
1570-
idleUntil { rowFor(app, book.path)!!.spineIndex != originChapter } // down-payment landed
1565+
// Not idling keeps the pagination genuinely in flight for the teardown below, and keeps
1566+
// the coarse down-payment from being refined first — see [waitWithoutIdling].
1567+
waitWithoutIdling { rowFor(app, book.path)!!.spineIndex != originChapter }
15711568

15721569
// Teardown while the pagination is still in flight — onDestroy cancels lifecycleScope,
15731570
// landing the coroutine's `finally` exactly where a real double-Back/swipe-away would.
@@ -1595,7 +1592,9 @@ class ReaderActivityTest {
15951592
// Lift-off on a distant chapter writes the down-payment, then the commit is cancelled
15961593
// before its pagination can refine it — exactly what a second touch on the track does.
15971594
activity.commitScrubForTest(fraction = 0.9f)
1598-
idleUntil { rowFor(app, book.path)!!.spineIndex != before.spineIndex } // down-payment landed
1595+
// Not idling guarantees the cancel below lands on an unresolved commit — see
1596+
// [waitWithoutIdling].
1597+
waitWithoutIdling { rowFor(app, book.path)!!.spineIndex != before.spineIndex }
15991598
activity.cancelScrubJobForTest()
16001599
idleUntil { activity.scrubIdleForTest }
16011600
idleUntil { rowFor(app, book.path)!!.spineIndex == before.spineIndex }
@@ -1632,7 +1631,10 @@ class ReaderActivityTest {
16321631
// Snap onto the empty chapter 1. Down-payment (1, 0) fires on the spine-index gate alone,
16331632
// before the walk below ever runs.
16341633
activity.commitScrubForTest(fraction = 0.3f, snappedChapter = 1)
1635-
idleUntil { rowFor(app, book.path)!!.spineIndex == 1 } // down-payment landed
1634+
// The (1, 0) down-payment is a transient here — the repair overwrites it with the origin
1635+
// moments later — so it can only be observed with the commit held still. See
1636+
// [waitWithoutIdling].
1637+
waitWithoutIdling { rowFor(app, book.path)!!.spineIndex == 1 }
16361638
idleUntil { activity.scrubIdleForTest }
16371639
idleUntil { rowFor(app, book.path)!!.spineIndex == origin.spineIndex }
16381640

@@ -2884,10 +2886,30 @@ class ReaderActivityTest {
28842886
check(condition()) { "condition never became true within ${timeoutMs}ms" }
28852887
}
28862888

2887-
/** Like [idleUntil], but deliberately never pumps the main looper — for waiting on a real
2888-
* generation's background-thread side effect (e.g. a strip landing on disk) without draining
2889-
* its queued `runOnUiThread` posts, which a test needs to stay queued until a config change
2890-
* has made their token stale. */
2889+
/**
2890+
* Like [idleUntil], but deliberately never pumps the main looper. Two uses, one principle:
2891+
* pumping the looper is what lets main-dispatched work advance, so NOT pumping pins that work
2892+
* exactly where it is while a background-thread side effect is waited on.
2893+
*
2894+
* 1. Waiting for a real generation's background side effect (e.g. a strip landing on disk)
2895+
* without draining its queued `runOnUiThread` posts, which a test needs to stay queued
2896+
* until a config change has made their token stale.
2897+
*
2898+
* 2. Waiting for a scrub commit's **down-payment** row. The down-payment rides
2899+
* `positionWriteScope` (`Dispatchers.IO.limitedParallelism(1)`) — a real thread — so it
2900+
* lands with no main-thread help at all. The commit coroutine that would overwrite it rides
2901+
* `lifecycleScope`, i.e. `Dispatchers.Main.immediate`: its body runs inline as far as the
2902+
* off-main pagination, then can only resume via a post back to Main, which Robolectric's
2903+
* paused looper will not deliver until something idles it. So not idling holds the commit
2904+
* at that pagination suspension — precisely where a teardown or a second touch would cancel
2905+
* it — and guarantees the row still holds the down-payment when it is read.
2906+
*
2907+
* Using [idleUntil] for case 2 is a race, and it is the race that broke a release build: the
2908+
* commit is driven forward by the very wait meant to observe it standing still, so on a loaded
2909+
* machine it RESOLVES first. A resolved commit is correctly never repaired and its refined
2910+
* write replaces the coarse one — so the test then either reads the wrong offset or waits
2911+
* forever for a repair that will never come.
2912+
*/
28912913
private fun waitWithoutIdling(timeoutMs: Long = 5_000, condition: () -> Boolean) {
28922914
val deadline = System.currentTimeMillis() + timeoutMs
28932915
while (!condition() && System.currentTimeMillis() < deadline) {

0 commit comments

Comments
 (0)