Make a dist deploy durable: hot-load and write, not one or the other (MOB-118)

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

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.

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

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

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