From 0cee81550255799a0f27db7608073152f413173d Mon Sep 17 00:00:00 2001 From: GenericJam Date: Sat, 5 Sep 2026 09:13:08 -0600 Subject: [PATCH 1/2] Make a dist deploy durable: hot-load and write, not one or the other MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Deployer.deploy_all/1` picked one transport per device — 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 — and `mix mob.connect` restarts the app, so connecting to inspect your change was enough to undo it. That is why this presented as intermittent "the deploy did nothing" rather than as a clear bug, and why it survived: every step reported success and there was no error to notice. `--native` was unaffected because it passes `force_fs: true` and skips dist. The two paths were never genuinely alternatives. The hot load is the latency win; the file write is what makes it survive. The dist branch now does both, calling the existing platform deploy with `restart: false` — restarting there 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, because reporting success for an app that will silently revert is the failure being fixed rather than a smaller version of it. Verified on the iOS simulator with `mix mob.attest`, which gives this an acceptance test it did not previously have: deploy, let `mix mob.connect` restart the app, attest. 67 modules, 0 stale, exit 0. The same sequence reported 12 stale before. Also improves attest's unreachable-device error, which now shows the `--node` invocation to scope a run — every discovered device is a candidate, so connecting to one of four and attesting failed with no hint about how to say which one you meant. Fixes MOB-118. MOB-161 is a duplicate of it, filed from the same symptom before I searched the tracker. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 12 +++++ .../2026-09-05-dist-deploy-writes-disk-too.md | 50 +++++++++++++++++++ lib/mix/tasks/mob.attest.ex | 10 +++- lib/mob_dev/deployer.ex | 47 +++++++++++++++-- test/mob_dev/wiring_test.exs | 33 ++++++++++++ 5 files changed, 148 insertions(+), 4 deletions(-) create mode 100644 decisions/2026-09-05-dist-deploy-writes-disk-too.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 9fcf583..5d31e3f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,18 @@ ### 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. + - **`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..ad35741 --- /dev/null +++ b/decisions/2026-09-05-dist-deploy-writes-disk-too.md @@ -0,0 +1,50 @@ +# 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. + +## Consequences + +A dist deploy now costs an rsync it did not before. That is measured in +milliseconds against a hot load that already crossed the network, and it buys +the property the command implies. + +`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. Before the fix +the same sequence reported 12 stale. + +MOB-161 was filed as a separate bug from the same symptom and is a duplicate of +this. Worth recording that the tracker already held the diagnosis, and the +second report cost more than searching would have. diff --git a/lib/mix/tasks/mob.attest.ex b/lib/mix/tasks/mob.attest.ex index 360588c..bdf5f6e 100644 --- a/lib/mix/tasks/mob.attest.ex +++ b/lib/mix/tasks/mob.attest.ex @@ -231,7 +231,15 @@ 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")) + hint = + if down == [], + do: "", + else: + "\n\nEvery discovered device is a candidate. If you meant to check " <> + "one, scope it:\n mix mob.attest --node #{hd(down) |> to_string()}\n" <> + "or connect the rest with `mix mob.connect --no-iex`." + + 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..cbf4226 100644 --- a/lib/mob_dev/deployer.ex +++ b/lib/mob_dev/deployer.ex @@ -114,9 +114,16 @@ defmodule MobDev.Deployer do dist_port = dist_port_override || Tunnel.serial_base_port(device.serial) node = Device.node_name(device) + 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 @@ -1391,20 +1398,54 @@ 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 + 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 + + case result do + {:ok, _} -> + {:ok, device} + + {:error, reason} -> + # The running app is correct and will silently revert on restart, which + # is worse than a clean failure — say so rather than reporting success. + {:error, "hot load succeeded but the on-disk BEAMs were not updated: #{reason}"} + + {:skipped, reason} -> + {:skipped, reason} + end + end + # ── Helpers ────────────────────────────────────────────────────────────────── defp collect_beam_dirs do diff --git a/test/mob_dev/wiring_test.exs b/test/mob_dev/wiring_test.exs index f68e2e9..fa71a6b 100644 --- a/test/mob_dev/wiring_test.exs +++ b/test/mob_dev/wiring_test.exs @@ -239,4 +239,37 @@ defmodule MobDev.WiringTest do end end end + + describe "a dist deploy also writes the filesystem (MOB-118)" do + @deployer File.read!(Path.expand("../../lib/mob_dev/deployer.ex", __DIR__)) + + test "the dist branch persists after hot-loading" do + # The two paths used to be mutually exclusive: dist hot-loaded into the + # running VM and never touched disk, so the app reverted to the last + # filesystem deploy on its next restart — and `mix mob.connect` restarts + # it, so connecting to inspect your change undid it. + body = region(@deployer, "defp push_via_dist(node, device, beam_dirs", "\n end") + + assert body =~ "HotPush.push_all([node])" + assert body =~ "persist_after_dist(device, beam_dirs, platform_opts)" + end + + test "persisting does not restart — the modules are already live" do + body = region(@deployer, "defp persist_after_dist(", "\n end") + + assert body =~ "Keyword.put(platform_opts, :restart, false)", + "restarting here throws away the state the hot load exists to preserve" + + assert body =~ ":android -> deploy_android(device, beam_dirs, opts)" + assert body =~ ":ios -> deploy_ios(device, beam_dirs, opts)" + end + + test "a failed disk write is reported, not swallowed" do + # The running app is correct and will silently revert, which is worse + # than a clean failure. + body = region(@deployer, "defp persist_after_dist(", "\n end") + + assert body =~ "hot load succeeded but the on-disk BEAMs were not updated" + end + end end From 22bb08a5d0757a2ed42eb5efcc1f71d10918d8a2 Mon Sep 17 00:00:00 2001 From: GenericJam Date: Sat, 5 Sep 2026 09:34:59 -0600 Subject: [PATCH 2/2] Act on the review: the fix broke a path that worked MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An adversarial review found the first version turned a working deploy into a hard failure, and gave a hot load the ability to brick a device. `Discovery.IOS` finds physical iPhones by probing EPMD across the LAN, and its own comment says LAN-only devices "will fall back to dist-only in the deployer". That fallback is the invariant the first version removed: such a device answers dist, hot-loads fine, then `deploy_ios_physical` bails with "device only reachable via WiFi — use `mix mob.push` for BEAM-only updates", which the persist step wrapped as an error. Green deploy, exit 0, became red deploy, exit 1 — and the tool now contradicted its own error message. Worse, on a USB-attached iPhone the write is `xcrun devicectl ... --remove-existing-content` with no undo. A dist deploy used to be a pure in-memory hot load that could not damage the device; the first version made every one of them wipe and rewrite `Documents/otp/`, so a cable knock mid-copy leaves the app unbootable with no way back but another successful deploy. The decision record claimed the only cost was "an rsync". Physical iOS is therefore excluded: it gets the hot load and a warning that the change will not survive a restart. The documented physical-iOS dist workflow is USB unplugged anyway, which is exactly the state in which the write cannot run. A `:skipped` persist no longer fails the device either. It means "app not installed for that platform", but the device just answered over dist, so it plainly is — failing there reports a device as unreached when it was reached and updated. Two more from the review. 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 runs over. Harmless 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. It checks `adb shell id -u` first, which is read-only. And the attest hint I had smuggled into this commit pointed at a node from the *unreachable* list, so following it was guaranteed to fail differently; it now names a node that answered, or says to connect. The three tests were source-text greps, and the review demonstrated a one-line mutation that restores the original bug while passing all of them: pass `[]` for `beam_dirs` at the call site and nothing is written, while every body those tests read stays identical. Replaced with behavioural tests over two extracted decisions — `persistable?/1` and `dist_outcome/2` — which is the pattern `categorize_results/1` already sets in this module. The decision record now says which platforms were measured and which were not, and corrects the cost claim: "milliseconds" was true only of the iOS simulator, the one platform verified. Android moves several MB and sleeps ~1.8s. Re-verified after the changes: deploy, let mix mob.connect restart the app, attest — 67 modules, 0 stale, exit 0. Refs MOB-118 Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 3 + .../2026-09-05-dist-deploy-writes-disk-too.md | 45 ++++-- lib/mix/tasks/mob.attest.ex | 18 ++- lib/mob_dev/deployer.ex | 132 ++++++++++++------ test/mob_dev/deployer_dist_persist_test.exs | 67 +++++++++ test/mob_dev/wiring_test.exs | 33 ----- 6 files changed, 211 insertions(+), 87 deletions(-) create mode 100644 test/mob_dev/deployer_dist_persist_test.exs diff --git a/CHANGELOG.md b/CHANGELOG.md index 5d31e3f..62d488a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,9 @@ 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 diff --git a/decisions/2026-09-05-dist-deploy-writes-disk-too.md b/decisions/2026-09-05-dist-deploy-writes-disk-too.md index ad35741..286b1b3 100644 --- a/decisions/2026-09-05-dist-deploy-writes-disk-too.md +++ b/decisions/2026-09-05-dist-deploy-writes-disk-too.md @@ -34,17 +34,46 @@ 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 -A dist deploy now costs an rsync it did not before. That is measured in -milliseconds against a hot load that already crossed the network, and it buys -the property the command implies. +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. Before the fix -the same sequence reported 12 stale. +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 was filed as a separate bug from the same symptom and is a duplicate of -this. Worth recording that the tracker already held the diagnosis, and the -second report cost more than searching would have. +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 bdf5f6e..bad5315 100644 --- a/lib/mix/tasks/mob.attest.ex +++ b/lib/mix/tasks/mob.attest.ex @@ -231,14 +231,22 @@ 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) + # 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 = - if down == [], - do: "", - else: - "\n\nEvery discovered device is a candidate. If you meant to check " <> - "one, scope it:\n mix mob.attest --node #{hd(down) |> to_string()}\n" <> + 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 diff --git a/lib/mob_dev/deployer.ex b/lib/mob_dev/deployer.ex index cbf4226..2345d3e 100644 --- a/lib/mob_dev/deployer.ex +++ b/lib/mob_dev/deployer.ex @@ -114,6 +114,9 @@ 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, @@ -127,21 +130,8 @@ defmodule MobDev.Deployer do 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} @@ -359,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() @@ -380,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 @@ -1424,26 +1432,68 @@ defmodule MobDev.Deployer do # `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 - opts = Keyword.put(platform_opts, :restart, false) + 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 + result = + case device.platform do + :android -> deploy_android(device, beam_dirs, opts) + :ios -> deploy_ios(device, beam_dirs, opts) + end - case result do - {:ok, _} -> - {:ok, device} + dist_outcome(device, result) + else + warn_hot_load_only(device) + {:ok, device} + end + end - {:error, reason} -> - # The running app is correct and will silently revert on restart, which - # is worse than a clean failure — say so rather than reporting success. - {:error, "hot load succeeded but the on-disk BEAMs were not updated: #{reason}"} + @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 - {:skipped, reason} -> - {:skipped, reason} - end + @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 ────────────────────────────────────────────────────────────────── 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 diff --git a/test/mob_dev/wiring_test.exs b/test/mob_dev/wiring_test.exs index fa71a6b..f68e2e9 100644 --- a/test/mob_dev/wiring_test.exs +++ b/test/mob_dev/wiring_test.exs @@ -239,37 +239,4 @@ defmodule MobDev.WiringTest do end end end - - describe "a dist deploy also writes the filesystem (MOB-118)" do - @deployer File.read!(Path.expand("../../lib/mob_dev/deployer.ex", __DIR__)) - - test "the dist branch persists after hot-loading" do - # The two paths used to be mutually exclusive: dist hot-loaded into the - # running VM and never touched disk, so the app reverted to the last - # filesystem deploy on its next restart — and `mix mob.connect` restarts - # it, so connecting to inspect your change undid it. - body = region(@deployer, "defp push_via_dist(node, device, beam_dirs", "\n end") - - assert body =~ "HotPush.push_all([node])" - assert body =~ "persist_after_dist(device, beam_dirs, platform_opts)" - end - - test "persisting does not restart — the modules are already live" do - body = region(@deployer, "defp persist_after_dist(", "\n end") - - assert body =~ "Keyword.put(platform_opts, :restart, false)", - "restarting here throws away the state the hot load exists to preserve" - - assert body =~ ":android -> deploy_android(device, beam_dirs, opts)" - assert body =~ ":ios -> deploy_ios(device, beam_dirs, opts)" - end - - test "a failed disk write is reported, not swallowed" do - # The running app is correct and will silently revert, which is worse - # than a clean failure. - body = region(@deployer, "defp persist_after_dist(", "\n end") - - assert body =~ "hot load succeeded but the on-disk BEAMs were not updated" - end - end end