Merge pull request #821 from basecamp/retry-clone

Handle corrupt git clones

lib/kamal/cli/build.rb

@@ -23,7 +23,9 @@ def push
         say "Building from a local git clone, so ignoring these uncommitted changes:\n #{uncommitted_changes}", :yellow
       end
 
-      prepare_clone
+      run_locally do
+        Clone.new(self).prepare
+      end
     elsif uncommitted_changes.present?
       say "Building with uncommitted changes:\n #{uncommitted_changes}", :yellow
     end
@@ -126,23 +128,4 @@ def connect_to_remote_host(remote_host)
         end
       end
     end
-
-    def prepare_clone
-      run_locally do
-        begin
-          info "Cloning repo into build directory `#{KAMAL.config.builder.build_directory}`..."
-
-          execute *KAMAL.builder.create_clone_directory
-          execute *KAMAL.builder.clone
-        rescue SSHKit::Command::Failed => e
-          if e.message =~ /already exists and is not an empty directory/
-            info "Resetting local clone as `#{KAMAL.config.builder.build_directory}` already exists..."
-
-            KAMAL.builder.clone_reset_steps.each { |step| execute *step }
-          else
-            raise
-          end
-        end
-      end
-    end
 end

lib/kamal/cli/build/clone.rb

@@ -0,0 +1,61 @@
+require "uri"
+
+class Kamal::Cli::Build::Clone
+  attr_reader :sshkit
+  delegate :info, :error, :execute, :capture_with_info, to: :sshkit
+
+  def initialize(sshkit)
+    @sshkit = sshkit
+  end
+
+  def prepare
+    begin
+      clone_repo
+    rescue SSHKit::Command::Failed => e
+      if e.message =~ /already exists and is not an empty directory/
+        reset
+      else
+        raise Kamal::Cli::Build::BuildError, "Failed to clone repo: #{e.message}"
+      end
+    end
+
+    validate!
+  rescue Kamal::Cli::Build::BuildError => e
+    error "Error preparing clone: #{e.message}, deleting and retrying..."
+
+    FileUtils.rm_rf KAMAL.config.builder.clone_directory
+    clone_repo
+    validate!
+  end
+
+  private
+    def clone_repo
+      info "Cloning repo into build directory `#{KAMAL.config.builder.build_directory}`..."
+
+      FileUtils.mkdir_p KAMAL.config.builder.clone_directory
+      execute *KAMAL.builder.clone
+    end
+
+    def reset
+      info "Resetting local clone as `#{KAMAL.config.builder.build_directory}` already exists..."
+
+      KAMAL.builder.clone_reset_steps.each { |step| execute *step }
+    rescue SSHKit::Command::Failed => e
+      raise Kamal::Cli::Build::BuildError, "Failed to clone repo: #{e.message}"
+    end
+
+    def validate!
+      status = capture_with_info(*KAMAL.builder.clone_status).strip
+
+      unless status.empty?
+        raise Kamal::Cli::Build::BuildError, "Clone in #{KAMAL.config.builder.build_directory} is dirty, #{status}"
+      end
+
+      revision = capture_with_info(*KAMAL.builder.clone_revision).strip
+      if revision != Kamal::Git.revision
+        raise Kamal::Cli::Build::BuildError, "Clone in #{KAMAL.config.builder.build_directory} is not on the correct revision, expected `#{Kamal::Git.revision}` but got `#{revision}`"
+      end
+    rescue SSHKit::Command::Failed => e
+      raise Kamal::Cli::Build::BuildError, "Failed to validate clone: #{e.message}"
+    end
+end

lib/kamal/commands/builder.rb

@@ -2,7 +2,8 @@
 
 class Kamal::Commands::Builder < Kamal::Commands::Base
   delegate :create, :remove, :push, :clean, :pull, :info, :validate_image, to: :target
-  delegate :clone_directory, :build_directory, to: :"config.builder"
+
+  include Clone
 
   def name
     target.class.to_s.remove("Kamal::Commands::Builder::").underscore.inquiry
