Host cannot accept Steam player after reconnect (e.g. Fix & Restart)

Source/Client/Networking/NetworkingSteam.cs

@@ -2,6 +2,7 @@
 using System.Collections.Generic;
 using System.Diagnostics;
 using System.Linq;
+using System.Threading.Tasks;
 using Multiplayer.Common;
 using Multiplayer.Common.Networking.Packet;
 using Steamworks;
@@ -17,6 +18,19 @@ public abstract class SteamBaseConn(CSteamID remoteId, ushort recvChannel, ushor
         public readonly ushort recvChannel = recvChannel; // currently only for client
         public readonly ushort sendChannel = sendChannel; // currently only for server
 
+        // Time given to Steam to flush a queued goodbye packet before the P2P session is freed (#843).
+        private static readonly TimeSpan SteamGoodbyeFlushDelay = TimeSpan.FromSeconds(3);
+
+        public override object? RemoteIdentity => remoteId;
+
+        // Steam carries every connection with a peer over one P2P session keyed by their id, so a
+        // replacement from the same id arrived on this very session and closing would take it down too.
+        public override void CloseReplacedBy(ConnectionBase replacement, MpDisconnectReason reason)
+        {
+            if (replacement is SteamBaseConn conn && conn.remoteId == remoteId) return;
+            base.CloseReplacedBy(replacement, reason);
+        }
+
         protected override void SendRaw(byte[] raw, bool reliable = true)
         {
             byte[] full = new byte[1 + raw.Length];
@@ -42,12 +56,48 @@ public void SendRawSteam(byte[] raw, bool reliable)
 
         public abstract void OnError(EP2PSessionError error);
 
+        // A goodbye is only ever non-null server-side, and CloseP2PSessionWithUser discards queued unsent
+        // packets. Closing immediately would drop the just-queued goodbye (e.g. a wrong-password or kick
+        // reason) before Steam flushes it, so defer the close to let the reliable packet reach the client.
         protected override void OnClose(ServerDisconnectPacket? goodbye)
         {
-            if (goodbye.HasValue) Send(goodbye.Value);
-            // TODO this should probably include SteamNetworking.CloseP2PSessionWithUser to free up any leftover
-            //   resources in the Steam API. The API docs are not clear whether the connection is closed instantly, or
-            //   are the queued packets sent.
+            if (!goodbye.HasValue)
+            {
+                CloseSteamSession();
+                return;
+            }
+
+            Send(goodbye.Value);
+
+            var server = serverPlayer.Server;
+            var id = remoteId;
+
+            Task.Delay(SteamGoodbyeFlushDelay).ContinueWith(_ => server.Enqueue(() =>
+            {
+                // A fast reconnect (SteamP2PNetManager.Tick) reuses this same remoteId on a fresh
+                // connection during the delay window. Closing then would tear that new session down, so
+                // skip the close if another connection already replaced this one (#843).
+                if (server.playerManager.Players.Any(p => p.conn is SteamBaseConn c && c != this && c.remoteId == id))
+                {
+                    ServerLog.Log($"Skipping delayed Steam P2P session close with {id}; a reconnect replaced it");
+                    return;
+                }
+
+                CloseSteamSession();
+            }));
+        }
+
+        // Frees the underlying Steam P2P session. This is required so that a later reconnect from
+        // the same user produces a fresh P2PSessionRequest_t (and, on the host, a new accept prompt)
+        // instead of Steam silently reusing the still-open session, which left the peer stuck and the
+        // host without a prompt (#843).
+        //
+        // Called immediately when no goodbye is queued; server-initiated disconnects that queue a goodbye
+        // defer it (see OnClose) so SendP2PPacket can flush the reason before the session is torn down.
+        protected void CloseSteamSession()
+        {
+            ServerLog.Log($"Closing Steam P2P session with {remoteId}");
+            SteamNetworking.CloseP2PSessionWithUser(remoteId);
         }
 
         public override string ToString() => $"SteamP2P ({remoteId}:{username})";
@@ -108,6 +158,10 @@ public override void OnKeepAliveArrived(bool idMatched)
 
         private void OnDisconnect()
         {
+            // The P2P timeout/error path does not go through OnClose, so close the Steam session here
+            // too. Otherwise the host keeps a half-open session with the departed client and their
+            // reconnect reuses it without firing a new accept prompt (#843).
+            CloseSteamSession();
             serverPlayer.Server.playerManager.SetDisconnected(this, MpDisconnectReason.ClientLeft);
         }
     }
@@ -132,10 +186,17 @@ public void Tick()
                 var player = playerManager.Players
                     .FirstOrDefault(p => p.conn is SteamBaseConn conn && conn.remoteId == packet.remote);
 
-                if (packet.joinPacket && player == null)
+                if (packet.joinPacket)
                 {
                     ConnectionBase conn = new SteamServerConn(packet.remote, packet.channel);
 
+                    // A join packet from a remote we still consider connected means their previous
+                    // session died and they are reconnecting on a fresh one (e.g. a quick rejoin before
+                    // the old connection timed out). Without replacing the stale player the join would be
+                    // discarded, leaving them stuck on "waiting for host to accept" (#843).
+                    if (player != null)
+                        playerManager.ReplaceStale(player, conn);
+
                     var preConnect = playerManager.OnPreConnect(packet.remote);
                     if (preConnect != null)
                     {
@@ -155,14 +216,13 @@ public void Tick()
 
                     conn.Send(Packets.Server_SteamAccept);
                 }
-                else if (!packet.joinPacket && player != null)
+                else if (player != null)
                 {
                     player.HandleReceive(packet.data, packet.reliable);
                 }
                 else
                 {
-                    ServerLog.Error(
-                        $"Received a join packet: {packet.joinPacket} for player: {player} (player should only be null when joinPacket is true)");
+                    ServerLog.Error($"Received a data packet from {packet.remote}, who has no connection");
                 }
             }
         }

