From ae16c052d5e12ec6e736d9216cb5be10b7da67f9 Mon Sep 17 00:00:00 2001 From: Ben Meadors Date: Mon, 27 Jul 2026 10:22:33 -0500 Subject: [PATCH] Time out an abandoned admin edit transaction (#11254) 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 a one minute idle timeout. Each deferred write restarts the clock, so the window bounds the gap between writes rather than the length of the edit; a bulk import sends them milliseconds apart. The next admin message after it lapses retires the transaction, 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. That also makes an early expiry cheap -- the worst case for a client that really was still going is that its remaining writes are saved individually instead of batched. 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 --- src/modules/AdminModule.cpp | 25 +++++++++++- src/modules/AdminModule.h | 7 ++++ test/support/AdminModuleTestShim.h | 11 +++++- test/test_admin_radio/test_main.cpp | 60 +++++++++++++++++++++++++++++ 4 files changed, 101 insertions(+), 2 deletions(-) diff --git a/src/modules/AdminModule.cpp b/src/modules/AdminModule.cpp index 08b94dc04..98a9c9d93 100644 --- a/src/modules/AdminModule.cpp +++ b/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); diff --git a/src/modules/AdminModule.h b/src/modules/AdminModule.h index 020e9f0d6..b8e8c59ad 100644 --- a/src/modules/AdminModule.h +++ b/src/modules/AdminModule.h @@ -40,6 +40,13 @@ class AdminModule : public ProtobufModule, 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 diff --git a/test/support/AdminModuleTestShim.h b/test/support/AdminModuleTestShim.h index 1b06457f1..15fe9e782 100644 --- a/test/support/AdminModuleTestShim.h +++ b/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() { diff --git a/test/test_admin_radio/test_main.cpp b/test/test_admin_radio/test_main.cpp index 966cc6859..4b88be701 100644 --- a/test/test_admin_radio/test_main.cpp +++ b/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);