@@ -54,23 +55,6 @@ def ensure_local_dependencies_installed
     end
   end
 
-  def create_clone_directory
-    make_directory clone_directory
-  end
-
-  def clone
-    git :clone, Kamal::Git.root, path: clone_directory
-  end
-
-  def clone_reset_steps
-    [
-      git(:remote, "set-url", :origin, Kamal::Git.root, path: build_directory),
-      git(:fetch, :origin, path: build_directory),
-      git(:reset, "--hard", Kamal::Git.revision, path: build_directory),
-      git(:clean, "-fdx", path: build_directory)
-    ]
-  end
-
   private
     def ensure_local_docker_installed
       docker "--version"

lib/kamal/commands/builder/clone.rb

@@ -0,0 +1,28 @@
+module Kamal::Commands::Builder::Clone
+  extend ActiveSupport::Concern
+
+  included do
+    delegate :clone_directory, :build_directory, to: :"config.builder"
+  end
+
+  def clone
+    git :clone, Kamal::Git.root, path: clone_directory
+  end
+
+  def clone_reset_steps
+    [
+      git(:remote, "set-url", :origin, Kamal::Git.root, path: build_directory),
+      git(:fetch, :origin, path: build_directory),
+      git(:reset, "--hard", Kamal::Git.revision, path: build_directory),
+      git(:clean, "-fdx", path: build_directory)
+    ]
+  end
+
+  def clone_status
+    git :status, "--porcelain", path: build_directory
+  end
+
+  def clone_revision
+    git :"rev-parse", :HEAD, path: build_directory
+  end
+end

test/cli/build_test.rb

@@ -9,10 +9,18 @@ class CliBuildTest < CliTestCase
   end
 
   test "push" do
-    with_build_directory do
+    with_build_directory do |build_directory|
       Kamal::Commands::Hook.any_instance.stubs(:hook_exists?).returns(true)
       hook_variables = { version: 999, service_version: "app@999", hosts: "1.1.1.1,1.1.1.2,1.1.1.3,1.1.1.4", command: "build", subcommand: "push" }
 
+      SSHKit::Backend::Abstract.any_instance.expects(:capture_with_info)
+        .with(:git, "-C", anything, :"rev-parse", :HEAD)
+        .returns(Kamal::Git.revision)
+
+      SSHKit::Backend::Abstract.any_instance.expects(:capture_with_info)
+        .with(:git, "-C", anything, :status, "--porcelain")
+        .returns("")
+
       run_command("push", "--verbose").tap do |output|
         assert_hook_ran "pre-build", output, **hook_variables
         assert_match /Cloning repo into build directory/, output
@@ -23,28 +31,33 @@ class CliBuildTest < CliTestCase
     end
   end
 
-  test "push reseting clone" do
-    with_build_directory do
+  test "push resetting clone" do
+    with_build_directory do |build_directory|
       stub_setup
-      build_dir = "#{Dir.tmpdir}/kamal-clones/app-#{pwd_sha}/kamal/"
 
-      SSHKit::Backend::Abstract.any_instance.stubs(:execute).with(:docker, "--version", "&&", :docker, :buildx, "version")
-      SSHKit::Backend::Abstract.any_instance.stubs(:execute).with(:docker, :buildx, :create, "--use", "--name", "kamal-app-multiarch")
+      SSHKit::Backend::Abstract.any_instance.expects(:execute).with(:docker, "--version", "&&", :docker, :buildx, "version")
 
-      SSHKit::Backend::Abstract.any_instance.stubs(:execute).with(:mkdir, "-p", "#{Dir.tmpdir}/kamal-clones/app-#{pwd_sha}")
-      SSHKit::Backend::Abstract.any_instance.stubs(:execute)
+      SSHKit::Backend::Abstract.any_instance.expects(:execute)
         .with(:git, "-C", "#{Dir.tmpdir}/kamal-clones/app-#{pwd_sha}", :clone, Dir.pwd)
         .raises(SSHKit::Command::Failed.new("fatal: destination path 'kamal' already exists and is not an empty directory"))
         .then
         .returns(true)
