From 587f54a4d70546ae22da9bff7005da247c55c819 Mon Sep 17 00:00:00 2001 From: Segfault <5221072+Segfaultd@users.noreply.github.com> Date: Thu, 8 Oct 2026 17:20:38 +0200 Subject: [PATCH 1/2] test(ping): assert occasional ping directly instead of via a latency threshold PingTests.PingStatisticsAndOccasionalPing failed intermittently (7/12 under CPU load, 40/40 idle) on assertions that measure the host rather than the library: a loopback average of 12 ms against a 10 ms ceiling is the scheduler, not a bug. It also bound fixed port 60000, which CLAUDE.md warns against. Loosening the ceilings alone would have removed coverage. A mutant making SetOccasionalPing a no-op is caught today -- but only because one missing ping sample leaves the average above 10 ms. The absolute threshold was doing double duty as the sole check on the feature the test is named after, indirectly and by a margin of 2 ms. So the coverage is moved before the thresholds are relaxed: - OccasionalPingIsObservableAsPingTraffic counts ID_CONNECTED_PING arriving at the receiver through a PluginInterface2 OnInternalPacket hook. SetOccasionalPing has no public observable -- the pings never surface as packets -- and this is the layer where they are visible. Two senders run in the SAME window, one with the flag on and one off, so the only difference between the counts is the flag rather than two stretches of machine time. The window is 11 s to outrun the 5 s interval in peer.cpp. - The statistics test gains the relational assertions, which hold at any latency: statistics are populated at all, lastPing >= lowestPing, averagePing >= lowestPing. These fail only on a real bug. - Absolute ceilings become a single generous set (no CI/non-CI split: a test held to different standards per environment cannot be reproduced locally when CI fails) and are asserted last, so a load-induced trip no longer hides the real invariants. - OS-assigned ephemeral port. Verified on macOS arm64: - new test vs the no-op SetOccasionalPing mutant: fails 3/3, reporting "2 pings with it enabled vs 2 with it disabled" - clean margin is stable at 4 pings on vs 2 off across 4 runs - statistics test under the CPU load that gave 7/12: now 12/12 - full ctest 285/285 Fixes #68 --- Tests/Integration/PingTestsTests.cpp | 166 +++++++++++++++++++++++++-- 1 file changed, 156 insertions(+), 10 deletions(-) diff --git a/Tests/Integration/PingTestsTests.cpp b/Tests/Integration/PingTestsTests.cpp index 16e38b924..fc5b8d7fd 100644 --- a/Tests/Integration/PingTestsTests.cpp +++ b/Tests/Integration/PingTestsTests.cpp @@ -11,8 +11,13 @@ #include "CommonFunctions.h" #include "RakTimer.h" +#include #include +#include "mafianet/internal_packet.h" +#include "mafianet/message_identifiers.h" +#include "mafianet/plugin_interface2.h" + using namespace MafiaNet; /* @@ -45,6 +50,49 @@ SetOccasionalPing namespace { + // Counts ID_CONNECTED_PING arriving from each of two senders. + // + // SetOccasionalPing has no public observable: the pings it triggers are consumed inside the + // library and never surface as packets. The old test inferred it from the average ping dropping + // below a 10 ms threshold -- one extra sample moving an average across a hard bound, which is both + // indirect and the reason the test tripped on ordinary machine load. OnInternalPacket sees the + // pings themselves, on the receiving side, and counting them is a direct assertion. + // + // UsesReliabilityLayer() must return true for OnInternalPacket to be wired up, which also means + // this has to be attached before Startup(). + class ConnectedPingCounter : public PluginInterface2 + { + public: + std::atomic portA; + std::atomic portB; + std::atomic countA; + std::atomic countB; + + ConnectedPingCounter() : portA(0), portB(0), countA(0), countB(0) {} + + virtual bool UsesReliabilityLayer(void) const { return true; } + + // Runs on the network thread; the counters are atomic because the test reads them from its own. + virtual void OnInternalPacket(InternalPacket *internalPacket, unsigned frameNumber, + SystemAddress remoteSystemAddress, MafiaNet::TimeMS time, int isSend) + { + (void) frameNumber; + (void) time; + if (isSend) + return; + if (internalPacket == 0 || internalPacket->data == 0 || internalPacket->dataBitLength < 8) + return; + if (internalPacket->data[0] != (unsigned char)ID_CONNECTED_PING) + return; + + const unsigned short port = remoteSystemAddress.GetPort(); + if (port != 0 && port == portA.load()) + ++countA; + else if (port != 0 && port == portB.load()) + ++countB; + } + }; + ::testing::AssertionResult AverageValueOk(int averagePing, int maxAveragePingMs) { if (averagePing < 0) @@ -77,11 +125,20 @@ class PingTests : public ::testing::Test TEST_F(PingTests, PingStatisticsAndOccasionalPing) { - // Relax timing thresholds in CI environments (Docker/emulated can have higher latency) - const bool isCI = (getenv("CI") != nullptr); - const int maxLastPingMs = isCI ? 500 : 100; // Allow higher ping in CI - const int maxLowestPingMs = isCI ? 100 : 10; // Allow higher lowest ping in CI - const int maxAveragePingMs = isCI ? 100 : 10; // Allow higher average ping in CI + // Absolute ceilings are a sanity bound against a gross regression -- a stall, a milliseconds/ + // microseconds mix-up -- not a measurement of the host. The old local values (10 ms average and + // lowest) were tight enough that ordinary machine load tripped them: a loopback average of 12 ms on + // a busy machine is the library behaving correctly and the assertion measuring the CPU scheduler. + // + // They were also carrying the only check that SetOccasionalPing did anything, by way of one extra + // ping sample pulling the average under the bound. That is now asserted directly in + // OccasionalPingIsObservableAsPingTraffic, so these can be loosened without losing coverage. + // + // One set of numbers rather than a CI/non-CI split: a test that holds to different standards + // depending on the environment cannot be reproduced locally when it fails in CI. + const int maxLastPingMs = 1000; + const int maxLowestPingMs = 500; + const int maxAveragePingMs = 500; RakPeerInterface *sender, *sender2, *receiver; @@ -91,15 +148,16 @@ TEST_F(PingTests, PingStatisticsAndOccasionalPing) receiver = RakPeerInterface::GetInstance(); destroyList.Push(receiver, _FILE_AND_LINE_); - SocketDescriptor receiverSd(60000, 0); + SocketDescriptor receiverSd(0, "127.0.0.1"); // OS-assigned, per CLAUDE.md -- 60000 was fixed receiver->Startup(2, &receiverSd, 1); receiver->SetMaximumIncomingConnections(2); + const unsigned short receiverPort = receiver->GetInternalID(UNASSIGNED_SYSTEM_ADDRESS).GetPort(); Packet *packet; SystemAddress currentSystem; currentSystem.SetBinaryAddress("127.0.0.1"); - currentSystem.SetPortHostOrder(60000); + currentSystem.SetPortHostOrder(receiverPort); printf("Connecting sender2\n"); ASSERT_TRUE(TestHelpers::WaitAndConnectTwoPeersLocally(sender2, receiver, 5000)) << "Could not connect after 5 seconds"; @@ -136,15 +194,24 @@ TEST_F(PingTests, PingStatisticsAndOccasionalPing) lastPing = sender2->GetLastPing(currentSystem); lowestPing = sender2->GetLowestPing(currentSystem); + // The relational properties are the real correctness content: they hold at any latency, so they + // fail only on a genuine bug rather than on a loaded machine. + ASSERT_GE(lastPing, 0) << "GetLastPing reported no data after an explicitly pinged connection"; + ASSERT_GE(lowestPing, 0) << "GetLowestPing reported no data after an explicitly pinged connection"; + ASSERT_GE(averagePing, 0) << "GetAveragePing reported no data after an explicitly pinged connection"; + + ASSERT_GE(lastPing, lowestPing) << "There is a problem if the lastping is lower than the lowestping stat"; + EXPECT_GE(averagePing, lowestPing) << "the average ping cannot be below the lowest sample"; + + // Absolute bounds last, so a load-induced trip is reported after the real invariants have been + // checked rather than hiding them. ASSERT_TRUE(AverageValueOk(averagePing, maxAveragePingMs)); ASSERT_LE(lastPing, maxLastPingMs) << "Problem with the last ping time, greater than expected for localhost"; ASSERT_LE(lowestPing, maxLowestPingMs) << "The lowest ping for localhost should drop below expected threshold at least once"; - ASSERT_GE(lastPing, lowestPing) << "There is a problem if the lastping is lower than the lowestping stat"; - - CommonFunctions::DisconnectAndWait(sender2, (char *) "127.0.0.1", 60000);//Eliminate variables. + CommonFunctions::DisconnectAndWait(sender2, (char *) "127.0.0.1", receiverPort);//Eliminate variables. printf("Connecting sender\n"); ASSERT_TRUE(TestHelpers::WaitAndConnectTwoPeersLocally(sender, receiver, 5000)) << "Could not connect after 5 seconds"; @@ -171,3 +238,82 @@ TEST_F(PingTests, PingStatisticsAndOccasionalPing) ASSERT_TRUE(AverageValueOk(averagePing, maxAveragePingMs)); } + +// Direct coverage for SetOccasionalPing, which the statistics test only ever inferred from an +// average-ping threshold. +// +// Both senders run in the SAME observation window against the same receiver, one with occasional +// ping on and one off. Sequential phases would compare two different stretches of machine time; side +// by side, the only difference between the two counts is the flag. +// +// The window has to outrun the 5 s occasional-ping interval (peer.cpp: nextPingTime = timeMS + 5000), +// so the enabled sender is expected to produce the immediate ping plus at least one interval ping. +// The assertion that carries the test is the relative one: pings keep coming when it is on and stop +// when it is off. +TEST_F(PingTests, OccasionalPingIsObservableAsPingTraffic) +{ + ConnectedPingCounter counter; + + RakPeerInterface *receiver = RakPeerInterface::GetInstance(); + destroyList.Push(receiver, _FILE_AND_LINE_); + receiver->AttachPlugin(&counter); // before Startup: UsesReliabilityLayer() is true + SocketDescriptor receiverSd(0, "127.0.0.1"); + ASSERT_EQ(receiver->Startup(2, &receiverSd, 1), RAKNET_STARTED); + receiver->SetMaximumIncomingConnections(2); + const unsigned short receiverPort = receiver->GetInternalID(UNASSIGNED_SYSTEM_ADDRESS).GetPort(); + + RakPeerInterface *pinging, *quiet; + TestHelpers::StandardClientPrep(pinging, destroyList); + TestHelpers::StandardClientPrep(quiet, destroyList); + + counter.portA.store(pinging->GetInternalID(UNASSIGNED_SYSTEM_ADDRESS).GetPort()); + counter.portB.store(quiet->GetInternalID(UNASSIGNED_SYSTEM_ADDRESS).GetPort()); + ASSERT_NE(counter.portA.load(), 0); + ASSERT_NE(counter.portB.load(), 0); + ASSERT_NE(counter.portA.load(), counter.portB.load()); + + pinging->SetOccasionalPing(true); + quiet->SetOccasionalPing(false); + + ASSERT_TRUE(TestHelpers::WaitAndConnectTwoPeersLocally(pinging, receiver, 5000)) << "Could not connect after 5 seconds"; + ASSERT_TRUE(TestHelpers::WaitAndConnectTwoPeersLocally(quiet, receiver, 5000)) << "Could not connect after 5 seconds"; + + // Counts from the handshake are not what is under test; only what happens from here on is. + counter.countA.store(0); + counter.countB.store(0); + + // Longer than the 5 s interval so an enabled sender must ping more than once. + RakTimer timer(11000); + timer.Start(); + Packet *packet; + while (!timer.IsExpired()) + { + for (packet = receiver->Receive(); packet; receiver->DeallocatePacket(packet), packet = receiver->Receive()) + { + } + for (packet = pinging->Receive(); packet; pinging->DeallocatePacket(packet), packet = pinging->Receive()) + { + } + for (packet = quiet->Receive(); packet; quiet->DeallocatePacket(packet), packet = quiet->Receive()) + { + } + RakSleep(10); + } + + const int withOccasionalPing = counter.countA.load(); + const int withoutOccasionalPing = counter.countB.load(); + + // Printed on success too: the margin between these two is what makes the test meaningful, so a + // future reader can see it narrowing before it starts failing. + printf("ID_CONNECTED_PING received over 11 s: occasional ping on = %d, off = %d\n", + withOccasionalPing, withoutOccasionalPing); + + EXPECT_GE(withOccasionalPing, 2) + << "SetOccasionalPing(true) produced " << withOccasionalPing + << " pings over 11 s; the interval is 5 s, so at least two were due"; + EXPECT_GT(withOccasionalPing, withoutOccasionalPing) + << "occasional ping made no difference: " << withOccasionalPing << " pings with it enabled vs " + << withoutOccasionalPing << " with it disabled"; + + receiver->DetachPlugin(&counter); // before TearDown destroys the peer +} From 3da33882478cafad3676b022cdbd1a383d0e7e69 Mon Sep 17 00:00:00 2001 From: Segfault <5221072+Segfaultd@users.noreply.github.com> Date: Thu, 8 Oct 2026 17:38:59 +0200 Subject: [PATCH 2/2] fix(test): keep the ping-counting plugin alive through TearDown Review finding on #70, and a real one. ConnectedPingCounter was a local in the test body, while the receiver it is attached to is owned by destroyList and destroyed later in TearDown(). Any failed ASSERT_ before the detach -- the Startup check or either connect check -- returns with the plugin still attached and then destroys it, after which Shutdown() makes a virtual call into it (peer.cpp, OnRakPeerShutdown) and the network thread can still deliver OnInternalPacket during the shutdown window. Confirmed under ASAN by forcing an early return: the unfixed test dies with "AddressSanitizer: BUS on unknown address (pc 0x03e014000007)" -- a corrupted vtable pointer -- so one failed assertion became a crash that hid the original failure. With the plugin as a fixture member the same forced failure reports cleanly and keeps its message, and ASAN is silent. A fixture member is destroyed after TearDown() returns, so it outlives every peer that could call into it and the explicit DetachPlugin is no longer needed. This is what CLAUDE.md requires: cleanup must survive a failed ASSERT_, never rely on code after the assertions. Not adopted from the review: moving ConnectedPingCounter out of the anonymous namespace. It is already declared above the fixture, so the fixture can name it as is. Verified: full ctest 285/285, plus both ping tests clean under ASAN. --- Tests/Integration/PingTestsTests.cpp | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/Tests/Integration/PingTestsTests.cpp b/Tests/Integration/PingTestsTests.cpp index fc5b8d7fd..21e8b9ad1 100644 --- a/Tests/Integration/PingTestsTests.cpp +++ b/Tests/Integration/PingTestsTests.cpp @@ -121,6 +121,17 @@ class PingTests : public ::testing::Test } DataStructures::List destroyList; + + // Fixture-owned, not a local in the test body. A failed ASSERT_ returns before any cleanup the + // body would have done, and the peers are destroyed later in TearDown() -- Shutdown() makes a + // virtual call into every attached plugin (peer.cpp, OnRakPeerShutdown), and the network thread + // can still deliver OnInternalPacket during the shutdown window. A stack-local plugin is already + // destroyed by then, which ASAN reports as a BUS on a corrupted vtable pointer: one failed + // assertion becomes a crash that hides the original failure. + // + // A fixture member is destroyed after TearDown() returns, so no explicit detach is needed: the + // plugin outlives every peer that could call into it. + ConnectedPingCounter counter; }; TEST_F(PingTests, PingStatisticsAndOccasionalPing) @@ -252,8 +263,6 @@ TEST_F(PingTests, PingStatisticsAndOccasionalPing) // when it is off. TEST_F(PingTests, OccasionalPingIsObservableAsPingTraffic) { - ConnectedPingCounter counter; - RakPeerInterface *receiver = RakPeerInterface::GetInstance(); destroyList.Push(receiver, _FILE_AND_LINE_); receiver->AttachPlugin(&counter); // before Startup: UsesReliabilityLayer() is true @@ -314,6 +323,4 @@ TEST_F(PingTests, OccasionalPingIsObservableAsPingTraffic) EXPECT_GT(withOccasionalPing, withoutOccasionalPing) << "occasional ping made no difference: " << withOccasionalPing << " pings with it enabled vs " << withoutOccasionalPing << " with it disabled"; - - receiver->DetachPlugin(&counter); // before TearDown destroys the peer }