fix: fixed CSM-009

CODESMELL.md

@@ -167,19 +167,22 @@
 
 ---
 
-### CSM-009 — Thumbnail Generation: Missing Error Handling
+### ~~CSM-009 — Thumbnail Generation: Missing Error Handling~~ ✅ FIXED
-- **File:** `lib/bds/media/thumbnails.ex:107-165`
+- **Fixed:** 2026-05-09
-- **What:**
+- **What was done:**
-  - `Image.thumbnail!` (Z. 149,154) and `Image.write!` (Z. 159,164) are bang variants that crash on failure.
+  - Replaced all bang variants with non-bang error-tuple handling:
-  - `File.mkdir_p/1` (Z. 133) result is discarded — if directory creation fails, the write will crash with a confusing error.
+    - `Image.autorotate!` → `Image.autorotate` with `{:ok, {image, rotation_info}}` destructuring.
-  - `Image.embed!` (Z. 150) and `Image.flatten!` (Z. 158) are also bang variants.
+    - `Image.thumbnail!` → `Image.thumbnail` returning `{:ok, image}` / `{:error, reason}`.
-- **Note on scheduler blocking:** The `Image` library uses libvips NIFs which run image processing in a C thread pool, not on the BEAM scheduler. The original concern about scheduler blocking is **not substantiated** for this library. The real issues are the bang variants and unchecked `File.mkdir_p`.
+    - `Image.embed!` → `Image.embed` with `with` chain.
-- **Fix:**
+    - `Image.flatten!` → `Image.flatten` with `with` chain.
-  - Replace `File.mkdir_p/1` with `case File.mkdir_p(dir) do :ok -> ...; {:error, reason} -> {:error, reason}`.
+    - `Image.write!` → `Image.write` with `{:ok, _}` / `{:error, reason}` handling.
-  - Replace bang `Image.thumbnail!` with `Image.thumbnail` and handle `{:error, _}`.
+  - `File.mkdir_p` result is now checked — errors halt thumbnail generation with `{:error, reason}`.
-  - Replace bang `Image.write!` with `Image.write` and handle `{:error, _}`.
+  - `write_all_thumbnails` uses `Enum.reduce_while` to stop on first error and return `{:error, reason}`.
-  - Wrap the whole `write_all_thumbnails` in a try/return-error-tuple pattern.
+  - `ensure_thumbnails` spec updated to `:ok | {:error, term()}`.
-- **Test:** Feed a corrupt image; assert `ensure_thumbnails` returns `{:error, _}` instead of crashing.
+  - `regenerate_thumbnails` propagates `{:error, reason}` from `ensure_thumbnails`.
+  - `regenerate_missing_thumbnails` replaced `try/rescue` with `case` on the new error tuples.
+  - Call sites in `BDS.Media` (`import_media`, `replace_media_binary`) use `log_thumbnail_error/2` — media operations succeed even if thumbnails fail, with a warning logged.
+  - Added 6 tests in `test/bds/csm009_thumbnail_error_handling_test.exs`: corrupt image returns `{:error, _}`, non-image returns `:ok`, missing source returns `{:error, _}`, regenerate corrupt returns `{:error, _}`, regenerate_missing counts failures, import succeeds despite thumbnail failure.
 
 ---
 
@@ -431,6 +434,7 @@
   - CSM-006: Fixed. Batch INSERT for reindexing, preloaded post records for rendering.
   - CSM-007: Fixed. Decomposed into refresh_layout, refresh_sidebar, refresh_content, reload_shell.
   - CSM-008: Fixed. Panel data pre-computed in event handlers, tab meta skips DB for complete entries.
+  - CSM-009: Fixed. All bang Image/File variants replaced with error-tuple handling, `ensure_thumbnails` returns `{:error, _}` instead of crashing.
 - [x] CSM-001 fix covers ALL 6 affected files, not just `import_definitions.ex`.
 - [x] CSM-003 fix covers ALL `Repo.delete!` call sites (posts, tags, scripts, media, projects, templates, translations).
 - [x] CSM-007 decomposition is the prerequisite for fixing CSM-008 (render-path queries).

