fix: address code review of the CLI review fixes (MTT-16534)

- NetworkManager keeps the role it last started as (internal LastStartedRole), so reconnect refuses a stopped host or server however it was started, and the CLI's own role dictionary, and its leak, is gone.
- The simulator forgets a blackout only once every live driver has been let through, so timeout false still lifts one left on after a script recompile in play mode.
- After a recompile in play mode, the simulator subscribes to new drivers again when it's on.
- The console warns again when the simulator settings change, instead of keeping the first values.
- config set address/port explains a hybrid session when the transport has already been swapped to UnifiedNetcodeTransport.
- A distributed authority manager isn't reported as hybrid, since that topology rejects hybrid prefabs.
- Start's doc comment is back on Start.

Author: jy-vivox

com.unity.netcode.gameobjects/Documentation~/unity-cli-commands.md

@@ -57,7 +57,7 @@ The total bytes sent and received are exact. Per message type, the bytes receive
 
 ### netcode.gameobjects.simulator
 
-The simulator applies packet delay, jitter and loss to UnityTransport through its simulator pipeline stage, which is available in the Editor and development builds. Settings last for the Editor session and also apply to sessions started later. Once the simulator, `timeout` and `lag_spike` are all off, the transport gets its own simulator settings back. While the simulator is on, the Console shows a warning in each play session it affects.
+The simulator applies packet delay, jitter and loss to UnityTransport through its simulator pipeline stage, which is available in the Editor and development builds. Settings last for the Editor session and also apply to sessions started later. Once the simulator, `timeout` and `lag_spike` are all off, the transport gets its own simulator settings back. While the simulator is on, the Console shows a warning in each play session it affects, and again when its settings change.
 
 | **Command** | **Description** |
 | :--- | :--- |

com.unity.netcode.gameobjects/Editor/CliQueries/NgoCliQueries.Config.cs

