Preserve radio2 ACK ownership without extending expired leases

Keep tracked reply generations stable in feature-off builds as well as optional transport builds. After ACK copies drain, let expired temporary profiles restore saved settings while failed permanent commits stay staged and retry.

Cover packet generations, rollover, persistent commit faults, cancellation and eventual commit across feature-off, Meshtext and Ethernet configurations. Document ACK ownership and post-reply storage retry behavior.

docs/radio_profiles.md

@@ -71,10 +71,15 @@ copies finish, with at least one successfully transmitted. A reply that cannot
 be queued or whose copies all fail cancels the change. Queued replies have a
 five-minute deadline; an already-transmitting copy may finish under the normal
 radio watchdog. This confirms transmission, not reception by the remote client.
+An active temporary profile keeps its radio generation until those tracked
+reply copies finish or are retired, but its remaining lease continues counting
+down during that wait.
 
 Permanent remote changes are prepared in an uncommitted file before replying;
 the saved configuration is replaced only after reply transmission. If that
-replacement fails, the old channel remains active and the commit retries.
+replacement fails, the requested settings remain pending and the commit retries.
+An expired temporary profile returns to the last saved configuration during
+those post-reply storage retries; the uncommitted settings do not become active.
 Local `get radio2.status` reports this pending state. Roles without the tracked remote
 reply path require local USB for these immediate secondary-profile changes.
 

src/helpers/RadioProfileCLI.cpp

