Close TD-23 silent rescue sweep
This commit is contained in:
14
TECHDEBTS.md
14
TECHDEBTS.md
@@ -855,7 +855,19 @@ add `ON DELETE CASCADE` via an Ecto migration on the
|
|||||||
**Acceptance.** Injected failure between the two deletes leaves both intact;
|
**Acceptance.** Injected failure between the two deletes leaves both intact;
|
||||||
no orphaned messages possible.
|
no orphaned messages possible.
|
||||||
|
|
||||||
### TD-23: Sweep blanket `rescue`/`catch` blocks for silent failure
|
### TD-23: Sweep blanket `rescue`/`catch` blocks for silent failure ✅ DONE (2026-06-12)
|
||||||
|
|
||||||
|
**Status: implemented.** The named sweep surface was tightened in the intended
|
||||||
|
way instead of being rewritten wholesale. `ImportAnalysis` and
|
||||||
|
`ImportExecution` now log contextual warnings when top-level import handling,
|
||||||
|
repo-transaction wrappers, media linking, or progress callbacks fall back after
|
||||||
|
exceptions; the tests now prove those progress-callback failures are no longer
|
||||||
|
silent. In `ImportAnalysis.year_from/1`, the blanket integer-date rescue was
|
||||||
|
narrowed to `ArgumentError`, which is the actual parsing failure path there.
|
||||||
|
`OverlayComponents` kept its defensive UI fallbacks, but every rescue now logs
|
||||||
|
why the fallback path was taken instead of silently swallowing the error.
|
||||||
|
`MainWindow` needed no change in this pass: its catches are already scoped to
|
||||||
|
expected `:exit`/wx failures rather than bare rescue-all blocks.
|
||||||
|
|
||||||
**Context.** Several modules rescue *all* exceptions into `:ok`/fallbacks
|
**Context.** Several modules rescue *all* exceptions into `:ok`/fallbacks
|
||||||
(`shutdown.ex` defensibly — it must never block quit; but also
|
(`shutdown.ex` defensibly — it must never block quit; but also
|
||||||
|
|||||||
@@ -5,6 +5,7 @@ defmodule BDS.Desktop.ShellLive.OverlayComponents do
|
|||||||
use Gettext, backend: BDS.Gettext
|
use Gettext, backend: BDS.Gettext
|
||||||
|
|
||||||
import Ecto.Query
|
import Ecto.Query
|
||||||
|
require Logger
|
||||||
|
|
||||||
alias BDS.{I18n, Media, Metadata, Posts, Repo}
|
alias BDS.{I18n, Media, Metadata, Posts, Repo}
|
||||||
alias BDS.Media.Media, as: MediaRecord
|
alias BDS.Media.Media, as: MediaRecord
|
||||||
@@ -70,7 +71,9 @@ defmodule BDS.Desktop.ShellLive.OverlayComponents do
|
|||||||
{:ok, metadata} = Metadata.get_project_metadata(project_id)
|
{:ok, metadata} = Metadata.get_project_metadata(project_id)
|
||||||
metadata
|
metadata
|
||||||
rescue
|
rescue
|
||||||
_error -> %{main_language: "en", blog_languages: []}
|
error ->
|
||||||
|
log_overlay_warning("project metadata", error)
|
||||||
|
%{main_language: "en", blog_languages: []}
|
||||||
end
|
end
|
||||||
|
|
||||||
defp posts(nil), do: []
|
defp posts(nil), do: []
|
||||||
@@ -137,7 +140,9 @@ defmodule BDS.Desktop.ShellLive.OverlayComponents do
|
|||||||
select: pm.media_id
|
select: pm.media_id
|
||||||
)
|
)
|
||||||
rescue
|
rescue
|
||||||
_error -> []
|
error ->
|
||||||
|
log_overlay_warning("post media ids for #{post_id}", error)
|
||||||
|
[]
|
||||||
end
|
end
|
||||||
|
|
||||||
defp post_media_ids(_tab), do: []
|
defp post_media_ids(_tab), do: []
|
||||||
@@ -150,7 +155,9 @@ defmodule BDS.Desktop.ShellLive.OverlayComponents do
|
|||||||
)
|
)
|
||||||
|> Map.new(fn {language, status} -> {language, Atom.to_string(status || :draft)} end)
|
|> Map.new(fn {language, status} -> {language, Atom.to_string(status || :draft)} end)
|
||||||
rescue
|
rescue
|
||||||
_error -> %{}
|
error ->
|
||||||
|
log_overlay_warning("post translations for #{post_id}", error)
|
||||||
|
%{}
|
||||||
end
|
end
|
||||||
|
|
||||||
defp existing_translations(%{type: :media, id: media_id}) do
|
defp existing_translations(%{type: :media, id: media_id}) do
|
||||||
@@ -161,7 +168,9 @@ defmodule BDS.Desktop.ShellLive.OverlayComponents do
|
|||||||
)
|
)
|
||||||
|> Map.new(fn {language, status} -> {language, status} end)
|
|> Map.new(fn {language, status} -> {language, status} end)
|
||||||
rescue
|
rescue
|
||||||
_error -> %{}
|
error ->
|
||||||
|
log_overlay_warning("media translations for #{media_id}", error)
|
||||||
|
%{}
|
||||||
end
|
end
|
||||||
|
|
||||||
defp existing_translations(_tab), do: %{}
|
defp existing_translations(_tab), do: %{}
|
||||||
@@ -179,7 +188,9 @@ defmodule BDS.Desktop.ShellLive.OverlayComponents do
|
|||||||
_other -> metadata.main_language || "en"
|
_other -> metadata.main_language || "en"
|
||||||
end
|
end
|
||||||
rescue
|
rescue
|
||||||
_error -> metadata.main_language || "en"
|
error ->
|
||||||
|
log_overlay_warning("post source language for #{post_id}", error)
|
||||||
|
metadata.main_language || "en"
|
||||||
end
|
end
|
||||||
|
|
||||||
defp source_language(%{type: :media, id: media_id}, metadata) do
|
defp source_language(%{type: :media, id: media_id}, metadata) do
|
||||||
@@ -188,7 +199,9 @@ defmodule BDS.Desktop.ShellLive.OverlayComponents do
|
|||||||
_other -> metadata.main_language || "en"
|
_other -> metadata.main_language || "en"
|
||||||
end
|
end
|
||||||
rescue
|
rescue
|
||||||
_error -> metadata.main_language || "en"
|
error ->
|
||||||
|
log_overlay_warning("media source language for #{media_id}", error)
|
||||||
|
metadata.main_language || "en"
|
||||||
end
|
end
|
||||||
|
|
||||||
defp source_language(_tab, metadata), do: metadata.main_language || "en"
|
defp source_language(_tab, metadata), do: metadata.main_language || "en"
|
||||||
@@ -242,7 +255,9 @@ defmodule BDS.Desktop.ShellLive.OverlayComponents do
|
|||||||
[]
|
[]
|
||||||
end
|
end
|
||||||
rescue
|
rescue
|
||||||
_error -> []
|
error ->
|
||||||
|
log_overlay_warning("post AI fields for #{post_id}", error)
|
||||||
|
[]
|
||||||
end
|
end
|
||||||
|
|
||||||
defp ai_fields(%{type: :media, id: media_id}, title, _subtitle, page_language) do
|
defp ai_fields(%{type: :media, id: media_id}, title, _subtitle, page_language) do
|
||||||
@@ -279,7 +294,9 @@ defmodule BDS.Desktop.ShellLive.OverlayComponents do
|
|||||||
[]
|
[]
|
||||||
end
|
end
|
||||||
rescue
|
rescue
|
||||||
_error -> []
|
error ->
|
||||||
|
log_overlay_warning("media AI fields for #{media_id}", error)
|
||||||
|
[]
|
||||||
end
|
end
|
||||||
|
|
||||||
defp ai_fields(_tab, _title, _subtitle, _page_language), do: []
|
defp ai_fields(_tab, _title, _subtitle, _page_language), do: []
|
||||||
@@ -309,7 +326,9 @@ defmodule BDS.Desktop.ShellLive.OverlayComponents do
|
|||||||
reference_list: reference_list
|
reference_list: reference_list
|
||||||
}
|
}
|
||||||
rescue
|
rescue
|
||||||
_error ->
|
error ->
|
||||||
|
log_overlay_warning("delete media details for #{media_id}", error)
|
||||||
|
|
||||||
%{
|
%{
|
||||||
title: BDS.Gettext.lgettext(page_language, "ui", "Delete Media"),
|
title: BDS.Gettext.lgettext(page_language, "ui", "Delete Media"),
|
||||||
entity_name: media_id,
|
entity_name: media_id,
|
||||||
@@ -330,7 +349,9 @@ defmodule BDS.Desktop.ShellLive.OverlayComponents do
|
|||||||
reference_list: []
|
reference_list: []
|
||||||
}
|
}
|
||||||
rescue
|
rescue
|
||||||
_error ->
|
error ->
|
||||||
|
log_overlay_warning("delete tag details", error)
|
||||||
|
|
||||||
%{
|
%{
|
||||||
title: BDS.Gettext.lgettext(page_language, "ui", "Delete Tag"),
|
title: BDS.Gettext.lgettext(page_language, "ui", "Delete Tag"),
|
||||||
entity_name: "tag",
|
entity_name: "tag",
|
||||||
@@ -367,7 +388,9 @@ defmodule BDS.Desktop.ShellLive.OverlayComponents do
|
|||||||
message: BDS.Gettext.lgettext(page_language, "ui", "Cannot be undone.")
|
message: BDS.Gettext.lgettext(page_language, "ui", "Cannot be undone.")
|
||||||
}
|
}
|
||||||
rescue
|
rescue
|
||||||
_error ->
|
error ->
|
||||||
|
log_overlay_warning("merge tag details for project #{project_id}", error)
|
||||||
|
|
||||||
%{
|
%{
|
||||||
target: "tag",
|
target: "tag",
|
||||||
count: 1,
|
count: 1,
|
||||||
@@ -391,4 +414,8 @@ defmodule BDS.Desktop.ShellLive.OverlayComponents do
|
|||||||
|> String.replace(~r/[^a-z0-9]+/u, "-")
|
|> String.replace(~r/[^a-z0-9]+/u, "-")
|
||||||
|> String.trim("-")
|
|> String.trim("-")
|
||||||
end
|
end
|
||||||
|
|
||||||
|
defp log_overlay_warning(context, error) do
|
||||||
|
Logger.warning("overlay component fallback for #{context}: #{Exception.message(error)}")
|
||||||
|
end
|
||||||
end
|
end
|
||||||
|
|||||||
@@ -2,6 +2,7 @@ defmodule BDS.ImportAnalysis do
|
|||||||
@moduledoc false
|
@moduledoc false
|
||||||
|
|
||||||
import Ecto.Query
|
import Ecto.Query
|
||||||
|
require Logger
|
||||||
|
|
||||||
alias BDS.Media.Media
|
alias BDS.Media.Media
|
||||||
alias BDS.Posts.Post
|
alias BDS.Posts.Post
|
||||||
@@ -25,7 +26,12 @@ defmodule BDS.ImportAnalysis do
|
|||||||
wxr_data = WxrParser.parse_file(wxr_file_path)
|
wxr_data = WxrParser.parse_file(wxr_file_path)
|
||||||
{:ok, build_report(project_id, wxr_data, wxr_file_path, uploads_folder_path, on_progress)}
|
{:ok, build_report(project_id, wxr_data, wxr_file_path, uploads_folder_path, on_progress)}
|
||||||
rescue
|
rescue
|
||||||
error -> {:error, %{message: Exception.message(error)}}
|
error ->
|
||||||
|
Logger.warning(
|
||||||
|
"import analysis failed project_id=#{project_id} file=#{wxr_file_path}: #{Exception.message(error)}"
|
||||||
|
)
|
||||||
|
|
||||||
|
{:error, %{message: Exception.message(error)}}
|
||||||
end
|
end
|
||||||
|
|
||||||
defp build_report(project_id, wxr_data, wxr_file_path, uploads_folder_path, on_progress) do
|
defp build_report(project_id, wxr_data, wxr_file_path, uploads_folder_path, on_progress) do
|
||||||
@@ -459,7 +465,7 @@ defmodule BDS.ImportAnalysis do
|
|||||||
value
|
value
|
||||||
end
|
end
|
||||||
rescue
|
rescue
|
||||||
_error -> nil
|
_error in [ArgumentError] -> nil
|
||||||
end
|
end
|
||||||
|
|
||||||
defp year_from(value) when is_binary(value) do
|
defp year_from(value) when is_binary(value) do
|
||||||
@@ -491,7 +497,12 @@ defmodule BDS.ImportAnalysis do
|
|||||||
try do
|
try do
|
||||||
callback.(step, detail)
|
callback.(step, detail)
|
||||||
rescue
|
rescue
|
||||||
_error -> :ok
|
error ->
|
||||||
|
Logger.warning(
|
||||||
|
"import analysis progress callback failed step=#{inspect(step)}: #{Exception.message(error)}"
|
||||||
|
)
|
||||||
|
|
||||||
|
:ok
|
||||||
end
|
end
|
||||||
|
|
||||||
:ok
|
:ok
|
||||||
|
|||||||
@@ -1,6 +1,8 @@
|
|||||||
defmodule BDS.ImportExecution do
|
defmodule BDS.ImportExecution do
|
||||||
@moduledoc false
|
@moduledoc false
|
||||||
|
|
||||||
|
require Logger
|
||||||
|
|
||||||
alias BDS.Media
|
alias BDS.Media
|
||||||
alias BDS.Metadata
|
alias BDS.Metadata
|
||||||
alias BDS.Posts
|
alias BDS.Posts
|
||||||
@@ -118,7 +120,9 @@ defmodule BDS.ImportExecution do
|
|||||||
{:error, reason} -> {:error, %{message: format_import_error(reason)}}
|
{:error, reason} -> {:error, %{message: format_import_error(reason)}}
|
||||||
end
|
end
|
||||||
rescue
|
rescue
|
||||||
error -> {:error, %{message: Exception.message(error)}}
|
error ->
|
||||||
|
Logger.warning("import execution failed project_id=#{project_id}: #{Exception.message(error)}")
|
||||||
|
{:error, %{message: Exception.message(error)}}
|
||||||
end
|
end
|
||||||
|
|
||||||
defp execute_taxonomies(category_items, tag_items, project_id, result, on_progress, started_at) do
|
defp execute_taxonomies(category_items, tag_items, project_id, result, on_progress, started_at) do
|
||||||
@@ -363,9 +367,19 @@ defmodule BDS.ImportExecution do
|
|||||||
try do
|
try do
|
||||||
Media.link_media_to_post(media_id, post_id)
|
Media.link_media_to_post(media_id, post_id)
|
||||||
rescue
|
rescue
|
||||||
_ -> :ok
|
error ->
|
||||||
|
Logger.warning(
|
||||||
|
"import execution failed to link media_id=#{media_id} post_id=#{post_id}: #{Exception.message(error)}"
|
||||||
|
)
|
||||||
|
|
||||||
|
:ok
|
||||||
catch
|
catch
|
||||||
_, _ -> :ok
|
kind, reason ->
|
||||||
|
Logger.warning(
|
||||||
|
"import execution failed to link media_id=#{media_id} post_id=#{post_id}: #{kind}: #{inspect(reason)}"
|
||||||
|
)
|
||||||
|
|
||||||
|
:ok
|
||||||
end
|
end
|
||||||
end)
|
end)
|
||||||
|
|
||||||
@@ -586,9 +600,13 @@ defmodule BDS.ImportExecution do
|
|||||||
defp run_repo_transaction(fun) when is_function(fun, 0) do
|
defp run_repo_transaction(fun) when is_function(fun, 0) do
|
||||||
Repo.transaction(fun)
|
Repo.transaction(fun)
|
||||||
rescue
|
rescue
|
||||||
error -> {:error, error}
|
error ->
|
||||||
|
Logger.warning("import execution transaction failed: #{Exception.message(error)}")
|
||||||
|
{:error, error}
|
||||||
catch
|
catch
|
||||||
kind, reason -> {:error, {kind, reason}}
|
kind, reason ->
|
||||||
|
Logger.warning("import execution transaction failed: #{kind}: #{inspect(reason)}")
|
||||||
|
{:error, {kind, reason}}
|
||||||
end
|
end
|
||||||
|
|
||||||
defp post_item_label(item) do
|
defp post_item_label(item) do
|
||||||
@@ -628,11 +646,20 @@ defmodule BDS.ImportExecution do
|
|||||||
try do
|
try do
|
||||||
callback.(phase, current, total, %{detail: detail, eta: eta})
|
callback.(phase, current, total, %{detail: detail, eta: eta})
|
||||||
rescue
|
rescue
|
||||||
_error ->
|
error ->
|
||||||
|
Logger.warning(
|
||||||
|
"import execution progress callback failed phase=#{inspect(phase)}: #{Exception.message(error)}"
|
||||||
|
)
|
||||||
|
|
||||||
try do
|
try do
|
||||||
callback.(phase, current, total, detail)
|
callback.(phase, current, total, detail)
|
||||||
rescue
|
rescue
|
||||||
_error -> :ok
|
legacy_error ->
|
||||||
|
Logger.warning(
|
||||||
|
"import execution legacy progress callback failed phase=#{inspect(phase)}: #{Exception.message(legacy_error)}"
|
||||||
|
)
|
||||||
|
|
||||||
|
:ok
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
|||||||
@@ -224,6 +224,30 @@ defmodule BDS.ImportAnalysisTest do
|
|||||||
assert_received {:analysis_progress, "Discovering macros...", nil}
|
assert_received {:analysis_progress, "Discovering macros...", nil}
|
||||||
end
|
end
|
||||||
|
|
||||||
|
import ExUnit.CaptureLog
|
||||||
|
|
||||||
|
test "analyze_wxr logs and continues when the progress callback raises", %{
|
||||||
|
project: project,
|
||||||
|
temp_dir: temp_dir
|
||||||
|
} do
|
||||||
|
uploads_dir = Path.join(temp_dir, "uploads")
|
||||||
|
File.mkdir_p!(Path.join(uploads_dir, "2024/05"))
|
||||||
|
File.write!(Path.join(uploads_dir, "2024/05/import-asset.txt"), "legacy attachment")
|
||||||
|
|
||||||
|
wxr_path = Path.join(temp_dir, "legacy.xml")
|
||||||
|
File.write!(wxr_path, basic_wxr_xml())
|
||||||
|
|
||||||
|
log =
|
||||||
|
capture_log(fn ->
|
||||||
|
assert {:ok, _report} =
|
||||||
|
ImportAnalysis.analyze_wxr(project.id, wxr_path, uploads_dir,
|
||||||
|
on_progress: fn _step, _detail -> raise "boom" end
|
||||||
|
)
|
||||||
|
end)
|
||||||
|
|
||||||
|
assert log =~ "import analysis progress callback failed"
|
||||||
|
end
|
||||||
|
|
||||||
defp sha256(value) do
|
defp sha256(value) do
|
||||||
:sha256
|
:sha256
|
||||||
|> :crypto.hash(value)
|
|> :crypto.hash(value)
|
||||||
|
|||||||
@@ -112,6 +112,34 @@ defmodule BDS.ImportExecutionTest do
|
|||||||
assert File.exists?(Path.join(temp_dir, imported_media.file_path))
|
assert File.exists?(Path.join(temp_dir, imported_media.file_path))
|
||||||
end
|
end
|
||||||
|
|
||||||
|
import ExUnit.CaptureLog
|
||||||
|
|
||||||
|
test "execute_import logs and continues when the progress callback raises", %{
|
||||||
|
project: project,
|
||||||
|
temp_dir: temp_dir
|
||||||
|
} do
|
||||||
|
uploads_dir = Path.join(temp_dir, "uploads")
|
||||||
|
File.mkdir_p!(Path.join(uploads_dir, "2024/05"))
|
||||||
|
File.write!(Path.join(uploads_dir, "2024/05/import-asset.txt"), "legacy attachment")
|
||||||
|
|
||||||
|
wxr_path = Path.join(temp_dir, "legacy.xml")
|
||||||
|
File.write!(wxr_path, basic_wxr_xml())
|
||||||
|
|
||||||
|
assert {:ok, report} = ImportAnalysis.analyze_wxr(project.id, wxr_path, uploads_dir)
|
||||||
|
|
||||||
|
log =
|
||||||
|
capture_log(fn ->
|
||||||
|
assert {:ok, _result} =
|
||||||
|
ImportExecution.execute_import(project.id, report,
|
||||||
|
default_author: "Imported Author",
|
||||||
|
uploads_folder_path: uploads_dir,
|
||||||
|
on_progress: fn _phase, _current, _total, _detail -> raise "boom" end
|
||||||
|
)
|
||||||
|
end)
|
||||||
|
|
||||||
|
assert log =~ "import execution progress callback failed"
|
||||||
|
end
|
||||||
|
|
||||||
test "execute_import skips conflicts by default and can import them with a new slug", %{
|
test "execute_import skips conflicts by default and can import them with a new slug", %{
|
||||||
project: project,
|
project: project,
|
||||||
temp_dir: temp_dir
|
temp_dir: temp_dir
|
||||||
|
|||||||
Reference in New Issue
Block a user