fix(beacon): guard the radio switch/restore against re-entry and early restore

Two checks in reconfigureForBeaconTX(), both independent of radio state, so
the switch/restore state machine no longer rests on sendingPacket's lifetime -
which is exactly the implicit coupling that let #11573 through.

A re-entrancy guard. Both branches end in iface->reconfigure(), whose
setStandby() runs completeSending(), which calls straight back in here. While
one call is applying a config, a nested call returns false and leaves it
alone. This covers the switch branch too, which had the same exposure with a
quieter symptom: a second switch before the restore would take the re-entrant
call as a restore and undo the switch still being applied, sending the beacon
on the home channel instead of its target.

A restore gate. The restore now waits for the packet that armed the switch to
actually finish, tracked by id against our own target table rather than by
asking the radio. Every caller that completes or abandons a beacon clears that
packet's target settings first, so a live entry means the TX has not happened
yet. cancelSending() now clears too, which is what keeps a cancelled beacon
from pinning the radio on the beacon config.

Together these make explicit the invariant completeSending()'s if (p) block
was carrying by accident: a future hoist of that call gets a logged no-op
instead of a crash and a misdirected beacon.

Also sets radioSwitched before reconfigure() rather than after, in both
branches, so the flag never describes a radio state that is not yet true.

Diagnostics, because every step of this dance was previously silent about its
own state. Count consecutive switches with no restore between them and log the
depth on both sides, so a change-change-change-restore run reads off the log;
switch #2 onwards prints the held home snapshot, which is the value that has
to survive a second switch. The restore names the config it is restoring to,
so a stale snapshot is visible directly. The re-entrancy guard logs when it
fires - expected exactly twice per beacon, so a burst means something new is
re-entering rather than a silent reboot. And setTargetRadioSettings() now
warns on the slot eviction that previously left a packet to key up on whatever
config was running - no crash, no log, wrong channel.

Reachable only with beacon broadcast enabled (the default flags are
LISTEN_ENABLED | LEGACY_SPLIT, so broadcast is off) and a target differing
from the running config; an identical target takes the early return and never
switches.

Tests: three re-entrancy cases against a RadioInterface whose reconfigure()
re-enters exactly as completeSending() does - bounded, so a regression fails
an assertion instead of overflowing the stack and taking the runner with it -
plus a restore that must defer until the beacon it switched for completes.

src/mesh/RadioLibInterface.cpp