-      SSHKit::Backend::Abstract.any_instance.stubs(:execute).with(:git, "-C", build_dir, :remote, "set-url", :origin, Dir.pwd)
-      SSHKit::Backend::Abstract.any_instance.stubs(:execute).with(:git, "-C", build_dir, :fetch, :origin)
-      SSHKit::Backend::Abstract.any_instance.stubs(:execute).with(:git, "-C", build_dir, :reset, "--hard", Kamal::Git.revision)
-      SSHKit::Backend::Abstract.any_instance.stubs(:execute).with(:git, "-C", build_dir, :clean, "-fdx")
+      SSHKit::Backend::Abstract.any_instance.expects(:execute).with(:git, "-C", build_directory, :remote, "set-url", :origin, Dir.pwd)
+      SSHKit::Backend::Abstract.any_instance.expects(:execute).with(:git, "-C", build_directory, :fetch, :origin)
+      SSHKit::Backend::Abstract.any_instance.expects(:execute).with(:git, "-C", build_directory, :reset, "--hard", Kamal::Git.revision)
+      SSHKit::Backend::Abstract.any_instance.expects(:execute).with(:git, "-C", build_directory, :clean, "-fdx")
 
-      SSHKit::Backend::Abstract.any_instance.stubs(:execute)
+      SSHKit::Backend::Abstract.any_instance.expects(:execute)
         .with(:docker, :buildx, :build, "--push", "--platform", "linux/amd64,linux/arm64", "--builder", "kamal-app-multiarch", "-t", "dhh/app:999", "-t", "dhh/app:latest", "--label", "service=\"app\"", "--file", "Dockerfile", ".")
 
+      SSHKit::Backend::Abstract.any_instance.expects(:capture_with_info)
+        .with(:git, "-C", anything, :"rev-parse", :HEAD)
+        .returns(Kamal::Git.revision)
+
+      SSHKit::Backend::Abstract.any_instance.expects(:capture_with_info)
+        .with(:git, "-C", anything, :status, "--porcelain")
+        .returns("")
+
       run_command("push", "--verbose").tap do |output|
         assert_match /Cloning repo into build directory/, output
         assert_match /Resetting local clone/, output
@@ -64,25 +77,65 @@ class CliBuildTest < CliTestCase
     end
   end
 
+  test "push with corrupt clone" do
+    with_build_directory do |build_directory|
+      stub_setup
+
+      SSHKit::Backend::Abstract.any_instance.expects(:execute).with(:docker, "--version", "&&", :docker, :buildx, "version")
+
+      SSHKit::Backend::Abstract.any_instance.expects(:execute)
+        .with(:git, "-C", "#{Dir.tmpdir}/kamal-clones/app-#{pwd_sha}", :clone, Dir.pwd)
+        .raises(SSHKit::Command::Failed.new("fatal: destination path 'kamal' already exists and is not an empty directory"))
+        .then
+        .returns(true)
+        .twice
+
+      SSHKit::Backend::Abstract.any_instance.expects(:execute).with(:git, "-C", build_directory, :remote, "set-url", :origin, Dir.pwd)
+        .raises(SSHKit::Command::Failed.new("fatal: not a git repository"))
+
+      SSHKit::Backend::Abstract.any_instance.expects(:capture_with_info)
+        .with(:git, "-C", anything, :"rev-parse", :HEAD)
+        .returns(Kamal::Git.revision)
+
+      SSHKit::Backend::Abstract.any_instance.expects(:capture_with_info)
+        .with(:git, "-C", anything, :status, "--porcelain")
+        .returns("")
+
+      Dir.stubs(:chdir)
+
+      run_command("push", "--verbose") do |output|
+        assert_match /Cloning repo into build directory `#{build_directory}`\.\.\..*Cloning repo into build directory `#{build_directory}`\.\.\./, output
+        assert_match "Resetting local clone as `#{build_directory}` already exists...", output
+        assert_match "Error preparing clone: Failed to clone repo: fatal: not a git repository, deleting and retrying...", output
+      end
+    end
+  end
+
   test "push without builder" do