@@ -92,6 +92,11 @@ internal static ConfigSetResult SetConfig(string field, string value, string man
                     {
                         if (manager.NetworkConfig.NetworkTransport is not UnityTransport utp)
                         {
+                            // A hybrid session that has run replaced the UnityTransport with UnifiedNetcodeTransport.
+                            if (IsHybridSession(manager))
+                            {
+                                throw new ArgumentException(HybridSessionNote(manager, $"{name}, a UnityTransport setting, can't be set on it"));
+                            }
                             throw new ArgumentException($"{name} is a UnityTransport setting, and {manager.name} uses {manager.NetworkConfig.NetworkTransport?.GetType().Name ?? "no transport"}.");
                         }
                         RequireDirectConnection(utp, name);

com.unity.netcode.gameobjects/Editor/CliQueries/NgoCliQueries.Session.cs

@@ -23,9 +23,6 @@ internal enum SessionMode
         /// address and port first. Returns once the start call returns; use <see cref="WaitReadyAsync"/> to wait
         /// for the connection.
         /// </summary>
-        // The role each NetworkManager last started as through the CLI: once it has stopped, it no longer says.
-        private static readonly Dictionary<NetworkManager, SessionMode> s_StartedAs = new Dictionary<NetworkManager, SessionMode>();
-
         internal static SessionResult Start(SessionMode mode, string managerFilter, string address, int port)
         {
             RequireDefined(mode, "mode");
@@ -81,7 +78,6 @@ internal static SessionResult Start(SessionMode mode, string managerFilter, stri
                     throw new InvalidOperationException($"{manager.name} failed to start as {mode.ToString().ToLowerInvariant()}{errors.Describe()}");
                 }
             }
-            s_StartedAs[manager] = mode;
 
             result.Status = DescribeManager(manager);
             if (mode == SessionMode.Client)
@@ -152,8 +148,9 @@ internal static async Task<WaitReadyResult> ReconnectAsync(string managerFilter,
                 throw new InvalidOperationException($"{manager.name} is the {GetRole(manager)}. reconnect is for clients; restart a server with netcode.gameobjects.session stop, then start.");
             }
             // A stopped host or server no longer reports IsServer, and would otherwise start again as a client.
-            if (!manager.IsListening && s_StartedAs.TryGetValue(manager, out var startedAs) && startedAs != SessionMode.Client)
+            if (!manager.IsListening && manager.LastStartedRole.IsServer)
             {
+                var startedAs = manager.LastStartedRole.IsClient ? SessionMode.Host : SessionMode.Server;
                 throw new InvalidOperationException($"{manager.name} was stopped as the {startedAs.ToString().ToLowerInvariant()}. reconnect is for clients; start it again with netcode.gameobjects.session start --mode {startedAs}.");
             }
 
@@ -172,7 +169,6 @@ internal static async Task<WaitReadyResult> ReconnectAsync(string managerFilter,
                     throw new InvalidOperationException($"{manager.name} shut down but failed to start again{errors.Describe()}");
                 }
             }
-            s_StartedAs[manager] = SessionMode.Client;
 
             var remaining = timeoutSeconds - (float)(EditorApplication.timeSinceStartup - started);
             var ready = await WaitReadyAsync(manager, Math.Max(remaining, 0.1f), 0);

com.unity.netcode.gameobjects/Editor/CliQueries/NgoCliQueries.Simulator.cs

@@ -21,7 +21,7 @@ namespace Unity.Netcode.GameObjects.Editor.Cli
     /// No driver is touched, and no transport event subscribed to, until the CLI turns the simulator, a timeout or
     /// a lag spike on. Each driver's own simulator parameters (from the transport or a Network Simulator component)
     /// are saved before the first override and put back once all three are off. While the simulator is on, the
-    /// console warns once per play session, when it first changes a driver.
+    /// console warns when it first changes a driver in a play session, and again when its settings change.
     /// </remarks>
     [InitializeOnLoad]
     internal static class NgoCliSimulator
@@ -88,12 +88,17 @@ private struct Baseline
 
         // Keyed by the id UnityTransport passes to TransportInitialized (UnityTransport.TransportId).
         private static readonly Dictionary<int, Baseline> s_Baselines = new Dictionary<int, Baseline>();
-        private static bool s_WarnedThisPlaySession;
+        private static string s_LastWarning;
 
         static NgoCliSimulator()
         {
             EditorApplication.playModeStateChanged -= OnPlayModeStateChanged;
             EditorApplication.playModeStateChanged += OnPlayModeStateChanged;
+            // A script recompile in play mode resets UnityTransport's events without running SubscribeToDrivers again.
+            if (Enabled)
+            {
+                SubscribeWhilePlaying();
+            }
         }
 
         // UnityTransport clears its driver events when play mode starts, so subscribe once the runtime is up, and
@@ -133,7 +138,7 @@ private static void OnPlayModeStateChanged(PlayModeStateChange change)
             {
                 TimeoutActive = false;
                 s_LagSpikeEnd = 0;
-                s_WarnedThisPlaySession = false;
+                s_LastWarning = null;
                 // The drivers are disposed with play mode.
                 BlackoutApplied = false;
                 EditorApplication.update -= EndLagSpikeWhenDue;
@@ -170,14 +175,20 @@ internal static List<UnityTransport> LiveTransports()
         internal static List<string> ApplyToLiveTransports(List<UnityTransport> live = null, bool clearBlackout = false)
         {
             var applied = new List<string>();
+            // A driver left alone without clearBlackout may still drop every packet, e.g. after a script recompile.
+            var blackoutLifted = true;
             foreach (var utp in live ?? LiveTransports())
             {
                 if (Apply(utp.TransportId, utp.GetNetworkDriver(), clearBlackout))
                 {
                     applied.Add(NgoCliQueries.GetHierarchyPath(utp.transform));
                 }
+                else if (!clearBlackout)
+                {
+                    blackoutLifted = false;
+                }
             }
-            if (!Blackout)
+            if (!Blackout && blackoutLifted)
             {
                 // Every live driver is back to its own loss, or the simulator's.
                 BlackoutApplied = false;
@@ -217,7 +228,7 @@ private static bool Apply(int transportId, NetworkDriver driver, bool clearBlack
                 stage.PacketJitterMs = JitterMs;
                 stage.PacketDropPercentage = DropPercent;
                 stage.PacketDropInterval = 0;
-                WarnOncePerPlaySession();
+                WarnSimulatorOn();
             }
             driver.ModifySimulatorStageParameters(stage);
 
@@ -239,16 +250,17 @@ private static bool Apply(int transportId, NetworkDriver driver, bool clearBlack
 
         /// <summary>
         /// The simulator outlasts the play session that turned it on, so say so in the console, where someone who
-        /// didn't run the command will look for the cause of the lag.
+        /// didn't run the command will look for the cause of the lag. Warns again when the settings change.
         /// </summary>
-        private static void WarnOncePerPlaySession()
+        private static void WarnSimulatorOn()
         {
-            if (s_WarnedThisPlaySession)
+            var warning = $"[Netcode] The Unity CLI network simulator is on for this editor session: {DelayMs} ms delay, {JitterMs} ms jitter and {DropPercent}% packet loss on every UnityTransport. Turn it off with netcode.gameobjects.simulator set --enable false.";
+            if (warning == s_LastWarning)
             {
                 return;
             }
-            s_WarnedThisPlaySession = true;
-            Debug.LogWarning($"[Netcode] The Unity CLI network simulator is on for this editor session: {DelayMs} ms delay, {JitterMs} ms jitter and {DropPercent}% packet loss on every UnityTransport. Turn it off with netcode.gameobjects.simulator set --enable false.");
+            s_LastWarning = warning;
+            Debug.LogWarning(warning);
         }
 
         /// <summary>

com.unity.netcode.gameobjects/Editor/CliQueries/NgoCliQueries.cs

@@ -147,10 +147,14 @@ internal static bool IsHybridSession(NetworkManager manager)
             {
                 return false;
             }
-            // A running session has registered its prefabs; before it starts, look at the configured ones.
-            return manager.IsListening
-                ? manager.NetworkConfig.Prefabs != null && manager.NetworkConfig.Prefabs.HasGhostPrefabs
-                : ConfiguredHybridPrefabs(manager.NetworkConfig).Count > 0;
+            // A running session has registered its prefabs; before it starts, look at the configured ones. Distributed
+            // authority rejects hybrid prefabs, so such a session never runs on a Netcode for Entities world.
+            if (manager.IsListening)
+            {
+                return manager.NetworkConfig.Prefabs != null && manager.NetworkConfig.Prefabs.HasGhostPrefabs;
+            }
+            return manager.NetworkConfig.NetworkTopology != NetworkTopologyTypes.DistributedAuthority
+                && ConfiguredHybridPrefabs(manager.NetworkConfig).Count > 0;
 #else
             return false;
 #endif

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

@@ -661,6 +661,12 @@ public ulong LocalClientId
         /// </summary>
         public bool IsHost => ConnectionManager.LocalClient.IsHost;
 
+        /// <summary>
+        /// The role this NetworkManager last started as. Unlike <see cref="IsServer"/> and <see cref="IsClient"/>, it
+        /// stays set after a shutdown, so editor tooling can tell a stopped host or server from a stopped client.
+        /// </summary>
+        internal (bool IsServer, bool IsClient) LastStartedRole { get; private set; }
+
         /// <summary>
         /// When disconnected from the server, the server may send a reason. If a reason was sent, this property will
         /// tell client code what the reason was. It should be queried after the OnClientDisconnectCallback is called
@@ -1476,6 +1482,7 @@ public bool StartServer()
             {
                 return false;
             }
+            LastStartedRole = (true, false);
             ConnectionManager.LocalClient.ClientId = ServerClientId;
 
             try
@@ -1558,6 +1565,7 @@ public bool StartClient()
             {
                 return false;
             }
+            LastStartedRole = (false, true);
 
             try
             {
@@ -1632,6 +1640,7 @@ public bool StartHost()
             {
                 return false;
             }
+            LastStartedRole = (true, true);
 
             try
             {

com.unity.netcode.gameobjects/Tests/Editor/Cli/NgoCliQueriesTests.cs

@@ -3,6 +3,7 @@
 using System.Collections.Generic;
 using System.Net;
 using System.Net.Sockets;
+using System.Reflection;
 using System.Text.RegularExpressions;
 using System.Threading.Tasks;
 using NUnit.Framework;
@@ -568,6 +569,15 @@ public void HybridPrefabs_TransportCommandsSayTheyDontApply()
             var simulatorNotes = NgoCliQueries.SetSimulator(true, null, 50, -1, -1).Notes;
             StringAssert.Contains("hybrid session", simulatorNotes[simulatorNotes.Count - 1]);
             StringAssert.Contains("hybrid session", Only(NgoCliQueries.GetSimulator().Notes));
+
+            manager.NetworkConfig.NetworkTopology = NetworkTopologyTypes.DistributedAuthority;
+            Assert.IsFalse(NgoCliQueries.IsHybridSession(manager), "Distributed authority rejects hybrid prefabs.");
+            manager.NetworkConfig.NetworkTopology = NetworkTopologyTypes.ClientServer;
+
+            // Once a hybrid session has run, the NetworkManager's transport is UnifiedNetcodeTransport.
+            manager.NetworkConfig.NetworkTransport = manager.gameObject.AddComponent<Unity.Netcode.Unified.UnifiedNetcodeTransport>();
+            var swapped = Assert.Throws<ArgumentException>(() => NgoCliQueries.SetConfig("address", "10.0.0.2", null, false));
+            StringAssert.Contains("hybrid session", swapped.Message);
         }
 #endif
 
@@ -701,6 +711,8 @@ public IEnumerator RunningHost_SupportsTheRuntimeQueries()
             CollectionAssert.IsNotEmpty(simulator.AppliedLiveTo, "The running transport gets the settings now.");
             driver.CurrentSettings.TryGet<SimulatorUtility.Parameters>(out var applied);
             Assert.AreEqual(50, applied.PacketDelayMs);
+            LogAssert.Expect(LogType.Warning, new Regex("network simulator is on for this editor session: 80 ms delay"));
+            NgoCliQueries.SetSimulator(true, null, 80, -1, -1);
 
             var timeout = NgoCliQueries.SetTimeout(true);
             Assert.IsTrue(timeout.Current.TimeoutActive);
@@ -715,6 +727,15 @@ public IEnumerator RunningHost_SupportsTheRuntimeQueries()
             driver.CurrentSettings.TryGet<SimulatorUtility.Parameters>(out var restored);
             Assert.AreEqual(original.PacketDelayMs, restored.PacketDelayMs, "Turning the simulator off restores the transport's own settings.");
 
+            // A script recompile in play mode forgets the timeout and the saved settings, but the drivers keep dropping every packet.
+            NgoCliQueries.SetTimeout(true);
+            typeof(NgoCliSimulator).GetProperty("TimeoutActive", BindingFlags.Static | BindingFlags.NonPublic).SetValue(null, false);
+            ((IDictionary)typeof(NgoCliSimulator).GetField("s_Baselines", BindingFlags.Static | BindingFlags.NonPublic).GetValue(null)).Clear();
+            NgoCliQueries.SetSimulator(false, null, -1, -1, -1);
+            NgoCliQueries.SetTimeout(false);
+            driver.CurrentSettings.TryGet<NetworkSimulatorParameter>(out var lifted);
+            Assert.AreEqual(0f, lifted.ReceivePacketLossPercent, "timeout false still lifts a blackout the CLI left on.");
+
             // Losing every packet on purpose, e.g. from a Network Simulator component, isn't mistaken for a CLI timeout.
             driver.CurrentSettings.TryGet<NetworkSimulatorParameter>(out var userLoss);
             userLoss.ReceivePacketLossPercent = 100f;
@@ -747,8 +768,14 @@ public IEnumerator RunningHost_SupportsTheRuntimeQueries()
             Assert.AreEqual(0, NgoCliQueries.ListObjects(null, "CliSpawnable", -1, 10).Total);
 
             NgoCliQueries.Stop(null, false);
-            yield return null;
-            yield return null;
+            yield return WaitForCondition(() => !manager.IsListening && !manager.ShutdownInProgress);
+
+            // However it was started, a stopped server isn't started again as a client.
+            Assert.IsTrue(manager.StartServer());
+            manager.Shutdown();
+            yield return WaitForCondition(() => !manager.IsListening && !manager.ShutdownInProgress);
+            var stoppedServer = Assert.ThrowsAsync<InvalidOperationException>(() => NgoCliQueries.ReconnectAsync(null, 5));
+            StringAssert.Contains("stopped as the server", stoppedServer.Message);
         }
 
         /// <summary>