From 789d4fa8846ac7e20b210ef6aab3b0545ee261ef Mon Sep 17 00:00:00 2001 From: Guarzo Date: Sat, 1 Aug 2026 19:03:01 +0000 Subject: [PATCH 1/4] fix(map): stop MapRepo.get/2 masking all errors as :not_found MapRepo.get/2 flattened every error from Api.Map.by_id/1 into {:error, :not_found}. That turned infrastructure faults into a "map does not exist" signal: a DBConnection.OwnershipError in test surfaced as "Failed to load map state" -> "map not loaded" -> "Timeout waiting for map ... Check Map.Manager is running", pointing at Map.Manager (which was running fine) instead of the real cause. Keep the :not_found translation for a genuine Ash NotFound, and propagate + log anything else. Co-Authored-By: Claude Opus 5 (1M context) --- lib/wanderer_app/repositories/map_repo.ex | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/lib/wanderer_app/repositories/map_repo.ex b/lib/wanderer_app/repositories/map_repo.ex index 9a06da84..b196c48f 100644 --- a/lib/wanderer_app/repositories/map_repo.ex +++ b/lib/wanderer_app/repositories/map_repo.ex @@ -21,8 +21,15 @@ defmodule WandererApp.MapRepo do {:ok, map} -> map |> load_relationships(relationships) - _ -> + {:error, %Ash.Error.Query.NotFound{}} -> {:error, :not_found} + + {:error, reason} = error -> + # Previously every error was flattened into {:error, :not_found}, which + # masked infrastructure faults (e.g. DBConnection ownership errors) as + # "map does not exist" and made them very hard to diagnose. + Logger.error("MapRepo.get failed for map #{inspect(map_id)}: #{inspect(reason)}") + error end end From 887769402b8f0cd1b8d874e4a4dbc73863624158 Mon Sep 17 00:00:00 2001 From: Guarzo Date: Sat, 1 Aug 2026 19:03:01 +0000 Subject: [PATCH 2/4] test(map): fix 8 failing character location tracking tests Four independent causes, each confirmed by instrumentation rather than inference. 1. Sandbox access. MapPool GenServers are spawned dynamically and load map state from the DB during init (six Task.async loaders in do_init_state/1). IntegrationCase's one-shot supervision-tree grant runs before any pool exists, and its polling monitor granted only mock ownership, never Ecto sandbox access -- and would lose the race regardless. Every loader died with DBConnection.OwnershipError. Fixed with an opt-in @moduletag :shared_sandbox. Shared mode is the only mechanism covering a process that queries immediately upon spawn. It is opt-in rather than a global flip because Ecto shared mode is node-global, and it is now reverted to :manual on exit -- not reverting is why an earlier global-flip attempt broke CommonAPIControllerTest. 2. Characters were never tracked. update_characters/1 iterates get_tracked_character_ids/1, which needs the character in map.characters AND a tracking_start_time cache key. The tests set only presence_character_ids, which that path never reads, so the tracked list was empty and the whole body was a no-op. Added track_character_on_map/2. 3. Characters were offline. With online: false the :character_online handler deletes the solar_system_id cache key that check_location_update just wrote, so no movement was ever observed. 4. Map scopes. get_effective_scopes/1 prefers the scopes array over scope, and scopes defaults to [:wormholes]. The tests set only scope: :all, so hi-sec movement was rejected as an invalid connection. Separately, these tests asserted that Jita is added to the map. Jita is hardcoded in @prohibited_systems and can never be added, so those assertions could never pass. Rather than weaken them, the start system is now Hek (not prohibited), which preserves each test's intent, and a new regression test pins the Jita prohibition itself. Verified by set-diff against merge-base b7ddbc48 at full-suite scope (two runs each, counts are unstable so sets were compared): zero new failures, all 9 target failures fixed. Suite is 15 tests, 0 failures across seeds 0, 1 and 42, with no OwnershipError and no map-start timeout remaining. Co-Authored-By: Claude Opus 5 (1M context) --- .../map/character_location_tracking_test.exs | 199 ++++++++++++------ test/support/integration_case.ex | 84 ++++++-- test/support/map_test_helpers.ex | 60 ++++++ 3 files changed, 255 insertions(+), 88 deletions(-) diff --git a/test/integration/map/character_location_tracking_test.exs b/test/integration/map/character_location_tracking_test.exs index 70b326d1..b9c2296d 100644 --- a/test/integration/map/character_location_tracking_test.exs +++ b/test/integration/map/character_location_tracking_test.exs @@ -19,6 +19,11 @@ defmodule WandererApp.Map.CharacterLocationTrackingTest do use WandererApp.IntegrationCase, async: false + # These tests start real map servers. The MapPool GenServer loads map state + # from the database during init, so it needs shared sandbox mode -- see + # WandererApp.IntegrationCase. + @moduletag :shared_sandbox + import Mox setup :verify_on_exit! @@ -26,15 +31,29 @@ defmodule WandererApp.Map.CharacterLocationTrackingTest do import WandererApp.MapTestHelpers alias WandererApp.Map.Server.CharactersImpl - alias WandererApp.Map.Server.SystemsImpl @test_character_eve_id 2_123_456_789 - # EVE Online solar system IDs for testing - @system_jita 30_000_142 + # EVE Online solar system IDs for testing. + # + # NOTE: Jita (30_000_142) is deliberately NOT used here. It is hardcoded as + # permanently un-addable in production: + # + # # map_server_connections_impl.ex + # @jita 30_000_142 + # @prohibited_systems [@jita] + # + # These tests previously used Hek as the "start system" and asserted it was + # added to the map, which asserts behaviour the application forbids by design + # and can never pass. Hek is a plain hi-sec system with no such restriction, so + # the intent of each test (a system the character moved away from gets added) + # is preserved while testing behaviour that is actually reachable. + # The prohibition itself is pinned by its own regression test below. + @system_hek 30_002_053 @system_amarr 30_002_187 @system_dodixie 30_002_659 @system_rens 30_002_510 + @system_jita_prohibited 30_000_142 setup do # Setup system static info cache for test systems @@ -65,6 +84,11 @@ defmodule WandererApp.Map.CharacterLocationTrackingTest do slug: "test-char-tracking-#{:rand.uniform(1_000_000)}", owner_id: character.id, scope: :all, + # `scopes` (array) takes precedence over `scope` in + # CharactersImpl.get_effective_scopes/1 and defaults to [:wormholes]. + # Without setting it, hi-sec movement is rejected as an invalid + # connection and no system is ever added. + scopes: [:hi, :low, :null, :pochven, :wormholes], only_tracked_characters: false }) @@ -93,7 +117,7 @@ defmodule WandererApp.Map.CharacterLocationTrackingTest do @tag :integration test "character location update adds system to map", %{map: map, character: character} do # This test verifies the basic flow: - # 1. Character starts tracking on a map at Jita + # 1. Character starts tracking on a map at Hek # 2. Character moves to Amarr # 3. update_characters() is called # 4. Both systems are added to the map @@ -102,23 +126,23 @@ defmodule WandererApp.Map.CharacterLocationTrackingTest do ensure_map_started(map.id) # Setup: Add character to presence - add_character_to_map_presence(map.id, character.id) + track_character_on_map(map.id, character.id) - # Setup: Character starts at Jita - set_character_location(character.id, @system_jita) + # Setup: Character starts at Hek + set_character_location(character.id, @system_hek) # Setup: Set start_solar_system_id (this happens when tracking starts) # Note: The start system is NOT added until the character moves WandererApp.Cache.insert( "map:#{map.id}:character:#{character.id}:start_solar_system_id", - @system_jita + @system_hek ) # Execute: First update - start system is intentionally NOT added yet CharactersImpl.update_characters(map.id) - # Verify: Jita should NOT be on map yet (design: start position not added) - refute system_on_map?(map.id, @system_jita), + # Verify: Hek should NOT be on map yet (design: start position not added) + refute system_on_map?(map.id, @system_hek), "Start system should not be added until character moves" # Character moves to Amarr @@ -128,8 +152,8 @@ defmodule WandererApp.Map.CharacterLocationTrackingTest do CharactersImpl.update_characters(map.id) # Verify: Both systems should now be on map - assert wait_for_system_on_map(map.id, @system_jita), - "Jita should be added after character moves" + assert wait_for_system_on_map(map.id, @system_hek), + "Hek should be added after character moves" assert wait_for_system_on_map(map.id, @system_amarr), "Amarr should be added as the new location" @@ -148,20 +172,20 @@ defmodule WandererApp.Map.CharacterLocationTrackingTest do ensure_map_started(map.id) # Setup: Add character to presence - add_character_to_map_presence(map.id, character.id) + track_character_on_map(map.id, character.id) - # Setup: Character starts at Jita - set_character_location(character.id, @system_jita) + # Setup: Character starts at Hek + set_character_location(character.id, @system_hek) WandererApp.Cache.insert( "map:#{map.id}:character:#{character.id}:start_solar_system_id", - @system_jita + @system_hek ) # First update - start system is intentionally NOT added yet CharactersImpl.update_characters(map.id) - refute system_on_map?(map.id, @system_jita), + refute system_on_map?(map.id, @system_hek), "Start system should not be added until character moves" # Character moves to Amarr @@ -171,8 +195,8 @@ defmodule WandererApp.Map.CharacterLocationTrackingTest do CharactersImpl.update_characters(map.id) # Verify: Both systems should be on map after character moves - assert wait_for_system_on_map(map.id, @system_jita), - "Jita should be added after character moves" + assert wait_for_system_on_map(map.id, @system_hek), + "Hek should be added after character moves" assert wait_for_system_on_map(map.id, @system_amarr), "Amarr should be added as the new location" @@ -188,31 +212,31 @@ defmodule WandererApp.Map.CharacterLocationTrackingTest do # Note: Start system is NOT added until character moves (design decision) ensure_map_started(map.id) - add_character_to_map_presence(map.id, character.id) + track_character_on_map(map.id, character.id) - # Character starts at Jita - set_character_location(character.id, @system_jita) + # Character starts at Hek + set_character_location(character.id, @system_hek) WandererApp.Cache.insert( "map:#{map.id}:character:#{character.id}:start_solar_system_id", - @system_jita + @system_hek ) # First update - start system is intentionally NOT added yet CharactersImpl.update_characters(map.id) - refute system_on_map?(map.id, @system_jita), + refute system_on_map?(map.id, @system_hek), "Start system should not be added until character moves" # Rapid jump to Amarr (intermediate system) set_character_location(character.id, @system_amarr) - # Second update - should add both Jita (start) and Amarr (current) + # Second update - should add both Hek (start) and Amarr (current) CharactersImpl.update_characters(map.id) - # Verify both Jita and Amarr are now on map - assert wait_for_system_on_map(map.id, @system_jita), - "Jita (start) should be on map after movement" + # Verify both Hek and Amarr are now on map + assert wait_for_system_on_map(map.id, @system_hek), + "Hek (start) should be on map after movement" assert wait_for_system_on_map(map.id, @system_amarr), "Amarr should be on map" @@ -223,7 +247,7 @@ defmodule WandererApp.Map.CharacterLocationTrackingTest do CharactersImpl.update_characters(map.id) # Verify: All three systems should be on map - assert wait_for_system_on_map(map.id, @system_jita), "Jita (start) should still be on map" + assert wait_for_system_on_map(map.id, @system_hek), "Hek (start) should still be on map" assert wait_for_system_on_map(map.id, @system_amarr), "Amarr (intermediate) should still be on map - this is the critical test" @@ -240,21 +264,21 @@ defmodule WandererApp.Map.CharacterLocationTrackingTest do # don't cause intermediate systems to be lost due to cache races. ensure_map_started(map.id) - add_character_to_map_presence(map.id, character.id) + track_character_on_map(map.id, character.id) - # Start at Jita - set_character_location(character.id, @system_jita) + # Start at Hek + set_character_location(character.id, @system_hek) WandererApp.Cache.insert( "map:#{map.id}:character:#{character.id}:start_solar_system_id", - @system_jita + @system_hek ) CharactersImpl.update_characters(map.id) # Simulate rapid updates happening faster than update_characters cycle (1 second) # Jump through 4 systems in quick succession - systems = [@system_amarr, @system_dodixie, @system_rens, @system_jita] + systems = [@system_amarr, @system_dodixie, @system_rens, @system_hek] for system <- systems do set_character_location(character.id, system) @@ -266,7 +290,7 @@ defmodule WandererApp.Map.CharacterLocationTrackingTest do # Verify: All systems should eventually be on the map # Even if some updates happened concurrently - for system <- [@system_jita | systems] do + for system <- [@system_hek | systems] do assert wait_for_system_on_map(map.id, system), "System #{system} should be on map despite rapid movements" end @@ -283,15 +307,15 @@ defmodule WandererApp.Map.CharacterLocationTrackingTest do # start_solar_system_id should not be lost after first use ensure_map_started(map.id) - add_character_to_map_presence(map.id, character.id) + track_character_on_map(map.id, character.id) - # Set character at Jita - set_character_location(character.id, @system_jita) + # Set character at Hek + set_character_location(character.id, @system_hek) # Set start_solar_system_id WandererApp.Cache.insert( "map:#{map.id}:character:#{character.id}:start_solar_system_id", - @system_jita + @system_hek ) # First update @@ -301,7 +325,7 @@ defmodule WandererApp.Map.CharacterLocationTrackingTest do {:ok, start_system} = WandererApp.Cache.lookup("map:#{map.id}:character:#{character.id}:start_solar_system_id") - assert start_system == @system_jita, + assert start_system == @system_hek, "start_solar_system_id should persist after first update (not be taken/removed)" # Character moves to Amarr @@ -311,7 +335,7 @@ defmodule WandererApp.Map.CharacterLocationTrackingTest do CharactersImpl.update_characters(map.id) # Verify both systems are on map - assert wait_for_system_on_map(map.id, @system_jita) + assert wait_for_system_on_map(map.id, @system_hek) assert wait_for_system_on_map(map.id, @system_amarr) end @@ -325,22 +349,22 @@ defmodule WandererApp.Map.CharacterLocationTrackingTest do # Design: Start system is NOT added until character moves ensure_map_started(map.id) - add_character_to_map_presence(map.id, character.id) + track_character_on_map(map.id, character.id) - # Character is at Jita, no previous location - set_character_location(character.id, @system_jita) + # Character is at Hek, no previous location + set_character_location(character.id, @system_hek) # Set start_solar_system_id WandererApp.Cache.insert( "map:#{map.id}:character:#{character.id}:start_solar_system_id", - @system_jita + @system_hek ) # First update - character still at start position CharactersImpl.update_characters(map.id) - # Verify Jita is NOT added yet (design: start position not added until movement) - refute system_on_map?(map.id, @system_jita), + # Verify Hek is NOT added yet (design: start position not added until movement) + refute system_on_map?(map.id, @system_hek), "Start system should not be added until character moves" # Character moves to Amarr @@ -350,8 +374,8 @@ defmodule WandererApp.Map.CharacterLocationTrackingTest do CharactersImpl.update_characters(map.id) # Verify both systems are added after movement - assert wait_for_system_on_map(map.id, @system_jita), - "Jita should be added after character moves away" + assert wait_for_system_on_map(map.id, @system_hek), + "Hek should be added after character moves away" assert wait_for_system_on_map(map.id, @system_amarr), "Amarr should be added as the new location" @@ -366,7 +390,7 @@ defmodule WandererApp.Map.CharacterLocationTrackingTest do # (Retry logic not yet implemented) ensure_map_started(map.id) - add_character_to_map_presence(map.id, character.id) + track_character_on_map(map.id, character.id) test_pid = self() @@ -380,12 +404,12 @@ defmodule WandererApp.Map.CharacterLocationTrackingTest do nil ) - # Set character at Jita and set start location - set_character_location(character.id, @system_jita) + # Set character at Hek and set start location + set_character_location(character.id, @system_hek) WandererApp.Cache.insert( "map:#{map.id}:character:#{character.id}:start_solar_system_id", - @system_jita + @system_hek ) # Trigger update which may encounter database issues @@ -411,14 +435,14 @@ defmodule WandererApp.Map.CharacterLocationTrackingTest do # and logged without crashing the entire update_characters cycle ensure_map_started(map.id) - add_character_to_map_presence(map.id, character.id) + track_character_on_map(map.id, character.id) # Set up character location - set_character_location(character.id, @system_jita) + set_character_location(character.id, @system_hek) WandererApp.Cache.insert( "map:#{map.id}:character:#{character.id}:start_solar_system_id", - @system_jita + @system_hek ) # Run update_characters - should complete even if individual character updates fail @@ -478,14 +502,14 @@ defmodule WandererApp.Map.CharacterLocationTrackingTest do # Note: Recovery ETS table not yet implemented ensure_map_started(map.id) - add_character_to_map_presence(map.id, character.id) + track_character_on_map(map.id, character.id) # Set up character with location - set_character_location(character.id, @system_jita) + set_character_location(character.id, @system_hek) WandererApp.Cache.insert( "map:#{map.id}:character:#{character.id}:start_solar_system_id", - @system_jita + @system_hek ) # Run multiple update cycles to verify stability @@ -528,10 +552,10 @@ defmodule WandererApp.Map.CharacterLocationTrackingTest do }) # Add character to presence and set location - add_character_to_map_presence(map.id, character.id) + track_character_on_map(map.id, character.id) solar_system_id = - Enum.at([@system_jita, @system_amarr, @system_dodixie, @system_rens], rem(i, 4)) + Enum.at([@system_hek, @system_amarr, @system_dodixie, @system_rens], rem(i, 4)) set_character_location(character.id, solar_system_id) @@ -563,7 +587,7 @@ defmodule WandererApp.Map.CharacterLocationTrackingTest do # emit proper telemetry events for monitoring ensure_map_started(map.id) - add_character_to_map_presence(map.id, character.id) + track_character_on_map(map.id, character.id) test_pid = self() @@ -582,7 +606,7 @@ defmodule WandererApp.Map.CharacterLocationTrackingTest do ) # Set up character location - set_character_location(character.id, @system_jita) + set_character_location(character.id, @system_hek) # Trigger update_characters CharactersImpl.update_characters(map.id) @@ -615,14 +639,14 @@ defmodule WandererApp.Map.CharacterLocationTrackingTest do # 3. Cachex.get(:character_state_cache, character_id) - character state ensure_map_started(map.id) - add_character_to_map_presence(map.id, character.id) + track_character_on_map(map.id, character.id) # Set location in character cache - set_character_location(character.id, @system_jita) + set_character_location(character.id, @system_hek) WandererApp.Cache.insert( "map:#{map.id}:character:#{character.id}:start_solar_system_id", - @system_jita + @system_hek ) CharactersImpl.update_characters(map.id) @@ -631,7 +655,7 @@ defmodule WandererApp.Map.CharacterLocationTrackingTest do {:ok, map_cached_location} = WandererApp.Cache.lookup("map:#{map.id}:character:#{character.id}:solar_system_id") - assert map_cached_location == @system_jita, + assert map_cached_location == @system_hek, "Map-specific cache should match character cache" # Move character @@ -685,7 +709,7 @@ defmodule WandererApp.Map.CharacterLocationTrackingTest do %{ character_id: character.id, map_id: map.id, - from_system: @system_jita, + from_system: @system_hek, to_system: @system_amarr } ) @@ -702,4 +726,43 @@ defmodule WandererApp.Map.CharacterLocationTrackingTest do :telemetry.detach("test-character-location-events") end end + + describe "Prohibited systems" do + @tag :integration + test "Jita is never added to a map, even when a character moves through it", + %{map: map, character: character} do + # Pins the reason every other test in this module uses Hek rather than + # Jita as its start system. Jita is hardcoded as permanently un-addable + # (`@prohibited_systems [@jita]` in map_server_connections_impl.ex). + # + # If this test starts failing, the prohibition was changed or removed -- + # revisit the constants at the top of this file rather than deleting this. + ensure_map_started(map.id) + track_character_on_map(map.id, character.id) + + set_character_location(character.id, @system_jita_prohibited) + + WandererApp.Cache.insert( + "map:#{map.id}:character:#{character.id}:start_solar_system_id", + @system_jita_prohibited + ) + + CharactersImpl.update_characters(map.id) + + # Move away from Jita. For any non-prohibited system this is exactly the + # sequence that causes the start system to be added to the map. + set_character_location(character.id, @system_amarr) + CharactersImpl.update_characters(map.id) + + # The prohibition is evaluated per-connection, not per-system: Jita being + # prohibited invalidates the whole Jita->Amarr movement, so neither + # endpoint is added. Assert both, so the test pins the actual semantics + # rather than a convenient half of them. + refute system_on_map?(map.id, @system_jita_prohibited), + "Jita is in @prohibited_systems and must never be added to a map" + + refute system_on_map?(map.id, @system_amarr), + "A connection touching a prohibited system is rejected entirely" + end + end end diff --git a/test/support/integration_case.ex b/test/support/integration_case.ex index bebfa7b3..2850e4dd 100644 --- a/test/support/integration_case.ex +++ b/test/support/integration_case.ex @@ -2,17 +2,19 @@ defmodule WandererApp.IntegrationCase do @moduledoc """ This module defines the test case for integration tests. - Integration tests use shared sandbox mode (`shared: true`) when running async - to avoid timing issues with dynamically spawned processes like MapPool GenServers - that need database access immediately upon spawn. + Integration tests default to a private sandbox owner. Suites that start real + map servers must opt into shared sandbox mode: - For async integration tests, shared mode allows: - - MapPool GenServers to access the database without explicit allowance - - Tests to run in parallel without complex permission granting - - Reliable test execution without race conditions + use WandererApp.IntegrationCase, async: false + @moduletag :shared_sandbox - For synchronous integration tests, shared mode is disabled (shared: false) - for better isolation. + Shared mode is required when a dynamically spawned process queries the + database immediately upon spawn (MapPool GenServers load map state during + `init`), because there is no window in which the test process can allow it + onto the connection first. + + Shared mode is node-global, so it is opt-in and reverted on exit. It is only + valid with `async: false`. Use this case for: - API controller integration tests that spawn map servers @@ -78,20 +80,39 @@ defmodule WandererApp.IntegrationCase do {:ok, _} = WandererApp.Repo.start_link() end - # For integration tests: - # - Use shared: true for async tests to avoid MapPool timing issues - # - Use shared: false for sync tests for better isolation - shared_mode = tags[:async] == true + # Shared mode is opt-in per suite via `@moduletag :shared_sandbox`. + # + # Suites that start real map servers need it: MapPool GenServers are spawned + # dynamically and load map state from the DB *during init*, so there is no + # point at which the test process can allow them onto the connection first + # (polling loses the race). Shared mode is the only mechanism that covers a + # process that queries immediately upon spawn. + # + # It is opt-in rather than global because Sandbox shared mode applies to the + # whole node: enabling it for every sync integration suite regresses suites + # that rely on owner-private connections. Ecto only permits shared mode when + # the test is not async, so the tag is rejected on async suites. + shared_mode = tags[:shared_sandbox] == true + + if shared_mode and tags[:async] == true do + raise ArgumentError, + "#{inspect(tags[:module])} sets @moduletag :shared_sandbox but is `async: true`. " <> + "Ecto sandbox shared mode is node-global and is only safe with `async: false`." + end # Set up sandbox mode based on test type pid = if shared_mode do - # For async tests with shared mode: - # Checkout the sandbox connection instead of starting an owner - # This allows multiple async tests to use the same connection pool :ok = Ecto.Adapters.SQL.Sandbox.checkout(WandererApp.Repo) - # Put the connection in shared mode Ecto.Adapters.SQL.Sandbox.mode(WandererApp.Repo, {:shared, self()}) + + # Shared mode is node-global, so it MUST be reverted to :manual when the + # test ends. Leaving it set leaks into every later suite on the node and + # is why an earlier global-flip attempt broke unrelated controller tests. + on_exit(fn -> + Ecto.Adapters.SQL.Sandbox.mode(WandererApp.Repo, :manual) + end) + self() else # For sync tests, start a dedicated owner @@ -117,11 +138,17 @@ defmodule WandererApp.IntegrationCase do Allows a process to access the database by granting it sandbox access. This is necessary for background processes that need database access in non-shared mode. """ - def allow_database_access(pid) when is_pid(pid) do - owner_pid = Process.get(:sandbox_owner_pid) + def allow_database_access(pid, owner_pid \\ nil) when is_pid(pid) do + owner_pid = owner_pid || Process.get(:sandbox_owner_pid) if owner_pid do - Ecto.Adapters.SQL.Sandbox.allow(WandererApp.Repo, owner_pid, pid) + # Already-allowed processes raise; re-allowing on every poll tick is + # expected, so treat that as success rather than crashing the monitor. + try do + Ecto.Adapters.SQL.Sandbox.allow(WandererApp.Repo, owner_pid, pid) + rescue + _ -> :ok + end end end @@ -192,6 +219,23 @@ defmodule WandererApp.IntegrationCase do |> Enum.filter(&is_pid/1) |> Enum.filter(&Process.alive?/1) |> Enum.each(fn child_pid -> + # Grant BOTH mock ownership and Ecto sandbox access. + # + # MapPool GenServers are spawned dynamically *after* the one-shot + # grant_supervision_tree_access/2 call above has already run, so + # they are never on the sandbox connection. Their map-state + # loaders (Task.async in Map.Server.Impl.do_init_state/1) then die + # with DBConnection.OwnershipError, the error is reported as + # "map not loaded", and the test surfaces only a misleading + # "Timeout waiting for map ... Check Map.Manager is running". + # + # Sandbox.allow/3 also propagates to the pool's Task.async + # children via $callers, which is what the loaders rely on. + # + # owner_pid is passed explicitly: this runs inside the spawned + # monitor process, where the :sandbox_owner_pid process dict entry + # set by setup_sandbox/1 is not visible. + allow_database_access(child_pid, owner_pid) WandererApp.Test.MockOwnership.allow_mocks_for_process(child_pid, owner_pid) end) diff --git a/test/support/map_test_helpers.ex b/test/support/map_test_helpers.ex index 31c3453d..b85110fe 100644 --- a/test/support/map_test_helpers.ex +++ b/test/support/map_test_helpers.ex @@ -322,6 +322,29 @@ defmodule WandererApp.MapTestHelpers do wandering: [], triglavian_invasion_status: nil, sun_type_id: 45041 + }, + # Hek. Used as the default "start system" in location-tracking tests + # because, unlike Jita, it is not in @prohibited_systems and so can + # actually be added to a map. + 30_002_053 => %{ + solar_system_id: 30_002_053, + region_id: 10_000_042, + constellation_id: 20_000_302, + solar_system_name: "Hek", + solar_system_name_lc: "hek", + constellation_name: "Hedgehog", + region_name: "Metropolis", + system_class: 0, + security: "0.5", + type_description: "High Security", + class_title: "High Sec", + is_shattered: false, + effect_name: nil, + effect_power: nil, + statics: [], + wandering: [], + triglavian_invasion_status: nil, + sun_type_id: 45041 } } end @@ -349,12 +372,18 @@ defmodule WandererApp.MapTestHelpers do {:ok, existing_character} = WandererApp.Character.get_character(character_id) # Update character cache (mimics Character.update_character/2) + # `online: true` is required, not cosmetic. When a character is offline the + # `{:character_online, ...}` handler in CharactersImpl DELETES the + # `map::character::solar_system_id` cache key that + # `check_location_update` just wrote, so no location change is ever observed + # and no system is added. Characters reporting a location are online. character_data = Map.merge(existing_character, %{ solar_system_id: solar_system_id, structure_id: structure_id, station_id: station_id, ship: ship, + online: Keyword.get(opts, :online, true), updated_at: DateTime.utc_now() }) @@ -379,6 +408,37 @@ defmodule WandererApp.MapTestHelpers do WandererApp.Cache.insert("map_#{map_id}:presence_character_ids", updated_chars) end + @doc """ + Makes a character actually tracked for `update_characters/1`. + + Presence alone is NOT enough. `WandererApp.Map.get_tracked_character_ids/1` + requires both: + + * the character to be registered on the map (`map.characters`), and + * a `character::map::tracking_start_time` cache key + + Production sets these via `CharactersImpl.track_character/2`, which needs real + `MapCharacterSettings` rows and ESI access tokens. Tests short-circuit to the + same end state, mirroring `Character.TrackingUtils.track_character/3`. + + Without this, `update_characters/1` iterates an empty list and silently does + nothing, which reads as "the feature is broken" rather than "the character was + never tracked". + """ + def track_character_on_map(map_id, character_id) do + add_character_to_map_presence(map_id, character_id) + + {:ok, character} = WandererApp.Character.get_character(character_id) + :ok = WandererApp.Map.add_character(map_id, character) + + WandererApp.Cache.put( + "character:#{character_id}:map:#{map_id}:tracking_start_time", + DateTime.utc_now() + ) + + :ok + end + @doc """ Helper to get all systems currently on the map. Uses :map_cache instead of :map_state_cache because add_system/2 updates :map_cache. From 33acc374b4baf9c989804b1c457050a9828166c9 Mon Sep 17 00:00:00 2001 From: Guarzo Date: Sat, 1 Aug 2026 19:29:25 +0000 Subject: [PATCH 3/4] fix(map): match Ash-wrapped NotFound in MapRepo.get/2 Review of the previous commit found that Ash returns NotFound wrapped inside Ash.Error.Invalid, not as a bare struct. Verified against a live query: `Api.Map.by_id()` returns {:error, %Ash.Error.Invalid{errors: [%Ash.Error.Query.NotFound{}]}} so the bare-NotFound clause never matched. A genuinely missing map would have propagated as a generic error and logged a spurious error line. Match the wrapped shape as well. Confirmed `MapRepo.get/2` now returns {:error, :not_found} for a missing map with no error log. No caller depends on the :not_found atom specifically (all use wildcard or with/else), so propagation of real faults is unaffected. Also make track_character_on_map/2 clear stale per-map location caches, mirroring TrackingUtils.track_character/4, which the helper claimed to mirror but did not. Co-Authored-By: Claude Opus 5 (1M context) --- lib/wanderer_app/repositories/map_repo.ex | 25 ++++++++++++++++++----- test/support/map_test_helpers.ex | 8 ++++++++ 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/lib/wanderer_app/repositories/map_repo.ex b/lib/wanderer_app/repositories/map_repo.ex index b196c48f..a99d441a 100644 --- a/lib/wanderer_app/repositories/map_repo.ex +++ b/lib/wanderer_app/repositories/map_repo.ex @@ -21,18 +21,33 @@ defmodule WandererApp.MapRepo do {:ok, map} -> map |> load_relationships(relationships) + # Ash wraps NotFound inside Ash.Error.Invalid, so match the wrapped shape + # as well as a bare NotFound. Getting this wrong turns a genuine + # "map does not exist" into a generic error for every caller. {:error, %Ash.Error.Query.NotFound{}} -> {:error, :not_found} - {:error, reason} = error -> - # Previously every error was flattened into {:error, :not_found}, which - # masked infrastructure faults (e.g. DBConnection ownership errors) as - # "map does not exist" and made them very hard to diagnose. - Logger.error("MapRepo.get failed for map #{inspect(map_id)}: #{inspect(reason)}") + {:error, %Ash.Error.Invalid{errors: errors}} = error -> + if Enum.any?(errors, &match?(%Ash.Error.Query.NotFound{}, &1)) do + {:error, :not_found} + else + log_get_failure(map_id, error) + error + end + + {:error, _reason} = error -> + log_get_failure(map_id, error) error end end + # Previously every error was flattened into {:error, :not_found}, which masked + # infrastructure faults (e.g. DBConnection ownership errors) as "map does not + # exist" and made them very hard to diagnose. + defp log_get_failure(map_id, {:error, reason}) do + Logger.error("MapRepo.get failed for map #{inspect(map_id)}: #{inspect(reason)}") + end + def get_by_slug_with_permissions(map_slug, current_user) do map_slug |> WandererApp.Api.Map.get_map_by_slug!() diff --git a/test/support/map_test_helpers.ex b/test/support/map_test_helpers.ex index b85110fe..7700f6ca 100644 --- a/test/support/map_test_helpers.ex +++ b/test/support/map_test_helpers.ex @@ -436,6 +436,14 @@ defmodule WandererApp.MapTestHelpers do DateTime.utc_now() ) + # Production clears stale per-map location caches when tracking starts + # (TrackingUtils.track_character/4). Mirror it, so a character tracked after + # a previous location was cached does not appear to have "already been" at + # that system. + WandererApp.Cache.delete("map:#{map_id}:character:#{character_id}:solar_system_id") + WandererApp.Cache.delete("map:#{map_id}:character:#{character_id}:station_id") + WandererApp.Cache.delete("map:#{map_id}:character:#{character_id}:structure_id") + :ok end From a3777b390a41205cb30e6de48f8baa0059561020 Mon Sep 17 00:00:00 2001 From: Guarzo Date: Sat, 8 Aug 2026 16:56:57 -0400 Subject: [PATCH 4/4] fix(test): address upstream review findings - correct the Jita/Hek comment to match the systems the tests actually use - document the sandbox contract on IntegrationCase and make shared mode an explicit opt-in that is reverted on exit - drop the blanket rescue around Sandbox.allow/4: it returns {:already, :owner | :allowed} rather than raising, so the rescue only masked genuine infrastructure faults - use Cache.insert/3 and document the :online option --- .../map/character_location_tracking_test.exs | 2 +- test/support/integration_case.ex | 28 ++++++++++--------- test/support/map_test_helpers.ex | 4 +-- 3 files changed, 18 insertions(+), 16 deletions(-) diff --git a/test/integration/map/character_location_tracking_test.exs b/test/integration/map/character_location_tracking_test.exs index b9c2296d..4420fc6a 100644 --- a/test/integration/map/character_location_tracking_test.exs +++ b/test/integration/map/character_location_tracking_test.exs @@ -43,7 +43,7 @@ defmodule WandererApp.Map.CharacterLocationTrackingTest do # @jita 30_000_142 # @prohibited_systems [@jita] # - # These tests previously used Hek as the "start system" and asserted it was + # These tests previously used Jita as the "start system" and asserted it was # added to the map, which asserts behaviour the application forbids by design # and can never pass. Hek is a plain hi-sec system with no such restriction, so # the intent of each test (a system the character moved away from gets added) diff --git a/test/support/integration_case.ex b/test/support/integration_case.ex index 2850e4dd..047124f8 100644 --- a/test/support/integration_case.ex +++ b/test/support/integration_case.ex @@ -64,15 +64,18 @@ defmodule WandererApp.IntegrationCase do end @doc """ - Sets up the sandbox with shared mode for async integration tests. + Sets up the test sandbox, per the `:shared_sandbox` moduletag. - For async tests (async: true): - - Uses shared: true to allow dynamically spawned processes database access - - Trades some isolation for reliability and simplicity + With `@moduletag :shared_sandbox` (only valid with `async: false`): + - Uses shared: true so dynamically spawned processes (e.g. MapPool + GenServers that query the DB during `init`) get database access + - Trades some isolation for reliability with background processes - For sync tests (async: false): - - Uses shared: false for better isolation + Without the tag (the default): + - Starts a dedicated, private sandbox owner (shared: false) - Child processes require explicit allowance + + Raises `ArgumentError` if `:shared_sandbox` is set on an `async: true` suite. """ def setup_sandbox(tags) do # Ensure the repo is started before setting up sandbox @@ -142,13 +145,12 @@ defmodule WandererApp.IntegrationCase do owner_pid = owner_pid || Process.get(:sandbox_owner_pid) if owner_pid do - # Already-allowed processes raise; re-allowing on every poll tick is - # expected, so treat that as success rather than crashing the monitor. - try do - Ecto.Adapters.SQL.Sandbox.allow(WandererApp.Repo, owner_pid, pid) - rescue - _ -> :ok - end + # Returns `:ok | {:already, :owner | :allowed}` on every poll tick; + # re-allowing an already-allowed process is a normal, non-error case, so + # both are treated as success here. It raises only for infrastructure + # faults (repo not started, or `pid`/`owner_pid` not resolving to a live + # process) -- those should surface rather than be swallowed. + Ecto.Adapters.SQL.Sandbox.allow(WandererApp.Repo, owner_pid, pid) end end diff --git a/test/support/map_test_helpers.ex b/test/support/map_test_helpers.ex index 7700f6ca..ba31fdb2 100644 --- a/test/support/map_test_helpers.ex +++ b/test/support/map_test_helpers.ex @@ -356,7 +356,7 @@ defmodule WandererApp.MapTestHelpers do ## Parameters - character_id: The character ID to update - solar_system_id: The solar system ID where the character is located - - opts: Optional parameters (structure_id, station_id, ship) + - opts: Optional parameters (structure_id, station_id, ship, online) ## Examples iex> set_character_location(character.id, 30_000_142, ship: 670) @@ -431,7 +431,7 @@ defmodule WandererApp.MapTestHelpers do {:ok, character} = WandererApp.Character.get_character(character_id) :ok = WandererApp.Map.add_character(map_id, character) - WandererApp.Cache.put( + WandererApp.Cache.insert( "character:#{character_id}:map:#{map_id}:tracking_start_time", DateTime.utc_now() )