Source/Client/Networking/SteamIntegration.cs

@@ -34,23 +34,37 @@ public static void InitCallbacks()
             {
                 ServerLog.Log($"Received P2P session request from {req.m_steamIDRemote}");
                 var session = Multiplayer.session;
-                if (Multiplayer.LocalServer?.settings.steam == true && !session.pendingSteam.Contains(req.m_steamIDRemote))
+                if (Multiplayer.LocalServer?.settings.steam != true)
+                    return;
+
+                var remoteId = req.m_steamIDRemote;
+
+                if (Multiplayer.settings.autoAcceptSteam)
+                {
+                    SteamNetworking.AcceptP2PSessionWithUser(remoteId);
+                }
+                // pendingSteam doubles as the dedup set: an entry exists exactly while a prompt is
+                // unanswered (both accept and reject clear it), so this skips duplicate prompts
+                // without a stale entry ever blocking a reconnect permanently (#843).
+                else if (!session.pendingSteam.Contains(remoteId))
                 {
-                    if (Multiplayer.settings.autoAcceptSteam)
-                        SteamNetworking.AcceptP2PSessionWithUser(req.m_steamIDRemote);
-                    else
+                    session.pendingSteam.Add(remoteId);
+                    PendingPlayerWindow.EnqueueJoinRequest(remoteId, (joinReq, accepted) =>
                     {
-                        session.pendingSteam.Add(req.m_steamIDRemote);
-                        PendingPlayerWindow.EnqueueJoinRequest(req.m_steamIDRemote, (joinReq, accepted) =>
-                        {
-                            if(joinReq.steamId.HasValue && accepted) AcceptPlayerJoinRequest(joinReq.steamId.Value);
-                        });
-                    }
-                    session.knownUsers.Add(req.m_steamIDRemote);
-                    session.NotifyChat();
-
-                    SteamFriends.RequestUserInformation(req.m_steamIDRemote, true);
+                        if (!joinReq.steamId.HasValue) return;
+                        if (accepted)
+                            AcceptPlayerJoinRequest(joinReq.steamId.Value);
+                        else
+                            // Clean up so the player isn't blocked from prompting again on reconnect.
+                            session.pendingSteam.Remove(joinReq.steamId.Value);
+                    });
                 }