@@ -283,11 +283,9 @@ void RadioProfileCLI::publish() {
   if (external_secondary_override_) return;
 #endif
   if (publish_pending_ && (int32_t)(millis() - publish_after_ms_) < 0) return;
-#if MESH_ENABLE_MESHTEXT_CHAT || MESH_ENABLE_ETHERNET_TUNNEL
-  // An accepted remote command owns the old RX generation until its reply has
-  // completed (or its atomic storage commit has succeeded).
-  if (hasReplyMutation()) return;
-#endif
+  // Reply packets own the old RX generation until they finish. A post-ACK
+  // storage retry must not extend an expired temporary lease.
+  if (hasReplyMutation() && !remote_delivered_) return;
   publish_pending_ = false;
   RadioProfileConfig config = temp_active_ ? temporary_ : saved_;
 #if MESH_ENABLE_MESHTEXT_CHAT || MESH_ENABLE_ETHERNET_TUNNEL
@@ -547,9 +545,9 @@ void RadioProfileCLI::loop() {
   last_ms_ = now;
   remote_commit_retry_ms_ = elapsed >= remote_commit_retry_ms_ ? 0 : remote_commit_retry_ms_ - elapsed;
   if (remote_delivered_ && !remote_commit_retry_ms_ && !applyReplyMutation()) {
-    // Keep the accepted operation pending and the old channel live if the
-    // post-ACK atomic rename fails. Never report/apparently apply an unsaved
-    // tuple, and avoid a hot storage retry loop.
+    // Keep the accepted operation pending if the post-ACK atomic rename fails.
+    // Saved intent stays authoritative while expired leases can restore it;
+    // never apply an unsaved tuple or retry storage in a hot loop.
     remote_commit_retry_ms_ = 1000;
   }
   if (!hasReplyMutation() && temp_pending_ && (int32_t)(now - temp_start_ms_) >= 0) {

test/test_radio_reply_mutation.py

@@ -156,6 +156,164 @@ int main() {
 '''
 
 
+LEASE_REPLY = r'''
+// Reuse the memory filesystem/radio fixture with the production Dispatcher.
+#define main existing_cli_main
+#include "test/fixtures/radio_profiles/cli_test.cpp"
+#undef main
+#include <helpers/StaticPoolPacketManager.h>
+
+class LeaseMillis : public mesh::MillisecondClock {
+ public:
+  unsigned long getMillis() override { return g_mock_millis; }
+};
+class LeaseDispatcher : public mesh::Dispatcher {
+ public:
+  LeaseDispatcher(mesh::Radio& radio, mesh::MillisecondClock& clock,
+                  mesh::PacketManager& manager) : Dispatcher(radio, clock, manager) {}
+  mesh::DispatcherAction onRecvPacket(mesh::Packet*) override { return ACTION_RELEASE; }
+  using Dispatcher::isPacketRadioCurrent;
+};
+
+static void storageRetryLeases() {
+  unsigned scenarios = 0;
+  for (bool temporary : {false, true}) for (bool off : {false, true})
+  for (bool late_ack : {false, true}) for (bool cancel : {false, true})
+  for (bool rollover : {false, true}) {
+    if (!temporary && late_ack) continue;
+    Fixture f(true);
+    f.cmd("set radio2 910.5,500,8,5,rxtx"); f.advance(2000, false);
+    const auto saved = f.fs.files["/radio_profiles"];
+    if (rollover) { g_mock_millis = UINT32_MAX - 30000; f.cli.loop(); }
+    if (temporary) {
+      f.cmd("set tempradio2 911.5,500,8,5,rxtx,1"); f.advance(2000, false);
+      f.advance(59000, false);
+    }
+    const uint32_t generation = f.radio.p.generation[1];
+    f.cli.beginReplyCommand();
+    f.cmd(off ? "set radio2 off" : "set radio2 912.5,500,8,5,rxtx");
+    f.cli.endReplyCommand();
+    // Fail every final rename, while allowing the previous saved image to roll back.
+    f.fs.fail_rename_from.push_back("/radio_profiles.reply");
+    if (late_ack) {
+      f.advance(1000, false);
+      assert(f.cli.secondaryRemainingSeconds() == 0);
+      assert(f.radio.p.secondary_temporary && f.radio.p.generation[1] == generation);
+    }
+    assert(f.cli.finishReplyMutation(true));
+    assert(f.radio.p.generation[1] == generation); // callback cannot retune
+    f.advance(1, false);
+    assert(f.cli.hasReplyMutation());
+    assert(f.radio.p.secondary_temporary == (temporary && !late_ack));
+    assert(f.radio.p.secondary.params.freq == (temporary && !late_ack ? 911.5f : 910.5f));
+    if (temporary && !late_ack) {
+      assert(f.cli.secondaryRemainingSeconds() == 1);
+      f.advance(999, false);
+    }
+    assert(!f.radio.p.secondary_temporary && f.radio.p.secondary.params.freq == 910.5f);
+    assert(f.radio.p.generation[1] == generation + temporary);
+
+    // Persistent storage faults keep the mutation pending, but cannot extend the lease.
+    for (unsigned minute = 0; minute < 120; ++minute) {
+      f.advance(60000, false);
+      assert(f.cli.hasReplyMutation() && !f.radio.p.secondary_temporary);
+      assert(f.radio.p.secondary.params.freq == 910.5f);
+      assert(f.cli.savedSecondaryConfig().params.freq == 910.5f);
+      assert(f.fs.files["/radio_profiles"] == saved);
+    }
+    assert(strstr(f.cmd("get radio2.status"), "storage commit (retrying)"));
+    assert(strstr(f.cmd("set radio2 off", false), "acknowledgement pending"));
+    const uint32_t restored_generation = f.radio.p.generation[1];
+    if (cancel) assert(f.cli.finishReplyMutation(false));
+    else f.fs.fail_rename_from.clear();
+    f.advance(1000, false);
+    assert(!f.cli.hasReplyMutation() && !f.radio.p.secondary_temporary);
+    const auto expected_mode = !cancel && off ? mesh::RadioProfileMode::Off : mesh::RadioProfileMode::RxTx;
+    const float expected_freq = cancel ? 910.5f : off ? 0 : 912.5f;
+    assert(f.radio.p.secondary.mode == expected_mode && f.radio.p.secondary.params.freq == expected_freq);
+    assert(f.radio.p.generation[1] == restored_generation + !cancel);
+    assert((f.fs.files["/radio_profiles"] == saved) == cancel);
+    Radio reboot; mesh::RadioProfileCLI restored;
+    restored.begin(&f.fs, &reboot, &f.clock, true);
+    assert(reboot.p.secondary.mode == expected_mode && reboot.p.secondary.params.freq == expected_freq);
+    ++scenarios;
+  }
+  std::printf("Storage retry leases: chat=%d ethernet=%d, %u scenarios passed\n",
+      MESH_ENABLE_MESHTEXT_CHAT, MESH_ENABLE_ETHERNET_TUNNEL, scenarios);
+}
+
+int main() {
+  unsigned scenarios = 0;
+  for (bool delivered : {false, true}) for (bool rollover : {false, true}) {
+    Fixture f(true);
+    f.cmd("set radio2 910.5,500,8,5,rxtx"); f.advance(2000, false);
+    const auto saved = f.fs.files["/radio_profiles"];
+    if (rollover) { g_mock_millis = UINT32_MAX - 30000; f.cli.loop(); }
+    f.cmd("set tempradio2 911.5,500,8,5,rxtx,1"); f.advance(2000, false);
+    f.advance(59000, false);
+    assert(f.radio.p.secondary_temporary && f.cli.secondaryRemainingSeconds() == 1);
+    const uint32_t generation = f.radio.p.generation[1];
+
+    f.cli.beginReplyCommand();
+    f.cmd("set radio2 912.5,500,8,5,rxtx");
+    f.cli.endReplyCommand();
+    assert(f.cli.hasReplyMutation());
+    LeaseMillis ms;
+    StaticPoolPacketManager manager(4);
+    LeaseDispatcher dispatcher(f.radio, ms, manager);
+    dispatcher.begin();
+    mesh::Packet* reply = dispatcher.obtainNewPacket(); assert(reply);
+    reply->header = ROUTE_TYPE_DIRECT | (PAYLOAD_TYPE_RESPONSE << PH_TYPE_SHIFT);
+    reply->payload_len = 1; reply->payload[0] = 42;
+    reply->radio_profile = 1; reply->radio_generation = generation;
+    reply->radio_local = false; reply->radio_reply = true;
+    assert(dispatcher.sendPacket(reply, 0, 5000));
+    assert(manager.getOutboundTotal() == 2);
+    mesh::Packet* replies[2] = {manager.getOutboundByIdx(0), manager.getOutboundByIdx(1)};
+    uint8_t targets = 0;
+    for (auto* packet : replies) {
+      assert(packet->radio_bound && packet->radio_origin == 1);
+      assert(packet->radio_origin_generation == generation);
+      assert(dispatcher.isPacketRadioCurrent(packet));
+      targets |= 1U << packet->radio_profile;
+    }
+    assert(targets == 3);
+
+    // The lease expires while both acknowledgement copies still own the old generation.
+    for (uint32_t elapsed : {1000U, 10000U}) {
+      f.advance(elapsed, false);
+      assert(f.cli.hasReplyMutation() && f.cli.secondaryRemainingSeconds() == 0);
+      for (auto* packet : replies) assert(dispatcher.isPacketRadioCurrent(packet));
+      assert(f.radio.p.generation[1] == generation && f.radio.p.secondary_temporary);
+      assert(f.radio.p.secondary.params.freq == 911.5f);
+      assert(f.fs.files["/radio_profiles"] == saved);
+    }
+
+    // Completion/cancellation cannot publish while Dispatcher is in its callback.
+    assert(manager.removeOutboundByIdx(0) == replies[0]);
+    assert(manager.removeOutboundByIdx(0) == replies[1]);
+    assert(f.cli.finishReplyMutation(delivered));
+    assert(f.radio.p.generation[1] == generation);
+    for (auto* packet : replies) assert(dispatcher.isPacketRadioCurrent(packet));
+    f.advance(1, false);
+    assert(!f.cli.hasReplyMutation() && !f.radio.p.secondary_temporary);
+    assert(f.radio.p.generation[1] == generation + 1);
+    assert(f.radio.p.secondary.params.freq == (delivered ? 912.5f : 910.5f));
+    assert((f.fs.files["/radio_profiles"] == saved) == !delivered);
+    for (auto* packet : replies) {
+      assert(!dispatcher.isPacketRadioCurrent(packet));
+      dispatcher.releasePacket(packet);
+    }
+    assert(manager.getFreeCount() == 4);
+    ++scenarios;
+  }
+  std::printf("Lease reply ownership: chat=%d ethernet=%d, %u scenarios passed\n",
+      MESH_ENABLE_MESHTEXT_CHAT, MESH_ENABLE_ETHERNET_TUNNEL, scenarios);
+  storageRetryLeases();
+}
+'''
+
+
 class RadioReplyMutationTest(unittest.TestCase):
     @staticmethod
     def compiler_flags():
@@ -185,6 +343,30 @@ class RadioReplyMutationTest(unittest.TestCase):
                 env['ASAN_OPTIONS'] = env.get('ASAN_OPTIONS', '') + ':detect_leaks=0'
             subprocess.run([str(exe)], check=True, env=env)
 
+    def test_temporary_lease_expiry_preserves_reply_generations(self):
+        compiler = shutil.which('g++') or shutil.which('clang++')
+        self.assertIsNotNone(compiler)
+        with tempfile.TemporaryDirectory() as work:
+            cpp = Path(work) / 'lease_reply.cpp'
+            cpp.write_text(LEASE_REPLY, encoding='ascii')
+            env = os.environ.copy()
+            if os.name != 'nt':
+                env['ASAN_OPTIONS'] = env.get('ASAN_OPTIONS', '') + ':detect_leaks=0'
+            for chat, ethernet in ((0, 0), (1, 0), (0, 1)):
+                with self.subTest(chat=chat, ethernet=ethernet):
+                    exe = Path(work) / f'lease_reply-{chat}-{ethernet}.exe'
+                    result = subprocess.run([compiler, *self.compiler_flags(),
+                        f'-DMESH_ENABLE_MESHTEXT_CHAT={chat}',
+                        f'-DMESH_ENABLE_ETHERNET_TUNNEL={ethernet}',
+                        '-I', str(ROOT/'test/fixtures/radio_profiles/mocks'),
+                        '-I', str(ROOT/'test/mocks'), '-I', str(ROOT/'src'), '-I', str(ROOT),
+                        str(ROOT/'src/helpers/RadioProfileCLI.cpp'),
+                        str(ROOT/'src/Dispatcher.cpp'), str(ROOT/'src/Packet.cpp'),
+                        str(ROOT/'src/helpers/StaticPoolPacketManager.cpp'), str(cpp),
+                        '-o', str(exe)], capture_output=True, text=True)
+                    self.assertEqual(result.returncode, 0, result.stderr[-14000:])
+                    subprocess.run([str(exe)], check=True, env=env)
+
     def test_production_repeater_ack_callbacks_and_deadline(self):
         source = (ROOT/'examples/simple_repeater/MyMesh.cpp').read_text(encoding='utf-8')
         methods = '\n'.join(method(source, signature) for signature in (