Refactor UserServiceManager methods for clarity and improved logging

actor

@@ -1 +1 @@
-Subproject commit 23901493bbd0731ab22bf70aaed99f0b3ca7642d
+Subproject commit 386e227a8a24372f78cfe68525fd9a3f99442bc7

server/src/uds/core/managers/userservice.py

@@ -306,7 +306,7 @@ class UserServiceManager(metaclass=singleton.Singleton):
 
     def forced_move_assigned_to_cache_l1(self, user_service: UserService) -> None:
         """
-        Clones the record of a user serviceself.
+        Clones the record of a user service.
         For this, the original userservice will ve moved to cache, and a new one will be created
         to mark it as "REMOVED"
 
@@ -320,6 +320,7 @@ class UserServiceManager(metaclass=singleton.Singleton):
         user_service_copy.in_use = False
         user_service_copy.state = State.REMOVED
         user_service_copy.os_state = State.USABLE
+        log.log(user_service_copy, types.log.LogLevel.INFO, 'Service moved to cache')
 
         # Save the new element.
         user_service_copy.save()
@@ -560,6 +561,7 @@ class UserServiceManager(metaclass=singleton.Singleton):
         if servicepool.uses_cache:
             cache: typing.Optional[UserService] = None
             # Now try to locate 1 from cache already "ready" (must be usable and at level 1)
+            # First, a cached service that is "fully" ready
             with transaction.atomic():
                 caches = typing.cast(
                     list[UserService],
@@ -574,6 +576,8 @@ class UserServiceManager(metaclass=singleton.Singleton):
                 if caches:
                     cache = caches[0]
                     # Ensure element is reserved correctly on DB
+                    # Avoid concurrency problems due to cache being assigned to user
+                    # in the middle of this process
                     if (
                         servicepool.cached_users_services()
                         .select_for_update()
@@ -587,6 +591,7 @@ class UserServiceManager(metaclass=singleton.Singleton):
 
             # Out of previous atomic
             if not cache:
+                # Now try to locate 1 from cache already "ready" (must be usable and at level 1)
                 with transaction.atomic():
                     caches = typing.cast(
                         list[UserService],
@@ -606,9 +611,7 @@ class UserServiceManager(metaclass=singleton.Singleton):
                             cache = None
                     else:
                         cache = None
-
+            else:
-            # Out of atomic transaction
-            if cache:
                 # Early assign
                 cache.assign_to(user)
 
@@ -885,7 +888,7 @@ class UserServiceManager(metaclass=singleton.Singleton):
         userservice: typing.Optional[UserService] = None
 
         if kind in 'A':  # This is an assigned service
-            logger.debug('Getting assugned user service %s', uuid_userservice_pool)
+            logger.debug('Getting assigned user service %s', uuid_userservice_pool)
             try:
                 userservice = UserService.objects.get(uuid=uuid_userservice_pool, user=user)
                 userservice.service_pool.validate_user(user)

server/tests/core/managers/test_userservice.py

@@ -47,7 +47,7 @@ logger = logging.getLogger(__name__)
 
 class TestUserserviceManager(UDSTransactionTestCase):
     manager: UserServiceManager = UserServiceManager.manager()  # For convenience debugging
-    
+
     def test_forced_mode_assigned_to_l1(self) -> None:
         # Create an user service, we need
         userservice = services_fixtures.create_db_assigned_userservices()[0]
@@ -55,15 +55,15 @@ class TestUserserviceManager(UDSTransactionTestCase):
         orig_uuid = userservice.uuid
         orig_src_ip = userservice.src_ip
         orig_src_hostname = userservice.src_hostname
-        
+
         self.assertEqual(models.UserService.objects.all().count(), 1)
         # And uuser service is assigned to an user
         self.assertIsNotNone(userservice.user)
         # And cache level is None
         self.assertEqual(userservice.cache_level, core_types.services.CacheLevel.NONE)
-        
+
         self.manager.forced_move_assigned_to_cache_l1(userservice)
-        
+
         # Now, should have 2 user services, one in cache and one in db
         self.assertEqual(models.UserService.objects.all().count(), 2)
         # Reload userservice, that should be now in cache
@@ -78,8 +78,7 @@ class TestUserserviceManager(UDSTransactionTestCase):
         # Source ip and hostname should be empty
         self.assertEqual(userservice.src_ip, '')
         self.assertEqual(userservice.src_hostname, '')
-        
+
-        
         # Look for the created one (that is the assigned, deleted)
         assigned = models.UserService.objects.exclude(uuid=orig_uuid).get()
         self.assertEqual(assigned.cache_level, core_types.services.CacheLevel.NONE)
@@ -87,12 +86,47 @@ class TestUserserviceManager(UDSTransactionTestCase):
         self.assertIsNotNone(assigned.user)
         # Should be removed
         self.assertEqual(assigned.state, core_types.states.State.REMOVED)
-                         
+
         # unique_id should be same as the original one
         self.assertEqual(userservice.unique_id, assigned.unique_id)
         # src_ip and src_hostname should be the original ones
         self.assertEqual(assigned.src_ip, orig_src_ip)
         self.assertEqual(assigned.src_hostname, orig_src_hostname)
-        
+
     def test_release_from_logout(self) -> None:
-        pass
+        pass
+
+    def test_get_user_service_with_initial_no_cache(self) -> None:
+        userservice = services_fixtures.create_db_assigned_userservices()[0]
+        user = userservice.user
+        assert user is not None
+
+        # Fix userservice to set it as cache l1
+        userservice.cache_level = core_types.services.CacheLevel.L1
+        userservice.user = None
+        userservice.save()
+
+        service_pool = userservice.service_pool
+
+        service_pool.initial_srvs = 1
+        service_pool.cache_l1_srvs = 0
+        service_pool.cache_l2_srvs = 0
+        service_pool.max_srvs = 3
+        service_pool.save()
+
+        assigned_info = self.manager.get_user_service_info(
+            user,
+            core_types.os.DetectedOsInfo(
+                core_types.os.KnownOS.LINUX, core_types.os.KnownBrowser.CHROME, '1.0.0'
+            ),
+            '1.2.3.4',
+            f'P{service_pool.uuid}',
+            service_pool.transports.all()[0].uuid,
+        )
+
+        self.assertEqual(assigned_info.userservice.uuid, userservice.uuid)
+        self.assertEqual(assigned_info.userservice.cache_level, core_types.services.CacheLevel.NONE)
+        self.assertEqual(assigned_info.userservice.user, user)
+        self.assertEqual(assigned_info.userservice.src_ip, '1.2.3.4')
+        self.assertEqual(assigned_info.userservice.src_hostname, '1.2.3.4')
+        self.assertEqual(assigned_info.transport.uuid, service_pool.transports.all()[0].uuid)

server/tests/fixtures/services.py

@@ -188,7 +188,7 @@ def create_db_transport(transport_instance: 'Transport|None' = None, **kwargs: t
 def create_db_userservice(
     service_pool: models.ServicePool,
     publication: models.ServicePoolPublication,
-    user: models.User,
+    user: 'models.User|None',
 ) -> models.UserService:
     user_service: 'models.UserService' = service_pool.userServices.create(
         friendly_name='user-service-{}'.format(glob['user_service_id']),