[ovitrif]

The compose-ui / ui-tests job added in #1757 can hang for its full 45 minutes after every test has passed. That cancels the job and fails build, which requires it.

What happens:

• After the tests, reactivecircus/android-emulator-runner runs adb emu kill.
• The emulator logs removeAll and then never exits.
• The action waits on the emulator process, so the step sits idle until the job timeout cancels it.

Seen in these runs:

• fix: pin paykit request keypad and ok button #1758, run 41529634155, twice, including the rerun. All 256 tests passed at 22:36 (report artifact: tests="256" failures="0"), then the job idled until its cancellation at 23:07.
• feat/arkade-lightning, run 41523743337. The same pattern: tests passed at 21:54, the job was cancelled at 22:27.
• master, run 41522823574: the emulator exited normally and the job passed. The hang is intermittent.

Description

• On GitHub Actions, run-ui-tests.sh now cleans up the processes that can hold the step open, through an EXIT trap that runs whether the tests pass or fail:
  • it stops the Gradle daemon;
  • it sends adb emu kill, waits up to 30 seconds for the emulator to exit, then force-kills it with a warning;
  • it kills the emulator's crashpad_handler and stops the adb server.
• The first attempt stopped only the emulator, and the hang came back with the emulator already gone (run 41543568436). When the job was cancelled, GitHub terminated three orphans: java (the Gradle daemon), crashpad_handler and adb. The runner waits until the emulator's output closes, and these processes outlive the emulator and keep that output open.
• Local runs of the script (documented in docs/android-test-lanes.md) skip the trap and behave as before. Its kills are host-wide, so they would also stop other emulators on a shared machine.
• The runner's own emu kill afterwards finds no device, and the action catches that error. The step ends with the script's own exit code, so the test result still decides pass or fail.

Out of Scope

• The AVD snapshot step keeps the runner's normal shutdown, because it needs a clean shutdown to save the snapshot it caches. No hang has been seen there.

Design

N/A — no UI changes.

Preview

N/A

QA Notes

Setup

N/A — CI-only change; no device testing required.

Journeys

N/A — no user-visible behaviour change.

Manual Tests

N/A

Automated Checks

N/A

Evidence

• bash -n passes on the script. shellcheck reports only an existing note about a deliberately single-quoted adb shell command.
• I checked locally that an EXIT trap keeps the script's exit code: a failing script still exits 3, a passing one 0.
• This PR's own compose-ui / ui-tests run is the end-to-end check.

Gaps

• The hang is intermittent, so a green run on this PR does not prove it can't recur. The run log shows whether the bounded stop was needed.

Models used

• Planning/scoping: claude-opus-5-5 (reasoning: medium)
• Implementation: claude-opus-5-5 (reasoning: medium)
• Review: Not performed

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-11-02T23:13:21.037041Z	15575e1	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[github-actions]

Regtest APK

Built from e6cfbe8 (run).

Download bitkit-dev-debug universal APK (expires in 30 days).

[greptile-apps]

Confidence Score: 5/5

[Medium impact] The PR appears safe to merge; no actionable failures were found.

Summary

Adds an EXIT trap to run-ui-tests.sh that requests emulator shutdown, waits up to 30 seconds for the process to disappear, and force-kills it if needed.

• The UI-test script bounds emulator shutdown when it exits.

_{Reviews (1) · Last reviewed commit: "ci: stop emulator teardown from hanging ..." · Reviewed by Greptile}

[talosmachina]

No findings. Adds an EXIT trap to run-ui-tests.sh that runs adb emu kill, waits up to 30 s for qemu-system to exit, then force-kills it, so a hung emulator shutdown no longer holds compose-ui / ui-tests until its 45 min timeout. Reviewed 15575e1, full tier (CI script).

What I checked, and 4 candidates I ruled out

