Scope the datagram send state indication to servers (#50)

Description
Brings this branch in line with the upstream version of the #47 fix, microsoft#6179.

#47 fixed the real bug — a server application is never told it may send datagrams unless path MTU discovery happens to change the max send length — but it changed client behaviour on the way. Preparing the same fix for upstream showed that the client-side part is neither needed nor desirable, so this narrows it.

What #47 did to clients
#47 re-evaluated the send state for every connection at QuicConnSetConfiguration, and started the indicated state at (FALSE, 0) for both roles. On a client that produces an extra DATAGRAM_STATE_CHANGED at ConnectionStart, carrying SendEnabled = TRUE before the peer's transport parameters are known. That indication is speculative: when the peer turns out not to support datagrams it is reversed a moment later. It also required changing thirteen ordered event expectations in EventTest.cpp, which is the test suite telling us the client's event sequence changed.

What this changes
Two changes make the fix server-only.

The re-evaluation at SetConfiguration runs only for servers. A client's send state changes always have an owner to indicate to, so its indicated state never goes stale and it needs nothing there.

The indicated state starts out as what the application can be assumed to know already, which differs by role. A client opens the connection itself, so it knows the documented initial state of (TRUE, UINT16_MAX). A server is handed a connection whose send state was settled before it ever saw it, so it knows nothing and assumes the conservative default of not being able to send.

Result
Client behaviour is bit-for-bit identical to before Track the datagram send state last indicated to the application #47.
Servers gain exactly one indication: the application learns, once, that it may send datagrams.
A server whose peer does not support datagrams is still told nothing, matching what it already assumes. No spurious event is introduced.
EventTest.cpp needs no changes, which is the check that client behaviour is untouched.
The core of #47 — tracking the state last indicated separately from the live state, so a change made while unowned is not mistaken for one already reported — is unchanged and still what makes the fix work when the recomputed values happen to match those recorded while unowned.

Testing
Also carried over from upstream: QuicTestDatagramNegotiation now checks that the server was told it can send, rather than only that it can. TestConnection records the count and contents of the DATAGRAM_STATE_CHANGED events indicated to it, since GetParam(QUIC_PARAM_CONN_DATAGRAM_SEND_ENABLED) reports the live state whether or not the application ever heard about it.

With the server re-evaluation removed, DatagramNegotiation fails on all four parameterisations, so the assertion covers the original bug.
*Datagram*:*Event*:*ValidateConn*:*Basic*:*Mtu*:*Resumption* passes in full: 529 tests, no failures. No compiler warnings.
Documentation
No documentation change. docs/API.md and docs/api/QUIC_CONNECTION_EVENT.md already describe the behaviour this implements.

Author: masa-koz

src/core/connection.c

@@ -127,7 +127,7 @@ QuicConnAlloc(
     QuicOperationQueueInitialize(&Connection->OperQ);
     QuicSendInitialize(&Connection->Send, &Connection->Settings);
     QuicPathIDSetInitialize(&Connection->PathIDs);
-    QuicDatagramInitialize(&Connection->Datagram);
+    QuicDatagramInitialize(&Connection->Datagram, IsServer);
     QuicRangeInitialize(
         QUIC_MAX_RANGE_DECODE_ACKS,
         &Connection->DecodedAckRanges);
@@ -2445,21 +2445,21 @@ QuicConnSetConfiguration(
         "[conn][%p] Handshake start",
         Connection);
 
-    //
-    // Re-evaluate the datagram send state now that the connection is started
-    // and owned by the application.
-    //
-    // `Started` is an input to the max send length (QuicDatagramOnSendStateChanged
-    // derives it from QUIC_DPLPMTUD_MIN_MTU until then, and from the path's MTU
-    // afterwards), so it has to be recomputed here regardless. For a server this
-    // is also the first evaluation with an owner to indicate to: the peer's
-    // transport parameters are processed before the listener hands the
-    // connection over, so nothing before this point could reach the application.
-    //
-    // Clients reach this path before any peer transport parameters exist, so
-    // their behaviour is unchanged.
-    //
-    QuicDatagramOnSendStateChanged(&Connection->Datagram);
+    if (QuicConnIsServer(Connection)) {
+        //
+        // Evaluate the datagram send state for a server. This is the first
+        // point at which both of its inputs are settled and there is an
+        // external owner to indicate the result to: the peer's transport
+        // parameters are processed before the listener hands the connection to
+        // the application, and `Started` selects the MTU that the max send
+        // length is derived from.
+        //
+        // A client's send state is evaluated once its peer's transport
+        // parameters arrive, which is always after it has an owner, so it needs
+        // nothing here.
+        //
+        QuicDatagramOnSendStateChanged(&Connection->Datagram);
+    }
 
     Status =
         QuicCryptoInitializeTls(

src/core/datagram.c

@@ -74,13 +74,21 @@ QuicCalculateDatagramLength(
 _IRQL_requires_max_(PASSIVE_LEVEL)
 void
 QuicDatagramInitialize(
-    _In_ QUIC_DATAGRAM* Datagram
+    _In_ QUIC_DATAGRAM* Datagram,
+    _In_ BOOLEAN IsServer
     )
 {
     Datagram->SendEnabled = TRUE;
     Datagram->MaxSendLength = UINT16_MAX;
-    Datagram->IndicatedSendEnabled = FALSE;
-    Datagram->IndicatedMaxSendLength = 0;
+    //
+    // The indicated state starts out as what the application can be assumed to
+    // know already. A client opens the connection itself, so it knows the initial
+    // state set just above; a server is handed a connection whose send state is
+    // settled before it ever sees it, so it knows nothing and assumes the
+    // conservative default of not being able to send.
+    //
+    Datagram->IndicatedSendEnabled = IsServer ? FALSE : TRUE;
+    Datagram->IndicatedMaxSendLength = IsServer ? 0 : UINT16_MAX;
     Datagram->PrioritySendQueueTail = &Datagram->SendQueue;
     Datagram->SendQueueTail = &Datagram->SendQueue;
     CxPlatDispatchLockInitialize(&Datagram->ApiQueueLock);
@@ -286,13 +294,11 @@ QuicDatagramOnSendStateChanged(
     }
 
     //
-    // Whether the live state moved, and whether the application's view of it is
-    // stale, are separate questions. They diverge when the state changes with no
-    // external owner to indicate to, which is the normal case for a server: the
-    // peer's transport parameters are processed before the listener hands the
-    // connection over. Comparing only against the live state there would leave
-    // the change looking already reported, and the application would never learn
-    // that datagrams are sendable.
+    // Whether the live state moved and whether the application's view of it is
+    // stale are separate questions, so they are asked separately. The two answers
+    // differ whenever the state changed with no external owner to indicate to,
+    // which is the normal case for a server: the peer's transport parameters are
+    // processed before the listener hands the connection over.
     //
     const BOOLEAN StateChanged =
         SendEnabled != Datagram->SendEnabled ||

src/core/datagram.h

@@ -34,11 +34,10 @@ typedef struct QUIC_DATAGRAM {
 
     //
     // The send state last indicated to the application, which is not always the
-    // live state above. The send state can change while the connection has no
-    // external owner to indicate to — a server connection processes the peer's
-    // transport parameters before the listener hands it to the application —
-    // and such a change must not be mistaken for one that was already reported,
-    // or the application is never told at all.
+    // live state above. The two differ while a change made with no external
+    // owner to indicate it to remains unreported, which is the normal case for a
+    // server: the peer's transport parameters are processed before the listener
+    // hands the connection to the application.
     //
     uint16_t IndicatedMaxSendLength;
 
@@ -49,9 +48,7 @@ typedef struct QUIC_DATAGRAM {
     BOOLEAN SendEnabled : 1;
 
     //
-    // The `SendEnabled` last indicated to the application. Starts FALSE: until
-    // an indication is made the application has been told nothing, and assumes
-    // datagrams are not sendable.
+    // The `SendEnabled` last indicated to the application.
     //
     BOOLEAN IndicatedSendEnabled : 1;
 
@@ -60,7 +57,8 @@ typedef struct QUIC_DATAGRAM {
 _IRQL_requires_max_(PASSIVE_LEVEL)
 void
 QuicDatagramInitialize(
-    _In_ QUIC_DATAGRAM* Datagram
+    _In_ QUIC_DATAGRAM* Datagram,
+    _In_ BOOLEAN IsServer
     );
 
 _IRQL_requires_max_(PASSIVE_LEVEL)

src/test/lib/DatagramTest.cpp

@@ -111,6 +111,16 @@ QuicTestDatagramNegotiation(
 
                 TEST_TRUE(Server->GetDatagramSendEnabled()); // Client always enabled
 
+                //
+                // Being able to send datagrams is not enough; the app has to be
+                // told. For a server that indication comes once the connection
+                // is started, the first point at which its send state has an
+                // owner to be indicated to.
+                //
+                TEST_TRUE(Server->GetDatagramStateChangedCount() > 0);
+                TEST_TRUE(Server->GetIndicatedDatagramSendEnabled());
+                TEST_NOT_EQUAL(0, Server->GetIndicatedDatagramMaxSendLength());
+
                 CxPlatSleep(100); // Necessary?
 
                 if (DatagramReceiveEnabled) {

src/test/lib/TestConnection.cpp

@@ -948,7 +948,9 @@ TestConnection::HandleConnectionEvent(
         break;
 
     case QUIC_CONNECTION_EVENT_DATAGRAM_STATE_CHANGED:
-        // Use This
+        DatagramStateChangedCount++;
+        IndicatedDatagramSendEnabled = Event->DATAGRAM_STATE_CHANGED.SendEnabled != FALSE;
+        IndicatedDatagramMaxSendLength = Event->DATAGRAM_STATE_CHANGED.MaxSendLength;
         break;
 
     case QUIC_CONNECTION_EVENT_RESUMPTION_TICKET_RECEIVED:

src/test/lib/TestConnection.h

@@ -106,6 +106,10 @@ class TestConnection
     uint32_t DatagramsLost{};
     uint32_t DatagramsAcknowledged{};
 
+    uint32_t DatagramStateChangedCount{};
+    bool IndicatedDatagramSendEnabled{false};
+    uint16_t IndicatedDatagramMaxSendLength{};
+
     const uint8_t* NegotiatedAlpn{};
     uint8_t NegotiatedAlpnLength{};
 
@@ -336,6 +340,25 @@ class TestConnection
         LockGuard LockScope{Lock};
         return DatagramsCanceled;
     }
+    //
+    // The state carried by the last QUIC_CONNECTION_EVENT_DATAGRAM_STATE_CHANGED
+    // indicated to this connection, and how many have been indicated. Distinct
+    // from GetDatagramSendEnabled, which queries the live state whether or not
+    // the app was ever told about it.
+    //
+    uint32_t GetDatagramStateChangedCount() const {
+        LockGuard LockScope{Lock};
+        return DatagramStateChangedCount;
+    }
+    bool GetIndicatedDatagramSendEnabled() const {
+        LockGuard LockScope{Lock};
+        return IndicatedDatagramSendEnabled;
+    }
+    uint16_t GetIndicatedDatagramMaxSendLength() const {
+        LockGuard LockScope{Lock};
+        return IndicatedDatagramMaxSendLength;
+    }
+
     uint32_t GetDatagramsSuspectLost() const {
         LockGuard LockScope{Lock};
         return DatagramsSuspectLost;