Add :tmp_dir application config setting #145

cbecker · 2026-01-03T16:06:18Z

Allows to specify where to save temporary files to. See #144 for more details

achempion

Thank you for submitting the PR. I can merge it after these changes

use :tmp_dir option name
have a test case covering both TMPDIR and :tmp_dir options (search for test "recv_timeout" do in the codebase for a setup example)
update documentation for the new option

cbecker · 2026-01-15T21:54:59Z

Thanks. I improved the tests in local_test.ex, and did the other items.

I am not sure what you mean with testing for both cases for tests in store_test.ex, where the recv_timeout tests are, because those tests don't use the temp storage, so nothing fails if, for example, the test storage is set to a path that cannot be written. Do you mean to change a different suite of tests?

test/storage/local_test.exs

achempion

Thanks for the update, here are couple of things that I noticed

the testing approach is too complicated
documentation can live inside the defmodule Waffle.Definition.Storage module instead of readme

I am not sure what you mean with testing for both cases for tests in store_test.ex

I linked an abstract example that tests a configuration option.

cbecker · 2026-01-16T08:56:25Z

Thanks.

Ideally, docs belong to Waffle.File, but I added them to Definition.Storage now, removed from README.md
Removed the testing approach you said it's too complex
Added a test, but I am still not sure it's what you want. Let me know. I still believe having local_storage tests as I did earlier (the first commit) is important, but I'm happy to go for what you think is best.

achempion

looking good, left couple of comments

achempion · 2026-01-16T09:06:51Z

lib/waffle/definition/storage.ex

+        tmp_dir: "/path/to/custom/tmp"
+
+  Waffle may fail to remove temporary files if the process using them crashes.
+  Having a periodic cleanup process or using a custom temporary directory is recommended.


nit: can we please remove this line (122)?

achempion · 2026-01-16T09:07:22Z

lib/waffle/file.ex

+  @doc """
+  Directory used for temporary files.
+  """
+  def temp_dir() do


nit: defp tmp_dir()

can we keep it public? then I can easily read the exact temp dir from Waffle in my cleanup genserver. (we can read it from the application env, but may be handy to have this public)

achempion · 2026-01-16T09:07:31Z

lib/waffle/file.ex

