Fix SctpDataSender lost-wakeup race: move _senderMre.Reset() to top of DoSend loop (#1560)

Root cause: _senderMre.Reset() was called AFTER the send work and BEFORE
_senderMre.Wait(burstPeriod). If a SACK arrived between the last chunk
sent and the Reset, its _senderMre.Set() signal was wiped by the Reset()
and the sender thread then blocked for the full BURST_PERIOD_MILLISECONDS
(50 ms default) before noticing more cwnd was available. On localhost
loopback where SACKs round-trip in microseconds the race window was hit
almost every burst, capping actual throughput at the theoretical
MAX_BURST * MTU / BURST_PERIOD steady-state (~104 KB/s for MTU=1300 +
50 ms period) rather than the much higher rate the link could sustain.

Fix: Reset at the START of each iteration so any Set() fired during the
send work is preserved through to the next Wait(). Wait returns promptly
on the SACK signal, and throughput becomes bounded by MRE wake latency
(sub-millisecond) rather than the 50 ms burst period. RFC 4960 §7.2.2
defaults (MAX_BURST, BURST_PERIOD_MILLISECONDS) unchanged.

Includes a regression test, SctpDataSenderUnitTest.Throughput_FastSack
Wake_ExceedsBurstCeiling: sends 360 x 1400-byte chunks (504 KB total)
with synchronous SACK delivery on the same thread, asserts completion
in under 2 seconds. Pre-fix measurement: 5613 ms / 89.8 KB/s. Post-fix
on the same machine: 94 ms / 5.4 MB/s (60x speedup). Pre-fix test
fails at the 2000 ms threshold; post-fix passes with ~20x headroom.

No regressions on the existing SctpData* tests (53/55 pass, 2 pre-
existing CI skips unrelated to this change).

Diagnostic credit to Geordi (a crew AI agent on the LostBeard team)
who traced the throughput ceiling to SctpDataSender.cs line by line
before filing the handoff that prompted this investigation. Fix
authored by LostBeard with AI-assisted code review from Riker (another
crew AI agent) and a Rule-4b verify-before-you-speak step where the
test was run against an intentionally-reverted version of the fix to
prove the failure mode matches the measured ceiling before re-applying.

Author: LostBeard

src/SIPSorcery/net/SCTP/SctpDataSender.cs

@@ -526,6 +526,19 @@ private void DoSend(object state)
 
             while (!_isClosed)
             {
+                // Reset the sender wake event at the START of each iteration. Resetting
+                // AFTER the send work introduces a lost-wakeup race with SACK arrival:
+                // a SACK that fires _senderMre.Set() between the last chunk sent and
+                // the Reset() gets its signal wiped by Reset(), and the thread then
+                // blocks in _senderMre.Wait() for the full BURST_PERIOD_MILLISECONDS
+                // even though more work is ready to go. On loopback where SACKs
+                // round-trip in microseconds this window is hit almost every burst,
+                // capping throughput at the RFC4960 §7.2.2 steady-state of roughly
+                // MAX_BURST * MTU / BURST_PERIOD (~104 KB/s for MTU=1300, period=50ms).
+                // Resetting first preserves any Set() that happens during send work so
+                // Wait() returns immediately on the next iteration.
+                _senderMre.Reset();
+
                 var outstandingBytes = _outstandingBytes;
                 // DateTime.Now calls have been a tiny bit expensive in the past so get a small saving by only
                 // calling once per loop.
@@ -615,8 +628,6 @@ private void DoSend(object state)
                     }
                 }
 
-                _senderMre.Reset();
-
                 int wait = GetSendWaitMilliseconds();
                 //logger.LogTrace($"SCTP sender wait period {wait}ms, arwnd {_receiverWindow}, cwnd {_congestionWindow} " +
                 //    $"outstanding bytes {_outstandingBytes}, send queue {_sendQueue.Count}, missing {_missingChunks.Count} "

test/unit/net/SCTP/SctpDataSenderUnitTest.cs

@@ -15,7 +15,9 @@
 
 using System;
 using System.Collections.Concurrent;
+using System.Diagnostics;
 using System.Linq;
+using System.Threading;
 using System.Threading.Tasks;
 using Microsoft.Extensions.Logging;
 using SIPSorcery.Sys;
