Remove the accessory image a reboot supersedes

lib/kamal/cli/accessory.rb

@@ -82,9 +82,11 @@ def reboot(name)
       else
         prepare(name)
         pull_image(name)
+        superseded = superseded_image_ids(name)
         stop(name)
         remove_container(name)
         boot(name, prepare: false)
+        remove_superseded_images(name, superseded)
       end
     end
   end
@@ -305,6 +307,30 @@ def upgrade(name)
   end
 
   private
+    # Read the image the accessory is on before its container goes, so the old
+    # image can be removed once the new one is up.
+    def superseded_image_ids(name)
+      Concurrent::Hash.new.tap do |image_ids|
+        with_accessory(name) do |accessory, hosts|
+          on(hosts) do |host|
+            image_ids[host.to_s] = capture_with_info(*accessory.current_image_id, raise_on_non_zero_exit: false).strip.presence
+          end
+        end
+      end
+    end
+
+    def remove_superseded_images(name, image_ids)
+      with_accessory(name) do |accessory, hosts|
+        on(hosts) do |host|
+          next unless (image_id = image_ids[host.to_s])
+
+          unless execute(*accessory.remove_image_id(image_id), raise_on_non_zero_exit: false)
+            info "Kept #{name} image #{image_id[0..18]} on #{host}, docker declined to remove it"
+          end
+        end
+      end
+    end
+
     def with_accessory(name)
       if KAMAL.config.accessory(name)
         accessory = KAMAL.accessory(name)

lib/kamal/commands/accessory.rb

@@ -109,6 +109,16 @@ def remove_image
     docker :image, :rm, "--force", image
   end
 
+  def current_image_id
+    docker :inspect, service_name, "--format", "'{{.Image}}'"
+  end
+
+  # Refuses while any container still references it, so nothing has to compare
+  # the old image against the new one.
+  def remove_image_id(image_id)
+    docker :image, :rm, image_id
+  end
+
   def ensure_env_directory
     make_directory env_directory
   end

test/commands/accessory_test.rb

@@ -193,6 +193,18 @@ class CommandsAccessoryTest < ActiveSupport::TestCase
       new_command(:mysql).follow_logs(host: "1.1.1.5", since: "5m", lines: 123)
   end
 
+  test "current image id names the container, so a stray label match cannot be picked" do
+    assert_equal \
+      "docker inspect app-mysql --format '{{.Image}}'",
+      new_command(:mysql).current_image_id.join(" ")
+  end
+
+  test "remove image id is unforced, so docker refuses while it is in use" do
+    assert_equal \
+      "docker image rm sha256:abc123",
+      new_command(:mysql).remove_image_id("sha256:abc123").join(" ")
+  end
+
   test "remove container" do
     assert_equal \
       "docker container prune --force --filter label=service=app-mysql",

test/integration/accessory_test.rb

@@ -1,6 +1,9 @@
 require_relative "integration_test"
 
 class AccessoryTest < IntegrationTest
+  # The busybox accessory is on the web role, so it runs on both hosts.
+  ACCESSORY_HOSTS = [ "vm1", "vm2" ]
+
   test "boot, stop, start, restart, logs, remove" do
     kamal :accessory, :boot, :busybox
     assert_accessory_running :busybox
@@ -23,6 +26,25 @@ class AccessoryTest < IntegrationTest
     boot = kamal :accessory, :boot, :busybox, capture: true
     assert_match /Skipping booting `busybox` on vm1, vm2, a container already exists/, boot
 
+    # Rebooting onto the same image must keep it — docker refuses to remove an
+    # image a container still references.
+    reboot = kamal :accessory, :reboot, :busybox, capture: true
+    assert_match /Kept busybox image sha256:\h+ on vm\d, docker declined to remove it/, reboot
+    assert_accessory_running :busybox
+    ACCESSORY_HOSTS.each { |host| assert_includes image_tags(host), "busybox:1.36.0" }
+
+    # Point the accessory at a different image, so the reboot supersedes the one
+    # it is running and the old id becomes unreferenced on every host.
+    superseded = accessory_image_ids
+    deployer_exec "sed -i 's|image: busybox:1.36.0|image: busybox:1.37.0|' config/deploy.yml"
+
+    kamal :accessory, :reboot, :busybox
+    assert_accessory_running :busybox, version: "1.37.0"
+    superseded.each do |host, image_id|
+      assert_not_equal image_id, accessory_image_id(host)
+      assert_not_includes image_ids(host), image_id
+    end
+
     kamal :accessory, :remove, :busybox, "-y"
     assert_accessory_not_running :busybox
   end
@@ -54,12 +76,14 @@ class AccessoryTest < IntegrationTest
   end
 
   private
-    def assert_accessory_running(name)
-      assert_match /busybox:1.36.0   "sh -c 'echo \\"Start/, accessory_details(name)
+    def assert_accessory_running(name, version: "1.36.0")
+      assert_match /busybox:#{version}   "sh -c 'echo \\"Start/, accessory_details(name)
     end
 
+    # Version-agnostic, so the check still covers removal after the accessory
+    # has been repointed at another image.
     def assert_accessory_not_running(name)
-      assert_no_match /busybox:1.36.0   "sh -c 'echo \\"Start/, accessory_details(name)
+      assert_no_match /busybox:\S+   "sh -c 'echo \\"Start/, accessory_details(name)
     end
 
     def assert_accessory_volume_mount_options(name)
@@ -77,6 +101,22 @@ def assert_accessory_directory_mode_and_owner(name)
       assert_match /750 1000:1000/, dir_stat, "Expected directory to have 750 mode and 1000:1000 owner"
     end
 
+    def accessory_image_ids
+      ACCESSORY_HOSTS.to_h { |host| [ host, accessory_image_id(host) ] }
+    end
+
+    def accessory_image_id(host)
+      docker_compose("exec #{host} docker inspect custom-busybox --format '{{.Image}}'", capture: true).strip
+    end
+
+    def image_ids(host)
+      docker_compose("exec #{host} docker image ls -aq --no-trunc", capture: true).lines.map(&:strip)
+    end
+
+    def image_tags(host)
+      docker_compose("exec #{host} docker image ls busybox --format '{{.Repository}}:{{.Tag}}'", capture: true)
+    end
+
     def accessory_details(name)
       kamal :accessory, :details, name, capture: true
     end