From f4cd6e34e63e8262c39b5883cb7c1d6512f135c8 Mon Sep 17 00:00:00 2001 From: Rye Date: Sun, 1 Nov 2026 07:52:06 -0500 Subject: [PATCH 1/9] LLCircuitData averages a ping's round trip with that ping no longer in flight pingTimerStop called setPingDelay before it lowered mPingsInTransit. setPingDelay starts from getPingDelayAveraged(), which takes getPingInTransitTime() as a floor, and that still counted the ping being answered as in flight: a heartbeat less a second plus its round trip, over 4 s at the 5 s heartbeat, clamped to 2000 ms. So every answer put the average at 2000 ms before moving it a fifth of the way to the round trip, leaving it at 1600 ms plus a fifth of the round trip whatever the link did, and a reliable packet was resent after 8 s plus the round trip where five times the averaged ping was meant. It now counts the pings still in flight, those sent after the one answered, before it averages. From the starting 1000 ms, a 200 ms answer now leaves the average at 840 ms and the resend timeout at 4.2 s, where it left 1640 ms and 8.2 s, and twenty of them leave about 209 ms and 1.05 s. The rest of setPingDelay's rule is as it was: a slower answer raises the average to its round trip at once (1500 ms, where it gave 1900 ms), a faster one moves it a fifth of the way, and it stays within 100-2000 ms. The semi-reliable timeout follows the same average. While a ping is out the average still reads 2000 ms, since getPingInTransitTime counts one in flight as at least 4 s, so a packet sent then still waits 10 s, and the lost-packet timeout, worked just after a ping is sent, stays at its 5 s cap. llcircuit_test builds llcircuit.cpp alone, with the rest of llmessage stubbed, so the circuit runs on the test's own clock. Co-Authored-By: Claude Opus 5.5 --- indra/llmessage/CMakeLists.txt | 4 +- indra/llmessage/llcircuit.cpp | 6 +- indra/llmessage/tests/llcircuit_test.cpp | 271 +++++++++++++++++++++++ 3 files changed, 278 insertions(+), 3 deletions(-) create mode 100644 indra/llmessage/tests/llcircuit_test.cpp diff --git a/indra/llmessage/CMakeLists.txt b/indra/llmessage/CMakeLists.txt index 1fafe5240d..7db102668e 100644 --- a/indra/llmessage/CMakeLists.txt +++ b/indra/llmessage/CMakeLists.txt @@ -170,9 +170,9 @@ target_precompile_headers(llmessage REUSE_FROM llprecompiled) # tests if(AL_BUILD_TESTS) - # These three stand in for parts of llmessage and llcorehttp with stubs of + # These four stand in for parts of llmessage and llcorehttp with stubs of # their own, so they build their one file and do not link llmessage. - foreach(test llcoproceduremanager lltrustedmessageservice lltemplatemessagedispatcher) + foreach(test llcoproceduremanager lltrustedmessageservice lltemplatemessagedispatcher llcircuit) al_add_test_binary( ${test}_test PROJECT llmessage diff --git a/indra/llmessage/llcircuit.cpp b/indra/llmessage/llcircuit.cpp index 2c97f0cf02..d3bf182835 100644 --- a/indra/llmessage/llcircuit.cpp +++ b/indra/llmessage/llcircuit.cpp @@ -1289,9 +1289,13 @@ void LLCircuitData::pingTimerStop(const U8 ping_id) } U32Milliseconds msec = delta_ping*mHeartbeatInterval + time; - setPingDelay(msec); + // Only the pings sent after this one are still in flight. Count them + // before averaging: setPingDelay starts from getPingDelayAveraged(), + // which takes the time in flight of every ping counted as a floor. mPingsInTransit = delta_ping; + setPingDelay(msec); + if (mBlocked && (mPingsInTransit <= PING_RELEASE_BLOCK)) { mBlocked = false; diff --git a/indra/llmessage/tests/llcircuit_test.cpp b/indra/llmessage/tests/llcircuit_test.cpp new file mode 100644 index 0000000000..ba3717f0cc --- /dev/null +++ b/indra/llmessage/tests/llcircuit_test.cpp @@ -0,0 +1,271 @@ +/** + * @file llcircuit_test.cpp + * @brief LLCircuitData's averaged ping and the resend timeout it gives. + * + * $LicenseInfo:firstyear=2026&license=viewerlgpl$ + * Alchemy Viewer Source Code + * Copyright (C) 2026, Rye + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; + * version 2.1 of the License only. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + * $/LicenseInfo$ + */ + +#include "linden_common.h" + +#include "llcircuit.h" + +#include "llpacketbuffer.h" +#include "llpacketring.h" +#include "llpounceable.h" +#include "lltransfermanager.h" +#include "message.h" + +#include "altestmatchers.h" + +#include + +#include + +// The circuit reads its times from the message system's clock, which here is +// the test's own. +namespace +{ + F64Seconds sMessageTime; +} + +F64Seconds LLMessageSystem::getMessageTimeSeconds(const bool) +{ + return sMessageTime; +} + +U64Microseconds LLMessageSystem::getMessageTimeUsecs(const bool) +{ + return sMessageTime; +} + +// The rest of llmessage that llcircuit.cpp calls. A circuit makes its throttle +// group, and as it goes ends its host's transfers and reads whether the +// message system logs verbosely; the rest is reached only by sending, +// resending or acking a packet, which no case does. The message system a +// circuit reads is made with none of the real one's sockets or templates. +LLPounceable gMessageSystem; + +LLMessageSystem::LLMessageSystem(const std::string&, U32, S32, S32, S32, bool, const F32 circuit_heartbeat_interval, + const F32 circuit_timeout) +: mVerboseLog(false), + mCircuitInfo(F32Seconds(circuit_heartbeat_interval), F32Seconds(circuit_timeout)) +{ +} + +LLMessageSystem::~LLMessageSystem() +{ +} + +LLPacketRing::LLPacketRing() +{ +} + +LLPacketRing::~LLPacketRing() +{ +} + +LLPacketBuffer::~LLPacketBuffer() +{ +} + +void LLMessageSystem::newMessageFast(const char*) +{ +} + +void LLMessageSystem::nextBlockFast(const char*) +{ +} + +void LLMessageSystem::addU8Fast(const char*, U8) +{ +} + +void LLMessageSystem::addU32Fast(const char*, U32) +{ +} + +S32 LLMessageSystem::sendMessage(const LLHost&) +{ + return 0; +} + +S32 LLMessageSystem::sendMessage(const LLHost&, LLStoredMessagePtr) +{ + return 0; +} + +bool LLMessageSystem::sendPacketToSocket(const char*, S32, LLHost) +{ + return false; +} + +LLTransferManager gTransferManager; + +LLTransferManager::LLTransferManager() +{ +} + +LLTransferManager::~LLTransferManager() +{ +} + +void LLTransferManager::cleanupConnection(const LLHost&) +{ +} + +LLThrottleGroup::LLThrottleGroup() +{ +} + +bool LLThrottleGroup::checkOverflow(S32, F32) +{ + return false; +} + +bool LLThrottleGroup::throttleOverflow(S32, F32) +{ + return false; +} + +bool LLThrottleGroup::dynamicAdjust() +{ + return false; +} + +LLReliablePacket::LLReliablePacket(S32, U8*, S32, LLReliablePacketParams*) +: mBuffer(nullptr), + mBufferLength(0) +{ +} + +std::string LLHost::getIPandPort() const +{ + return {}; +} + +std::ostream& operator<<(std::ostream& os, const LLHost&) +{ + return os; +} + +char const* const _PREHASH_ID = "ID"; +char const* const _PREHASH_OldestUnacked = "OldestUnacked"; +char const* const _PREHASH_PacketAck = "PacketAck"; +char const* const _PREHASH_Packets = "Packets"; +char const* const _PREHASH_PingID = "PingID"; +char const* const _PREHASH_StartPingCheck = "StartPingCheck"; + +namespace +{ + using ll_test::WithinRounding; + + // The heartbeat and timeout the message system's tests start it with. + const F32Seconds HEARTBEAT(5.f); + const F32Seconds TIMEOUT(100.f); + + // The clock starts at 1024 s, and every ping starts on a whole second + // below 2048 s: there a double rounds a whole second and 0.2 s up, so an + // answer 0.2 s later measures 200 ms rather than truncating to 199. + const F64Seconds CLOCK_START(1024.0); + + // gMessageSystem is the stub while this lives, and what it was before + // once it ends; the clock is at its start. + class StubMessageSystemScope + { + public: + StubMessageSystemScope() + : mPrior(gMessageSystem) + { + static LLMessageSystem system("", 0, 0, 0, 0, false, HEARTBEAT.value(), TIMEOUT.value()); + gMessageSystem = &system; + sMessageTime = CLOCK_START; + } + + ~StubMessageSystemScope() + { + gMessageSystem = mPrior; + } + + StubMessageSystemScope(const StubMessageSystemScope&) = delete; + StubMessageSystemScope& operator=(const StubMessageSystemScope&) = delete; + + private: + LLMessageSystem* mPrior; + }; + + // What the message system waits before resending a reliable packet on + // the circuit, as it works it out when it sends the packet and again + // when it resends it. + F32Seconds resend_timeout(LLCircuitData& circuit) + { + return llmax(LL_MINIMUM_RELIABLE_TIMEOUT_SECONDS, + F32Seconds(LL_RELIABLE_TIMEOUT_FACTOR * circuit.getPingDelayAveraged())); + } + + // A ping sent at `sent` and answered `round_trip` later, with no other + // ping in flight. The circuit numbers a ping only as it sends one itself, + // so the last one sent is still number 0. + void answer_ping(LLCircuitData& circuit, F64Seconds sent, F64Seconds round_trip) + { + sMessageTime = sent; + circuit.pingTimerStart(); + sMessageTime = sent + round_trip; + circuit.pingTimerStop(0); + } + + TEST_CASE("llcircuit: an answered ping moves the averaged ping toward its round trip", "[llcircuit]") + { + StubMessageSystemScope message_system; + LLCircuitData circuit(LLHost(), 0, HEARTBEAT, TIMEOUT); + REQUIRE(circuit.getPingDelayAveraged() == F32Milliseconds(INITIAL_PING_VALUE_MSEC)); + + answer_ping(circuit, CLOCK_START, F64Seconds(0.2)); + CHECK(circuit.getPingsInTransit() == 0); + CHECK(circuit.getPingDelay() == U32Milliseconds(200)); + + // A fifth of the way from 1000 ms to 200 ms, and five times that. + CHECK_THAT(circuit.getPingDelayAveraged().value(), WithinRounding(840.f, 2000.f)); + CHECK_THAT(resend_timeout(circuit).value(), WithinRounding(4.2f, 10.f)); + + // Each answer a heartbeat later moves it a fifth of the rest of the way. + const S32 answers = 20; + for (S32 answer = 1; answer < answers; ++answer) + { + answer_ping(circuit, CLOCK_START + answer * HEARTBEAT, F64Seconds(0.2)); + } + const F32 settled = 200.f + 800.f * std::pow(0.8f, F32(answers)); + CHECK_THAT(circuit.getPingDelayAveraged().value(), WithinRounding(settled, 2000.f)); + CHECK_THAT(resend_timeout(circuit).value(), WithinRounding(5.f * settled / 1000.f, 10.f)); + } + + TEST_CASE("llcircuit: a slow answer raises the averaged ping to its round trip at once", "[llcircuit]") + { + StubMessageSystemScope message_system; + LLCircuitData circuit(LLHost(), 0, HEARTBEAT, TIMEOUT); + + answer_ping(circuit, CLOCK_START, F64Seconds(1.5)); + CHECK_THAT(circuit.getPingDelayAveraged().value(), WithinRounding(1500.f, 2000.f)); + CHECK_THAT(resend_timeout(circuit).value(), WithinRounding(7.5f, 10.f)); + + // and falls back a fifth of the way at the next fast one. + answer_ping(circuit, CLOCK_START + HEARTBEAT, F64Seconds(0.2)); + CHECK_THAT(circuit.getPingDelayAveraged().value(), WithinRounding(1240.f, 2000.f)); + } +} From 475007ebd4d1ef27868ad525509d0b27db292467 Mon Sep 17 00:00:00 2001 From: Rye Date: Sun, 1 Nov 2026 07:59:56 -0500 Subject: [PATCH 2/9] LLCircuitData times waiting acks from the first since the last ones went collectRAck started the collect timer only while mAckCreationTime was 0, and only sendAcks put it back to 0, when it sent the acks in a PacketAck. When the acks went instead on the end of a packet sent to the host, sendMessage took them off the list and left the timer as it was, and sendAcks then dropped the circuit with nothing to send, the timer still set. The next ack to come in was timed from the first of the ones already carried, so it went in a PacketAck of its own at the very next processAcks rather than after AckCollectTime (0.1 s), where a packet sent to the host in the meantime could have carried it. collectRAck now starts the timer whenever an ack comes in to an empty list, however the last ones went, so sendAcks no longer puts it back. llcircuit_test checks that an ack coming in after a packet carried the last ones waits the collect time before it goes in a PacketAck, and that acks gather for the collect time and go in one. It reaches the circuit's acks through ll_test::CircuitProbe, which llcircuit.h befriends, and its stubbed message system now records the acks it is sent. Co-Authored-By: Claude Opus 5.5 --- doc/TESTING.md | 1 + indra/llmessage/llcircuit.cpp | 9 +- indra/llmessage/llcircuit.h | 6 + indra/llmessage/tests/llcircuit_test.cpp | 146 +++++++++++++++++++---- 4 files changed, 133 insertions(+), 29 deletions(-) diff --git a/doc/TESTING.md b/doc/TESTING.md index 71bff29698..8d5afc7d00 100644 --- a/doc/TESTING.md +++ b/doc/TESTING.md @@ -63,6 +63,7 @@ Everything a test file defines goes in an anonymous namespace. A test binary is - `VertexBufferProbe` (`llrender/llfonttextcache.h`); - `RenderProbe` (`llrender/llrender.h`); - `VolumeProbe` (`llmath/llvolume.h`); + - `CircuitProbe` (`llmessage/llcircuit.h`); - `TextViewProbe` (`llui/text/altextview.h` and `altextruler.h`); - `alincludesearch_data` (`alscript/preprocessor/alincludesearch.h`). - **Internal is not enough for LL's name-keyed registries.** `LLSingleton`, `LLInstanceTracker` and `LLThreadLocalSingletonPointer` key on `typeid(T).name()`, which is `(anonymous namespace)::X` for every anonymous `X`. A type used with one of them needs a name unique across the tree. diff --git a/indra/llmessage/llcircuit.cpp b/indra/llmessage/llcircuit.cpp index d3bf182835..c04a4db22e 100644 --- a/indra/llmessage/llcircuit.cpp +++ b/indra/llmessage/llcircuit.cpp @@ -1076,13 +1076,13 @@ bool LLCircuitData::collectRAck(TPACKETID packet_num) { // First extra ack, we need to add ourselves to the list of circuits that need to send acks gMessageSystem->mCircuitInfo.mSendAckMap[mHost] = this; - } - mAcks.push_back(packet_num); - if (mAckCreationTime == 0) - { + // and the acks have waited since now, whether the last ones went in + // a PacketAck or on the end of a packet sent to the host. mAckCreationTime = getAgeInSeconds(); } + + mAcks.push_back(packet_num); return true; } @@ -1136,7 +1136,6 @@ void LLCircuit::sendAcks(F32 collect_time) // empty out the acks list cd->mAcks.clear(); - cd->mAckCreationTime = 0.f; } // remove data map mSendAckMap.erase(cur_it); diff --git a/indra/llmessage/llcircuit.h b/indra/llmessage/llcircuit.h index 1f43e4d272..2d35c8a177 100644 --- a/indra/llmessage/llcircuit.h +++ b/indra/llmessage/llcircuit.h @@ -69,6 +69,11 @@ class LLMessageSystem; class LLEncodedDatagramService; class LLSD; +namespace ll_test +{ + struct CircuitProbe; +} + // // Classes // @@ -160,6 +165,7 @@ class LLCircuitData friend class LLCircuit; friend class LLMessageSystem; friend class LLEncodedDatagramService; + friend struct ll_test::CircuitProbe; friend void crash_on_spaceserver_timeout (const LLHost &host, void *); // HACK, so it has access to setAlive() so it can send a final shutdown message. protected: TPACKETID nextPacketOutID(); diff --git a/indra/llmessage/tests/llcircuit_test.cpp b/indra/llmessage/tests/llcircuit_test.cpp index ba3717f0cc..71349b09f2 100644 --- a/indra/llmessage/tests/llcircuit_test.cpp +++ b/indra/llmessage/tests/llcircuit_test.cpp @@ -1,6 +1,6 @@ /** * @file llcircuit_test.cpp - * @brief LLCircuitData's averaged ping and the resend timeout it gives. + * @brief LLCircuitData's averaged ping, and the acks it collects. * * $LicenseInfo:firstyear=2026&license=viewerlgpl$ * Alchemy Viewer Source Code @@ -37,12 +37,16 @@ #include #include +#include // The circuit reads its times from the message system's clock, which here is -// the test's own. +// the test's own, and sends its acks through the message system, which here +// records the IDs each PacketAck carries. namespace { F64Seconds sMessageTime; + std::vector sAckedIDs; + std::vector sMessageIDs; } F64Seconds LLMessageSystem::getMessageTimeSeconds(const bool) @@ -55,55 +59,59 @@ U64Microseconds LLMessageSystem::getMessageTimeUsecs(const bool) return sMessageTime; } -// The rest of llmessage that llcircuit.cpp calls. A circuit makes its throttle -// group, and as it goes ends its host's transfers and reads whether the -// message system logs verbosely; the rest is reached only by sending, -// resending or acking a packet, which no case does. The message system a -// circuit reads is made with none of the real one's sockets or templates. -LLPounceable gMessageSystem; - -LLMessageSystem::LLMessageSystem(const std::string&, U32, S32, S32, S32, bool, const F32 circuit_heartbeat_interval, - const F32 circuit_timeout) -: mVerboseLog(false), - mCircuitInfo(F32Seconds(circuit_heartbeat_interval), F32Seconds(circuit_timeout)) +void LLMessageSystem::newMessageFast(const char*) { + sMessageIDs.clear(); } -LLMessageSystem::~LLMessageSystem() +void LLMessageSystem::nextBlockFast(const char*) { } -LLPacketRing::LLPacketRing() +void LLMessageSystem::addU8Fast(const char*, U8) { } -LLPacketRing::~LLPacketRing() +void LLMessageSystem::addU32Fast(const char*, U32 u) { + sMessageIDs.push_back(u); } -LLPacketBuffer::~LLPacketBuffer() +S32 LLMessageSystem::sendMessage(const LLHost&) { + sAckedIDs.insert(sAckedIDs.end(), sMessageIDs.begin(), sMessageIDs.end()); + return 0; } -void LLMessageSystem::newMessageFast(const char*) +// The rest of llmessage that llcircuit.cpp calls. A circuit makes its throttle +// group, and as it goes ends its host's transfers and reads whether the +// message system logs verbosely; the rest is reached only by sending, +// resending or acking a reliable packet, which no case does. The message +// system a circuit reads is made with none of the real one's sockets or +// templates. +LLPounceable gMessageSystem; + +LLMessageSystem::LLMessageSystem(const std::string&, U32, S32, S32, S32, bool, const F32 circuit_heartbeat_interval, + const F32 circuit_timeout) +: mVerboseLog(false), + mCircuitInfo(F32Seconds(circuit_heartbeat_interval), F32Seconds(circuit_timeout)) { } -void LLMessageSystem::nextBlockFast(const char*) +LLMessageSystem::~LLMessageSystem() { } -void LLMessageSystem::addU8Fast(const char*, U8) +LLPacketRing::LLPacketRing() { } -void LLMessageSystem::addU32Fast(const char*, U32) +LLPacketRing::~LLPacketRing() { } -S32 LLMessageSystem::sendMessage(const LLHost&) +LLPacketBuffer::~LLPacketBuffer() { - return 0; } S32 LLMessageSystem::sendMessage(const LLHost&, LLStoredMessagePtr) @@ -172,8 +180,30 @@ char const* const _PREHASH_Packets = "Packets"; char const* const _PREHASH_PingID = "PingID"; char const* const _PREHASH_StartPingCheck = "StartPingCheck"; +namespace ll_test +{ + // What the message system does with a circuit's acks as packets come in + // from its host and go out to it. + struct CircuitProbe + { + // A reliable packet came in, to be acked. + static void receiveReliable(LLCircuitData& circuit, TPACKETID id) + { + circuit.collectRAck(id); + } + + // A packet went out with every waiting ack appended to it, taken off + // the circuit's list as LLMessageSystem::sendMessage takes them. + static void carryAcks(LLCircuitData& circuit) + { + circuit.mAcks.erase(circuit.mAcks.begin(), circuit.mAcks.end()); + } + }; +} + namespace { + using ll_test::CircuitProbe; using ll_test::WithinRounding; // The heartbeat and timeout the message system's tests start it with. @@ -185,8 +215,13 @@ namespace // answer 0.2 s later measures 200 ms rather than truncating to 199. const F64Seconds CLOCK_START(1024.0); + // How long the viewer lets acks gather before it sends them in a + // PacketAck of their own (AckCollectTime). + const F32 ACK_COLLECT_TIME = 0.1f; + // gMessageSystem is the stub while this lives, and what it was before - // once it ends; the clock is at its start. + // once it ends, with no circuit left on its lists; the clock is at its + // start. class StubMessageSystemScope { public: @@ -196,10 +231,13 @@ namespace static LLMessageSystem system("", 0, 0, 0, 0, false, HEARTBEAT.value(), TIMEOUT.value()); gMessageSystem = &system; sMessageTime = CLOCK_START; + sAckedIDs.clear(); } ~StubMessageSystemScope() { + gMessageSystem->mCircuitInfo.mSendAckMap.clear(); + gMessageSystem->mCircuitInfo.mUnackedCircuitMap.clear(); gMessageSystem = mPrior; } @@ -230,6 +268,16 @@ namespace circuit.pingTimerStop(0); } + // Waits until the circuit is `seconds` older than it was at `since`, by + // its own age, which is what it times its acks by. + void wait_until_aged(const LLCircuitData& circuit, F64 since, F64 seconds) + { + while (circuit.getAgeInSeconds() - since <= seconds) + { + ms_sleep(5); + } + } + TEST_CASE("llcircuit: an answered ping moves the averaged ping toward its round trip", "[llcircuit]") { StubMessageSystemScope message_system; @@ -268,4 +316,54 @@ namespace answer_ping(circuit, CLOCK_START + HEARTBEAT, F64Seconds(0.2)); CHECK_THAT(circuit.getPingDelayAveraged().value(), WithinRounding(1240.f, 2000.f)); } + + TEST_CASE("llcircuit: an ack after a packet carried the last ones waits the collect time", "[llcircuit]") + { + StubMessageSystemScope message_system; + LLCircuitData circuit(LLHost(), 0, HEARTBEAT, TIMEOUT); + LLCircuit& circuits = gMessageSystem->mCircuitInfo; + + // An ack waits, a packet to the host carries it, and the next frame + // finds the circuit with none to send. + const F64 first_ack = circuit.getAgeInSeconds(); + CircuitProbe::receiveReliable(circuit, 1); + CircuitProbe::carryAcks(circuit); + circuits.sendAcks(ACK_COLLECT_TIME); + CHECK(circuits.mSendAckMap.empty()); + CHECK(sAckedIDs.empty()); + + // Well past the collect time after the first, another ack comes in, + // and waits the collect time from its own arrival + wait_until_aged(circuit, first_ack, 2.0 * ACK_COLLECT_TIME); + const F64 second_ack = circuit.getAgeInSeconds(); + CircuitProbe::receiveReliable(circuit, 2); + circuits.sendAcks(ACK_COLLECT_TIME); + CHECK(sAckedIDs.empty()); + CHECK(circuits.mSendAckMap.size() == 1); + + // before it goes in a PacketAck. + wait_until_aged(circuit, second_ack, ACK_COLLECT_TIME); + circuits.sendAcks(ACK_COLLECT_TIME); + CHECK(sAckedIDs == std::vector{ 2 }); + CHECK(circuits.mSendAckMap.empty()); + } + + TEST_CASE("llcircuit: acks gather for the collect time and go in one PacketAck", "[llcircuit]") + { + StubMessageSystemScope message_system; + LLCircuitData circuit(LLHost(), 0, HEARTBEAT, TIMEOUT); + LLCircuit& circuits = gMessageSystem->mCircuitInfo; + + const F64 first_ack = circuit.getAgeInSeconds(); + CircuitProbe::receiveReliable(circuit, 1); + CircuitProbe::receiveReliable(circuit, 2); + circuits.sendAcks(ACK_COLLECT_TIME); + CHECK(sAckedIDs.empty()); + + wait_until_aged(circuit, first_ack, ACK_COLLECT_TIME); + CircuitProbe::receiveReliable(circuit, 3); + circuits.sendAcks(ACK_COLLECT_TIME); + CHECK(sAckedIDs == std::vector{ 1, 2, 3 }); + CHECK(circuits.mSendAckMap.empty()); + } } From eb0cd7fe7c5750a200cc2786cea10f18a32e1577 Mon Sep 17 00:00:00 2001 From: Rye Date: Sun, 1 Nov 2026 08:04:11 -0500 Subject: [PATCH 3/9] LLMessageSystem fits the acks it appends to a packet to the MTU with their count sendMessage worked out how many acks fit on the end of a packet as (MTUBYTES - length) / 4, the room for their IDs alone, and then wrote the byte that counts them after them. A packet that fell 4n bytes short of MTUBYTES (1200), with n or more acks waiting, took n of them and went out at 1201 bytes. It now leaves room for the count byte. A 1196-byte packet carries no ack, where it carried one and went out at 1201 bytes; a 1195-byte one carries one at 1200 bytes, as before; a 1100-byte one carries 24 at 1197 bytes, where it carried 25 at 1201. message_test sends through a message system built from a template of one message the test writes, with acks put on the circuit through ll_test::CircuitProbe, which llcircuit.h befriends. Co-Authored-By: Claude Opus 5.5 --- indra/llmessage/message.cpp | 7 +- indra/llmessage/tests/message_test.cpp | 131 +++++++++++++++++++++++++ 2 files changed, 135 insertions(+), 3 deletions(-) diff --git a/indra/llmessage/message.cpp b/indra/llmessage/message.cpp index 81d1bbfcac..b840974c32 100644 --- a/indra/llmessage/message.cpp +++ b/indra/llmessage/message.cpp @@ -1651,9 +1651,10 @@ S32 LLMessageSystem::sendMessage(const LLHost &host) mReliablePacketsOut++; } - // tack packet acks onto the end of this message; signed math so an - // over-MTU payload yields a negative count instead of wrapping huge - S32 space_left = (MTUBYTES - (S32)buffer_length) / (S32)sizeof(TPACKETID); // space left for packet ids + // tack packet acks onto the end of this message, and the byte that counts + // them; signed math so an over-MTU payload yields a negative count + // instead of wrapping huge + S32 space_left = (MTUBYTES - (S32)buffer_length - 1) / (S32)sizeof(TPACKETID); // space left for packet ids S32 ack_count = (S32)cdp->mAcks.size(); bool is_ack_appended = false; std::vector acks; diff --git a/indra/llmessage/tests/message_test.cpp b/indra/llmessage/tests/message_test.cpp index c88e29fba1..9d68a1a9f6 100644 --- a/indra/llmessage/tests/message_test.cpp +++ b/indra/llmessage/tests/message_test.cpp @@ -43,6 +43,25 @@ #include #endif +namespace ll_test +{ + // What the message system does with a circuit's acks as packets come in + // from its host. + struct CircuitProbe + { + // A reliable packet came in, to be acked. + static void receiveReliable(LLCircuitData& circuit, TPACKETID id) + { + circuit.collectRAck(id); + } + + static size_t acksWaiting(const LLCircuitData& circuit) + { + return circuit.mAcks.size(); + } + }; +} + namespace { struct Response : public LLHTTPNode::Response @@ -155,6 +174,118 @@ namespace CHECK(socket == -1); } + // A message template file of the test's own, there while this lives. + class TemplateFile + { + public: + TemplateFile(const std::string& path, const char* text) + : mPath(path) + { + llofstream file(mPath.c_str()); + file << text; + } + + ~TemplateFile() + { + if (LLFile::remove(mPath) != 0) + { + LL_WARNS() << "Could not remove " << mPath << LL_ENDL; + } + } + + TemplateFile(const TemplateFile&) = delete; + TemplateFile& operator=(const TemplateFile&) = delete; + + private: + std::string mPath; + }; + + // One message, AckBudgetTest, whose one block holds bytes of any length: + // its packet is the 6-byte header, the 4-byte number of a Low message, + // the 2-byte length and the bytes. + const char* const ACK_BUDGET_TEMPLATE = + "version 2.0\n" + "{\n" + " AckBudgetTest Low 1 NotTrusted Unencoded\n" + " {\n" + " Payload Single\n" + " { Data Variable 2 }\n" + " }\n" + "}\n"; + + // gMessageSystem is `system` while this lives, and what it was before + // once it ends. + class UseMessageSystem + { + public: + explicit UseMessageSystem(LLMessageSystem& system) + : mPrior(gMessageSystem) + { + gMessageSystem = &system; + } + + ~UseMessageSystem() + { + gMessageSystem = mPrior; + } + + UseMessageSystem(const UseMessageSystem&) = delete; + UseMessageSystem& operator=(const UseMessageSystem&) = delete; + + private: + LLMessageSystem* mPrior; + }; + + TEST_CASE_METHOD(LLMessageSystemTestData, "LLMessageSystem: acks appended to a packet fit in the MTU with their count", "[LLMessageSystem]") + { + const std::string template_path = mTestConfigDir + mSep + "message_template.msg"; + const TemplateFile message_template(template_path, ACK_BUDGET_TEMPLATE); + LLMessageSystem system(template_path, NET_USE_OS_ASSIGNED_PORT, 1, 0, 0, false, 5.f, 100.f); + REQUIRE(system.isOK()); + const UseMessageSystem use(system); + + // Packets go to the system's own socket, which reads none of them. + const LLHost host("127.0.0.1", system.getListenPort()); + system.enableCircuit(host, false); + LLCircuitData* circuit = system.mCircuitInfo.findCircuit(host); + REQUIRE(circuit != nullptr); + + // Sends AckBudgetTest holding `data_size` bytes, and says how long + // its packet was. + auto send = [&](S32 data_size) + { + const std::vector data(data_size, 0x5a); + system.newMessage("AckBudgetTest"); + system.nextBlock("Payload"); + system.addBinaryData("Data", data.data(), data_size); + return system.sendMessage(host); + }; + + UNSCOPED_INFO("the message alone"); + CHECK(send(1184) == 1196); + + for (TPACKETID id = 1; id <= 30; ++id) + { + ll_test::CircuitProbe::receiveReliable(*circuit, id); + } + + { + INFO("4 bytes to the MTU hold an ack but not the count after it"); + CHECK(send(1184) == 1196); + CHECK(ll_test::CircuitProbe::acksWaiting(*circuit) == 30); + } + { + INFO("5 bytes hold one and its count"); + CHECK(send(1183) == MTUBYTES); + CHECK(ll_test::CircuitProbe::acksWaiting(*circuit) == 29); + } + { + INFO("100 bytes hold 24 and their count"); + CHECK(send(1088) == 1100 + 24 * 4 + 1); + CHECK(ll_test::CircuitProbe::acksWaiting(*circuit) == 5); + } + } + #if !LL_WINDOWS TEST_CASE_METHOD(LLMessageSystemTestData, "LLMessageSystem: start_net closes its socket when it finds no port to bind", "[LLMessageSystem]") // start_net closes the socket it made when it finds no port to bind From 6bb0ddd97e5ccdb3a862dd33e4d07e68bd852c47 Mon Sep 17 00:00:00 2001 From: Rye Date: Sun, 1 Nov 2026 08:08:11 -0500 Subject: [PATCH 4/9] LLZeroCode::decode fits a final run of zeros that ends at the capacity decode checked a run against the capacity as where it stood plus the run's count, after it had already written the run's first zero, though the run writes only the count less one more. So a run that would end exactly at the capacity was refused as an overflow, while plain bytes could fill the buffer: a packet that decoded to exactly 8192 bytes, the message system's receive buffer, and ended in zeros was dropped. It now checks the count less one, so such a run fits and a run a byte longer is still refused. The comments that describe the scheme are corrected. llzerocode.h gave 0x00 0x00 N as 255 + N zeros and N as up to 254; it is 256 + N, each further 0x00 before the count adding 256, and encode() writes counts up to 255. message.h gave 0x00 N as N + 1 zeros and a lone zero byte as 0x00 0x00 0x00; it is N zeros, and a lone zero is 0x00 0x01. A test's comment that gave each extra marker as 255 zeros now says 256. Co-Authored-By: Claude Opus 5.5 --- indra/llmessage/llzerocode.h | 16 ++++--- indra/llmessage/message.h | 9 ++-- indra/llmessage/tests/llzerocode_test.cpp | 54 ++++++++++++++++++++++- 3 files changed, 67 insertions(+), 12 deletions(-) diff --git a/indra/llmessage/llzerocode.h b/indra/llmessage/llzerocode.h index ac014dc550..53a886ed42 100644 --- a/indra/llmessage/llzerocode.h +++ b/indra/llmessage/llzerocode.h @@ -35,12 +35,12 @@ // first header_size bytes of the buffer (the packet header - flags, // sequence number, offset, etc.) untouched aside from the flag bit below. // -// Runs of zero bytes in the body are replaced by a two-byte token: -// 0x00 N - represents N zero bytes, for N in 1..254 -// 0x00 0x00 N - represents (255 + N) zero bytes (wrap/overflow case, -// produced by decode()'s wire format but never emitted -// by encode(), which instead starts a fresh 0x00 token -// every 255 zero bytes) +// Runs of zero bytes in the body are replaced by a 0x00 byte and a count: +// 0x00 N - represents N zero bytes, for N in 1..255; encode() +// starts a fresh token every 255 zero bytes +// 0x00 0x00 N - represents (256 + N) zero bytes, each further 0x00 +// before the count adding 256 (read by decode(), never +// written by encode()) namespace LLZeroCode { // High bit of the first header byte: set by encode() and cleared by @@ -191,7 +191,9 @@ namespace LLZeroCode } else { - if ((U32)(outptr - dst) + (*inptr) > dst_capacity) + // The run's first zero is already written, so it has + // the count less one to go. + if ((U32)(outptr - dst) + (*inptr) - 1 > dst_capacity) { // Discard rather than decode on into a scrambled // buffer, the same as the two cases above. diff --git a/indra/llmessage/message.h b/indra/llmessage/message.h index 3b3b85aaae..5a927d5c6a 100644 --- a/indra/llmessage/message.h +++ b/indra/llmessage/message.h @@ -157,10 +157,11 @@ class LLMessageStringTable : public LLSingleton // // Zero-coding (applied when LL_ZERO_CODE_FLAG is set): // -// Runs of zero bytes in the body are replaced by a two-byte token: -// 0x00 N – represents (N + 1) zero bytes, for N in 1..254 -// 0x00 0x00 N – represents (256 + N) zero bytes (wrap/overflow case) -// A literal 0x00 byte that starts no run is encoded as 0x00 0x00 0x00. +// Runs of zero bytes in the body are replaced by a 0x00 byte and a count: +// 0x00 N – represents N zero bytes, for N in 1..255 +// 0x00 0x00 N – represents (256 + N) zero bytes, each further 0x00 +// before the count adding 256 (read, never written) +// A lone zero byte is a run of one, 0x00 0x01. // The six-byte packet header is excluded from zero-coding and is always // transmitted as-is. diff --git a/indra/llmessage/tests/llzerocode_test.cpp b/indra/llmessage/tests/llzerocode_test.cpp index 4ec13cf8f5..d3bc749a30 100644 --- a/indra/llmessage/tests/llzerocode_test.cpp +++ b/indra/llmessage/tests/llzerocode_test.cpp @@ -28,6 +28,8 @@ #include "../llzerocode.h" +#include "message.h" + #include #include @@ -143,7 +145,7 @@ namespace } // decode() must also accept the "0x00 0x00 N" doubled-marker wire format - // (worth +255 zero bytes per extra marker) for compatibility with any + // (worth +256 zero bytes per extra marker) for compatibility with any // encoder other than this one's chunking strategy, even though encode() // itself never emits it. TEST_CASE("LLZeroCode: decode accepts the doubled-marker wrap form", "[LLZeroCode]") @@ -225,6 +227,56 @@ namespace CHECK(dec_size == 0u); } + // Decodes `encoded`, whose header is 6 bytes, into a destination of + // `capacity` bytes, and says how many it wrote: 0 where it overflowed. + U32 decoded_size_within(const std::vector& encoded, U32 capacity, bool& overflow) + { + std::vector dec(capacity, 0xDD); + return LLZeroCode::decode(encoded.data(), (U32)encoded.size(), dec.data(), capacity, 6, overflow); + } + + // A run, as plain bytes may, can end exactly at the destination's + // capacity, and not a byte past it. + TEST_CASE("LLZeroCode: decode fits a final run that ends at the capacity", "[LLZeroCode]") + { + bool overflow = false; + { + INFO("4 plain bytes and a run of 6 after the header: 16 bytes"); + const std::vector encoded = makeBuffer(6, LLZeroCode::FLAG, {1, 2, 3, 4, 0x00, 6}); + CHECK(decoded_size_within(encoded, 16, overflow) == 16u); + CHECK_FALSE(overflow); + CHECK(decoded_size_within(encoded, 15, overflow) == 0u); + CHECK(overflow); + } + { + INFO("a run of 256 + 4 after the header: 266 bytes"); + const std::vector encoded = makeBuffer(6, LLZeroCode::FLAG, {0x00, 0x00, 4}); + CHECK(decoded_size_within(encoded, 266, overflow) == 266u); + CHECK_FALSE(overflow); + CHECK(decoded_size_within(encoded, 265, overflow) == 0u); + CHECK(overflow); + } + } + + // The message system decodes into a buffer of MAX_BUFFER_SIZE bytes, and + // a packet that fills it and ends in zeros decodes whole. + TEST_CASE("LLZeroCode: a packet that fills the receive buffer and ends in zeros round-trips", "[LLZeroCode]") + { + std::vector body(MAX_BUFFER_SIZE - LL_PACKET_ID_SIZE, 0); + body[0] = 1; + const std::vector src = makeBuffer(LL_PACKET_ID_SIZE, 0x00, body); + std::vector enc(2 * src.size(), 0); + const S32 enc_size = LLZeroCode::encode(src.data(), (U32)src.size(), enc.data(), (U32)enc.size(), LL_PACKET_ID_SIZE); + REQUIRE(enc_size > 0); + + std::vector dec(MAX_BUFFER_SIZE, 0xDD); + bool overflow = false; + const U32 dec_size = LLZeroCode::decode(enc.data(), (U32)enc_size, dec.data(), (U32)dec.size(), LL_PACKET_ID_SIZE, overflow); + CHECK_FALSE(overflow); + REQUIRE(dec_size == (U32)MAX_BUFFER_SIZE); + CHECK(memcmp(dec.data(), src.data(), src.size()) == 0); + } + // A pathological alternating zero/non-zero pattern grows under zero-coding // (every isolated zero costs 2 output bytes for 1 input byte), so encode // must refuse it and leave the original buffer in use. From a9fbfbe0ae45987ad3b3e6d39d7e9cfa6b0f6344 Mon Sep 17 00:00:00 2001 From: Rye Date: Sun, 1 Nov 2026 08:13:36 -0500 Subject: [PATCH 5/9] LLMessageSystem::disableCircuit removes the circuit whether or not it has a code disableCircuit removed a circuit only when its host had a circuit code mapped, which only the UseCircuitCode handler fills, on the side that receives UseCircuitCode. The viewer opens every circuit to a simulator without a code, so on the viewer it removed nothing and logged that it could not find one: the circuit stayed up, pinging, acking and answering pings, until the watchdog dropped it once the simulator had answered no ping for 100 s. It now removes the circuit in either case, and where a code is mapped clears it as before. What each caller now does: - CloseCircuit (close_circuit) removes the sender's circuit. The receive path already finds the circuit again after the handler, since any message may remove it. When the sender is the agent's region, LLViewerRegion::updateNetStats finds no circuit, the region is no longer alive, and LLAppViewer disconnects with AgentLostConnection, as it does after the watchdog drops the circuit, now at once rather than 100 s later. A neighbour's region is left not alive, and a later EnableSimulator opens a new circuit and LLWorld::addRegion makes the region again. - DisableSimulator (process_disable_simulator) removes the region and now its circuit. A later EnableSimulator for that host opens a new circuit and region, as it did after the watchdog. For the agent's own region removeRegion has already disconnected (YouHaveBeenDisconnected); the lost circuit then asks forceDisconnect again, which does nothing a second time. - A region's retirement (ALRegionRetirement::finish) now closes the circuit as its comment says it does: what that simulator still sends is dropped as off-circuit, including its own DisableSimulator, and the region comes back only through a new EnableSimulator. - The message log's Close Circuit removed the circuit already, through removeCircuitData where no code was mapped; it does the same. - LLXferManager's kill of an untrusted circuit with too many file requests now removes it on the viewer too. - UseCircuitCode, where a code is mapped, is unchanged. Where a host came back with a new code on the same address, the handler had just erased the address's mapping, so the old circuit stayed and was taken on again; it is now removed and opened afresh. message_test checks that a circuit opened without a code is removed and can be opened again, and that one with a code is removed with its mapping. Co-Authored-By: Claude Opus 5.5 --- indra/llmessage/message.cpp | 11 ++----- indra/llmessage/tests/message_test.cpp | 40 ++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 8 deletions(-) diff --git a/indra/llmessage/message.cpp b/indra/llmessage/message.cpp index b840974c32..fe6801adc6 100644 --- a/indra/llmessage/message.cpp +++ b/indra/llmessage/message.cpp @@ -2011,16 +2011,11 @@ void LLMessageSystem::disableCircuit(const LLHost &host) LL_INFOS("Messaging") << "Host " << LLHost(old_ip, old_port) << " circuit " << code << " removed from lookup table" << LL_ENDL; gMessageSystem->mIPPortToCircuitCode.erase(ip_port); } - mCircuitInfo.removeCircuitData(host); - } - else - { - // Sigh, since we can open circuits which don't have circuit - // codes, it's possible for this to happen... - - LL_WARNS("Messaging") << "Couldn't find circuit code for " << host << LL_ENDL; } + // A circuit opened without a code, as the viewer opens every circuit to + // a simulator, goes too. + mCircuitInfo.removeCircuitData(host); } diff --git a/indra/llmessage/tests/message_test.cpp b/indra/llmessage/tests/message_test.cpp index 9d68a1a9f6..1309c89bbf 100644 --- a/indra/llmessage/tests/message_test.cpp +++ b/indra/llmessage/tests/message_test.cpp @@ -286,6 +286,46 @@ namespace } } + // The viewer opens every circuit to a simulator with no circuit code, as + // EnableSimulator does, and CloseCircuit, DisableSimulator and a region's + // retirement disable it. + TEST_CASE_METHOD(LLMessageSystemTestData, "LLMessageSystem: disableCircuit removes a circuit opened without a circuit code", "[LLMessageSystem]") + { + const LLHost host("127.0.0.1", 13000); + gMessageSystem->enableCircuit(host, true); + REQUIRE(gMessageSystem->mCircuitInfo.findCircuit(host) != nullptr); + REQUIRE(gMessageSystem->findCircuitCode(host) == 0); + + gMessageSystem->disableCircuit(host); + CHECK(gMessageSystem->mCircuitInfo.findCircuit(host) == nullptr); + + // A later EnableSimulator opens it again. + gMessageSystem->enableCircuit(host, true); + const LLCircuitData* reopened = gMessageSystem->mCircuitInfo.findCircuit(host); + REQUIRE(reopened != nullptr); + CHECK(reopened->isAlive()); + CHECK(reopened->getTrusted()); + } + + TEST_CASE_METHOD(LLMessageSystemTestData, "LLMessageSystem: disableCircuit removes a circuit and the circuit code it has", "[LLMessageSystem]") + { + const LLHost host("127.0.0.1", 13001); + const U32 code = 1234; + const U64 ip_port = ((U64)host.getAddress() << 32) | host.getPort(); + LLUUID session_id; + session_id.generate(); + REQUIRE(gMessageSystem->addCircuitCode(code, session_id)); + gMessageSystem->mIPPortToCircuitCode[ip_port] = code; + gMessageSystem->mCircuitCodeToIPPort[code] = ip_port; + gMessageSystem->enableCircuit(host, false); + REQUIRE(gMessageSystem->findCircuitCode(host) == code); + + gMessageSystem->disableCircuit(host); + CHECK(gMessageSystem->mCircuitInfo.findCircuit(host) == nullptr); + CHECK(gMessageSystem->findCircuitCode(host) == 0); + CHECK(gMessageSystem->mCircuitCodeToIPPort.count(code) == 0); + } + #if !LL_WINDOWS TEST_CASE_METHOD(LLMessageSystemTestData, "LLMessageSystem: start_net closes its socket when it finds no port to bind", "[LLMessageSystem]") // start_net closes the socket it made when it finds no port to bind From a30ca40506c3da9b203828a950b7629564536d90 Mon Sep 17 00:00:00 2001 From: Rye Date: Sun, 1 Nov 2026 08:25:41 -0500 Subject: [PATCH 6/9] LLMessageSystem drops a reliable packet it has handled, marked resent or not checkMessages looked a packet up in its circuit's record of reliable packets already handled only when the packet was marked resent. A reliable packet the network delivered twice came the second time unmarked, so its handler ran again, and the sequence check on arrival counted it in again and logged it as out of order. Every reliable packet is now looked up: a duplicate is dropped and acked again, as a resent one was, and the arrival check skips it too. An unreliable packet is not looked up: only reliable packets' numbers are recorded, so a match could only be a stale entry. A fresh packet does not match an entry. A sender numbers each packet it sends on a circuit one past the last, and the record holds only numbers already handled, so a fresh packet is numbered above all of them. At each StartPingCheck the record drops every number below the oldest packet the sender is still waiting to have acked, and after 60 s any number above the highest seen, which only the 24-bit numbering wrapping leaves, so no entry is left when the numbering comes round to it again. The exception is a sender that starts its numbering over on a circuit the receiver still holds, which leaves the entries near the old highest number until the new numbers reach them. Now that disableCircuit removes the circuit, on the viewer that takes a simulator opening a new circuit within the watchdog's 100 s after dropping its end of the old one with no CloseCircuit or DisableSimulator reaching the viewer. message_test sends a reliable packet twice, unmarked, from a socket of its own to a message system built from a template the test writes, both before either is read and after the first is handled, and checks it is handled once each time; a copy marked resent is still dropped. Co-Authored-By: Claude Opus 5.5 --- indra/llmessage/message.cpp | 74 ++++++++--------- indra/llmessage/tests/message_test.cpp | 107 +++++++++++++++++++++++++ 2 files changed, 140 insertions(+), 41 deletions(-) diff --git a/indra/llmessage/message.cpp b/indra/llmessage/message.cpp index fe6801adc6..a7f6b057bf 100644 --- a/indra/llmessage/message.cpp +++ b/indra/llmessage/message.cpp @@ -639,46 +639,36 @@ bool LLMessageSystem::checkMessages(LockMessageChecker&, S64 frame_count, if (buffer[0] & LL_RESENT_FLAG) { recv_resent = true; - if (cdp && cdp->isDuplicateResend(mCurrentRecvPacketID)) + } + + // A reliable packet the network delivers twice comes the second + // time without the resent flag, and is as much a duplicate as a + // resend. Only reliable packets' numbers are recorded, so an + // unreliable one is not looked up. + if (recv_reliable && cdp && cdp->isDuplicateResend(mCurrentRecvPacketID)) + { + // We need to ACK here to suppress + // further resends of packets we've + // already seen. + cdp->collectRAck(mCurrentRecvPacketID); + + LL_DEBUGS("Messaging") << "Discarding duplicate from " << host << LL_ENDL; + if(mVerboseLog) { - // We need to ACK here to suppress - // further resends of packets we've - // already seen. - if (recv_reliable) - { - //mAckList.addData(new LLPacketAck(host, mCurrentRecvPacketID)); - // *************************************** - // TESTING CODE - //if(mCircuitInfo.mCurrentCircuit->mHost != host) - //{ - // LL_WARNS("Messaging") << "DISCARDED PACKET HOST MISMATCH! HOST: " - // << host << " CIRCUIT: " - // << mCircuitInfo.mCurrentCircuit->mHost - // << LL_ENDL; - //} - // *************************************** - //mCircuitInfo.mCurrentCircuit->mAcks.put(mCurrentRecvPacketID); - cdp->collectRAck(mCurrentRecvPacketID); - } - - LL_DEBUGS("Messaging") << "Discarding duplicate resend from " << host << LL_ENDL; - if(mVerboseLog) - { - std::ostringstream str; - str << "MSG: <- " << host; - std::string tbuf; - tbuf = llformat( "\t%6d\t%6d\t%6d ", receive_size, (mIncomingCompressedSize ? mIncomingCompressedSize : receive_size), mCurrentRecvPacketID); - str << tbuf << "(unknown)" - << (recv_reliable ? " reliable" : "") - << " resent " - << ((num_acks > 0) ? "acks" : "") - << " DISCARD DUPLICATE"; - LL_INFOS("Messaging") << str.str() << LL_ENDL; - } - mPacketsIn++; - valid_packet = false; - continue; + std::ostringstream str; + str << "MSG: <- " << host; + std::string tbuf; + tbuf = llformat( "\t%6d\t%6d\t%6d ", receive_size, (mIncomingCompressedSize ? mIncomingCompressedSize : receive_size), mCurrentRecvPacketID); + str << tbuf << "(unknown)" + << " reliable" + << (recv_resent ? " resent " : " ") + << ((num_acks > 0) ? "acks" : "") + << " DISCARD DUPLICATE"; + LL_INFOS("Messaging") << str.str() << LL_ENDL; } + mPacketsIn++; + valid_packet = false; + continue; } // UseCircuitCode can be a valid, off-circuit packet. @@ -1168,10 +1158,12 @@ S32 LLMessageSystem::bufferInboundPacket() // Check packet sequencing here, in true socket-arrival order, before // this packet is sorted into the high/low priority inbound queue. - // Skip genuine duplicate resends, same as checkMessages()/logValidMsg() - // would do further downstream. + // Skip duplicates of reliable packets already handled, marked resent + // or not, same as checkMessages()/logValidMsg() would do further + // downstream. bool recv_resent = (data[0] & LL_RESENT_FLAG) != 0; - if (!recv_resent || !cdp->isDuplicateResend(recv_packet_id)) + bool recv_reliable = (data[0] & LL_RELIABLE_FLAG) != 0; + if (!recv_reliable || !cdp->isDuplicateResend(recv_packet_id)) { cdp->checkPacketInID(recv_packet_id, recv_resent); pkt.setPacketIDChecked(true); diff --git a/indra/llmessage/tests/message_test.cpp b/indra/llmessage/tests/message_test.cpp index 1309c89bbf..ac0f2dca20 100644 --- a/indra/llmessage/tests/message_test.cpp +++ b/indra/llmessage/tests/message_test.cpp @@ -372,5 +372,112 @@ namespace CHECK(after == before); } #endif + + // One message, DuplicateTest, whose one block holds a U32. + const char* const DUPLICATE_TEST_TEMPLATE = + "version 2.0\n" + "{\n" + " DuplicateTest Low 1 NotTrusted Unencoded\n" + " {\n" + " Payload Single\n" + " { Data U32 }\n" + " }\n" + "}\n"; + + // A socket of the test's own, closed when this ends. + class SenderSocket + { + public: + SenderSocket() + { + mPort = NET_USE_OS_ASSIGNED_PORT; + mResult = start_net(mSocket, mPort); + } + + ~SenderSocket() + { + end_net(mSocket); + } + + SenderSocket(const SenderSocket&) = delete; + SenderSocket& operator=(const SenderSocket&) = delete; + + S32 mSocket = -1; + int mPort = 0; + S32 mResult = -1; + }; + + S32 sDuplicateTestsHandled = 0; + + void handle_duplicate_test(LLMessageSystem*, void**) + { + ++sDuplicateTestsHandled; + } + + // Reads what has come in until the system has taken `packets` packets + // in all, or a second has passed. + void read_until(LLMessageSystem& system, U32 packets) + { + LockMessageChecker lmc(&system); + const F64 give_up = LLTimer::getTotalSeconds() + 1.0; + while (system.mPacketsIn < packets && LLTimer::getTotalSeconds() < give_up) + { + if (!lmc.checkMessages()) + { + ms_sleep(1); + } + } + } + + TEST_CASE_METHOD(LLMessageSystemTestData, "LLMessageSystem: a reliable packet that arrives twice unmarked is handled once", "[LLMessageSystem]") + { + const std::string template_path = mTestConfigDir + mSep + "message_template.msg"; + const TemplateFile message_template(template_path, DUPLICATE_TEST_TEMPLATE); + LLMessageSystem system(template_path, NET_USE_OS_ASSIGNED_PORT, 1, 0, 0, false, 5.f, 100.f); + REQUIRE(system.isOK()); + const UseMessageSystem use(system); + system.setHandlerFunc("DuplicateTest", handle_duplicate_test); + sDuplicateTestsHandled = 0; + + // The packets come from a socket of the test's own, on a circuit the + // system has to it, as it has to a simulator. + const SenderSocket sender; + REQUIRE(sender.mResult == 0); + system.enableCircuit(LLHost("127.0.0.1", sender.mPort), true); + const LLHost receiver("127.0.0.1", system.getListenPort()); + + // DuplicateTest numbered `id`: the flags, the packet number, no extra + // header, the number of Low message 1 and its U32. + auto send = [&](U8 flags, U8 id) + { + const U8 packet[] = { flags, 0, 0, 0, id, 0, 0xff, 0xff, 0x00, 0x01, 1, 2, 3, 4 }; + REQUIRE(send_packet(sender.mSocket, (const char*)packet, (int)sizeof(packet), receiver.getAddress(), receiver.getPort())); + }; + + { + INFO("both copies in before either is read"); + send(LL_RELIABLE_FLAG, 1); + send(LL_RELIABLE_FLAG, 1); + read_until(system, 2); + REQUIRE(system.mPacketsIn == 2); + CHECK(sDuplicateTestsHandled == 1); + } + { + INFO("the second copy in after the first is handled"); + send(LL_RELIABLE_FLAG, 2); + read_until(system, 3); + send(LL_RELIABLE_FLAG, 2); + read_until(system, 4); + REQUIRE(system.mPacketsIn == 4); + CHECK(sDuplicateTestsHandled == 2); + } + { + INFO("a copy marked resent, as before"); + send(LL_RELIABLE_FLAG | LL_RESENT_FLAG, 2); + read_until(system, 5); + REQUIRE(system.mPacketsIn == 5); + CHECK(sDuplicateTestsHandled == 2); + } + } } From d0f7aab185089da37b267958349800b57b62b749 Mon Sep 17 00:00:00 2001 From: Rye Date: Sun, 1 Nov 2026 08:44:44 -0500 Subject: [PATCH 7/9] LLCircuitData counts an unmarked packet a duplicate only of a number handled moments ago The duplicate check now looks up every reliable packet, but the record of numbers already handled outlives a sender that starts its numbering over on a circuit the receiver keeps up: the old numbers near the last highest one stay until the new numbering reaches them, and the fresh packets that carry them would be dropped and acked, their messages lost. Before that change only resent packets could meet them. isDuplicateResend is now isDuplicate, which takes whether the packet was marked resent. A resend still matches any number the record holds. An unmarked packet matches only a number handled within the last 5 s: a copy the network makes comes within moments of the first, while a sender's numbering takes far longer to come round to an old number, so an older match is a fresh packet. llcircuit_test checks that an unmarked packet matches a number handled 4 s before and not 10 s before, and that a resend matches both. Co-Authored-By: Claude Opus 5.5 --- indra/llmessage/llcircuit.cpp | 20 ++++++++++++-- indra/llmessage/llcircuit.h | 5 +++- indra/llmessage/message.cpp | 4 +-- indra/llmessage/tests/llcircuit_test.cpp | 34 ++++++++++++++++++++++++ 4 files changed, 58 insertions(+), 5 deletions(-) diff --git a/indra/llmessage/llcircuit.cpp b/indra/llmessage/llcircuit.cpp index c04a4db22e..90cacae253 100644 --- a/indra/llmessage/llcircuit.cpp +++ b/indra/llmessage/llcircuit.cpp @@ -63,6 +63,7 @@ const S32 PING_RELEASE_BLOCK = 2; // How many pings behind we have to be to co const F32Seconds TARGET_PERIOD_LENGTH(5.f); const F32Seconds LL_DUPLICATE_SUPPRESSION_TIMEOUT(60.f); //this can be long, as time-based cleanup is // only done when wrapping packetids, now... +const F32Seconds LL_DUPLICATE_COPY_WINDOW(5.f); // how long after a reliable packet is handled an unmarked copy of it counts as a duplicate LLCircuitData::LLCircuitData(const LLHost &host, TPACKETID in_id, const F32Seconds circuit_heartbeat_interval, const F32Seconds circuit_timeout) @@ -598,9 +599,24 @@ void LLCircuit::resendUnackedPackets(S32& unacked_list_length, S32& unacked_list } -bool LLCircuitData::isDuplicateResend(TPACKETID packetnum) +bool LLCircuitData::isDuplicate(TPACKETID packetnum, bool resent) { - return (mRecentlyReceivedReliablePackets.find(packetnum) != mRecentlyReceivedReliablePackets.end()); + packet_time_map::const_iterator it = mRecentlyReceivedReliablePackets.find(packetnum); + if (it == mRecentlyReceivedReliablePackets.end()) + { + return false; + } + if (resent) + { + return true; + } + + // A copy the network made of a packet comes within moments of the first, + // unmarked. An older number an unmarked packet meets is left from + // numbering the sender has since started over, on a circuit kept up. + U64Microseconds delta_t_usec = LLMessageSystem::getMessageTimeUsecs() - it->second; + F64Seconds delta_t_sec = delta_t_usec; + return delta_t_sec < LL_DUPLICATE_COPY_WINDOW; } diff --git a/indra/llmessage/llcircuit.h b/indra/llmessage/llcircuit.h index 2d35c8a177..ae1ffcf81e 100644 --- a/indra/llmessage/llcircuit.h +++ b/indra/llmessage/llcircuit.h @@ -182,7 +182,10 @@ class LLCircuitData bool updateWatchDogTimers(LLMessageSystem *msgsys); // Return false if the circuit is dead and should be cleaned up void addReliablePacket(S32 mSocket, U8 *buf_ptr, S32 buf_len, LLReliablePacketParams *params); - bool isDuplicateResend(TPACKETID packetnum); + // Whether a reliable packet numbered packetnum was handled already: any + // number the record holds for a resent packet, and for an unmarked one + // only a number handled moments ago. + bool isDuplicate(TPACKETID packetnum, bool resent); // Call this method when a reliable message comes in - this will // correctly place the packet in the correct list to be acked // later. RAack = requested ack diff --git a/indra/llmessage/message.cpp b/indra/llmessage/message.cpp index a7f6b057bf..dfc339de7f 100644 --- a/indra/llmessage/message.cpp +++ b/indra/llmessage/message.cpp @@ -645,7 +645,7 @@ bool LLMessageSystem::checkMessages(LockMessageChecker&, S64 frame_count, // time without the resent flag, and is as much a duplicate as a // resend. Only reliable packets' numbers are recorded, so an // unreliable one is not looked up. - if (recv_reliable && cdp && cdp->isDuplicateResend(mCurrentRecvPacketID)) + if (recv_reliable && cdp && cdp->isDuplicate(mCurrentRecvPacketID, recv_resent)) { // We need to ACK here to suppress // further resends of packets we've @@ -1163,7 +1163,7 @@ S32 LLMessageSystem::bufferInboundPacket() // downstream. bool recv_resent = (data[0] & LL_RESENT_FLAG) != 0; bool recv_reliable = (data[0] & LL_RELIABLE_FLAG) != 0; - if (!recv_reliable || !cdp->isDuplicateResend(recv_packet_id)) + if (!recv_reliable || !cdp->isDuplicate(recv_packet_id, recv_resent)) { cdp->checkPacketInID(recv_packet_id, recv_resent); pkt.setPacketIDChecked(true); diff --git a/indra/llmessage/tests/llcircuit_test.cpp b/indra/llmessage/tests/llcircuit_test.cpp index 71349b09f2..287eeb850e 100644 --- a/indra/llmessage/tests/llcircuit_test.cpp +++ b/indra/llmessage/tests/llcircuit_test.cpp @@ -198,6 +198,18 @@ namespace ll_test { circuit.mAcks.erase(circuit.mAcks.begin(), circuit.mAcks.end()); } + + // A reliable packet was handled, its number recorded as + // LLMessageSystem::checkMessages records it. + static void handled(LLCircuitData& circuit, TPACKETID id) + { + circuit.mRecentlyReceivedReliablePackets[id] = LLMessageSystem::getMessageTimeUsecs(); + } + + static bool isDuplicate(LLCircuitData& circuit, TPACKETID id, bool resent) + { + return circuit.isDuplicate(id, resent); + } }; } @@ -366,4 +378,26 @@ namespace CHECK(sAckedIDs == std::vector{ 1, 2, 3 }); CHECK(circuits.mSendAckMap.empty()); } + + TEST_CASE("llcircuit: an unmarked packet is a duplicate only of a number handled moments ago", "[llcircuit]") + { + StubMessageSystemScope message_system; + LLCircuitData circuit(LLHost(), 0, HEARTBEAT, TIMEOUT); + CircuitProbe::handled(circuit, 7); + + // A copy the network made comes unmarked, a resend marked. + CHECK(CircuitProbe::isDuplicate(circuit, 7, false)); + CHECK(CircuitProbe::isDuplicate(circuit, 7, true)); + CHECK_FALSE(CircuitProbe::isDuplicate(circuit, 8, false)); + CHECK_FALSE(CircuitProbe::isDuplicate(circuit, 8, true)); + + sMessageTime += F64Seconds(4.0); + CHECK(CircuitProbe::isDuplicate(circuit, 7, false)); + + // Long after, an unmarked packet with that number is a fresh one from + // numbering the sender started over; a resend still matches. + sMessageTime += F64Seconds(6.0); + CHECK_FALSE(CircuitProbe::isDuplicate(circuit, 7, false)); + CHECK(CircuitProbe::isDuplicate(circuit, 7, true)); + } } From 3d37e9c2a0402003fc70d8d87ceffb8b545b790d Mon Sep 17 00:00:00 2001 From: Rye Date: Sun, 1 Nov 2026 10:13:33 -0500 Subject: [PATCH 8/9] LLCircuitData keeps a number a ping passes until no copy of it can still come Each StartPingCheck carries the oldest packet its sender still waits to have acked, and clearDuplicateList dropped every handled number below it at once. The sender pings once it has the ack, but a copy of the packet can still be on its way: a resend sent before the ack reached the sender, or a copy the network made and delayed. Arriving after the ping, it found its number gone and was handled a second time. clearDuplicateList now drops a number below the oldest unacked only once it was handled LL_DUPLICATE_COPY_WINDOW (5 s) or longer ago, the same window within which isDuplicate still matches an unmarked copy. A number the ping passes sooner stays until a later ping. The warning in updateWatchDogTimers now says when suppression can still fail: a copy that comes after the ping and more than 5 s after its packet was handled. llcircuit_test checks that a number a ping passes 1 s after it was handled is still a duplicate, marked resent or not, and that a ping 6 s after lets it go while keeping the number it does not pass. The probe's handled counts the packet in as checkMessages does, so the circuit knows the highest number it has had. Co-Authored-By: Claude Opus 5.5 --- indra/llmessage/llcircuit.cpp | 45 +++++++++++++++--------- indra/llmessage/tests/llcircuit_test.cpp | 28 +++++++++++++-- 2 files changed, 55 insertions(+), 18 deletions(-) diff --git a/indra/llmessage/llcircuit.cpp b/indra/llmessage/llcircuit.cpp index 90cacae253..a0274fd1c1 100644 --- a/indra/llmessage/llcircuit.cpp +++ b/indra/llmessage/llcircuit.cpp @@ -63,7 +63,8 @@ const S32 PING_RELEASE_BLOCK = 2; // How many pings behind we have to be to co const F32Seconds TARGET_PERIOD_LENGTH(5.f); const F32Seconds LL_DUPLICATE_SUPPRESSION_TIMEOUT(60.f); //this can be long, as time-based cleanup is // only done when wrapping packetids, now... -const F32Seconds LL_DUPLICATE_COPY_WINDOW(5.f); // how long after a reliable packet is handled an unmarked copy of it counts as a duplicate +const F32Seconds LL_DUPLICATE_COPY_WINDOW(5.f); // how long after a reliable packet is handled a copy of it may still arrive: + // an unmarked copy counts as a duplicate, and a ping keeps its number LLCircuitData::LLCircuitData(const LLHost &host, TPACKETID in_id, const F32Seconds circuit_heartbeat_interval, const F32Seconds circuit_timeout) @@ -874,11 +875,10 @@ bool LLCircuitData::updateWatchDogTimers(LLMessageSystem *msgsys) } // WARNING! - // Duplicate suppression can FAIL if packets are delivered out of - // order, although it's EXTREMELY unlikely. It would require - // that the ping get delivered out of order enough that the ACK - // for the packet that it was out of order with was received BEFORE - // the ping was sent. + // Duplicate suppression can FAIL if a copy of a packet is delivered + // after the ping, and longer than LL_DUPLICATE_COPY_WINDOW after the + // packet was handled: the ping lets the receiver forget a number its + // oldest unacked passes once it was handled that long ago. // Find the current oldest reliable packetID // This is to handle the case if we actually manage to wrap our @@ -1015,26 +1015,39 @@ void LLCircuitData::clearDuplicateList(TPACKETID oldest_id) { // purge old data from the duplicate suppression queue - // we want to KEEP all x where oldest_id <= x <= last incoming packet, and delete everything else. + // we want to KEEP all x where oldest_id <= x <= last incoming packet, and delete everything else + // once no copy of it can still be coming. //LL_INFOS() << mHost << ": clearing before oldest " << oldest_id << LL_ENDL; //LL_INFOS() << "Recent list before: " << mRecentlyReceivedReliablePackets.size() << LL_ENDL; + U64Microseconds mt_usec = LLMessageSystem::getMessageTimeUsecs(); + + packet_time_map::iterator pit; if (oldest_id < mHighestPacketID) { - // Clean up everything with a packet ID less than oldest_id. - packet_time_map::iterator pit_start; - packet_time_map::iterator pit_end; - pit_start = mRecentlyReceivedReliablePackets.begin(); - pit_end = mRecentlyReceivedReliablePackets.lower_bound(oldest_id); - mRecentlyReceivedReliablePackets.erase(pit_start, pit_end); + // Clean up everything with a packet ID less than oldest_id that was + // handled longer ago than a copy of it takes to arrive. The sender + // pings once it has the ack, and a resend that crossed the ack, or a + // copy the network made, can come after the ping. + packet_time_map::iterator pit_end = mRecentlyReceivedReliablePackets.lower_bound(oldest_id); + for (pit = mRecentlyReceivedReliablePackets.begin(); pit != pit_end; ) + { + U64Microseconds delta_t_usec = mt_usec - (*pit).second; + F64Seconds delta_t_sec = delta_t_usec; + if (delta_t_sec >= LL_DUPLICATE_COPY_WINDOW) + { + mRecentlyReceivedReliablePackets.erase(pit++); + } + else + { + ++pit; + } + } } // Do timeout checks on everything with an ID > mHighestPacketID. // This should be empty except for wrapping IDs. Thus, this should be // highly rare. - U64Microseconds mt_usec = LLMessageSystem::getMessageTimeUsecs(); - - packet_time_map::iterator pit; for(pit = mRecentlyReceivedReliablePackets.upper_bound(mHighestPacketID); pit != mRecentlyReceivedReliablePackets.end(); ) { diff --git a/indra/llmessage/tests/llcircuit_test.cpp b/indra/llmessage/tests/llcircuit_test.cpp index 287eeb850e..344605be2e 100644 --- a/indra/llmessage/tests/llcircuit_test.cpp +++ b/indra/llmessage/tests/llcircuit_test.cpp @@ -199,10 +199,11 @@ namespace ll_test circuit.mAcks.erase(circuit.mAcks.begin(), circuit.mAcks.end()); } - // A reliable packet was handled, its number recorded as - // LLMessageSystem::checkMessages records it. + // A reliable packet was handled, its number counted in and recorded + // as LLMessageSystem::checkMessages counts and records it. static void handled(LLCircuitData& circuit, TPACKETID id) { + circuit.checkPacketInID(id, false); circuit.mRecentlyReceivedReliablePackets[id] = LLMessageSystem::getMessageTimeUsecs(); } @@ -400,4 +401,27 @@ namespace CHECK_FALSE(CircuitProbe::isDuplicate(circuit, 7, false)); CHECK(CircuitProbe::isDuplicate(circuit, 7, true)); } + + TEST_CASE("llcircuit: a ping keeps a number handled moments ago that its sender has had acked", "[llcircuit]") + { + StubMessageSystemScope message_system; + LLCircuitData circuit(LLHost(), 0, HEARTBEAT, TIMEOUT); + CircuitProbe::handled(circuit, 7); + CircuitProbe::handled(circuit, 8); + CircuitProbe::handled(circuit, 9); + + // A second on, the sender has had 7 acked and pings with 8 its oldest + // unacked; a resend of 7 that crossed the ack, or a copy the network + // made, may still be on its way. + sMessageTime += F64Seconds(1.0); + circuit.clearDuplicateList(8); + CHECK(CircuitProbe::isDuplicate(circuit, 7, false)); + CHECK(CircuitProbe::isDuplicate(circuit, 7, true)); + + // A ping six seconds on lets 7 go, and keeps 8, which it does not pass. + sMessageTime += F64Seconds(5.0); + circuit.clearDuplicateList(8); + CHECK_FALSE(CircuitProbe::isDuplicate(circuit, 7, true)); + CHECK(CircuitProbe::isDuplicate(circuit, 8, true)); + } } From 406baace8533ab8887373e3abfc5eb52f274eba5 Mon Sep 17 00:00:00 2001 From: Rye Date: Sun, 1 Nov 2026 10:26:28 -0500 Subject: [PATCH 9/9] LLCircuitData counts the copy window from a packet's latest copy A number the ping passes was kept for 5 s from when its packet was first handled. While acks for a packet are lost its sender goes on resending it, and each resend is caught as a duplicate and acked again; once an ack gets through the sender pings, and a resend it sent before that ack came can still arrive after the ping. With the packet handled more than 5 s before, the ping had already dropped its number, and the resend was handled a second time: handled at 0, resent at 4 s, pinged past at 6.5 s, resent again at 7 s. isDuplicate is now checkDuplicate, which keeps a duplicate's number as seen now, so LL_DUPLICATE_COPY_WINDOW runs from the latest copy. A resend that crossed the ack was sent before the ack reached its sender, so it comes within about a round trip and the ack collect time of the copy that ack answered. An unmarked copy likewise matches within 5 s of the latest copy rather than the first. Both callers, checkMessages and the arrival check in bufferInboundPacket, call it unchanged otherwise. llcircuit_test plays the case above: the resend at 7 s is a duplicate, and a ping at 12 s, 5 s after it, drops the number. Co-Authored-By: Claude Opus 5.5 --- indra/llmessage/llcircuit.cpp | 33 ++++++++------ indra/llmessage/llcircuit.h | 5 ++- indra/llmessage/message.cpp | 4 +- indra/llmessage/tests/llcircuit_test.cpp | 56 ++++++++++++++++++------ 4 files changed, 67 insertions(+), 31 deletions(-) diff --git a/indra/llmessage/llcircuit.cpp b/indra/llmessage/llcircuit.cpp index a0274fd1c1..cac0529fb8 100644 --- a/indra/llmessage/llcircuit.cpp +++ b/indra/llmessage/llcircuit.cpp @@ -63,7 +63,7 @@ const S32 PING_RELEASE_BLOCK = 2; // How many pings behind we have to be to co const F32Seconds TARGET_PERIOD_LENGTH(5.f); const F32Seconds LL_DUPLICATE_SUPPRESSION_TIMEOUT(60.f); //this can be long, as time-based cleanup is // only done when wrapping packetids, now... -const F32Seconds LL_DUPLICATE_COPY_WINDOW(5.f); // how long after a reliable packet is handled a copy of it may still arrive: +const F32Seconds LL_DUPLICATE_COPY_WINDOW(5.f); // how long after a reliable packet, or its latest copy, a copy may still arrive: // an unmarked copy counts as a duplicate, and a ping keeps its number LLCircuitData::LLCircuitData(const LLHost &host, TPACKETID in_id, @@ -600,24 +600,30 @@ void LLCircuit::resendUnackedPackets(S32& unacked_list_length, S32& unacked_list } -bool LLCircuitData::isDuplicate(TPACKETID packetnum, bool resent) +bool LLCircuitData::checkDuplicate(TPACKETID packetnum, bool resent) { - packet_time_map::const_iterator it = mRecentlyReceivedReliablePackets.find(packetnum); + packet_time_map::iterator it = mRecentlyReceivedReliablePackets.find(packetnum); if (it == mRecentlyReceivedReliablePackets.end()) { return false; } - if (resent) - { - return true; - } - // A copy the network made of a packet comes within moments of the first, + // A copy the network made of a packet comes within moments of the last, // unmarked. An older number an unmarked packet meets is left from // numbering the sender has since started over, on a circuit kept up. - U64Microseconds delta_t_usec = LLMessageSystem::getMessageTimeUsecs() - it->second; + U64Microseconds mt_usec = LLMessageSystem::getMessageTimeUsecs(); + U64Microseconds delta_t_usec = mt_usec - it->second; F64Seconds delta_t_sec = delta_t_usec; - return delta_t_sec < LL_DUPLICATE_COPY_WINDOW; + if (!resent && delta_t_sec >= LL_DUPLICATE_COPY_WINDOW) + { + return false; + } + + // While acks for a packet are lost its sender goes on resending it, and + // the last resend can come after the ping that says it was acked: the + // window a copy may still arrive in runs from the latest copy. + it->second = mt_usec; + return true; } @@ -877,8 +883,8 @@ bool LLCircuitData::updateWatchDogTimers(LLMessageSystem *msgsys) // WARNING! // Duplicate suppression can FAIL if a copy of a packet is delivered // after the ping, and longer than LL_DUPLICATE_COPY_WINDOW after the - // packet was handled: the ping lets the receiver forget a number its - // oldest unacked passes once it was handled that long ago. + // packet or its last copy came: the ping lets the receiver forget a + // number its oldest unacked passes once nothing of it came for that long. // Find the current oldest reliable packetID // This is to handle the case if we actually manage to wrap our @@ -1026,7 +1032,8 @@ void LLCircuitData::clearDuplicateList(TPACKETID oldest_id) if (oldest_id < mHighestPacketID) { // Clean up everything with a packet ID less than oldest_id that was - // handled longer ago than a copy of it takes to arrive. The sender + // handled, or last seen again, longer ago than a copy of it takes to + // arrive. The sender // pings once it has the ack, and a resend that crossed the ack, or a // copy the network made, can come after the ping. packet_time_map::iterator pit_end = mRecentlyReceivedReliablePackets.lower_bound(oldest_id); diff --git a/indra/llmessage/llcircuit.h b/indra/llmessage/llcircuit.h index ae1ffcf81e..8ea96652ad 100644 --- a/indra/llmessage/llcircuit.h +++ b/indra/llmessage/llcircuit.h @@ -184,8 +184,9 @@ class LLCircuitData void addReliablePacket(S32 mSocket, U8 *buf_ptr, S32 buf_len, LLReliablePacketParams *params); // Whether a reliable packet numbered packetnum was handled already: any // number the record holds for a resent packet, and for an unmarked one - // only a number handled moments ago. - bool isDuplicate(TPACKETID packetnum, bool resent); + // only a number handled or seen again moments ago. A duplicate's number + // is kept as seen now. + bool checkDuplicate(TPACKETID packetnum, bool resent); // Call this method when a reliable message comes in - this will // correctly place the packet in the correct list to be acked // later. RAack = requested ack diff --git a/indra/llmessage/message.cpp b/indra/llmessage/message.cpp index dfc339de7f..e2cab409c5 100644 --- a/indra/llmessage/message.cpp +++ b/indra/llmessage/message.cpp @@ -645,7 +645,7 @@ bool LLMessageSystem::checkMessages(LockMessageChecker&, S64 frame_count, // time without the resent flag, and is as much a duplicate as a // resend. Only reliable packets' numbers are recorded, so an // unreliable one is not looked up. - if (recv_reliable && cdp && cdp->isDuplicate(mCurrentRecvPacketID, recv_resent)) + if (recv_reliable && cdp && cdp->checkDuplicate(mCurrentRecvPacketID, recv_resent)) { // We need to ACK here to suppress // further resends of packets we've @@ -1163,7 +1163,7 @@ S32 LLMessageSystem::bufferInboundPacket() // downstream. bool recv_resent = (data[0] & LL_RESENT_FLAG) != 0; bool recv_reliable = (data[0] & LL_RELIABLE_FLAG) != 0; - if (!recv_reliable || !cdp->isDuplicate(recv_packet_id, recv_resent)) + if (!recv_reliable || !cdp->checkDuplicate(recv_packet_id, recv_resent)) { cdp->checkPacketInID(recv_packet_id, recv_resent); pkt.setPacketIDChecked(true); diff --git a/indra/llmessage/tests/llcircuit_test.cpp b/indra/llmessage/tests/llcircuit_test.cpp index 344605be2e..ed8f6cb68d 100644 --- a/indra/llmessage/tests/llcircuit_test.cpp +++ b/indra/llmessage/tests/llcircuit_test.cpp @@ -207,9 +207,9 @@ namespace ll_test circuit.mRecentlyReceivedReliablePackets[id] = LLMessageSystem::getMessageTimeUsecs(); } - static bool isDuplicate(LLCircuitData& circuit, TPACKETID id, bool resent) + static bool checkDuplicate(LLCircuitData& circuit, TPACKETID id, bool resent) { - return circuit.isDuplicate(id, resent); + return circuit.checkDuplicate(id, resent); } }; } @@ -387,19 +387,19 @@ namespace CircuitProbe::handled(circuit, 7); // A copy the network made comes unmarked, a resend marked. - CHECK(CircuitProbe::isDuplicate(circuit, 7, false)); - CHECK(CircuitProbe::isDuplicate(circuit, 7, true)); - CHECK_FALSE(CircuitProbe::isDuplicate(circuit, 8, false)); - CHECK_FALSE(CircuitProbe::isDuplicate(circuit, 8, true)); + CHECK(CircuitProbe::checkDuplicate(circuit, 7, false)); + CHECK(CircuitProbe::checkDuplicate(circuit, 7, true)); + CHECK_FALSE(CircuitProbe::checkDuplicate(circuit, 8, false)); + CHECK_FALSE(CircuitProbe::checkDuplicate(circuit, 8, true)); sMessageTime += F64Seconds(4.0); - CHECK(CircuitProbe::isDuplicate(circuit, 7, false)); + CHECK(CircuitProbe::checkDuplicate(circuit, 7, false)); // Long after, an unmarked packet with that number is a fresh one from // numbering the sender started over; a resend still matches. sMessageTime += F64Seconds(6.0); - CHECK_FALSE(CircuitProbe::isDuplicate(circuit, 7, false)); - CHECK(CircuitProbe::isDuplicate(circuit, 7, true)); + CHECK_FALSE(CircuitProbe::checkDuplicate(circuit, 7, false)); + CHECK(CircuitProbe::checkDuplicate(circuit, 7, true)); } TEST_CASE("llcircuit: a ping keeps a number handled moments ago that its sender has had acked", "[llcircuit]") @@ -415,13 +415,41 @@ namespace // made, may still be on its way. sMessageTime += F64Seconds(1.0); circuit.clearDuplicateList(8); - CHECK(CircuitProbe::isDuplicate(circuit, 7, false)); - CHECK(CircuitProbe::isDuplicate(circuit, 7, true)); + CHECK(CircuitProbe::checkDuplicate(circuit, 7, false)); + CHECK(CircuitProbe::checkDuplicate(circuit, 7, true)); - // A ping six seconds on lets 7 go, and keeps 8, which it does not pass. + // A ping five seconds after those copies lets 7 go, and keeps 8, which + // it does not pass. sMessageTime += F64Seconds(5.0); circuit.clearDuplicateList(8); - CHECK_FALSE(CircuitProbe::isDuplicate(circuit, 7, true)); - CHECK(CircuitProbe::isDuplicate(circuit, 8, true)); + CHECK_FALSE(CircuitProbe::checkDuplicate(circuit, 7, true)); + CHECK(CircuitProbe::checkDuplicate(circuit, 8, true)); + } + + TEST_CASE("llcircuit: each copy of a handled packet restarts the time a ping keeps its number", "[llcircuit]") + { + StubMessageSystemScope message_system; + LLCircuitData circuit(LLHost(), 0, HEARTBEAT, TIMEOUT); + const F64Seconds handled_at = sMessageTime; + CircuitProbe::handled(circuit, 7); + CircuitProbe::handled(circuit, 8); + CircuitProbe::handled(circuit, 9); + + // The ack for 7 is lost, and the sender resends it. + sMessageTime = handled_at + F64Seconds(4.0); + CHECK(CircuitProbe::checkDuplicate(circuit, 7, true)); + + // The ack for that resend reaches the sender, which pings with 8 its + // oldest unacked, while a resend of 7 it sent before the ack came is + // still on its way. + sMessageTime = handled_at + F64Seconds(6.5); + circuit.clearDuplicateList(8); + sMessageTime = handled_at + F64Seconds(7.0); + CHECK(CircuitProbe::checkDuplicate(circuit, 7, true)); + + // A ping five seconds after that last copy lets 7 go. + sMessageTime = handled_at + F64Seconds(12.0); + circuit.clearDuplicateList(8); + CHECK_FALSE(CircuitProbe::checkDuplicate(circuit, 7, true)); } }