-    with_build_directory do
+    with_build_directory do |build_directory|
       stub_setup
 
-      SSHKit::Backend::Abstract.any_instance.stubs(:execute)
+      SSHKit::Backend::Abstract.any_instance.expects(:execute)
         .with(:docker, "--version", "&&", :docker, :buildx, "version")
 
-      SSHKit::Backend::Abstract.any_instance.stubs(:execute)
+      SSHKit::Backend::Abstract.any_instance.expects(:execute)
         .with(:docker, :buildx, :create, "--use", "--name", "kamal-app-multiarch")
 
-      SSHKit::Backend::Abstract.any_instance.stubs(:execute)
+      SSHKit::Backend::Abstract.any_instance.expects(:execute)
         .with { |*args| args[0..1] == [ :docker, :buildx ] }
         .raises(SSHKit::Command::Failed.new("no builder"))
         .then
         .returns(true)
 
-      SSHKit::Backend::Abstract.any_instance.stubs(:execute).with { |*args| args.first.start_with?("git") }
+      SSHKit::Backend::Abstract.any_instance.expects(:execute).with { |*args| args.first.start_with?("git") }
+
+      SSHKit::Backend::Abstract.any_instance.expects(:capture_with_info)
+        .with(:git, "-C", anything, :"rev-parse", :HEAD)
+        .returns(Kamal::Git.revision)
 
-      SSHKit::Backend::Abstract.any_instance.stubs(:execute).with(:mkdir, "-p", "#{Dir.tmpdir}/kamal-clones/app-#{pwd_sha}")
+      SSHKit::Backend::Abstract.any_instance.expects(:capture_with_info)
+        .with(:git, "-C", anything, :status, "--porcelain")
+        .returns("")
 
       run_command("push").tap do |output|
         assert_match /WARN Missing compatible builder, so creating a new one first/, output
@@ -183,7 +236,7 @@ def with_build_directory
       build_directory = File.join Dir.tmpdir, "kamal-clones", "app-#{pwd_sha}", "kamal"
       FileUtils.mkdir_p build_directory
       FileUtils.touch File.join build_directory, "Dockerfile"
-      yield
+      yield build_directory + "/"
     ensure
       FileUtils.rm_rf build_directory
     end

test/cli/main_test.rb

@@ -108,6 +108,14 @@ class CliMainTest < CliTestCase
     SSHKit::Backend::Abstract.any_instance.expects(:capture_with_debug)
       .with(:stat, ".kamal/locks/app", ">", "/dev/null", "&&", :cat, ".kamal/locks/app/details", "|", :base64, "-d")
 
+    SSHKit::Backend::Abstract.any_instance.expects(:capture_with_info)
+      .with(:git, "-C", anything, :"rev-parse", :HEAD)
+      .returns(Kamal::Git.revision)
+
+    SSHKit::Backend::Abstract.any_instance.expects(:capture_with_info)
+      .with(:git, "-C", anything, :status, "--porcelain")
+      .returns("")
+
     assert_raises(Kamal::Cli::LockError) do
       run_command("deploy")
     end
@@ -129,6 +137,14 @@ class CliMainTest < CliTestCase
       .with { |*arg| arg[0..1] == [ :mkdir, ".kamal/locks/app" ] }
       .raises(SocketError, "getaddrinfo: nodename nor servname provided, or not known")
 
+    SSHKit::Backend::Abstract.any_instance.expects(:capture_with_info)
+      .with(:git, "-C", anything, :"rev-parse", :HEAD)
+      .returns(Kamal::Git.revision)
+
+    SSHKit::Backend::Abstract.any_instance.expects(:capture_with_info)
+      .with(:git, "-C", anything, :status, "--porcelain")
+      .returns("")
+
     assert_raises(SSHKit::Runner::ExecuteError) do
       run_command("deploy")
     end