From cf25330458c462c936528fde2ef9821a6bf1f282 Mon Sep 17 00:00:00 2001 From: Erik Date: Fri, 24 Jul 2026 11:38:37 +0200 Subject: [PATCH] fix(net): survive transient socket errors in the receive loop (2026-07-24 audit review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit WorldSession.NetReceiveLoop wrapped its entire while loop in a single try/catch, so ANY non-timeout SocketException permanently killed the background receive thread: the catch block at the loop's end was empty (misattributed the error to "socket closed during shutdown"), and the finally called _inboundQueue.Writer.TryComplete(), which silently and irrecoverably stopped all inbound processing for the rest of the session — no log line, no recovery path, and LiveSessionHost.Reconnect has zero production callers to notice. The realistic trigger is a well-known Windows UdpClient quirk: an ICMP "port unreachable" reply to an EARLIER Send (e.g. against a stale ACE session that already tore down its socket) surfaces as a WSAECONNRESET SocketException on this socket's NEXT, completely unrelated Receive call. NetClient.Receive already swallows the expected SocketError.TimedOut heartbeat case; anything else reaching WorldSession was a real, transient, per-datagram error being treated as session-fatal. Three changes, root-cause not a band-aid: - NetClient's constructor now disables SIO_UDP_CONNRESET reporting on Windows, so a delayed ICMP error can't poison receives at all. - NetReceiveLoop now catches SocketException PER ITERATION, logs it, and continues polling instead of exiting. The existing clean-shutdown paths (cancellation, ObjectDisposedException during Dispose) are unchanged — only the non-timeout-socket-error case that used to kill the loop is now recoverable. - NetClient.Receive no longer calls the ReceiveTimeout setter (a setsockopt syscall) on every single call — only when the requested timeout differs from the last-applied value, cached in a new field. This was an unrelated but adjacent finding (4x/sec syscall churn at the 250ms heartbeat cadence) in the same audit. No change to outbound wire behavior, ack cadence, heartbeat interval, or datagram ordering — this is purely receive-loop resilience. Tests: NetClientTests gained a SIO_UDP_CONNRESET construction smoke test and two ReceiveTimeout-caching tests. A new WorldSessionNetReceiveLoopResilienceTests drives the actual private NetReceiveLoop method (via the existing internal IWorldSessionTransport seam + reflection) with a scripted transport that throws a non-timeout SocketException on the first call, proving the loop survives it and keeps enqueueing subsequent datagrams — fully deterministic, no real sockets. Full solution suite green: 3204/3206 Core, 3462/3465 App, 552/552 Core.Net (skips pre-existing). Co-Authored-By: Claude Fable 5 (cherry picked from commit c72ce028a927e15b8a54a86bbd0661a723dc84ca) --- src/AcDream.Core.Net/NetClient.cs | 24 ++++- src/AcDream.Core.Net/WorldSession.cs | 32 +++++- .../AcDream.Core.Net.Tests/NetClientTests.cs | 60 +++++++++++ ...rldSessionNetReceiveLoopResilienceTests.cs | 102 ++++++++++++++++++ 4 files changed, 215 insertions(+), 3 deletions(-) create mode 100644 tests/AcDream.Core.Net.Tests/WorldSessionNetReceiveLoopResilienceTests.cs 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() { } + } +}