chore: more transactions with filesystem actions cleanup

2026-05-01 16:19:50 +02:00
parent 3133beffcb
commit 7f5077c6ad
8 changed files with 177 additions and 50 deletions
--- a/CODESMELL.md
+++ b/CODESMELL.md
@@ -51,9 +51,9 @@ _None._ All modules previously on the queue have been split; refresh the queue i

 ## 3. Side Effects in Transactions

-**Status:** ✅ done in `BDS.Media` (2026-04-30). Started elsewhere: `BDS.Templates.update_template/2` now keeps only DB writes inside its transaction and runs template-file rewrites, published-post rewrites, and tags JSON flushes after commit. Open elsewhere — no audit yet for `BDS.Posts`, `BDS.Publishing`, `BDS.Generation`.
+**Status:** ✅ done for explicit `Repo.transaction/1` sites (2026-05-10). `BDS.Media`, `BDS.Templates.update_template/2`, `BDS.Metadata`, `BDS.Tags`, and `BDS.Projects` now keep filesystem/search/template-rebuild side effects outside their DB transactions. Remaining explicit transactions (`BDS.PostLinks`, `BDS.AI.Catalog`, project activation/deletion cleanup, and media link helpers) are DB-only or already run filesystem cleanup after commit. `BDS.Posts`, `BDS.Publishing`, and `BDS.Generation` do not currently use `Repo.transaction/1`.

-**Plan:** spot-check every `Repo.transaction/1` outside `BDS.Media`. Rule: only DB writes inside; filesystem and `Search.sync_*` after commit.
+**Rule:** only DB writes inside explicit transactions; filesystem, search sync, template rebuilds, and published-file rewrites run after commit.

 ---

@@ -171,6 +171,8 @@ Most tests share the SQLite repo and named GenServers (`BDS.Tasks`, `BDS.Search`

 ### 2026-05-10

+- **Side effects in transactions**: `BDS.Metadata.update_project_metadata/2`, `sync_project_metadata_from_filesystem/1`, and the shared category/publishing `update_state` path now keep only project/settings row changes inside `Repo.transaction/1`. Metadata JSON files are flushed after commit and `Persistence.atomic_write/2` now returns `{:error, reason}` for directory-creation failures instead of raising. Added regression coverage for a failed metadata filesystem flush preserving the committed project/settings changes.
+- **Side effects in transactions**: `BDS.Tags.sync_tags_from_posts/1`, `delete_tag/1`, `rename_tag/2`, and `merge_tags/2` now commit tag/post-tag DB changes before published-post rewrites and `meta/tags.json` flushes. `BDS.Projects.ensure_default_project/0` and `create_project/1` now commit the project row before rebuilding templates from filesystem files. Added regressions for failed tag JSON flushes and failed template rebuilds preserving committed DB changes. Finished the explicit `Repo.transaction/1` audit: remaining transactions are DB-only or already defer filesystem cleanup until after commit; `BDS.Posts`, `BDS.Publishing`, and `BDS.Generation` have no explicit transaction sites.
 - **Process dictionary for i18n state (Section 2)**: encapsulated behind `BDS.Desktop.UILocale` (`lib/bds/desktop/ui_locale.ex`, ~50 lines). Public surface: `put/1` (set without restore, for LV render boundaries that return lazy `Rendered`), `with_locale/2` (set + try/after restore, for short-lived eager contexts), `current/0` (read, returns `nil` when unset). The two raw `Process.put(:bds_ui_locale, _)` sites (`BDS.Desktop.ShellLive.render/1` and `BDS.Desktop.ShellLive.SidebarComponents.sidebar_content/1`) now call `UILocale.put/1`; the ~30 raw `Process.get(:bds_ui_locale)` reads (every editor `translated/1,2` helper plus `BDS.Desktop.ShellData.effective_ui_language/1`) now call `BDS.Desktop.UILocale.current/0`. The full thread-locale-through-assigns rewrite (~733 HEEx call sites) was deliberately rejected as too invasive; the encapsulation removes the implicit-global smell while preserving Phoenix's lazy `Rendered` evaluation. The render path uses `put/1` (not `with_locale/2`) because Phoenix `~H` returns a `Rendered` whose `dynamic` is invoked by LiveView *after* `render/1` returns; a `try/after Process.delete` would clear the binding before child components materialize. Only `BDS.Desktop.UILocale` is allowed to touch the `:bds_ui_locale` process key. Validates clean: `mix compile --warnings-as-errors`, `mix dialyzer --format short` (Total errors: 0), `mix test` (342 tests, 0 failures, 4 skipped, three consecutive runs).

 ### 2026-05-09
--- a/lib/bds/metadata.ex
+++ b/lib/bds/metadata.ex
@@ -88,10 +88,10 @@ defmodule BDS.Metadata do
        |> Repo.update!()

      persist_setting(project_id, "project", stringify_project_metadata(project_metadata), now)
-      write_project_metadata_files(updated_project, state, project_metadata)
-      load_state(updated_project)
+      {updated_project, project_metadata}
    end)
    |> unwrap_transaction()
