From d759ef32f1d350be5b711afa6b556008f295fa72 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Sat, 5 Sep 2026 22:23:33 -0500 Subject: [PATCH 1/6] Take the picker test off the API 37 gating leg, and give the SystemUI disable root The gating `E2E API 37` leg failed three of the last ten status_check runs. Four runs were read logcat-first -- 34006456986, 34001744574, 34001377499 and the green 34002313300 -- and each carries exactly two `hasReadColorBufferDma` aborts before the suite (surfaceflinger, during boot and the SystemUI disable) and exactly one during it: `system_server`, thread `TaskSnapshotPer`, always inside `pickingAFileThroughTheSystemPickerFillsInTheFileCard`'s window. Nothing else in the gating set reaches the mapper. So that test kills the framework on this image whether it passes or not, and whether the leg goes red is luck: 34001377499 passed it and lost the leg anyway with `failed: 0`, 34002313300 passed it 0.6 s after the abort and went green. That is #108. The test now carries `@FailsOnEmulatorApi37` and the baseline goes 3 -> 4; the marker's own wording widens from "does not pass on this image" to "cannot be run on this image", because this carrier passes about half the time. `docs/api-37-emulator-crash.md` had counted those aborts on 2026-08-24, put them in its table, and then read the pass/fail column alone. The correction is recorded beside the original rather than replacing it. Probed on the same image and recorded there too: there is no shell knob for task snapshots -- not in `getprop`, `settings`, `device_config` or `cmd window` -- so the marker is the available answer rather than the lazy one. Two separate defects came out of the same logcats. `disable_region_sampling` has never restarted the framework on CI. `adb shell stop` and `start` are root-only and every API 37 leg has printed `Must be root` for both, so SystemUI stayed up for the whole run -- visible directly as `WindowManagerShell ... app=com.android.systemui` minutes after "final state: SystemUI disabled". Both waits also printed their own exhaustion as an elapsed time, so "system_server down after ~40 s" is what a stop that did nothing looks like. Measured on the local android-37.0 AVD, same fingerprint as CI: `adb root` makes `stop` return 0 with `pidof system_server` empty. Root is dropped again before Gradle runs, and both waits now say whether they observed anything. And when the picker test does fail, the abort is the coda rather than the cause: `InputDispatcher: No new touched window at (539.0, 525.0)` is in both reds and absent from the green, so the tap on the root is discarded, the picker is never navigated, and all four back presses land on an activity WindowManager says has not added a window yet. `forceStopThePicker` goes around input entirely so `pickTheFixture`'s whole-picker retry -- which exists for exactly this -- becomes reachable. That one is a fix on every API level, not just 37. Co-Authored-By: Claude Opus 5 (1M context) --- .github/scripts/e2e-run.sh | 56 +++++++- CLAUDE.md | 25 +++- .../FailsOnEmulatorApi37.kt | 25 +++- .../saf/SafPickerRoundTripTest.kt | 72 +++++++++- docs/api-37-emulator-crash.md | 135 ++++++++++++++++++ 5 files changed, 293 insertions(+), 20 deletions(-) diff --git a/.github/scripts/e2e-run.sh b/.github/scripts/e2e-run.sh index a631598..1b3e89e 100755 --- a/.github/scripts/e2e-run.sh +++ b/.github/scripts/e2e-run.sh @@ -78,12 +78,30 @@ WEDGE_TIMEOUT=1200 # system_server that was still exiting, so its wait was not a wait), bring it back, verify the # package against `pm list packages -d`, and require a 45 s window with zero new aborts. # Three rounds, because one is not reliable and the failure is silent. +# +# THE RESTART NEEDED ROOT, AND DID NOT HAVE IT UNTIL 2026-09-05. `adb shell stop` and +# `adb shell start` are root-only, so every API 37 leg ever run printed `Must be root` twice and +# restarted nothing -- green legs and red ones alike. SystemUI therefore stayed up for the whole +# run, which the logcat shows directly (`WindowManagerShell ... app=com.android.systemui`, from a +# live SystemUI pid, minutes after the "final state: SystemUI disabled" line). The paragraph above +# says why the `pm disable-user` on its own buys nothing: it does not retract the registration. +# +# The images are userdebug -- `google/sdk_gphone64_x86_64/emu64xa:17/...:userdebug/dev-keys` -- so +# `adb root` is available and was the only thing missing. Measured on the local android-37.0 AVD, +# same image and fingerprint as CI: `stop` -> `Must be root`; `adb root` -> `whoami` says `root`; +# `stop` -> exit 0 and `pidof system_server` empty; `start` -> exit 0. +# +# Root is dropped again before the suite runs. Everything Gradle does afterwards -- install, +# instrument, uninstall -- has to be what the other four legs do, and `adb root` changes the uid +# every later `adb shell` runs as. Both calls restart adbd, hence `wait-for-device` after each. # --------------------------------------------------------------------------- count_aborts() { adb logcat -d -b crash 2> /dev/null | grep -c 'hasReadColorBufferDma'; } systemui_disabled() { adb shell pm list packages -d 2> /dev/null | grep -q 'com.android.systemui'; } +adb_as_root() { adb root > /dev/null 2>&1; adb wait-for-device; } +adb_as_shell() { adb unroot > /dev/null 2>&1; adb wait-for-device; } disable_region_sampling() { - local round=1 i out before after + local round=1 i out down back before after while [ "$round" -le 3 ]; do echo "--- SystemUI disable, round $round ---" for i in $(seq 1 10); do @@ -94,22 +112,48 @@ disable_region_sampling() { done echo " restarting the framework" - adb shell stop + adb_as_root + echo " adbd is running as $(adb shell whoami 2>&1 | tr -d '\r')" + out="$(adb shell stop 2>&1 | tr -d '\r')" + [ -n "$out" ] && echo " stop said: $out" + # `down` rather than reading `i` afterwards: the loop leaves `i` at 20 whether it broke on the + # process being gone or simply ran out, and the old version printed that as "system_server down + # after ~40 s" for a stop that had done nothing at all. A wait that did not observe the thing + # it was waiting for has to say so. + down=no for i in $(seq 1 20); do - [ -z "$(adb shell pidof system_server 2> /dev/null | tr -d '\r\n')" ] && break + if [ -z "$(adb shell pidof system_server 2> /dev/null | tr -d '\r\n')" ]; then + down=yes + break + fi sleep 2 done - echo " system_server down after ~$((i * 2)) s" - adb shell start + if [ "$down" = "yes" ]; then + echo " system_server down after ~$((i * 2)) s" + else + echo " system_server STILL RUNNING after ~$((i * 2)) s -- the stop did not take" + fi + out="$(adb shell start 2>&1 | tr -d '\r')" + [ -n "$out" ] && echo " start said: $out" + adb_as_shell + # Same flag, same reason as `down` above: this loop also used to report its own exhaustion as + # an elapsed time, so "services back after ~150 s" and "services never came back" printed the + # same line. + back=no for i in $(seq 1 30); do if adb shell service check package 2> /dev/null | grep -q ': found' \ && adb shell service check activity 2> /dev/null | grep -q ': found' \ && [ -n "$(adb shell pidof system_server 2> /dev/null | tr -d '\r\n')" ]; then - echo " services back after ~$((i * 5)) s" + back=yes break fi sleep 5 done + if [ "$back" = "yes" ]; then + echo " services back after ~$((i * 5)) s" + else + echo " services NOT back after ~$((i * 5)) s -- package, activity or system_server missing" + fi if systemui_disabled; then echo " verified: com.android.systemui is in pm list packages -d" diff --git a/CLAUDE.md b/CLAUDE.md index ad54617..c5f7bd7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -76,17 +76,28 @@ days. Read it as the current answer, and see the git history if you need the old `angle_indirect` and `swangle_indirect` all boot, while `auto`, `off`, `guest` and `swiftshader_indirect` do not. `docs/local-emulator.md` has the evidence and the per-API renderer table. -- **CI runs API 37, and it gates.** The matrix is 33/34/35/36/37. **Three** of the 60 instrumented - tests cannot pass on that image, for two unrelated reasons: two Media3 hardware transcodes fail - inside the emulator's own `c2.goldfish.h264.decoder`, and one SAF test takes the framework down - when it rotates the display. All three carry `@FailsOnEmulatorApi37` and run in a separate - `continue-on-error` job; the gating leg runs the other 57. +- **CI runs API 37, and it gates.** The matrix is 33/34/35/36/37. **Four** of the 60 instrumented + tests cannot be *run* on that image, for three unrelated reasons: two Media3 hardware transcodes + fail inside the emulator's own `c2.goldfish.h264.decoder`, one SAF test takes the framework down + when it rotates the display, and its sibling — the SAF picker round trip — aborts `system_server` + from the task-snapshot path whether it passes or not. All four carry `@FailsOnEmulatorApi37` and + run in a separate `continue-on-error` job; the gating leg runs the other 56. + + **That third reason is why "cannot pass" became "cannot be run" on 2026-09-05.** Four gating + runs were read logcat-first — 34006456986, 34001744574, 34001377499 and the green 34002313300 — + and each carries exactly two `hasReadColorBufferDma` aborts before the suite (surfaceflinger, + during boot and the SystemUI disable) and exactly **one** during it: `system_server`, thread + `TaskSnapshotPer`, always inside the picker test's window, and nothing else in the gating set + reaches the mapper at all. Whether the leg went red was luck — one run passed the test and lost + the leg anyway with `failed: 0`, another passed it 0.6 s after the abort and went green. That is + #108, it cost roughly a third of the gating legs over the wave-4 landings (#190), and a marker + is what it needed. `docs/api-37-emulator-crash.md` has the timings. That job is still called `E2E API 37 Media3 hardware transcode (advisory)`, which no longer describes everything in it. The name is kept deliberately — it is not a required context and people have learned to look for it — so **read the marker, not the name**, for what it holds. **It is red on every PR, by design**: do not read it as your change breaking something, and do - not read a green run as evidence those three tests pass. + not read a green run as evidence those four tests pass. `docs/api-37-emulator-crash.md` has the measurements. **That instruction is also why nobody looks, so the job now reports its own shape** — expected, @@ -106,7 +117,7 @@ days. Read it as the current answer, and see the git history if you need the old is gradle never returning, so the log it left says nothing about it. Still true, and the reason the advisory job is not simply deleted: **API 37 needs a manual check on -the Pixel 10 Pro XL before each release.** Those three tests are the one thing CI cannot answer +the Pixel 10 Pro XL before each release.** Those four tests are the one thing CI cannot answer for. On a device or emulator, build only the ABI it can execute: diff --git a/app/src/androidTest/java/org/libremediaconverter/FailsOnEmulatorApi37.kt b/app/src/androidTest/java/org/libremediaconverter/FailsOnEmulatorApi37.kt index a816864..7ff9511 100644 --- a/app/src/androidTest/java/org/libremediaconverter/FailsOnEmulatorApi37.kt +++ b/app/src/androidTest/java/org/libremediaconverter/FailsOnEmulatorApi37.kt @@ -1,7 +1,7 @@ package org.libremediaconverter /** - * Marks an instrumented test that does not pass on the `android-37.x` **emulator** system images. + * Marks an instrumented test that cannot be run on the `android-37.x` **emulator** system images. * * This is a marker, not a skip. Nothing reads it except CI, and CI reads it twice — once with * `notAnnotation` to build the gating API 37 leg, and once with `annotation` to build the advisory @@ -9,6 +9,15 @@ package org.libremediaconverter * That is the whole reason there is one annotation rather than a pair of test lists: two lists * drift, and the drift is silent in both directions (a test that runs nowhere reads as green). * + * **"Cannot be run" covers two things, and it said only the first until 2026-09-05.** Three of the + * four carriers simply fail: two Media3 transcodes die in the image's own `c2.goldfish.h264 + * .decoder`, and the SAF rotation test takes the framework down with it. The fourth — + * `SafPickerRoundTripTest.pickingAFileThroughTheSystemPickerFillsInTheFileCard` — **passes about + * half the time and aborts `system_server` every time**, which is worse for a gating leg than an + * honest failure: it fails the leg from the teardown, with no failing test to point at (#108). + * The wording was widened rather than the test excused; that test's own KDoc has the four-run + * measurement. + * * It says only what has been measured: **on the emulator, at API 37.** The same tests pass on a * physical Pixel 10 Pro XL at API 37 and at API 33–36 on the same runner under the same renderer, * so this must never be read as "this test is allowed to fail at API 37" — only as "the API 37 @@ -17,7 +26,7 @@ package org.libremediaconverter * * Removing it is the goal, and the trigger is written down: a new API 37.x system image, or an * ATD image for 37. Delete the annotation from the tests, and the advisory job goes empty and - * the gating one grows by two. + * the gating one grows by four. * * **How many tests carry it is committed below**, as [FAILS_ON_EMULATOR_API37_BASELINE], and the * advisory job checks the run against it. Adding or removing a marker means changing that number @@ -37,11 +46,19 @@ annotation class FailsOnEmulatorApi37 * keep printing with nothing to compare to, so it announces that it could not read the baseline * rather than falling quiet. If you see that notice, this line is what it means. * - * **One number, both checks, and that is what the marker means.** A test carrying it cannot pass + * **One number, both checks, and that is what the marker means.** A test carrying it cannot be run * on this image, so the count is simultaneously how many the advisory leg runs and how many fail. * A *smaller* failure count is the interesting direction: it means one of them now passes, which * is the trigger the KDoc above names for deleting the annotation. * + * **The fourth carrier is the one to read that sentence carefully for.** + * `pickingAFileThroughTheSystemPickerFillsInTheFileCard` was marked on 2026-09-05 for aborting + * `system_server` rather than for failing (#108), and on the gating leg it passed two runs of + * four. On the advisory leg it runs after the rotation test has already taken the framework down, + * which is why the count still holds there — measured, not assumed, and the measurement is the + * reason this line did not have to become two numbers. If it ever starts reporting three failures + * out of four, read that as this test having got lucky rather than as an image that improved. + * * So: adding or removing a [FailsOnEmulatorApi37] means changing this number, in this file, in * the same diff. The report says so on the run itself if you forget — it prints the tree's own * `grep` count beside this one. @@ -52,4 +69,4 @@ annotation class FailsOnEmulatorApi37 * `INSTRUMENTATION_ABORTED`, so the count is a number taken from a partial run. The report * records the truncation next to the counts for that reason. */ -const val FAILS_ON_EMULATOR_API37_BASELINE = 3 +const val FAILS_ON_EMULATOR_API37_BASELINE = 4 diff --git a/app/src/androidTest/java/org/libremediaconverter/saf/SafPickerRoundTripTest.kt b/app/src/androidTest/java/org/libremediaconverter/saf/SafPickerRoundTripTest.kt index ba08da1..959f8c3 100644 --- a/app/src/androidTest/java/org/libremediaconverter/saf/SafPickerRoundTripTest.kt +++ b/app/src/androidTest/java/org/libremediaconverter/saf/SafPickerRoundTripTest.kt @@ -296,7 +296,35 @@ class SafPickerRoundTripTest { device.waitForIdle() } + /** + * **Marked for API 37 because of what it does to the image, not because it fails there.** + * + * This is the one place the marker's KDoc phrase "cannot pass on this image" does not fit, and + * the distinction is worth keeping rather than smoothing over. Across the four gating API 37 + * runs whose logcats were read on 2026-09-05 — 34006456986, 34001744574, 34001377499 and the + * green 34002313300 — the leg carries exactly two `hasReadColorBufferDma` aborts before the + * suite starts (both `surfaceflinger`, during boot and the SystemUI disable) and then exactly + * **one** during it. Every time, that one is `system_server` on the `TaskSnapshotPer` thread, + * and every time it lands inside this test's window. No other test in the gating set reaches + * the mapper at all. + * + * So this test kills the framework on that image whether it passes or not, and whether the leg + * goes red is luck: 34001377499 passed it and lost the leg anyway (`failed: 0`, teardown + * broken), 34002313300 passed it 0.6 s after the abort and went green. That is #108, and it is + * why the leg was failing on unrelated PRs. + * + * `docs/api-37-emulator-crash.md` measured this test on 2026-08-24, recorded "passes, 4 aborts + * in the window", and concluded that a rotation reaches the mapper where starting DocumentsUI + * does not. The aborts were seen; what was not drawn out is that they are this test's own and + * are not intermittent. + * + * The marker is what routes it off the gating leg and into the advisory job beside its + * rotation sibling. **It is not a statement about the picker**: the same test passes on API + * 33–36 on the same runner and on the Pixel 10 Pro XL, which is where API 37's answer comes + * from. + */ @Test + @FailsOnEmulatorApi37 fun pickingAFileThroughTheSystemPickerFillsInTheFileCard() { pickTheFixture() @@ -602,6 +630,9 @@ class SafPickerRoundTripTest { * It is also why this counts backs rather than pressing a fixed number of them. One back is * enough from Recent and two are needed from inside the root, but a third from Recent would * finish `MainActivity` and take the rest of the test with it. + * + * **[forceStopThePicker] is the escalation after the presses, and it exists because a back + * press is not always deliverable.** See its own KDoc for the measurement. */ private fun dismissThePicker() { repeat(BACK_PRESSES) { @@ -616,15 +647,50 @@ class SafPickerRoundTripTest { // The check after the last press, and not a spare one: `repeat` presses on its final // iteration too, so without this a dismissal that worked on the last press would still be // reported as a failure to close. + if (awaitAppFocus()) return + forceStopThePicker() if (!awaitAppFocus()) { throw AssertionError( - "the system picker would not close: after $BACK_PRESSES back presses the app " + - "still does not have the window focus, and ${device.currentPackageName} is " + - "in front. What could be seen: " + describeWindows(), + "the system picker would not close: after $BACK_PRESSES back presses and a " + + "force-stop of $DOCUMENTS_UI_PACKAGE the app still does not have the window " + + "focus, and ${device.currentPackageName} is in front. What could be seen: " + + describeWindows(), ) } } + /** + * Kills the picker's process, for when no back press can reach it. + * + * **The failure this exists for cannot be answered with input, and that is the whole point.** + * Measured on the gating API 37 legs of runs 34006456986 and 34001744574, which fail this way + * and whose logcats say the same thing in the same order. `UiObject2.click()` on the fixture's + * root is injected at the node's centre and the framework discards it — + * `InputDispatcher: No new touched window at (539.0, 525.0) in display 0` — because + * `PickActivity` has published accessibility nodes but has no touchable window there yet. + * `click()` cannot see that and returns normally, so the walk goes on to wait out + * [PICKER_TIMEOUT_MS] for a fixture that was never navigated to. By the time this function's + * caller starts pressing back, WindowManager is still saying + * `no window has focus but ...PickActivity may eventually add a window when it finishes + * starting up` — and goes on saying it for another 63 s. Every one of the four presses is + * dropped, and DocumentsUI ANRs on `Input dispatching timed out`. + * + * So the picker is in front, unreachable by key or by touch, and [pickTheFixture]'s whole + * point — that a second `PickActivity` rebuilds every window and list in it — is unreachable + * with it. `am force-stop` goes around input entirely: `UiAutomation` runs shell commands as + * uid 2000, which holds `FORCE_STOP_PACKAGES`, so the picker's process is killed, its + * activity leaves the task it was launched into, and `MainActivity` — the activity below it in + * that same task — is resumed with the focus. + * + * **Only on the failure path**, after every back press has been spent, so a picker that closes + * the ordinary way never reaches this and is not altered by it. If the framework itself is + * gone, this cannot help either, and the caller still reports what it could see. + */ + private fun forceStopThePicker() { + device.executeShellCommand("am force-stop $DOCUMENTS_UI_PACKAGE") + device.waitForIdle() + } + /** True once [MainActivity] has the window focus, false if it does not take it in time. */ private fun awaitAppFocus(): Boolean = try { composeRule.waitUntil("the app has the window focus back", FOCUS_TIMEOUT_MS) { diff --git a/docs/api-37-emulator-crash.md b/docs/api-37-emulator-crash.md index 92530fe..d8905a3 100644 --- a/docs/api-37-emulator-crash.md +++ b/docs/api-37-emulator-crash.md @@ -325,6 +325,48 @@ retract an existing registration — it only stops the package being started aga and proceeding straight to the tests fails exactly as before. The harness therefore does `stop; start` afterwards, so the framework that comes back never starts SystemUI at all. +**And on CI that `stop; start` did nothing at all until 2026-09-05.** Both are root-only commands, +adbd on a freshly booted emulator is not root, and the step log had been saying so on every API 37 +leg since the function was written — `Must be root`, twice, between lines that read as if the +restart had happened: + +``` + restarting the framework +Must be root + system_server down after ~40 s +Must be root + services back after ~5 s + verified: com.android.systemui is in pm list packages -d +``` + +Neither number was an observation. The `pidof` loop breaks when the process is gone and otherwise +falls out at its last iteration, and the old code printed the iteration count either way — so +"down after ~40 s" is what a stop that did nothing looks like. The paragraph above is what makes +this matter rather than merely untidy: without the restart the disable buys nothing, and the +logcat confirms it directly — SystemUI is alive for the whole run, logging +`WindowManagerShell ... app=com.android.systemui` minutes after `final state: SystemUI disabled`. + +The images are userdebug, so `adb root` is all that was missing. Measured on the local +`android-37.0` AVD, same fingerprint as CI +(`google/sdk_gphone64_x86_64/emu64xa:17/CE2A.260420.019/15611780:userdebug/dev-keys`): + +``` +adb shell stop -> Must be root +adb root -> restarting adbd as root +adb shell whoami -> root +adb shell stop -> exit 0; adb shell pidof system_server -> (empty) +adb shell start -> exit 0 +``` + +`disable_region_sampling` now takes root for the restart and drops it again with `adb unroot` +before Gradle runs, so install, instrument and uninstall happen as the other four legs do it. Both +waits report whether they observed what they were waiting for instead of printing their own +exhaustion as an elapsed time. + +**It does not touch the picker test's abort**, and it was never going to: that one is +WindowManager inside `system_server`, not SystemUI. What it fixes is the *idle* trigger this +section is about, which had been left running on every leg. + ### The two deviations, stated plainly 1. **The renderer is ANGLE, not the host GPU.** Shared with nothing else in the matrix — API @@ -357,6 +399,99 @@ So a rotation, which rebuilds every surface at once, is what the mapper does not starting DocumentsUI is not. Only the rotation test carries `@FailsOnEmulatorApi37`; the picker test runs on the gating leg like anything else. +#### That last sentence was wrong for twelve days, and the aborts in the table said so + +**Corrected 2026-09-05.** Read the second row again: the picker test passes *and takes four +`hasReadColorBufferDma` aborts with it*. This section counted them, put them in the table, and then +drew the conclusion from the pass/fail column alone. The right question is not "does the test +pass" but "does the image survive it", and the answer had been printed in the right-hand column +from the day it was written. + +Four gating API 37 runs read logcat-first — 34006456986, 34001744574, 34001377499, and the **green** +34002313300 — say it without ambiguity. Each carries exactly two aborts before the suite starts +(both `surfaceflinger`, during boot and the SystemUI disable) and then exactly **one** during it: + +| run | picker test window | the run's only in-suite abort | leg | +|---|---|---|---| +| 34006456986 | 02:33:04.2 → 02:34:46.9, **failed** | 02:34:46.845 | red, `failed: 1` | +| 34001744574 | 00:55:41.4 → 00:57:23.9, **failed** | 00:57:23.794 | red, `failed: 1` | +| 34001377499 | 00:35:53.3 → 00:36:00.6, passed | 00:35:59.662 | red, `failed: 0` | +| 34002313300 | 00:58:12.7 → 00:58:19.8, passed | 00:58:19.218 | green | + +Every one is `system_server`, thread `TaskSnapshotPer`, and every one lands inside that test's +window. Nothing else in the gating set of 57 reaches the mapper at all. So the picker test is +**deterministic** in what it does to the image and a coin flip in what the leg reports: 34001377499 +passed it and lost the leg from teardown with no failing test to name, and 34002313300 passed it +0.6 s after the abort and went green. + +That is #108, which had been filed against this behaviour in August and left open because the +trigger was unknown. The trigger is this test. It now carries `@FailsOnEmulatorApi37` too, and the +marker's KDoc had to widen from "does not pass on this image" to "cannot be run on this image" to +say so honestly. + +The stack, for the record, is a different caller from either of the two above: + +``` +Cmdline: system_server name: TaskSnapshotPer +Abort message: 'Assertion failed: !rcEnc->featureInfo()->hasReadColorBufferDma' + + #04 mapper.ranchu.so GoldfishMapper::readFromHost(cb_handle_t const&) const+543 + #06 libui.so android::Gralloc5Mapper::lock(...)+63 + #10 libandroid_runtime.so android::lockImageFromBuffer(...)+374 + #15 framework.jar android.media.ImageReader$SurfaceImage.getPlanes+50 + #17 services.jar com.android.server.wm.TaskSnapshotConvertUtil.copyToSwBitmapDirect+56 + #28 services.jar com.android.server.wm.SnapshotPersistQueue$StoreWriteQueueItem.writeBuffer+66 + #32 services.jar com.android.server.wm.SnapshotPersistQueue$1.run+186 +``` + +WindowManager writing a task snapshot to disk, which needs the buffer as a software bitmap, which +is the non-DMA readback path. `PickActivity` is started **into the app's own task** (`Task #11 +A=10234:org.libremediaconverter` in the logcat), so the snapshot being persisted is that task's, +and the churn at the end of the pick is what schedules it. + +#### There is no shell knob for task snapshots, and that was checked rather than assumed + +#108 asks whether `TaskSnapshotPersister` is suppressible the way the region-sampling listener was. +Probed on a local `android-37.0 google_apis x86_64` AVD, 2026-09-05: + +``` +getprop | grep -i snapshot # nothing but apexd-snapshotde +settings list global | grep -iE 'snapshot|recents' # empty +device_config list window_manager | grep -i snapshot # empty +cmd window help # no snapshot or screenshot command +dumpsys window | grep -i snapshot # mSnapshotEnabled=true, for Task and Activity +``` + +`mSnapshotEnabled` is real state and there is nothing that sets it from outside. The only +`device_config` hits anywhere in the tree are aconfig flags — e.g. +`windowing_frontend/com.android.window.flags.respect_requested_task_snapshot_resolution` — which +tune the snapshot rather than disable it. So the marker is the available answer, not the lazy one. + +#### When the picker test does fail, the abort is the coda and not the cause + +Worth separating, because the failure message points the wrong way. In both runs where the test +itself went red, it had been broken for 98 seconds before the abort landed. The discriminator is +one line, present in both reds and absent from the green: + +``` +I/InputDispatcher: No new touched window at (539.0, 525.0) in display 0 +``` + +(539, 525) is the centre of the fixture's root row — the same coordinates the green run clicks. +The touch reaches no window and is discarded; `UiObject2.click()` cannot see that and returns +normally. DocumentsUI then logs nothing at all, where the green run logs `DocumentStack` and +`Creating new directory loader` 40 ms after its click. The walk waits out its timeout twice for a +fixture it never navigated to, and by the time the back presses start, WindowManager is still +saying `no window has focus but ...PickActivity may eventually add a window when it finishes +starting up` — for another 63 s. All four presses are dropped, DocumentsUI ANRs on +`Input dispatching timed out`, and only *then* does the abort fire and make the failure message +read `no windows at all`. + +`SafPickerRoundTripTest.forceStopThePicker` is the answer to that half: `am force-stop` goes around +input entirely, so the picker's process can be removed from a task no key press can reach and +`pickTheFixture`'s whole-picker retry — which exists for exactly this — becomes reachable again. +That is a fix to the test on every level, not to API 37. + #### The correction that produced that table **The first version of this section said both tests failed, and put the marker on the class.** The From 393b931fffd6a980b21f43b096506ae61a7e0627 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Sat, 5 Sep 2026 22:54:27 -0500 Subject: [PATCH 2/6] Give api37-debug's own SystemUI disable the same root, and say why there are two The debug workflow's header says it does not fork e2e-run.sh, and it does not -- but it drives the SystemUI disable from its own probe step, so `disable_system_ui` can be turned off for a dispatch. That is a second copy of the same logic, and run 34008889182 showed it carrying the same defect the real leg had: `Must be root` twice, and `system_server down after 40 s` printed for a stop that did nothing. Same fix, and a header note so the next person changing one knows to change both. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/api37-debug.yml | 46 +++++++++++++++++++++++++++---- 1 file changed, 40 insertions(+), 6 deletions(-) diff --git a/.github/workflows/api37-debug.yml b/.github/workflows/api37-debug.yml index 31e3a87..b9cbe66 100644 --- a/.github/workflows/api37-debug.yml +++ b/.github/workflows/api37-debug.yml @@ -28,6 +28,13 @@ name: API 37 debug # - It does not fork .github/scripts/e2e-run.sh. That script owns the FAILED-vs-WEDGED # split, the SIGQUIT thread dump and the streamed logcat, and it is the copy CI # exercises every day. This calls it, exactly as status_check.yml does. +# +# The SystemUI disable below is the exception, and it is a real one: this workflow +# drives it from its own probe step so `disable_system_ui` can be turned off for a +# dispatch, where the real leg gets it through `E2E_DISABLE_SYSTEM_UI`. Two copies of +# that logic therefore exist, and on 2026-09-05 both carried the same defect -- no +# `adb root`, so `adb shell stop` answered `Must be root` and nothing restarted. Fix +# one and the other needs the same change in the same diff. # - It does not change status_check.yml. If a configuration here turns out to work, # the change to the real matrix is proposed separately. # @@ -294,24 +301,51 @@ jobs: # pm disable-user does not retract SystemUI's existing region-sampling # registration -- by the time boot completes it has already registered. Only a # framework restart brings back a SystemUI-less SurfaceFlinger. See - # disable_region_sampling in tools/local-emulator/run-e2e.sh. + # disable_region_sampling in .github/scripts/e2e-run.sh, which this mirrors. + # + # `adb root` first, because `stop` and `start` are root-only and adbd is not root + # on a freshly booted emulator: without it both printed `Must be root` and the + # restart never happened, on this workflow and on the real leg alike, for as long + # as either has existed. Root is dropped again before the suite runs so Gradle's + # install and instrument happen as an unprivileged adb does them. Both flags below + # exist because each loop used to print its own exhaustion as an elapsed time -- + # "system_server down after 40 s" was what a stop that did nothing looked like. echo " restarting the framework" - adb shell stop + adb root > /dev/null 2>&1; adb wait-for-device + echo " adbd is running as $(adb shell whoami 2>&1 | tr -d '\r')" + out="$(adb shell stop 2>&1 | tr -d '\r')" + [ -n "$out" ] && echo " stop said: $out" + down=no for i in $(seq 1 20); do - [ -z "$(adb shell pidof system_server 2> /dev/null | tr -d '\r\n')" ] && break + if [ -z "$(adb shell pidof system_server 2> /dev/null | tr -d '\r\n')" ]; then + down=yes + break + fi sleep 2 done - echo " system_server down after $((i * 2)) s" - adb shell start + if [ "$down" = "yes" ]; then + echo " system_server down after $((i * 2)) s" + else + echo " system_server STILL RUNNING after $((i * 2)) s -- the stop did not take" + fi + out="$(adb shell start 2>&1 | tr -d '\r')" + [ -n "$out" ] && echo " start said: $out" + adb unroot > /dev/null 2>&1; adb wait-for-device + back=no for i in $(seq 1 30); do if adb shell service check package 2> /dev/null | grep -q ': found' \ && adb shell service check activity 2> /dev/null | grep -q ': found' \ && [ -n "$(adb shell pidof system_server 2> /dev/null | tr -d '\r\n')" ]; then - echo " services back after $((i * 5)) s" + back=yes break fi sleep 5 done + if [ "$back" = "yes" ]; then + echo " services back after $((i * 5)) s" + else + echo " services NOT back after $((i * 5)) s" + fi if systemui_disabled; then echo " verified: com.android.systemui is in pm list packages -d" From e2f8ef2918f879889b1281a9948bbb1b12f67c4b Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Sat, 5 Sep 2026 22:55:42 -0500 Subject: [PATCH 3/6] Cite the two measurements the last two commits asserted The advisory-leg count (4 expected, 4 received, 4 failed with the new marker) is api37-debug run 34008889182, dispatched with the annotation as its filter. The force-stop recovery was made to go red before it was believed: on a local API 36 emulator, with the picker left open and the back presses removed, the test passes with forceStopThePicker() and fails with exactly the API 37 message without it. Co-Authored-By: Claude Opus 5 (1M context) --- .../org/libremediaconverter/FailsOnEmulatorApi37.kt | 11 +++++++---- docs/api-37-emulator-crash.md | 12 ++++++++++++ 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/app/src/androidTest/java/org/libremediaconverter/FailsOnEmulatorApi37.kt b/app/src/androidTest/java/org/libremediaconverter/FailsOnEmulatorApi37.kt index 7ff9511..8db3c5a 100644 --- a/app/src/androidTest/java/org/libremediaconverter/FailsOnEmulatorApi37.kt +++ b/app/src/androidTest/java/org/libremediaconverter/FailsOnEmulatorApi37.kt @@ -54,10 +54,13 @@ annotation class FailsOnEmulatorApi37 * **The fourth carrier is the one to read that sentence carefully for.** * `pickingAFileThroughTheSystemPickerFillsInTheFileCard` was marked on 2026-09-05 for aborting * `system_server` rather than for failing (#108), and on the gating leg it passed two runs of - * four. On the advisory leg it runs after the rotation test has already taken the framework down, - * which is why the count still holds there — measured, not assumed, and the measurement is the - * reason this line did not have to become two numbers. If it ever starts reporting three failures - * out of four, read that as this test having got lucky rather than as an image that improved. + * four. The count still holds on the advisory leg, and that was **measured rather than assumed**: + * `api37-debug.yml` run 34008889182, dispatched with this annotation as its filter, reports + * `expected: 4, received: 4, failed: 4` (and `completed cleanly: no`, which is this job's normal). + * It fails there because it runs alongside the rotation test, which takes the framework down first + * — so the reason this line did not have to become two numbers is a property of the advisory leg, + * not of the test. If it ever reports three failures out of four, read that as this test having + * got lucky rather than as an image that improved. * * So: adding or removing a [FailsOnEmulatorApi37] means changing this number, in this file, in * the same diff. The report says so on the run itself if you forget — it prints the tree's own diff --git a/docs/api-37-emulator-crash.md b/docs/api-37-emulator-crash.md index d8905a3..911043a 100644 --- a/docs/api-37-emulator-crash.md +++ b/docs/api-37-emulator-crash.md @@ -492,6 +492,18 @@ input entirely, so the picker's process can be removed from a task no key press `pickTheFixture`'s whole-picker retry — which exists for exactly this — becomes reachable again. That is a fix to the test on every level, not to API 37. +**It was made to bite before it was believed.** On a local API 36 emulator, with the walk cut short +so the picker is left open and in front and with `device.pressBack()` removed, so that nothing but +the force-stop can close it: + +| | result | +|---|---| +| with `forceStopThePicker()` | **passes** — `ActivityManager: Force stopping com.google.android.documentsui ... from pid 5334`, `Killing 5269:com.google.android.documentsui (adj 0)`, a second `PickActivity` opens, the retry completes the pick | +| with the one call removed | **fails** — `the system picker would not close: after 4 back presses ... com.google.android.documentsui is in front`, which is the API 37 failure verbatim | + +The unmutated class passes on that emulator either way, which is the point of running the mutation +at all: the recovery path is unreachable on a healthy device, so a green suite says nothing about it. + #### The correction that produced that table **The first version of this section said both tests failed, and put the marker on the class.** The From 745c4f62cefcaaa7b56b592aa819710d8e3b593e Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Sat, 5 Sep 2026 22:58:10 -0500 Subject: [PATCH 4/6] The local runner had the same missing root, and hid it in /dev/null Third copy of the same defect. tools/local-emulator/run-e2e.sh sends `adb shell stop` and `start` to /dev/null, so its `Must be root` was never printed and the framework restart it credits has never happened either. That matters for what docs/api-37-emulator-crash.md's abort numbers are evidence of, so the caveat goes next to them rather than in a commit message: on API 37 the image restarts its own framework every minute or so, and a restart landing after a successful `pm disable-user` brings back a SystemUI-less zygote on its own. That produces the recorded rate collapse by accident, and it is why the same code bought nothing on CI's much quieter swiftshader legs, where the logcat shows SystemUI alive for the whole run. Also verified, because the previous commit asserted it: the advisory leg really does run thePickedInputSurvivesARealRotation before the picker test -- run 34008889182 logs the four in the order Media3, Media3, rotation, picker. Co-Authored-By: Claude Opus 5 (1M context) --- docs/api-37-emulator-crash.md | 14 +++++++++++++- tools/local-emulator/run-e2e.sh | 22 ++++++++++++++++++++-- 2 files changed, 33 insertions(+), 3 deletions(-) diff --git a/docs/api-37-emulator-crash.md b/docs/api-37-emulator-crash.md index 911043a..7ea7e07 100644 --- a/docs/api-37-emulator-crash.md +++ b/docs/api-37-emulator-crash.md @@ -315,7 +315,19 @@ clean zero. Its own post-disable check on the run recorded below printed So what is reliably achieved is a **rate collapse** — from roughly one abort every fourteen seconds to one every forty-five — which a 47-second Gradle run survives and a five-minute one -might not. The 180-second zero above is one measurement on a device that had been up for twelve +might not. + +**Read every number in this section knowing that the restart it credits was not happening.** +Found 2026-09-05: `stop` and `start` are root-only, adbd was not root in any of the three copies +of this logic, and `run-e2e.sh` sent both to `/dev/null`, so its `Must be root` was never even +printed. What the local runs almost certainly had instead is the image restarting its own +framework — on API 37 that happens every minute or so — and a restart landing after a successful +`pm disable-user` brings back a SystemUI-less zygote by itself. That would produce exactly the +rate collapse recorded above, by accident and on the image's schedule rather than the harness's, +which also explains why the same code bought nothing at all on CI's far quieter +`swiftshader_indirect` legs: there the logcat shows SystemUI alive for the whole run. The +measurements above are still what was observed; what they are evidence *of* is narrower than it +was written to be. All three copies now take root. The 180-second zero above is one measurement on a device that had been up for twelve minutes and had already cycled its framework several times. The harness prints the quiet-check delta on every run precisely so this is visible rather than assumed. diff --git a/tools/local-emulator/run-e2e.sh b/tools/local-emulator/run-e2e.sh index e09b8d5..4afd1f9 100755 --- a/tools/local-emulator/run-e2e.sh +++ b/tools/local-emulator/run-e2e.sh @@ -384,8 +384,26 @@ disable_region_sampling() { fi echo " restarting the framework so the region-sampling listener goes with it" - emu_adb shell stop > /dev/null 2>&1 - emu_adb shell start > /dev/null 2>&1 + # `adb root` first, and the two redirects below used to hide why. `stop` and `start` are + # root-only, adbd is not root on a booted emulator, and both were answering `Must be root` + # into /dev/null -- so this restarted nothing, here and in both CI copies, for as long as + # any of them has existed (2026-09-05). Measured on a local android-37.0 AVD: `stop` alone + # says `Must be root`; after `adb root`, `whoami` says root, `stop` returns 0 and + # `pidof system_server` comes back empty. + # + # Read the abort table in docs/api-37-emulator-crash.md with that in mind: on API 37 the + # image restarts its own framework every minute or so, and a restart AFTER a successful + # `pm disable-user` brings back a SystemUI-less zygote by itself. That is the likeliest + # reason the disable appeared to work here while doing nothing on CI's much quieter + # swiftshader legs, where the logcat shows SystemUI alive for the whole run. + # + # Output is kept rather than discarded now, for the same reason. + emu_adb root > /dev/null 2>&1 + emu_adb wait-for-device + emu_adb shell stop 2>&1 | sed 's/^/ stop: /' + emu_adb shell start 2>&1 | sed 's/^/ start: /' + emu_adb unroot > /dev/null 2>&1 + emu_adb wait-for-device # There is no property worth waiting on here, and an earlier version of this only looked # like it was waiting on one: `stop` does not clear sys.boot_completed, so it still reads # `1` throughout the restart and any loop over it returns at once. The loop below is the From 97558c259f82a9b47fa4eec397e777f49421d1e0 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Sat, 5 Sep 2026 23:15:57 -0500 Subject: [PATCH 5/6] The root fix was wrong, and this is what it found: the disable does nothing Three commits back I gave `disable_region_sampling` the `adb root` it needed, on the strength of `Must be root` appearing in every API 37 leg's log. That part was right and the conclusion drawn from it was not. api37-debug run 34010167885, with the restart finally real: pm attempt 1: Package com.android.systemui new state: disabled-user restarting the framework adbd is running as root system_server down after 2 s NOT DISABLED after the restart -- the package state did not survive three rounds of it, `final state: SystemUI STILL ENABLED`, and the leg reported `expected: 0, received: 0`. Making the restart work cost the leg every test it had. Bisected locally on android-37.0: a `stop` 2 s after `pm disable-user` kills system_server before PackageManager flushes its delayed write, and a 15 s pause makes the state survive. That repairs the wrong thing. With the package verified disabled before AND after a clean restart, `com.android.systemui` comes up 3 s after `system_server` regardless -- and CI's own logcat says the same with no restart at all: run 34006456986 verifies the package disabled at 02:29:33 and has SystemUI pid 4275 alive from 02:28:52 for the whole run. So `pm disable-user` does not stop SystemUI starting on this image, with or without a restart, and the restart is removed from all three copies rather than repaired. What is kept is the 45-second window with no new aborts, which is what was always doing the work: the boot aborts land at 02:28:18 and 02:28:43 and the wait is what puts instrumentation at 02:32:42, after them rather than inside one. `pm disable-user` is kept too, because every green leg and every number quoted about this row was measured with it applied. The prose the earlier commits got wrong is corrected in place, and one of the corrections is good news: status_check.yml's caveat that this row runs a configuration no other leg or Pixel run uses, so nothing depending on system UI may trust it, describes a state that has never existed. The row is more comparable to API 33-36 than it has been claiming, not less. Co-Authored-By: Claude Opus 5 (1M context) --- .github/scripts/e2e-run.sh | 160 ++++++++++++----------------- .github/workflows/api37-debug.yml | 76 ++++---------- .github/workflows/status_check.yml | 46 ++++----- CLAUDE.md | 11 ++ docs/api-37-emulator-crash.md | 128 ++++++++++++----------- tools/local-emulator/run-e2e.sh | 46 +++------ 6 files changed, 203 insertions(+), 264 deletions(-) diff --git a/.github/scripts/e2e-run.sh b/.github/scripts/e2e-run.sh index 1b3e89e..d6ea1d2 100755 --- a/.github/scripts/e2e-run.sh +++ b/.github/scripts/e2e-run.sh @@ -52,58 +52,72 @@ WEDGE_TIMEOUT=1200 # the same shape as E2E_EXTRA_GRADLE_ARGS below. The other four E2E legs run byte-identical # commands with it unset. # -# WHY IT RUNS HERE, BEFORE THE LOGCAT STREAM: `adb shell stop` ends the `adb logcat` started -# below, and nothing restarts it, so a disable performed after that point would cost this leg -# its whole diagnostic story for the part of the run that matters. Everything this function -# counts comes from `adb logcat -d -b crash`, which is a fresh read each time and independent -# of the stream. +# WHY IT RUNS HERE, BEFORE THE LOGCAT STREAM: it is a 45-second wait, and the stream below is +# meant to cover the suite rather than the wait. Everything this function counts comes from +# `adb logcat -d -b crash`, a fresh read each time and independent of the stream. (The original +# reason was stronger and no longer applies: `adb shell stop` would have ended the streamed +# `adb logcat` and nothing restarts it. There is no `stop` here any more -- see below.) # -# WHAT IT IS FOR: the android-37.x images abort surfaceflinger from RegionSamplingThread inside -# their own gralloc mapper (docs/api-37-emulator-crash.md). surfaceflinger is a critical service, -# so init SIGKILLs zygote with it and the framework restarts under the run -- Gradle then reports +# WHAT IT IS FOR -- AND THE NAME IS NOW WRONG, WHICH IS WHY THIS PARAGRAPH IS LONG. +# The android-37.x images abort surfaceflinger from RegionSamplingThread inside their own gralloc +# mapper (docs/api-37-emulator-crash.md). surfaceflinger is a critical service, so init SIGKILLs +# zygote with it and the framework restarts under the run -- Gradle then reports # `cmd: Can't find service: package` and `Starting 0 tests`. RegionSamplingThread exists only -# because SystemUI registers a nav-bar luma-sampling listener, so removing the package removes -# the whole chain. Measured cadence of those kills: 20-90 s apart, median 60-70 s, three to five -# in a four-minute window -- fast enough that install and instrumentation start-up do not fit -# inside one gap. +# because SystemUI registers a nav-bar luma-sampling listener, so this was written to remove the +# package and with it the whole chain. Measured cadence of those kills on `-gpu host`: 20-90 s +# apart, median 60-70 s, three to five in a four-minute window. +# +# **THE DISABLE HALF OF THAT HAS NEVER WORKED, AND THE QUIET WINDOW IS WHAT THE LEG ACTUALLY +# GETS.** Measured 2026-09-05, two ways that agree: +# +# - On CI, in the gating leg of run 34006456986: `pm disable-user` is accepted at 02:28:37.9 and +# `com.android.systemui` really is in `pm list packages -d` at 02:29:33 -- and SystemUI is +# started anyway at 02:28:39.5 and again at 02:28:52.3, the second of which (pid 4275) is +# alive for the whole instrumentation run, logging `WindowManagerShell ... +# app=com.android.systemui` minutes after this function prints its final line. +# - Locally on android-37.0, with the package verified disabled before AND after a deliberate +# `stop; start`: `com.android.systemui` comes up 3 s after `system_server` regardless. +# +# So `pm disable-user --user 0 com.android.systemui` does not stop SystemUI starting on this +# image, whatever else happens. The name `E2E_DISABLE_SYSTEM_UI` and the name of this function are +# kept because the matrix row, both workflows and two documents refer to them, and a rename would +# touch all of that to no benefit -- read this comment, not the name. +# +# WHAT IS LEFT IS LOAD-BEARING, so do not delete the function as dead weight. It is the 45-second +# window with zero new `hasReadColorBufferDma` aborts. The boot-time aborts land close together -- +# 02:28:18 and 02:28:43 in that same run -- and the wait is what puts instrumentation (02:32:42) +# after them rather than inside one. That is what stops a leg reporting `Starting 0 tests`, and it +# is why the three-round retry stays. +# +# THE `pm disable-user` CALL STAYS TOO, for a narrower reason than it was written for: every green +# leg and every measurement quoted anywhere about this row was taken with it applied and SystemUI +# running. Removing it would change the configuration the numbers came from, which is not a change +# to make while fixing a flake. +# +# AND THE FRAMEWORK RESTART IS GONE, having been measured to be worse than nothing. It was written +# as `adb shell stop; adb shell start`, which are root-only; adbd is not root, so every leg printed +# `Must be root` twice and restarted nothing. Adding `adb root` made it real, and api37-debug run +# 34010167885 is what that looks like: `pm disable-user` reports success, the stop lands ~2 s later +# and kills system_server before PackageManager has flushed its delayed write of package +# restrictions, so the state is gone on the way back up -- `NOT DISABLED after the restart`, three +# rounds, `final state: SystemUI STILL ENABLED`, and the leg then reported `expected: 0, +# received: 0`. A 15 s pause before the stop does make the state survive (bisected locally), and it +# still does not help, because of the two measurements above. So the restart is removed rather than +# repaired: it cost the leg every test it had, and there is nothing for it to buy. # # NOTHING HERE TRUSTS A COMMAND'S OWN REPORT, and that is not paranoia: of four runs of an # earlier one-shot version, one (32646029143) reported `new state: disabled-user` and then -# started SystemUI eight more times, with ten more aborts. `pm disable-user` can be accepted by -# a system_server that is SIGKILLed before the state is written, and `pm disable-user` does not -# retract SystemUI's existing region-sampling registration either -- by the time boot completes -# it has already registered, so only a framework restart brings back a SystemUI-less -# surfaceflinger. Hence: disable, take the framework DOWN and confirm system_server is really -# gone (an earlier probe asked `service check` 0.3 s after `stop` and got `found` from the -# system_server that was still exiting, so its wait was not a wait), bring it back, verify the -# package against `pm list packages -d`, and require a 45 s window with zero new aborts. -# Three rounds, because one is not reliable and the failure is silent. -# -# THE RESTART NEEDED ROOT, AND DID NOT HAVE IT UNTIL 2026-09-05. `adb shell stop` and -# `adb shell start` are root-only, so every API 37 leg ever run printed `Must be root` twice and -# restarted nothing -- green legs and red ones alike. SystemUI therefore stayed up for the whole -# run, which the logcat shows directly (`WindowManagerShell ... app=com.android.systemui`, from a -# live SystemUI pid, minutes after the "final state: SystemUI disabled" line). The paragraph above -# says why the `pm disable-user` on its own buys nothing: it does not retract the registration. -# -# The images are userdebug -- `google/sdk_gphone64_x86_64/emu64xa:17/...:userdebug/dev-keys` -- so -# `adb root` is available and was the only thing missing. Measured on the local android-37.0 AVD, -# same image and fingerprint as CI: `stop` -> `Must be root`; `adb root` -> `whoami` says `root`; -# `stop` -> exit 0 and `pidof system_server` empty; `start` -> exit 0. -# -# Root is dropped again before the suite runs. Everything Gradle does afterwards -- install, -# instrument, uninstall -- has to be what the other four legs do, and `adb root` changes the uid -# every later `adb shell` runs as. Both calls restart adbd, hence `wait-for-device` after each. +# started SystemUI eight more times. So this reports what `pm list packages -d` says AND what +# `pidof` says, side by side, rather than one line implying both. # --------------------------------------------------------------------------- count_aborts() { adb logcat -d -b crash 2> /dev/null | grep -c 'hasReadColorBufferDma'; } systemui_disabled() { adb shell pm list packages -d 2> /dev/null | grep -q 'com.android.systemui'; } -adb_as_root() { adb root > /dev/null 2>&1; adb wait-for-device; } -adb_as_shell() { adb unroot > /dev/null 2>&1; adb wait-for-device; } +systemui_pid() { adb shell pidof com.android.systemui 2> /dev/null | tr -d '\r\n'; } disable_region_sampling() { - local round=1 i out down back before after + local round=1 i out pid before after while [ "$round" -le 3 ]; do - echo "--- SystemUI disable, round $round ---" + echo "--- round $round ---" for i in $(seq 1 10); do out="$(adb shell pm disable-user --user 0 com.android.systemui 2>&1 | tr -d '\r')" echo " pm attempt $i: $out" @@ -111,62 +125,20 @@ disable_region_sampling() { sleep 5 done - echo " restarting the framework" - adb_as_root - echo " adbd is running as $(adb shell whoami 2>&1 | tr -d '\r')" - out="$(adb shell stop 2>&1 | tr -d '\r')" - [ -n "$out" ] && echo " stop said: $out" - # `down` rather than reading `i` afterwards: the loop leaves `i` at 20 whether it broke on the - # process being gone or simply ran out, and the old version printed that as "system_server down - # after ~40 s" for a stop that had done nothing at all. A wait that did not observe the thing - # it was waiting for has to say so. - down=no - for i in $(seq 1 20); do - if [ -z "$(adb shell pidof system_server 2> /dev/null | tr -d '\r\n')" ]; then - down=yes - break - fi - sleep 2 - done - if [ "$down" = "yes" ]; then - echo " system_server down after ~$((i * 2)) s" - else - echo " system_server STILL RUNNING after ~$((i * 2)) s -- the stop did not take" - fi - out="$(adb shell start 2>&1 | tr -d '\r')" - [ -n "$out" ] && echo " start said: $out" - adb_as_shell - # Same flag, same reason as `down` above: this loop also used to report its own exhaustion as - # an elapsed time, so "services back after ~150 s" and "services never came back" printed the - # same line. - back=no - for i in $(seq 1 30); do - if adb shell service check package 2> /dev/null | grep -q ': found' \ - && adb shell service check activity 2> /dev/null | grep -q ': found' \ - && [ -n "$(adb shell pidof system_server 2> /dev/null | tr -d '\r\n')" ]; then - back=yes - break - fi - sleep 5 - done - if [ "$back" = "yes" ]; then - echo " services back after ~$((i * 5)) s" - else - echo " services NOT back after ~$((i * 5)) s -- package, activity or system_server missing" - fi - if systemui_disabled; then - echo " verified: com.android.systemui is in pm list packages -d" + echo " pm list packages -d: com.android.systemui is in it" else - echo " NOT DISABLED after the restart -- the package state did not survive" - round=$((round + 1)) - continue + echo " pm list packages -d: com.android.systemui is NOT in it" fi + # Printed next to the line above precisely because the two disagree on this image, and a + # reader who sees only the first will believe something that is not true. + pid="$(systemui_pid)" + echo " com.android.systemui pid: ${pid:-none} (expected: a pid -- see the header)" before="$(count_aborts)" sleep 45 after="$(count_aborts)" - echo " abort rate, SystemUI disabled: $((after - before)) new in 45 s (total ${after:-0})" + echo " aborts: $((after - before)) new in 45 s (total ${after:-0})" [ "$((after - before))" -eq 0 ] && break echo " still aborting after round $round" round=$((round + 1)) @@ -175,16 +147,16 @@ disable_region_sampling() { # A warning rather than an exit. If the disable did not take, the run is about to report # `Starting 0 tests` and fail on its own -- and it will do so with the logcat, the crash # buffer and the diagnostics attached, which is more useful than dying here with none of it. - if systemui_disabled; then - echo " final state: SystemUI disabled" + if [ "$((after - before))" -eq 0 ]; then + echo " final state: 45 s with no new aborts -- the suite starts here" else - echo "::warning::E2E api${LABEL}: SystemUI is still enabled -- expect INSTRUMENTATION_ABORTED" + echo "::warning::E2E api${LABEL}: still aborting after three rounds -- expect INSTRUMENTATION_ABORTED" fi return 0 } if [ "${E2E_DISABLE_SYSTEM_UI:-}" = "1" ]; then - echo "::group::E2E api${LABEL} -- removing the region-sampling listener" + echo "::group::E2E api${LABEL} -- waiting out the boot-time gralloc aborts" disable_region_sampling echo "::endgroup::" fi diff --git a/.github/workflows/api37-debug.yml b/.github/workflows/api37-debug.yml index b9cbe66..533648b 100644 --- a/.github/workflows/api37-debug.yml +++ b/.github/workflows/api37-debug.yml @@ -32,9 +32,11 @@ name: API 37 debug # The SystemUI disable below is the exception, and it is a real one: this workflow # drives it from its own probe step so `disable_system_ui` can be turned off for a # dispatch, where the real leg gets it through `E2E_DISABLE_SYSTEM_UI`. Two copies of -# that logic therefore exist, and on 2026-09-05 both carried the same defect -- no -# `adb root`, so `adb shell stop` answered `Must be root` and nothing restarted. Fix -# one and the other needs the same change in the same diff. +# that logic therefore exist and must be changed together. **This instrument is also +# what established that the disable half of it does nothing** -- run 34010167885, in +# which making its framework restart real cost the leg every test it had. Read +# .github/scripts/e2e-run.sh's header for the measurements; the restart is gone from +# both copies and what remains is the 45-second quiet window. # - It does not change status_check.yml. If a configuration here turns out to work, # the change to the real matrix is proposed separately. # @@ -298,72 +300,30 @@ jobs: sleep 5 done - # pm disable-user does not retract SystemUI's existing region-sampling - # registration -- by the time boot completes it has already registered. Only a - # framework restart brings back a SystemUI-less SurfaceFlinger. See - # disable_region_sampling in .github/scripts/e2e-run.sh, which this mirrors. - # - # `adb root` first, because `stop` and `start` are root-only and adbd is not root - # on a freshly booted emulator: without it both printed `Must be root` and the - # restart never happened, on this workflow and on the real leg alike, for as long - # as either has existed. Root is dropped again before the suite runs so Gradle's - # install and instrument happen as an unprivileged adb does them. Both flags below - # exist because each loop used to print its own exhaustion as an elapsed time -- - # "system_server down after 40 s" was what a stop that did nothing looked like. - echo " restarting the framework" - adb root > /dev/null 2>&1; adb wait-for-device - echo " adbd is running as $(adb shell whoami 2>&1 | tr -d '\r')" - out="$(adb shell stop 2>&1 | tr -d '\r')" - [ -n "$out" ] && echo " stop said: $out" - down=no - for i in $(seq 1 20); do - if [ -z "$(adb shell pidof system_server 2> /dev/null | tr -d '\r\n')" ]; then - down=yes - break - fi - sleep 2 - done - if [ "$down" = "yes" ]; then - echo " system_server down after $((i * 2)) s" - else - echo " system_server STILL RUNNING after $((i * 2)) s -- the stop did not take" - fi - out="$(adb shell start 2>&1 | tr -d '\r')" - [ -n "$out" ] && echo " start said: $out" - adb unroot > /dev/null 2>&1; adb wait-for-device - back=no - for i in $(seq 1 30); do - if adb shell service check package 2> /dev/null | grep -q ': found' \ - && adb shell service check activity 2> /dev/null | grep -q ': found' \ - && [ -n "$(adb shell pidof system_server 2> /dev/null | tr -d '\r\n')" ]; then - back=yes - break - fi - sleep 5 - done - if [ "$back" = "yes" ]; then - echo " services back after $((i * 5)) s" - else - echo " services NOT back after $((i * 5)) s" - fi - + # NO FRAMEWORK RESTART. There was one here, and making it work (it needed + # `adb root`) is what proved the whole disable is ineffective on this image: + # SystemUI starts anyway, measured on CI and locally, and the restart itself + # loses the package state to PackageManager's delayed write and leaves the leg + # reporting `Starting 0 tests`. e2e-run.sh's header carries the measurements. + # What is left, and what is load-bearing, is the quiet window below. if systemui_disabled; then - echo " verified: com.android.systemui is in pm list packages -d" + echo " pm list packages -d: com.android.systemui is in it" else - echo " NOT DISABLED after the restart -- the package state did not survive" - round=$((round + 1)) - continue + echo " pm list packages -d: com.android.systemui is NOT in it" fi + # Beside it, because the two disagree on this image and the first line alone + # reads as a claim about the process that is not true. + echo " com.android.systemui pid: $(adb shell pidof com.android.systemui 2> /dev/null | tr -d '\r\n')" before="$(count_aborts)" sleep 45 after="$(count_aborts)" - echo "--- abort rate, SystemUI disabled: $((after - before)) new in 45 s (total ${after:-0}) ---" + echo "--- aborts: $((after - before)) new in 45 s (total ${after:-0}) ---" [ "$((after - before))" -eq 0 ] && break echo " still aborting after round $round" round=$((round + 1)) done - systemui_disabled && echo "final state: SystemUI disabled" || echo "final state: SystemUI STILL ENABLED -- expect Starting 0 tests" + systemui_disabled && echo "final state: com.android.systemui is disabled in pm (it still runs)" || echo "final state: com.android.systemui is not even disabled in pm" fi echo "--- crash buffer (tail 60) ---" diff --git a/.github/workflows/status_check.yml b/.github/workflows/status_check.yml index 7745e24..db71918 100644 --- a/.github/workflows/status_check.yml +++ b/.github/workflows/status_check.yml @@ -254,31 +254,31 @@ jobs: api-level: "36" # API 37, and it is NOT the same device as the four rows above it. # - # CAVEAT, read this before trusting a green here: this leg runs with - # SystemUI disabled and the framework restarted under it. No other leg - # and no Pixel run uses that configuration. It is defensible only because - # nothing THIS LEG RUNS touches system UI -- Media3, FFmpeg and - # WorkManager tests -- and because the alternative is no CI coverage of - # the level this app targets. **Anything that ever does depend on system - # UI must not trust this row.** E2E_DISABLE_SYSTEM_UI is what does it; - # .github/scripts/e2e-run.sh explains the mechanism and why every step of - # it is verified rather than assumed. + # THE CAVEAT THAT USED TO BE HERE IS WITHDRAWN, 2026-09-05, and the + # withdrawal is good news. It said this leg "runs with SystemUI disabled + # and the framework restarted under it", that no other leg or Pixel run + # uses that configuration, and that anything depending on system UI must + # not trust this row. **None of that was ever true.** Measured: the + # framework restart is two root-only adb commands that answered `Must be + # root` on every leg ever run, and `pm disable-user` does not stop SystemUI + # starting on this image anyway -- in run 34006456986 the package is + # verified disabled at 02:29:33 and SystemUI (pid 4275) is up from 02:28:52 + # for the whole run. So this row's device configuration is the same as the + # other four's, and a green here means what a green on 33-36 means. # - # "this leg" and not "this suite", since 2026-08-24, and the difference is - # now load-bearing: SafPickerRoundTripTest DOES touch system UI. It drives - # DocumentsUI and rotates the display, and both reach the gralloc mapper - # this image aborts in -- disabling SystemUI removes the IDLE trigger, not - # those. Measured per method on android-37.0: the ROTATION test takes the - # framework down (INSTRUMENTATION_ABORTED) and carries - # @FailsOnEmulatorApi37, so notAnnotation below keeps it off this row; the - # PICKER test passes and runs here like anything else. A rotation rebuilds - # every surface at once, and starting another app's activity does not. + # E2E_DISABLE_SYSTEM_UI still exists and still runs, because what it + # actually buys is a 45-second window with no new gralloc aborts before the + # suite starts -- the boot-time ones land close together and instrumentation + # has to begin after them, not between them. The name is stale and kept: + # read .github/scripts/e2e-run.sh's header, which carries the measurements. # - # So this row does now run one test that depends on system UI, and the - # caveat above still applies to it: a green here is not evidence the picker - # works on a device with SystemUI running -- the Pixel release check is. - # docs/api-37-emulator-crash.md has the per-method measurements, and the - # correction that produced them. + # notAnnotation below keeps four tests off this row, not two, and one of + # them is new. SafPickerRoundTripTest's PICKER test was measured on + # 2026-08-24 as passing here and was left on the leg; four gating logcats + # read on 2026-09-05 show it aborting system_server from the task-snapshot + # path on every single run, pass or fail, which is what had been failing + # unrelated PRs (#108). Both of that class's tests now carry the marker. + # docs/api-37-emulator-crash.md has the timings and the correction. # # api-level must be a POINT release. A bare 37 is not an SDK package and # fails during setup, which cost a run to discover. `37.0` is the choice diff --git a/CLAUDE.md b/CLAUDE.md index c5f7bd7..75ef00e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -93,6 +93,17 @@ days. Read it as the current answer, and see the git history if you need the old #108, it cost roughly a third of the gating legs over the wave-4 landings (#190), and a marker is what it needed. `docs/api-37-emulator-crash.md` has the timings. + **A second thing came out of those logcats, and it withdraws a caveat rather than adding one.** + The API 37 row was documented as the one leg running "with SystemUI disabled and the framework + restarted under it", which nothing else does. Neither half was ever happening: `adb shell stop` + and `start` are root-only and answered `Must be root` on every leg ever run, and `pm + disable-user` does not stop SystemUI starting on this image anyway — measured on CI and locally, + with and without a real restart. **So this row's device configuration is the same as the other + four's, and a green here means what a green at 33–36 means.** `E2E_DISABLE_SYSTEM_UI` is kept + under its now-stale name because what it really buys is a 45-second window with no new gralloc + aborts before the suite starts, which is load-bearing; `.github/scripts/e2e-run.sh`'s header is + where that is written down. + That job is still called `E2E API 37 Media3 hardware transcode (advisory)`, which no longer describes everything in it. The name is kept deliberately — it is not a required context and people have learned to look for it — so **read the marker, not the name**, for what it holds. diff --git a/docs/api-37-emulator-crash.md b/docs/api-37-emulator-crash.md index 7ea7e07..4b8cc0d 100644 --- a/docs/api-37-emulator-crash.md +++ b/docs/api-37-emulator-crash.md @@ -317,77 +317,88 @@ So what is reliably achieved is a **rate collapse** — from roughly one abort e seconds to one every forty-five — which a 47-second Gradle run survives and a five-minute one might not. -**Read every number in this section knowing that the restart it credits was not happening.** -Found 2026-09-05: `stop` and `start` are root-only, adbd was not root in any of the three copies -of this logic, and `run-e2e.sh` sent both to `/dev/null`, so its `Must be root` was never even -printed. What the local runs almost certainly had instead is the image restarting its own -framework — on API 37 that happens every minute or so — and a restart landing after a successful -`pm disable-user` brings back a SystemUI-less zygote by itself. That would produce exactly the -rate collapse recorded above, by accident and on the image's schedule rather than the harness's, -which also explains why the same code bought nothing at all on CI's far quieter -`swiftshader_indirect` legs: there the logcat shows SystemUI alive for the whole run. The -measurements above are still what was observed; what they are evidence *of* is narrower than it -was written to be. All three copies now take root. The 180-second zero above is one measurement on a device that had been up for twelve -minutes and had already cycled its framework several times. The harness prints the quiet-check -delta on every run precisely so this is visible rather than assumed. +**And that restart has never happened — which is how the disable turned out not to work either.** +Corrected 2026-09-05; this replaces the two paragraphs above rather than qualifying them. -One ordering detail cost a whole run and is now encoded in `disable_region_sampling`: by the time -`sys.boot_completed` flips, SystemUI has **already registered**, and `pm disable-user` does not -retract an existing registration — it only stops the package being started again. Disabling it -and proceeding straight to the tests fails exactly as before. The harness therefore does -`stop; start` afterwards, so the framework that comes back never starts SystemUI at all. +`adb shell stop` and `start` are root-only, adbd is not root on a booted emulator, and all three +copies of this logic called them without `adb root`. On CI both printed `Must be root`, between +lines that read as if the restart had happened; `run-e2e.sh` sent them to `/dev/null`, so its +`Must be root` was never even visible. Neither number in those logs was an observation either — +the `pidof` loop breaks when the process is gone and otherwise falls out at its last iteration, +and the old code printed the iteration count either way, so `system_server down after ~40 s` is +what a stop that did nothing looks like. -**And on CI that `stop; start` did nothing at all until 2026-09-05.** Both are root-only commands, -adbd on a freshly booted emulator is not root, and the step log had been saying so on every API 37 -leg since the function was written — `Must be root`, twice, between lines that read as if the -restart had happened: +Adding `adb root` made the restart real, and **that is what proved the disable ineffective**. +`api37-debug` run 34010167885, `disable_system_ui=true`: ``` +--- disable round 1 --- + pm attempt 1: Package com.android.systemui new state: disabled-user restarting the framework -Must be root - system_server down after ~40 s -Must be root - services back after ~5 s - verified: com.android.systemui is in pm list packages -d + adbd is running as root + system_server down after 2 s + services back after 10 s + NOT DISABLED after the restart -- the package state did not survive ``` -Neither number was an observation. The `pidof` loop breaks when the process is gone and otherwise -falls out at its last iteration, and the old code printed the iteration count either way — so -"down after ~40 s" is what a stop that did nothing looks like. The paragraph above is what makes -this matter rather than merely untidy: without the restart the disable buys nothing, and the -logcat confirms it directly — SystemUI is alive for the whole run, logging -`WindowManagerShell ... app=com.android.systemui` minutes after `final state: SystemUI disabled`. +Three rounds of that, then `final state: SystemUI STILL ENABLED`, and the leg reported +`expected: 0, received: 0` — `Starting 0 tests`, the exact failure this function exists to +prevent. -The images are userdebug, so `adb root` is all that was missing. Measured on the local -`android-37.0` AVD, same fingerprint as CI -(`google/sdk_gphone64_x86_64/emu64xa:17/CE2A.260420.019/15611780:userdebug/dev-keys`): +Bisected locally on `android-37.0`, which explains the lost state and nothing else: + +| arm | sequence | disabled after the restart? | +|---|---|---| +| A | `pm disable-user`, then `stop` at once | **no** | +| B | `pm disable-user`, wait 15 s, then `stop` | **yes** | + +That is PackageManager's delayed write of package restrictions: the stop kills `system_server` +before the settings are flushed, and arm A is what CI did. **Arm B does not help either**, which +is the measurement that matters. With the package verified `disabled-user` before *and* after a +further clean restart: ``` -adb shell stop -> Must be root -adb root -> restarting adbd as root -adb shell whoami -> root -adb shell stop -> exit 0; adb shell pidof system_server -> (empty) -adb shell start -> exit 0 + package still disabled? YES + processes: + 9275 00:17 system_server + 9695 00:14 com.android.systemui <- started 3 s after system_server ``` -`disable_region_sampling` now takes root for the restart and drops it again with `adb unroot` -before Gradle runs, so install, instrument and uninstall happen as the other four legs do it. Both -waits report whether they observed what they were waiting for instead of printing their own -exhaustion as an elapsed time. +CI's own logcat says the same without any restart at all. In the gating leg of run 34006456986, +`pm disable-user` is accepted at 02:28:37.9 and the package really is in `pm list packages -d` at +02:29:33 — and SystemUI is started at 02:28:39.5 and again at 02:28:52.3, the second of which +(pid 4275) is alive for the whole instrumentation run. -**It does not touch the picker test's abort**, and it was never going to: that one is -WindowManager inside `system_server`, not SystemUI. What it fixes is the *idle* trigger this -section is about, which had been left running on every leg. +**So `pm disable-user --user 0 com.android.systemui` does not stop SystemUI starting on this +image**, with or without a framework restart, on CI or locally. The premise this section was +built on — "the framework that comes back never starts SystemUI at all" — is false. + +Two things follow, pointing in opposite directions. + +- **The restart is removed rather than repaired**, in all three copies. It cost a leg every test + it had and there is nothing for it to buy. What is kept is the 45-second window with zero new + aborts, which was always the part doing the work: in that same run the boot aborts land at + 02:28:18 and 02:28:43, and the wait is what puts instrumentation at 02:32:42 — after them + rather than inside one. The `pm disable-user` call is kept too, for a narrower reason than it + was written for: every green leg and every number quoted about this row was measured with it + applied, and changing the configuration while fixing a flake is not a trade worth making. +- **The rate collapse recorded above is not evidence of what it says.** Both arms of that + comparison had SystemUI running. What it measured is a device twelve minutes into its uptime + against one that had just booted — a real difference, and a different claim. The quiet gate is + still worth having on exactly that reading. ### The two deviations, stated plainly 1. **The renderer is ANGLE, not the host GPU.** Shared with nothing else in the matrix — API 33–36 run `-gpu host` locally, and CI runs `swiftshader_indirect`. -2. **SystemUI is disabled.** The API 37 leg does not run the same device configuration as any - other leg or as the Pixel. It was defensible here because nothing in this suite touched - system UI — Media3, FFmpeg and WorkManager tests — and because the alternative is no local - API 37 coverage at all. **Anything that ever does depend on system UI must not trust this - leg.** Something now does; see the section below. +2. **SystemUI is asked to be disabled, and runs anyway.** This was written as the deviation that + mattered — "anything that ever does depend on system UI must not trust this leg" — and the + measurements above say the deviation does not exist: the package is marked `disabled-user` and + `com.android.systemui` is up for the whole leg regardless. **The correction is good news + rather than bad.** This row is *more* comparable to API 33–36 and to the Pixel than it has + been claiming, not less, and the test that depends on system UI (see the section below) was + never running in the exotic configuration this bullet describes. What `pm disable-user` leaves + behind is a package-manager flag nothing acts on. ### Something does depend on system UI now, and half of it is excluded @@ -395,12 +406,13 @@ Added 2026-08-24, and the first entry on this page that is not a codec. `SafPickerRoundTripTest` drives the real system file picker and rotates the display. Both reach the gralloc mapper — DocumentsUI is another app's windows, and a rotation rebuilds every surface -on screen — and **disabling SystemUI does not help**, because it removes the *idle* trigger -(RegionSamplingThread's nav-bar luma sampling) and not this one. +on screen — and **disabling SystemUI does not help**. Two reasons now, and only the first was +known when this was written: it removes the *idle* trigger (RegionSamplingThread's nav-bar luma +sampling) and not this one, and — see the section above — it does not remove SystemUI either. -Measured one method per fresh emulator, `android-37.0`, `swangle_indirect`, SystemUI disabled and -verified quiet — separately, because inferring the second from the first is the mistake this -page's opening correction is about: +Measured one method per fresh emulator, `android-37.0`, `swangle_indirect`, with the disable +applied and verified quiet — separately, because inferring the second from the first is the +mistake this page's opening correction is about: | test | result on android-37.0 | `hasReadColorBufferDma` aborts in the window | |---|---|---| diff --git a/tools/local-emulator/run-e2e.sh b/tools/local-emulator/run-e2e.sh index 4afd1f9..04a2424 100755 --- a/tools/local-emulator/run-e2e.sh +++ b/tools/local-emulator/run-e2e.sh @@ -355,12 +355,10 @@ boot_emulator() { # may be in one of its restarts and `pm` is simply not published yet. The first attempt at this # failed exactly that way, with `cmd: Can't find service: package`. # -# The framework restart at the end is not optional, and finding that out cost a run. By the -# time `sys.boot_completed` flips, SystemUI has already registered its region-sampling listener, -# and `pm disable-user` does not retract a registration that already happened -- it only stops -# the package being started again. So the first attempt disabled SystemUI, reported success, and -# then died exactly as before with `Starting 0 tests` and four more aborts. `stop; start` cycles -# zygote deliberately, and the framework that comes back up does not start SystemUI at all. +# This used to end with a framework restart, described here as "not optional". It was neither +# optional nor happening -- see the block inside the function. What the first attempt's +# `Starting 0 tests` and four more aborts actually showed is that a `pm disable-user` on its own +# buys nothing, which is still true; what was wrong is the conclusion that a restart would. disable_region_sampling() { local api="$1" out i before after ready case "$api" in 37 | 37.*) ;; *) return 0 ;; esac @@ -383,32 +381,18 @@ disable_region_sampling() { return 0 fi - echo " restarting the framework so the region-sampling listener goes with it" - # `adb root` first, and the two redirects below used to hide why. `stop` and `start` are - # root-only, adbd is not root on a booted emulator, and both were answering `Must be root` - # into /dev/null -- so this restarted nothing, here and in both CI copies, for as long as - # any of them has existed (2026-09-05). Measured on a local android-37.0 AVD: `stop` alone - # says `Must be root`; after `adb root`, `whoami` says root, `stop` returns 0 and - # `pidof system_server` comes back empty. + # NO FRAMEWORK RESTART, and the two lines that used to be here are why this comment is long. + # They were `emu_adb shell stop` and `emu_adb shell start`, both redirected to /dev/null, and + # both root-only -- so what they printed there was `Must be root` and what they did was nothing, + # here and in the two CI copies alike. Making them real (2026-09-05) is what established that + # the disable never worked in the first place: with the package verified `disabled-user` before + # AND after a clean restart on android-37.0, `com.android.systemui` comes up 3 s after + # `system_server` regardless, and the same is visible in CI's own logcat. The restart also loses + # the package state to PackageManager's delayed write if it lands too soon after the `pm` call, + # which cost api37-debug run 34010167885 every test in the leg. # - # Read the abort table in docs/api-37-emulator-crash.md with that in mind: on API 37 the - # image restarts its own framework every minute or so, and a restart AFTER a successful - # `pm disable-user` brings back a SystemUI-less zygote by itself. That is the likeliest - # reason the disable appeared to work here while doing nothing on CI's much quieter - # swiftshader legs, where the logcat shows SystemUI alive for the whole run. - # - # Output is kept rather than discarded now, for the same reason. - emu_adb root > /dev/null 2>&1 - emu_adb wait-for-device - emu_adb shell stop 2>&1 | sed 's/^/ stop: /' - emu_adb shell start 2>&1 | sed 's/^/ start: /' - emu_adb unroot > /dev/null 2>&1 - emu_adb wait-for-device - # There is no property worth waiting on here, and an earlier version of this only looked - # like it was waiting on one: `stop` does not clear sys.boot_completed, so it still reads - # `1` throughout the restart and any loop over it returns at once. The loop below is the - # wait -- and it polls the better thing anyway, since `Can't find service: package` is the - # failure it exists to prevent. + # So the useful part of this function is the quiet window below, not the disable. See + # .github/scripts/e2e-run.sh's header, and docs/api-37-emulator-crash.md. ready=0 for i in $(seq 1 30); do if emu_adb shell service check package 2> /dev/null | grep -q ': found' \ From 557b3edab40ccedab2b1df04b9cd5b2756ce1ccb Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Sat, 5 Sep 2026 23:26:56 -0500 Subject: [PATCH 6/6] Compare the advisory failure count only on a run that finished The verification dispatch of the reworked harness (34011072884) came back 4 expected / 3 received / 3 failed, where the one before it (34008889182) had been 4/4/4 on the identical configuration. Nothing about the test list changed between them: the abort landed one test earlier and the picker test never started. The baseline check would have called that "one now passes", which is the wrong reading and the kind of notice #120 is about -- a deviation that is wrong often enough to teach everyone to skim past deviation notices. So `failed` is compared only when `completed cleanly` is yes, and `expected` is compared always, because `Starting N tests` is printed before anything can abort and is what actually answers "is the marked set the size the baseline says". Two cases in e2e-report-shape-test.sh, as a pair: a truncated run short by one is not a deviation, and a CLEAN run short by one still is -- so the first cannot have bought its quiet by disabling the check. This is a consequence of adding the fourth marker rather than a pre-existing bug worth its own ticket: with three, the advisory leg had been completing. Co-Authored-By: Claude Opus 5 (1M context) --- .github/scripts/e2e-report-shape-test.sh | 36 +++++++++++++++++++ .github/scripts/e2e-report-shape.sh | 16 ++++++++- .../FailsOnEmulatorApi37.kt | 19 ++++++---- 3 files changed, 63 insertions(+), 8 deletions(-) diff --git a/.github/scripts/e2e-report-shape-test.sh b/.github/scripts/e2e-report-shape-test.sh index f354d3c..56894c2 100755 --- a/.github/scripts/e2e-report-shape-test.sh +++ b/.github/scripts/e2e-report-shape-test.sh @@ -184,6 +184,42 @@ out="$(run_report "$root")" assert_contains "same-line annotation removed: counts 2, so it was worth 1" "$out" \ " baseline DEVIATION: the tree carries 2 tests marked \`@FailsOnEmulatorApi37\` but the baseline says 3 — update FAILS_ON_EMULATOR_API37_BASELINE" +# --------------------------------------------------------------------------- +# 4. A run the abort truncated, with fewer failures than the baseline: NOT a deviation. +# +# `expected` comes from `Starting N tests`, printed before anything can abort, so it still +# answers "is the marked set the size the baseline says". `failed` is a tally of what actually +# ran, and on a truncated run the tests after the abort never start. Measured on 2026-09-05, two +# api37-debug dispatches of the same four marked tests: 4/4/4 and then 4/3/3. Announcing the +# second as "one now passes" is the wrong reading, and #120 is the standing lesson about a notice +# that is wrong often enough to be skimmed past. +# --------------------------------------------------------------------------- +root="$(make_root "$FIXTURE_DIR" 3)" +cat > "$root/gradle.log" <<'TRUNCATED' +> Task :app:connectedDebugAndroidTest +Starting 3 tests on test(AVD) - 16 +There was 2 failure(s). +Test run failed to complete. Expected 3 tests, received 2. onError: commandError=false message=INSTRUMENTATION_ABORTED: System has crashed. +TRUNCATED +out="$(run_report "$root")" +assert_contains "truncated run: the truncation is reported" "$out" ' completed cleanly: no' +assert_absent "truncated run: the short failure count is not a deviation" "$out" 'tests failed, the baseline is' + +# --------------------------------------------------------------------------- +# 5. The same short failure count on a run that finished IS a deviation. +# +# The pair is the point: case 4 must not have bought its quiet by disabling the check outright. +# --------------------------------------------------------------------------- +root="$(make_root "$FIXTURE_DIR" 3)" +cat > "$root/gradle.log" <<'CLEAN' +> Task :app:connectedDebugAndroidTest +Starting 3 tests on test(AVD) - 16 +There was 2 failure(s). +CLEAN +out="$(run_report "$root")" +assert_contains "clean run, short by one: the deviation fires" "$out" \ + '2 tests failed, the baseline is 3' + echo if [ "$failures" -eq 0 ]; then echo "e2e-report-shape-test.sh: all checks passed" diff --git a/.github/scripts/e2e-report-shape.sh b/.github/scripts/e2e-report-shape.sh index 7b42321..48c10af 100755 --- a/.github/scripts/e2e-report-shape.sh +++ b/.github/scripts/e2e-report-shape.sh @@ -252,8 +252,22 @@ if [ -n "$baseline" ]; then if [ "$expected" != "unknown" ] && [ "$expected" != "$baseline" ]; then deviations+=("the runner started $expected tests, the baseline is $baseline") fi + # `expected` is compared on every run and `failed` only on a run that finished, and the + # difference is the truncation this file already records rather than compares. `expected` + # comes from `Starting N tests`, which is printed before anything can abort, so it answers + # "is the marked set the size the baseline says" whatever happens afterwards. `failed` is a + # tally of what actually ran: on a truncated run the tests after the abort never start, so + # comparing it to the baseline announces a deviation about the framework dying rather than + # about the test list. Measured on 2026-09-05, two api37-debug dispatches of the same four + # marked tests: 4/4/4 and then 4/3/3, the second having lost the last test to the abort. + # Announcing that as "one now passes" is exactly the wrong reading, and #120 is the standing + # lesson about a notice that is wrong often enough to be skimmed past. if [ "$failed" != "unknown" ] && [ "$failed" != "$baseline" ]; then - deviations+=("$failed tests failed, the baseline is $baseline — every test carrying the marker is expected to fail on this image, so fewer means one now passes and more means a new one joined") + if [ "$completed" = "**no**" ]; then + echo "::debug::$failed of $baseline marked tests failed, on a run the abort truncated — not compared" + else + deviations+=("$failed tests failed, the baseline is $baseline — every test carrying the marker is expected to fail on this image, so fewer means one now passes and more means a new one joined") + fi fi fi if [ -n "$marked" ] && [ "$marked" != "$baseline" ]; then diff --git a/app/src/androidTest/java/org/libremediaconverter/FailsOnEmulatorApi37.kt b/app/src/androidTest/java/org/libremediaconverter/FailsOnEmulatorApi37.kt index 8db3c5a..d53d372 100644 --- a/app/src/androidTest/java/org/libremediaconverter/FailsOnEmulatorApi37.kt +++ b/app/src/androidTest/java/org/libremediaconverter/FailsOnEmulatorApi37.kt @@ -54,13 +54,18 @@ annotation class FailsOnEmulatorApi37 * **The fourth carrier is the one to read that sentence carefully for.** * `pickingAFileThroughTheSystemPickerFillsInTheFileCard` was marked on 2026-09-05 for aborting * `system_server` rather than for failing (#108), and on the gating leg it passed two runs of - * four. The count still holds on the advisory leg, and that was **measured rather than assumed**: - * `api37-debug.yml` run 34008889182, dispatched with this annotation as its filter, reports - * `expected: 4, received: 4, failed: 4` (and `completed cleanly: no`, which is this job's normal). - * It fails there because it runs alongside the rotation test, which takes the framework down first - * — so the reason this line did not have to become two numbers is a property of the advisory leg, - * not of the test. If it ever reports three failures out of four, read that as this test having - * got lucky rather than as an image that improved. + * four. It fails on the advisory leg because the rotation test runs before it and takes the + * framework down first — measured, `api37-debug.yml` run 34008889182, which reports + * `expected: 4, received: 4, failed: 4` with the four in the order Media3, Media3, rotation, + * picker. + * + * **But a second dispatch of the identical configuration reported 4/3/3**, having lost the last + * test to the abort rather than to anything about the test list, and that is why + * `e2e-report-shape.sh` compares `failed` only on a run that finished. `expected` is compared + * always — it comes from `Starting N tests`, which is printed before anything can abort, so it is + * the field that answers "is the marked set the size this number says". Read a *clean* run + * reporting fewer failures than this as one of them now passing; read a truncated one as the + * framework having died, which is this job's normal. * * So: adding or removing a [FailsOnEmulatorApi37] means changing this number, in this file, in * the same diff. The report says so on the run itself if you forget — it prints the tree's own