From 3fd604ceac3d760714c0cad5fb2fc0e7c6679e78 Mon Sep 17 00:00:00 2001 From: Segfault <5221072+Segfaultd@users.noreply.github.com> Date: Thu, 8 Oct 2026 13:27:29 +0200 Subject: [PATCH] test(sessionconfig): wait on both sides at once in the static exchange tests SessionConfigLive.StaticExchangeDeliversBothPayloads failed intermittently (~2 in 10 on master) at the ID_NEW_INCOMING_CONNECTION assertion, always after burning the full 15s deadline. Root cause is in the test, not the library. In static mode SendSessionConfigResponse() releases the server's withheld ID_NEW_INCOMING_CONNECTION in the same step that it sends ID_SESSION_CONFIG to the client, so the server's connection packet is enqueued strictly before the client's ID_CONNECTION_REQUEST_ACCEPTED, which requires that message to make a round trip first. The test waited on the client with PumpUntil(), whose alsoPump drain discards every packet the other peer has queued. When the scheduler lands between the two enqueues, ID_NEW_INCOMING_CONNECTION is thrown away and the following wait can never observe it. Confirmed by instrumenting PumpUntil's discard path: under load, every failing run logged exactly one discarded ID_NEW_INCOMING_CONNECTION while waiting for ID_CONNECTION_REQUEST_ACCEPTED (6 discards, 6 failures). Both tests now use the existing PumpUntilBoth() helper, which was added for this hazard and which the rest of the suite already uses. Verified on macOS arm64 under the 8-way parallel load that reproduced the failure (5 failures in 115 runs before): 240/240 runs pass after the fix, plus SessionConfigLive.* --gtest_repeat=10 and the full 276-test ctest suite. Fixes #62 --- Tests/Integration/SessionConfigLiveTests.cpp | 33 ++++++++++++-------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/Tests/Integration/SessionConfigLiveTests.cpp b/Tests/Integration/SessionConfigLiveTests.cpp index cba702eb2..cb3123203 100644 --- a/Tests/Integration/SessionConfigLiveTests.cpp +++ b/Tests/Integration/SessionConfigLiveTests.cpp @@ -264,10 +264,20 @@ TEST_F(SessionConfigLive, StaticExchangeDeliversBothPayloads) const unsigned short port = StartPeers(); ASSERT_EQ(client->Connect("127.0.0.1", port, 0, 0), CONNECTION_ATTEMPT_STARTED); - Packet *accepted = PumpUntil(client, ID_CONNECTION_REQUEST_ACCEPTED, server, kConnectTimeoutMs); - ASSERT_NE(accepted, nullptr) << "client never reported a connection"; + // Both waits have to run in the same loop. In static mode the server releases its withheld + // ID_NEW_INCOMING_CONNECTION in the same step that it sends ID_SESSION_CONFIG, so it is queued + // strictly BEFORE the client turns that message into ID_CONNECTION_REQUEST_ACCEPTED. Waiting on + // the client first with PumpUntil() discards whatever the server has queued meanwhile, so any + // scheduling gap between the two throws ID_NEW_INCOMING_CONNECTION away and the second wait then + // burns its full deadline. + Packet *accepted = 0; + Packet *incoming = 0; + ASSERT_TRUE(PumpUntilBoth(client, ID_CONNECTION_REQUEST_ACCEPTED, &accepted, server, ID_NEW_INCOMING_CONNECTION, &incoming, kConnectTimeoutMs)) + << "the connection was not reported on both sides"; const RakNetGUID serverGuid = accepted->guid; + const RakNetGUID clientGuid = incoming->guid; client->DeallocatePacket(accepted); + server->DeallocatePacket(incoming); // The payload must already be readable — that is the whole contract of the withheld packet. unsigned int length = 0; @@ -276,11 +286,6 @@ TEST_F(SessionConfigLive, StaticExchangeDeliversBothPayloads) ASSERT_EQ(length, (unsigned int)strlen(kServerPayload)); EXPECT_EQ(memcmp(fromServer, kServerPayload, length), 0); - Packet *incoming = PumpUntil(server, ID_NEW_INCOMING_CONNECTION, client, kConnectTimeoutMs); - ASSERT_NE(incoming, nullptr) << "server never reported a connection"; - const RakNetGUID clientGuid = incoming->guid; - server->DeallocatePacket(incoming); - length = 0; const char *fromClient = server->GetRemoteSessionConfig(clientGuid, &length); ASSERT_NE(fromClient, nullptr); @@ -294,18 +299,20 @@ TEST_F(SessionConfigLive, EmptyPayloadsStillConnect) const unsigned short port = StartPeers(); ASSERT_EQ(client->Connect("127.0.0.1", port, 0, 0), CONNECTION_ATTEMPT_STARTED); - Packet *accepted = PumpUntil(client, ID_CONNECTION_REQUEST_ACCEPTED, server, kConnectTimeoutMs); - ASSERT_NE(accepted, nullptr); + // Same ordering hazard as StaticExchangeDeliversBothPayloads: the handshake still runs with empty + // payloads, so the server's connection packet is queued before the client's and must not be + // discarded by a one-sided wait. + Packet *accepted = 0; + Packet *incoming = 0; + ASSERT_TRUE(PumpUntilBoth(client, ID_CONNECTION_REQUEST_ACCEPTED, &accepted, server, ID_NEW_INCOMING_CONNECTION, &incoming, kConnectTimeoutMs)) + << "the connection was not reported on both sides"; const RakNetGUID serverGuid = accepted->guid; client->DeallocatePacket(accepted); + server->DeallocatePacket(incoming); unsigned int length = 12345; client->GetRemoteSessionConfig(serverGuid, &length); EXPECT_EQ(length, 0u); - - Packet *incoming = PumpUntil(server, ID_NEW_INCOMING_CONNECTION, client, kConnectTimeoutMs); - ASSERT_NE(incoming, nullptr); - server->DeallocatePacket(incoming); } // The load-bearing assertion. In interactive mode the server holds the decision, so neither peer may