+    |> flush_project_metadata_update(state)
    |> maybe_backfill_embeddings(project_id, state, project_metadata)
  end

@@ -106,8 +106,7 @@ defmodule BDS.Metadata do
        |> Enum.sort()

      persist_setting(project.id, "categories", %{"categories" => categories}, now)
-      write_categories_json(project, categories)
-      %{state | categories: categories}
+      {%{state | categories: categories}, fn -> write_categories_json(project, categories) end}
    end)
  end

@@ -119,9 +118,14 @@ defmodule BDS.Metadata do

      persist_setting(project.id, "categories", %{"categories" => categories}, now)
      persist_setting(project.id, "category_meta", %{"categories" => category_settings}, now)
-      write_categories_json(project, categories)
-      write_category_meta_json(project, category_settings)
-      %{state | categories: categories, category_settings: category_settings}
+
+      {%{state | categories: categories, category_settings: category_settings},
+       fn ->
+         with :ok <- write_categories_json(project, categories),
+              :ok <- write_category_meta_json(project, category_settings) do
+           :ok
+         end
+       end}
    end)
  end

@@ -133,8 +137,9 @@ defmodule BDS.Metadata do
      category_settings = Map.put(state.category_settings, category, normalized)

      persist_setting(project.id, "category_meta", %{"categories" => category_settings}, now)
-      write_category_meta_json(project, category_settings)
-      %{state | category_settings: category_settings}
+
+      {%{state | category_settings: category_settings},
+       fn -> write_category_meta_json(project, category_settings) end}
    end)
  end

@@ -144,8 +149,9 @@ defmodule BDS.Metadata do
    update_state(project_id, fn project, state, now ->
      publishing_preferences = normalize_publishing_preferences(prefs)
      persist_setting(project.id, "publishing", publishing_preferences, now)
-      write_publishing_json(project, publishing_preferences)
-      %{state | publishing_preferences: publishing_preferences}
+
+      {%{state | publishing_preferences: publishing_preferences},
+       fn -> write_publishing_json(project, publishing_preferences) end}
    end)
  end

@@ -176,13 +182,10 @@ defmodule BDS.Metadata do
      )

      persist_setting(project_id, "publishing", filesystem_state.publishing_preferences, now)
-      write_project_json(updated_project, stringify_project_metadata(filesystem_state))
-      write_categories_json(updated_project, filesystem_state.categories)
-      write_category_meta_json(updated_project, filesystem_state.category_settings)
-      write_publishing_json(updated_project, filesystem_state.publishing_preferences)
-      load_state(updated_project)
+      updated_project
    end)
    |> unwrap_transaction()
+    |> flush_synced_project_metadata(filesystem_state)
  end

  @spec flush_project_metadata_to_filesystem(String.t()) :: {:ok, metadata_state()}
@@ -200,10 +203,9 @@ defmodule BDS.Metadata do
    state = load_state(project)
    now = Persistence.now_ms()

-    Repo.transaction(fn ->
-      updater.(project, state, now)
-    end)
+    Repo.transaction(fn -> updater.(project, state, now) end)
    |> unwrap_transaction()
+    |> flush_state_update()
  end

  defp load_state(project) do
@@ -338,11 +340,40 @@ defmodule BDS.Metadata do
    }
  end