Read in full: .github/scripts/run-ui-tests.sh, .github/workflows/ui-tests.yml, and src/main.ts plus src/emulator-manager.ts of reactivecircus/android-emulator-runner@v2 (a421e43).
Mechanism checked: the action starts the emulator as sh -c "emulator ... &" and calls killEmulator after the script, which runs adb emu kill inside a try/catch that only logs. So when the script has already stopped the emulator, the action's second emu kill fails with no device and the step result is unchanged. The script's own exit status still decides the step, because a script failure is the only thing that reaches core.setFailed.
Build: not built. The script needs an emulator, and on this track Talos does not build bitkit-android branches. This PR's own compose-ui / ui-tests run is the end-to-end check, and it is still in progress.

Ruled out

• Trap masks the exit code: a local run of the same trap shape under set -euo pipefail exits 0 on success, 1 on a set -e failure and 3 on exit 3. The trap never calls exit, and every command in it that can fail is guarded by || true or || return 0.
• pgrep -f matches the script itself: the script runs as bash .github/scripts/run-ui-tests.sh, and pgrep excludes its own process. The runner hosts a single emulator, so the pattern cannot hit an unrelated process.
• Trap fires before boot: if adb wait-for-device or the install fails, the same bounded stop runs, and the action's catch still runs killEmulator. Same result as before, just without the hang.
• Snapshot step loses its clean shutdown: the trap lives only in the script of the test step, which already runs with -no-snapshot-save. The cache-generation step uses echo as its script and is untouched.

Merge confidence: 4/5. No findings, but the job that exercises the change (compose-ui / ui-tests) had not finished when I reviewed.

Reviewer model: claude-opus-5-5
Reasoning effort: Not exposed (the review runtime does not report its reasoning setting to the reviewer)

[ovitrif]

Pushed 9aa22aa. The first attempt stopped the emulator, but the step still hung (run 41543568436). The emulator was gone, yet three orphans stayed alive until GitHub cancelled the job: java (the Gradle daemon), crashpad_handler and adb. The exit trap now also stops the Gradle daemon, kills crashpad_handler and stops the adb server. This run is the next check.

[piotr-iohk]

QA review

Update: e6cfbe8 installs the teardown trap only when GITHUB_ACTIONS=true. The local-use finding is verified fixed and its thread is resolved. All 256 Compose tests passed on this head, teardown completed promptly, and required CI is green. The fresh full QA reassessment has no new actionable findings. The retained finding and validation below describe the earlier reviewed revision.

Scope: full review of the whole PR diff against its merge base (b82f451), at 9aa22aa. The change adds an EXIT trap to .github/scripts/run-ui-tests.sh. Covered: what the trap does, its caller in ui-tests.yml and the ci.yml build gate, and how reactivecircus/android-emulator-runner@v2 behaves, checked in its source at a421e43.

Priority reassessed: 1 nonblocking follow-up (P3 / LOW). The original clean conclusion overlooked the documented local script invocation; its limited local-use impact is now treated as nonblocking for the intended isolated hosted-CI fix. The unconditional cleanup can kill unrelated local emulator and crash-handler processes. Restrict it to the isolated hosted CI runner or to PIDs owned by this run. See the updated QA review and inline finding.

The updated assessment is retained in QA Watch; the original runtime observations below remain historical evidence. A safe mocked check with CI variables absent confirmed that both successful and failed exits execute the broad kill commands while preserving exit codes 0 and 3. No real process was terminated by that check.

The action starts the emulator in the background with sh -c "emulator … &". After the script, it runs adb -s emulator-<port> emu kill and catches the error if that fails. The script runs with ANDROID_SERIAL set, so the trap's own adb emu kill targets the same emulator. The trap stops the Gradle daemon, the emulator, crashpad_handler and the adb server. The runner's later emu kill then fails, and the action catches that, so the step result still comes from the script. The pgrep/pkill -f patterns are broad. On an ephemeral hosted runner, no other process in this job is expected to have qemu-system or crashpad_handler in its command line.

Validation:

• Executed check: a small bash 5.1 test with the same set -euo pipefail and EXIT-trap shape kept the script's exit codes. A failing command gave 3, an explicit exit 1 gave 1 and a pass gave 0. The trap's || true and if ! checks don't change the result.
• This PR's own compose-ui / ui-tests job at this head: 256 tests ran with 0 failures. The emulator's removeAll came about 2 seconds after the tests finished, and the force-kill warning never appeared. The runner's later emu kill failed with "Connection refused", which the action caught. The report was uploaded, and the job finished in about 9.5 minutes, so it did not run into the 45-minute timeout.
• The hang is intermittent, so one green run doesn't prove it can't come back.

