From 9df586df7eb678eed92b692066c2fe14e4ba27b1 Mon Sep 17 00:00:00 2001 From: mikecarper Date: Fri, 16 Oct 2026 06:02:01 -0700 Subject: [PATCH] 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 | 7 +- src/helpers/RadioProfileCLI.cpp | 14 +-- test/test_radio_reply_mutation.py | 182 ++++++++++++++++++++++++++++++ 3 files changed, 194 insertions(+), 9 deletions(-) diff --git a/docs/radio_profiles.md b/docs/radio_profiles.md index 611631ca3..1113da913 100644 --- a/docs/radio_profiles.md +++ b/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. diff --git a/src/helpers/RadioProfileCLI.cpp b/src/helpers/RadioProfileCLI.cpp index a944bc16f..2f014bc2e 100644 --- a/src/helpers/RadioProfileCLI.cpp +++ b/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) { diff --git a/test/test_radio_reply_mutation.py b/test/test_radio_reply_mutation.py index cc86e0ae5..6fc29be1b 100644 --- a/test/test_radio_reply_mutation.py +++ b/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 + +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 (