lib/bds/media.ex

@@ -24,6 +24,8 @@ defmodule BDS.Media do
       ensure_thumbnails: 2
     ]
 
+  require Logger
+
   import Ecto.Query
 
   alias BDS.Media.Media
@@ -105,7 +107,7 @@ defmodule BDS.Media do
          end) do
       {:ok, media} ->
         :ok = write_sidecar(project, media)
-        :ok = ensure_thumbnails(project, media)
+        log_thumbnail_error(ensure_thumbnails(project, media), media.id)
         :ok = Search.sync_media(media)
         {:ok, media}
 
@@ -321,7 +323,7 @@ defmodule BDS.Media do
               {:ok, updated_media} ->
                 _ = File.rm(previous_destination_backup)
                 :ok = write_sidecar(project, updated_media)
-                :ok = ensure_thumbnails(project, updated_media)
+                log_thumbnail_error(ensure_thumbnails(project, updated_media), updated_media.id)
                 :ok = Search.sync_media(updated_media)
                 {:ok, updated_media}
 
@@ -342,4 +344,10 @@ defmodule BDS.Media do
         order_by: [asc: translation.language]
     )
   end
+
+  defp log_thumbnail_error(:ok, _media_id), do: :ok
+
+  defp log_thumbnail_error({:error, reason}, media_id) do
+    Logger.warning("Thumbnail generation failed for media #{media_id}: #{inspect(reason)}")
+  end
 end

lib/bds/media/thumbnails.ex

@@ -38,8 +38,11 @@ defmodule BDS.Media.Thumbnails do
 
       media ->
         project = Projects.get_project!(media.project_id)
-        :ok = ensure_thumbnails(project, media)
+
-        {:ok, media}
+        case ensure_thumbnails(project, media) do
+          :ok -> {:ok, media}
+          {:error, reason} -> {:error, reason}
+        end
     end
   end
 
@@ -79,16 +82,15 @@ defmodule BDS.Media.Thumbnails do
         if missing_paths == [] do
           %{acc | processed: acc.processed + 1}
         else
-          try do
+          case ensure_thumbnails(project, media) do
-            :ok = ensure_thumbnails(project, media)
+            :ok ->
+              %{
+                processed: acc.processed + 1,
+                generated: acc.generated + length(missing_paths),
+                failed: acc.failed
+              }
 
-            %{
+            {:error, _reason} ->
-              processed: acc.processed + 1,
-              generated: acc.generated + length(missing_paths),
-              failed: acc.failed
-            }
-          rescue
-            _error ->
               %{acc | processed: acc.processed + 1, failed: acc.failed + 1}
           end
         end
@@ -98,23 +100,19 @@ defmodule BDS.Media.Thumbnails do
     end)
   end
 
-  @spec ensure_thumbnails(BDS.Projects.Project.t(), Media.t()) :: :ok
+  @spec ensure_thumbnails(BDS.Projects.Project.t(), Media.t()) :: :ok | {:error, term()}
   def ensure_thumbnails(project, media) do
     if image_mime?(media.mime_type) do
       source_path = Path.join(Projects.project_data_dir(project), media.file_path)
 
-      case Image.open(source_path) do
+      with {:ok, image} <- Image.open(source_path),
-        {:ok, image} ->
+           {:ok, {rotated, _rotation_info}} <- Image.autorotate(image),
-          image
+           :ok <- write_all_thumbnails(rotated, project, media) do
-          |> Image.autorotate!()
+        :ok
-          |> write_all_thumbnails(project, media)
-
-        {:error, _reason} ->
-          :ok
       end
+    else
+      :ok
     end
-
-    :ok
   end
 
   @spec delete_thumbnail_files(String.t(), Media.t()) :: :ok
@@ -128,16 +126,17 @@ defmodule BDS.Media.Thumbnails do
 
   defp write_all_thumbnails(image, project, media) do
     thumbnail_paths(media)
