Merge branch 'fix-issue-1460' into 'master'

[FIX] Race condition creating duplicate EveOnline entities across workers

Closes #1460

See merge request allianceauth/allianceauth!1876

allianceauth/authentication/managers.py

@@ -23,9 +23,8 @@ def available_states_query(character):
 
 class CharacterOwnershipManager(Manager):
     def create_by_token(self, token):
-        if not EveCharacter.objects.filter(character_id=token.character_id).exists():
-            EveCharacter.objects.create_character(token.character_id)
-        return self.create(character=EveCharacter.objects.get(character_id=token.character_id), user=token.user, owner_hash=token.character_owner_hash)
+        character = EveCharacter.objects.get_or_create_esi(token.character_id)
+        return self.create(character=character, user=token.user, owner_hash=token.character_owner_hash)
 
 
 class StateQuerySet(QuerySet):

allianceauth/authentication/signals.py

@@ -95,11 +95,8 @@ def record_character_ownership(sender, instance, created, *args, **kwargs):
             query = Q(owner_hash=instance.character_owner_hash)
         # purge ownership records if the hash or auth user account has changed
         CharacterOwnership.objects.filter(character__character_id=instance.character_id).exclude(query).delete()
-        # create character if needed
-        if EveCharacter.objects.filter(character_id=instance.character_id).exists() is False:
-            logger.debug(f'Token is for a new character. Creating model for {instance.character_name} ({instance.character_id})')
-            EveCharacter.objects.create_character(instance.character_id)
-        char = EveCharacter.objects.get(character_id=instance.character_id)
+        # create character if needed (race-safe: a concurrent token save may create it first)
+        char = EveCharacter.objects.get_or_create_esi(instance.character_id)
         # check if we need to create ownership
         if instance.user and not CharacterOwnership.objects.filter(
                 character__character_id=instance.character_id).exists():

allianceauth/corputils/views.py

@@ -33,10 +33,7 @@ def corpstats_add(request, token) -> HttpResponseRedirect:
             corp_id = EveCharacter.objects.get(character_id=token.character_id).corporation_id
         else:
             corp_id = get_characters_character_id(token.character_id).corporation_id
-        try:
-            corp = EveCorporationInfo.objects.get(corporation_id=corp_id)
-        except EveCorporationInfo.DoesNotExist:
-            corp = EveCorporationInfo.objects.create_corporation(corp_id=corp_id, use_etag=False)
+        corp = EveCorporationInfo.objects.get_or_create_esi(corporation_id=corp_id)
         cs = CorpStats.objects.create(token=token, corp=corp)
         try:
             cs.update()

allianceauth/eveonline/autogroups/models.py

@@ -130,7 +130,7 @@ class AutogroupsConfig(models.Model):
                 group = self.get_alliance_group(alliance)
         except EveAllianceInfo.DoesNotExist:
             logger.debug(f'User {user} main characters alliance does not exist in the database. Creating.')
-            alliance = EveAllianceInfo.objects.create_alliance(user.profile.main_character.alliance_id)
+            alliance = EveAllianceInfo.objects.get_or_create_esi(user.profile.main_character.alliance_id)
             group = self.get_alliance_group(alliance)
         except AttributeError:
             logger.warning(f'User {user} does not have a main character. Group membership not updated')
@@ -151,7 +151,7 @@ class AutogroupsConfig(models.Model):
                 group = self.get_corp_group(corp)
         except EveCorporationInfo.DoesNotExist:
             logger.debug(f'User {user} main characters corporation does not exist in the database. Creating.')
