Clean up all temp files created during processing and storage

Author: dmarkow

lib/arc/actions/store.ex

@@ -20,11 +20,23 @@ defmodule Arc.Actions.Store do
   defp put(_definition, { error = {:error, _msg}, _scope}), do: error
   defp put(definition, {%Arc.File{}=file, scope}) do
     case definition.validate({file, scope}) do
-      true -> put_versions(definition, {file, scope})
+      true ->
+        put_versions(definition, {file, scope})
+        |> cleanup(file)
       _    -> {:error, :invalid_file}
     end
   end
 
+  defp cleanup(result, file) do
+    # If we were working with binary data or a remote file, a tempfile was
+    # created that we need to clean up.
+    if file.tempfile? do
+      File.rm(file.path)
+    end
+
+    result
+  end
+
   defp put_versions(definition, {file, scope}) do
     if definition.async do
       definition.__versions
@@ -81,6 +93,13 @@ defmodule Arc.Actions.Store do
         file_name = Arc.Definition.Versioning.resolve_file_name(definition, version, {file, scope})
         file      = %Arc.File{file | file_name: file_name}
         result = definition.__storage.put(definition, version, {file, scope})
+
+        # If this version was transformed in any way, a tempfile was created
+        # that we need to clean up.
+        if file.tempfile? do
+          File.rm(file.path)
+        end
+
         result
     end
   end

lib/arc/file.ex

@@ -1,5 +1,5 @@
 defmodule Arc.File do
-  defstruct [:path, :file_name, :binary]
+  defstruct [:path, :file_name, :binary, :tempfile?]
 
   def generate_temporary_path(file \\ nil) do
     extension = Path.extname((file && file.path) || "")
@@ -18,7 +18,7 @@ defmodule Arc.File do
     filename = Path.basename(uri.path)
 
     case save_file(uri, filename) do
-      {:ok, local_path} -> %Arc.File{path: local_path, file_name: filename}
+      {:ok, local_path} -> %Arc.File{path: local_path, file_name: filename, tempfile?: true}
       :error -> {:error, :invalid_file_path}
     end
 end
@@ -33,6 +33,7 @@ end
 
   def new(%{filename: filename, binary: binary}) do
     %Arc.File{binary: binary, file_name: Path.basename(filename)}
+    |> write_binary()
   end
 
   # Accepts a map conforming to %Plug.Upload{} syntax
@@ -43,16 +44,15 @@ end
     end
   end
 
-  def ensure_path(file = %{path: path}) when is_binary(path), do: file
-  def ensure_path(file = %{binary: binary}) when is_binary(binary), do: write_binary(file)
 
   defp write_binary(file) do
     path = generate_temporary_path(file)
     :ok = File.write!(path, file.binary)
 
     %__MODULE__{
       file_name: file.file_name,
-      path: path
+      path: path,
+      tempfile?: true
     }
   end
 

lib/arc/processor.ex

@@ -12,6 +12,6 @@ defmodule Arc.Processor do
   end
 
   defp apply_transformation(file, {cmd, conversion}) do
-    Arc.Transformations.Convert.apply(cmd, Arc.File.ensure_path(file), conversion)
+    Arc.Transformations.Convert.apply(cmd, file, conversion)
   end
 end

lib/arc/transformations/convert.ex

@@ -6,9 +6,10 @@ defmodule Arc.Transformations.Convert do
 
     ensure_executable_exists!(program)
 
-    case System.cmd(program, args_list(args), stderr_to_stdout: true) do
+    result = System.cmd(program, args_list(args), stderr_to_stdout: true)
+    case result do
       {_, 0} ->
-        {:ok, %Arc.File{file | path: new_path}}
+        {:ok, %Arc.File{file | path: new_path, tempfile?: true}}
       {error_message, _exit_code} ->
         {:error, error_message}
     end

mix.lock