-    |> Enum.each(fn {size, relative_path} ->
+    |> Enum.reduce_while(:ok, fn {size, relative_path}, :ok ->
       destination = Path.join(Projects.project_data_dir(project), relative_path)
-      :ok = File.mkdir_p(Path.dirname(destination))
 
-      image
+      with :ok <- File.mkdir_p(Path.dirname(destination)),
-      |> render_thumbnail(size)
+           {:ok, rendered} <- render_thumbnail(image, size),
-      |> write_thumbnail(destination, size)
+           :ok <- write_thumbnail(rendered, destination, size) do
+        {:cont, :ok}
+      else
+        {:error, reason} -> {:halt, {:error, reason}}
+      end
     end)
-
-    :ok
   end
 
   defp render_thumbnail(image, :small), do: bounded_thumbnail(image, 150, 150)
@@ -145,23 +144,41 @@ defmodule BDS.Media.Thumbnails do
   defp render_thumbnail(image, :large), do: bounded_thumbnail(image, 800, 800)
 
   defp render_thumbnail(image, :ai) do
-    image
+    with {:ok, thumbnail} <-
-    |> Image.thumbnail!("448x448", fit: :contain, resize: :both, autorotate: false)
+           Image.thumbnail(image, "448x448",
-    |> Image.embed!(448, 448, x: :center, y: :center, background_color: :black)
+             fit: :contain,
+             resize: :both,
+             autorotate: false
+           ),
+         {:ok, embedded} <-
+           Image.embed(thumbnail, 448, 448,
+             x: :center,
+             y: :center,
+             background_color: :black
+           ) do
+      {:ok, embedded}
+    end
   end
 
   defp bounded_thumbnail(image, width, height) do
-    Image.thumbnail!(image, "#{width}x#{height}", fit: :contain, resize: :down, autorotate: false)
+    Image.thumbnail(image, "#{width}x#{height}",
+      fit: :contain,
+      resize: :down,
+      autorotate: false
+    )
   end
 
   defp write_thumbnail(image, destination, :ai) do
-    flattened = Image.flatten!(image, background_color: :black)
+    with {:ok, flattened} <- Image.flatten(image, background_color: :black),
-    Image.write!(flattened, destination, quality: 85, strip_metadata: true)
+         {:ok, _} <- Image.write(flattened, destination, quality: 85, strip_metadata: true) do
-    :ok
+      :ok
+    end
   end
 
   defp write_thumbnail(image, destination, _size) do
-    Image.write!(image, destination, quality: 80, strip_metadata: true)
+    case Image.write(image, destination, quality: 80, strip_metadata: true) do
-    :ok
+      {:ok, _} -> :ok
+      {:error, reason} -> {:error, reason}
+    end
   end
 end

test/bds/csm009_thumbnail_error_handling_test.exs

@@ -0,0 +1,128 @@
+defmodule BDS.CSM009ThumbnailErrorHandlingTest do
+  use ExUnit.Case, async: false
+
+  alias BDS.Media.Thumbnails
+  alias BDS.Repo
+
+  setup do
+    :ok = Ecto.Adapters.SQL.Sandbox.checkout(BDS.Repo)
+    temp_dir = Path.join(System.tmp_dir!(), "bds-csm009-#{System.unique_integer([:positive])}")
+    File.mkdir_p!(temp_dir)
+    on_exit(fn -> File.rm_rf(temp_dir) end)
+
+    {:ok, project} = BDS.Projects.create_project(%{name: "CSM009", data_path: temp_dir})
+    %{project: project, temp_dir: temp_dir}
+  end
+
+  test "ensure_thumbnails returns {:error, _} for a corrupt image instead of crashing", %{
+    project: project,
+    temp_dir: temp_dir
+  } do
+    source_path = Path.join(temp_dir, "corrupt.jpg")
+    File.write!(source_path, "not a real image")
+
+    {:ok, media} =
+      BDS.Media.import_media(%{project_id: project.id, source_path: source_path})
+
+    assert media.mime_type == "image/jpeg"
+
+    Enum.each(Map.values(Thumbnails.thumbnail_paths(media)), fn path ->
+      File.rm(Path.join(temp_dir, path))
+    end)
+
+    result = Thumbnails.ensure_thumbnails(project, media)
+    assert {:error, _reason} = result
+  end
+
+  test "ensure_thumbnails returns :ok for non-image media", %{project: project, temp_dir: temp_dir} do
+    source_path = Path.join(temp_dir, "readme.txt")
+    File.write!(source_path, "just text")
+
+    {:ok, media} =
+      BDS.Media.import_media(%{project_id: project.id, source_path: source_path})
+
+    assert Thumbnails.ensure_thumbnails(project, media) == :ok
+  end
+
+  test "ensure_thumbnails returns {:error, _} when source file is missing", %{
+    project: project,
+    temp_dir: temp_dir
+  } do
+    source_path = Path.join(temp_dir, "sample.jpg")
+    File.write!(source_path, tiny_jpeg_binary())
+
+    {:ok, media} =
+      BDS.Media.import_media(%{project_id: project.id, source_path: source_path})
+
+    File.rm!(Path.join(temp_dir, media.file_path))
+
+    result = Thumbnails.ensure_thumbnails(project, media)
+    assert {:error, _reason} = result
+  end
+
+  test "regenerate_thumbnails returns {:error, _} for corrupt source", %{
+    project: project,
+    temp_dir: temp_dir
+  } do
+    source_path = Path.join(temp_dir, "corrupt2.jpg")
+    File.write!(source_path, "not valid jpeg data")
+
+    {:ok, media} =
+      BDS.Media.import_media(%{project_id: project.id, source_path: source_path})
+
+    Enum.each(Map.values(Thumbnails.thumbnail_paths(media)), fn path ->
+      File.rm(Path.join(temp_dir, path))
+    end)
+
+    File.write!(Path.join(temp_dir, media.file_path), "not valid jpeg data")
+
+    result = Thumbnails.regenerate_thumbnails(media.id)
+    assert {:error, _reason} = result
+  end
+
+  test "regenerate_missing_thumbnails counts failures without crashing", %{
+    project: project,
+    temp_dir: temp_dir
+  } do
+    source_path = Path.join(temp_dir, "good.jpg")
+    File.write!(source_path, tiny_jpeg_binary())
+    {:ok, good_media} = BDS.Media.import_media(%{project_id: project.id, source_path: source_path})
+
+    corrupt_path = Path.join(temp_dir, "bad.jpg")
+    File.write!(corrupt_path, "corrupt data")
+    {:ok, bad_media} = BDS.Media.import_media(%{project_id: project.id, source_path: corrupt_path})
+
+    Enum.each(Map.values(Thumbnails.thumbnail_paths(good_media)), fn path ->
+      File.rm(Path.join(temp_dir, path))
+    end)
+
+    Enum.each(Map.values(Thumbnails.thumbnail_paths(bad_media)), fn path ->
+      File.rm(Path.join(temp_dir, path))
+    end)
+
+    File.write!(Path.join(temp_dir, bad_media.file_path), "corrupt data")
+
+    result = Thumbnails.regenerate_missing_thumbnails(project.id)
+    assert result.failed >= 1
+    assert result.processed >= 2
+  end
+
+  test "import_media succeeds even when thumbnail generation fails for corrupt image", %{
+    project: project,
+    temp_dir: temp_dir
+  } do
+    source_path = Path.join(temp_dir, "bad_import.jpg")
+    File.write!(source_path, "not a real image at all")
+
+    assert {:ok, media} =
+             BDS.Media.import_media(%{project_id: project.id, source_path: source_path})
+
+    assert media.mime_type == "image/jpeg"
+    assert Repo.get(BDS.Media.Media, media.id) != nil
+  end
+
+  defp tiny_jpeg_binary do
+    Image.new!(3, 2, color: [255, 0, 0])
+    |> Image.write!(:memory, suffix: ".jpg", quality: 85)
+  end
+end