Time out an abandoned admin edit transaction

begin_edit_settings sets a bool that only the matching commit ever
clears. If the commit never arrives -- the client dropped its link
partway through a bulk config import, or went away entirely -- the
transaction stays open indefinitely, and every later config write from
any client is applied to RAM, acknowledged, and then never saved. The
writes look successful and are visible in get_config, but the node
reverts all of them at the next boot, and only a reboot clears it.

Give the transaction an idle timeout. Each deferred write refreshes the
clock; once it has sat untouched for the same 300s the session passkey
uses, the next admin message retires it, persisting the segments it had
deferred and flushing the warnings it was holding. The check runs after
the auth gates and before the switch, so every case sees consistent
state and the recovery's flash write happens in the main loop rather
than a disconnect callback.

It saves rather than rolls back because there is nothing to roll back
to: each write already took effect in RAM and was acknowledged, so the
transaction only ever deferred the save. Closing on disconnect instead
was tempting, but the reporter's captures show iOS dropping and
reconnecting within half a second several times per session, which would
abort imports that currently survive the blip.

Also drops the file-scope hasOpenEditTransaction, shadowed by the class
member at every use site and referenced nowhere else in the tree.

Reported in #11245

Author: thebentern

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,11 @@ bool AdminModule::handleReceivedProtobuf(const meshtastic_MeshPacket &mp, meshta
             return handled;
         }
     }
+    // This message is authorized, so it is a fair chance to notice that the client which opened an
+    // edit transaction is never coming back. Do it before the switch, so every case below (and
+    // everything they call) sees consistent transaction state.
+    expireStaleEditTransaction();
+
     switch (r->which_payload_variant) {
 
 #ifdef MESHTASTIC_ENCRYPTED_STORAGE
@@ -481,12 +485,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 +1881,32 @@ void AdminModule::reboot(int32_t seconds)
     rebootAtMsec = (seconds < 0) ? 0 : (millis() + seconds * 1000);
 }
 
+// A begin_edit_settings whose commit never arrives -- the client dropped the link partway through a
+// bulk config import, or simply went away -- used to leave the transaction open indefinitely. Every
+// later config write, from any client, was then applied to RAM and acknowledged but never written to
+// flash, so the node silently reverted all of it at the next boot and the only cure was a reboot.
+//
+// Bound how long an unattended transaction can hold saves hostage. Once one goes idle, persist what
+// it already applied and let the node go back to saving normally. The window is refreshed by each
+// write the transaction defers, so it only expires on a client that has genuinely stopped talking --
+// long enough that a brief BLE drop and reconnect mid-import doesn't trip it.
+void AdminModule::expireStaleEditTransaction()
+{
+    if (!hasOpenEditTransaction || Throttle::isWithinTimespanMs(editTransactionActivityMs, kEditTransactionIdleMs))
+        return;
+
+    LOG_WARN("Edit transaction abandoned for %us; committing what it applied", kEditTransactionIdleMs / 1000);
+    hasOpenEditTransaction = false;
+    int segments = deferredEditSegments;
+    deferredEditSegments = 0;
+    // Persist exactly what the transaction deferred; a never-used transaction has nothing to save.
+    // No reboot: the settings are already live in RAM, and the client that would have expected one
+    // is gone.
+    if (segments)
+        saveChanges(segments, false);
+    flushChannelWarnings();
+}
+
 void AdminModule::saveChanges(int saveWhat, bool shouldReboot)
 {
 #ifdef PIO_UNIT_TESTING
@@ -1885,6 +1917,9 @@ 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");
+        // The transaction is still being used, so it isn't the abandoned kind we time out.
+        editTransactionActivityMs = millis();
+        deferredEditSegments |= saveWhat;
     }
     if (shouldReboot && !hasOpenEditTransaction) {
         reboot(DEFAULT_REBOOT_SECONDS);

src/modules/AdminModule.h

@@ -40,6 +40,14 @@ class AdminModule : public ProtobufModule<meshtastic_AdminMessage>, public Obser
 
   private:
     bool hasOpenEditTransaction = false;
+    // How long an open edit transaction may sit idle before we assume its client is never coming
+    // back. Matches the session-passkey window; far longer than any real begin/commit sequence,
+    // including one carried over several mesh hops.
+    static constexpr uint32_t kEditTransactionIdleMs = 300 * 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,21 @@ 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 activity clock the way begin_edit_settings does, so a long-running suite can't age
+    // the transaction into expiry mid-test.
+    void deferSaves()
+    {
+        hasOpenEditTransaction = true;
+        editTransactionActivityMs = millis();
+    }
     int savedSegments() const { return lastSaveWhatForTest; }
 
+    bool editTransactionOpen() const { return hasOpenEditTransaction; }
+    // Backdate the open transaction's activity clock past the idle window, so the next admin message
+    // sees an abandoned transaction without the test waiting out the real timeout. The subtraction
+    // wraps when millis() is small, which is exactly what Throttle's unsigned comparison expects.
+    void ageEditTransaction() { editTransactionActivityMs = millis() - kEditTransactionIdleMs - 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,17 @@ static void sendCommitEdit()
     sendAdmin(m);
 }
 
+// An admin message that changes nothing, for asserting on what merely receiving one does.
+// It answers with a metadata response, so release the queued 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 +1651,57 @@ static void test_warn_transaction_singleChannel_keepsSpecificMessage()
     TEST_ASSERT_EQUAL_INT(0, warningsContaining("on channels"));
 }
 
+// An edit transaction whose commit never arrives (client dropped the link mid-import) must not hold
+// saves hostage forever. Once it goes idle, the next admin message retires it, persisting what it
+// applied and flushing the warnings it was holding.
+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 later config write must be saved normally once the abandoned transaction is gone, instead of
+// being silently deferred to a commit that is never coming.
+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'"));
+}
+
+// The timeout must not disturb a transaction that is still in use -- a bulk import sends many
+// writes in a row, and each one 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 +1863,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);