fix: SceneEventProgress does not handle NetworkManager shut down during scene event (#2254)

* fix
This resolves the issue where SceneEventProgress was not checking if the NetworkManager was no longer listening or a shutdown was in progress.

* update
Added additional NetworkLog warning when a client times out during connection approval.

* test
Added a mock test to verify SceneEventProgress can handle the NetworkManager shutting down during a scene event.

Author: NoelStephensUnity

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

@@ -1678,13 +1678,18 @@ private IEnumerator ApprovalTimeout(ulong clientId)
             else
             {
                 float timeStarted = Time.realtimeSinceStartup;
-
-                //We yield every frame incase a pending client disconnects and someone else gets its connection id
+                //We yield every frame in case a pending client disconnects and someone else gets its connection id
                 while (IsListening && (Time.realtimeSinceStartup - timeStarted) < NetworkConfig.ClientConnectionBufferTimeout && !IsConnectedClient)
                 {
                     yield return null;
                 }
 
+                if (!IsConnectedClient && NetworkLog.CurrentLogLevel <= LogLevel.Normal)
+                {
+                    // TODO: Possibly add a callback for users to be notified of this condition here?
+                    NetworkLog.LogWarning($"[ApprovalTimeout] Client timed out! You might need to increase the {nameof(NetworkConfig.ClientConnectionBufferTimeout)} duration.  Approval Check Start: {timeStarted} | Approval Check Stopped: {Time.realtimeSinceStartup}");
+                }
+
                 if (!IsListening)
                 {
                     yield break;

com.unity.netcode.gameobjects/Runtime/SceneManagement/SceneEventProgress.cs

@@ -201,6 +201,13 @@ internal void ClientFinishedSceneEvent(ulong clientId)
         /// </remarks>
         private bool HasFinished()
         {
+            // If the network session is terminated/terminating then finish tracking
+            // this scene event
+            if (!IsNetworkSessionActive())
+            {
+                return true;
+            }
+
             // Clients skip over this
             foreach (var clientStatus in ClientsProcessingSceneEvent)
             {
@@ -222,11 +229,25 @@ internal void SetAsyncOperation(AsyncOperation asyncOperation)
             m_AsyncOperation = asyncOperation;
             m_AsyncOperation.completed += new Action<AsyncOperation>(asyncOp2 =>
             {
-                OnSceneEventCompleted?.Invoke(SceneEventId);
+                // Don't invoke the callback if the network session is disconnected
+                // during a SceneEventProgress
+                if (IsNetworkSessionActive())
+                {
+                    OnSceneEventCompleted?.Invoke(SceneEventId);
+                }
+
+                // Go ahead and try finishing even if the network session is terminated/terminating
+                // as we might need to stop the coroutine
                 TryFinishingSceneEventProgress();
             });
         }
 
+
+        internal bool IsNetworkSessionActive()
+        {
+            return m_NetworkManager != null && m_NetworkManager.IsListening && !m_NetworkManager.ShutdownInProgress;
+        }
+
         /// <summary>
         /// Will try to finish the current scene event in progress as long as
         /// all conditions are met.
@@ -235,10 +256,15 @@ internal void TryFinishingSceneEventProgress()
         {
             if (HasFinished() || HasTimedOut())
             {
-                OnComplete?.Invoke(this);
-                m_NetworkManager.SceneManager.SceneEventProgressTracking.Remove(Guid);
-                m_NetworkManager.OnClientDisconnectCallback -= OnClientDisconnectCallback;
-                if (m_NetworkManager.IsServer)
+                // Don't attempt to finalize this scene event if we are no longer listening or a shutdown is in progress
+                if (IsNetworkSessionActive())
+                {
+                    OnComplete?.Invoke(this);
+                    m_NetworkManager.SceneManager.SceneEventProgressTracking.Remove(Guid);
+                    m_NetworkManager.OnClientDisconnectCallback -= OnClientDisconnectCallback;
+                }
+
+                if (m_TimeOutCoroutine != null)
                 {
                     m_NetworkManager.StopCoroutine(m_TimeOutCoroutine);
                 }

testproject/Assets/Tests/Runtime/NetworkSceneManager/SceneEventProgressTests.cs

@@ -13,6 +13,8 @@ namespace TestProject.RuntimeTests
     public class SceneEventProgressTests : NetcodeIntegrationTest
     {
         private const string k_SceneUsedToGetAsyncOperation = "EmptyScene";
+        private const string k_SceneUsedToGetClientAsyncOperation = "UnitTestBaseScene";
+
         protected override int NumberOfClients => 4;
 
         private bool m_SceneEventProgressCompleted;
@@ -51,6 +53,7 @@ private void StartNewSceneEventProgress()
             m_CurrentSceneEventProgress.SetAsyncOperation(SceneManager.LoadSceneAsync(k_SceneUsedToGetAsyncOperation, LoadSceneMode.Additive));
         }
 
+
         private void MockServerLoadedSene(Scene scene, LoadSceneMode loadSceneMode)
         {
             if (scene.name == k_SceneUsedToGetAsyncOperation)
@@ -155,6 +158,31 @@ public IEnumerator ClientsDisconnectDuring()
             VerifyClientsThatCompleted();
         }
 
+        [UnityTest]
+        public IEnumerator ClientsShutdownDuring()
+        {
+            StartNewSceneEventProgress();
+
+            for (int i = 0; i < NumberOfClients; i++)
+            {
+                var currentClientNetworkManager = m_ClientNetworkManagers[i];
+                // Two clients will shutdown
+                var clientFinished = i % 2 == 0;
+                SetClientFinished(currentClientNetworkManager.LocalClientId, clientFinished);
+
+                if (!clientFinished)
+                {
+                    currentClientNetworkManager.Shutdown();
+                }
+                // wait anywhere from 100-500ms until processing next client
+                var randomWaitPeriod = Random.Range(0.1f, 0.5f);
+                yield return new WaitForSeconds(randomWaitPeriod);
+            }
+            yield return WaitForConditionOrTimeOut(() => m_SceneEventProgressCompleted);
+            AssertOnTimeout($"Timed out waiting for SceneEventProgress to time out!");
+            VerifyClientsThatCompleted();
+        }
+
         /// <summary>
         /// This verifies that SceneEventProgress will still complete
         /// even when clients late join.
@@ -163,16 +191,13 @@ public IEnumerator ClientsDisconnectDuring()
         public IEnumerator ClientsLateJoinDuring()
         {
             StartNewSceneEventProgress();
-
             for (int i = 0; i < NumberOfClients; i++)
             {
                 // Two clients will connect during a SceneEventProgress
                 var shouldNewClientJoin = i % 2 == 0;
                 var currentClientNetworkManager = m_ClientNetworkManagers[i];
-
                 // All connected clients will finish their SceneEventProgress
                 SetClientFinished(currentClientNetworkManager.LocalClientId, true);
-
                 if (shouldNewClientJoin)
                 {
                     yield return CreateAndStartNewClient();
@@ -181,11 +206,84 @@ public IEnumerator ClientsLateJoinDuring()
                 var randomWaitPeriod = Random.Range(0.1f, 0.5f);
                 yield return new WaitForSeconds(randomWaitPeriod);
             }
-
             yield return WaitForConditionOrTimeOut(() => m_SceneEventProgressCompleted);
             AssertOnTimeout($"Timed out waiting for SceneEventProgress to finish!");
+            VerifyClientsThatCompleted();
+        }
+
+        private List<ulong> m_ClientsThatTimedOutAndFinished = new List<ulong>();
+
+        private SceneEventProgress StartClientSceneEventProgress(NetworkManager networkManager)
+        {
+            var sceneEventProgress = new SceneEventProgress(networkManager, SceneEventProgressStatus.Started);
 
+            // Mock Scene Loading Event for mocking timed out client
+            m_CurrentSceneEventProgress.SceneEventId = (uint)networkManager.LocalClientId;
+            var asyncOperation = SceneManager.LoadSceneAsync(k_SceneUsedToGetClientAsyncOperation, LoadSceneMode.Additive);
+            asyncOperation.completed += new System.Action<AsyncOperation>(asyncOp2 =>
+            {
+                m_ClientsThatTimedOutAndFinished.Add(networkManager.LocalClientId);
+            });
+
+            m_CurrentSceneEventProgress.SetAsyncOperation(asyncOperation);
+            return sceneEventProgress;
+        }
+
+        private Dictionary<NetworkManager, SceneEventProgress> m_ClientsToFinishAfterDisconnecting = new Dictionary<NetworkManager, SceneEventProgress>();
+        private bool TimedOutClientsFinishedSceneEventProgress()
+        {
+            // Now, verify all "mock timed out" clients still finished their SceneEventProgress
+            foreach (var entry in m_ClientsToFinishAfterDisconnecting)
+            {
+                if (!m_ClientsThatTimedOutAndFinished.Contains(entry.Key.LocalClientId))
+                {
+                    return false;
+                }
+            }
+            return true;
+        }
+
+        /// <summary>
+        /// This mocks a client timing out during a SceneEventProgress
+        /// </summary>
+        [UnityTest]
+        public IEnumerator ClientsMockTimeOutDuring()
+        {
+            m_ClientsThatTimedOutAndFinished.Clear();
+            m_ClientsToFinishAfterDisconnecting.Clear();
+            StartNewSceneEventProgress();
+
+            for (int i = 0; i < NumberOfClients; i++)
+            {
+                // Two clients will mock timing out during a SceneEventProgress
+                var shouldClientFinish = i % 2 == 0;
+                var currentClientNetworkManager = m_ClientNetworkManagers[i];
+
+                // Set whether the client should or should not have finished
+                SetClientFinished(currentClientNetworkManager.LocalClientId, shouldClientFinish);
+                if (!shouldClientFinish)
+                {
+                    var sceneEventProgress = StartClientSceneEventProgress(currentClientNetworkManager);
+                    m_ClientsToFinishAfterDisconnecting.Add(currentClientNetworkManager, sceneEventProgress);
+                    currentClientNetworkManager.Shutdown();
+                }
+            }
+
+            yield return WaitForConditionOrTimeOut(() => m_SceneEventProgressCompleted);
+            AssertOnTimeout($"Timed out waiting for SceneEventProgress to finish!");
             VerifyClientsThatCompleted();
+
+            yield return WaitForConditionOrTimeOut(TimedOutClientsFinishedSceneEventProgress);
+            if (s_GlobalTimeoutHelper.TimedOut)
+            {
+                foreach (var entry in m_ClientsToFinishAfterDisconnecting)
+                {
+                    Assert.IsTrue(m_ClientsThatTimedOutAndFinished.Contains(entry.Key.LocalClientId), $"Client-{entry.Key.LocalClientId} did not complete its {nameof(SceneEventProgress)}!");
+                    // Now, as a final check we try to finish the "mock" timed out client's scene event progress
+                    entry.Value.TryFinishingSceneEventProgress();
+                }
+            }
+            m_ClientsToFinishAfterDisconnecting.Clear();
         }
     }
 }