Limitations: Code-only review; no device or emulator was started. I only read the integration path (the default for master pushes and manual runs) and didn't see it run.

Device testing: not performed in this review. This is a CI-only change with no effect on the app.

Suggested additional test cases

• CI, Android Instrumented Tests started manually with suites=integration on this branch: check that the step ends shortly after the tests and passes or fails with the Gradle result, without running into the 45-minute timeout.
• CI, a throwaway branch with one Compose UI test made to fail on purpose: check that compose-ui / ui-tests fails promptly with the test failure, not a timeout cancellation, and that the report is still uploaded.

Reviewer model: claude-opus-5-5
Reasoning effort: high

[piotr-iohk]

QA review

Update: e6cfbe8 installs the teardown trap only when GITHUB_ACTIONS=true. The local-use finding is verified fixed and its thread is resolved. All 256 Compose tests passed on this head, teardown completed promptly, and required CI is green. The fresh full QA reassessment has no new actionable findings. The retained finding and validation below describe the earlier reviewed revision.

Scope: full reassessment of the complete one-file PR diff at 9aa22aa against merge base b82f451, including the entire script, both hosted and documented local invocation, the reusable workflow and required build gate, and emulator-runner source at a421e43855164a8197daf9d8d40fe71c6996bb0d. No source coverage was inherited. This reassessment corrects the earlier clean review's assumption that the script runs only on an ephemeral hosted runner.

1 nonblocking P3 / LOW follow-up. The teardown is appropriate on the isolated hosted CI runner. The remaining documented local-use concern does not warrant withholding approval of this CI fix.

Validation: a safe mocked execution of the pinned script with CI variables absent confirmed that broad process cleanup runs on both success and failure while preserving exit codes 0 and 3. The mock's process lookup represented an unrelated emulator remaining after the selected device stopped; no real emulator or crash handler was killed. Current-head CI is green, including Compose, but that isolated-runner success does not cover the documented local multi-emulator case.

Suggested additional test cases

• Mock two emulator process identities, select only one with ANDROID_SERIAL, and exercise successful and failed test exits. Cleanup must preserve the unselected emulator and crash handler and preserve the original exit code.
• CI, manually run suites=integration and a deliberately failing Compose fixture; verify prompt teardown and report upload while retaining each test result.

Device testing: not performed. No app build or emulator was started for this reassessment.

Findings

• [LOW] Restrict teardown to processes owned by this test run — inline at .github/scripts/run-ui-tests.sh:24.

Reviewer model: gpt-6.1-sol
Reasoning effort: high

[ovitrif]

Pushed e6cfbe8: the teardown trap now runs only on GitHub Actions. This answers the review finding that the host-wide pkill calls could stop unrelated emulators when the script runs locally, as docs/android-test-lanes.md documents. CI behaviour is unchanged. The description now notes the local case.

[piotr-iohk]

QA review

Scope: Full reassessment, at the explicit request for a full review, of the complete PR diff at e6cfbe8 against merge base b82f451. Covered: the run-ui-tests.sh teardown and its gate; its only caller, the ui-tests.yml lane that ci.yml calls as compose-ui on hosted ubuntu-latest; the local invocation documented in android-test-lanes.md; and the reactivecircus/android-emulator-runner@v2 teardown it interacts with, pinned at a421e43 (v2.38.0). No coverage is inherited from an earlier review.

No new actionable code findings.

• The trap is installed only on CI. The EXIT trap is installed only when GITHUB_ACTIONS is exactly true. The action runs the script through sh -c with the runner environment plus ANDROID_SERIAL=emulator-<port>. So on the hosted runner the gate is true, and the trap's adb emu kill targets the job's own emulator.
  • In the compose-ui / ui-tests run at the pinned head, the emulator began shutting down 0.6 s after all 256 tests passed, before the action's own Terminate Emulator.
  • No forced-kill warning appeared. The action's emu kill then got Connection refused, which the action catches.
  • The runner's orphan-process cleanup terminated nothing, and the step ended about 3 s after the tests.