+  defp flush_project_metadata_update({:ok, {updated_project, project_metadata}}, state) do
+    with :ok <- write_project_metadata_files(updated_project, state, project_metadata) do
+      {:ok, load_state(updated_project)}
+    end
+  end
+
+  defp flush_project_metadata_update(error, _state), do: error
+
+  defp flush_synced_project_metadata({:ok, updated_project}, filesystem_state) do
+    with :ok <- write_project_json(updated_project, stringify_project_metadata(filesystem_state)),
+         :ok <- write_categories_json(updated_project, filesystem_state.categories),
+         :ok <- write_category_meta_json(updated_project, filesystem_state.category_settings),
+         :ok <- write_publishing_json(updated_project, filesystem_state.publishing_preferences) do
+      {:ok, load_state(updated_project)}
+    end
+  end
+
+  defp flush_synced_project_metadata(error, _filesystem_state), do: error
+
+  defp flush_state_update({:ok, {state, write_files}}) when is_function(write_files, 0) do
+    with :ok <- write_files.() do
+      {:ok, state}
+    end
+  end
+
+  defp flush_state_update(error), do: error
+
  defp write_project_metadata_files(project, state, project_metadata) do
-    write_project_json(project, stringify_project_metadata(project_metadata))
-    write_categories_json(project, state.categories)
-    write_category_meta_json(project, state.category_settings)
-    write_publishing_json(project, state.publishing_preferences)
+    with :ok <- write_project_json(project, stringify_project_metadata(project_metadata)),
+         :ok <- write_categories_json(project, state.categories),
+         :ok <- write_category_meta_json(project, state.category_settings),
+         :ok <- write_publishing_json(project, state.publishing_preferences) do
+      :ok
+    end
  end

  defp write_project_json(project, project_json),
@@ -363,7 +394,7 @@ defmodule BDS.Metadata do
  defp write_json(project, file_name, payload) do
    meta_dir = Path.join(Projects.project_data_dir(project), "meta")
    path = Path.join(meta_dir, file_name)
-    :ok = Persistence.atomic_write(path, Jason.encode!(payload))
+    Persistence.atomic_write(path, Jason.encode!(payload))
  end

  defp read_json(project, file_name) do
--- a/lib/bds/persistence.ex
+++ b/lib/bds/persistence.ex
@@ -66,10 +66,10 @@ defmodule BDS.Persistence do
  def parse_timestamp(_value), do: nil

  def atomic_write(path, contents) when is_binary(path) and is_binary(contents) do
-    :ok = File.mkdir_p(Path.dirname(path))
    temp_path = path <> ".tmp"

-    with :ok <- File.write(temp_path, contents),
+    with :ok <- File.mkdir_p(Path.dirname(path)),
+         :ok <- File.write(temp_path, contents),
         :ok <- File.rename(temp_path, path) do
      :ok
    else
--- a/lib/bds/projects.ex
+++ b/lib/bds/projects.ex
@@ -84,11 +84,10 @@ defmodule BDS.Projects do
            })
            |> Repo.insert!()

-          {:ok, _templates} = Templates.rebuild_templates_from_files(project.id)
          project
        end)
        |> case do
-          {:ok, project} -> {:ok, project}
+          {:ok, project} -> rebuild_project_templates(project)
          {:error, reason} -> {:error, reason}
        end
    end
@@ -127,11 +126,14 @@ defmodule BDS.Projects do
        })
        |> Repo.insert!()

-      {:ok, _templates} = Templates.rebuild_templates_from_files(project.id)
      project
    end)
    |> case do
-      {:ok, project} -> sync_filesystem_metadata(project)
+      {:ok, project} ->
+        with {:ok, project} <- rebuild_project_templates(project) do
+          sync_filesystem_metadata(project)
+        end
+
      {:error, reason} -> {:error, reason}
    end
  end
@@ -217,6 +219,12 @@ defmodule BDS.Projects do
    end
  end

+  defp rebuild_project_templates(%Project{} = project) do
+    with {:ok, _templates} <- Templates.rebuild_templates_from_files(project.id) do
+      {:ok, project}
+    end
+  end
+
  defp unique_slug(base_slug) do
    normalized = if base_slug in [nil, ""], do: "project", else: base_slug

