From 83e9d5fd95e3d4b70b1204171babd979835efbc1 Mon Sep 17 00:00:00 2001 From: PJ Date: Sat, 15 Aug 2026 20:30:08 +0530 Subject: [PATCH 01/15] fix(sidecar): reach the adb server the environment names buildDadb hardcoded localhost:5037, so a serial-addressed device always resolved through this machine's adb server and ADB_SERVER_SOCKET was ignored. Read the endpoint the way the adb CLI does instead. Fixes #79 --- .../dev/sanderling/sidecar/DriverBackend.kt | 70 +++++++++++++++++-- 1 file changed, 65 insertions(+), 5 deletions(-) diff --git a/sidecar/src/main/kotlin/dev/sanderling/sidecar/DriverBackend.kt b/sidecar/src/main/kotlin/dev/sanderling/sidecar/DriverBackend.kt index c73b11c..ae9bfab 100644 --- a/sidecar/src/main/kotlin/dev/sanderling/sidecar/DriverBackend.kt +++ b/sidecar/src/main/kotlin/dev/sanderling/sidecar/DriverBackend.kt @@ -1044,15 +1044,75 @@ internal fun dadbTargetFor(serial: String?): DadbTarget { } } +internal data class AdbServerEndpoint(val host: String, val port: Int) + +private const val ADB_SERVER_HOST = "localhost" +private const val ADB_SERVER_PORT = 5037 + +// adbServerEndpoint reads where the adb server listens the way the adb CLI +// reads it: ADB_SERVER_SOCKET ("tcp:host:port", or "tcp:port" for a server on +// this machine) outranks the older ANDROID_ADB_SERVER_ADDRESS / +// ANDROID_ADB_SERVER_PORT pair, and unset means the loopback default. +// +// A value it cannot read throws instead of falling back to loopback. The +// fallback is the dangerous answer: emulator serials are numbered per server, +// so a run aimed at a remote emulator-5554 would quietly drive whatever this +// machine calls emulator-5554 and report the results as the remote device's. +internal fun adbServerEndpoint( + env: (String) -> String? = System::getenv, +): AdbServerEndpoint { + val socket = env("ADB_SERVER_SOCKET")?.trim().orEmpty() + if (socket.isNotEmpty()) return parseAdbServerSocket(socket) + val host = env("ANDROID_ADB_SERVER_ADDRESS")?.trim().orEmpty() + val port = env("ANDROID_ADB_SERVER_PORT")?.trim().orEmpty() + return AdbServerEndpoint( + host.ifEmpty { ADB_SERVER_HOST }, + if (port.isEmpty()) { + ADB_SERVER_PORT + } else { + adbServerPort(port, "ANDROID_ADB_SERVER_PORT=\"$port\"") + }, + ) +} + +private fun parseAdbServerSocket(value: String): AdbServerEndpoint { + val named = "ADB_SERVER_SOCKET=\"$value\"" + val address = value.removePrefix("tcp:") + if (address == value) rejectAdbServerSocket(named) + val colon = address.lastIndexOf(':') + if (colon < 0) { + return AdbServerEndpoint( + ADB_SERVER_HOST, + adbServerPort(address, named), + ) + } + val host = address.substring(0, colon) + if (host.isEmpty()) rejectAdbServerSocket(named) + return AdbServerEndpoint( + host, + adbServerPort(address.substring(colon + 1), named), + ) +} + +private fun rejectAdbServerSocket(named: String): Nothing = + throw IllegalArgumentException("$named is not tcp:host:port") + +private fun adbServerPort(text: String, named: String): Int = + text.toIntOrNull()?.takeIf { it in 1..65535 } + ?: throw IllegalArgumentException("$named has no usable port") + private fun buildDadb(serial: String?): dadb.Dadb = when (val target = dadbTargetFor(serial)) { is DadbTarget.Tcp -> dadb.Dadb.create(target.host, target.port) - is DadbTarget.Server -> dadb.adbserver.AdbServer.createDadb( - "localhost", - 5037, - "host:transport:${target.serial}", - ) + is DadbTarget.Server -> { + val server = adbServerEndpoint() + dadb.adbserver.AdbServer.createDadb( + server.host, + server.port, + "host:transport:${target.serial}", + ) + } } internal fun findBoundsBySelector( From a71cbfc685124256707b73fa413daa3765292761 Mon Sep 17 00:00:00 2001 From: PJ Date: Sat, 15 Aug 2026 20:30:11 +0530 Subject: [PATCH 02/15] test(sidecar): pin the adb server endpoint parsing --- .../dev/sanderling/sidecar/DadbTargetTest.kt | 101 ++++++++++++++++++ 1 file changed, 101 insertions(+) diff --git a/sidecar/src/test/kotlin/dev/sanderling/sidecar/DadbTargetTest.kt b/sidecar/src/test/kotlin/dev/sanderling/sidecar/DadbTargetTest.kt index 7db71f8..1d1cee5 100644 --- a/sidecar/src/test/kotlin/dev/sanderling/sidecar/DadbTargetTest.kt +++ b/sidecar/src/test/kotlin/dev/sanderling/sidecar/DadbTargetTest.kt @@ -2,6 +2,8 @@ package dev.sanderling.sidecar import org.junit.Test import kotlin.test.assertEquals +import kotlin.test.assertFailsWith +import kotlin.test.assertTrue class DadbTargetTest { @@ -22,4 +24,103 @@ class DadbTargetTest { @Test fun colonWithNonNumericPortIsAServerSerial() { assertEquals(DadbTarget.Server("emulator:5554x"), dadbTargetFor("emulator:5554x")) } + + // A serial-addressed device is reached through whichever adb server the + // environment names. Ignoring it sends the run to this machine's own + // server, where the serial either is missing or, worse, names a different + // device that happens to share the emulator numbering. + @Test fun adbServerSocketNamesARemoteServer() { + assertEquals( + AdbServerEndpoint("100.68.126.75", 5037), + adbServerEndpoint( + env("ADB_SERVER_SOCKET" to "tcp:100.68.126.75:5037"), + ), + ) + } + + @Test fun adbServerSocketWithOnlyAPortStaysLocal() { + assertEquals( + AdbServerEndpoint("localhost", 5038), + adbServerEndpoint(env("ADB_SERVER_SOCKET" to "tcp:5038")), + ) + } + + @Test fun androidAdbServerAddressAndPortPairIsHonoured() { + assertEquals( + AdbServerEndpoint("10.0.0.4", 5040), + adbServerEndpoint( + env( + "ANDROID_ADB_SERVER_ADDRESS" to "10.0.0.4", + "ANDROID_ADB_SERVER_PORT" to "5040", + ), + ), + ) + } + + @Test fun adbServerSocketOutranksTheOlderPair() { + assertEquals( + AdbServerEndpoint("100.68.126.75", 5037), + adbServerEndpoint( + env( + "ADB_SERVER_SOCKET" to "tcp:100.68.126.75:5037", + "ANDROID_ADB_SERVER_ADDRESS" to "10.0.0.4", + "ANDROID_ADB_SERVER_PORT" to "5040", + ), + ), + ) + } + + @Test fun unsetEnvironmentKeepsTheLoopbackDefault() { + assertEquals( + AdbServerEndpoint("localhost", 5037), + adbServerEndpoint(env()), + ) + assertEquals( + AdbServerEndpoint("localhost", 5037), + adbServerEndpoint(env("ADB_SERVER_SOCKET" to "")), + ) + } + + @Test fun eitherHalfOfTheOlderPairAloneKeepsTheOtherDefault() { + assertEquals( + AdbServerEndpoint("10.0.0.4", 5037), + adbServerEndpoint(env("ANDROID_ADB_SERVER_ADDRESS" to "10.0.0.4")), + ) + assertEquals( + AdbServerEndpoint("localhost", 5040), + adbServerEndpoint(env("ANDROID_ADB_SERVER_PORT" to "5040")), + ) + } + + // A value that cannot be read must stop the run and say which variable + // held what. Falling back to loopback would drive this machine's devices + // while the operator believes the run is on the remote ones. + @Test fun malformedValuesFailNamingTheVariableAndItsContents() { + val cases = mapOf( + "ADB_SERVER_SOCKET" to listOf( + "100.68.126.75:5037", + "tcp:100.68.126.75:pear", + "tcp:", + "tcp::5037", + "unix:/tmp/adb", + "tcp:100.68.126.75:70000", + ), + "ANDROID_ADB_SERVER_PORT" to listOf("pear", "0", "-1"), + ) + for ((variable, values) in cases) { + for (value in values) { + val failure = assertFailsWith(value) { + adbServerEndpoint(env(variable to value)) + } + val message = failure.message.orEmpty() + assertTrue(message.contains(variable), message) + assertTrue(message.contains(value), message) + } + } + } +} + +private fun env(vararg entries: Pair): (String) -> String? { + val values = entries.toMap() + return { values[it] } } From b4cd9d5cd173c2499d59bd2ee231a5a9fcd9c9a1 Mon Sep 17 00:00:00 2001 From: PJ Date: Sat, 15 Aug 2026 20:41:59 +0530 Subject: [PATCH 03/15] fix(sidecar): close a keyboard standing in the snapshot A tap on a text field raises the keyboard and nothing closed it, so the tree the picker chooses from was missing every app node underneath it, the submit control included. Close it before the read rather than after the tap: the picker only ever sees snapshots, and the keyboard is still on its way up when the tap returns. Fixes #78 --- .../dev/sanderling/sidecar/DriverBackend.kt | 80 ++++++++++++++++++- 1 file changed, 78 insertions(+), 2 deletions(-) diff --git a/sidecar/src/main/kotlin/dev/sanderling/sidecar/DriverBackend.kt b/sidecar/src/main/kotlin/dev/sanderling/sidecar/DriverBackend.kt index ae9bfab..7652d5d 100644 --- a/sidecar/src/main/kotlin/dev/sanderling/sidecar/DriverBackend.kt +++ b/sidecar/src/main/kotlin/dev/sanderling/sidecar/DriverBackend.kt @@ -799,11 +799,72 @@ internal fun typeChunks( // every InputText, which raises the IME again long before this probe runs. A // caller that dismissed twice in a row, or typed without focusing first, would // lose that margin. +// +// treeWithoutKeyboard closes a keyboard too, on the snapshot path, and does not +// cost this one its margin: it reads no flag, and the two are a waitForIdle and +// a hierarchy fetch apart, several times the window in which this one is stale. internal fun dismissSoftKeyboard(shell: (String) -> String) { if (!shell("dumpsys input_method").contains("mInputShown=true")) return shell("input keyevent 4") } +// KEYBOARD_DISMISS_READS bounds the re-reads a snapshot spends waiting for the +// IME window to leave the tree after BACK. The window goes over an animation, +// so the first read back can still carry it; a hierarchy read plus the interval +// costs ~250ms on the emulator, which covers a retraction several times over +// without turning a keyboard the app keeps re-raising into an unbounded wait. +internal const val KEYBOARD_DISMISS_READS = 4 +internal const val KEYBOARD_DISMISS_INTERVAL_MILLIS = 100L + +// imePackageOf takes the package half of an input-method component id +// ("pkg/.Service"), the form both `settings get secure default_input_method` +// and dumpsys' mCurMethodId use. Anything that is not a package name reads as +// "no IME known", which disables the dismissal rather than guessing. +internal fun imePackageOf(component: String): String? = + component.trim().substringBefore('/') + .takeIf { it.isNotEmpty() && it.contains('.') } + +// treeShowsIme reports whether the keyboard window is in the tree, by the view +// ids the IME's own resources give it ("pkg:id/name"). +internal fun treeShowsIme(treeJson: String, imePackage: String): Boolean = + treeJson.contains("$imePackage:id/") + +// treeWithoutKeyboard closes a keyboard standing in the snapshot and returns a +// tree read after it has gone, or the tree it was given when none is open. +// +// It belongs here, before the read the picker chooses from, rather than after +// the tap that raised the keyboard. Two reasons. The picker only ever sees +// snapshots, so a dismissal anywhere later leaves this step choosing between +// the handful of targets an open keyboard left in the tree, which is the +// budget the fuzzer was losing. And the state it has to judge is settled here: +// the action landed a waitForIdle ago, where straight after the tap the +// keyboard is still on its way up and nothing it could read would say so yet. +// +// The tree is also a better guard than mInputShown. BACK closes an open +// keyboard and navigates when none is open, so pressing it is only safe on a +// true reading; mInputShown trails the keyboard by up to 0.6s, while a tree +// carrying the IME's own view ids is the keyboard being on screen, read a +// moment ago. Once dismissed, the re-reads confirm it went rather than pressing +// BACK again, so a keyboard the app puts straight back costs re-reads and never +// a second back press. +internal fun treeWithoutKeyboard( + tree: String, + imePackage: String?, + dismiss: () -> Unit, + reread: () -> String, + sleep: (Long) -> Unit = { Thread.sleep(it) }, +): String { + if (imePackage == null || !treeShowsIme(tree, imePackage)) return tree + dismiss() + var current = tree + repeat(KEYBOARD_DISMISS_READS) { + sleep(KEYBOARD_DISMISS_INTERVAL_MILLIS) + current = reread() + if (!treeShowsIme(current, imePackage)) return current + } + return current +} + // resumedActivityPackage matches a "package/activity" component, mirroring the // Go scope guard's regex so both read the same dumpsys wording. private val resumedActivityPackage = @@ -856,6 +917,14 @@ internal fun retryOpen( class MaestroDriverBackend(private val serial: String?) : DriverBackend { private val dadb: dadb.Dadb = buildDadb(serial) + private val imePackage: String? by lazy { + imePackageOf( + runCatching { + dadb.shell("settings get secure default_input_method").allOutput + }.getOrDefault(""), + ) + } + // A fresh AndroidDriver per open attempt. Its gRPC channel is built once in // the constructor and permanently shut down by close(), so reopening a // closed instance would reuse a dead channel; rebuild it each try instead. @@ -991,8 +1060,15 @@ class MaestroDriverBackend(private val serial: String?) : DriverBackend { // 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 = - SnapshotSample(awaitSettledTree { hierarchy() }, screenshot()) + override fun snapshot(): SnapshotSample { + val tree = treeWithoutKeyboard( + awaitSettledTree { hierarchy() }, + imePackage, + dismiss = { runCatching { dadb.shell("input keyevent 4") } }, + reread = { awaitSettledTree { hierarchy() } }, + ) + return SnapshotSample(tree, screenshot()) + } override fun waitForIdle(durationMillis: Long) { // waitForAppToSettle blocks on the View-system animation and maestro's From 1e350f7fd3442b56c2c65666398f74620a00bab0 Mon Sep 17 00:00:00 2001 From: PJ Date: Sat, 15 Aug 2026 20:42:02 +0530 Subject: [PATCH 04/15] test(sidecar): pin the tree-guarded keyboard dismissal --- .../dev/sanderling/sidecar/InputTextTest.kt | 120 ++++++++++++++++++ 1 file changed, 120 insertions(+) diff --git a/sidecar/src/test/kotlin/dev/sanderling/sidecar/InputTextTest.kt b/sidecar/src/test/kotlin/dev/sanderling/sidecar/InputTextTest.kt index 45e016e..26eb887 100644 --- a/sidecar/src/test/kotlin/dev/sanderling/sidecar/InputTextTest.kt +++ b/sidecar/src/test/kotlin/dev/sanderling/sidecar/InputTextTest.kt @@ -240,8 +240,128 @@ class InputTextTest { } assertEquals(listOf("dumpsys input_method"), commands) } + + // Typing is not the only thing that raises the keyboard: tapping a field + // raises it too, and nothing was closing that one. The snapshot the picker + // chooses from is missing every app node the keyboard covers, so the step + // spends its budget choosing between the few targets left. Closing it + // before the tree is read is what gives the step its targets back. + @Test fun aKeyboardInTheTreeIsClosedBeforeTheTreeIsReturned() { + var backs = 0 + val reads = mutableListOf() + val settled = treeWithoutKeyboard( + IME_TREE, + IME_PACKAGE, + dismiss = { backs++ }, + reread = { APP_TREE.also { reads.add(it) } }, + sleep = {}, + ) + assertEquals(APP_TREE, settled) + assertEquals(1, backs, "one BACK closes the keyboard") + assertEquals(1, reads.size, "the tree is re-read once it is gone") + } + + // The guard has to be the tree itself. BACK with no keyboard open + // navigates out of the screen, so a snapshot that pressed it on every read + // would walk the fuzzer backwards out of the app a step at a time. + @Test fun aTreeWithNoKeyboardIsReturnedUntouched() { + var backs = 0 + var reads = 0 + val settled = treeWithoutKeyboard( + APP_TREE, + IME_PACKAGE, + dismiss = { backs++ }, + reread = { + reads++ + APP_TREE + }, + sleep = {}, + ) + assertEquals(APP_TREE, settled) + assertEquals(0, backs, "no keyboard in the tree means no BACK") + assertEquals(0, reads, "and no second hierarchy read to pay for") + } + + // An unknown IME package is the honest "cannot tell", and the safe way to + // be wrong is to leave the keyboard up rather than press BACK blind. + @Test fun anUnknownImePackageSendsNoBack() { + var backs = 0 + assertEquals( + IME_TREE, + treeWithoutKeyboard( + IME_TREE, + null, + dismiss = { backs++ }, + reread = { APP_TREE }, + sleep = {}, + ), + ) + assertEquals(0, backs) + } + + // A keyboard the app puts straight back gets ONE back press, not one per + // re-read. The flag behind the older dismissal lags a BACK by up to 0.6s, + // and a burst of them inside that window is how a dismissal turns into + // navigation. + @Test fun aKeyboardThatStaysUpIsNotBackPressedRepeatedly() { + var backs = 0 + var reads = 0 + val settled = treeWithoutKeyboard( + IME_TREE, + IME_PACKAGE, + dismiss = { backs++ }, + reread = { + reads++ + IME_TREE + }, + sleep = {}, + ) + assertEquals(IME_TREE, settled, "the caller still gets a tree") + assertEquals(1, backs) + assertTrue( + reads in 1..KEYBOARD_DISMISS_READS, + "bounded re-reads, got $reads", + ) + } + + @Test fun imePackageOfReadsTheComponentAndRejectsNonsense() { + assertEquals( + "com.google.android.inputmethod.latin", + imePackageOf( + "com.google.android.inputmethod.latin/.LatinIME\n", + ), + ) + assertEquals(null, imePackageOf("null")) + assertEquals(null, imePackageOf("")) + assertEquals(null, imePackageOf(" \n")) + } + + @Test fun treeShowsImeMatchesTheImesOwnViewIdsOnly() { + assertTrue(treeShowsIme(IME_TREE, IME_PACKAGE)) + assertTrue(!treeShowsIme(APP_TREE, IME_PACKAGE)) + } } +private const val IME_PACKAGE = "com.google.android.inputmethod.latin" + +private val APP_TREE = + """ + {"attributes":{"resource-id":"AddAccountScreen"},"children":[ + {"attributes":{"resource-id":"AccountNameField"},"children":[]}, + {"attributes":{"resource-id":"AddAccountSubmit"},"children":[]}]} + """.trimIndent() + +// The submit control is gone: an open keyboard does not merely cover the node, +// it takes it out of the tree the picker enumerates. +private val IME_TREE = + """ + {"attributes":{"resource-id":"AddAccountScreen"},"children":[ + {"attributes":{"resource-id":"AccountNameField"},"children":[]}, + {"attributes":{ + "resource-id":"com.google.android.inputmethod.latin:id/keyboard_holder" + },"children":[]}]} + """.trimIndent() + private val IME_OPEN_DUMPSYS = """ mCurMethodId=com.google.android.inputmethod.latin/.LatinIME From fbc5102b25be411e938abe0277eee66c7891def8 Mon Sep 17 00:00:00 2001 From: PJ Date: Sat, 15 Aug 2026 20:46:47 +0530 Subject: [PATCH 05/15] chore(make): a target that runs folio's unit tests --- Makefile | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 46ad520..a6607fa 100644 --- a/Makefile +++ b/Makefile @@ -29,7 +29,7 @@ WEB_DIST := replay-ui/dist GOLINES := $(shell $(GO) env GOPATH)/bin/golines -.PHONY: bootstrap proto sidecar sidecar-embed sanderling sanderling-web sanderling-android sanderling-ios install test test-go test-browser test-companion test-kotlin test-spec-api test-ci-scripts spec-typecheck web-test web-typecheck web-build web-dev replay-dev docs clean release-cli release-npm-dry fmt fmt-go fmt-kotlin fmt-ts fmt-swift +.PHONY: bootstrap proto sidecar sidecar-embed sanderling sanderling-web sanderling-android sanderling-ios install test test-go test-browser test-companion test-kotlin test-folio test-spec-api test-ci-scripts spec-typecheck web-test web-typecheck web-build web-dev replay-dev docs clean release-cli release-npm-dry fmt fmt-go fmt-kotlin fmt-ts fmt-swift bootstrap: $(GO) mod download @@ -145,6 +145,14 @@ test-companion: $(COMPANION_EMBED) $(RUNNER_EMBED) test-kotlin: ANDROID_HOME=$(ANDROID_HOME) $(GRADLE) :sidecar:test +# folio is its own gradle build, so nothing in the root build runs its tests. +# Kept out of `test` because the metro plugin folio compiles with needs a 21+ +# runtime, where the sidecar toolchain pins 17: folding this in would raise the +# JDK floor of the target everyone runs constantly. CI runs it as its own step. +test-folio: + cd examples/folio && ANDROID_HOME=$(ANDROID_HOME) $(GRADLE) \ + :core:testDebugUnitTest :app:shared:testDebugUnitTest + # The CI scripts that read a trace and decide whether a green leg is # evidence. bash and python3 only, which is all a runner has. test-ci-scripts: From ca409ca6b87ddae2ad1eb1271e9fa023043db309 Mon Sep 17 00:00:00 2001 From: PJ Date: Sat, 15 Aug 2026 20:46:48 +0530 Subject: [PATCH 06/15] chore(folio): a just recipe for the unit tests --- examples/folio/justfile | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/examples/folio/justfile b/examples/folio/justfile index 4328766..a0bdac6 100644 --- a/examples/folio/justfile +++ b/examples/folio/justfile @@ -78,6 +78,13 @@ _ensure-device: echo "emulator did not finish booting in time (see /tmp/folio-emulator.log)" >&2 exit 1 +# Run folio's own unit tests. Named test-unit because `test` is the fuzz run. +test-unit: + #!/usr/bin/env bash + set -euo pipefail + export ANDROID_HOME="$(just _android-home)" + ./gradlew :core:testDebugUnitTest :app:shared:testDebugUnitTest + # Build the folio debug APK without installing it. build: #!/usr/bin/env bash From cc32640d034aeeac2153acc89ae552cde5460531 Mon Sep 17 00:00:00 2001 From: PJ Date: Sat, 15 Aug 2026 20:46:48 +0530 Subject: [PATCH 07/15] ci: run folio's unit tests on every pr --- .github/workflows/ci.yml | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f4bef62..8bd9390 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -27,11 +27,16 @@ jobs: go-version-file: go.mod cache: true - - name: Set up JDK 17 + - name: Set up the JDKs uses: actions/setup-java@v4 with: distribution: temurin - java-version: "17" + # The sidecar toolchain pins 17; the metro gradle plugin folio + # compiles with needs a 21 runtime. Both are installed so each + # gradle build can pick its own. + java-version: | + 17 + 21 - name: Set up Android SDK uses: android-actions/setup-android@v3 @@ -95,6 +100,12 @@ jobs: - name: Run tests run: make test + # folio's own gradle build, which `make test` does not reach. Its own + # step rather than part of `test` so the target developers run + # constantly keeps its 17 floor. + - name: Run folio's unit tests + run: make test-folio + browser: runs-on: ubuntu-latest steps: From f65bc1631c6ca64f4e4672c100502a90b44bad52 Mon Sep 17 00:00:00 2001 From: PJ Date: Sat, 15 Aug 2026 20:52:00 +0530 Subject: [PATCH 08/15] ci: switch to jdk 21 only for the folio step --- .github/workflows/ci.yml | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8bd9390..82771f2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -27,16 +27,11 @@ jobs: go-version-file: go.mod cache: true - - name: Set up the JDKs + - name: Set up JDK 17 uses: actions/setup-java@v4 with: distribution: temurin - # The sidecar toolchain pins 17; the metro gradle plugin folio - # compiles with needs a 21 runtime. Both are installed so each - # gradle build can pick its own. - java-version: | - 17 - 21 + java-version: "17" - name: Set up Android SDK uses: android-actions/setup-android@v3 @@ -100,9 +95,16 @@ jobs: - name: Run tests run: make test - # folio's own gradle build, which `make test` does not reach. Its own - # step rather than part of `test` so the target developers run - # constantly keeps its 17 floor. + # folio is its own gradle build, and the metro plugin it compiles with + # needs a 21 runtime where the sidecar toolchain pins 17. Switching + # JAVA_HOME after `make test` rather than installing both up front + # leaves every step above this one on exactly the JDK it ran on before. + - name: Set up JDK 21 for folio + uses: actions/setup-java@v4 + with: + distribution: temurin + java-version: "21" + - name: Run folio's unit tests run: make test-folio From 64f99babd5363f01636edb007715fec3de322429 Mon Sep 17 00:00:00 2001 From: PJ Date: Sat, 15 Aug 2026 21:01:00 +0530 Subject: [PATCH 09/15] docs(sidecar): put the measured read cost in the dismissal bound --- .../kotlin/dev/sanderling/sidecar/DriverBackend.kt | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/sidecar/src/main/kotlin/dev/sanderling/sidecar/DriverBackend.kt b/sidecar/src/main/kotlin/dev/sanderling/sidecar/DriverBackend.kt index 7652d5d..89292f2 100644 --- a/sidecar/src/main/kotlin/dev/sanderling/sidecar/DriverBackend.kt +++ b/sidecar/src/main/kotlin/dev/sanderling/sidecar/DriverBackend.kt @@ -809,10 +809,12 @@ internal fun dismissSoftKeyboard(shell: (String) -> String) { } // KEYBOARD_DISMISS_READS bounds the re-reads a snapshot spends waiting for the -// IME window to leave the tree after BACK. The window goes over an animation, -// so the first read back can still carry it; a hierarchy read plus the interval -// costs ~250ms on the emulator, which covers a retraction several times over -// without turning a keyboard the app keeps re-raising into an unbounded wait. +// IME window to leave the tree after BACK. The window leaves over an +// animation, so the first read back can still carry it. A hierarchy read +// measures at a 76ms median and a 168ms p90 on the API 34 emulator, so with +// the interval these four reads watch most of a second: several retractions +// over, without turning a keyboard the app keeps re-raising into a wait with +// no end. internal const val KEYBOARD_DISMISS_READS = 4 internal const val KEYBOARD_DISMISS_INTERVAL_MILLIS = 100L From 23a2454459d6749131a17f11f1fa94de9eddaa75 Mon Sep 17 00:00:00 2001 From: PJ Date: Sat, 15 Aug 2026 21:17:06 +0530 Subject: [PATCH 10/15] fix(sidecar): bound the diagnostic adb reads adbOutput and readLogcat read to EOF and then waited with no timeout, so a wedged adb held the step for as long as it liked; one stall over a remote adb server measured ~100s. The bound has to sit on the read, not on waitFor: a wedged adb never reaches EOF, so a bounded waitFor after the read is a line that never runs. --- .../dev/sanderling/sidecar/DriverBackend.kt | 86 ++++++++++++++++--- 1 file changed, 75 insertions(+), 11 deletions(-) diff --git a/sidecar/src/main/kotlin/dev/sanderling/sidecar/DriverBackend.kt b/sidecar/src/main/kotlin/dev/sanderling/sidecar/DriverBackend.kt index 89292f2..a9a78a8 100644 --- a/sidecar/src/main/kotlin/dev/sanderling/sidecar/DriverBackend.kt +++ b/sidecar/src/main/kotlin/dev/sanderling/sidecar/DriverBackend.kt @@ -329,11 +329,12 @@ internal fun readLogcat( arguments.add(since) } return try { - val process = ProcessBuilder( - adbCmd(serial) + arguments, - ).redirectErrorStream(false).start() - val output = process.inputStream.bufferedReader().readText() - process.waitFor() + val command = adbCmd(serial) + arguments + val output = readProcessOutput( + ProcessBuilder(command).redirectErrorStream(false).start(), + ADB_OUTPUT_TIMEOUT_MILLIS, + describe = { command.joinToString(" ") }, + ) StubDriverBackend.parseLogcatOutput(output) } catch (cause: Exception) { println("adb logcat failed: $cause") @@ -359,13 +360,76 @@ internal fun readProcMetrics(serial: String?, bundleId: String): MetricsSample { private fun adbCmd(serial: String?): List = if (serial == null) listOf("adb") else listOf("adb", "-s", serial) +// ADB_OUTPUT_TIMEOUT_MILLIS bounds the diagnostic adb reads: dumpsys, logcat, +// `settings get`, /proc stats. None of them is the driver's data path, so the +// bound wants to be generous enough that it cannot fire on a link that works, +// and it is: a hierarchy fetch, far heavier than any of these, measures at a +// 76ms median and a 168ms p90 over the same remote adb link. What it caps is +// the other end, where a wedged adb once held a step ~100s. +internal const val ADB_OUTPUT_TIMEOUT_MILLIS = 10_000L + +private val adbReaders: java.util.concurrent.ExecutorService = + java.util.concurrent.Executors.newCachedThreadPool { runnable -> + Thread(runnable, "adb-output").apply { isDaemon = true } + } + +// readProcessOutput returns a process's stdout, or "" when it does not arrive +// inside timeoutMillis. +// +// The bound belongs on the READ, not on waitFor. readText ends at EOF, and a +// wedged adb neither writes nor exits, so EOF never comes and a waitFor with a +// timeout after it is a line that never runs. Waiting first and reading after +// is worse still: a process with more to say than a pipe buffer holds, which +// logcat and dumpsys both are, blocks writing while the waiter waits for it to +// finish, and neither ever moves. +// +// So the read runs on a daemon thread and killing the process is what releases +// it: destroy closes the pipe, the reader sees EOF, the thread ends. Returning +// "" hands every caller the answer it already treats as "adb said nothing", +// which is the safe direction for all of them. +internal fun readProcessOutput( + process: Process, + timeoutMillis: Long, + describe: () -> String, + log: (String) -> Unit = { System.err.println(it) }, +): String { + val reader = adbReaders.submit { + process.inputStream.bufferedReader().readText() + } + return try { + val output = reader.get( + timeoutMillis, + java.util.concurrent.TimeUnit.MILLISECONDS, + ) + if (!process.waitFor( + timeoutMillis, + java.util.concurrent.TimeUnit.MILLISECONDS, + ) + ) { + process.destroyForcibly() + } + output + } catch (cause: java.util.concurrent.TimeoutException) { + process.destroyForcibly() + reader.cancel(true) + log( + "warn: ${describe()} gave nothing in ${timeoutMillis}ms; " + + "killed it and read no answer", + ) + "" + } catch (cause: Exception) { + process.destroyForcibly() + "" + } +} + private fun adbOutput(serial: String?, arguments: List): String = try { - val process = ProcessBuilder( - adbCmd(serial) + arguments, - ).redirectErrorStream(false).start() - val output = process.inputStream.bufferedReader().readText() - process.waitFor() - output + val command = adbCmd(serial) + arguments + readProcessOutput( + ProcessBuilder(command).redirectErrorStream(false).start(), + ADB_OUTPUT_TIMEOUT_MILLIS, + describe = { command.joinToString(" ") }, + ) } catch (cause: Exception) { "" } From 34b8be2d8cd72813086a1f500573ba4a9ae5273a Mon Sep 17 00:00:00 2001 From: PJ Date: Sat, 15 Aug 2026 21:17:09 +0530 Subject: [PATCH 11/15] test(sidecar): pin the bound on a wedged adb read --- .../sidecar/AdbOutputTimeoutTest.kt | 120 ++++++++++++++++++ 1 file changed, 120 insertions(+) create mode 100644 sidecar/src/test/kotlin/dev/sanderling/sidecar/AdbOutputTimeoutTest.kt diff --git a/sidecar/src/test/kotlin/dev/sanderling/sidecar/AdbOutputTimeoutTest.kt b/sidecar/src/test/kotlin/dev/sanderling/sidecar/AdbOutputTimeoutTest.kt new file mode 100644 index 0000000..6a741b9 --- /dev/null +++ b/sidecar/src/test/kotlin/dev/sanderling/sidecar/AdbOutputTimeoutTest.kt @@ -0,0 +1,120 @@ +package dev.sanderling.sidecar + +import org.junit.Test +import java.io.InputStream +import java.io.OutputStream +import java.util.concurrent.CountDownLatch +import kotlin.test.assertEquals +import kotlin.test.assertTrue + +class AdbOutputTimeoutTest { + + // An adb wedged on the link neither writes nor exits, so the read never + // reaches EOF. Without a bound on the READ the step waits for as long as + // adb feels like it: one such stall measured ~100s against a remote adb + // server. The bound has to release the reader as well as return, which is + // what killing the process does. + @Test(timeout = 20_000L) + fun aWedgedReadIsAbandonedAtTheBoundInsteadOfHangingForever() { + val process = FakeProcess(BlockingStream()) + val logged = mutableListOf() + + val started = System.currentTimeMillis() + val output = readProcessOutput(process, 200L, { "adb shell pidof" }) { + logged.add(it) + } + val elapsed = System.currentTimeMillis() - started + + assertEquals("", output, "a read that never lands is no answer") + assertTrue(process.destroyed, "the wedged adb must be killed, not left") + assertTrue( + elapsed < 10_000L, + "returned in ${elapsed}ms, not at a bound", + ) + assertEquals(1, logged.size, "a silent timeout hides a degrading link") + } + + @Test(timeout = 20_000L) + fun theAbandonedReadNamesTheCommandAndTheBound() { + val logged = mutableListOf() + readProcessOutput( + FakeProcess(BlockingStream()), + 200L, + { "adb -s emulator-5556 shell cat /proc/6103/stat" }, + ) { logged.add(it) } + + val line = logged.single() + assertTrue( + line.contains("adb -s emulator-5556 shell cat /proc/6103/stat"), + line, + ) + assertTrue(line.contains("200"), line) + } + + // The bound must cost the healthy path nothing: output that arrives comes + // back whole, and the process is left to exit on its own. + @Test(timeout = 20_000L) + fun outputThatArrivesComesBackWholeAndTheProcessSurvives() { + val text = "VmRSS:\t 123456 kB\nVmSize:\t 654321 kB\n" + val process = FakeProcess(text.byteInputStream()) + val logged = mutableListOf() + + val output = readProcessOutput(process, 5_000L, { "adb shell cat" }) { + logged.add(it) + } + + assertEquals(text, output) + assertTrue(!process.destroyed, "a process that answered is not killed") + assertTrue(logged.isEmpty(), "nothing to report on the healthy path") + } + + // Output larger than a pipe buffer is why the read cannot be deferred + // until after the process exits: a process with more to say than the + // buffer holds blocks writing while a waiter waits for it to finish. + @Test(timeout = 20_000L) + fun outputLargerThanAPipeBufferComesBackWhole() { + val text = "x".repeat(512 * 1024) + val output = readProcessOutput( + FakeProcess(text.byteInputStream()), + 5_000L, + { "adb logcat -d" }, + ) {} + assertEquals(text.length, output.length) + } +} + +// BlockingStream models a wedged adb: no bytes, and no EOF either, until the +// process is killed and the pipe closes under the reader. +private class BlockingStream : InputStream() { + private val released = CountDownLatch(1) + + override fun read(): Int { + released.await() + return -1 + } + + override fun close() { + released.countDown() + } +} + +private class FakeProcess(private val stream: InputStream) : Process() { + @Volatile var destroyed = false + private set + + override fun getOutputStream(): OutputStream = + OutputStream.nullOutputStream() + + override fun getInputStream(): InputStream = stream + + override fun getErrorStream(): InputStream = InputStream.nullInputStream() + + override fun waitFor(): Int = 0 + + override fun exitValue(): Int = 0 + + override fun destroy() { + destroyed = true + stream.close() + } +} From 9ff82b15652f8eb481d4c0a74cb8d6851c3d6268 Mon Sep 17 00:00:00 2001 From: PJ Date: Sat, 15 Aug 2026 21:23:45 +0530 Subject: [PATCH 12/15] fix(sidecar): an unreadable animation count is not idle Defaulting the count to zero made a dumpsys that said nothing mean nothing is animating, so a degraded link broke out of the settle early and handed the runner a frame caught mid-animation. Unknown now waits, inside the deadline waitForIdle already holds. --- .../main/kotlin/dev/sanderling/sidecar/DriverBackend.kt | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/sidecar/src/main/kotlin/dev/sanderling/sidecar/DriverBackend.kt b/sidecar/src/main/kotlin/dev/sanderling/sidecar/DriverBackend.kt index a9a78a8..638a559 100644 --- a/sidecar/src/main/kotlin/dev/sanderling/sidecar/DriverBackend.kt +++ b/sidecar/src/main/kotlin/dev/sanderling/sidecar/DriverBackend.kt @@ -538,8 +538,14 @@ class StubDriverBackend( companion object { private const val IDLE_POLL_INTERVAL_MILLIS = 50L + // A count we could not read is not a count of zero. Defaulting it to + // zero made an unreadable dumpsys mean "nothing is animating, go + // ahead", which is the one answer the caller cannot check: it breaks + // out of the settle and snapshots whatever frame is on screen. Unknown + // keeps it waiting instead, inside the deadline waitForIdle already + // holds, and it agrees with the probe's own exception path. internal fun isAnimationCountIdle(grepOutput: String): Boolean = - (grepOutput.trim().toIntOrNull() ?: 0) == 0 + grepOutput.trim().toIntOrNull() == 0 internal fun parseResolvedActivity( bundleId: String, From 07e8abb9ef912853e5467734c952aef7ebbf0694 Mon Sep 17 00:00:00 2001 From: PJ Date: Sat, 15 Aug 2026 21:23:45 +0530 Subject: [PATCH 13/15] test(sidecar): unknown animation state must not read as idle --- .../dev/sanderling/sidecar/IdleDetectionTest.kt | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/sidecar/src/test/kotlin/dev/sanderling/sidecar/IdleDetectionTest.kt b/sidecar/src/test/kotlin/dev/sanderling/sidecar/IdleDetectionTest.kt index 82d48bd..201b3fe 100644 --- a/sidecar/src/test/kotlin/dev/sanderling/sidecar/IdleDetectionTest.kt +++ b/sidecar/src/test/kotlin/dev/sanderling/sidecar/IdleDetectionTest.kt @@ -21,11 +21,17 @@ class IdleDetectionTest { assertFalse(StubDriverBackend.isAnimationCountIdle("3\n")) } - @Test fun idleWhenOutputEmpty() { - assertTrue(StubDriverBackend.isAnimationCountIdle("")) + // A dumpsys that said nothing does not say the device is still. Reading + // absence as idle is how a settle returns instantly on a degraded link and + // hands the runner a screen caught mid-animation; the caller bounds its own + // wait, so the cost of being wrong the other way is a wait it already + // budgeted for. The exception path of the same probe already answers false. + @Test fun unreadableOutputIsNotIdle() { + assertFalse(StubDriverBackend.isAnimationCountIdle("")) + assertFalse(StubDriverBackend.isAnimationCountIdle(" \n")) } - @Test fun idleWhenOutputIsNotANumber() { - assertTrue(StubDriverBackend.isAnimationCountIdle("error: no service")) + @Test fun unparseableOutputIsNotIdle() { + assertFalse(StubDriverBackend.isAnimationCountIdle("error: no service")) } } From 8d7ef4d681048de02788453ad5ad3076965556f0 Mon Sep 17 00:00:00 2001 From: PJ Date: Sat, 15 Aug 2026 21:26:51 +0530 Subject: [PATCH 14/15] fix(sidecar): a foreground read that fails degrades the typing guard An unreadable dumpsys passed a null owner to typeChunks, which switches the mid-type focus guard off outright and lets the rest of the string spray into whatever holds the foreground. Fall back to the launched bundle instead: the guard stays armed, typing still happens, and the degradation is said out loud rather than assumed away. --- .../dev/sanderling/sidecar/DriverBackend.kt | 58 ++++++++++++++++--- 1 file changed, 51 insertions(+), 7 deletions(-) diff --git a/sidecar/src/main/kotlin/dev/sanderling/sidecar/DriverBackend.kt b/sidecar/src/main/kotlin/dev/sanderling/sidecar/DriverBackend.kt index 638a559..98132b1 100644 --- a/sidecar/src/main/kotlin/dev/sanderling/sidecar/DriverBackend.kt +++ b/sidecar/src/main/kotlin/dev/sanderling/sidecar/DriverBackend.kt @@ -937,6 +937,39 @@ internal fun treeWithoutKeyboard( return current } +// typingOwner picks what the mid-type foreground guard holds later reads +// against. A dumpsys it could read names the resumed package, and that is the +// answer. +// +// A read that failed is the interesting case, and neither obvious answer is +// right. Passing null hands typeChunks "no owner", which switches the guard off +// altogether and lets the rest of the string spray into whatever holds the +// foreground: an unreadable probe must never read as focus being fine. But +// refusing to type is worse in practice. The failure is a degraded link, which +// lasts, so every InputText in the run becomes a no-op, the budget goes on +// typing nothing, and the run ends green having tested nothing. +// +// So it falls back to the bundle the run launched, which leaves the guard armed +// against the app the keystrokes were meant for. That reference is better than +// the resumed package anyway: a foreground already stolen before typing began +// reads as its own owner, and the guard then matches it happily chunk after +// chunk. +internal fun typingOwner( + dumpsys: String, + launchedBundleId: String?, + warn: (String) -> Unit, +): String? { + parseResumedPackage(dumpsys)?.let { return it } + warn( + "warn: could not read the foreground app; guarding typing with " + + ( + launchedBundleId?.let { "the launched bundle $it" } + ?: "nothing, no launch was recorded" + ), + ) + return launchedBundleId +} + // resumedActivityPackage matches a "package/activity" component, mirroring the // Go scope guard's regex so both read the same dumpsys wording. private val resumedActivityPackage = @@ -1013,6 +1046,9 @@ class MaestroDriverBackend(private val serial: String?) : DriverBackend { } } + @Volatile + private var launchedBundleId: String? = null + override fun launch( bundleId: String, clearState: Boolean, @@ -1020,6 +1056,7 @@ class MaestroDriverBackend(private val serial: String?) : DriverBackend { ) { if (clearState) driver.clearAppState(bundleId) driver.launchApp(bundleId, env) + launchedBundleId = bundleId } override fun terminate(bundleId: String) = driver.stopApp(bundleId) @@ -1063,8 +1100,14 @@ class MaestroDriverBackend(private val serial: String?) : DriverBackend { // started in has lost the foreground, the remaining keystrokes would spray // into whatever window stole it (the launcher search box, in practice), so // typing stops instead of leaking out of the app under test. + // + // typingOwner decides what "the app the type started in" means when the + // read that would name it fails: the launched bundle, so a link that cannot + // answer degrades the guard rather than switching it off. private fun typeShellSafe(text: String) { - val owner = foregroundPackage() + val owner = typingOwner(foregroundDumpsys(), launchedBundleId) { + System.err.println(it) + } val typed = typeChunks(chunkForInput(text, INPUT_CHUNK_CHARS), owner, { foregroundPackage() @@ -1078,14 +1121,15 @@ class MaestroDriverBackend(private val serial: String?) : DriverBackend { } } + private fun foregroundDumpsys(): String = adbOutput( + serial, + listOf("shell", "dumpsys", "activity", "activities"), + ) + // foregroundPackage returns the package of the top resumed activity, or null // if it cannot be read. Used to detect mid-type focus escapes. - private fun foregroundPackage(): String? = parseResumedPackage( - adbOutput( - serial, - listOf("shell", "dumpsys", "activity", "activities"), - ), - ) + private fun foregroundPackage(): String? = + parseResumedPackage(foregroundDumpsys()) override fun eraseText(characterCount: Int) = driver.eraseText(characterCount) From ec1fdc85ad2b170c2288513f4c1f3bdde8d908d2 Mon Sep 17 00:00:00 2001 From: PJ Date: Sat, 15 Aug 2026 21:26:51 +0530 Subject: [PATCH 15/15] test(sidecar): pin the degraded typing guard both ways --- .../dev/sanderling/sidecar/InputTextTest.kt | 77 +++++++++++++++++++ 1 file changed, 77 insertions(+) diff --git a/sidecar/src/test/kotlin/dev/sanderling/sidecar/InputTextTest.kt b/sidecar/src/test/kotlin/dev/sanderling/sidecar/InputTextTest.kt index 26eb887..2d2d8bf 100644 --- a/sidecar/src/test/kotlin/dev/sanderling/sidecar/InputTextTest.kt +++ b/sidecar/src/test/kotlin/dev/sanderling/sidecar/InputTextTest.kt @@ -206,6 +206,77 @@ class InputTextTest { ) } + @Test fun aReadableDumpsysNamesTheResumedPackage() { + val warnings = mutableListOf() + assertEquals( + "app.folio", + typingOwner(RESUMED_DUMPSYS, "app.folio") { warnings.add(it) }, + ) + assertTrue(warnings.isEmpty(), "nothing to report when the read worked") + } + + // A dumpsys that said nothing is not evidence that focus is fine. Handing + // typeChunks a null owner turns the guard off outright, and the keystrokes + // then go wherever the foreground happens to be. + @Test fun anUnreadableDumpsysGuardsWithTheLaunchedBundle() { + val warnings = mutableListOf() + assertEquals( + "app.folio", + typingOwner("", "app.folio") { warnings.add(it) }, + ) + assertEquals(1, warnings.size, "a degraded guard must not be silent") + assertTrue(warnings.single().contains("app.folio"), warnings.single()) + } + + // Wording no marker matches is the same "we do not know" as an empty read. + @Test fun dumpsysWithNoResumedMarkerGuardsWithTheLaunchedBundle() { + assertEquals( + "app.folio", + typingOwner(" mFocusedApp=null\n nothing here\n", "app.folio") {}, + ) + } + + // The whole point of the fallback: on a link that cannot answer, typing is + // still guarded, so a foreground that was stolen stops it after the first + // chunk instead of spraying the rest into whatever took focus. + @Test fun unreadableLinkStillStopsTypingWhenFocusWasStolen() { + val owner = typingOwner("", "app.folio") {} + val sent = mutableListOf() + + typeChunks(listOf("aaa", "bbb", "ccc"), owner, { + "com.android.launcher" + }) { sent.add(it) } + + assertEquals( + listOf("aaa"), + sent, + "an unguarded type would have sent every chunk to the launcher", + ) + } + + // And the other half of the trade: the fallback must not turn a degraded + // link into a run that types nothing. A no-op InputText on every step is a + // green run that tested nothing, which is worse than the spray it avoids. + @Test fun unreadableLinkStillTypesWhenTheAppKeepsFocus() { + val owner = typingOwner("", "app.folio") {} + val sent = mutableListOf() + + val typed = typeChunks(listOf("aaa", "bbb", "cc"), owner, { + "app.folio" + }) { sent.add(it) } + + assertEquals(listOf("aaa", "bbb", "cc"), sent) + assertEquals(8, typed) + } + + // With no launch recorded there is nothing to guard against, and the honest + // answer is to say the guard is off rather than imply it ran. + @Test fun noLaunchedBundleLeavesTheGuardOffAndSaysSo() { + val warnings = mutableListOf() + assertEquals(null, typingOwner("", null) { warnings.add(it) }) + assertEquals(1, warnings.size) + } + @Test fun maestroKeyForResolvesAndRejects() { assertEquals(maestro.KeyCode.BACK, maestroKeyFor("back")) assertEquals(maestro.KeyCode.BACK, maestroKeyFor("BACK")) @@ -342,6 +413,12 @@ class InputTextTest { } } +private val RESUMED_DUMPSYS = + """ + mFocusedApp=ActivityRecord{1a u0 app.folio/.MainActivity t14} + topResumedActivity=ActivityRecord{f3 u0 app.folio/.MainActivity t14} + """.trimIndent() + private const val IME_PACKAGE = "com.google.android.inputmethod.latin" private val APP_TREE =