• Exit codes are preserved. The script's exit status still decides the step: the action calls setFailed on a non-zero exit, and its later emu kill error is only logged. Every trap command is either guarded with || true or used as a condition, so set -e cannot replace the original status.
• Process ownership.
  • When GITHUB_ACTIONS is not exactly true (unset locally, or any other value), none of the new code runs. A developer host never sees the host-wide pkill -9 -f qemu-system and pkill -9 -f crashpad_handler, the user-wide ./gradlew --stop, or adb kill-server.
  • On Actions, those kills only reach the job's ephemeral VM, because the only caller pins ubuntu-latest.
  • GITHUB_ACTIONS is also true on self-hosted runners. If this lane ever moves to a shared self-hosted host, the guard should also require RUNNER_ENVIRONMENT == github-hosted.
• Residual timing risk (not a regression). The trap's adb emu kill and ./gradlew --stop calls have no time limit of their own.
  • adb keeps reading the emulator console until the emulator exits or answers the trailing quit. The emulator's kill handler leaves the console open while it shuts down.
  • So an emulator that wedged with its console still open could stall the trap before its 30 s fallback. The emulator's own 20 s self-kill watchdog, which appears in the CI logs, normally limits this.
  • The action's own emu kill has the same exposure at the base revision. Wrapping these calls in timeout would make the stated "bounded waits" unconditional.

Validation:

• I ran the pinned script locally against stub versions of adb, gradlew, pgrep, pkill and sleep, invoked through sh -c as the action does. No device was used.
  • The script's exit status was preserved in every case: pass (0), result-assertion failure (1), Gradle failure (7), invalid annotation (1), a failing second suite (5), the forced-kill path, and every trap command failing.
  • SIGTERM ran the trap and exited with 143.
  • With GITHUB_ACTIONS unset, false, TRUE or 1, no trap ran and the commands matched the base script.
  • bash -n passes.
• The step also passed at 9aa22aa, which differs from the head only by the gate.
• Both CI runs are single passing samples of a hang that only happens sometimes, so they don't prove it can't recur. The failing-test path has not run on a hosted runner.

Limitations:

• The log of the earlier hanging run 41543568436 could not be retrieved here. The diagnosis that leftover orphan processes held the step open therefore rests on the author's report.
• I checked whether an adb server restarted by the action's teardown would hold the step's output open, using AOSP system/core 5aa0e42. It would not: the daemon redirects its stdio. That source is older than the runner's platform-tools.

Device testing: not performed in this review. Only the CI script changes, not app behavior, so Android/iOS parity does not apply.

Suggested additional test cases

1. GitHub Actions, failing exit path. Dispatch Android Instrumented Tests on ci/ui-tests-emulator-teardown with suites: foo.Bar. Expected:
   • After installing the app, the script logs the invalid-annotation ::error::.
   • The emulator's shutdown lines appear before Terminate Emulator.
   • The step fails with exit code 1.
   • The job finishes within minutes instead of idling until the 45-minute timeout.
2. Local, shared host. Start two emulators (one a disposable test target) and leave an idle Gradle daemon running. With GITHUB_ACTIONS unset, run ANDROID_SERIAL=<test emulator> ANDROID_TEST_SUITES=ComposeUi bash .github/scripts/run-ui-tests.sh. Expected afterwards:
   • adb devices still lists both emulators.
   • The other emulator's qemu-system and crashpad_handler processes are still running.
   • ./gradlew --status still shows the daemon.

Earlier findings reconciled at this revision: 1 verified fixed.

Reviewer model: claude-opus-5-5
Reasoning effort: xhigh

[piotr-iohk]

Verified e6cfbe8 and the clean full QA reassessment. The local-use cleanup finding is verified fixed, and there are no remaining actionable findings.

All 256 Compose tests passed, emulator teardown completed promptly, and the required build, lint and e2e-status checks are green. No additional manual device session was required for this CI-only change.

Reviewer model: gpt-6.1-sol
Reasoning effort: high

[ovitrif]

Merging: approved on e6cfbe8, CI green including compose-ui / ui-tests (9m31s, no teardown hang), no open threads.