@@ -8,10 +8,10 @@
   "idna": {:hex, :idna, "6.0.0", "689c46cbcdf3524c44d5f3dde8001f364cd7608a99556d8fbd8239a5798d4c10", [:rebar3], [{:unicode_util_compat, "0.4.1", [hex: :unicode_util_compat, repo: "hexpm", optional: false]}], "hexpm"},
   "makeup": {:hex, :makeup, "0.5.5", "9e08dfc45280c5684d771ad58159f718a7b5788596099bdfb0284597d368a882", [:mix], [{:nimble_parsec, "~> 0.4", [hex: :nimble_parsec, repo: "hexpm", optional: false]}], "hexpm"},
   "makeup_elixir": {:hex, :makeup_elixir, "0.10.0", "0f09c2ddf352887a956d84f8f7e702111122ca32fbbc84c2f0569b8b65cbf7fa", [:mix], [{:makeup, "~> 0.5.5", [hex: :makeup, repo: "hexpm", optional: false]}], "hexpm"},
-  "meck": {:hex, :meck, "0.8.12", "1f7b1a9f5d12c511848fec26bbefd09a21e1432eadb8982d9a8aceb9891a3cf2", [:rebar3], [], "hexpm"},
+  "meck": {:hex, :meck, "0.8.13", "ffedb39f99b0b99703b8601c6f17c7f76313ee12de6b646e671e3188401f7866", [:rebar3], [], "hexpm"},
   "metrics": {:hex, :metrics, "1.0.1", "25f094dea2cda98213cecc3aeff09e940299d950904393b2a29d191c346a8486", [:rebar3], [], "hexpm"},
   "mimerl": {:hex, :mimerl, "1.0.2", "993f9b0e084083405ed8252b99460c4f0563e41729ab42d9074fd5e52439be88", [:rebar3], [], "hexpm"},
-  "mock": {:hex, :mock, "0.3.2", "e98e998fd76c191c7e1a9557c8617912c53df3d4a6132f561eb762b699ef59fa", [:mix], [{:meck, "~> 0.8.8", [hex: :meck, repo: "hexpm", optional: false]}], "hexpm"},
+  "mock": {:hex, :mock, "0.3.3", "42a433794b1291a9cf1525c6d26b38e039e0d3a360732b5e467bfc77ef26c914", [:mix], [{:meck, "~> 0.8.13", [hex: :meck, repo: "hexpm", optional: false]}], "hexpm"},
   "nimble_parsec": {:hex, :nimble_parsec, "0.4.0", "ee261bb53214943679422be70f1658fff573c5d0b0a1ecd0f18738944f818efe", [:mix], [], "hexpm"},
   "parse_trans": {:hex, :parse_trans, "3.3.0", "09765507a3c7590a784615cfd421d101aec25098d50b89d7aa1d66646bc571c1", [:rebar3], [], "hexpm"},
   "poison": {:hex, :poison, "3.1.0", "d9eb636610e096f86f25d9a46f35a9facac35609a7591b3be3326e99a0484665", [:mix], [], "hexpm"},

test/storage/local_test.exs

@@ -3,11 +3,14 @@ defmodule ArcTest.Storage.Local do
   @img "test/support/image.png"
   @badimg "test/support/invalid_image.png"
 
-  setup_all do
+  setup do
     File.mkdir_p("arctest/uploads")
+    File.mkdir_p("arctest/tmp")
+    System.put_env("TMPDIR", "arctest/tmp")
 
     on_exit fn ->
       File.rm_rf("arctest/uploads")
+      File.rm_rf("arctest/tmp")
     end
   end
 
@@ -68,4 +71,23 @@ defmodule ArcTest.Storage.Local do
     assert !File.exists?("arctest/uploads/original-#{filename}")
     assert !File.exists?("arctest/uploads/1/thumb-#{filename}")
   end
+
+  test "temp files from processing are cleaned up" do
+    filepath = @img
+    DummyDefinition.store(filepath)
+    assert length(File.ls!("arctest/tmp")) == 0
+  end
+
+  test "temp files from handling binary data are cleaned up" do
+    filepath = @img
+    filename = "image.png"
+    DummyDefinition.store(%{binary: File.read!(filepath), filename: filename})
+    assert File.exists?("arctest/uploads/original-#{filename}")
+    assert length(File.ls!("arctest/tmp")) == 0
+  end
+
+  test "temp files from handling remote URLs are cleaned up" do
+    DummyDefinition.store("https://www.google.com/favicon.ico")
+    assert length(File.ls!("arctest/tmp")) == 0
+  end
 end