@@ -90,6 +92,75 @@ public async Task FillCongestionWindow()
             Assert.True(sender._congestionWindow < sender._outstandingBytes);
         }
 
+        /// <summary>
+        /// Regression test for the DoSend Reset-race (producer-consumer lost-wakeup).
+        /// Before the fix, <c>_senderMre.Reset()</c> ran AFTER the send work, so any
+        /// <c>Set()</c> fired by an incoming SACK in the window between the last chunk
+        /// sent and the Reset was wiped, and the thread blocked for the full
+        /// <see cref="SctpDataSender.BURST_PERIOD_MILLISECONDS"/> (50 ms) before noticing
+        /// more cwnd was available. This capped throughput at
+        /// <c>MAX_BURST * MTU / BURST_PERIOD = 4 * 1300 / 50 ms = 104 KB/s</c> even on
+        /// localhost loopback where SACKs round-trip in microseconds.
+        ///
+        /// With the fix (Reset moved to the TOP of the loop, preserving Set-during-send
+        /// for the next Wait), 500 KB ships in well under 500 ms on any reasonable
+        /// machine - the bottleneck becomes the ManualResetEventSlim wake latency
+        /// (sub-millisecond), not the 50 ms burst period.
+        ///
+        /// Pre-fix expected: 500 KB / 104 KB/s = ~4800 ms + slow-start ramp = ~5+ seconds.
+        /// Post-fix expected: under 500 ms.
+        /// Threshold set at 2000 ms to leave slack for CI jitter while still proving
+        /// we're not on the broken 100-KB/s ceiling.
+        /// </summary>
+        [Fact]
+        public async Task Throughput_FastSackWake_ExceedsBurstCeiling()
+        {
+            uint arwnd = SctpAssociation.DEFAULT_ADVERTISED_RECEIVE_WINDOW;
+            ushort mtu = 1400;
+            uint initialTSN = 0;
+
+            SctpDataReceiver receiver = new SctpDataReceiver(arwnd, mtu, initialTSN);
+            SctpDataSender sender = new SctpDataSender("throughput-probe", null, mtu, initialTSN, arwnd);
+
+            // Deliver data + SACK synchronously on the sender's own DoSend thread - the
+            // worst case for the Reset race, because the SACK's Set() fires BEFORE the
+            // Reset that wiped it pre-fix.
+            Action<SctpDataChunk> doSend = (chunk) =>
+            {
+                receiver.OnDataChunk(chunk);
+                sender.GotSack(receiver.GetSackChunk());
+            };
+            sender._sendDataChunk = doSend;
+            sender.StartSending();
+
+            // 360 chunks of 1400 bytes = 504 KB. Large enough to exit slow-start and
+            // enter the steady-state where the race would dominate, small enough that
+            // a fast machine finishes in milliseconds.
+            const int chunksToSend = 360;
+            var buffer = new byte[mtu];
+
+            var sw = Stopwatch.StartNew();
+            for (int i = 0; i < chunksToSend; i++)
+            {
+                sender.SendData(0, 0, buffer);
+            }
+
+            // Wait for all chunks to be acknowledged by the receiver.
+            uint expectedAckTSN = (uint)(chunksToSend - 1);
+            while (receiver.CumulativeAckTSN != expectedAckTSN && sw.ElapsedMilliseconds < 10000)
+            {
+                await Task.Delay(5);
+            }
+            sw.Stop();
+
+            Assert.Equal(expectedAckTSN, receiver.CumulativeAckTSN);
+            Assert.True(sw.ElapsedMilliseconds < 2000,
+                $"Throughput regression: {chunksToSend * mtu} bytes took {sw.ElapsedMilliseconds} ms " +
+                $"(effective {(double)(chunksToSend * mtu) / sw.ElapsedMilliseconds:F1} KB/s). " +
+                $"Expected under 2000 ms with SACK-wake race fix. Pre-fix throughput is " +
+                $"capped at ~104 KB/s by the Reset race, giving ~{chunksToSend * mtu / 104} ms.");
+        }
+
         /// <summary>
         /// Tests that the congestion window increases in slow start mode.
         /// </summary>