Time out an abandoned admin edit transaction

src/modules/AdminModule.cpp

@@ -69,7 +69,6 @@
 #endif
 
 AdminModule *adminModule;
-bool hasOpenEditTransaction;
 
 #if !(MESHTASTIC_EXCLUDE_PKI_KEYGEN || MESHTASTIC_EXCLUDE_PKI)
 static bool licensedIdentityWillMigrate()
@@ -240,6 +239,9 @@ bool AdminModule::handleReceivedProtobuf(const meshtastic_MeshPacket &mp, meshta
             return handled;
         }
     }
+    // Before the switch, so every case below sees consistent transaction state.
+    expireStaleEditTransaction();
+
     switch (r->which_payload_variant) {
 
 #ifdef MESHTASTIC_ENCRYPTED_STORAGE
@@ -481,12 +483,14 @@ bool AdminModule::handleReceivedProtobuf(const meshtastic_MeshPacket &mp, meshta
     case meshtastic_AdminMessage_begin_edit_settings_tag: {
         LOG_INFO("Begin transaction for editing settings");
         hasOpenEditTransaction = true;
+        editTransactionActivityMs = millis();
         break;
     }
     case meshtastic_AdminMessage_commit_edit_settings_tag: {
         disableBluetooth();
         LOG_INFO("Commit transaction for edited settings");
         hasOpenEditTransaction = false;
+        deferredEditSegments = 0;
         saveChanges(SEGMENT_CONFIG | SEGMENT_MODULECONFIG | SEGMENT_DEVICESTATE | SEGMENT_CHANNELS | SEGMENT_NODEDATABASE);
         flushChannelWarnings(); // one coalesced message for everything edited in this transaction
         break;
@@ -1875,6 +1879,23 @@ void AdminModule::reboot(int32_t seconds)
     rebootAtMsec = (seconds < 0) ? 0 : (millis() + seconds * 1000);
 }
 
+// Without this, a commit that never arrives leaves the transaction open forever and every later
+// config write from any client is applied, acknowledged, and then never saved.
+void AdminModule::expireStaleEditTransaction()
+{
+    if (!hasOpenEditTransaction || Throttle::isWithinTimespanMs(editTransactionActivityMs, EDIT_TRANSACTION_IDLE_MS))
+        return;
+
+    LOG_WARN("Edit transaction abandoned for %us; committing what it applied", EDIT_TRANSACTION_IDLE_MS / 1000);
+    hasOpenEditTransaction = false;
+    int segments = deferredEditSegments;
+    deferredEditSegments = 0;
+    // No reboot: the settings are already live in RAM and the client that would expect one is gone.
+    if (segments)
+        saveChanges(segments, false);
+    flushChannelWarnings();
+}
+
 void AdminModule::saveChanges(int saveWhat, bool shouldReboot)
 {
 #ifdef PIO_UNIT_TESTING
@@ -1885,6 +1906,8 @@ void AdminModule::saveChanges(int saveWhat, bool shouldReboot)
         service->reloadConfig(saveWhat); // Calls saveToDisk among other things
     } else {
         LOG_INFO("Delay save of changes to disk until the open transaction is committed");
+        editTransactionActivityMs = millis(); // still in use, so not the abandoned kind we time out
+        deferredEditSegments |= saveWhat;
     }
     if (shouldReboot && !hasOpenEditTransaction) {
         reboot(DEFAULT_REBOOT_SECONDS);

src/modules/AdminModule.h

@@ -40,6 +40,13 @@ class AdminModule : public ProtobufModule<meshtastic_AdminMessage>, public Obser
 
   private:
     bool hasOpenEditTransaction = false;
+    // Each deferred write restarts the clock, so this bounds the gap between writes, not the length
+    // of the edit; a bulk import sends them milliseconds apart.
+    static constexpr uint32_t EDIT_TRANSACTION_IDLE_MS = 60 * 1000;
+    uint32_t editTransactionActivityMs = 0; // millis() of the last save this transaction deferred
+    int deferredEditSegments = 0;           // segments that transaction has touched but not yet saved
+    /// Retire an open edit transaction whose client stopped talking, persisting what it applied.
+    void expireStaleEditTransaction();
 #ifdef PIO_UNIT_TESTING
     int lastSaveWhatForTest = 0;
 #endif

test/support/AdminModuleTestShim.h

@@ -21,9 +21,18 @@ class AdminModuleTestShim : public AdminModule
     meshtastic_MeshPacket *reply() { return myReply; }
 
     // With an "open edit transaction" saveChanges() is a pure no-op: no reloadConfig/saveToDisk/reboot.
-    void deferSaves() { hasOpenEditTransaction = true; }
+    // Stamps the clock like begin_edit_settings, so a slow suite can't age the transaction into expiry.
+    void deferSaves()
+    {
+        hasOpenEditTransaction = true;
+        editTransactionActivityMs = millis();
+    }
     int savedSegments() const { return lastSaveWhatForTest; }
 
+    bool editTransactionOpen() const { return hasOpenEditTransaction; }
+    // Backdate past the idle window so a test sees an abandoned transaction without waiting it out.
+    void ageEditTransaction() { editTransactionActivityMs = millis() - EDIT_TRANSACTION_IDLE_MS - 1; }
+
     // Setters may allocate an error reply from packetPool; drain it each iteration or the pool leaks.
     void drainReply()
     {

test/test_admin_radio/test_main.cpp

@@ -1574,6 +1574,16 @@ static void sendCommitEdit()
     sendAdmin(m);
 }
 
+// An admin message that changes nothing. It answers, so drain the reply or the packet pool leaks.
+static void sendGetDeviceMetadata()
+{
+    meshtastic_AdminMessage m = meshtastic_AdminMessage_init_zero;
+    m.which_payload_variant = meshtastic_AdminMessage_get_device_metadata_request_tag;
+    m.get_device_metadata_request = true;
+    sendAdmin(m);
+    testAdmin->drainReply();
+}
+
 // Preset = LongFast on US, unlicensed owner. "LongFast" is the display name we compare against.
 static void usePresetLongFast()
 {
@@ -1640,6 +1650,53 @@ static void test_warn_transaction_singleChannel_keepsSpecificMessage()
     TEST_ASSERT_EQUAL_INT(0, warningsContaining("on channels"));
 }
 
+// An idle transaction is retired by the next admin message, flushing the warnings it held.
+static void test_editTransaction_abandoned_isRetiredOnNextAdminMessage()
+{
+    usePresetLongFast();
+    sendBeginEdit();
+    sendSetChannel(makeChannel(0, meshtastic_Channel_Role_PRIMARY, "long fast", DEFAULT_KEY, 1));
+    // Deferred, exactly as before: nothing emitted while the transaction looks alive.
+    TEST_ASSERT_EQUAL_INT(0, (int)capturedWarnings.size());
+    TEST_ASSERT_TRUE(testAdmin->editTransactionOpen());
+
+    testAdmin->ageEditTransaction();
+    sendGetDeviceMetadata(); // any later admin message, from any client
+
+    TEST_ASSERT_FALSE(testAdmin->editTransactionOpen());
+    TEST_ASSERT_EQUAL_INT(1, warningsContaining("looks like a mistype of 'LongFast'"));
+}
+
+// A write arriving after abandonment is saved, not deferred to a commit that never comes.
+static void test_editTransaction_abandoned_laterWriteIsNoLongerDeferred()
+{
+    usePresetLongFast();
+    sendBeginEdit();
+    testAdmin->ageEditTransaction();
+
+    sendSetChannel(makeChannel(0, meshtastic_Channel_Role_PRIMARY, "long fast", DEFAULT_KEY, 1));
+
+    // The write itself retired the stale transaction, so its own warning is emitted immediately.
+    TEST_ASSERT_FALSE(testAdmin->editTransactionOpen());
+    TEST_ASSERT_EQUAL_INT(1, warningsContaining("looks like a mistype of 'LongFast'"));
+}
+
+// A transaction still in use is left alone: each write refreshes the window.
+static void test_editTransaction_active_isNotRetired()
+{
+    usePresetLongFast();
+    sendBeginEdit();
+    sendSetChannel(makeChannel(0, meshtastic_Channel_Role_PRIMARY, "long fast", DEFAULT_KEY, 1));
+    sendSetChannel(makeChannel(1, meshtastic_Channel_Role_SECONDARY, "long fast", DEFAULT_KEY, 1));
+
+    TEST_ASSERT_TRUE(testAdmin->editTransactionOpen());
+    TEST_ASSERT_EQUAL_INT(0, (int)capturedWarnings.size());
+
+    sendCommitEdit();
+    TEST_ASSERT_FALSE(testAdmin->editTransactionOpen());
+    TEST_ASSERT_EQUAL_INT(1, warningsContaining("There may be name issues on channels 0, 1"));
+}
+
 static void test_warn_license_noTransaction_emittedImmediately()
 {
     usePresetLongFast();
@@ -1801,6 +1858,9 @@ void setup()
     RUN_TEST(test_warn_cleanChannel_noMessage);
     RUN_TEST(test_warn_transaction_multipleChannels_singleCoalescedMessage);
     RUN_TEST(test_warn_transaction_singleChannel_keepsSpecificMessage);
+    RUN_TEST(test_editTransaction_abandoned_isRetiredOnNextAdminMessage);
+    RUN_TEST(test_editTransaction_abandoned_laterWriteIsNoLongerDeferred);
+    RUN_TEST(test_editTransaction_active_isNotRetired);
     RUN_TEST(test_warn_license_noTransaction_emittedImmediately);
     RUN_TEST(test_warn_license_transaction_coalescedToSingleMessage);