From 1afe9c3a7d892cad802ccd54de738f8d72031a96 Mon Sep 17 00:00:00 2001 From: PJ Date: Sun, 16 Aug 2026 01:20:28 +0530 Subject: [PATCH] fix(sidecar): the hierarchy rpc serves the tree the snapshot reads the runner compares the two per step, but snapshot settles and closes a keyboard while hierarchy was a bare contentDescriptor. measured on emulator -5556 (api 34) with an ime open: 489 nodes against the snapshot's 134. both now come off snapshotTree under the same lock; the reread still costs ~75ms when no keyboard is up. --- .../dev/sanderling/sidecar/DriverBackend.kt | 35 ++++++++++++------- .../dev/sanderling/sidecar/DriverService.kt | 8 ++++- .../sanderling/sidecar/DriverServiceTest.kt | 12 +++++-- 3 files changed, 39 insertions(+), 16 deletions(-) diff --git a/sidecar/src/main/kotlin/dev/sanderling/sidecar/DriverBackend.kt b/sidecar/src/main/kotlin/dev/sanderling/sidecar/DriverBackend.kt index 3d69e2f..ded9614 100644 --- a/sidecar/src/main/kotlin/dev/sanderling/sidecar/DriverBackend.kt +++ b/sidecar/src/main/kotlin/dev/sanderling/sidecar/DriverBackend.kt @@ -30,11 +30,21 @@ interface DriverBackend { fun healthy(): Boolean fun metrics(bundleId: String): MetricsSample + // snapshotTree is the tree a snapshot reads, without the screenshot. It is + // what the Hierarchy RPC serves, so the runner's two reads of a step come + // off one pipeline: a backend that waits out a transition or closes a + // keyboard before reading has to do the same on both, or the two trees + // differ over what the backend did between them rather than over what the + // app did. On this device an IME standing open is a 489-node tree against + // the snapshot's 134. + fun snapshotTree(): String = hierarchy() + // snapshot captures hierarchy then screenshot back-to-back. The service // layer holds a mutex around the call so concurrent callers observe a // serialized pair from the same on-device frame. Backends may override // to fuse the two reads more tightly when their native API allows. - fun snapshot(): SnapshotSample = SnapshotSample(hierarchy(), screenshot()) + fun snapshot(): SnapshotSample = + SnapshotSample(snapshotTree(), screenshot()) // close releases device-side resources on shutdown. The iOS backend must // stop its XCTest runner here: an orphaned runner session auto-restarts @@ -1250,8 +1260,8 @@ class MaestroDriverBackend(private val serial: String?) : DriverBackend { override fun recentLogs(sinceUnixMillis: Long, minLevel: String) = readLogcat(serial, sinceUnixMillis, minLevel) - // snapshot waits out a NavHost cross-fade before it reads, so the runner is - // never handed a tree holding two routes at once. It belongs here rather + // snapshotTree waits out a NavHost cross-fade before it reads, so the runner + // is never handed a tree holding two routes at once. It belongs here rather // than in waitForIdle: the runner gives waitForIdle a one-second deadline // and abandons the RPC when it expires, which is not enough room for a // 700ms fade that began before the settle did, and a wait that outlives the @@ -1262,16 +1272,15 @@ class MaestroDriverBackend(private val serial: String?) : DriverBackend { // The predicate costs nothing on a settled frame: the read it needs is the // read the snapshot was going to do anyway. That is what makes this // affordable, where the structural poll that used to run in waitForIdle was - // not: it fetched the hierarchy ~4 more times on every mutating step. - override fun snapshot(): SnapshotSample { - val tree = treeWithoutKeyboard( - awaitSettledTree { hierarchy() }, - imePackage, - dismiss = { runCatching { dadb.shell("input keyevent 4") } }, - reread = { awaitSettledTree { hierarchy() } }, - ) - return SnapshotSample(tree, screenshot()) - } + // not: it fetched the hierarchy ~4 more times on every mutating step. The + // keyboard leg costs nothing either when no IME is standing in the tree, + // which is what lets the Hierarchy RPC serve this too. + override fun snapshotTree(): String = treeWithoutKeyboard( + awaitSettledTree { hierarchy() }, + imePackage, + dismiss = { runCatching { dadb.shell("input keyevent 4") } }, + reread = { awaitSettledTree { hierarchy() } }, + ) override fun waitForIdle(durationMillis: Long) { // waitForAppToSettle blocks on the View-system animation and maestro's diff --git a/sidecar/src/main/kotlin/dev/sanderling/sidecar/DriverService.kt b/sidecar/src/main/kotlin/dev/sanderling/sidecar/DriverService.kt index 647655b..ea3b137 100644 --- a/sidecar/src/main/kotlin/dev/sanderling/sidecar/DriverService.kt +++ b/sidecar/src/main/kotlin/dev/sanderling/sidecar/DriverService.kt @@ -170,12 +170,18 @@ class DriverService( } } + // The runner reads this a second time per step to see whether the screen + // changed while it was looking, so it has to describe the same thing the + // snapshot's tree describes: same settle, same keyboard handling, same + // lock. Served off the bare backend read, the pair differed over what the + // backend did between them rather than over what the app did. override fun hierarchy( request: Empty, responseObserver: StreamObserver, ) { runRpc(responseObserver) { - HierarchyJSON.newBuilder().setJson(backend.hierarchy()).build() + val tree = synchronized(snapshotLock) { backend.snapshotTree() } + HierarchyJSON.newBuilder().setJson(tree).build() } } diff --git a/sidecar/src/test/kotlin/dev/sanderling/sidecar/DriverServiceTest.kt b/sidecar/src/test/kotlin/dev/sanderling/sidecar/DriverServiceTest.kt index 0187cca..29cd6a9 100644 --- a/sidecar/src/test/kotlin/dev/sanderling/sidecar/DriverServiceTest.kt +++ b/sidecar/src/test/kotlin/dev/sanderling/sidecar/DriverServiceTest.kt @@ -333,9 +333,17 @@ class DriverServiceTest { assertEquals(3, image.png.size()) } - @Test fun hierarchyReturnsBackendJson() { + // The runner reads the hierarchy a second time per step to see whether the + // screen changed while it was looking, so this has to answer with the tree + // the snapshot's read produces: same settle, same keyboard handling. Served + // off the bare backend read, the pair differs over what the backend did + // between the two reads rather than over what the app did. Measured on an + // API 34 emulator with an IME standing open, that is a 489-node tree + // against the snapshot's 134. + @Test fun hierarchyServesTheTreeTheSnapshotReads() { val backend = object : DriverBackend by StubDriverBackend("android") { - override fun hierarchy(): String = "{\"x\":1}" + override fun hierarchy(): String = "{\"bare\":1}" + override fun snapshotTree(): String = "{\"x\":1}" } val client = newClient(backend)