diff --git a/src/modules/AdminModule.cpp b/src/modules/AdminModule.cpp index 08b94dc045..98a9c9d934 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 020e9f0d6d..b8e8c59ade 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 1b06457f1e..15fe9e782e 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 966cc68597..4b88be7014 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);