diff --git a/CODESMELL.md b/CODESMELL.md index 57fdbc4..0d9e596 100644 --- a/CODESMELL.md +++ b/CODESMELL.md @@ -259,7 +259,7 @@ This suggests data isn't normalized at boundaries. Prefer atoms for internal str ## Priority Order 1. **Add `@spec` to public APIs of core contexts.** ✅ **DONE 2026-04-30.** See "Priority #1 Completion" section below. -2. **Extract filesystem / Search side effects out of `Repo.transaction` in `BDS.Media`.** Real correctness bug under SQLite busy retries. +2. **Extract filesystem / Search side effects out of `Repo.transaction` in `BDS.Media`.** ✅ **DONE 2026-04-30.** See "Priority #2 Completion" section below. 3. **Fix `MCP.atomize_keys`** to use `String.to_existing_atom/1` with a string-fallback. Removes a real atom-table DoS vector. 4. **Introduce `BDS.PostMedia` Ecto schema** and migrate the 6–8 raw `post_media` queries. Direct type-safety win. 5. **Replace `Repo.get` calls in `ShellLive`** with context functions (add new context functions where needed). @@ -322,6 +322,36 @@ Added `@spec` and `@type` declarations to the public APIs of all core contexts a --- +## Priority #2 Completion (2026-04-30) + +Refactored every `Repo.transaction/1` block in [lib/bds/media.ex](lib/bds/media.ex) so that only DB writes run inside the transaction. Filesystem writes (`File.cp`, `write_sidecar`, `write_translation_sidecar`, `ensure_thumbnails`, `delete_file_if_present`) and `Search.sync_media/1` calls now run *after* the transaction commits, so the SQLite write lock is released as fast as possible. + +**Functions refactored:** + +- `import_media/1` — copies the source file into place *before* the transaction (so a DB rollback can still observe a stale file), then runs only the `Repo.insert!` inside the transaction, then writes sidecar / thumbnails / search index. On DB failure the copied file is removed. +- `update_media/2` — DB update only inside the transaction; sidecar + search after. +- `upsert_media_translation/3` — DB insert/update only inside; sidecar + search after. +- `delete_media_translation/2` — DB delete only inside; sidecar deletion + search reindex + base sidecar rewrite after. +- `replace_media_file/2` — moves the existing destination to a `.bak` *before* the transaction, copies the new file in place, runs only the DB update inside, then refreshes sidecar/thumbnails/search and removes the backup. On DB failure the original file is restored from the backup. +- `link_media_to_post/2` and `unlink_media_from_post/2` — only the `post_media` raw INSERT/DELETE runs inside the transaction; sidecar rewrite happens after commit. + +**Why this matters:** + +SQLite has a single global write lock. Holding the transaction open while we copy files, regenerate Vix-backed thumbnails, and rebuild FTS5 indices makes that lock-hold time unbounded and proportional to image size. Other actors (the LiveView, background tasks, the CLI sync watcher) hit `:busy` retries that the existing `db_connection` busy-timeout cannot always cover. After this change the lock is held for milliseconds, regardless of file size. + +**Trade-offs accepted:** + +- The DB row and the filesystem are no longer atomically coupled. If the BEAM crashes between `Repo.insert!` and `write_sidecar/2`, the row exists without a sidecar. This is recoverable by the existing rebuild-from-database path (which re-emits sidecars), and is the same trade-off that exists everywhere else in the codebase that already runs side effects after transactions. +- `import_media/1` and `replace_media_file/2` use the inverse approach — file IO *before* transaction with explicit cleanup on rollback — because the file is the larger of the two side effects and the DB row is a pointer to it. This keeps the destination consistent on rollback. + +**Spec correction surfaced by Dialyzer:** + +- `BDS.Media.delete_media_translation/2` actually returns `{:ok, boolean()} | {:error, :not_found | term()}` (the `:ok` payload is `false` when no translation exists for the language, `true` when one was deleted). The previous spec advertised `{:ok, :deleted}`, which never matched any code path. + +**Validation:** `mix compile --warnings-as-errors` clean, `mix dialyzer --format short` 0 errors, `mix test` 342/0/4. + +--- + ## Bottom Line The biggest risks are **module size** and **duplicated helpers**, followed by the **process dictionary i18n** and **side effects in transactions**. Fixing the top 5 anti-patterns would significantly improve maintainability, testability, and reliability of the desktop app over long-running sessions. diff --git a/lib/bds/media.ex b/lib/bds/media.ex index 1d30d93..9b0f875 100644 --- a/lib/bds/media.ex +++ b/lib/bds/media.ex @@ -33,42 +33,43 @@ defmodule BDS.Media do destination = Path.join(Projects.project_data_dir(project), file_path) stat = File.stat!(source_path) - Repo.transaction(fn -> - media = - %Media{} - |> Media.changeset(%{ - id: Ecto.UUID.generate(), - project_id: project.id, - filename: file_name, - original_name: original_name, - mime_type: mime_type, - size: stat.size, - width: attr(attrs, :width) || width, - height: attr(attrs, :height) || height, - title: attr(attrs, :title), - alt: attr(attrs, :alt), - caption: attr(attrs, :caption), - author: attr(attrs, :author), - language: attr(attrs, :language), - file_path: file_path, - sidecar_path: sidecar_path, - checksum: attr(attrs, :checksum), - tags: attr(attrs, :tags) || [], - created_at: now, - updated_at: now - }) - |> Repo.insert!() + :ok = File.mkdir_p(Path.dirname(destination)) + :ok = File.cp(source_path, destination) - :ok = File.mkdir_p(Path.dirname(destination)) - :ok = File.cp(source_path, destination) - :ok = write_sidecar(project, media) - :ok = ensure_thumbnails(project, media) - :ok = Search.sync_media(media) - media - end) - |> case do - {:ok, media} -> {:ok, media} - {:error, reason} -> {:error, reason} + case Repo.transaction(fn -> + %Media{} + |> Media.changeset(%{ + id: Ecto.UUID.generate(), + project_id: project.id, + filename: file_name, + original_name: original_name, + mime_type: mime_type, + size: stat.size, + width: attr(attrs, :width) || width, + height: attr(attrs, :height) || height, + title: attr(attrs, :title), + alt: attr(attrs, :alt), + caption: attr(attrs, :caption), + author: attr(attrs, :author), + language: attr(attrs, :language), + file_path: file_path, + sidecar_path: sidecar_path, + checksum: attr(attrs, :checksum), + tags: attr(attrs, :tags) || [], + created_at: now, + updated_at: now + }) + |> Repo.insert!() + end) do + {:ok, media} -> + :ok = write_sidecar(project, media) + :ok = ensure_thumbnails(project, media) + :ok = Search.sync_media(media) + {:ok, media} + + {:error, reason} -> + _ = File.rm(destination) + {:error, reason} end end @@ -94,19 +95,18 @@ defmodule BDS.Media do project = Projects.get_project!(media.project_id) - Repo.transaction(fn -> - updated_media = - media - |> Media.changeset(updates) - |> Repo.update!() + case Repo.transaction(fn -> + media + |> Media.changeset(updates) + |> Repo.update!() + end) do + {:ok, updated_media} -> + :ok = write_sidecar(project, updated_media) + :ok = Search.sync_media(updated_media) + {:ok, updated_media} - :ok = write_sidecar(project, updated_media) - :ok = Search.sync_media(updated_media) - updated_media - end) - |> case do - {:ok, updated_media} -> {:ok, updated_media} - {:error, reason} -> {:error, reason} + {:error, reason} -> + {:error, reason} end end end @@ -292,25 +292,24 @@ defmodule BDS.Media do updated_at: now } - Repo.transaction(fn -> - updated_translation = - translation - |> Translation.changeset(translation_attrs) - |> Repo.insert_or_update!() + case Repo.transaction(fn -> + translation + |> Translation.changeset(translation_attrs) + |> Repo.insert_or_update!() + end) do + {:ok, updated_translation} -> + :ok = write_translation_sidecar(project, media, updated_translation) + :ok = Search.sync_media(media.id) + {:ok, updated_translation} - :ok = write_translation_sidecar(project, media, updated_translation) - :ok = Search.sync_media(media.id) - updated_translation - end) - |> case do - {:ok, updated_translation} -> {:ok, updated_translation} - {:error, reason} -> {:error, reason} + {:error, reason} -> + {:error, reason} end end end @spec delete_media_translation(String.t(), String.t() | atom()) :: - {:ok, :deleted} | {:error, :not_found} + {:ok, boolean()} | {:error, :not_found | term()} def delete_media_translation(media_id, language) do normalized_language = language |> to_string() |> String.trim() |> String.downcase() @@ -326,16 +325,15 @@ defmodule BDS.Media do translation -> project = Projects.get_project!(media.project_id) - Repo.transaction(fn -> - Repo.delete!(translation) - delete_file_if_present(media.project_id, translation_sidecar_path(media, normalized_language)) - :ok = Search.sync_media(media) - :ok = write_sidecar(project, media) - true - end) - |> case do - {:ok, deleted?} -> {:ok, deleted?} - {:error, reason} -> {:error, reason} + case Repo.transaction(fn -> Repo.delete!(translation) end) do + {:ok, _deleted} -> + delete_file_if_present(media.project_id, translation_sidecar_path(media, normalized_language)) + :ok = Search.sync_media(media) + :ok = write_sidecar(project, media) + {:ok, true} + + {:error, reason} -> + {:error, reason} end end end @@ -361,29 +359,31 @@ defmodule BDS.Media do else mime_type = media.mime_type || detect_mime(media.original_name || media.filename) {width, height} = image_dimensions(new_source_path, mime_type) + previous_destination_backup = destination <> ".bak" + _ = File.rename(destination, previous_destination_backup) + :ok = File.cp(new_source_path, destination) - Repo.transaction(fn -> - :ok = File.cp(new_source_path, destination) + case Repo.transaction(fn -> + media + |> Media.changeset(%{ + size: stat.size, + width: width || media.width, + height: height || media.height, + checksum: checksum, + updated_at: Persistence.now_ms() + }) + |> Repo.update!() + end) do + {:ok, updated_media} -> + _ = File.rm(previous_destination_backup) + :ok = write_sidecar(project, updated_media) + :ok = ensure_thumbnails(project, updated_media) + :ok = Search.sync_media(updated_media) + {:ok, updated_media} - updated_media = - media - |> Media.changeset(%{ - size: stat.size, - width: width || media.width, - height: height || media.height, - checksum: checksum, - updated_at: Persistence.now_ms() - }) - |> Repo.update!() - - :ok = write_sidecar(project, updated_media) - :ok = ensure_thumbnails(project, updated_media) - :ok = Search.sync_media(updated_media) - updated_media - end) - |> case do - {:ok, updated_media} -> {:ok, updated_media} - {:error, reason} -> {:error, reason} + {:error, reason} -> + _ = File.rename(previous_destination_backup, destination) + {:error, reason} end end end @@ -428,29 +428,29 @@ defmodule BDS.Media do {%Media{} = media, %BDS.Posts.Post{} = post} -> project = Projects.get_project!(media.project_id) - Repo.transaction(fn -> - case Repo.query("SELECT 1 FROM post_media WHERE post_id = ? AND media_id = ? LIMIT 1", [post.id, media.id]) do - {:ok, %{rows: [[1]]}} -> - :already_linked + case Repo.transaction(fn -> + case Repo.query("SELECT 1 FROM post_media WHERE post_id = ? AND media_id = ? LIMIT 1", [post.id, media.id]) do + {:ok, %{rows: [[1]]}} -> + :already_linked - _other -> - sort_order = next_sort_order(media.id) + _other -> + sort_order = next_sort_order(media.id) - {:ok, _result} = - Repo.query( - "INSERT INTO post_media (id, project_id, post_id, media_id, sort_order, created_at) VALUES (?, ?, ?, ?, ?, ?)", - [Ecto.UUID.generate(), media.project_id, post.id, media.id, sort_order, Persistence.now_ms()] - ) + {:ok, _result} = + Repo.query( + "INSERT INTO post_media (id, project_id, post_id, media_id, sort_order, created_at) VALUES (?, ?, ?, ?, ?, ?)", + [Ecto.UUID.generate(), media.project_id, post.id, media.id, sort_order, Persistence.now_ms()] + ) - :linked - end + :linked + end + end) do + {:ok, _result} -> + :ok = write_sidecar(project, media) + {:ok, :linked} - :ok = write_sidecar(project, media) - :ok - end) - |> case do - {:ok, :ok} -> {:ok, :linked} - {:error, reason} -> {:error, reason} + {:error, reason} -> + {:error, reason} end end end @@ -465,14 +465,18 @@ defmodule BDS.Media do %Media{} = media -> project = Projects.get_project!(media.project_id) - Repo.transaction(fn -> - {:ok, _result} = Repo.query("DELETE FROM post_media WHERE media_id = ? AND post_id = ?", [media.id, post_id]) - :ok = write_sidecar(project, media) - :ok - end) - |> case do - {:ok, :ok} -> {:ok, :unlinked} - {:error, reason} -> {:error, reason} + case Repo.transaction(fn -> + {:ok, _result} = + Repo.query("DELETE FROM post_media WHERE media_id = ? AND post_id = ?", [media.id, post_id]) + + :ok + end) do + {:ok, :ok} -> + :ok = write_sidecar(project, media) + {:ok, :unlinked} + + {:error, reason} -> + {:error, reason} end end end