diff --git a/src/mesh/TransmitHistory.cpp b/src/mesh/TransmitHistory.cpp index b615c307a..2c53921ef 100644 --- a/src/mesh/TransmitHistory.cpp +++ b/src/mesh/TransmitHistory.cpp @@ -29,10 +29,17 @@ void TransmitHistory::loadFromDisk() if (file.read((uint8_t *)&entry, sizeof(entry)) == sizeof(entry)) { if (entry.epochSeconds > 0) { history[entry.key] = entry.epochSeconds; - // Seed in-memory millis so throttle works even without RTC/GPS. - // Treating stored entries as "just sent" is safe — worst case the - // node waits one full interval before its first broadcast. - lastMillis[entry.key] = millis(); + // Do NOT seed lastMillis here. + // + // getLastSentToMeshMillis() reconstructs a millis()-relative value + // from the stored epoch, and Throttle::isWithinTimespanMs() uses + // the same unsigned subtraction pattern. That means recent reboots + // still throttle correctly, while long power-off periods no longer + // look like "just sent" and incorrectly suppress the first send. + // + // If we seeded lastMillis to millis() here, every loaded entry would + // appear to have been sent at boot time, regardless of the true age + // of the last transmission. That was the regression behind #9901. } } } @@ -68,6 +75,17 @@ void TransmitHistory::setLastSentToMesh(uint16_t key) } } +void TransmitHistory::setLastSentAtEpoch(uint16_t key, uint32_t epochSeconds) +{ + if (epochSeconds > 0) { + history[key] = epochSeconds; + dirty = true; + } else { + history.erase(key); + lastMillis.erase(key); + } +} + uint32_t TransmitHistory::getLastSentToMeshEpoch(uint16_t key) const { auto it = history.find(key); @@ -110,9 +128,13 @@ uint32_t TransmitHistory::getLastSentToMeshMillis(uint16_t key) const return 0; } - // Convert to a millis()-relative timestamp: millis() - msAgo - // This gives a value that, when passed to Throttle::isWithinTimespanMs(value, interval), - // correctly reports whether the transmit was within interval ms. + // Convert to a millis()-relative timestamp: millis() - msAgo. + // + // The result may wrap if msAgo is larger than the current uptime, and that is + // intentional. Throttle::isWithinTimespanMs() also uses unsigned subtraction, + // so the reconstructed age is preserved across wraparound: + // - recent reboot, 5 min ago -> (millis() - lastMs) == 300000, still throttled + // - long reboot, 30 min ago -> (millis() - lastMs) == 1800000, allowed return millis() - msAgo; } diff --git a/src/mesh/TransmitHistory.h b/src/mesh/TransmitHistory.h index 01201eaac..49fcf4001 100644 --- a/src/mesh/TransmitHistory.h +++ b/src/mesh/TransmitHistory.h @@ -35,6 +35,13 @@ class TransmitHistory */ void setLastSentToMesh(uint16_t key); + /** + * Directly set the stored epoch for a key without touching the runtime lastMillis map. + * Intended for testing purposes: lets tests simulate "the last broadcast happened N + * seconds ago" without needing to fake the system clock. + */ + void setLastSentAtEpoch(uint16_t key, uint32_t epochSeconds); + /** * Get the last transmit epoch seconds for a given key, or 0 if unknown. */ diff --git a/test/test_transmit_history/test_main.cpp b/test/test_transmit_history/test_main.cpp index 992668d97..655c8c6b4 100644 --- a/test/test_transmit_history/test_main.cpp +++ b/test/test_transmit_history/test_main.cpp @@ -161,44 +161,102 @@ static void test_save_and_load_round_trip() // After loadFromDisk, millis should be seeded (non-zero) for stored entries uint32_t restoredMillis = transmitHistory->getLastSentToMeshMillis(meshtastic_PortNum_NODEINFO_APP); if (restoredNodeInfo > 0) { - // If epoch was stored, millis should be seeded from load + // If epoch was stored (set seconds ago), epoch-conversion gives elapsed ≈ 0 s, + // so getLastSentToMeshMillis() should return a non-zero value. TEST_ASSERT_NOT_EQUAL(0, restoredMillis); } } // --- Boot without RTC scenario --- -static void test_load_seeds_millis_even_without_rtc() +// Crash-reboot protection: a send that happened moments before the reboot must still +// throttle after reload. This works because getLastSentToMeshMillis() reconstructs +// a millis()-relative timestamp from the stored epoch, and Throttle uses unsigned +// subtraction so the age survives wraparound even when uptime is near zero. +static void test_boot_after_recent_send_still_throttles() { - // This tests the critical crash-reboot scenario: - // After loadFromDisk(), even if getTime() returns 0 (no RTC), - // lastMillis should be seeded so throttle blocks immediate re-broadcast. - transmitHistory->setLastSentToMesh(meshtastic_PortNum_NODEINFO_APP); transmitHistory->saveToDisk(); - // Simulate reboot: destroy and recreate + // Simulate reboot delete transmitHistory; transmitHistory = nullptr; transmitHistory = TransmitHistory::getInstance(); transmitHistory->loadFromDisk(); - // The key insight: after load, getLastSentToMeshMillis should return non-zero - // because loadFromDisk seeds lastMillis[key] = millis() for every loaded entry. - // This ensures throttle works even without RTC. + // Epoch was set seconds ago; reconstructed age is still within the 10-min window. uint32_t result = transmitHistory->getLastSentToMeshMillis(meshtastic_PortNum_NODEINFO_APP); - uint32_t epoch = transmitHistory->getLastSentToMeshEpoch(meshtastic_PortNum_NODEINFO_APP); if (epoch > 0) { - // Data was persisted — millis must be seeded TEST_ASSERT_NOT_EQUAL(0, result); - - // And it should cause throttle to block (treating as "just sent") bool withinInterval = Throttle::isWithinTimespanMs(result, 10 * 60 * 1000); TEST_ASSERT_TRUE(withinInterval); } - // If epoch == 0, RTC wasn't available — no data was saved, so nothing to restore. - // This is expected on platforms without RTC during the very first boot. + // If epoch == 0, RTC was unavailable during setLastSentToMesh() so nothing was persisted. +} + +// Regression test for issue #9901: +// A device powered off for longer than the throttle window must broadcast NodeInfo +// on its next boot — it must not be silenced because loadFromDisk() once treated +// every loaded entry as "just sent" by seeding lastMillis to millis() at boot. +static void test_boot_after_long_gap_allows_nodeinfo() +{ + uint32_t now = getTime(); + if (now < 2) { + TEST_IGNORE_MESSAGE("No RTC available; skipping epoch-dependent test"); + return; + } + + // Simulate: last NodeInfo sent 30 minutes ago (outside the 10-min throttle window) + transmitHistory->setLastSentAtEpoch(meshtastic_PortNum_NODEINFO_APP, now - (30 * 60)); + transmitHistory->saveToDisk(); + + // Simulate reboot + delete transmitHistory; + transmitHistory = nullptr; + transmitHistory = TransmitHistory::getInstance(); + transmitHistory->loadFromDisk(); + + uint32_t restoredEpoch = transmitHistory->getLastSentToMeshEpoch(meshtastic_PortNum_NODEINFO_APP); + if (restoredEpoch == 0) { + TEST_IGNORE_MESSAGE("Epoch not persisted; skipping"); + return; + } + + uint32_t restoredMs = transmitHistory->getLastSentToMeshMillis(meshtastic_PortNum_NODEINFO_APP); + bool throttled = (restoredMs != 0) && Throttle::isWithinTimespanMs(restoredMs, 10 * 60 * 1000); + TEST_ASSERT_FALSE_MESSAGE(throttled, "NodeInfo must not be throttled after a 30-min gap (#9901)"); +} + +// Complementary: a rapid reboot must still throttle (crash-loop protection), even +// though the reconstructed lastMs may wrap because current uptime is small. +static void test_boot_within_throttle_window_still_throttles() +{ + uint32_t now = getTime(); + if (now < 2) { + TEST_IGNORE_MESSAGE("No RTC available; skipping epoch-dependent test"); + return; + } + + // Simulate: last NodeInfo sent 5 minutes ago (inside the 10-min throttle window) + transmitHistory->setLastSentAtEpoch(meshtastic_PortNum_NODEINFO_APP, now - (5 * 60)); + transmitHistory->saveToDisk(); + + // Simulate reboot + delete transmitHistory; + transmitHistory = nullptr; + transmitHistory = TransmitHistory::getInstance(); + transmitHistory->loadFromDisk(); + + uint32_t restoredEpoch = transmitHistory->getLastSentToMeshEpoch(meshtastic_PortNum_NODEINFO_APP); + if (restoredEpoch == 0) { + TEST_IGNORE_MESSAGE("Epoch not persisted; skipping"); + return; + } + + uint32_t restoredMs = transmitHistory->getLastSentToMeshMillis(meshtastic_PortNum_NODEINFO_APP); + bool throttled = (restoredMs != 0) && Throttle::isWithinTimespanMs(restoredMs, 10 * 60 * 1000); + TEST_ASSERT_TRUE_MESSAGE(throttled, "NodeInfo must still be throttled when last send was within the 10-min window"); } void setup() @@ -222,7 +280,11 @@ void setup() // Persistence RUN_TEST(test_save_and_load_round_trip); - RUN_TEST(test_load_seeds_millis_even_without_rtc); + RUN_TEST(test_boot_after_recent_send_still_throttles); + + // Issue #9901 regression tests + RUN_TEST(test_boot_after_long_gap_allows_nodeinfo); + RUN_TEST(test_boot_within_throttle_window_still_throttles); exit(UNITY_END()); }