post rebase fixes

Author: NomDeTom

docs/admin-config-save-gating.md

@@ -285,17 +285,25 @@ Run against a real node through the
 (`MESHTASTIC_FIRMWARE_ROOT` → this checkout), with the serial log open. A nRF52840 SX126x
 board (e.g. WisMesh Tag) is the reference target; the crash was reproduced there.
 
-**Transport: serial is sufficient - the on-device menus are not needed, and neither is BLE.**
-These operations are all client-protocol admin messages (`ToRadio`), carried identically over
-serial (`SerialConsole`/`StreamAPI`) or BLE; the on-device button/screen menus are a separate
-code path this work did not touch. Crucially, on nRF52 the BLE `onWrite` callback only
-_queues_ the packet - `handleToRadio` → `saveChanges` → `reloadConfig` (the reconfigure) runs
-on the **main FreeRTOS task** ([`NimbleBluetooth.cpp:135`](../src/nimble/NimbleBluetooth.cpp#L135)),
+The scenarios below cover the **AdminModule / client-protocol path only**. The on-device menu
+path was also migrated by this PR (see "On-device menus" above) and is a genuinely separate
+code path - it needs its own validation pass, driven by the buttons and screen on the board,
+and none of the transport reasoning in this section extends to it.
+
+**Transport for the admin path: serial is sufficient, and BLE is not needed.** These
+operations are all client-protocol admin messages (`ToRadio`), carried identically over serial
+(`SerialConsole`/`StreamAPI`) or BLE. Crucially, on nRF52 the BLE `onWrite` callback only
+_queues_ the packet - `handleToRadio` → `saveChanges` → `applyConfigChange` (the reconfigure)
+runs on the **main FreeRTOS task** ([`NimbleBluetooth.cpp:135`](../src/nimble/NimbleBluetooth.cpp#L135)),
 exactly where `SerialConsole` (an `OSThread`) services the serial stream. So the code and
 thread under test are the same whichever transport you use, and the original crash was
 serial-proven. Drive the admin messages over USB serial (e.g. the `meshtastic --port` CLI);
 use BLE only if you specifically want to reproduce the exact user-facing conditions.
 
+The menu path reaches the same `MeshService::applyConfigChange` on the same main task, so it
+inherits the same reasoning about _what_ runs - but only a menu-driven pass exercises the
+menu call sites' own flag choices, which is what this PR changed there.
+
 ### 1. Radio-reload / crash validation (regression guard for the favorite-node fix)
 
 Confirms the non-LoRa config saves no longer run the live radio reconfigure - the main-task

test/support/AdminModuleTestShim.h

@@ -20,12 +20,17 @@ class AdminModuleTestShim : public AdminModule
     // Peek at the reply a handler queued, before drainReply() releases it.
     meshtastic_MeshPacket *reply() { return myReply; }
 
-    // With an "open edit transaction" saveChanges() is a pure no-op: no reloadConfig/saveToDisk/reboot.
-    // Stamps the clock like begin_edit_settings, so a slow suite can't age the transaction into expiry.
+    // With an "open edit transaction" saveChanges() has no outward effect - no
+    // applyConfigChange/saveToDisk/reboot - it only records what the eventual commit should do.
+    // Mirrors begin_edit_settings exactly: stamps the clock, so a slow suite can't age the
+    // transaction into expiry, and clears the deferred decisions so they start from the same
+    // baseline a real transaction gets.
     void deferSaves()
     {
         hasOpenEditTransaction = true;
         editTransactionActivityMs = millis();
+        deferredShouldReboot = false;
+        deferredRadioAffected = false;
     }
     int savedSegments() const { return lastSaveWhatForTest; }
 

test/test_admin_radio/test_main.cpp

@@ -2622,6 +2622,104 @@ static void test_commitWithoutBegin_persistsButDoesNotReloadOrReboot()
     TEST_ASSERT_EQUAL_UINT32(0, rebootAtMsec);
 }
 
+// The abandonment path retires a transaction *outside* commit_edit_settings, so it decides both
+// axes on its own. It must reach the same answers the commit would have, not fall back to the
+// old implicit "reload the radio" - an abandoned display-only batch has no more business
+// reprogramming the SX126x than a committed one does.
+static void test_abandonedTransaction_liveOnlyBatch_expiresWithoutReloadOrReboot()
+{
+    rebootAtMsec = 0;
+    ConfigChangedCounter counter;
+    counter.observe(&service->configChanged);
+
+    sendBeginEdit();
+    meshtastic_Config c = meshtastic_Config_init_zero;
+    c.which_payload_variant = meshtastic_Config_position_tag;
+    c.payload_variant.position = config.position;
+    c.payload_variant.position.position_broadcast_secs = config.position.position_broadcast_secs + 60;
+    sendSetConfig(c);
+
+    const int before = mockMeshService->reloadCalls;
+    testAdmin->ageEditTransaction();
+    sendGetDeviceMetadata(); // any later admin message retires the stale transaction
+
+    TEST_ASSERT_FALSE(testAdmin->editTransactionOpen());
+    TEST_ASSERT_EQUAL_INT(before + 1, mockMeshService->reloadCalls); // the deferred write still lands
+    TEST_ASSERT_EQUAL_INT(0, counter.count);
+    TEST_ASSERT_EQUAL_UINT32(0, rebootAtMsec);
+}
+
+// The guard the other way: abandonment must not lose a real LoRa change's reconfigure either.
+static void test_abandonedTransaction_loraSet_stillReloadsRadioOnExpiry()
+{
+    usePresetLongFast();
+    ConfigChangedCounter counter;
+    counter.observe(&service->configChanged);
+
+    sendBeginEdit();
+    meshtastic_Config c = meshtastic_Config_init_zero;
+    c.which_payload_variant = meshtastic_Config_lora_tag;
+    c.payload_variant.lora = config.lora;
+    c.payload_variant.lora.modem_preset = meshtastic_Config_LoRaConfig_ModemPreset_MEDIUM_FAST;
+    sendSetConfig(c);
+
+    TEST_ASSERT_EQUAL_INT(0, counter.count); // deferred, as for any open transaction
+
+    testAdmin->ageEditTransaction();
+    sendGetDeviceMetadata();
+
+    TEST_ASSERT_FALSE(testAdmin->editTransactionOpen());
+    TEST_ASSERT_EQUAL_INT(1, counter.count);
+}
+
+// Reboot is the one axis abandonment deliberately drops: the settings are already live in RAM and
+// the client that asked for the restart is, by definition, gone.
+static void test_abandonedTransaction_rebootingFieldDoesNotRebootOnExpiry()
+{
+    config.position.gps_mode = meshtastic_Config_PositionConfig_GpsMode_DISABLED; // known start state
+    rebootAtMsec = 0;
+
+    sendBeginEdit();
+    meshtastic_Config c = meshtastic_Config_init_zero;
+    c.which_payload_variant = meshtastic_Config_position_tag;
+    c.payload_variant.position = config.position;
+    c.payload_variant.position.gps_mode = meshtastic_Config_PositionConfig_GpsMode_ENABLED;
+    sendSetConfig(c);
+
+    testAdmin->ageEditTransaction();
+    sendGetDeviceMetadata();
+
+    TEST_ASSERT_FALSE(testAdmin->editTransactionOpen());
+    TEST_ASSERT_EQUAL_UINT32(0, rebootAtMsec);
+}
+
+// Expiry must consume-and-clear the accumulated decisions, not just read them. A begin would reset
+// them anyway, so the leak only shows through the one path that consumes them without a begin: a
+// stray commit. Left uncleared, it inherits the abandoned LoRa transaction's answer and reloads.
+static void test_abandonedTransaction_flagsDoNotLeakIntoAStrayCommit()
+{
+    usePresetLongFast();
+
+    sendBeginEdit();
+    meshtastic_Config c = meshtastic_Config_init_zero;
+    c.which_payload_variant = meshtastic_Config_lora_tag;
+    c.payload_variant.lora = config.lora;
+    c.payload_variant.lora.modem_preset = meshtastic_Config_LoRaConfig_ModemPreset_MEDIUM_FAST;
+    sendSetConfig(c);
+    testAdmin->ageEditTransaction();
+    sendGetDeviceMetadata(); // abandoned, not committed - this is where the flags must be cleared
+
+    // Start counting after the expiry, so only the stray commit's reload would register.
+    rebootAtMsec = 0;
+    ConfigChangedCounter counter;
+    counter.observe(&service->configChanged);
+
+    sendCommitEdit(); // no matching begin
+
+    TEST_ASSERT_EQUAL_INT(0, counter.count);
+    TEST_ASSERT_EQUAL_UINT32(0, rebootAtMsec);
+}
+
 // -----------------------------------------------------------------------
 // Test runner
 // -----------------------------------------------------------------------
@@ -2818,6 +2916,10 @@ void setup()
     RUN_TEST(test_transaction_liveOnlyBatch_neitherRebootsNorReloads);
     RUN_TEST(test_transaction_flagsDoNotLeakIntoTheNextTransaction);
     RUN_TEST(test_commitWithoutBegin_persistsButDoesNotReloadOrReboot);
+    RUN_TEST(test_abandonedTransaction_liveOnlyBatch_expiresWithoutReloadOrReboot);
+    RUN_TEST(test_abandonedTransaction_loraSet_stillReloadsRadioOnExpiry);
+    RUN_TEST(test_abandonedTransaction_rebootingFieldDoesNotRebootOnExpiry);
+    RUN_TEST(test_abandonedTransaction_flagsDoNotLeakIntoAStrayCommit);
 
     exit(UNITY_END());
 }