fix: DontDestroyWithOwner v2.4.0 - v2.4.2 client-server regression (#3522)

This resolves the
[Forum-1658887](https://discussions.unity.com/t/network-objects-dont-despawn-on-disconnect-even-with-dont-destroy-with-owner-unchecked/1658887)
discovered client-server specific issue in the v2.4.2 release where
spawned objects with the default `NetworkObject.DontDestroyWithOwner`
setting (disabled/false) would not be despawned and destroyed when the
owning client disconnected.


## Changelog

- Fixed: Issue where spawned objects with
`NetworkObject.DontDestroyWithOwner` set to `false` would not be
destroyed when using a client-server network topology.

## Testing and Documentation

- Includes updates to the `NetworkObjectDontDestroyWithOwnerTests`
integration test.
- No documentation changes or additions were necessary.

<!-- Uncomment and mark items off with a * if this PR deprecates any
API:
### Deprecated API
- [ ] An `[Obsolete]` attribute was added along with a `(RemovedAfter
yyyy-mm-dd)` entry.
- [ ] An [api updater] was added.
- [ ] Deprecation of the API is explained in the CHANGELOG.
- [ ] The users can understand why this API was removed and what they
should use instead.
-->

## Backport
This is specific to NGO v2.4.2 release.

<!-- If this is a backport:
 - Add the following to the PR title: "\[Backport\] ..." .
 - Link to the original PR.
If this needs a backport - state this here
If a backport is not needed please provide the reason why.
If the "Backports" section is not present it will lead to a CI test
failure.
-->

Author: NoelStephensUnity

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -6,6 +6,13 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 
 Additional documentation and release notes are available at [Multiplayer Documentation](https://docs-multiplayer.unity3d.com).
 
+## [Unreleased]
+
+### Fixed
+
+- Fixed issue where spawned objects with `NetworkObject.DontDestroyWithOwner` is set to `false` would not be destroyed when using a client-server network topology. (#3522)
+
+
 ## [2.4.2] - 2025-06-13
 
 ### Fixed

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

@@ -1178,7 +1178,7 @@ internal void OnClientDisconnectFromServer(ulong clientId)
                     {
                         // If destroying with owner, then always despawn and destroy (or defer destroying to prefab handler)
                         // Handle an object with no observers other than the current disconnecting client as destroying with owner
-                        if (!ownedObject.DontDestroyWithOwner && (ownedObject.Observers.Count == 0 || (ownedObject.Observers.Count == 1 && ownedObject.Observers.Contains(clientId))))
+                        if (!ownedObject.DontDestroyWithOwner && (ownedObject.Observers.Count == 0 || (ownedObject.Observers.Count >= 1 && ownedObject.Observers.Contains(clientId))))
                         {
                             if (NetworkManager.PrefabHandler.ContainsHandler(ownedObject.GlobalObjectIdHash))
                             {

com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/NetworkObjectDontDestroyWithOwnerTests.cs

@@ -1,5 +1,7 @@
 using System.Collections;
+using System.Collections.Generic;
 using System.Linq;
+using System.Text;
 using NUnit.Framework;
 using Unity.Netcode.TestHelpers.Runtime;
 using UnityEngine;
@@ -13,72 +15,162 @@ namespace Unity.Netcode.RuntimeTests
     [TestFixture(HostOrServer.Server)]
     internal class NetworkObjectDontDestroyWithOwnerTests : NetcodeIntegrationTest
     {
-        private const int k_NumberObjectsToSpawn = 32;
-        protected override int NumberOfClients => 1;
+        private const int k_NumberObjectsToSpawn = 16;
+        protected override int NumberOfClients => 3;
 
-        // TODO: [CmbServiceTests] Adapt to run with the service
-        protected override bool UseCMBService()
-        {
-            return false;
-        }
-
-        protected GameObject m_PrefabToSpawn;
+        protected GameObject m_DestroyWithOwnerPrefab;
+        protected GameObject m_DontDestroyWithOwnerPrefab;
         protected GameObject m_PrefabNoObserversSpawn;
 
+        private ulong m_NonAuthorityClientId;
+
+        private List<ulong> m_DontDestroyObjectIds = new List<ulong>();
+        private List<ulong> m_DestroyObjectIds = new List<ulong>();
+
         public NetworkObjectDontDestroyWithOwnerTests(HostOrServer hostOrServer) : base(hostOrServer) { }
 
         protected override void OnServerAndClientsCreated()
         {
-            m_PrefabToSpawn = CreateNetworkObjectPrefab("ClientOwnedObject");
-            m_PrefabToSpawn.GetComponent<NetworkObject>().DontDestroyWithOwner = true;
+            m_DontDestroyWithOwnerPrefab = CreateNetworkObjectPrefab("DestroyWith");
+            m_DontDestroyWithOwnerPrefab.GetComponent<NetworkObject>().DontDestroyWithOwner = true;
+
+            m_DestroyWithOwnerPrefab = CreateNetworkObjectPrefab("DontDestroyWith");
 
             m_PrefabNoObserversSpawn = CreateNetworkObjectPrefab("NoObserversObject");
             var prefabNoObserversNetworkObject = m_PrefabNoObserversSpawn.GetComponent<NetworkObject>();
             prefabNoObserversNetworkObject.SpawnWithObservers = false;
             prefabNoObserversNetworkObject.DontDestroyWithOwner = true;
         }
 
-        [UnityTest]
-        public IEnumerator DontDestroyWithOwnerTest()
+        /// <summary>
+        /// Validates all instances of both prefab types have spawned on all clients.
+        /// </summary>
+        private bool HaveAllObjectInstancesSpawned(StringBuilder errorLog)
+        {
+            foreach (var networkManager in m_NetworkManagers)
+            {
+                var relativeSpawnedObjects = networkManager.SpawnManager.SpawnedObjects;
+                for (int i = 0; i < k_NumberObjectsToSpawn; i++)
+                {
+                    var dontDestroyObjectId = m_DontDestroyObjectIds[i];
+                    var destroyObjectId = m_DestroyObjectIds[i];
+                    if (!relativeSpawnedObjects.ContainsKey(dontDestroyObjectId))
+                    {
+                        errorLog.AppendLine($"[Client-{networkManager.LocalClientId}][DontDestroyWithOwner] Has not spawned {nameof(NetworkObject)}-{dontDestroyObjectId}!");
+                    }
+                    if (!relativeSpawnedObjects.ContainsKey(destroyObjectId))
+                    {
+                        errorLog.AppendLine($"[Client-{networkManager.LocalClientId}][DestroyWithOwner] Has not spawned {nameof(NetworkObject)}-{destroyObjectId}!");
+                    }
+                }
+            }
+            return errorLog.Length == 0;
+        }
+
+        /// <summary>
+        /// Helper method to spawn the two different sets of objects.
+        /// Those that will destroy with the owner and those that will not.
+        /// </summary>
+        /// <param name="dontDestroyWithOwner">type of prefab to use for spawning</param>
+        private void SpawnAllObjects(bool dontDestroyWithOwner)
         {
-            var client = m_ClientNetworkManagers[0];
-            var clientId = client.LocalClientId;
-            var networkObjects = SpawnObjects(m_PrefabToSpawn, m_ClientNetworkManagers[0], k_NumberObjectsToSpawn);
+            var networkObjectIds = new List<ulong>();
+            var nonAuthority = m_NetworkManagers.Where((c) => c.LocalClientId == m_NonAuthorityClientId).First();
+            var objectToSpawn = dontDestroyWithOwner ? m_DontDestroyWithOwnerPrefab : m_DestroyWithOwnerPrefab;
+            var spawnedObjects = SpawnObjects(objectToSpawn, nonAuthority, k_NumberObjectsToSpawn);
+            foreach (var spawnedObject in spawnedObjects)
+            {
+                networkObjectIds.Add(spawnedObject.GetComponent<NetworkObject>().NetworkObjectId);
+            }
 
-            // wait for object spawn on client to reach k_NumberObjectsToSpawn + 1 (k_NumberObjectsToSpawn and 1 for the player)
-            yield return WaitForConditionOrTimeOut(() => client.SpawnManager.GetClientOwnedObjects(clientId).Count() == k_NumberObjectsToSpawn + 1);
-            Assert.False(s_GlobalTimeoutHelper.TimedOut, $"Timed out waiting for client to have 33 NetworkObjects spawned! Only {client.SpawnManager.GetClientOwnedObjects(clientId).Count()} were assigned!");
+            if (dontDestroyWithOwner)
+            {
+                m_DontDestroyObjectIds.Clear();
+                m_DontDestroyObjectIds.AddRange(networkObjectIds);
+            }
+            else
+            {
+                m_DestroyObjectIds.Clear();
+                m_DestroyObjectIds.AddRange(networkObjectIds);
+            }
+        }
 
-            // Since clients spawn their objects locally in distributed authority mode, we have to rebuild the list of the client
-            // owned objects on the (DAHost) server-side because when the client disconnects it will destroy its local instances.
-            if (m_DistributedAuthority)
+        /// <summary>
+        /// Validates that the non-authority owner client disconnection
+        /// was registered on all clients.
+        /// </summary>
+        private bool NonAuthorityHasDisconnected(StringBuilder errorLog)
+        {
+            foreach (var networkManager in m_NetworkManagers)
             {
-                networkObjects.Clear();
-                var serversideClientOwnedObjects = m_ServerNetworkManager.SpawnManager.GetClientOwnedObjects(clientId);
+                if (!networkManager.IsListening)
+                {
+                    continue;
+                }
+
+                if (networkManager.ConnectedClientsIds.Contains(m_NonAuthorityClientId))
+                {
+                    errorLog.AppendLine($"[Client-{networkManager.LocalClientId}][ClientDisconnect] Still thinks Client-{m_NonAuthorityClientId} is connected!");
+                }
+            }
+            return errorLog.Length == 0;
+        }
 
-                foreach (var networkObject in serversideClientOwnedObjects)
+        /// <summary>
+        /// The primary validation for the <see cref="DontDestroyWithOwnerTest"/>.
+        /// This validates that:
+        /// - Spawned objects that are set to destroy with the owner gets destroyed/despawned when the owning client disconnects.
+        /// - Spawned objects that are set to not destroy with the owner are not destroyed/despawned when the owning client disconnects.
+        /// </summary>
+        private bool ValidateDontDestroyWithOwner(StringBuilder errorLog)
+        {
+            foreach (var networkManager in m_NetworkManagers)
+            {
+                var relativeSpawnedObjects = networkManager.SpawnManager.SpawnedObjects;
+                for (int i = 0; i < k_NumberObjectsToSpawn; i++)
                 {
-                    if (!networkObject.IsPlayerObject)
+                    var dontDestroyObjectId = m_DontDestroyObjectIds[i];
+                    var destroyObjectId = m_DestroyObjectIds[i];
+                    if (!relativeSpawnedObjects.ContainsKey(dontDestroyObjectId))
+                    {
+                        errorLog.AppendLine($"[Client-{networkManager.LocalClientId}][DontDestroyWithOwner][!Despawned!] {nameof(NetworkObject)}-{dontDestroyObjectId} should not despawn upon the owner disconnecting!");
+                    }
+                    if (relativeSpawnedObjects.ContainsKey(destroyObjectId))
                     {
-                        networkObjects.Add(networkObject.gameObject);
+                        errorLog.AppendLine($"[Client-{networkManager.LocalClientId}][DestroyWithOwner][!Not Despawned!] {nameof(NetworkObject)}-{destroyObjectId} should have despawned upon the owner disconnecting!");
                     }
                 }
             }
+            return errorLog.Length == 0;
+        }
 
-            // disconnect the client that owns all the clients
-            NetcodeIntegrationTestHelpers.StopOneClient(client);
+        /// <summary>
+        /// This validates that:
+        /// - Spawned objects that are set to destroy with the owner gets destroyed/despawned when the owning client disconnects.
+        /// - Spawned objects that are set to not destroy with the owner are not destroyed/despawned when the owning client disconnects.
+        /// </summary>
+        [UnityTest]
+        public IEnumerator DontDestroyWithOwnerTest()
+        {
+            var authority = GetAuthorityNetworkManager();
+            var nonAuthority = GetNonAuthorityNetworkManager();
+            m_NonAuthorityClientId = nonAuthority.LocalClientId;
+            SpawnAllObjects(true);
+            SpawnAllObjects(false);
 
-            var remainingClients = Mathf.Max(0, TotalClients - 1);
-            // wait for disconnect
-            yield return WaitForConditionOrTimeOut(() => m_ServerNetworkManager.ConnectedClients.Count == remainingClients);
-            Assert.False(s_GlobalTimeoutHelper.TimedOut, "Timed out waiting for client to disconnect!");
+            // This should never fail.
+            Assert.IsTrue(m_DontDestroyObjectIds.Count == m_DestroyObjectIds.Count, $"Mismatch in spawn count! ({m_DontDestroyObjectIds.Count}) vs ({m_DestroyObjectIds.Count})");
 
-            for (int i = 0; i < networkObjects.Count; i++)
-            {
-                var networkObject = networkObjects[i].GetComponent<NetworkObject>();
-                // ensure ownership was transferred back
-                Assert.That(networkObject.OwnerClientId == m_ServerNetworkManager.LocalClientId);
-            }
+            yield return WaitForConditionOrTimeOut(HaveAllObjectInstancesSpawned);
+            AssertOnTimeout($"Timed out waiting for all clients to spawn objects!");
+
+            yield return StopOneClient(nonAuthority);
+
+            yield return WaitForConditionOrTimeOut(NonAuthorityHasDisconnected);
+            AssertOnTimeout($"Timed out waiting for all clients to register that Client-{m_NonAuthorityClientId} has disconnected!");
+
+            yield return WaitForConditionOrTimeOut(ValidateDontDestroyWithOwner);
+            AssertOnTimeout($"Timed out while validating the DontDestroyWithOwnerTest results!");
         }
 
         [UnityTest]
@@ -87,22 +179,55 @@ public IEnumerator NetworkShowThenClientDisconnects()
             var authorityManager = GetAuthorityNetworkManager();
             var networkObject = SpawnObject(m_PrefabNoObserversSpawn, authorityManager).GetComponent<NetworkObject>();
             var longWait = new WaitForSeconds(0.25f);
+            // Wait long enough to assure that no client receives the spawn notification
             yield return longWait;
+
+            foreach (var networkManager in m_NetworkManagers)
+            {
+                // Skip the authority as it will have an instance
+                if (networkManager == authorityManager)
+                {
+                    continue;
+                }
+                Assert.False(networkManager.SpawnManager.SpawnedObjects.ContainsKey(networkObject.NetworkObjectId), $"[Client-{networkManager.LocalClientId}]" +
+                    $" Spawned an instance of {networkObject.name} when it was spawned with no observers!");
+            }
+
+            // Get a non-authority client, show the spawned object to it, and then make that client the owner.
             var nonAuthorityManager = GetNonAuthorityNetworkManager();
-            Assert.False(nonAuthorityManager.SpawnManager.SpawnedObjects.ContainsKey(networkObject.NetworkObjectId), $"[Client-{nonAuthorityManager.LocalClientId}] " +
-                $"Already has an instance of {networkObject.name} when it should not!");
             networkObject.NetworkShow(nonAuthorityManager.LocalClientId);
+            // This validates that the change in ownership is not sent when making a NetworkObject visible and changing the ownership
+            // within the same frame/callstack.
             networkObject.ChangeOwnership(nonAuthorityManager.LocalClientId);
 
+            // Verifies the object was spawned on the non-authority client and that the non-authority client is the owner.
             yield return WaitForConditionOrTimeOut(() => nonAuthorityManager.SpawnManager.SpawnedObjects.ContainsKey(networkObject.NetworkObjectId)
             && nonAuthorityManager.SpawnManager.SpawnedObjects[networkObject.NetworkObjectId].OwnerClientId == nonAuthorityManager.LocalClientId);
             AssertOnTimeout($"[Client-{nonAuthorityManager.LocalClientId}] Failed to spawn {networkObject.name} when it was shown!");
 
+            foreach (var networkManager in m_NetworkManagers)
+            {
+                // Skip the authority and the non-authority client as they should now both have instances
+                if (networkManager == authorityManager || networkManager == nonAuthorityManager)
+                {
+                    continue;
+                }
+
+                // No other client should have an instance
+                Assert.False(networkManager.SpawnManager.SpawnedObjects.ContainsKey(networkObject.NetworkObjectId), $"[Client-{networkManager.LocalClientId}]" +
+                    $" Spawned an instance of {networkObject.name} when it was shown to Client-{nonAuthorityManager.LocalClientId}!");
+            }
+
+            // Wait a few frames
             yield return s_DefaultWaitForTick;
 
+            // Shutdown the non-authority client to assure this does not cause the spawned object to despawn
             nonAuthorityManager.Shutdown();
 
+            // Wait long enough to assure all messages generated from the client shutting down have been processed
             yield return longWait;
+
+            // Validate the object is still spawned
             Assert.True(networkObject.IsSpawned, $"The spawned test prefab was despawned on the authority side when it shouldn't have been!");
         }
     }