Better error handling for scp uploads (#161)

* Better error handling for scp uploads

* Update handling upload warnings

Continue to collect warnings instead of stopping at the first.

Co-authored-by: Max Mulatz <[email protected]>

Author: andreasknoepfle

lib/sshkit/scp/upload.ex

@@ -107,6 +107,9 @@ defmodule SSHKit.SCP.Upload do
         {:data, _, 0, data} ->
           handle_error_data(state, options, data)
 
+        {:data, _, 1, data} ->
+          fatal(options, state, data)
+
         {:exit_status, _, status} ->
           exited(options, state, status)
 
@@ -215,9 +218,13 @@ defmodule SSHKit.SCP.Upload do
     {:cont, {:error, "SCP channel closed before completing the transfer"}}
   end
 
-  defp warning(_, state = {name, cwd, stack, errs}, buffer) do
+  defp warning(options, {name, _file, _stat, cwd, stack, errs}, buffer) do
+    warning(options, {name, cwd, stack, errs}, buffer)
+  end
+
+  defp warning(options, state = {_name, cwd, stack, errs}, buffer) do
     if String.last(buffer) == "\n" do
-      {:cont, {name, cwd, stack, errs ++ [String.trim(buffer)]}}
+      next(options, cwd, stack, errs ++ [String.trim(buffer)])
     else
       {:cont, {:warning, state, buffer}}
     end

test/sshkit/scp/upload_test.exs

@@ -155,7 +155,6 @@ defmodule SSHKit.SCP.UploadTest do
       channel: channel
     } do
       error_msg = "error part 1 error part 2 error part 3"
-      {name, cwd, stack, _errs} = state
 
       msg1 = {:data, channel, 0, <<1, "error part 1 ">>}
       state1 = {:warning, state, "error part 1 "}
@@ -166,7 +165,33 @@ defmodule SSHKit.SCP.UploadTest do
       assert {:cont, state2} == handler.(msg2, state1)
 
       msg3 = {:data, channel, 0, <<"error part 3\n">>}
-      assert {:cont, {name, cwd, stack, [error_msg]}} == handler.(msg3, state2)
+      assert {:cont, _directive, {_name, _cwd, _stack, [^error_msg]}} = handler.(msg3, state2)
+    end
+
+    test "proceeds with next file in stack after warning", %{
+      upload: %Upload{handler: handler, state: state},
+      channel: channel
+    } do
+      {name, cwd, _stack, _errs} = state
+      msg = {:data, channel, 0, <<1, "error message\n">>}
+
+      assert {:cont, 'D0755 0 local_dir\n',
+              {name, cwd <> "/local_dir", [["other.txt"], []], ["error message"]}} ==
+               handler.(msg, state)
+    end
+
+    test "collects warning when in write state", %{
+      upload: %Upload{handler: handler, state: state},
+      channel: channel
+    } do
+      {name, cwd, stack, _errs} = state
+
+      state = {:write, "local.txt", %{}, cwd, stack, []}
+      msg = {:data, channel, 0, <<1, "error message\n">>}
+
+      assert {:cont, 'D0755 0 local_dir\n',
+              {name, cwd <> "/local_dir", [["other.txt"], []], ["error message"]}} ==
+               handler.(msg, state)
     end
 
     test "aggregates connection errors in the state and halts", %{

test/sshkit/scp_functional_test.exs

@@ -52,6 +52,17 @@ defmodule SSHKit.SCPFunctionalTest do
         assert verify_mtime(conn, local, remote)
       end)
     end
+
+    @tag boot: [@bootconf]
+    test "uploads to nonexistent target directory (recursive: true)", %{hosts: [host]} do
+      local = "test/fixtures/local_dir"
+      remote = "/some/nonexistent/destination"
+
+      SSH.connect(host.name, host.options, fn conn ->
+        assert {:error, msg} = SCP.upload(conn, local, remote, recursive: true)
+        assert msg =~ "scp: /some/nonexistent/destination: No such file or directory"
+      end)
+    end
   end
 
   describe "download/4" do

test/sshkit_functional_test.exs

@@ -159,6 +159,34 @@ defmodule SSHKitFunctionalTest do
       assert verify_transfer(context, local, Path.basename(local))
     end
 
+    test "uploads a file to a directory that does not exist", %{hosts: hosts} do
+      local = "test/fixtures/local.txt"
+
+      context =
+        hosts
+        |> SSHKit.context()
+        |> SSHKit.path("/otp/releases")
+
+      assert [
+               error: "sh: cd: line 1: can't cd to /otp/releases",
+               error: "sh: cd: line 1: can't cd to /otp/releases"
+             ] = SSHKit.upload(context, local)
+    end
+
+    test "uploads a file to a directory we have no access to", %{hosts: hosts} do
+      local = "test/fixtures/local.txt"
+
+      context =
+        hosts
+        |> SSHKit.context()
+        |> SSHKit.path("/")
+
+      assert [
+               error: "SCP exited with non-zero exit code 1: scp: local.txt: Permission denied",
+               error: "SCP exited with non-zero exit code 1: scp: local.txt: Permission denied"
+             ] = SSHKit.upload(context, local)
+    end
+
     test "recursive: true", %{hosts: [host | _] = hosts} do
       local = "test/fixtures"
       remote = "/home/#{host.options[:user]}/fixtures"