From da6f2807e9bd17bd50b85e8f6663a9035f1fecfb Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Sat, 22 Aug 2026 23:06:24 -0500 Subject: [PATCH] Stop an interrupted sweep leaking the emulator, the AVD and the port Four corrections to the harness, none of which changes what a successful sweep does. R17 / #26 -- no trap. Ctrl-C during a sweep (now up to five boots long) left headless qemu on console port 5560 and an lmc_e2e_apiNN AVD behind. The next run's `emulator -port` then collides with the orphan and `emu_adb` can resolve to it -- on a workstation with the Pixel plugged in, exactly the ambiguity the ANDROID_SERIAL pinning exists to prevent. `cleanup` (stop_emulator + delete_created_avds, KEEP_AVD honoured) is now on EXIT, INT and TERM. It is idempotent and the normal path calls it explicitly before the summary, so cleanup output cannot land after the summary and the EXIT trap finds nothing to redo. The interrupt path passes a 6-second grace rather than 30: Ctrl-C has already reached the emulator through the foreground process group, so that wait is only for it to finish writing, and `kill -9` follows regardless. `exit "$overall"` stays the last line, so the exit code an EXIT trap could have swallowed is still the one that escapes. The emulator logs in $LOG_DIR are deliberately kept -- they are the only evidence a failed boot leaves. R31 / #40 -- `kill -9 "${EMU_PID:-0}"`. EMU_PID is empty, not unset, if the background launch never produced a job, so `:-0` converted "nothing to kill" into pid 0, which POSIX reads as the sender's whole process group. The `kill -0` wait loop had the same shape and would have spent its full grace period testing the group. All three sites now take a bare `$EMU_PID` behind one `[ -n ... ] || return 0` guard. boot_emulator's own `kill -0` is left alone: it runs only after the assignment and cannot reach the group form. R33 / #42 -- ensure_avd wrote CI's RAM and disk pins to a hardcoded $HOME/.android/avd/... path and checked nothing. With ANDROID_AVD_HOME (or ANDROID_USER_HOME, or ANDROID_SDK_HOME) set, the sed failed and the level ran on at default RAM and userdata, which surfaces much later as "not enough space" and reads as a device problem. `avd_config_path` now looks in every directory avdmanager honours -- no precedence is asserted, the existence check decides -- and a level that cannot be found or written fails instead of running unpinned. R34 / #43 -- disable_region_sampling's "one blocking wait on the device" did not wait: `adb shell stop` does not clear sys.boot_completed, so the property still read 1 and the loop returned at once. Deleted, and the comment now names the service-check loop below it as the actual wait -- which polls the better thing anyway, since `Can't find service: package` is the failure it exists to prevent. That loop also says so when it gives up after 150 s instead of proceeding silently. Deliberately not doing the `setprop sys.boot_completed 0` variant: the loop tested for an empty value, so a 0 would not have made it wait either, and the `!= 1` form it would need is an unbounded loop inside `adb shell` with no timeout. Checked with `bash -n` and with two stub harnesses in place of a device (shellcheck is not installed here): one drives the extracted lifecycle functions against fake binaries and asserts pid 0 really does hit the sender's process group, that an empty EMU_PID now signals nothing and returns at once, that SIGINT cleans up once and exits 130 within seconds, that KEEP_AVD survives the trap path, and that an explicit exit status survives the EXIT trap; the other runs the real script end to end on the boot-failure path, which stops short of e2e-run.sh, and checks the pins land in config.ini, the created AVD is removed, a misplaced config.ini fails the level, and `set -u` is not tripped anywhere. No emulator was booted. Co-Authored-By: Claude Opus 5 (1M context) --- tools/local-emulator/run-e2e.sh | 137 +++++++++++++++++++++++++++----- 1 file changed, 119 insertions(+), 18 deletions(-) diff --git a/tools/local-emulator/run-e2e.sh b/tools/local-emulator/run-e2e.sh index b461944..a512d93 100755 --- a/tools/local-emulator/run-e2e.sh +++ b/tools/local-emulator/run-e2e.sh @@ -95,6 +95,11 @@ RESULTS_DIR="app/build/outputs/androidTest-results" LOG_DIR="${TMPDIR:-/tmp}/lmc-local-e2e" mkdir -p "$LOG_DIR" +# The two things that outlive a level, declared here rather than where they are first +# assigned, because the cleanup trap below can fire before either has been reached. +EMU_PID="" +CREATED_AVDS=() + # ---------------------------------------------------------------- renderer preflight --- if [ -n "$GPU_MODE" ]; then case "$GPU_MODE" in @@ -187,6 +192,27 @@ gpu_for_api() { # normalised, so that `run-e2e.sh 37 37.0` does not have both levels fight over one AVD. avd_for_api() { echo "lmc_e2e_api${1//./_}"; } +# Where avdmanager actually put the AVD. `$HOME/.android/avd` is only the default: +# ANDROID_AVD_HOME, ANDROID_USER_HOME and ANDROID_SDK_HOME each move it, and hardcoding the +# default meant a machine that sets any of them silently ran every level at stock RAM and +# userdata size. Rather than encode a precedence that cannot be verified from here, look in +# every location avdmanager honours and let the existence check pick. +avd_config_path() { + local avd="$1" base cfg + for base in "${ANDROID_AVD_HOME:-}" \ + "${ANDROID_USER_HOME:+$ANDROID_USER_HOME/avd}" \ + "${ANDROID_SDK_HOME:+$ANDROID_SDK_HOME/.android/avd}" \ + "$HOME/.android/avd"; do + [ -n "$base" ] || continue + cfg="$base/${avd}.avd/config.ini" + if [ -f "$cfg" ]; then + echo "$cfg" + return 0 + fi + done + return 1 +} + ensure_avd() { local api="$1" avd="$2" local pkg @@ -213,10 +239,29 @@ ensure_avd() { fi # Written into config.ini rather than passed on the command line, which is how - # reactivecircus/android-emulator-runner applies the same two settings in CI. - local cfg="$HOME/.android/avd/${avd}.avd/config.ini" - sed -i -e '/^disk\.dataPartition\.size=/d' -e '/^hw\.ramSize=/d' "$cfg" - printf 'disk.dataPartition.size=%s\nhw.ramSize=%s\n' "$DISK_SIZE_BYTES" "$RAM_SIZE_MB" >> "$cfg" + # reactivecircus/android-emulator-runner applies the same two settings in CI. On the reuse + # path too, so an AVD left over from an older run gets today's pins. + # + # A level that cannot be pinned FAILS rather than running at the defaults. Unpinned, it + # dies much later with "not enough space", which reads as a device problem -- CI's own + # history is where that lesson comes from -- and nothing points back to a `sed` that + # edited a path this script guessed wrong. + local cfg + if ! cfg="$(avd_config_path "$avd")"; then + echo " FAILED: no config.ini for $avd in any directory avdmanager uses" + echo " (ANDROID_AVD_HOME=${ANDROID_AVD_HOME:-unset}, ANDROID_USER_HOME=${ANDROID_USER_HOME:-unset}," + echo " ANDROID_SDK_HOME=${ANDROID_SDK_HOME:-unset}, HOME=$HOME)" + return 1 + fi + if ! sed -i -e '/^disk\.dataPartition\.size=/d' -e '/^hw\.ramSize=/d' "$cfg"; then + echo " FAILED to rewrite $cfg" + return 1 + fi + if ! printf 'disk.dataPartition.size=%s\nhw.ramSize=%s\n' \ + "$DISK_SIZE_BYTES" "$RAM_SIZE_MB" >> "$cfg"; then + echo " FAILED to write the RAM/disk pins into $cfg" + return 1 + fi } boot_emulator() { @@ -281,7 +326,7 @@ boot_emulator() { # 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. disable_region_sampling() { - local api="$1" out i before after + local api="$1" out i before after ready case "$api" in 37 | 37.*) ;; *) return 0 ;; esac out="" @@ -305,16 +350,24 @@ disable_region_sampling() { 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 - # One blocking wait on the device, then confirm the services the test runner actually calls. - emu_adb wait-for-device shell \ - 'while [[ -z $(getprop sys.boot_completed) ]]; do sleep 2; done' > /dev/null 2>&1 + # 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. + ready=0 for i in $(seq 1 30); do if emu_adb shell service check package 2> /dev/null | grep -q ': found' \ && emu_adb shell service check activity 2> /dev/null | grep -q ': found'; then + ready=1 break fi sleep 5 done + if [ "$ready" -ne 1 ]; then + echo " WARNING: package and activity services still absent 150 s after the restart." + echo " Expect INSTRUMENTATION_ABORTED -- docs/api-37-emulator-crash.md" + fi # Prove it worked rather than assume it. Zero new aborts over this window is what makes the # difference between a run that completes and one that reports `Starting 0 tests`. @@ -336,17 +389,70 @@ disable_animations() { done } +# `${EMU_PID:-0}` used to guard these three calls, and it guarded the wrong thing: EMU_PID +# is *empty*, not unset, if the background launch never produced a job, and `kill` reads pid +# 0 as "the sender's whole process group" -- this script and, on a terminal, everything else +# in the foreground group with it. The `kill -0` wait loop had the same shape and would have +# spent its full grace period testing the group. Nothing to stop is now a return, never a +# guess. (boot_emulator's own `kill -0 "$EMU_PID"` is unguarded and cannot reach that form: +# it runs only after the assignment.) +# +# max_wait is a parameter so the interrupt path need not sit through the full grace period. stop_emulator() { + local max_wait="${1:-30}" waited=0 + [ -n "${EMU_PID:-}" ] || return 0 emu_adb emu kill > /dev/null 2>&1 - local waited=0 - while kill -0 "${EMU_PID:-0}" 2> /dev/null && [ "$waited" -lt 30 ]; do + while kill -0 "$EMU_PID" 2> /dev/null && [ "$waited" -lt "$max_wait" ]; do sleep 2 waited=$((waited + 2)) done - kill -9 "${EMU_PID:-0}" 2> /dev/null - wait "${EMU_PID:-0}" 2> /dev/null + kill -9 "$EMU_PID" 2> /dev/null + wait "$EMU_PID" 2> /dev/null + EMU_PID="" } +delete_created_avds() { + local avd + [ "${KEEP_AVD:-0}" = "1" ] && return 0 + for avd in ${CREATED_AVDS[@]+"${CREATED_AVDS[@]}"}; do + avdmanager delete avd -n "$avd" > /dev/null 2>&1 + done + CREATED_AVDS=() +} + +# What an interrupted sweep used to leave behind: a headless emulator holding console port +# $EMULATOR_PORT, and an lmc_e2e_apiNN AVD. The next run's `emulator -port` then collides +# with the orphan, and `emu_adb` can resolve to it -- on a workstation that also has the +# Pixel plugged in, exactly the ambiguity the ANDROID_SERIAL pinning exists to prevent. A +# sweep is up to five boots long, so the window for one Ctrl-C is not small. +# +# Idempotent, and called explicitly on the normal path so its output cannot land after the +# summary; the EXIT trap then finds nothing left to do. The emulator logs are deliberately +# NOT removed -- they live in $LOG_DIR and are the only evidence a failed boot leaves. +CLEANED=0 +cleanup() { + [ "$CLEANED" = "1" ] && return 0 + CLEANED=1 + stop_emulator "${1:-30}" + delete_created_avds +} + +# 6 s, not 30: Ctrl-C has already reached the emulator through the foreground process group, +# so this is only waiting for it to finish writing, and `kill -9` follows regardless. The +# EXIT trap is disarmed before exiting so the status below is the one that survives. +on_signal() { + echo + echo "interrupted (SIG$1) -- stopping the emulator and removing the AVDs this run created" + echo " emulator logs kept in $LOG_DIR" + cleanup 6 + trap - EXIT + exit "$2" +} + +trap 'on_signal INT 130' INT +trap 'on_signal TERM 143' TERM +trap cleanup EXIT + # The XML is authoritative. The console counter double-counts skips, so a run that reports # "42 tests" on stdout can be 40 in the report. # @@ -383,7 +489,6 @@ PY } # ------------------------------------------------------------------------------ main --- -CREATED_AVDS=() SUMMARY=() overall=0 @@ -447,11 +552,7 @@ for api in "${APIS[@]}"; do stop_emulator done -if [ "${KEEP_AVD:-0}" != "1" ]; then - for avd in ${CREATED_AVDS[@]+"${CREATED_AVDS[@]}"}; do - avdmanager delete avd -n "$avd" > /dev/null 2>&1 - done -fi +cleanup echo echo "===================== LOCAL E2E SUMMARY ======================"