+
+                if (!session.knownUsers.Contains(remoteId))
+                    session.knownUsers.Add(remoteId);
+                session.NotifyChat();
+
+                SteamFriends.RequestUserInformation(remoteId, true);
             });
 
             friendRchpUpdate = Callback<FriendRichPresenceUpdate_t>.Create(update =>

Source/Common/Networking/ConnectionBase.cs

@@ -12,6 +12,8 @@ public abstract class ConnectionBase
 
         public virtual int Latency { get; set; }
 
+        public virtual object? RemoteIdentity => null;
+
         public ConnectionStateEnum State { get; private set; }
         public MpConnectionState? StateObj { get; private set; }
         // If lenient is set, reliable packets without handlers are ignored instead of throwing an exception.
@@ -269,6 +271,8 @@ public void Close(MpDisconnectReason reason, byte[]? data = null)
                 OnClose(null);
         }
 
+        public virtual void CloseReplacedBy(ConnectionBase replacement, MpDisconnectReason reason) => Close(reason);
+
         protected abstract void OnClose(ServerDisconnectPacket? goodbye);
 
         /// Invoked after a keep alive timer arrives. Only used by the server

Source/Common/Networking/LiteNetConnection.cs

@@ -7,6 +7,11 @@ public class LiteNetConnection(NetPeer peer) : ConnectionBase
     {
         public readonly NetPeer peer = peer;
 
+        // Only a hint at who the remote is: players behind one NAT share an address, so a housemate
+        // can match; and a player whose address changed (mobile, VPN, or v4 vs v6 across the two
+        // NetManagers) won't match their own earlier connection.
+        public override object? RemoteIdentity => peer.Address;
+
         protected override void SendRaw(byte[] raw, bool reliable)
         {
             if (peer.ConnectionState == ConnectionState.Connected)

Source/Common/Networking/State/ServerJoiningState.cs

@@ -87,10 +87,19 @@ private void HandleUsername(ClientUsernamePacket packet)
             return;
         }
 
-        if (Server.GetPlayer(username) != null)
+        var existing = Server.GetPlayer(username);
+        if (existing != null)
         {
-            Player.Disconnect(MpDisconnectReason.UsernameAlreadyOnline);
-            return;
+            // Coming from the same remote as the player already holding this username means it's them
+            // reconnecting before their old connection was reaped, so give them their name back. Anyone
+            // else claiming it is turned away as before.
+            if (existing.conn.RemoteIdentity?.Equals(connection.RemoteIdentity) == true)
+                Server.playerManager.ReplaceStale(existing, connection);
+            else
+            {
+                Player.Disconnect(MpDisconnectReason.UsernameAlreadyOnline);
+                return;
+            }
         }
 
         connection.username = username;

Source/Common/PlayerManager.cs

@@ -62,6 +62,16 @@ public ServerPlayer OnConnected(ConnectionBase conn)
             return conn.serverPlayer;
         }
 
