diff --git a/CHANGELOG.md b/CHANGELOG.md index 5a599fa..3475f15 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,16 @@ +## [Unreleased] + +### Fixed + +- **Android deploys wait for adbd after `adb root` instead of sleeping + (MOB-459).** `adb root` restarts adbd; mob_dev slept a fixed 600–1000 ms and + went on, so on a loaded host the next adb call failed with "Selected Android + device(s) disconnected". `MobDev.AdbRoot.root/2` now runs `adb -s + wait-for-device` plus a `getprop sys.boot_completed` round trip, bounded by + 30 s (`MOB_ADB_RESTART_TIMEOUT_MS` or `config :mob_dev, + adb_restart_timeout_ms:`), and fails naming the serial on timeout. No wait + when adbd already runs as root or refuses root. + ## [0.7.21] - 2026-10-09 ### Added diff --git a/lib/mix/tasks/mob.battery_bench_android.ex b/lib/mix/tasks/mob.battery_bench_android.ex index 2422d13..6931738 100644 --- a/lib/mix/tasks/mob.battery_bench_android.ex +++ b/lib/mix/tasks/mob.battery_bench_android.ex @@ -651,13 +651,13 @@ defmodule Mix.Tasks.Mob.BatteryBenchAndroid do # Check if we can root rooted? = - case adb(device, ["root"]) do - {:ok, out} -> out =~ "restarting" or out =~ "already running as root" - _ -> false + case MobDev.AdbRoot.root(device) do + :rooted -> true + :not_rooted -> false + {:error, message} -> Mix.raise(message) end if rooted? do - :timer.sleep(600) adb!(device, ~w[shell mkdir -p #{beams_dir}]) Enum.each(beam_dirs, fn dir -> diff --git a/lib/mob_dev/adb_root.ex b/lib/mob_dev/adb_root.ex new file mode 100644 index 0000000..e85db8f --- /dev/null +++ b/lib/mob_dev/adb_root.ex @@ -0,0 +1,184 @@ +defmodule MobDev.AdbRoot do + @moduledoc """ + `adb root` that waits for the adbd restart it causes. + + On a non-root adbd, `adb root` answers `restarting adbd as root` and drops + the device's transport while adbd restarts. Under CPU load that restart takes + seconds, so a fixed sleep races it and the next adb command fails with + "device not found" / "Selected Android device(s) disconnected" (MOB-459). + `root/2` instead waits with `adb -s wait-for-device` plus a + `getprop sys.boot_completed` round trip, bounded by a timeout. + + The timeout defaults to 30 s; override it with the `MOB_ADB_RESTART_TIMEOUT_MS` + environment variable, `config :mob_dev, adb_restart_timeout_ms: ms`, or the + `:timeout_ms` option. + """ + + @default_timeout_ms 30_000 + @poll_ms 250 + + @typedoc "Runs `adb` with the given args, returning `{output, exit_status}`." + @type runner :: ([String.t()] -> {String.t(), non_neg_integer()}) + + @doc """ + Runs `adb -s root`. + + Returns `:rooted` once adbd runs as root and answers again, `:not_rooted` if + the device refuses root (production builds) or adb fails, and + `{:error, message}` if adbd restarted but the device didn't come back as + root within the timeout. Waits unless adbd already runs as root or refuses + it; adb sometimes prints nothing while it restarts adbd, so an empty reply + waits too. + + Options: `:runner` (see `t:runner/0`), `:timeout_ms`. + """ + @spec root(String.t(), keyword()) :: :rooted | :not_rooted | {:error, String.t()} + def root(serial, opts \\ []) do + runner = Keyword.get(opts, :runner, &system_adb/1) + + case runner.(["-s", serial, "root"]) do + {out, 0} -> + cond do + out =~ "already running as root" -> + :rooted + + # "restarting adbd as root", or nothing at all: adb can lose the + # connection to the restarting adbd before it prints the reply. + out =~ "restarting" or String.trim(out) == "" -> + with :ok <- wait_for_device(serial, [{:uid, 0} | opts]), do: :rooted + + # "cannot run as root in production builds", LineageOS's "ADB Root + # access is disabled by system setting", ... + true -> + :not_rooted + end + + _ -> + :not_rooted + end + end + + @doc """ + Waits until `serial` is listed again and answers a shell round trip with + `sys.boot_completed` = 1, after anything that restarts adbd (`adb root`, + `adb unroot`, `adb remount`). Same options as `root/2`, plus `:uid`: the + shell uid the restarted adbd must report. The old adbd can still answer for + a moment after `adb root` returns, so `root/2` passes `uid: 0` to tell the + new adbd from the old one. + """ + @spec wait_for_device(String.t(), keyword()) :: :ok | {:error, String.t()} + def wait_for_device(serial, opts \\ []) do + runner = Keyword.get(opts, :runner, &system_adb/1) + timeout = Keyword.get_lazy(opts, :timeout_ms, &timeout_ms/0) + deadline = System.monotonic_time(:millisecond) + timeout + parent = self() + + # `adb wait-for-device` blocks until the serial is back, possibly forever + # (an unplugged phone), and killing the task doesn't kill the OS process, + # so the default runner reports its pid for the timeout to kill. + wait = + if Keyword.has_key?(opts, :runner), + do: runner, + else: &port_adb(&1, parent) + + task = + Task.async(fn -> + wait.(["-s", serial, "wait-for-device"]) + poll_ready(runner, serial, Keyword.get(opts, :uid), deadline) + end) + + case Task.yield(task, timeout) || Task.shutdown(task, :brutal_kill) do + {:ok, :ok} -> + flush_os_pid() + :ok + + _ -> + kill_wait_process() + {:error, timeout_message(serial, timeout)} + end + end + + defp kill_wait_process do + receive do + {:adb_wait_os_pid, os_pid} -> + System.cmd("kill", [to_string(os_pid)], stderr_to_stdout: true) + after + 0 -> :ok + end + end + + defp flush_os_pid do + receive do + {:adb_wait_os_pid, _} -> :ok + after + 0 -> :ok + end + end + + defp port_adb(args, parent) do + case System.find_executable("adb") do + nil -> + system_adb(args) + + adb -> + port = + Port.open({:spawn_executable, adb}, [ + :binary, + :exit_status, + :stderr_to_stdout, + args: args + ]) + + with {:os_pid, os_pid} <- Port.info(port, :os_pid), + do: send(parent, {:adb_wait_os_pid, os_pid}) + + collect_port(port, "") + end + end + + defp collect_port(port, acc) do + receive do + {^port, {:data, data}} -> collect_port(port, acc <> data) + {^port, {:exit_status, status}} -> {acc, status} + end + end + + defp poll_ready(runner, serial, uid, deadline) do + with {out, 0} <- + runner.(["-s", serial, "shell", "getprop sys.boot_completed; id -u; echo ok"]), + ["1", got_uid, "ok"] <- String.split(out), + true <- uid == nil or got_uid == to_string(uid) do + :ok + else + _ -> + if System.monotonic_time(:millisecond) >= deadline do + :timeout + else + Process.sleep(@poll_ms) + poll_ready(runner, serial, uid, deadline) + end + end + end + + defp timeout_message(serial, timeout) do + "Android device #{serial} did not come back within #{timeout} ms after adbd restarted " <> + "(adb root). Raise MOB_ADB_RESTART_TIMEOUT_MS if the host is heavily loaded." + end + + @doc false + @spec timeout_ms() :: pos_integer() + def timeout_ms do + case System.get_env("MOB_ADB_RESTART_TIMEOUT_MS") do + nil -> + Application.get_env(:mob_dev, :adb_restart_timeout_ms, @default_timeout_ms) + + raw -> + case Integer.parse(String.trim(raw)) do + {ms, ""} when ms > 0 -> ms + _ -> @default_timeout_ms + end + end + end + + defp system_adb(args), do: System.cmd("adb", args, stderr_to_stdout: true) +end diff --git a/lib/mob_dev/deployer.ex b/lib/mob_dev/deployer.ex index 29e0fbb..a8d5b14 100644 --- a/lib/mob_dev/deployer.ex +++ b/lib/mob_dev/deployer.ex @@ -513,18 +513,7 @@ defmodule MobDev.Deployer do if already_root?(serial) do true else - case run_adb(["-s", serial, "root"]) do - {:ok, out} when is_binary(out) -> - if out =~ "restarting" or out =~ "already running as root" do - :timer.sleep(600) - true - else - false - end - - _ -> - false - end + root?(serial) end if rooted? do @@ -623,14 +612,7 @@ defmodule MobDev.Deployer do app_data = android_app_data() exqlite_lib = "#{app_data}/otp/lib/exqlite-#{vsn}" - rooted? = - case run_adb(["-s", serial, "root"]) do - {:ok, out} -> out =~ "restarting" or out =~ "already running as root" - _ -> false - end - - if rooted? do - :timer.sleep(600) + if root?(serial) do run_adb(["-s", serial, "shell", prune_other_versions_cmd(exqlite_lib, "exqlite")]) run_adb(["-s", serial, "shell", "mkdir -p #{exqlite_lib}/ebin #{exqlite_lib}/priv"]) run_adb(["-s", serial, "push", "#{Path.expand(exqlite_ebin)}/.", "#{exqlite_lib}/ebin/"]) @@ -693,14 +675,7 @@ defmodule MobDev.Deployer do if File.dir?(local_priv) do device_priv = "#{android_beams_dir()}/priv" - rooted? = - case run_adb(["-s", serial, "root"]) do - {:ok, out} -> out =~ "restarting" or out =~ "already running as root" - _ -> false - end - - if rooted? do - :timer.sleep(600) + if root?(serial) do run_adb(["-s", serial, "shell", "mkdir -p #{device_priv}"]) run_adb(["-s", serial, "push", "#{Path.expand(local_priv)}/.", "#{device_priv}/"]) # Make directories world-readable. mkdir as root creates them system:system @@ -882,26 +857,24 @@ defmodule MobDev.Deployer do # Try adb root first (works on emulators and eng builds). # Check the output text — non-rooted devices return exit 0 with # "cannot run as root in production builds". - rooted? = - case run_adb(["-s", serial, "root"]) do - {:ok, out} -> out =~ "restarting" or out =~ "already running as root" - _ -> false - end + case MobDev.AdbRoot.root(serial) do + :rooted -> + run_adb(["-s", serial, "shell", "mkdir -p #{android_beams_dir()}"]) + + # Labelled by deploy_android's final relabel_otp_android/1. + Enum.reduce_while(beam_dirs, :ok, fn dir, _ -> + case run_adb(["-s", serial, "push", "#{Path.expand(dir)}/.", "#{android_beams_dir()}/"]) do + {:ok, _} -> {:cont, :ok} + {:error, reason} -> {:halt, {:error, "push failed: #{reason}"}} + end + end) - if rooted? do - :timer.sleep(600) - run_adb(["-s", serial, "shell", "mkdir -p #{android_beams_dir()}"]) + :not_rooted -> + # Fall back to run-as tar (non-rooted physical devices). + push_beams_android_runas(serial, beam_dirs) - # Labelled by deploy_android's final relabel_otp_android/1. - Enum.reduce_while(beam_dirs, :ok, fn dir, _ -> - case run_adb(["-s", serial, "push", "#{Path.expand(dir)}/.", "#{android_beams_dir()}/"]) do - {:ok, _} -> {:cont, :ok} - {:error, reason} -> {:halt, {:error, "push failed: #{reason}"}} - end - end) - else - # Fall back to run-as tar (non-rooted physical devices). - push_beams_android_runas(serial, beam_dirs) + {:error, _} = error -> + error end end @@ -1887,6 +1860,23 @@ defmodule MobDev.Deployer do end) end + # `adb root`, waiting out the adbd restart it causes (MOB-459). + # Errors are warned and fall back to the run-as path, which then reports the + # device as failed instead of aborting the whole multi-device deploy. + defp root?(serial) do + case MobDev.AdbRoot.root(serial) do + :rooted -> + true + + :not_rooted -> + false + + {:error, message} -> + IO.puts(" #{color(:yellow)}⚠ #{message}#{color(:reset)}") + false + end + end + defp run_adb(args) do case System.cmd("adb", args, stderr_to_stdout: true) do {output, 0} -> {:ok, String.trim(output)} diff --git a/lib/mob_dev/native_build.ex b/lib/mob_dev/native_build.ex index ca10d0c..e3528e2 100644 --- a/lib/mob_dev/native_build.ex +++ b/lib/mob_dev/native_build.ex @@ -1688,15 +1688,7 @@ defmodule MobDev.NativeBuild do adb = fn args -> System.cmd("adb", ["-s", serial | args], stderr_to_stdout: true) end # Only works on rooted/emulator builds — silently skip on real devices. - rooted? = - case adb.(["root"]) do - {out, 0} -> out =~ "restarting" or out =~ "already running as root" - _ -> false - end - - if rooted? do - :timer.sleep(800) - + if root?(serial) do {lib_dir_out, _} = adb.([ "shell", @@ -1793,17 +1785,27 @@ defmodule MobDev.NativeBuild do adb.(["shell", "am", "force-stop", bundle_id]) :timer.sleep(500) - case adb.(["root"]) do - {out, 0} -> - if out =~ "restarting" or out =~ "already running as root" do - :timer.sleep(1000) - push_otp_root(adb, app_data, otp_dir, elixir_lib) - else - push_otp_runas(serial, bundle_id, app_data, otp_dir, elixir_lib) - end + case MobDev.AdbRoot.root(serial) do + :rooted -> push_otp_root(adb, app_data, otp_dir, elixir_lib) + :not_rooted -> push_otp_runas(serial, bundle_id, app_data, otp_dir, elixir_lib) + {:error, _} = error -> error + end + end - _ -> - push_otp_runas(serial, bundle_id, app_data, otp_dir, elixir_lib) + # `adb root`, waiting out the adbd restart it causes (MOB-459). For the + # best-effort relabel: a device that didn't come back is skipped with a + # warning; the OTP push that follows reports it. + defp root?(serial) do + case MobDev.AdbRoot.root(serial) do + :rooted -> + true + + :not_rooted -> + false + + {:error, message} -> + IO.puts(" #{IO.ANSI.yellow()}⚠ #{message}#{IO.ANSI.reset()}") + false end end diff --git a/test/mob_dev/adb_root_test.exs b/test/mob_dev/adb_root_test.exs new file mode 100644 index 0000000..c4c5924 --- /dev/null +++ b/test/mob_dev/adb_root_test.exs @@ -0,0 +1,80 @@ +defmodule MobDev.AdbRootTest do + use ExUnit.Case, async: true + + alias MobDev.AdbRoot + + # A stubbed adb: `root` answers `root_out`; the device answers the shell + # round trip only once `ready_after_ms` have passed since `root`. + defp runner(root_out, ready_after_ms) do + me = self() + {:ok, agent} = Agent.start_link(fn -> nil end) + + fn args -> + send(me, {:adb, args}) + + case args do + [_, _, "root"] -> + Agent.update(agent, fn _ -> System.monotonic_time(:millisecond) end) + {root_out, 0} + + [_, _, "wait-for-device"] -> + {"", 0} + + [_, _, "shell", _] -> + t0 = Agent.get(agent, & &1) + + # Before the restart lands, the old non-root adbd still answers. + if System.monotonic_time(:millisecond) - t0 >= ready_after_ms, + do: {"1\n0\nok\n", 0}, + else: {"1\n2000\nok\n", 0} + end + end + end + + test "waits out a slow adbd restart and succeeds" do + r = runner("restarting adbd as root\n", 1_200) + assert AdbRoot.root("emu-1", runner: r, timeout_ms: 5_000) == :rooted + assert_received {:adb, ["-s", "emu-1", "wait-for-device"]} + assert_received {:adb, ["-s", "emu-1", "shell", "getprop sys.boot_completed; id -u; echo ok"]} + end + + test "times out with an error naming the serial" do + r = runner("restarting adbd as root\n", :infinity) + assert {:error, msg} = AdbRoot.root("emu-1", runner: r, timeout_ms: 600) + assert msg =~ "emu-1" + assert msg =~ "600 ms" + end + + test "a wait-for-device that never returns still honours the timeout" do + r = fn + [_, _, "root"] -> {"restarting adbd as root", 0} + [_, _, "wait-for-device"] -> Process.sleep(:infinity) + end + + assert {:error, msg} = AdbRoot.root("emu-1", runner: r, timeout_ms: 300) + assert msg =~ "emu-1" + end + + test "an empty reply from adb root still waits for the restart" do + r = runner("", 500) + assert AdbRoot.root("emu-1", runner: r, timeout_ms: 5_000) == :rooted + assert_received {:adb, ["-s", "emu-1", "wait-for-device"]} + end + + test "no wait when adbd already runs as root" do + r = runner("adbd is already running as root\n", 0) + assert AdbRoot.root("emu-1", runner: r) == :rooted + refute_received {:adb, ["-s", "emu-1", "wait-for-device"]} + end + + for refusal <- [ + "adbd cannot run as root in production builds\n", + "ADB Root access is disabled by system setting - enable in Settings -> System -> Developer options\n" + ] do + test "no wait when the device refuses root: #{String.slice(refusal, 0, 24)}" do + r = runner(unquote(refusal), 0) + assert AdbRoot.root("emu-1", runner: r) == :not_rooted + refute_received {:adb, ["-s", "emu-1", "wait-for-device"]} + end + end +end