-            corp = EveCorporationInfo.objects.create_corporation(
+            corp = EveCorporationInfo.objects.get_or_create_esi(
                 corporation_id=user.profile.main_character.corporation_id
             )
             group = self.get_corp_group(corp)

allianceauth/eveonline/autogroups/tests/test_models.py

@@ -105,25 +105,28 @@ class AutogroupsConfigTestCase(TestCase):
 
         self.assertNotIn(group, self.member.groups.all())
 
-    # todo: this test case currently does not work, because it forces
-    # an exception during a transaction, which is not easily testable
-    # the production code itself should be fine though
-    # I therefore commented out the test case for now
-    """
     @patch('.models.EveAllianceInfo.objects.create_alliance')
-    def test_update_alliance_group_membership_no_alliance_model(
+    def test_update_alliance_group_membership_creates_missing_alliance(
         self,
         mock_create_alliance
     ):
-        def mock_create_alliance_side_effect(*args, **kwargs):
-            return EveAllianceInfo.objects.create(
-                alliance_id='3459',
-                alliance_name='alliance name',
-                alliance_ticker='alliance_ticker',
-                executor_corp_id='2345'
-            )
+        # The character's alliance is not yet in the database, so accessing the
+        # alliance property raises DoesNotExist and the membership update must
+        # create it via the race-safe get_or_create_esi() helper before
+        # assigning the group. This previously could not be tested because the
+        # create happened outside a savepoint; get_or_create_esi() fixes that.
+        created_alliance = EveAllianceInfo(
+            alliance_id='3459',
+            alliance_name='created alliance',
+            alliance_ticker='NEW',
+            executor_corp_id='2345',
+        )
 
-        mock_create_alliance.side_effect = mock_create_alliance_side_effect
+        def create_side_effect(*args, **kwargs):
+            created_alliance.save()
+            return created_alliance
+
+        mock_create_alliance.side_effect = create_side_effect
 
         obj = AutogroupsConfig.objects.create(alliance_groups=True)
         obj.states.add(AuthUtils.get_member_state())
@@ -134,7 +137,7 @@ class AutogroupsConfigTestCase(TestCase):
             corporation_name='test corp',
             corporation_ticker='tickr',
             alliance_id='3459',
-            alliance_name='alliance name',
+            alliance_name='created alliance',
         )
         self.member.profile.main_character = char
         self.member.profile.save()
@@ -142,10 +145,10 @@ class AutogroupsConfigTestCase(TestCase):
         # Act
         obj.update_alliance_group_membership(self.member)
 
-        group = obj.get_alliance_group(self.alliance)
-
-        self.assertNotIn(group, self.member.groups.all())
-    """
+        # The missing alliance was created and the group assigned.
+        mock_create_alliance.assert_called_once()
+        group = obj.get_alliance_group(created_alliance)
+        self.assertIn(group, self.member.groups.all())
 
     def test_update_corp_group_membership(self):
         obj = AutogroupsConfig.objects.create(corp_groups=True)

allianceauth/eveonline/managers.py

@@ -63,24 +63,15 @@ class EveCharacterManager(Manager["EveCharacter"]):
         faction_model = _eve_faction_model()
 
         corporation_id = character.corporation_id
-        try:
-            corporation_obj = corporation_model.objects.get(corporation_id=corporation_id)
-        except corporation_model.DoesNotExist:
-            corporation_obj = corporation_model.objects.create_corporation(corporation_id=corporation_id)
+        corporation_obj = corporation_model.objects.get_or_create_esi(corporation_id=corporation_id)
 
         if character.alliance_id:
-            try:
-                alliance_obj = alliance_model.objects.get(alliance_id=character.alliance_id)
-            except alliance_model.DoesNotExist:
-                alliance_obj = alliance_model.objects.create_alliance(alliance_id=character.alliance_id)
+            alliance_obj = alliance_model.objects.get_or_create_esi(alliance_id=character.alliance_id)
         else:
             alliance_obj = None
 
         if character.faction_id:
-            try:
-                faction_obj = faction_model.objects.get(faction_id=character.faction_id)
-            except faction_model.DoesNotExist:
-                faction_obj = faction_model.objects.create_faction(faction_id=character.faction_id)
+            faction_obj = faction_model.objects.get_or_create_esi(faction_id=character.faction_id)
         else:
             faction_obj = None
 
@@ -106,6 +97,16 @@ class EveCharacterManager(Manager["EveCharacter"]):
             last_updated_other=datetime.strptime(character_response.headers.get("Last-Modified"), "%a, %d %b %Y %H:%M:%S GMT").replace(tzinfo=timezone.utc)
         )
 
+    def get_or_create_esi(self, character_id: "CharacterID") -> "EveCharacter":
+        try:
+            return self.get(character_id=character_id)
+        except self.model.DoesNotExist:
+            try:
+                with transaction.atomic():
+                    return self.create_character(character_id=character_id)
+            except IntegrityError:
+                return self.get(character_id=character_id)
+
     def update_character(self, character_id) -> "EveCharacter":
         return self.get(character_id=character_id).update_character()
 