@@ -248,8 +248,14 @@ bool RadioLibInterface::isSending()
 bool RadioLibInterface::cancelSending(NodeNum from, PacketId id)
 {
     auto p = txQueue.remove(from, id);
-    if (p)
+    if (p) {
+#if !MESHTASTIC_EXCLUDE_BEACON
+        // Every path that abandons a queued packet clears its beacon target first; the restore is
+        // gated on no target being live, so a leftover entry would pin the radio on the beacon config.
+        MeshBeaconModule::clearTargetRadioSettings(p);
+#endif
         packetPool.release(p); // free the packet we just removed
+    }
 
     bool result = (p != NULL);
     LOG_DEBUG("cancelSending id=0x%08x, removed=%d", id, result);

src/modules/MeshBeaconModule.cpp

@@ -46,6 +46,16 @@ static bool getTargetRadioSettings(const meshtastic_MeshPacket *p, meshtastic_Co
     return false;
 }
 
+// Is a target entry still live for this packet id? Unlike sendingPacket or the radio's standby
+// state, this is our own bookkeeping - it answers "has that beacon finished" without asking the radio.
+static bool targetRadioSettingsLive(uint32_t id)
+{
+    for (const auto &entry : targetRadioSettings)
+        if (entry.inUse && entry.id == id)
+            return true;
+    return false;
+}
+
 // ---------------------------------------------------------------------------
 // MeshBeaconModule base
 // ---------------------------------------------------------------------------
@@ -74,8 +84,13 @@ void MeshBeaconModule::setTargetRadioSettings(const meshtastic_MeshPacket *p, me
         if (!target && !entry.inUse)
             target = &entry;
     }
-    if (!target)
+    if (!target) {
+        // All slots live: another beacon's target is about to be overwritten and that packet will key
+        // up on whatever config is running instead of its own.
+        LOG_WARN("Beacon: target table full (%u slots), evicting packet 0x%08x for 0x%08x",
+                 (unsigned)(sizeof(targetRadioSettings) / sizeof(targetRadioSettings[0])), targetRadioSettings[0].id, p->id);
         target = &targetRadioSettings[0];
+    }
     target->inUse = true;
     target->id = p->id;
     target->preset = preset;
@@ -151,14 +166,32 @@ meshtastic_ChannelSettings MeshBeaconModule::beaconChannelSettings(const meshtas
 
 bool MeshBeaconModule::reconfigureForBeaconTX(RadioInterface *iface, meshtastic_MeshPacket *p)
 {
-    // True while a beacon radio switch is in effect and still needs undoing. We track the switch
-    // explicitly rather than inferring it from "live config differs from the snapshot", because that
-    // heuristic both missed cases (a channel name/PSK swap that left preset/slot/region unchanged would
-    // never be restored) and fired falsely (a legitimate non-beacon channel edit would be reverted on
-    // the next TX). With the flag the restore fires for ANY field we changed and only when we changed
-    // it - including on TX-failure paths, which route through this same restore call.
+    // The four statics below hold the switch state explicitly. Inferring it from "live config differs
+    // from the snapshot" instead missed name/PSK-only swaps and fired on legitimate channel edits.
     static bool radioSwitched = false;
 
+    // Consecutive switches with no restore between them, so a multi-target run can be read off the log
+    // and the held home snapshot is attributable to a specific switch.
+    static uint8_t switchDepth = 0;
+
+    // The packet that armed the outstanding switch. Every caller that abandons or completes a beacon
+    // clears its target settings first, so a live entry here means that TX has not finished yet.
+    static uint32_t switchedForId = 0;
+
+    // Both branches end in iface->reconfigure(), whose setStandby() runs completeSending() and calls
+    // straight back in here. Ignore that re-entry: the outer call owns the config it is applying.
+    static bool applying = false;
+    if (applying) {
+        // Expected once per switch and once per restore. A burst of these means something new re-enters.
+        LOG_DEBUG("Beacon: ignore re-entrant reconfigure while a radio config is being applied");
+        return false;
+    }
+    struct ApplyingScope {
+        bool &flag;
+        explicit ApplyingScope(bool &f) : flag(f) { flag = true; }
+        ~ApplyingScope() { flag = false; }
+    } applyingScope(applying);
+
     meshtastic_ChannelSettings *primaryCh = &channels.getByIndex(channels.getPrimaryIndex()).settings;
     meshtastic_Config_LoRaConfig_ModemPreset targetPreset;
     uint16_t targetSlot;
@@ -202,18 +235,22 @@ bool MeshBeaconModule::reconfigureForBeaconTX(RadioInterface *iface, meshtastic_
             return false;
         }
 
-        // Snapshot current (non-beacon) settings so we restore to the latest config. Skip while a
-        // switch is already active, so a second switch before the restore can't capture the beacon
-        // config as the "home" we later restore to.
+        // Snapshot the live (non-beacon) config as "home". Skipped while a switch is already active,
+        // so a second switch before the restore cannot capture the beacon config instead.
         if (!radioSwitched) {
             originalModemPreset = config.lora.modem_preset;
             originalLoraChannel = config.lora.channel_num;
             originalRegion = config.lora.region;
             originalPrimaryChannel = *primaryCh;
+            switchDepth = 0;
         }
+        switchDepth++;
 
-        LOG_INFO("Beacon: switch radio for packet 0x%08x to preset=%d slot=%u region=%d", p->id, targetPreset, targetSlot,
-                 targetRegion);
+        LOG_INFO("Beacon: switch #%u radio for packet 0x%08x to preset=%d slot=%u region=%d", switchDepth, p->id, targetPreset,
+                 targetSlot, targetRegion);
+        if (switchDepth > 1)
+            LOG_WARN("Beacon: switching again with no restore between; home preset=%d slot=%u region=%d still held",
+                     originalModemPreset, originalLoraChannel, originalRegion);
         config.lora.modem_preset = targetPreset;
         config.lora.channel_num = targetSlot;
         if (targetRegion != config.lora.region)
@@ -222,13 +259,22 @@ bool MeshBeaconModule::reconfigureForBeaconTX(RadioInterface *iface, meshtastic_
 
         channels.fixupChannel(channels.getPrimaryIndex());
         p->channel = channels.getHash(channels.getPrimaryIndex());
+        radioSwitched = true; // set before reconfigure(), so the flag never lags the radio it describes
+        switchedForId = p->id;
         iface->reconfigure();
-        radioSwitched = true;
         return true;
 
     } else if ((!p || !getTargetRadioSettings(p, nullptr, nullptr)) && radioSwitched) {
 
-        LOG_INFO("Beacon: restore radio config after TX");
+        // Only restore once the beacon that armed the switch has finished; a caller arriving here on
+        // a radio state change would put the home config back under a beacon that has not keyed up.
+        if (targetRadioSettingsLive(switchedForId)) {
+            LOG_DEBUG("Beacon: skip restore, packet 0x%08x has not finished sending", switchedForId);
+            return false;
+        }
+
+        LOG_INFO("Beacon: restore radio config after TX, undoing %u switch(es) -> preset=%d slot=%u region=%d", switchDepth,
+                 originalModemPreset, originalLoraChannel, originalRegion);
         config.lora.modem_preset = originalModemPreset;
         config.lora.channel_num = originalLoraChannel;
         config.lora.region = originalRegion;
@@ -236,8 +282,10 @@ bool MeshBeaconModule::reconfigureForBeaconTX(RadioInterface *iface, meshtastic_
         primaryCh->name[sizeof(primaryCh->name) - 1] = '\0';
 
         channels.fixupChannel(channels.getPrimaryIndex());
+        radioSwitched = false; // cleared before reconfigure(), so the flag never lags the radio it describes
+        switchDepth = 0;
+        switchedForId = 0;
         iface->reconfigure();
-        radioSwitched = false;
         return true;
     }
     return false;

test/test_mesh_beacon/test_main.cpp

@@ -1338,6 +1338,178 @@ static void test_broadcaster_distinctTargets_bothSent(void)
     TEST_ASSERT_EQUAL_UINT32_MESSAGE(2, mockRouter->sentPackets.size(), "distinct targets must each be sent");
 }
 
+// ---------------------------------------------------------------------------
+// Radio switch/restore re-entrancy
+// ---------------------------------------------------------------------------
+
+/**
+ * Stands in for a real driver on the restore path. RadioLibInterface::reconfigure() standbys the
+ * chip, setStandby() calls completeSending(), and completeSending() calls back into
+ * reconfigureForBeaconTX(iface, nullptr) - so reconfigure() re-entering is the normal case, not an
+ * exotic one. Bounded, so a regression fails an assertion instead of overflowing the stack.
+ */
+class ReentrantRadioInterface : public RadioInterface
+{
+  public:
+    static constexpr int kReentryLimit = 16;
+    int reconfigureCalls = 0;
+    bool reenterOnReconfigure = false;
+
+    ErrorCode send(meshtastic_MeshPacket *p) override
+    {
+        packetPool.release(p);
+        return ERRNO_OK;
+    }
+
+    uint32_t getPacketTime(uint32_t totalPacketLen, bool received = false) override
+    {
+        (void)totalPacketLen;
+        (void)received;
+        return 0;
+    }
+
+    bool reconfigure() override
+    {
+        reconfigureCalls++;
+        if (reenterOnReconfigure && reconfigureCalls < kReentryLimit)
+            MeshBeaconModule::reconfigureForBeaconTX(this, nullptr);
+        return true;
+    }
+};
+
+/**
+ * The restore must clear its guard before reconfiguring, or completeSending() re-enters the restore
+ * branch and it reconfigures the radio once per level until the stack runs out. Seen in the field as
+ * a run of "Beacon: restore radio config after TX" with a full applyModemConfig() between each.
+ */
+static void test_beaconRestore_isNotReenteredByCompleteSending(void)
+{
+    resetConfig();
+    static const uint8_t homePsk[16] = {0xAA, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07,
+                                        0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f};
+    installTestPrimaryChannel("Home", homePsk, sizeof(homePsk));
+
+    ReentrantRadioInterface radio;
+    meshtastic_MeshPacket pkt = meshtastic_MeshPacket_init_zero;
+    pkt.id = 0x5EED0001;
+    MeshBeaconModule::setTargetRadioSettings(&pkt, meshtastic_Config_LoRaConfig_ModemPreset_LONG_SLOW, 0, false,
+                                             meshtastic_Config_LoRaConfig_RegionCode_UNSET, false, nullptr);
+
+    // Switch to the beacon config. Not the case under test, so leave re-entry off.
+    TEST_ASSERT_TRUE_MESSAGE(MeshBeaconModule::reconfigureForBeaconTX(&radio, &pkt), "beacon switch should have applied");
+    TEST_ASSERT_EQUAL_INT_MESSAGE(meshtastic_Config_LoRaConfig_ModemPreset_LONG_SLOW, config.lora.modem_preset,
+                                  "switch must leave the radio on the beacon preset");
+
+    // Now restore, with reconfigure() re-entering exactly as completeSending() does.
+    MeshBeaconModule::clearTargetRadioSettings(&pkt);
+    radio.reconfigureCalls = 0;
+    radio.reenterOnReconfigure = true;
+    TEST_ASSERT_TRUE_MESSAGE(MeshBeaconModule::reconfigureForBeaconTX(&radio, nullptr), "restore should have applied");
+
+    TEST_ASSERT_EQUAL_INT_MESSAGE(1, radio.reconfigureCalls, "restore must reconfigure the radio exactly once");
+    TEST_ASSERT_EQUAL_INT_MESSAGE(meshtastic_Config_LoRaConfig_ModemPreset_LONG_FAST, config.lora.modem_preset,
+                                  "restore must put the home preset back");
+    TEST_ASSERT_EQUAL_STRING_MESSAGE("Home", channels.getByIndex(channels.getPrimaryIndex()).settings.name,
+                                     "restore must put the home channel back");
+}
+
+/**
+ * A second switch before the restore has run must survive the same re-entry. completeSending() calls
+ * in with a null packet, which reads as "restore" - so without the guard it would undo the switch that
+ * is still being applied, leaving the beacon to transmit on the home channel instead of its target.
+ */
+static void test_beaconSwitch_isNotUndoneByCompleteSending(void)
+{
+    resetConfig();
+    static const uint8_t homePsk[16] = {0xAA, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07,
+                                        0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f};
+    installTestPrimaryChannel("Home", homePsk, sizeof(homePsk));
+
+    ReentrantRadioInterface radio;
+    meshtastic_MeshPacket first = meshtastic_MeshPacket_init_zero;
+    first.id = 0x5EED0002;
+    MeshBeaconModule::setTargetRadioSettings(&first, meshtastic_Config_LoRaConfig_ModemPreset_LONG_SLOW, 0, false,
+                                             meshtastic_Config_LoRaConfig_RegionCode_UNSET, false, nullptr);
+    TEST_ASSERT_TRUE_MESSAGE(MeshBeaconModule::reconfigureForBeaconTX(&radio, &first), "first switch should have applied");
+
+    // Second switch with the restore still outstanding, and reconfigure() re-entering.
+    meshtastic_MeshPacket second = meshtastic_MeshPacket_init_zero;
+    second.id = 0x5EED0003;
+    MeshBeaconModule::setTargetRadioSettings(&second, meshtastic_Config_LoRaConfig_ModemPreset_SHORT_FAST, 0, false,
+                                             meshtastic_Config_LoRaConfig_RegionCode_UNSET, false, nullptr);
+    radio.reconfigureCalls = 0;
+    radio.reenterOnReconfigure = true;
+    TEST_ASSERT_TRUE_MESSAGE(MeshBeaconModule::reconfigureForBeaconTX(&radio, &second), "second switch should have applied");
+
+    TEST_ASSERT_EQUAL_INT_MESSAGE(1, radio.reconfigureCalls, "second switch must reconfigure the radio exactly once");
+    TEST_ASSERT_EQUAL_INT_MESSAGE(meshtastic_Config_LoRaConfig_ModemPreset_SHORT_FAST, config.lora.modem_preset,
+                                  "second switch must not be undone mid-flight");
+
+    // The home config must still be recoverable afterwards - the snapshot survives a second switch.
+    radio.reenterOnReconfigure = false;
+    MeshBeaconModule::clearTargetRadioSettings(&first);
+    MeshBeaconModule::clearTargetRadioSettings(&second);
+    TEST_ASSERT_TRUE_MESSAGE(MeshBeaconModule::reconfigureForBeaconTX(&radio, nullptr), "restore should have applied");
+    TEST_ASSERT_EQUAL_INT_MESSAGE(meshtastic_Config_LoRaConfig_ModemPreset_LONG_FAST, config.lora.modem_preset,
+                                  "restore must return to the home preset, not the first beacon target");
+    TEST_ASSERT_EQUAL_STRING_MESSAGE("Home", channels.getByIndex(channels.getPrimaryIndex()).settings.name,
+                                     "restore must return to the home channel");
+}
+
+/** A restore with nothing switched must do nothing at all - the guard is what makes re-entry safe. */
+static void test_beaconRestore_withoutSwitch_isNoOp(void)
+{
+    resetConfig();
+    ReentrantRadioInterface radio;
+
+    // reconfigureForBeaconTX() keeps its switched/not-switched state in a function-local static, so an
+    // earlier test that aborted mid-way can leave a switch outstanding. Drain it before asserting.
+    MeshBeaconModule::reconfigureForBeaconTX(&radio, nullptr);
+
+    radio.reconfigureCalls = 0;
+    radio.reenterOnReconfigure = true;
+    TEST_ASSERT_FALSE_MESSAGE(MeshBeaconModule::reconfigureForBeaconTX(&radio, nullptr), "restore without a switch is a no-op");
+    TEST_ASSERT_EQUAL_INT_MESSAGE(0, radio.reconfigureCalls, "no-op restore must not touch the radio");
+}
+
+/**
+ * The restore is driven by "our beacon finished", not by "the radio changed state". completeSending()
+ * clears a packet's target settings before restoring, so a caller that arrives without that - the
+ * pre-TX channel scan standbys the radio, and setStandby() calls completeSending() - must be refused.
+ * Otherwise the home config goes back under a beacon that has not keyed up yet, and it transmits on
+ * the wrong preset with the beacon channel hash already stamped on it.
+ */
+static void test_beaconRestore_deferredUntilPacketCompletes(void)
+{
+    resetConfig();
+    static const uint8_t homePsk[16] = {0xAA, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07,
+                                        0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f};
+    installTestPrimaryChannel("Home", homePsk, sizeof(homePsk));
+
+    ReentrantRadioInterface radio;
+    meshtastic_MeshPacket pkt = meshtastic_MeshPacket_init_zero;
+    pkt.id = 0x5EED0004;
+    MeshBeaconModule::setTargetRadioSettings(&pkt, meshtastic_Config_LoRaConfig_ModemPreset_LONG_SLOW, 0, false,
+                                             meshtastic_Config_LoRaConfig_RegionCode_UNSET, false, nullptr);
+    TEST_ASSERT_TRUE_MESSAGE(MeshBeaconModule::reconfigureForBeaconTX(&radio, &pkt), "beacon switch should have applied");
+
+    // The packet has not been sent yet, so its target settings are still live.
+    radio.reconfigureCalls = 0;
+    TEST_ASSERT_FALSE_MESSAGE(MeshBeaconModule::reconfigureForBeaconTX(&radio, nullptr),
+                              "restore must be refused while the beacon is still outstanding");
+    TEST_ASSERT_EQUAL_INT_MESSAGE(0, radio.reconfigureCalls, "a refused restore must not touch the radio");
+    TEST_ASSERT_EQUAL_INT_MESSAGE(meshtastic_Config_LoRaConfig_ModemPreset_LONG_SLOW, config.lora.modem_preset,
+                                  "the beacon preset must still be in place when the packet keys up");
+
+    // completeSending() clears the target settings first; only then is the restore ours to make.
+    MeshBeaconModule::clearTargetRadioSettings(&pkt);
+    TEST_ASSERT_TRUE_MESSAGE(MeshBeaconModule::reconfigureForBeaconTX(&radio, nullptr), "restore should have applied");
+    TEST_ASSERT_EQUAL_INT_MESSAGE(meshtastic_Config_LoRaConfig_ModemPreset_LONG_FAST, config.lora.modem_preset,
+                                  "restore must put the home preset back");
+    TEST_ASSERT_EQUAL_STRING_MESSAGE("Home", channels.getByIndex(channels.getPrimaryIndex()).settings.name,
+                                     "restore must put the home channel back");
+}
+
 } // namespace
 
 // ===========================================================================
@@ -1462,6 +1634,13 @@ BEACON_TEST_ENTRY void setup()
     RUN_TEST(test_broadcaster_duplicateTargets_dedupedToOnePacket);
     RUN_TEST(test_broadcaster_distinctTargets_bothSent);
 
+    printf("\n=== Radio switch/restore re-entrancy ===\n");
+
+    RUN_TEST(test_beaconRestore_isNotReenteredByCompleteSending);
+    RUN_TEST(test_beaconSwitch_isNotUndoneByCompleteSending);
+    RUN_TEST(test_beaconRestore_withoutSwitch_isNoOp);
+    RUN_TEST(test_beaconRestore_deferredUntilPacketCompletes);
+
     exit(UNITY_END());
 }