+        // Drops a connection that a newly arrived one has superseded, so the arrival can take its place
+        // instead of being turned away (#843).
+        public void ReplaceStale(ServerPlayer stale, ConnectionBase replacement)
+        {
+            ServerLog.Log($"Replacing stale connection {stale.conn} with {replacement}");
+
+            stale.conn.CloseReplacedBy(replacement, MpDisconnectReason.ClientLeft);
+            SetDisconnected(stale.conn, MpDisconnectReason.ClientLeft);
+        }
+
         public void SetDisconnected(ConnectionBase conn, MpDisconnectReason reason)
         {
             if (conn.State == ConnectionStateEnum.Disconnected) return;

Source/Tests/Helper/RecordingConnection.cs

@@ -15,6 +15,10 @@ public RecordingConnection(string username)
 
     public override int Latency { get => 0; set { } }
 
+    public object? remoteIdentity;
+
+    public override object? RemoteIdentity => remoteIdentity;
+
     protected override void SendRaw(byte[] raw, bool reliable)
     {
         if (raw.Length == 0)

Source/Tests/Helper/TestUsernameOnlyState.cs

@@ -0,0 +1,27 @@
+using Multiplayer.Common;
+using Multiplayer.Common.Networking.Packet;
+
+namespace Tests;
+
+// Gets as far as claiming a username and then stays put, so tests have a connection for a later
+// client to collide with.
+public class TestUsernameOnlyState : AsyncConnectionState
+{
+    public TestUsernameOnlyState(ConnectionBase connection) : base(connection)
+    {
+    }
+
+    // Deliberately registers no packet handlers: handlers accumulate globally per connection state, so
+    // declaring one here would clash with the state a second client in the same test installs.
+    protected override async Task RunState()
+    {
+        connection.Send(ClientProtocolPacket.Current());
+        await TypedPacket<ServerProtocolOkPacket>();
+
+        connection.Send(new ClientUsernamePacket(connection.username!));
+        await TypedPacket<ServerInitDataRequestPacket>();
+
+        // Left unanswered, which parks the connection here still holding the username.
+        await TypedPacket<ServerJoinDataPacket>();
+    }
+}

Source/Tests/ServerTest.cs

@@ -1,4 +1,5 @@
 using System.Diagnostics;
+using System.Net;
 using LiteNetLib;
 using Multiplayer.Common;
 
@@ -125,6 +126,56 @@ public void StandaloneJoinWithExistingPlayer_DoesNotStartJoinPoint()
         Assert.That(server.worldData.CreatingJoinPoint, Is.False);
     }
 
+    [Test]
+    public void JoinFromSameAddress_ReplacesPlayerHoldingTheUsername()
+    {
+        var server = MakeServer(out var port);
+        var stalePlayer = AddPlayerHoldingTestUsername(server, IPAddress.Loopback);
+
+        ConnectClient(port, typeof(TestUsernameOnlyState));
+
+        WaitUntil(() => !server.playerManager.Players.Contains(stalePlayer),
+            "the connection at the same address was not replaced");
+        Assert.That(server.playerManager.GetPlayer("test1"), Is.Not.Null);
+    }
+
+    [Test]
+    public void JoinFromDifferentAddress_KeepsPlayerHoldingTheUsername()
+    {
+        var server = MakeServer(out var port);
+        var existingPlayer = AddPlayerHoldingTestUsername(server, IPAddress.Parse("10.0.0.1"));
+
+        ConnectClient(port, typeof(TestUsernameOnlyState));
+
+        // Nothing should displace them, so give the join time to go wrong before checking.
+        Thread.Sleep(500);
+
+        Assert.That(server.playerManager.Players.Contains(existingPlayer), Is.True);
+    }
+
+    // Stands in for a player whose client is gone but whose connection the server still holds.
+    private static ServerPlayer AddPlayerHoldingTestUsername(MultiplayerServer server, IPAddress address)
+    {
+        var conn = new RecordingConnection("test1") { remoteIdentity = address };
+        conn.ChangeState(ConnectionStateEnum.ServerPlaying);
+        var player = new ServerPlayer(100, conn);
+        conn.serverPlayer = player;
+        server.playerManager.Players.Add(player);
+        return player;
+    }
+
+    private static void WaitUntil(Func<bool> condition, string message)
+    {
+        var timeoutWatch = Stopwatch.StartNew();
+        while (!condition())
+        {
+            if (timeoutWatch.ElapsedMilliseconds > 2000)
+                Assert.Fail($"Timeout: {message}");
+
+            Thread.Sleep(50);
+        }
+    }
+
     private void ConnectClient(int port, Type joiningStateType)
     {
         var clientListener = new TestNetListener(joiningStateType);