fix: #3832 NetworkServer.Destroy now doesn't call ResetState for prefabs, fixes isServer flag always being false in OnDestroy (#3848)

* repro

* NetworkServer.Destroy(): refactor to split scene vs. prefab cases more obviously

* fix: #3832 NetworkServer.Destroy now doesn't call ResetState for prefabs, fixes isServer flag always being false in OnDestroy

* fix the runtime test

* Revert "repro"

This reverts commit 3a5a33e8e4.

---------

Co-authored-by: mischa <info@noobtuts.com>

Assets/Mirror/Core/NetworkServer.cs

@@ -1592,14 +1592,12 @@ static void SpawnObject(GameObject obj, NetworkConnection ownerConnection)
             RebuildObservers(identity, true);
         }
 
-        /// <summary>This takes an object that has been spawned and un-spawns it.</summary>
+        // internal Unspawn function which has the 'resetState' parameter.
-        // The object will be removed from clients that it was spawned on, or
+        // resetState calls .ResetState() on the object after unspawning.
-        // the custom spawn handler function on the client will be called for
+        // this is necessary for scene objects, but not for prefabs since we
-        // the object.
+        // don't want to reset their isServer flags etc.
-        // Unlike when calling NetworkServer.Destroy(), on the server the object
+        // fixes: https://github.com/MirrorNetworking/Mirror/issues/3832
-        // will NOT be destroyed. This allows the server to re-use the object,
+        static void UnSpawnInternal(GameObject obj, bool resetState)
-        // even spawn it again later.
-        public static void UnSpawn(GameObject obj)
         {
             // Debug.Log($"DestroyObject instance:{identity.netId}");
 
@@ -1673,9 +1671,21 @@ public static void UnSpawn(GameObject obj)
             identity.OnStopServer();
 
             // finally reset the state and deactivate it
+            if (resetState)
+            {
                 identity.ResetState();
                 identity.gameObject.SetActive(false);
             }
+        }
+
+        /// <summary>This takes an object that has been spawned and un-spawns it.</summary>
+        // The object will be removed from clients that it was spawned on, or
+        // the custom spawn handler function on the client will be called for
+        // the object.
+        // Unlike when calling NetworkServer.Destroy(), on the server the object
+        // will NOT be destroyed. This allows the server to re-use the object,
+        // even spawn it again later.
+        public static void UnSpawn(GameObject obj) => UnSpawnInternal(obj, resetState: true);
 
         // destroy /////////////////////////////////////////////////////////////
         /// <summary>Destroys this object and corresponding objects on all clients.</summary>
@@ -1698,17 +1708,31 @@ public static void Destroy(GameObject obj)
                 return;
             }
 
-            // first, we unspawn it on clients and server
+            // get the NetworkIdentity component first
-            UnSpawn(obj);
+            if (!GetNetworkIdentity(obj, out NetworkIdentity identity))
+            {
+                Debug.LogWarning($"NetworkServer.Destroy() called on {obj.name} which doesn't have a NetworkIdentity component.");
+                return;
+            }
 
-            // additionally, if it's a prefab then we destroy it completely.
+            // is this a scene object?
+            // then we simply unspawn & reset it so it can still be spawned again.
             // we never destroy scene objects on server or on client, since once
             // they are gone, they are gone forever and can't be instantiate again.
             // for example, server may Destroy() a scene object and once a match
             // restarts, the scene objects would be gone from the new match.
-            if (GetNetworkIdentity(obj, out NetworkIdentity identity) &&
+            if (identity.sceneId != 0)
-                identity.sceneId == 0)
             {
+                UnSpawnInternal(obj, resetState: true);
+            }
+            // is this a prefab?
+            // then we destroy it completely.
+            else
+            {
+                // unspawn without calling ResetState.
+                // otherwise isServer/isClient flags might be reset in OnDestroy.
+                // fixes: https://github.com/MirrorNetworking/Mirror/issues/3832
+                UnSpawnInternal(obj, resetState: false);
                 identity.destroyCalled = true;
 
                 // Destroy if application is running

Assets/Mirror/Tests/Editor/NetworkBehaviour/NetworkBehaviourTests.cs

@@ -855,6 +855,28 @@ public void OnStopLocalPlayer()
             Assert.That(comp.onStopLocalPlayerCalled, Is.EqualTo(1));
         }
 
+        // test to prevent: https://github.com/MirrorNetworking/Mirror/issues/3832
+        class NetworkBehaviourOnDestroy : NetworkBehaviour
+        {
+            void OnDestroy()
+            {
+                Debug.LogWarning("OnDestroy called with isServer=" + isServer);
+                // on server, IsServer should still be false in OnDestroy.
+                Assert.That(isServer, Is.EqualTo(NetworkServer.active));
+            }
+        }
+        [Test]
+        public void NetworkDestroy_OnDestroyFlags()
+        {
+            NetworkServer.Listen(1);
+            ConnectClientBlockingAuthenticatedAndReady(out _);
+
+            CreateNetworked(out GameObject _, out NetworkIdentity identity, out NetworkBehaviourOnDestroy comp);
+            NetworkServer.Destroy(identity.gameObject);
+
+            // the NetworkBehaviourOnDestroy component has the asset to check isServer..
+        }
+
         [Test]
         public void AuthorityIsFalseVariations()
         {