diff --git a/src/AcDream.Core.Net/NetClient.cs b/src/AcDream.Core.Net/NetClient.cs index c45a525c..d3ec4a93 100644 --- a/src/AcDream.Core.Net/NetClient.cs +++ b/src/AcDream.Core.Net/NetClient.cs @@ -21,11 +21,28 @@ public sealed class NetClient : IDisposable private readonly UdpClient _udp; private readonly IPEndPoint _remote; + /// Last value applied to the + /// underlying socket, so repeated calls with the + /// same timeout (every 250ms heartbeat tick) skip the setsockopt syscall. + private int _lastAppliedReceiveTimeoutMs = int.MinValue; + public NetClient(IPEndPoint remote) { _remote = remote; // Bind to an OS-assigned local port; server will reply to it. _udp = new UdpClient(new IPEndPoint(IPAddress.Any, 0)); + + if (OperatingSystem.IsWindows()) + { + // SIO_UDP_CONNRESET: Windows' UDP stack otherwise surfaces an + // ICMP "port unreachable" reply to an EARLIER Send (e.g. a stale + // server session that already tore down its socket) as a + // WSAECONNRESET SocketException on this socket's NEXT unrelated + // Receive call. An ICMP error from one send must not poison + // receiving for the rest of the session — disable that reporting. + const int SioUdpConnReset = -1744830452; // 0x9800000C + _udp.Client.IOControl((IOControlCode)SioUdpConnReset, new byte[] { 0 }, null); + } } /// The local endpoint the OS assigned us. @@ -62,7 +79,12 @@ public sealed class NetClient : IDisposable /// public byte[]? Receive(TimeSpan timeout, out IPEndPoint? from) { - _udp.Client.ReceiveTimeout = (int)timeout.TotalMilliseconds; + int timeoutMs = (int)timeout.TotalMilliseconds; + if (timeoutMs != _lastAppliedReceiveTimeoutMs) + { + _udp.Client.ReceiveTimeout = timeoutMs; + _lastAppliedReceiveTimeoutMs = timeoutMs; + } try { IPEndPoint any = new(IPAddress.Any, 0); diff --git a/src/AcDream.Core.Net/WorldSession.cs b/src/AcDream.Core.Net/WorldSession.cs index cbc2ca6e..c4599165 100644 --- a/src/AcDream.Core.Net/WorldSession.cs +++ b/src/AcDream.Core.Net/WorldSession.cs @@ -973,6 +973,21 @@ public sealed class WorldSession : IDisposable /// tries again. On shutdown, cancels the token /// and joins the thread. /// + /// + /// + /// 2026-07-24 audit fix: a per-iteration + /// (anything other than the expected , + /// which already swallows) used to be + /// caught by an OUTER try/catch that wrapped the entire while loop, + /// so a single transient error — classically Windows' WSAECONNRESET, + /// delivered on this socket's next Receive after an earlier Send hit + /// an ICMP port-unreachable — exited the loop for good. The + /// finally then completed 's writer, + /// permanently killing inbound processing for the rest of the session + /// with no log line. Non-timeout socket errors are recoverable, + /// per-datagram events, not session-fatal, so they're now caught + /// per-iteration, logged, and the loop keeps polling. + /// /// private void NetReceiveLoop() { @@ -980,13 +995,26 @@ public sealed class WorldSession : IDisposable { while (!_netCancel.Token.IsCancellationRequested) { - var bytes = _net.Receive(TimeSpan.FromMilliseconds(250), out _); + byte[]? bytes; + try + { + bytes = _net.Receive(TimeSpan.FromMilliseconds(250), out _); + } + catch (System.Net.Sockets.SocketException ex) + { + // Transient, recoverable socket error (e.g. WSAECONNRESET + // from a stale peer's ICMP port-unreachable) — log and + // keep receiving. Does NOT tear down _inboundQueue. + Console.Error.WriteLine( + $"[net] receive error (continuing): {ex.SocketErrorCode} {ex.Message}"); + continue; + } + if (bytes is not null) _inboundQueue.Writer.TryWrite(bytes); } } catch (OperationCanceledException) { /* graceful shutdown */ } - catch (System.Net.Sockets.SocketException) { /* socket closed during shutdown */ } catch (ObjectDisposedException) { /* NetClient disposed before thread noticed */ } finally { diff --git a/tests/AcDream.Core.Net.Tests/NetClientTests.cs b/tests/AcDream.Core.Net.Tests/NetClientTests.cs index b818f867..23110ac4 100644 --- a/tests/AcDream.Core.Net.Tests/NetClientTests.cs +++ b/tests/AcDream.Core.Net.Tests/NetClientTests.cs @@ -1,10 +1,70 @@ using System.Net; +using System.Net.Sockets; +using System.Reflection; using AcDream.Core.Net; namespace AcDream.Core.Net.Tests; public class NetClientTests { + [Fact] + public void Construction_AppliesWindowsConnResetMitigationWithoutThrowing() + { + // 2026-07-24 audit fix: SIO_UDP_CONNRESET must be disabled on + // Windows so a delayed ICMP port-unreachable from an earlier Send + // can't surface as a WSAECONNRESET SocketException on a later, + // unrelated Receive call. This is a smoke test — if the IOControl + // call regresses (wrong control code / wrong overload), the + // constructor itself throws here instead of only manifesting much + // later as a silently dead receive loop under real network churn. + using var client = new NetClient(new IPEndPoint(IPAddress.Loopback, 0)); + Assert.NotNull(client.LocalEndPoint); + } + + [Fact] + public void Receive_RepeatedSameTimeout_CachesLastAppliedValue() + { + // The setsockopt-equivalent (Socket.ReceiveTimeout assignment) used + // to run on every single Receive call (~4x/sec at the 250ms + // heartbeat cadence). It's now hoisted behind a last-applied-value + // cache. This test can't observe the syscall directly, but it does + // prove the cache field converges on the requested value and the + // underlying socket option stays in sync — a regression that + // stopped updating the cache (or stopped applying to the socket) + // would fail this. + using var client = new NetClient(new IPEndPoint(IPAddress.Loopback, 1)); + + client.Receive(TimeSpan.FromMilliseconds(50), out _); + client.Receive(TimeSpan.FromMilliseconds(50), out _); + + FieldInfo cacheField = typeof(NetClient).GetField( + "_lastAppliedReceiveTimeoutMs", BindingFlags.NonPublic | BindingFlags.Instance)!; + Assert.Equal(50, (int)cacheField.GetValue(client)!); + + FieldInfo udpField = typeof(NetClient).GetField( + "_udp", BindingFlags.NonPublic | BindingFlags.Instance)!; + var udp = (UdpClient)udpField.GetValue(client)!; + Assert.Equal(50, udp.Client.ReceiveTimeout); + } + + [Fact] + public void Receive_DifferingTimeouts_UpdatesCachedValueEachTime() + { + using var client = new NetClient(new IPEndPoint(IPAddress.Loopback, 1)); + + client.Receive(TimeSpan.FromMilliseconds(40), out _); + client.Receive(TimeSpan.FromMilliseconds(120), out _); + + FieldInfo cacheField = typeof(NetClient).GetField( + "_lastAppliedReceiveTimeoutMs", BindingFlags.NonPublic | BindingFlags.Instance)!; + Assert.Equal(120, (int)cacheField.GetValue(client)!); + + FieldInfo udpField = typeof(NetClient).GetField( + "_udp", BindingFlags.NonPublic | BindingFlags.Instance)!; + var udp = (UdpClient)udpField.GetValue(client)!; + Assert.Equal(120, udp.Client.ReceiveTimeout); + } + [Fact] public void SendReceive_LoopbackSelfTest_EchoRoundTrip() { diff --git a/tests/AcDream.Core.Net.Tests/WorldSessionNetReceiveLoopResilienceTests.cs b/tests/AcDream.Core.Net.Tests/WorldSessionNetReceiveLoopResilienceTests.cs new file mode 100644 index 00000000..ff0ca527 --- /dev/null +++ b/tests/AcDream.Core.Net.Tests/WorldSessionNetReceiveLoopResilienceTests.cs @@ -0,0 +1,102 @@ +using System.Net; +using System.Net.Sockets; +using System.Reflection; + +namespace AcDream.Core.Net.Tests; + +/// +/// 2026-07-24 audit fix: WorldSession.NetReceiveLoop used to wrap its +/// entire while loop in a single try/catch, so a non-timeout +/// (classically Windows delivering a delayed +/// WSAECONNRESET off an earlier Send's ICMP port-unreachable) exited the +/// loop for good and silently killed all inbound processing for the rest of +/// the session. These tests drive the actual private NetReceiveLoop +/// method — via the internal seam and +/// reflection, since the method has no public entry point — with a scripted +/// transport that throws a non-timeout SocketException on the first call. +/// No real sockets are involved, so the tests are deterministic. +/// +public sealed class WorldSessionNetReceiveLoopResilienceTests +{ + [Fact] + public void NetReceiveLoop_NonTimeoutSocketException_LogsAndKeepsReceiving() + { + var transport = new ScriptedTransport(); + var session = new WorldSession( + new IPEndPoint(IPAddress.Loopback, 9000), + transport); + + MethodInfo loopMethod = typeof(WorldSession).GetMethod( + "NetReceiveLoop", BindingFlags.NonPublic | BindingFlags.Instance)!; + + // Deliberately does NOT redirect Console.Error: xUnit runs test + // classes in this assembly in parallel by default, and Console.Error + // is process-global mutable state, so swapping it out here could + // race with another class's concurrently running test and either + // swallow its output or restore the wrong writer. The behavioral + // assertions below (call count + queue drain) fully cover the fix + // without needing to observe the log line's exact text. + var thread = new Thread(() => loopMethod.Invoke(session, null)) + { + IsBackground = true, + Name = "test.net-receive-loop", + }; + thread.Start(); + + // The scripted transport throws SocketException on call 1, returns + // one datagram on call 2, then throws ObjectDisposedException on + // call 3 to end the loop the same way a real shutdown-time + // disposal would. If the pre-fix bug were still present, the loop + // would exit after call 1 and this thread would never terminate on + // its own via the scripted ObjectDisposedException (call 3 would + // never be reached). + bool exited = thread.Join(TimeSpan.FromSeconds(5)); + Assert.True(exited, "NetReceiveLoop did not exit after the scripted ObjectDisposedException — " + + "the non-timeout SocketException likely killed the loop before it reached call 3."); + + // Proves the loop survived the first (non-timeout) SocketException + // and went on to make a second AND third Receive() call, instead of + // exiting after call 1 the way the pre-fix code did. + Assert.Equal(3, transport.CallCount); + + // The datagram handed back on call 2 must have made it into the + // inbound queue — i.e. the loop's normal write path still runs + // after recovering from the error. + int processed = session.Tick(); + Assert.Equal(1, processed); + } + + private sealed class ScriptedTransport : IWorldSessionTransport + { + private int _calls; + + public int CallCount => _calls; + + public void Send(ReadOnlySpan datagram) { } + + public void Send(IPEndPoint remote, ReadOnlySpan datagram) { } + + public byte[]? Receive(TimeSpan timeout, out IPEndPoint? from) + { + int n = Interlocked.Increment(ref _calls); + from = null; + switch (n) + { + case 1: + // Simulates a non-timeout SocketException, e.g. Windows' + // WSAECONNRESET off a stale peer's ICMP port-unreachable. + throw new SocketException((int)SocketError.ConnectionReset); + case 2: + from = new IPEndPoint(IPAddress.Loopback, 9000); + return new byte[] { 0xAA, 0xBB, 0xCC, 0xDD }; + default: + // Simulates NetClient having been disposed out from + // under the loop during shutdown — the pre-existing, + // still-preserved silent-exit path. + throw new ObjectDisposedException(nameof(ScriptedTransport)); + } + } + + public void Dispose() { } + } +}