+    Path.join(temp_dir(), file_name)
+  end
+
+  @doc """


can skip on doc here

achempion · 2026-01-16T09:09:31Z

test/actions/store_test.exs

+
+    with_mock Waffle.Storage.S3,
+      put: fn DummyDefinition, _, {%{file_name: "favicon.ico", path: path}, nil} ->
+        String.starts_with?(path, tmp_dir)


Will test still pass if key configuration Application.put_env(:waffle, :tmp_dir, tmp_dir) is removed from it?

if we leave the default env :tmp_dir (nil), then this test fails, because the file will be written to /tmp instead of waffletest/store_tmp

With which error it fails with? I can't see any asserts here

ah no sorry- missed that. added assert here and in the other test where it was neither done

achempion · 2026-01-16T09:11:20Z

test/actions/store_test.exs

    Application.put_env(:waffle, :recv_timeout, 5_000)
  end

+  test "custom tmp_dir used for temp files and succeeds if exists" do


Can we do a simpler approach with having a simple unit test for generate_temporary_path/1 function insdie the file_test.exs file (we would need to create it)?

I can't find the file_test.exs in the PR

forgot to git add, done now

achempion · 2026-01-16T09:11:41Z

lib/waffle/file.ex

      |> Kernel.<>(string_extension)

-    Path.join(System.tmp_dir(), file_name)
+    Path.join(temp_dir(), file_name)


tmp_dir()

cbecker · 2026-01-16T09:51:40Z

Thanks for the feedback, ready for another round

achempion · 2026-01-16T11:32:09Z

test/actions/store_test.exs

    Application.put_env(:waffle, :recv_timeout, 5_000)
  end

+  test "custom tmp_dir used for temp files and succeeds if exists" do


I can't find the file_test.exs in the PR

achempion · 2026-01-16T11:32:50Z

test/actions/store_test.exs

+
+    with_mock Waffle.Storage.S3,
+      put: fn DummyDefinition, _, {%{file_name: "favicon.ico", path: path}, nil} ->
+        String.starts_with?(path, tmp_dir)


With which error it fails with? I can't see any asserts here

achempion · 2026-01-16T11:33:40Z

test/actions/store_test.exs

+    File.rm_rf!(tmp_dir)
+  end
+
+  test "default tmp_dir used for temp files and succeeds if exists" do


we can skp on updating the store_test as file_test should be enough for this

let me know if you want to remove the store tests. I think it doesn't hurt to have them.

I would like to remove them to reduce test complexity

ok, reverted back to master

achempion · 2026-01-16T11:34:30Z

test/storage/local_test.exs

    File.mkdir_p("waffletest/uploads")
    File.mkdir_p("waffletest/tmp")
-    System.put_env("TMPDIR", "waffletest/tmp")
+    Application.put_env(:waffle, :tmp_dir, "waffletest/tmp")


I would like to keep the default tmpdir setting

ok, reverted

achempion

left some comments, plrease remove the tests from store_test

achempion · 2026-01-16T12:03:47Z

test/file_test.exs

@@ -0,0 +1,38 @@
+defmodule WaffleTest.File do
+  use ExUnit.Case


should be async: false to not overlap with other tests on global state changes

yes, default is false from ExUnit.Case's docs. Anyway specified now explicitly.

* `:async` - configures tests in this module to run concurrently with tests in other modules. Tests in the same module never run concurrently. It should be enabled only if tests do not change any global state. Defaults to `false`.

achempion · 2026-01-16T12:04:29Z

test/file_test.exs

+  describe "generate_temporary_path/1" do
+    test "uses configured tmp_dir" do
+      previous = Application.get_env(:waffle, :tmp_dir, :not_set)
+      custom_tmp = System.tmp_dir() <> "/waffle_test_custom"


can we move this inside the module attribute?

it's only used for this test, not in general, so I am not sure it makes sense. I did it anyway.

achempion · 2026-01-16T12:10:10Z

test/file_test.exs

+  use ExUnit.Case
+
+  describe "generate_temporary_path/1" do
+    test "uses configured tmp_dir" do


looks good, can we add a couple of changes

move path into the module attribute

use on_exit callback for the cleanup

the test flow can be simpler:

on_exit setup

put env

call function and assert on the same line

achempion

looking great, left a couple of comments

achempion · 2026-01-16T13:26:45Z

test/actions/store_test.exs

+    File.rm_rf!(tmp_dir)
+  end
+
+  test "default tmp_dir used for temp files and succeeds if exists" do


I would like to remove them to reduce test complexity

achempion · 2026-01-16T13:29:09Z

test/file_test.exs

+  def setup do
+    File.mkdir_p!(@custom_tmpdir)
+
+    on_exit fn ->


as we need it only inside a single test, it can be written like this

test "uses configured tmp_dir" do File.mkdir_p!(@custom_tmpdir) on_exit fn -> Application.delete_env(:waffle, :tmp_dir) File.rm_rf(@custom_tmpdir) end Application.put_env(:waffle, :tmp_dir, @custom_tmpdir) assert Waffle.File.generate_temporary_path() |> String.starts_with?(@custom_tmpdir) end

ok. not sure it makes sense to keep the module attribute, as it's only used here now- kept for now, as it's what you wanted earlier. No setup function anymore for the module.

achempion · 2026-01-16T13:29:38Z

test/file_test.exs

+defmodule WaffleTest.File do
+  use ExUnit.Case, async: false
+
+  @custom_tmpdir System.tmp_dir() <> "/waffle_test_custom"


nit: @custom_tmp_dir

achempion · 2026-01-16T13:30:14Z

test/file_test.exs

+      assert Waffle.File.generate_temporary_path() |> String.starts_with?(@custom_tmpdir)
+    end
+
+    test "generates path in default tmp_dir when config not set" do


nit: "uses system tmp_dir"

achempion · 2026-01-16T13:35:37Z

lib/waffle/file.ex

+    Path.join(tmp_dir(), file_name)
+  end
+
+  def tmp_dir() do


Can we please make it private? Any public interface is hard to change once external apps/libraries will start using it

cbecker · 2026-01-16T14:46:59Z

Done now, ready for another round.

cbecker · 2026-01-17T15:34:53Z

@achempion I think it's ready, let me know what you think. thanks!

achempion

Looking great, please squash git commits so I can merge keeping the clean git history

achempion · 2026-01-17T17:04:41Z

test/file_test.exs

+      assert Waffle.File.generate_temporary_path() |> String.starts_with?(@custom_tmp_dir)
+      on_exit fn ->
+        Application.delete_env(:waffle, :tmp_dir)
+        File.rm_rf(@custom_tmp_dir)


nit: File.rm_rf!/1

achempion · 2026-01-17T17:07:28Z

please check the CI build message

cbecker · 2026-01-17T17:56:51Z

all good, should be ok now. I don't know how to make CI run, seems to not happen by default on new commits.

cbecker · 2026-01-18T20:43:32Z

@achempion thanks! Is it possible to create an Hex release with these changes?

achempion · 2026-01-18T20:46:31Z

Thanks for the PR. I'll do a release soon. You can pin the dependency by the git hash in `mix.ex` directly from github in the meantime.

cbecker mentioned this pull request Jan 15, 2026

Temp file cleanup on process crash, or at least specify temp path #144

Closed

achempion reviewed Jan 15, 2026

View reviewed changes

cbecker commented Jan 15, 2026

View reviewed changes

test/storage/local_test.exs Show resolved Hide resolved

achempion reviewed Jan 15, 2026

View reviewed changes

cbecker force-pushed the cb/add-temp-dir-config branch from ec8a055 to d31781f Compare January 16, 2026 08:56

achempion reviewed Jan 16, 2026

View reviewed changes

cbecker changed the title ~~Add :temp_dir application config setting~~ Add :tmp_dir application config setting Jan 16, 2026

achempion approved these changes Jan 16, 2026

View reviewed changes

achempion approved these changes Jan 17, 2026

View reviewed changes

Add :tmp_dir application config setting

f58ce94

cbecker force-pushed the cb/add-temp-dir-config branch from 8d28548 to f58ce94 Compare January 17, 2026 17:55

achempion merged commit 2d32220 into elixir-waffle:master Jan 18, 2026
1 check passed

Add :tmp_dir application config setting #145

Add :tmp_dir application config setting #145

Uh oh!

Conversation

cbecker commented Jan 3, 2026

Uh oh!

achempion left a comment

Choose a reason for hiding this comment

Uh oh!

cbecker commented Jan 15, 2026

Uh oh!

Uh oh!

achempion left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbecker commented Jan 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achempion left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbecker commented Jan 16, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achempion left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

achempion left a comment •

edited

Loading

cbecker commented Jan 16, 2026 •

edited

Loading