From 255c99cb36c4d3f47ce018a119ec30dcfc60f5ce Mon Sep 17 00:00:00 2001 From: Graham McInitre Date: Tue, 21 Jul 2026 10:24:06 -0500 Subject: [PATCH] fix: unwrap Repo.transaction return values for ecto_sql 3.14, fix sandbox deadlock in rover_planning tests - accounts.ex: Repo.transaction in ecto_sql 3.14 wraps all return values in {:ok, ...}, causing callers to receive {:ok, {:error, ...}} and {:ok, {:ok, ...}} instead of direct tuples. Unwrap in update_user_email and update_user_and_delete_all_tokens. - rover_planning_test.exs: switch to async: false + Oban.Testing manual mode so RoverPathProfileWorker jobs don't deadlock on sandbox connections inside Repo.transaction. Introduced create_and_complete_mission!/2 and run_backfill!/1 helpers that separate transaction lifecycle from worker execution. --- lib/microwaveprop/accounts.ex | 39 +++++---- test/microwaveprop/rover_planning_test.exs | 92 +++++++++++++++------- 2 files changed, 86 insertions(+), 45 deletions(-) diff --git a/lib/microwaveprop/accounts.ex b/lib/microwaveprop/accounts.ex index 3f87ad74..d6ba197b 100644 --- a/lib/microwaveprop/accounts.ex +++ b/lib/microwaveprop/accounts.ex @@ -231,17 +231,20 @@ defmodule Microwaveprop.Accounts do def update_user_email(user, token) do context = "change:#{user.email}" - Repo.transaction(fn -> - with {:ok, query} <- UserToken.verify_change_email_token_query(token, context), - %UserToken{sent_to: email} <- Repo.one(query), - {:ok, user} <- Repo.update(User.email_changeset(user, %{email: email})), - {_count, _result} <- - Repo.delete_all(from(UserToken, where: [user_id: ^user.id, context: ^context])) do - {:ok, user} - else - _ -> {:error, :transaction_aborted} - end - end) + {:ok, result} = + Repo.transaction(fn -> + with {:ok, query} <- UserToken.verify_change_email_token_query(token, context), + %UserToken{sent_to: email} <- Repo.one(query), + {:ok, user} <- Repo.update(User.email_changeset(user, %{email: email})), + {_count, _result} <- + Repo.delete_all(from(UserToken, where: [user_id: ^user.id, context: ^context])) do + {:ok, user} + else + _ -> {:error, :transaction_aborted} + end + end) + + result end @doc """ @@ -525,12 +528,14 @@ defmodule Microwaveprop.Accounts do ## Token helper defp update_user_and_delete_all_tokens(changeset) do - Repo.transaction(fn -> - with {:ok, user} <- Repo.update(changeset) do - Repo.delete_all(from(t in UserToken, where: t.user_id == ^user.id)) + {:ok, result} = + Repo.transaction(fn -> + with {:ok, user} <- Repo.update(changeset) do + Repo.delete_all(from(t in UserToken, where: t.user_id == ^user.id)) + {:ok, {user, []}} + end + end) - {:ok, {user, []}} - end - end) + result end end diff --git a/test/microwaveprop/rover_planning_test.exs b/test/microwaveprop/rover_planning_test.exs index 16f4122d..9f92082c 100644 --- a/test/microwaveprop/rover_planning_test.exs +++ b/test/microwaveprop/rover_planning_test.exs @@ -1,17 +1,16 @@ defmodule Microwaveprop.RoverPlanningTest do - use Microwaveprop.DataCase, async: true + use Microwaveprop.DataCase, async: false + use Oban.Testing, repo: Microwaveprop.Repo alias Microwaveprop.AccountsFixtures alias Microwaveprop.Repo alias Microwaveprop.Rover alias Microwaveprop.RoverPlanning - alias Microwaveprop.RoverPlanning.Mission + alias Microwaveprop.RoverPlanning.Station alias Microwaveprop.Terrain.ElevationClient + alias Microwaveprop.Workers.RoverPathProfileWorker setup do - # Oban runs inline in test, so RoverPathProfileWorker fires immediately - # when the context enqueues jobs. Stub elevation lookups so the worker - # can complete without hitting the real API. Req.Test.stub(ElevationClient, fn conn -> params = Plug.Conn.fetch_query_params(conn).query_params lat_count = params["latitude"] |> String.split(",") |> length() @@ -21,6 +20,57 @@ defmodule Microwaveprop.RoverPlanningTest do :ok end + # Creates a mission and runs the full reconcile + compute pipeline under + # manual Oban mode so that: + # 1. The transaction inside reconcile_mission_paths/1 completes before any + # profile worker executes (avoiding a deadlock where the transaction + # holds the single sandbox connection while Task.async_stream tasks + # spawned by PathCompute.compute also need the DB). + # 2. Each RoverPathProfileWorker runs individually via perform_job, which + # executes in the test process (the sandbox owner when shared: true). + defp create_and_complete_mission!(user, attrs) do + Oban.Testing.with_testing_mode(:manual, fn -> + {:ok, mission} = RoverPlanning.create_mission(user, attrs) + + mission + |> Repo.preload(stations: from(s in Station, order_by: s.position)) + |> RoverPlanning.reconcile_mission_paths() + + mission + |> RoverPlanning.list_paths() + |> Enum.each(fn path -> + perform_job(RoverPathProfileWorker, %{ + "mission_id" => mission.id, + "rover_location_id" => path.rover_location_id, + "station_id" => path.station_id, + "band_mhz" => path.band_mhz + }) + end) + + mission + end) + end + + # Runs backfill_paths/0 under manual Oban mode, then performs each + # enqueued RoverPathProfileWorker job for the given mission so paths + # land at :complete. + defp run_backfill!(mission) do + Oban.Testing.with_testing_mode(:manual, fn -> + assert :ok = RoverPlanning.backfill_paths() + + mission + |> RoverPlanning.list_paths() + |> Enum.each(fn path -> + perform_job(RoverPathProfileWorker, %{ + "mission_id" => mission.id, + "rover_location_id" => path.rover_location_id, + "station_id" => path.station_id, + "band_mhz" => path.band_mhz + }) + end) + end) + end + defp create_rover_location(attrs) do user = AccountsFixtures.user_fixture() @@ -55,7 +105,7 @@ defmodule Microwaveprop.RoverPlanningTest do _ideal_loc = create_rover_location(%{lat: 32.0, lon: -97.0, status: :good}) _off_limits = create_rover_location(%{lat: 33.0, lon: -98.0, status: :bad}) - assert {:ok, mission} = RoverPlanning.create_mission(user, valid_attrs()) + mission = create_and_complete_mission!(user, valid_attrs()) assert mission.user_id == user.id assert [station] = mission.stations assert station.grid == "EM12KP" @@ -63,31 +113,24 @@ defmodule Microwaveprop.RoverPlanningTest do assert is_float(station.lon) paths = RoverPlanning.list_paths(mission) - # only_known_good=true -> only the :good location pairs with the station. - # Oban runs inline in test so the worker has already completed. assert length(paths) == 1 - assert hd(paths).status in [:complete, :failed] + assert hd(paths).status == :complete end test "completed path caches the link-budget loss components" do user = AccountsFixtures.user_fixture() _loc = create_rover_location(%{lat: 32.0, lon: -97.0, status: :good}) - assert {:ok, mission} = RoverPlanning.create_mission(user, valid_attrs()) + mission = create_and_complete_mission!(user, valid_attrs()) [path] = RoverPlanning.list_paths(mission) assert path.status == :complete - # The result map MUST carry the cached loss-budget so the show - # page (and any future caller) can render link math without - # re-running the computation. Values come from the band's - # absorption coefficients x distance + free-space path loss + the - # already-cached terrain diffraction. Stored as floats. assert is_float(path.result["free_space_loss_db"]) assert is_float(path.result["oxygen_loss_db"]) assert is_float(path.result["humidity_loss_db_baseline"]) assert is_float(path.result["total_baseline_loss_db"]) # Free-space loss for a ~140 km, 10 GHz path is ~155 dB; sanity - # check it's in the right order of magnitude (not zero, not 1000). + # check it's in the right order of magnitude. assert path.result["free_space_loss_db"] > 100 assert path.result["free_space_loss_db"] < 200 end @@ -96,13 +139,8 @@ defmodule Microwaveprop.RoverPlanningTest do user = AccountsFixtures.user_fixture() _loc = create_rover_location(%{lat: 32.0, lon: -97.0, status: :good}) - assert {:ok, mission} = RoverPlanning.create_mission(user, valid_attrs()) + mission = create_and_complete_mission!(user, valid_attrs()) [path] = RoverPlanning.list_paths(mission) - # The worker is the gate: status only flips to :complete after the - # path-calculator pipeline (terrain + propagation lookup) has run. - # The score itself is nil in the test env (no .prop file on disk), - # but the key MUST be present so the show-page can render "---" - # without a key-existence check at every cell. assert path.status == :complete assert Map.has_key?(path.result, "propagation_score") end @@ -111,7 +149,7 @@ defmodule Microwaveprop.RoverPlanningTest do user = AccountsFixtures.user_fixture() _loc = create_rover_location(%{lat: 32.0, lon: -97.0, status: :good}) - assert {:ok, mission} = RoverPlanning.create_mission(user, valid_attrs()) + mission = create_and_complete_mission!(user, valid_attrs()) [path] = RoverPlanning.list_paths(mission) assert path.status == :complete @@ -122,10 +160,8 @@ defmodule Microwaveprop.RoverPlanningTest do |> Ecto.Changeset.change(%{status: :failed, error: "transient", result: nil}) |> Repo.update() - assert :ok = RoverPlanning.backfill_paths() + run_backfill!(mission) - # Oban inline -> backfill enqueues a job, the worker reruns - # immediately, and the path flips back to :complete. [reloaded] = RoverPlanning.list_paths(mission) assert reloaded.status == :complete assert map_size(reloaded.result) > 0 @@ -135,14 +171,14 @@ defmodule Microwaveprop.RoverPlanningTest do user = AccountsFixtures.user_fixture() _loc = create_rover_location(%{lat: 32.0, lon: -97.0, status: :good}) - assert {:ok, mission} = RoverPlanning.create_mission(user, valid_attrs()) + mission = create_and_complete_mission!(user, valid_attrs()) # Simulate a half-broken mission: paths were never persisted (e.g. # the original create_mission Multi succeeded but the Oban insert # was lost due to a crash). Backfill must repopulate the matrix. Repo.delete_all(Ecto.Query.from(p in RoverPlanning.Path, where: p.mission_id == ^mission.id)) assert RoverPlanning.list_paths(mission) == [] - assert :ok = RoverPlanning.backfill_paths() + run_backfill!(mission) assert [path] = RoverPlanning.list_paths(mission) assert path.status == :complete