diff --git a/CHANGELOG.md b/CHANGELOG.md index 9fcf583..62d488a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,21 @@ ### Fixed +- **`mix mob.deploy` hot-pushed over distribution without writing the BEAMs to + disk, so changes reverted on the next restart.** The dist and filesystem + paths were mutually exclusive: when a device answered over dist, the new + modules were loaded into the running VM and the on-disk copies were left + stale. The app then reverted to the last filesystem deploy whenever it + restarted — and `mix mob.connect` restarts the app, so connecting to inspect + your change was enough to undo it. This is the long-standing "`mix + mob.deploy` didn't do anything" report; `--native` was unaffected because it + skips dist entirely. The dist path now hot-loads *and* writes, without + restarting, and a failed write is reported instead of being hidden behind a + successful hot load. Verify with `mix mob.attest` after a restart. + Physical iPhones are excluded and get the hot load plus a warning: they + are often LAN-only with no `devicectl` route, and the write is a + no-undo replace that a mid-copy interruption would leave unbootable. + - **`mix mob.deploy --help` and `mix mob.mutate --help` refused to help.** Strict option parsing turned the flag people try first, on the task they run most, into `Unrecognized or invalid option(s): --help` — telling the reader diff --git a/decisions/2026-09-05-dist-deploy-writes-disk-too.md b/decisions/2026-09-05-dist-deploy-writes-disk-too.md new file mode 100644 index 0000000..286b1b3 --- /dev/null +++ b/decisions/2026-09-05-dist-deploy-writes-disk-too.md @@ -0,0 +1,79 @@ +# A dist deploy hot-loads and writes; it is not a choice between them + +- Date: 2026-09-05 +- Status: accepted + +## Context + +`Deployer.deploy_all/1` picked one of two transports per device: Erlang +distribution when the device answered, filesystem copy otherwise. The dist +branch called `HotPush.push_all/1`, which loads modules into the running VM +over RPC and never touches disk. + +So a dist deploy updated the running app, printed `✓ (dist, no restart)`, and +left the on-disk BEAMs stale. The app reverted to the last filesystem deploy on +its next restart. `mix mob.connect` restarts the app, so the ordinary act of +connecting to inspect a change was enough to undo it — which is why this +presented as intermittent "the deploy did nothing" rather than as a clear bug. +`--native` was unaffected because it passes `force_fs: true` and skips dist. + +Every step reported success. There was no error to notice. + +## Decision + +After a successful hot load, write the filesystem too, by calling the existing +platform deploy with `restart: false`. The hot load is the latency win; the +file write is what makes it survive. They were never genuinely alternatives — +treating them as alternatives is what produced a deploy that was real until the +next restart. + +`restart: false` because the modules are already live; restarting would discard +exactly the state the hot load exists to preserve. + +A failed write is an error, even though the running app is correct at that +moment. Reporting success for an app that will silently revert is the failure +mode this fixes, not a smaller version of it. + +## Scope: not every device + +A physical iPhone is deliberately excluded. It gets the hot load and a warning +that the change will not survive a restart. + +Two independent reasons, either sufficient. `Discovery.IOS` finds physical +devices by probing EPMD across the LAN, and its own comment says LAN-only +devices "will fall back to dist-only in the deployer" — so a WiFi-discovered +iPhone has no `devicectl` route, and the first version of this change turned +that documented fallback into a hard exit 1 on a deploy that had previously +succeeded. And even over USB the write is an +`xcrun devicectl ... --remove-existing-content` replace with no undo: a cable +knock mid-copy leaves the app unbootable, with no way back but another +successful deploy. A hot load could never damage a device. Making it able to, +silently, on the most-used command in the repo, is not a fix. + +The documented physical-iOS dist workflow is USB *unplugged* anyway, which is +precisely the state in which the write cannot run. + +## Consequences + +The cost is not uniform, and the first draft of this record got it wrong by +saying "an rsync… measured in milliseconds". That is true only of the iOS +simulator, which is the one platform that was measured. On Android the persist +runs `pm list packages`, an `adb push` of every beam dir, an Elixir stdlib +sync and an exqlite setup — several MB of transfer plus around 1.8 s of +hardcoded sleeps. Making that incremental (`HotPush.snapshot_beams/0` and +`push_changed/2` already exist) is the obvious follow-up. + +Relatedly, the Android persist calls `adb root`, which on a non-root adbd +restarts adbd and drops every `adb forward` — including the dist tunnels an +open `mix mob.connect` session is using. Harmless while this ran only on the +fallback path; now that a dist deploy persists too, it would kill the user's +IEx session from another terminal. It now checks `adb shell id -u` first, +which is read-only. + +`mix mob.attest` gives this an acceptance test that did not previously exist: +deploy, restart, attest, expect zero stale. Verified on the iOS simulator — +67 modules, 0 stale, after `mix mob.connect` restarted the app; the same +sequence reported 12 stale before. Android and physical iOS are **not** +device-verified here, and physical iOS is excluded by design. + +MOB-161 is a duplicate of this, filed from the same symptom. diff --git a/lib/mix/tasks/mob.attest.ex b/lib/mix/tasks/mob.attest.ex index 360588c..bad5315 100644 --- a/lib/mix/tasks/mob.attest.ex +++ b/lib/mix/tasks/mob.attest.ex @@ -231,7 +231,23 @@ defmodule Mix.Tasks.Mob.Attest do Enum.map(failed, fn r -> "#{r.node}: #{elem(r.verdict, 1)}" end) ++ Enum.map(down, fn n -> "#{n}: unreachable, so it was never checked" end) - unless messages == [], do: Mix.raise(Enum.join(messages, "\n")) + # Any hint here has to name a node that ANSWERED — pointing at one from + # `down` tells the reader to run a command guaranteed to fail differently. + hint = + case {down, results} do + {[], _} -> + "" + + {_, [%{node: reachable} | _]} -> + "\n\nEvery discovered device is a candidate. To check just the one " <> + "that answered:\n mix mob.attest --node #{reachable}\n" <> + "or connect the rest with `mix mob.connect --no-iex`." + + _ -> + "\n\nConnect them with `mix mob.connect --no-iex` first." + end + + unless messages == [], do: Mix.raise(Enum.join(messages, "\n") <> hint) end @doc false diff --git a/lib/mob_dev/deployer.ex b/lib/mob_dev/deployer.ex index f05cd9e..2345d3e 100644 --- a/lib/mob_dev/deployer.ex +++ b/lib/mob_dev/deployer.ex @@ -114,27 +114,24 @@ defmodule MobDev.Deployer do dist_port = dist_port_override || Tunnel.serial_base_port(device.serial) node = Device.node_name(device) + # Shared by both branches. `restart` is honoured by the fallback and + # overridden to false by the dist path, where the modules are + # already live. + platform_opts = [ + restart: restart, + dist_port: dist_port, + node_suffix: node_suffix_override, + beam_flags: beam_flags + ] + {method, result} = if node in dist_nodes do - {:dist, push_via_dist(node, device)} + {:dist, push_via_dist(node, device, beam_dirs, platform_opts)} else fallback = case device.platform do - :android -> - deploy_android(device, beam_dirs, - restart: restart, - dist_port: dist_port, - node_suffix: node_suffix_override, - beam_flags: beam_flags - ) - - :ios -> - deploy_ios(device, beam_dirs, - restart: restart, - dist_port: dist_port, - node_suffix: node_suffix_override, - beam_flags: beam_flags - ) + :android -> deploy_android(device, beam_dirs, platform_opts) + :ios -> deploy_ios(device, beam_dirs, platform_opts) end {:adb, fallback} @@ -352,6 +349,15 @@ defmodule MobDev.Deployer do # If the Elixir stdlib on the device was installed by a different Elixir version # than the host (e.g. after `asdf` upgrade), regex literals and other stdlib # internals will be incompatible. Detect the mismatch and push updated BEAMs. + # `adb shell id -u` is read-only: it cannot restart adbd, so it is safe to + # call on a path that a live dist session depends on. + defp already_root?(serial) do + case run_adb(["-s", serial, "shell", "id", "-u"]) do + {:ok, out} when is_binary(out) -> String.trim(out) == "0" + _ -> false + end + end + defp sync_elixir_stdlib_android(serial) do host_vsn = System.version() pkg = android_package() @@ -373,18 +379,27 @@ defmodule MobDev.Deployer do elixir_lib = :code.lib_dir(:elixir) |> to_string() |> Path.dirname() + # Ask before acting. `adb root` on a non-root adbd RESTARTS adbd, which + # drops every `adb forward`/`reverse` — including the dist tunnels an + # open `mix mob.connect` session is using. That was tolerable when this + # ran only on the fallback path; now that a dist deploy persists too, it + # would kill the user's IEx session from another terminal for nothing. rooted? = - 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 + 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 + _ -> + false + end end if rooted? do @@ -1391,20 +1406,96 @@ defmodule MobDev.Deployer do # # This is why `mix mob.deploy` appeared to do nothing before this fix — the # code WAS pushed correctly, the screen just had no trigger to repaint. - defp push_via_dist(node, device) do + defp push_via_dist(node, device, beam_dirs, platform_opts) do {_pushed, failed} = HotPush.push_all([node]) if failed == [] do # Best-effort: ignored if no screen is currently registered (nav edge # cases, app in background, etc.). :rpc.call(node, :erlang, :send, [:mob_screen, :__mob_hot_reload__]) - {:ok, device} + persist_after_dist(device, beam_dirs, platform_opts) else mods = Enum.map_join(failed, ", ", fn {mod, _} -> inspect(mod) end) {:error, "dist push failed for: #{mods}"} end end + # The hot load is the latency win; the file write is what makes it survive. + # + # Before this, a dist deploy loaded the new modules into the running VM and + # never touched the filesystem, so the app reverted to the last + # filesystem-deployed version on its next restart — and `mix mob.connect` + # restarts the app, so simply connecting to inspect your change undid it. + # That is the long-standing "mix mob.deploy didn't do anything" report + # (MOB-118), and it was invisible because every step reported success. + # + # `restart: false` because the modules are already live: restarting here + # would throw away the state the hot load exists to preserve. + defp persist_after_dist(device, beam_dirs, platform_opts) do + if persistable?(device) do + opts = Keyword.put(platform_opts, :restart, false) + + result = + case device.platform do + :android -> deploy_android(device, beam_dirs, opts) + :ios -> deploy_ios(device, beam_dirs, opts) + end + + dist_outcome(device, result) + else + warn_hot_load_only(device) + {:ok, device} + end + end + + @doc false + # Whether writing this device's filesystem is both possible and safe from + # the dist path. + # + # A physical iPhone is neither. It is discovered by probing EPMD across the + # LAN (`Discovery.IOS`, whose own comment says "LAN-only devices will fall + # back to dist-only in the deployer"), and a LAN-only device has no + # `devicectl` route at all — the first version of this change turned that + # documented fallback into a hard exit 1 on a deploy that had previously + # succeeded. Even with USB attached, the write is an + # `xcrun devicectl ... --remove-existing-content` replace with no undo, so a + # cable knock mid-copy leaves the app unbootable. A hot load could never + # damage a device; making it able to, silently, is not a fix. + # + # The documented physical-iOS dist workflow is USB *unplugged* anyway, which + # is exactly the state in which the write cannot run. + @spec persistable?(Device.t()) :: boolean() + def persistable?(%Device{platform: :ios, type: :physical}), do: false + def persistable?(%Device{}), do: true + + @doc false + # How a persist result combines with an already-successful hot load. + # + # The device HAS been deployed to — the running app is correct. Only + # durability is in question, so only a genuine write failure is an error. + @spec dist_outcome( + Device.t(), + {:ok, Device.t()} | {:error, String.t()} | {:skipped, String.t()} + ) :: + {:ok, Device.t()} | {:error, String.t()} + def dist_outcome(device, {:ok, _}), do: {:ok, device} + + # `:skipped` here means the app is not installed for that platform — yet it + # answered over dist, so it plainly is. Failing the run on that would report + # a device as unreached when it was reached and updated. + def dist_outcome(device, {:skipped, _reason}), do: {:ok, device} + + def dist_outcome(_device, {:error, reason}), + do: {:error, "hot load succeeded but the on-disk BEAMs were not updated: #{reason}"} + + defp warn_hot_load_only(device) do + IO.puts( + "\n #{color(:yellow)}hot-loaded only — #{device.name || device.serial} has no " <> + "filesystem path from here, so this change will not survive a restart. " <> + "Use `mix mob.deploy --native` to make it durable.#{color(:reset)}" + ) + end + # ── Helpers ────────────────────────────────────────────────────────────────── defp collect_beam_dirs do diff --git a/test/mob_dev/deployer_dist_persist_test.exs b/test/mob_dev/deployer_dist_persist_test.exs new file mode 100644 index 0000000..b88bd4d --- /dev/null +++ b/test/mob_dev/deployer_dist_persist_test.exs @@ -0,0 +1,67 @@ +defmodule MobDev.DeployerDistPersistTest do + @moduledoc """ + What a dist deploy does after the hot load (MOB-118). + + The dist and filesystem paths used to be mutually exclusive, so a dist + deploy loaded modules into the running VM and left the on-disk copies stale. + The app reverted on its next restart — and `mix mob.connect` restarts it, so + connecting to look at your change undid it. + + The first version of the fix was tested with source-text assertions, and a + review showed a one-line mutation that **restores the original bug** passes + all of them: pass `[]` for `beam_dirs` at the call site and nothing is + written, while every body those tests read stays identical. These assert the + decisions instead. + """ + use ExUnit.Case, async: true + + alias MobDev.{Deployer, Device} + + defp device(attrs \\ []), + do: struct(%Device{name: "d", serial: "s", platform: :android}, attrs) + + describe "persistable?/1" do + test "an Android device is written to disk" do + assert Deployer.persistable?(device()) + assert Deployer.persistable?(device(type: :emulator)) + end + + test "an iOS simulator is written to disk" do + assert Deployer.persistable?(device(platform: :ios, type: :simulator)) + end + + test "a physical iPhone is not" do + # Two independent reasons, either sufficient. It is discovered by probing + # EPMD across the LAN, and a LAN-only device has no `devicectl` route at + # all — the first version of this fix turned that documented fallback + # into a hard exit 1 on a deploy that previously succeeded. And even over + # USB the write is a `--remove-existing-content` replace with no undo, so + # a cable knock mid-copy leaves the app unbootable. A hot load could + # never damage a device; making it able to, silently, is not a fix. + refute Deployer.persistable?(device(platform: :ios, type: :physical)) + end + end + + describe "dist_outcome/2" do + test "a written device is deployed" do + d = device() + assert Deployer.dist_outcome(d, {:ok, d}) == {:ok, d} + end + + test "a failed write is an error, even though the app is running the new code" do + # It will silently revert on restart, which is worse than a clean + # failure — that is the whole bug. + assert {:error, message} = Deployer.dist_outcome(device(), {:error, "disk full"}) + assert message =~ "hot load succeeded but the on-disk BEAMs were not updated" + assert message =~ "disk full" + end + + test "a skipped write does not fail a device that was reached" do + # `:skipped` means "app not installed for that platform" — but it just + # answered over dist, so it plainly is. Failing here would report a + # device as unreached when it was reached and updated. + d = device() + assert Deployer.dist_outcome(d, {:skipped, "not installed"}) == {:ok, d} + end + end +end