From 0e35978e2524a9ea862b45d807ce6b75c7c6ae6c Mon Sep 17 00:00:00 2001 From: PJ Date: Sat, 15 Aug 2026 12:46:40 +0530 Subject: [PATCH] fix(sidecar): measure the stability streak as observed quiet parameterising pollUntilStable also moved the clock to the start of the read that opened a run of identical snapshots, so a read's own duration counted as quiet. the pre-existing caller polls a real uiautomator dump: at 400ms a read, 750ms of required quiet became 250ms of observed quiet and the poll settled in two reads instead of four. the parameters stay, the semantics go back. --- .../dev/sanderling/sidecar/DriverBackend.kt | 23 +++++----- .../sanderling/sidecar/StabilityPollTest.kt | 43 ++++++++----------- 2 files changed, 29 insertions(+), 37 deletions(-) diff --git a/sidecar/src/main/kotlin/dev/sanderling/sidecar/DriverBackend.kt b/sidecar/src/main/kotlin/dev/sanderling/sidecar/DriverBackend.kt index b0ffebe..d987490 100644 --- a/sidecar/src/main/kotlin/dev/sanderling/sidecar/DriverBackend.kt +++ b/sidecar/src/main/kotlin/dev/sanderling/sidecar/DriverBackend.kt @@ -128,15 +128,11 @@ internal fun awaitSettledTree(read: () -> String): String { // streak resets and the loop keeps polling instead of declaring a partial // state stable. // -// The streak is measured from the start of the read that opened the current -// run of identical snapshots, not from when that read returned, so a slow read -// is charged to the streak. That is a deliberate trade and not a free one: the -// quiet the poll actually OBSERVED spans the last read's start back to the -// first read's return, which is shorter than streakMillis by up to the two -// reads' durations. On Android a hierarchy fetch costs more than the poll -// interval, so charging it is the difference between two reads and four, and a -// caller wanting the full streak observed has to widen streakMillis rather than -// assume it. StabilityPollTest.slowSnapshotReadsCountTowardTheStreak pins this. +// streakMillis is quiet the poll OBSERVED: the clock starts when the read that +// first matched its predecessor returns, so the reads spanning it are not +// charged to it. A hierarchy fetch on Android costs more than the poll +// interval, and charging it would let a 500ms read clear the default 750ms +// streak having watched 250ms of quiet. internal fun pollUntilStable( timeoutMillis: Long, streakMillis: Long = MIN_STABLE_STREAK_MILLIS, @@ -145,24 +141,25 @@ internal fun pollUntilStable( ) { if (timeoutMillis <= 0) return val deadline = System.currentTimeMillis() + timeoutMillis - var runStart = System.currentTimeMillis() var prior = try { snapshot() } catch (_: Exception) { null } + var streakStart = 0L while (System.currentTimeMillis() < deadline) { Thread.sleep(intervalMillis) - val currentStart = System.currentTimeMillis() val current = try { snapshot() } catch (_: Exception) { null } + val now = System.currentTimeMillis() if (prior != null && current != null && prior == current) { - if (System.currentTimeMillis() - runStart >= streakMillis) return + if (streakStart == 0L) streakStart = now + if (now - streakStart >= streakMillis) return } else { - runStart = currentStart + streakStart = 0L } prior = current } diff --git a/sidecar/src/test/kotlin/dev/sanderling/sidecar/StabilityPollTest.kt b/sidecar/src/test/kotlin/dev/sanderling/sidecar/StabilityPollTest.kt index ab599d0..c8da826 100644 --- a/sidecar/src/test/kotlin/dev/sanderling/sidecar/StabilityPollTest.kt +++ b/sidecar/src/test/kotlin/dev/sanderling/sidecar/StabilityPollTest.kt @@ -16,13 +16,12 @@ class StabilityPollTest { assertTrue(elapsed < 3000L, "should not run to cap when stable, elapsed=${elapsed}ms") } - @Test fun slowSnapshotReadsCountTowardTheStreak() { - // The streak runs from the START of the read that opened the run of - // identical snapshots, so the read's own duration is charged to it. - // Every other test here uses an instant lambda and so passes under - // either semantics; this one is the difference. StubDriverBackend's - // waitForIdle polls a real `uiautomator dump`, which costs hundreds of - // milliseconds, so the slow read is the case it runs in. + @Test fun slowSnapshotReadsDoNotEatTheStreak() { + // Every other test here uses an instant lambda and so passes whether or + // not a read is charged to the streak; this one is the difference. + // StubDriverBackend's waitForIdle polls a real `uiautomator dump`, + // which costs hundreds of milliseconds, so the slow read is the case it + // runs in. val readMillis = 400L val sampleStarts = mutableListOf() val sampleEnds = mutableListOf() @@ -33,26 +32,18 @@ class StabilityPollTest { sampleEnds += System.currentTimeMillis() - start "stable" } - val elapsed = System.currentTimeMillis() - start - - // One read plus one interval plus one read already clears 750ms, so the - // poll settles for two samples where an instant read takes four. - assertEquals( - 2, - sampleStarts.size, - "a ${readMillis}ms read should clear the streak in two samples, starts=$sampleStarts", - ) - assertTrue(elapsed >= MIN_STABLE_STREAK_MILLIS, "elapsed=${elapsed}ms") // What the poll actually watched: the last read began this long after - // the first one returned. It is the poll interval, not the streak, and - // a caller that needs MIN_STABLE_STREAK_MILLIS of observed quiet has to - // ask for a wider streak rather than assume this one delivers it. + // the first one returned, and every sample in between matched. val observedQuiet = sampleStarts.last() - sampleEnds.first() assertTrue( - observedQuiet < MIN_STABLE_STREAK_MILLIS, - "the poll returned having observed ${observedQuiet}ms of quiet, not " + - "${MIN_STABLE_STREAK_MILLIS}ms; if that changed, the streak semantics changed", + observedQuiet >= MIN_STABLE_STREAK_MILLIS, + "the poll returned having observed only ${observedQuiet}ms of quiet, not " + + "${MIN_STABLE_STREAK_MILLIS}ms; starts=$sampleStarts ends=$sampleEnds", + ) + assertTrue( + sampleStarts.size >= 3, + "a ${readMillis}ms read cannot clear the streak in one pair, starts=$sampleStarts", ) } @@ -77,7 +68,11 @@ class StabilityPollTest { } } val sinceTransition = System.currentTimeMillis() - transientAt - assertTrue(calls > 3, "must keep sampling past the transition, got $calls") + assertTrue( + calls >= 8, + "after the transition the poll needs a fresh matching pair and then a full " + + "${MIN_STABLE_STREAK_MILLIS}ms of quiet, which is 8 samples, got $calls", + ) assertTrue( sinceTransition >= MIN_STABLE_STREAK_MILLIS, "the calm prefix must not count: a full ${MIN_STABLE_STREAK_MILLIS}ms streak has to " +