fix(transmithistory): atomic saves, and no flash writes from the RX path

src/main.cpp

@@ -1474,6 +1474,13 @@ void loop()
 #endif
     power->powerCommandsCheck();
 
+    // Persist transmit-history changes here, outside packet processing:
+    // setLastSentToMesh() only marks state dirty, so the flash write (which
+    // stalls core 0 with the cache disabled) never runs inside Router RX
+    // handling. Throttled internally to once per 5 minutes.
+    if (transmitHistory)
+        transmitHistory->flushIfDue();
+
     if (RadioLibInterface::instance != nullptr) {
         static uint32_t lastRadioMissedIrqPoll;
         if (!Throttle::isWithinTimespanMs(lastRadioMissedIrqPoll, 1000)) {

src/mesh/TransmitHistory.cpp

@@ -1,6 +1,7 @@
 #include "TransmitHistory.h"
 #include "FSCommon.h"
 #include "SPILock.h"
+#include "SafeFile.h"
 #include "gps/RTC.h"
 #include <Throttle.h>
 
@@ -87,15 +88,24 @@ void TransmitHistory::setLastSentToMesh(uint16_t key)
         const uint8_t flags = (getRTCQuality() == RTCQualityNone) ? ENTRY_FLAG_BOOT_RELATIVE : ENTRY_FLAG_NONE;
         history[key] = makeStoredTimestamp(now, flags);
         dirty = true;
-        // Don't flush to disk on every transmit - flash has limited write endurance.
-        // The in-memory lastMillis map handles throttle during normal operation.
-        // Disk is flushed: before deep sleep (sleep.cpp) and periodically here,
-        // throttled to at most once per 5 minutes. Always save the first time
-        // after boot so a crash-reboot loop can't avoid persisting.
-        if (lastDiskSave == 0 || !Throttle::isWithinTimespanMs(lastDiskSave, SAVE_INTERVAL_MS)) {
-            if (saveToDisk()) {
-                lastDiskSave = millis();
-            }
+        // Do NOT write flash here. Callers reach this from inside packet
+        // processing (e.g. NodeInfoModule while the Router handles an RX), and
+        // a LittleFS write stalls core 0 with the flash cache disabled - the
+        // worst possible place for it. The main loop calls flushIfDue(), which
+        // persists at most once per SAVE_INTERVAL_MS (and immediately for the
+        // first change after boot, so a crash-reboot loop can't avoid
+        // persisting). sleep.cpp still flushes directly before deep sleep.
+    }
+}
+
+void TransmitHistory::flushIfDue()
+{
+    if (!dirty) {
+        return;
+    }
+    if (lastDiskSave == 0 || !Throttle::isWithinTimespanMs(lastDiskSave, SAVE_INTERVAL_MS)) {
+        if (saveToDisk()) {
+            lastDiskSave = millis();
         }
     }
 }
@@ -212,47 +222,45 @@ bool TransmitHistory::saveToDisk()
         return true;
     }
 
-    spiLock->lock();
-
-    FSCom.mkdir("/prefs");
+    {
+        concurrency::LockGuard g(spiLock);
+        FSCom.mkdir("/prefs");
+    }
 