--- a/lib/bds/tags.ex
+++ b/lib/bds/tags.ex
@@ -30,8 +30,9 @@ defmodule BDS.Tags do
      |> Repo.insert()
      |> case do
        {:ok, tag} ->
-          write_tags_json(project_id)
+          with :ok <- write_tags_json(project_id) do
            {:ok, tag}
+          end

        error ->
          error
@@ -85,11 +86,14 @@ defmodule BDS.Tags do
        |> Repo.insert!()
      end)

-      write_tags_json(project_id)
      list_tags(project_id)
    end)
    |> case do
-      {:ok, tags} -> {:ok, tags}
+      {:ok, tags} ->
+        with :ok <- write_tags_json(project_id) do
+          {:ok, tags}
+        end
+
      {:error, reason} -> {:error, reason}
    end
  end
@@ -111,8 +115,9 @@ defmodule BDS.Tags do
        |> Repo.update()
        |> case do
          {:ok, updated_tag} ->
-            write_tags_json(updated_tag.project_id)
+            with :ok <- write_tags_json(updated_tag.project_id) do
              {:ok, updated_tag}
+            end

          error ->
            error
@@ -135,10 +140,15 @@ defmodule BDS.Tags do
          end)

          Repo.delete!(tag)
-          write_tags_json(tag.project_id)
+          Enum.map(affected_posts, & &1.id)
        end)
        |> case do
-          {:ok, _} -> {:ok, :deleted}
+          {:ok, post_ids} ->
+            with :ok <- rewrite_published_posts(post_ids),
+                 :ok <- write_tags_json(tag.project_id) do
+              {:ok, :deleted}
+            end
+
          {:error, reason} -> {:error, reason}
        end
    end
@@ -168,11 +178,15 @@ defmodule BDS.Tags do
              |> Tag.changeset(%{name: normalized_name, updated_at: Persistence.now_ms()})
              |> Repo.update!()

-            write_tags_json(tag.project_id)
-            updated_tag
+            {updated_tag, Enum.map(affected_posts, & &1.id)}
          end)
          |> case do
-            {:ok, updated_tag} -> {:ok, updated_tag}
+            {:ok, {updated_tag, post_ids}} ->
+              with :ok <- rewrite_published_posts(post_ids),
+                   :ok <- write_tags_json(tag.project_id) do
+                {:ok, updated_tag}
+              end
+
            {:error, reason} -> {:error, reason}
          end
        end
@@ -193,18 +207,23 @@ defmodule BDS.Tags do

        Repo.transaction(fn ->
          source_names = Enum.map(source_tags, & &1.name)
+          affected_posts = posts_with_any_tag(target_tag.project_id, source_names)

-          posts_with_any_tag(target_tag.project_id, source_names)
-          |> Enum.each(fn post ->
+          Enum.each(affected_posts, fn post ->
            updated_tags = merge_post_tags(post.tags || [], source_names, target_tag.name)
            update_post_tags(post, updated_tags)
          end)

          Enum.each(source_tags, &Repo.delete!/1)
-          write_tags_json(target_tag.project_id)
+          Enum.map(affected_posts, & &1.id)
        end)
        |> case do
-          {:ok, _} -> {:ok, :merged}
+          {:ok, post_ids} ->
+            with :ok <- rewrite_published_posts(post_ids),
+                 :ok <- write_tags_json(target_tag.project_id) do
+              {:ok, :merged}
+            end
+
          {:error, reason} -> {:error, reason}
        end
    end
@@ -213,7 +232,6 @@ defmodule BDS.Tags do
  defp write_tags_json(project_id) do
    project = Projects.get_project!(project_id)
    path = Path.join([Projects.project_data_dir(project), "meta", "tags.json"])
-    :ok = File.mkdir_p(Path.dirname(path))

    payload =
      project_id
@@ -225,7 +243,7 @@ defmodule BDS.Tags do
        |> maybe_put("postTemplateSlug", tag.post_template_slug)
      end)

-    :ok = Persistence.atomic_write(path, Jason.encode!(payload))
+    Persistence.atomic_write(path, Jason.encode!(payload))
  end

  defp validate_unique_name(project_id, name) do