@@ -131,10 +132,7 @@ class EveAllianceManager(Manager["EveAllianceInfo"]):
         faction_model = _eve_faction_model()
 
         if alliance.faction_id:
-            try:
-                faction_obj = faction_model.objects.get(faction_id=alliance.faction_id)
-            except faction_model.DoesNotExist:
-                faction_obj = faction_model.objects.create_faction(faction_id=alliance.faction_id)
+            faction_obj = faction_model.objects.get_or_create_esi(faction_id=alliance.faction_id)
         else:
             faction_obj = None
 
@@ -184,18 +182,12 @@ class EveCorporationManager(Manager["EveCorporationInfo"]):
         faction_model = _eve_faction_model()
 
         if corporation.alliance_id:
-            try:
-                alliance_obj = alliance_model.objects.get(alliance_id=corporation.alliance_id)
-            except alliance_model.DoesNotExist:
-                alliance_obj = alliance_model.objects.create_alliance(alliance_id=corporation.alliance_id)
+            alliance_obj = alliance_model.objects.get_or_create_esi(alliance_id=corporation.alliance_id)
         else:
             alliance_obj = None
 
         if corporation.faction_id:
-            try:
-                faction_obj = faction_model.objects.get(faction_id=corporation.faction_id)
-            except faction_model.DoesNotExist:
-                faction_obj = faction_model.objects.create_faction(faction_id=corporation.faction_id)
+            faction_obj = faction_model.objects.get_or_create_esi(faction_id=corporation.faction_id)
         else:
             faction_obj = None
 

allianceauth/eveonline/models.py

@@ -239,8 +239,7 @@ class EveAllianceInfo(models.Model):
             return self
 
         for corp_id in corp_ids:
-            if not EveCorporationInfo.objects.filter(corporation_id=corp_id).exists():
-                EveCorporationInfo.objects.create_corporation(corporation_id=corp_id)
+            EveCorporationInfo.objects.get_or_create_esi(corporation_id=corp_id)
         EveCorporationInfo.objects.filter(
             corporation_id__in=corp_ids
         ).update(
@@ -266,10 +265,7 @@ class EveAllianceInfo(models.Model):
             return self
 
         if alliance.faction_id:
-            try:
-                self.faction = EveFactionInfo.objects.get(faction_id=alliance.faction_id)
-            except EveFactionInfo.DoesNotExist:
-                self.faction = EveFactionInfo.objects.create_faction(faction_id=alliance.faction_id)
+            self.faction = EveFactionInfo.objects.get_or_create_esi(faction_id=alliance.faction_id)
         else:
             self.faction = None
 
@@ -402,18 +398,12 @@ class EveCorporationInfo(models.Model):
             return self
 
         if corporation.alliance_id:
-            try:
-                self.alliance = EveAllianceInfo.objects.get(alliance_id=corporation.alliance_id)
-            except EveAllianceInfo.DoesNotExist:
-                self.alliance = EveAllianceInfo.objects.create_alliance(alliance_id=corporation.alliance_id)
+            self.alliance = EveAllianceInfo.objects.get_or_create_esi(alliance_id=corporation.alliance_id)
         else:
             self.alliance = None
 
         if corporation.faction_id:
-            try:
-                self.faction = EveFactionInfo.objects.get(faction_id=corporation.faction_id)
-            except EveFactionInfo.DoesNotExist:
-                self.faction = EveFactionInfo.objects.create_faction(faction_id=corporation.faction_id)
+            self.faction = EveFactionInfo.objects.get_or_create_esi(faction_id=corporation.faction_id)
         else:
             self.faction = None
 

allianceauth/eveonline/tests/test_managers.py

@@ -362,6 +362,41 @@ class EveCharacterManagerTestCase(TestCase):
         self.assertEqual(result.character_name, "character.name")
         self.assertIsNone(EveCharacter.objects.get_character_by_id(9999))
 
+    def test_get_or_create_esi_returns_existing_character(self) -> None:
+        character = EveCharacter.objects.create(
+            character_id=1234,
+            character_name="Existing Character",
+            corporation_id=2345,
+            corporation_name="Existing Corp",
+            corporation_ticker="EXC",
+        )
+
+        result = EveCharacter.objects.get_or_create_esi(1234)
+
+        self.assertEqual(result, character)
+
+    def test_get_or_create_esi_handles_integrity_error_for_character(self) -> None:
+        existing = EveCharacter.objects.create(
+            character_id=1234,
+            character_name="Existing Character",
+            corporation_id=2345,
+            corporation_name="Existing Corp",
+            corporation_ticker="EXC",
+        )
+
+        with mock.patch.object(
+            EveCharacter.objects,
+            "get",
+            side_effect=[EveCharacter.DoesNotExist, existing],
+        ), mock.patch.object(
+            EveCharacter.objects,
+            "create_character",
+            side_effect=IntegrityError,
+        ):
+            result = EveCharacter.objects.get_or_create_esi(1234)
+
+        self.assertEqual(result, existing)
+
 
 class EveAllianceManagerTestCase(TestCase):
     @mock.patch("allianceauth.eveonline.managers.open_api_provider.get_alliance")

allianceauth/eveonline/tests/test_models.py

@@ -3,6 +3,7 @@ from types import SimpleNamespace
 from unittest.mock import MagicMock, PropertyMock, patch
 
 from django.core.exceptions import ObjectDoesNotExist
+from django.db.utils import IntegrityError
 from django.test import TestCase
 
 from esi.exceptions import HTTPNotModified
@@ -363,6 +364,75 @@ class EveCharacterTestCase(TestCase):
         self.assertEqual(my_character.alliance_name, 'Created Alliance')
         self.assertEqual(my_character.faction_name, 'Created Faction')
 
+    @patch('allianceauth.eveonline.models.open_api_provider.get_affiliations')
+    def test_update_character_survives_alliance_creation_race(self, mock_get_affiliations) -> None:
+        """Regression test for the multi-worker race condition.
+
+        Two update tasks can run for the same character concurrently. Both see the
+        alliance as missing, both call ``create_alliance()``, and the loser's INSERT
+        raises ``IntegrityError`` on the unique ``alliance_id`` constraint
+        (``1062, "Duplicate entry ... for key
+        'eveonline_eveallianceinfo_alliance_id_fbfbea3c_uniq'"``).
+        ``update_character()`` must recover by returning the alliance the winning
+        worker committed instead of propagating the error.
+        """
+        my_character = EveCharacter.objects.create(
+            character_id=1001,
+            character_name='Race Victim',
+            corporation_id=2001,
+            corporation_name='Dummy Corp 1',
+            corporation_ticker='DC1',
+            alliance_id=None,
+        )
+        # Corporation already exists so only the alliance path exercises the race.
+        EveCorporationInfo.objects.create(
+            corporation_id=2002,
+            corporation_name='Dummy Corp 2',
+            corporation_ticker='DC2',
+            member_count=1,
+            ceo_id=3,
+        )
+        # The alliance the *other* worker wins the race to create.
+        winning_alliance = EveAllianceInfo.objects.create(
+            alliance_id=3001,
+            alliance_name='Winner Alliance',
+            alliance_ticker='WIN',
+            executor_corp_id=2002,
+        )
+        mock_get_affiliations.return_value = (
+            [
+                SimpleNamespace(
+                    character_id=1001,
+                    corporation_id=2002,
+                    alliance_id=3001,
+                    faction_id=None,
+                )
+            ],
+            response_stub(),
+        )
+
+        # Simulate the race: the first get() misses, create_alliance() then loses
+        # the unique-constraint race (IntegrityError), and the follow-up get()
+        # finds the row the winning worker committed.
+        with patch.object(
+            EveAllianceInfo.objects,
+            'get',
+            side_effect=[EveAllianceInfo.DoesNotExist, winning_alliance],
+        ), patch.object(
+            EveAllianceInfo.objects,
+            'create_alliance',
+            side_effect=IntegrityError(
+                "(1062, \"Duplicate entry '3001' for key "
+                "'eveonline_eveallianceinfo_alliance_id_fbfbea3c_uniq'\")"
+            ),
+        ):
+            result = my_character.update_character()
+
+        result.refresh_from_db()
+        self.assertEqual(result.alliance_id, 3001)
+        self.assertEqual(result.alliance_name, 'Winner Alliance')
+        self.assertEqual(result.alliance_ticker, 'WIN')
+
     def test_image_url(self) -> None:
         self.assertEqual(
             EveCharacter.generic_portrait_url(42),