From 7f5077c6adf4f6859e31ffdd9279dfe1d1076588 Mon Sep 17 00:00:00 2001 From: Chili Palmer Date: Fri, 1 May 2026 16:19:50 +0200 Subject: [PATCH] chore: more transactions with filesystem actions cleanup --- CODESMELL.md | 6 ++- lib/bds/metadata.ex | 79 ++++++++++++++++++++++++++------------ lib/bds/persistence.ex | 4 +- lib/bds/projects.ex | 16 ++++++-- lib/bds/tags.ex | 57 ++++++++++++++++++--------- test/bds/metadata_test.exs | 21 ++++++++++ test/bds/projects_test.exs | 20 ++++++++++ test/bds/tags_test.exs | 24 ++++++++++++ 8 files changed, 177 insertions(+), 50 deletions(-) diff --git a/CODESMELL.md b/CODESMELL.md index 1c83b88..b226382 100644 --- 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 diff --git a/lib/bds/metadata.ex b/lib/bds/metadata.ex index f3e54f0..ef8e610 100644 --- 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 diff --git a/lib/bds/persistence.ex b/lib/bds/persistence.ex index a42e7a2..a6c877b 100644 --- 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 diff --git a/lib/bds/projects.ex b/lib/bds/projects.ex index cd264a5..5c4e5a1 100644 --- 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 diff --git a/lib/bds/tags.ex b/lib/bds/tags.ex index 2ba07bd..dbf094c 100644 --- 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) - {:ok, tag} + 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) - {:ok, updated_tag} + 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 diff --git a/test/bds/metadata_test.exs b/test/bds/metadata_test.exs index a3f7813..62ce8e1 100644 --- 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") diff --git a/test/bds/projects_test.exs b/test/bds/projects_test.exs index 43fbf4a..d40b0c2 100644 --- 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 diff --git a/test/bds/tags_test.exs b/test/bds/tags_test.exs index def39c2..f0bc826 100644 --- 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"})