@@ -312,8 +330,11 @@ defmodule BDS.Tags do
    post
    |> Post.changeset(%{tags: updated_tags, updated_at: Persistence.now_ms()})
    |> Repo.update!()
+  end

-    Posts.rewrite_published_post(post.id)
+  defp rewrite_published_posts(post_ids) do
+    Enum.each(post_ids, &Posts.rewrite_published_post/1)
+    :ok
  end

  defp maybe_put(map, _key, nil), do: map
--- a/test/bds/metadata_test.exs
+++ b/test/bds/metadata_test.exs
@@ -54,6 +54,27 @@ defmodule BDS.MetadataTest do
    assert loaded.blog_languages == ["de", "fr"]
  end

+  test "update_project_metadata keeps committed database changes when filesystem flush fails", %{
+    project: project,
+    temp_dir: temp_dir
+  } do
+    meta_path = Path.join(temp_dir, "meta")
+    File.rm_rf!(meta_path)
+    File.write!(meta_path, "not a directory")
+
+    assert {:error, _reason} =
+             BDS.Metadata.update_project_metadata(project.id, %{
+               name: "Committed Metadata",
+               description: "Stored before flush"
+             })
+
+    assert BDS.Projects.get_project!(project.id).name == "Committed Metadata"
+
+    assert {:ok, loaded} = BDS.Metadata.get_project_metadata(project.id)
+    assert loaded.name == "Committed Metadata"
+    assert loaded.description == "Stored before flush"
+  end
+
  test "category and publishing updates write their meta files and sync_project_metadata_from_filesystem loads them",
       %{project: project, temp_dir: temp_dir} do
    assert {:ok, _metadata} = BDS.Metadata.add_category(project.id, "news")
--- a/test/bds/projects_test.exs
+++ b/test/bds/projects_test.exs
@@ -72,6 +72,26 @@ defmodule BDS.ProjectsTest do
    refute {"not-found", :not_found} in starter_slugs
  end

+  test "create_project keeps committed project when template rebuild fails", %{
+    temp_root: temp_root
+  } do
+    temp_dir = Path.join(temp_root, "broken-template-blog")
+    templates_dir = Path.join(temp_dir, "templates")
+    File.mkdir_p!(templates_dir)
+
+    File.write!(
+      Path.join(templates_dir, "broken.liquid"),
+      "---\ntitle: Broken Template\nkind: post\n---\nBody"
+    )
+
+    assert_raise KeyError, fn ->
+      BDS.Projects.create_project(%{name: "Broken Templates", data_path: temp_dir})
+    end
+
+    assert %Project{name: "Broken Templates"} =
+             Repo.get_by(Project, data_path: temp_dir)
+  end
+
  test "set_active_project clears the previous active project and activates the target", %{
    temp_root: temp_root
  } do
--- a/test/bds/tags_test.exs
+++ b/test/bds/tags_test.exs
@@ -94,6 +94,30 @@ defmodule BDS.TagsTest do
    assert [%{"name" => "Beta"}] = Jason.decode!(File.read!(tags_path))
  end

+  test "rename_tag keeps committed database changes when tags json flush fails", %{
+    project: project,
+    temp_dir: temp_dir
+  } do
+    assert {:ok, tag} = BDS.Tags.create_tag(%{project_id: project.id, name: "Alpha"})
+
+    assert {:ok, post} =
+             BDS.Posts.create_post(%{
+               project_id: project.id,
+               title: "Tagged Draft",
+               content: "Body",
+               tags: ["Alpha"]
+             })
+
+    meta_path = Path.join(temp_dir, "meta")
+    File.rm_rf!(meta_path)
+    File.write!(meta_path, "not a directory")
+
+    assert {:error, _reason} = BDS.Tags.rename_tag(tag.id, "Beta")
+
+    assert Repo.get!(BDS.Tags.Tag, tag.id).name == "Beta"
+    assert Repo.get!(Post, post.id).tags == ["Beta"]
+  end
+
  test "merge_tags moves source tags onto the target, deduplicates post tags, deletes sources, and refreshes tags.json",
       %{project: project, temp_dir: temp_dir} do
    assert {:ok, source_a} = BDS.Tags.create_tag(%{project_id: project.id, name: "Alpha"})