fix: Various bugs causing rust test failures on trunk (#3941)

* Ensure objects are cleaned up on a failed spawn

* Ensure messages are deferred until after the client is fully connected and synchronized

* Fix SceneLoadingAndNotificationsTests

* Cleanup unneeded changes

* Fix up the logging on AuthorityLocalSpawn

* Move tests back to trunk

* fix style issues

* standards fix

* Small fixes

* Fix null reference exception

* dotnet-fix

* dotnet-fix

* Non-authority gets synchronize event now

* Update object destroying on resynchronize

* Update CHANGELOG

* Fix NetworkObjectId check

* Use internal RemoveParent instead of public one with extra checks

Author: EmandM

.yamato/_triggers.yml

@@ -75,20 +75,20 @@ pr_code_changes_checks:
     # Run API validation to early-detect all new APIs that would force us to release new minor version of the package. Note that for this to work the package version in package.json must correspond to "actual package state" which means that it should be higher than last released version
     - .yamato/vetting-test.yml#vetting_test
 
-    # Run package EditMode and Playmode package tests on 6000.5 and an older supported editor (6000.0)
-    - .yamato/package-tests.yml#package_test_-_ngo_6000.5_mac
+    # Run package EditMode and Playmode package tests on trunk and an older supported editor (6000.0)
+    - .yamato/package-tests.yml#package_test_-_ngo_trunk_mac
     - .yamato/package-tests.yml#package_test_-_ngo_6000.0_win
 
-    # Run testproject EditMode and Playmode project tests on 6000.5 and an older supported editor (6000.0)
-    - .yamato/project-tests.yml#test_testproject_win_6000.5
+    # Run testproject EditMode and Playmode project tests on trunk and an older supported editor (6000.0)
+    - .yamato/project-tests.yml#test_testproject_win_trunk
     - .yamato/project-tests.yml#test_testproject_mac_6000.0
 
     # Run standalone test. We run it only on Ubuntu since it's the fastest machine, and it was noted that for example distribution on macOS is taking 40m since we switched to Apple Silicon
     # Coverage on other standalone machines is present in Nightly job so it's enough to not run all of them for PRs
     # desktop_standalone_test and cmb_service_standalone_test are both reusing desktop_standalone_build dependency so we run those in the same configuration on PRs to reduce waiting time.
-    # Note that our daily tests will anyway run both test configurations in "minimal supported" and "6000.5" configurations
-    - .yamato/desktop-standalone-tests.yml#desktop_standalone_test_testproject_ubuntu_il2cpp_6000.5
-    - .yamato/cmb-service-standalone-tests.yml#cmb_service_standalone_test_testproject_ubuntu_il2cpp_6000.5
+    # Note that our daily tests will anyway run both test configurations in "minimal supported" and "trunk" configurations
+    - .yamato/desktop-standalone-tests.yml#desktop_standalone_test_testproject_ubuntu_il2cpp_trunk
+    - .yamato/cmb-service-standalone-tests.yml#cmb_service_standalone_test_testproject_ubuntu_il2cpp_trunk
   triggers:
     expression: |-
       (pull_request.comment eq "ngo" OR

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -13,6 +13,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Changed
 
+- Hardened error handling and recovery during `NetworkObject` spawn. (#3941)
 - Replaced Debug usage by NetcodeLog on `NetworkSpawnManager` and `NetworkObject`. (#3933)
 - Improved performance of `NetworkBehaviour`. (#3915)
 - Improved performance of `NetworkTransform`. (#3907)

com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs

@@ -972,27 +972,39 @@ internal void HandleConnectionApproval(ulong ownerClientId, bool createPlayerObj
             }
 
             // Server-side spawning (only if there is a prefab hash or player prefab provided)
-            if (!NetworkManager.DistributedAuthorityMode && createPlayerObject && (playerPrefabHash.HasValue || NetworkManager.NetworkConfig.PlayerPrefab != null))
+            var idHashToSpawn = playerPrefabHash ?? NetworkManager.NetworkConfig.PlayerPrefab?.GetComponent<NetworkObject>()?.GlobalObjectIdHash;
+            if (!NetworkManager.DistributedAuthorityMode && createPlayerObject && idHashToSpawn.HasValue)
             {
-                var playerObject = playerPrefabHash.HasValue ? NetworkManager.SpawnManager.GetNetworkObjectToSpawn(playerPrefabHash.Value, ownerClientId, playerPosition, playerRotation)
-                : NetworkManager.SpawnManager.GetNetworkObjectToSpawn(NetworkManager.NetworkConfig.PlayerPrefab.GetComponent<NetworkObject>().GlobalObjectIdHash, ownerClientId, playerPosition, playerRotation);
+                var playerObject = NetworkManager.SpawnManager.GetNetworkObjectToSpawn(idHashToSpawn.Value, ownerClientId, playerPosition, playerRotation);
 
                 if (playerObject == null)
                 {
-                    Debug.LogError($"[{nameof(NetworkObject)}] Player prefab is null! Cannot spawn player object!");
+                    if (NetworkManager.LogLevel <= LogLevel.Error)
+                    {
+                        NetworkLog.LogError($"[{nameof(NetworkObject)}] Player prefab is null! Cannot spawn player object!");
+                    }
                 }
                 else
                 {
                     // Spawn the player NetworkObject locally
-                    NetworkManager.SpawnManager.AuthorityLocalSpawn(
+                    if (NetworkManager.SpawnManager.AuthorityLocalSpawn(
                         playerObject,
                         NetworkManager.SpawnManager.GetNetworkObjectId(),
                         sceneObject: false,
                         playerObject: true,
                         ownerClientId,
-                        destroyWithScene: false);
+                        destroyWithScene: false))
+                    {
+                        client.AssignPlayerObject(ref playerObject);
+                    }
+                    else
+                    {
+                        if (NetworkManager.LogLevel <= LogLevel.Developer)
+                        {
+                            NetworkLog.LogError($"[{nameof(NetworkObject)}] Player prefab failed to spawn!");
+                        }
+                    }
 
-                    client.AssignPlayerObject(ref playerObject);
                 }
             }
 
@@ -1122,8 +1134,25 @@ internal void CreateAndSpawnPlayer(ulong ownerId)
                 }
                 return;
             }
-            var globalObjectIdHash = playerPrefab.GetComponent<NetworkObject>().GlobalObjectIdHash;
-            var networkObject = NetworkManager.SpawnManager.GetNetworkObjectToSpawn(globalObjectIdHash, ownerId, playerPrefab.transform.position, playerPrefab.transform.rotation);
+            var prefabObject = playerPrefab.GetComponent<NetworkObject>();
+            if (prefabObject == null)
+            {
+                if (NetworkManager.LogLevel <= LogLevel.Normal)
+                {
+                    NetworkLog.LogError("Failed to fetch valid player prefab. Ensure PlayerPrefab that is set in NetcodeConfig contains a NetworkObject component.");
+                }
+                return;
+            }
+            var networkObject = NetworkManager.SpawnManager.GetNetworkObjectToSpawn(prefabObject.GlobalObjectIdHash, ownerId, playerPrefab.transform.position, playerPrefab.transform.rotation);
+            if (networkObject == null)
+            {
+                if (NetworkManager.LogLevel <= LogLevel.Normal)
+                {
+                    NetworkLog.LogError("Failed to spawn player prefab!");
+                }
+                return;
+            }
+
             networkObject.IsSceneObject = false;
             networkObject.NetworkManagerOwner = NetworkManager;
             networkObject.SpawnAsPlayerObject(ownerId, networkObject.DestroyWithScene);

com.unity.netcode.gameobjects/Runtime/Core/FindObjects.cs

@@ -21,17 +21,19 @@ internal static class FindObjects
         /// <typeparam name="T"></typeparam>
         /// <param name="includeInactive">When true, inactive objects will be included.</param>
         /// <param name="orderByIdentifier">When true, the array returned will be sorted by identifier.</param>
-        /// <returns>Resulst as an <see cref="Array"/> of type T</returns>
+        /// <returns>Results as an <see cref="Array"/> of type T</returns>
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
         public static T[] ByType<T>(bool includeInactive = false, bool orderByIdentifier = false) where T : Object
         {
             var inactive = includeInactive ? UnityEngine.FindObjectsInactive.Include : UnityEngine.FindObjectsInactive.Exclude;
 #if NGO_FINDOBJECTS_NOSORTING
             var results = Object.FindObjectsByType<T>(inactive);
+#if !NGO_FINDOBJECTS_UNORDERED_IDS
             if (orderByIdentifier)
             {
                 Array.Sort(results, (a, b) => a.GetEntityId().CompareTo(b.GetEntityId()));
             }
+#endif
 #else
             var results = Object.FindObjectsByType<T>(inactive, orderByIdentifier ? UnityEngine.FindObjectsSortMode.InstanceID : UnityEngine.FindObjectsSortMode.None);
 #endif

com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs

@@ -1809,7 +1809,15 @@ internal void SpawnInternal(bool destroyWithScene, ulong ownerClientId, bool pla
                 }
             }
 
-            NetworkManagerOwner.SpawnManager.AuthorityLocalSpawn(this, NetworkManagerOwner.SpawnManager.GetNetworkObjectId(), IsSceneObject.HasValue && IsSceneObject.Value, playerObject, ownerClientId, destroyWithScene);
+            if (!NetworkManagerOwner.SpawnManager.AuthorityLocalSpawn(this, NetworkManagerOwner.SpawnManager.GetNetworkObjectId(), IsSceneObject.HasValue && IsSceneObject.Value, playerObject, ownerClientId, destroyWithScene))
+            {
+                if (NetworkManagerOwner.LogLevel <= LogLevel.Normal)
+                {
+                    NetworkLog.LogWarning($"[{name}] Failed to finish spawning!");
+                }
+                ResetOnDespawn();
+                return;
+            }
 
             if ((NetworkManagerOwner.DistributedAuthorityMode && NetworkManagerOwner.DAHost) || (!NetworkManagerOwner.DistributedAuthorityMode && NetworkManagerOwner.IsServer))
             {
@@ -1873,7 +1881,7 @@ public static NetworkObject InstantiateAndSpawn(GameObject networkPrefab, Networ
         /// <summary>
         /// This invokes <see cref="NetworkSpawnManager.InstantiateAndSpawn(NetworkObject, ulong, bool, bool, bool, Vector3, Quaternion)"/>.
         /// </summary>
-        /// <param name="networkManager">The local instance of the NetworkManager connected to an session in progress.</param>
+        /// <param name="networkManager">The local instance of the NetworkManager connected to a session in progress.</param>
         /// <param name="ownerClientId">The owner of the <see cref="NetworkObject"/> instance (defaults to server).</param>
         /// <param name="destroyWithScene">Whether the <see cref="NetworkObject"/> instance will be destroyed when the scene it is located within is unloaded (default is false).</param>
         /// <param name="isPlayerObject">Whether the <see cref="NetworkObject"/> instance is a player object or not (default is false).</param>
@@ -2185,6 +2193,11 @@ internal void SetNetworkParenting(ulong? latestParent, bool worldPositionStays)
             m_CachedWorldPositionStays = worldPositionStays;
         }
 
+        internal void ClearNetworkParenting()
+        {
+            m_LatestParent = null;
+        }
+
         /// <summary>
         /// Set the parent of the NetworkObject transform.
         /// </summary>
@@ -3219,7 +3232,7 @@ internal void SynchronizeNetworkBehaviours<T>(ref BufferSerializer<T> serializer
                 {
                     reader.ReadValueSafe(out ushort networkBehaviourId);
                     var networkBehaviour = GetNetworkBehaviourAtOrderIndex(networkBehaviourId);
-                    networkBehaviour.Synchronize(ref serializer, targetClientId);
+                    networkBehaviour?.Synchronize(ref serializer, targetClientId);
                 }
             }
         }
@@ -3311,86 +3324,46 @@ internal static NetworkObject Deserialize(in SerializedObject serializedObject,
         {
             var endOfSynchronizationData = reader.Position + serializedObject.SynchronizationDataSize;
 
-            byte[] instantiationData = null;
-            if (serializedObject.HasInstantiationData)
-            {
-                reader.ReadValueSafe(out instantiationData);
-            }
-
-            // Attempt to create a local NetworkObject
-            var networkObject = networkManager.SpawnManager.CreateLocalNetworkObject(serializedObject, instantiationData);
-            if (networkObject == null)
+            if (serializedObject.NetworkObjectId == default)
             {
-                // Log the error that the NetworkObject failed to construct
-                if (networkManager.LogLevel <= LogLevel.Normal)
-                {
-                    NetworkLog.LogError($"Failed to spawn {nameof(NetworkObject)} for Hash {serializedObject.Hash}.");
-                }
-
-                try
-                {
-                    // If we failed to load this NetworkObject, then skip past the Network Variable and (if any) synchronization data
-                    reader.Seek(endOfSynchronizationData);
-                }
-                catch (Exception ex)
+                if (networkManager.LogLevel <= LogLevel.Error)
                 {
-                    Debug.LogException(ex);
+                    NetworkLog.LogErrorServer($"[{nameof(GlobalObjectIdHash)}={serializedObject.Hash}] Received spawn request with invalid {nameof(NetworkObjectId)} {serializedObject.NetworkObjectId}. This should not happen!");
                 }
 
-                // We have nothing left to do here.
+                reader.Seek(endOfSynchronizationData);
                 return null;
             }
-            networkObject.NetworkManagerOwner = networkManager;
 
-            // This will get set again when the NetworkObject is spawned locally, but we set it here ahead of spawning
-            // in order to be able to determine which NetworkVariables the client will be allowed to read.
-            networkObject.OwnerClientId = serializedObject.OwnerClientId;
+            // Do the SpawnManager parts of the object spawn
+            var succeeded = networkManager.SpawnManager.NonAuthorityLocalSpawn(in serializedObject, out var networkObject, reader, serializedObject.DestroyWithScene);
 
-            // Special Case: Invoke NetworkBehaviour.OnPreSpawn methods here before SynchronizeNetworkBehaviours
-            networkObject.InvokeBehaviourNetworkPreSpawn();
+            // Process any deferred messages once the object is 100% finished spawning
+            // Ensure this is done whether the spawn succeeds or fails
+            networkManager.DeferredMessageManager.ProcessTriggers(IDeferredNetworkMessageManager.TriggerType.OnSpawn, serializedObject.NetworkObjectId);
 
-            // Process the remaining synchronization data from the buffer
-            try
+            // Ensure that the buffer is completely reset
+            if (reader.Position != endOfSynchronizationData)
             {
-                // Synchronize NetworkBehaviours
-                var bufferSerializer = new BufferSerializer<BufferSerializerReader>(new BufferSerializerReader(reader));
-                networkObject.SynchronizeNetworkBehaviours(ref bufferSerializer, networkManager.LocalClientId);
-
-                // Ensure that the buffer is completely reset
-                if (reader.Position != endOfSynchronizationData)
+                if (networkManager.LogLevel <= LogLevel.Normal)
                 {
-                    Debug.LogWarning($"[Size mismatch] Expected: {endOfSynchronizationData} Currently At: {reader.Position}!");
-                    reader.Seek(endOfSynchronizationData);
+                    NetworkLog.LogWarning($"[{networkObject.name}][Deserialize][{nameof(NetworkBehaviour)}Synchronization][Size mismatch] Expected: {endOfSynchronizationData} Currently At: {reader.Position}!");
                 }
-            }
-            catch
-            {
                 reader.Seek(endOfSynchronizationData);
             }
 
-            // If we are an in-scene placed NetworkObject and we originally had a parent but when synchronized we are
-            // being told we do not have a parent, then we want to clear the latest parent so it is not automatically
-            // "re-parented" to the original parent. This can happen if not unloading the scene and the parenting of
-            // the in-scene placed Networkobject changes several times over different sessions.
-            if (serializedObject.IsSceneObject && !serializedObject.HasParent && networkObject.m_LatestParent.HasValue)
-            {
-                networkObject.m_LatestParent = null;
-            }
-
-            // Spawn the NetworkObject
-            if (networkObject.IsSpawned)
+            // If the SpawnManager spawn doesn't succeed, be sure to clean up
+            if (!succeeded)
             {
-                if (NetworkManager.Singleton.LogLevel <= LogLevel.Error)
+                // If the networkObject was created but the spawn failed, the created object needs to be destroyed
+                if (networkObject != null)
                 {
-                    NetworkLog.LogErrorServer($"[{networkObject.name}] Object-{networkObject.NetworkObjectId} is already spawned!");
+                    Destroy(networkObject.gameObject);
                 }
+
                 return null;
             }
 
-            // Invoke the non-authority local spawn method
-            // (It also invokes post spawn and handles processing derferred messages)
-            networkManager.SpawnManager.NonAuthorityLocalSpawn(networkObject, serializedObject, serializedObject.DestroyWithScene);
-
             if (serializedObject.SyncObservers)
             {
                 foreach (var observer in serializedObject.Observers)

com.unity.netcode.gameobjects/Runtime/Messaging/Messages/DestroyObjectMessage.cs

@@ -112,7 +112,7 @@ public bool Deserialize(FastBufferReader reader, ref NetworkContext context, int
             }
 
             // Client-Server mode we always defer where in distributed authority mode we only defer if it is not a targeted destroy
-            if (!networkManager.DistributedAuthorityMode || (networkManager.DistributedAuthorityMode && !IsTargetedDestroy))
+            if (!networkManager.DistributedAuthorityMode || !networkManager.IsConnectedClient || (networkManager.DistributedAuthorityMode && !IsTargetedDestroy))
             {
                 networkManager.DeferredMessageManager.DeferMessage(IDeferredNetworkMessageManager.TriggerType.OnSpawn, NetworkObjectId, reader, ref context, k_Name);
             }