From 225ecdd7e660e6b5d8c29ca645555966f33b21ec Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Sun, 23 Aug 2026 17:20:49 -0500 Subject: [PATCH] Split the API 37 leg so the part that works can gate CI has never run the API level this app targets. The reason it did not was never "API 37 is untestable" -- it was that two tests fail on the emulator image, so one row would be permanently red or permanently allow-listed. This splits that row instead of choosing between those two. E2E API 37 gates. It runs 55 of the suite's 57 instrumented tests and must be green. E2E API 37 Media3 hardware transcode runs the other two, reports, and never blocks (continue-on-error). Both are driven off ONE marker, @FailsOnEmulatorApi37: the gating job passes notAnnotation, the advisory job passes annotation. Two lists would drift, and drift is silent in both directions -- a test that ends up in neither job reads as green. Excluding by class was not an option either: Media3EngineTest has four tests and two of them pass here, so notClass would have thrown away real coverage. The advisory job is named for what it runs, not for what we think is wrong. Both its tests drive a full H.264 -> H.265 hardware transcode, which is what distinguishes them from the two Media3EngineTest cases that pass -- those never decode video. The goldfish-decoder theory sits in a comment inside the job, where it can be corrected without renaming a check people have learned to look for; docs/api-37-emulator-crash.md keeps measurement and inference apart. The SystemUI disable moves into .github/scripts/e2e-run.sh behind E2E_DISABLE_SYSTEM_UI, unset everywhere but the two API 37 jobs, so the other four legs run byte-identical commands -- the same shape as E2E_EXTRA_GRADLE_ARGS. It runs BEFORE the streamed logcat starts, deliberately: `adb shell stop` would end that logcat and nothing restarts it, so a disable placed after it would cost the leg its diagnostics for the part of the run that matters. The body is probe v2 from api37-debug.yml -- the version measured 4/4 -- not the older one-round form: three rounds, waits for system_server to actually be gone, verifies against `pm list packages -d`, and requires a 45 s window with zero new aborts. The weaker probe reported success on a run that then started SystemUI eight more times. The caveat is written next to the row rather than left implicit: this leg runs with SystemUI disabled and the framework restarted under it, a device configuration no other leg and no Pixel run uses. Anything that touches system UI must not trust it, and the Pixel check before each release is still the only API 37 run with SystemUI intact. docs/api-37-emulator-crash.md's "So should CI take API 37?" said no on three reasons. Two were claims about CI that had never been measured; the section now carries the eight runs that measured them, and the third reason is what the split answers. docs/local-emulator.md and api37-debug.yml's header carried the same "the matrix stops at 36" claim and are corrected with it. CLAUDE.md is left alone deliberately -- its "CI's matrix therefore stops at API 36" clause is now false, and that correction is parked in the doc's existing "Correction owed to CLAUDE.md" section, where two others are already waiting. Making E2E API 37 an actually-required check is a repository-settings change and must come after this is on main: adding a required context that does not exist on the default branch blocks every PR. Co-Authored-By: Claude Opus 5 (1M context) --- .github/scripts/e2e-run.sh | 98 ++++++++++++ .github/workflows/api37-debug.yml | 22 ++- .github/workflows/status_check.yml | 148 ++++++++++++++++-- .../FailsOnEmulatorApi37.kt | 24 +++ .../convert/Media3EngineTest.kt | 3 + docs/api-37-emulator-crash.md | 92 ++++++++--- docs/local-emulator.md | 4 +- 7 files changed, 354 insertions(+), 37 deletions(-) create mode 100644 app/src/androidTest/java/org/libremediaconverter/FailsOnEmulatorApi37.kt diff --git a/.github/scripts/e2e-run.sh b/.github/scripts/e2e-run.sh index c5759cb..b736815 100755 --- a/.github/scripts/e2e-run.sh +++ b/.github/scripts/e2e-run.sh @@ -40,6 +40,104 @@ WEDGE_LOG="$TMP/wedge-diagnostics-api${LABEL}.txt" # enough under the job's 60-min cap that a genuine wedge still leaves time to capture it. WEDGE_TIMEOUT=1200 +# --------------------------------------------------------------------------- +# API 37 only, and nothing else sets it, so this is inert everywhere it is not wanted -- +# 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. +# +# 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 +# `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. +# +# 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. +# --------------------------------------------------------------------------- +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'; } + +disable_region_sampling() { + local round=1 i out before after + while [ "$round" -le 3 ]; do + echo "--- SystemUI disable, 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" + case "$out" in *"new state: disabled"*) break ;; esac + sleep 5 + done + + echo " restarting the framework" + adb shell stop + for i in $(seq 1 20); do + [ -z "$(adb shell pidof system_server 2> /dev/null | tr -d '\r\n')" ] && break + sleep 2 + done + echo " system_server down after ~$((i * 2)) s" + adb shell start + 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" + break + fi + sleep 5 + done + + if systemui_disabled; then + echo " verified: com.android.systemui is in pm list packages -d" + else + echo " NOT DISABLED after the restart -- the package state did not survive" + round=$((round + 1)) + continue + fi + + before="$(count_aborts)" + sleep 45 + after="$(count_aborts)" + echo " abort rate, SystemUI disabled: $((after - before)) new in 45 s (total ${after:-0})" + [ "$((after - before))" -eq 0 ] && break + echo " still aborting after round $round" + round=$((round + 1)) + done + + # 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" + else + echo "::warning::E2E api${LABEL}: SystemUI is still enabled -- expect INSTRUMENTATION_ABORTED" + fi + return 0 +} + +if [ "${E2E_DISABLE_SYSTEM_UI:-}" = "1" ]; then + echo "::group::E2E api${LABEL} -- removing the region-sampling listener" + disable_region_sampling + echo "::endgroup::" +fi + # Stream logcat from now until the step ends, into a file that survives to the artifact upload. # Without this, a failure that happens on-device leaves nothing behind: `adb logcat -d` at the # end only has whatever is still in the ring buffer, and a chatty test run evicts the cause. diff --git a/.github/workflows/api37-debug.yml b/.github/workflows/api37-debug.yml index 3a223a5..31e3a87 100644 --- a/.github/workflows/api37-debug.yml +++ b/.github/workflows/api37-debug.yml @@ -3,14 +3,20 @@ name: API 37 debug # --------------------------------------------------------------------------- # WHAT THIS IS FOR, AND WHY IT IS SEPARATE # -# status_check.yml's E2E matrix stops at API 36 because the android-37.x emulator -# images abort surfaceflinger inside their own gralloc mapper. That was established -# locally, under -gpu host and under ANGLE (docs/api-37-emulator-crash.md). It was -# NOT established on a GitHub runner: CI runs -gpu swiftshader_indirect, and the one -# local measurement of that mode was void for a purely local reason (Fedora's SELinux -# denies execheap to SwiftShader's Reactor JIT -- docs/local-emulator.md). So what CI -# actually does at API 37 is an open question, and this workflow is the instrument for -# answering it. +# The android-37.x emulator images abort surfaceflinger inside their own gralloc +# mapper. That was established locally, under -gpu host and under ANGLE +# (docs/api-37-emulator-crash.md), but NOT on a GitHub runner: CI runs +# -gpu swiftshader_indirect, and the one local measurement of that mode was void for a +# purely local reason (Fedora's SELinux denies execheap to SwiftShader's Reactor JIT -- +# docs/local-emulator.md). What CI actually does at API 37 was an open question, and +# this workflow is the instrument that answered it. +# +# IT IS STILL THE INSTRUMENT. status_check.yml now carries API 37 -- a gating leg that +# disables SystemUI first, and an advisory one for the two @FailsOnEmulatorApi37 tests +# -- so this file's job is no longer to decide that, but to test a change to it for one +# dispatch instead of one commit. The next questions it exists for are written down +# under "When to revisit" in docs/api-37-emulator-crash.md: a new API 37.x image, or an +# ATD image for 37, either of which could retire the whole workaround. # # It is a copy of that E2E job with the matrix replaced by workflow_dispatch inputs, # so one hypothesis costs one dispatch rather than one commit. It triggers on nothing diff --git a/.github/workflows/status_check.yml b/.github/workflows/status_check.yml index 171163b..f650df4 100644 --- a/.github/workflows/status_check.yml +++ b/.github/workflows/status_check.yml @@ -178,8 +178,9 @@ jobs: # across it -- none below 34, dataSync at 34, mediaProcessing from 35. Testing a # single level would leave two thirds of that branch unexercised. # - # It stops at 36 rather than targetSdk 37 because the android-37.0 emulator image - # is broken, not because 37 does not matter. See docs/api-37-emulator-crash.md. + # It reaches targetSdk 37, but the API 37 row is not like the other four and the + # comment on it says how. Two tests are excluded there and run in their own + # advisory job below. See docs/api-37-emulator-crash.md. # # FFmpeg is not built here. The AAR is committed under bin/, so a red run means the # code is broken rather than that a cross-compile hiccuped. @@ -204,13 +205,28 @@ jobs: api-level: "35" - label: "36" api-level: "36" - # No API 37 row. targetSdk is 37, but the android-37.0 emulator image - # crash-loops surfaceflinger inside its own gralloc mapper, so every test - # fails there no matter what this app does. Ruling that in took four CI - # rounds, so the evidence and the ruled-out fixes are written down rather - # than left to be rediscovered: docs/api-37-emulator-crash.md. That file - # also records what to try first when re-adding it -- note that the row - # needs api-level "37.0", since a bare 37 fails during SDK setup. + # 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 in this suite touches system UI -- these are 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. + # + # api-level must be "37.0". A bare 37 is not an SDK package and fails + # during setup, which cost a run to discover. + # + # notAnnotation removes the two tests that do not pass on this image; they + # run in the advisory job below, off the same marker so they cannot end up + # in both or neither. docs/api-37-emulator-crash.md has the measurements. + - label: "37" + api-level: "37.0" + disable-system-ui: "1" + gradle-args: "-Pandroid.testInstrumentationRunnerArguments.notAnnotation=org.libremediaconverter.FailsOnEmulatorApi37" steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -236,6 +252,12 @@ jobs: - name: Instrumented tests uses: reactivecircus/android-emulator-runner@a421e43855164a8197daf9d8d40fe71c6996bb0d # v2.38.0 + # Both of these are empty on every row but 37, and both are read with a + # `:-` default in e2e-run.sh, so the four legs below 37 run the identical + # gradle command they always have. + env: + E2E_DISABLE_SYSTEM_UI: ${{ matrix.disable-system-ui }} + E2E_EXTRA_GRADLE_ARGS: ${{ matrix.gradle-args }} with: api-level: ${{ matrix.api-level }} target: google_apis @@ -297,3 +319,111 @@ jobs: name: e2e-wedge-api${{ matrix.label }} path: ${{ runner.temp }}/wedge-diagnostics-api${{ matrix.label }}.txt if-no-files-found: ignore + + # --------------------------------------------------------------------------- + # The two API 37 tests the gating row above excludes, run on their own so they + # stay visible instead of disappearing behind a notAnnotation. + # + # continue-on-error: it reports, it never blocks. That is the whole reason it is + # a separate job rather than a sixth matrix row: a row would share the gating + # job's `E2E API