-    // Remove old file first
-    if (FSCom.exists(FILENAME)) {
-        FSCom.remove(FILENAME);
+    // SafeFile writes to FILENAME.tmp, verifies the content by hash readback,
+    // then renames over the old file - so a crash mid-write can no longer
+    // destroy the existing history (the old remove-before-write here did
+    // exactly that). The file is tiny, so fullAtomic costs nothing.
+    // SafeFile takes spiLock itself around each filesystem operation.
+    SafeFile file(FILENAME, true);
+
+    FileHeader header{};
+    header.magic = MAGIC;
+    header.version = VERSION;
+    header.count = (uint8_t)min((size_t)MAX_ENTRIES, history.size());
+
+    file.write((uint8_t *)&header, sizeof(header));
+
+    uint8_t written = 0;
+    for (const auto &[key, stored] : history) {
+        if (written >= MAX_ENTRIES)
+            break;
+        Entry entry{};
+        entry.key = key;
+        entry.epochSeconds = stored.seconds;
+        entry.flags = stored.flags;
+        file.write((uint8_t *)&entry, sizeof(entry));
+        written++;
     }
 
-    auto file = FSCom.open(FILENAME, FILE_O_WRITE);
-    if (file) {
-        FileHeader header{};
-        header.magic = MAGIC;
-        header.version = VERSION;
-        header.count = (uint8_t)min((size_t)MAX_ENTRIES, history.size());
-
-        file.write((uint8_t *)&header, sizeof(header));
-
-        uint8_t written = 0;
-        for (const auto &[key, stored] : history) {
-            if (written >= MAX_ENTRIES)
-                break;
-            Entry entry{};
-            entry.key = key;
-            entry.epochSeconds = stored.seconds;
-            entry.flags = stored.flags;
-            file.write((uint8_t *)&entry, sizeof(entry));
-            written++;
-        }
-        file.flush();
-        file.close();
-        LOG_DEBUG("TransmitHistory: saved %u entries to disk", written);
-        dirty = false;
-        spiLock->unlock();
-        return true;
-    } else {
-        LOG_WARN("TransmitHistory: failed to open file for writing");
+    if (!file.close()) {
+        LOG_WARN("TransmitHistory: failed to write history file");
+        return false;
     }
 
-    spiLock->unlock();
-    return false;
+    LOG_DEBUG("TransmitHistory: saved %u entries to disk", written);
+    dirty = false;
+    return true;
 }
 
 void TransmitHistory::clear()
@@ -289,6 +297,8 @@ void TransmitHistory::setLastSentToMesh(uint16_t key)
     lastMillis[key] = millis();
 }
 
+void TransmitHistory::flushIfDue() {}
+
 uint32_t TransmitHistory::getLastSentToMeshEpoch(uint16_t key) const
 {
     return 0;

src/mesh/TransmitHistory.h

@@ -12,7 +12,8 @@
  * from the stored epoch time, which plugs directly into existing throttle logic.
  *
  * On every broadcast transmit, modules call setLastSentToMesh() which updates the
- * in-memory cache and flushes to disk.
+ * in-memory cache and marks the history dirty; the main loop persists it via
+ * flushIfDue() so the flash write never runs inside packet processing.
  *
  * Keys are meshtastic_PortNum values (one entry per portnum).
  */
@@ -31,10 +32,18 @@ class TransmitHistory
 
     /**
      * Record that a broadcast was sent for the given key right now.
-     * Stores epoch seconds and flushes to disk.
+     * Stores epoch seconds in memory and marks the history dirty; the actual
+     * flash write happens later via flushIfDue() (or saveToDisk() directly).
      */
     void setLastSentToMesh(uint16_t key);
 
+    /**
+     * Persist dirty entries if a save is due: immediately for the first change
+     * after boot, then at most once per SAVE_INTERVAL_MS. Cheap no-op when
+     * nothing is dirty. Called from the main loop, outside packet processing.
+     */
+    void flushIfDue();
+
 #ifdef PIO_UNIT_TESTING
     /**
      * Directly set the stored epoch for a key without touching the runtime lastMillis map.

test/test_transmit_history/test_main.cpp

@@ -168,6 +168,23 @@ static void test_save_and_load_round_trip()
     }
 }
 
+// setLastSentToMesh() no longer writes flash inline (the write is deferred out of the
+// packet-processing path). flushIfDue() is the deferred writer: with pending changes
+// and no prior save this boot, it must persist immediately - through the SafeFile
+// tmp-write/readback/rename path - and survive a reload.
+static void test_flushIfDue_persists_pending_changes()
+{
+    transmitHistory->clear(); // remove any on-disk file left by earlier tests
+
+    transmitHistory->setLastSentAtEpoch(meshtastic_PortNum_NODEINFO_APP, 1700000000);
+    transmitHistory->flushIfDue(); // first pending change after clear() -> saves immediately
+
+    resetTransmitHistory();
+    transmitHistory->loadFromDisk();
+
+    TEST_ASSERT_EQUAL_UINT32(1700000000, transmitHistory->getLastSentToMeshEpoch(meshtastic_PortNum_NODEINFO_APP));
+}
+
 // --- Boot without RTC scenario ---
 
 // Crash-reboot protection: a send that happened moments before the reboot must still
@@ -324,6 +341,7 @@ void setup()
 
     // Persistence
     RUN_TEST(test_save_and_load_round_trip);
+    RUN_TEST(test_flushIfDue_persists_pending_changes);
     RUN_TEST(test_boot_after_recent_send_still_throttles);
 
     // Issue #9901 regression tests