fix(mesh): don't NAK an MQTT-only send that was already ACKed via the…

src/mesh/NextHopRouter.cpp

@@ -430,9 +430,15 @@ int32_t NextHopRouter::doRetransmissions()
         if (Throttle::deadlinePassedAt(now, p.nextTxMsec)) {
             if (p.numRetransmissions == 0) {
                 if (isFromUs(p.packet)) {
-                    LOG_DEBUG("Reliable send failed, return nak fr=0x%08x,to=0x%08x,id=0x%08x", p.packet->from, p.packet->to,
-                              p.packet->id);
-                    sendAckNak(meshtastic_Routing_Error_MAX_RETRANSMIT, getFrom(p.packet), p.packet->id, p.packet->channel);
+                    if (p.mqttAcked) {
+                        // The MQTT echo already handed the client a successful ACK for this id, so a NAK
+                        // now would only downgrade a message it has already shown as delivered.
+                        LOG_DEBUG("Retransmissions exhausted for 0x%08x but it was ACKed via MQTT, no nak", p.packet->id);
+                    } else {
+                        LOG_DEBUG("Reliable send failed, return nak fr=0x%08x,to=0x%08x,id=0x%08x", p.packet->from, p.packet->to,
+                                  p.packet->id);
+                        sendAckNak(meshtastic_Routing_Error_MAX_RETRANSMIT, getFrom(p.packet), p.packet->id, p.packet->channel);
+                    }
                 }
                 // Note: we don't stop retransmission here, instead the Nak packet gets processed in sniffReceived
                 stopRetransmission(it->first);

src/mesh/NextHopRouter.h

@@ -42,6 +42,13 @@ struct PendingPacket {
     /** Initial remaining retry count, used to detect whether a retry has fired. */
     uint8_t initialNumRetransmissions = 0;
 
+    /** We saw our own packet come back down from the MQTT broker we uplinked it to, which is a weak
+        delivery confirmation: the broker has it, but we have no evidence any LoRa node did. We keep
+        retransmitting over the air (MQTT reach is not mesh reach), but the originator has already been
+        told the send succeeded, so the terminal MAX_RETRANSMIT NAK must be suppressed - otherwise the
+        client flips a delivered message to failed. See ReliableRouter::sniffReceived. */
+    bool mqttAcked = false;
+
     PendingPacket() {}
     explicit PendingPacket(meshtastic_MeshPacket *p, uint8_t numRetransmissions);
 };

src/mesh/ReliableRouter.cpp

@@ -169,18 +169,34 @@ void ReliableRouter::sniffReceived(const meshtastic_MeshPacket *p, const meshtas
         PacketId nakId = (c && c->error_reason != meshtastic_Routing_Error_NONE) ? p->decoded.request_id : 0;
 
         // We intentionally don't check wasSeenRecently, because it is harmless to delete non existent retransmission records
-        if ((ackId || nakId) &&
-            // Implicit ACKs from MQTT should not stop retransmissions
-            !(isFromUs(p) && p->transport_mechanism == meshtastic_MeshPacket_TransportMechanism_TRANSPORT_MQTT)) {
-            LOG_DEBUG("Received a %s for 0x%08x, stopping retransmissions", ackId ? "ACK" : "NAK", ackId);
-            if (ackId) {
-                stopRetransmission(p->to, ackId);
-                // M3: an end-to-end ACK proves the directed route to the ACK's sender currently works,
-                // so clear its failure count and refresh freshness (keeps a good route pinned).
-                if (!isBroadcast(getFrom(p)))
-                    noteRouteSuccess(getFrom(p), millis());
+        if (ackId || nakId) {
+            // An ACK we generated for ourselves because our own packet came back down from the broker we
+            // uplinked it to (see onReceiveProto() in MQTT.cpp). The broker having the packet is not
+            // evidence that any LoRa node does, so this must not stop retransmissions. But the originator
+            // has already been handed a successful ACK, so remember it on the pending record and let
+            // doRetransmissions() suppress the terminal NAK - without this the client shows a delivered
+            // message and then flips it to failed once the retries run out, which on an MQTT-only mesh
+            // (no neighbors in range to implicitly ACK) is every single message.
+            const bool mqttSelfAck =
+                isFromUs(p) && p->transport_mechanism == meshtastic_MeshPacket_TransportMechanism_TRANSPORT_MQTT;
+
+            if (mqttSelfAck) {
+                // Only a success ACK counts; a NAK we minted for ourselves is not a delivery claim.
+                if (PendingPacket *record = ackId ? findPendingPacket(p->to, ackId) : nullptr) {
+                    LOG_DEBUG("Received an MQTT ACK for 0x%08x, keep retransmitting but suppress NAK", ackId);
+                    record->mqttAcked = true;
+                }
             } else {
-                stopRetransmission(p->to, nakId);
+                LOG_DEBUG("Received a %s for 0x%08x, stopping retransmissions", ackId ? "ACK" : "NAK", ackId);
+                if (ackId) {
+                    stopRetransmission(p->to, ackId);
+                    // M3: an end-to-end ACK proves the directed route to the ACK's sender currently works,
+                    // so clear its failure count and refresh freshness (keeps a good route pinned).
+                    if (!isBroadcast(getFrom(p)))
+                        noteRouteSuccess(getFrom(p), millis());
+                } else {
+                    stopRetransmission(p->to, nakId);
+                }
             }
         }
     }

src/mqtt/MQTT.cpp

@@ -118,7 +118,10 @@ inline void onReceiveProto(char *topic, byte *payload, size_t length)
     if (strcmp(e.gateway_id, nodeId.c_str()) == 0) {
         // Generate an implicit ACK towards ourselves (handled and processed only locally!) for this message.
         // We do this because packets are not rebroadcasted back into MQTT anymore and we assume that at least one node
-        // receives it when we get our own packet back. Then we'll stop our retransmissions.
+        // receives it when we get our own packet back. This ACK reaches the client, but deliberately does NOT stop
+        // LoRa retransmissions - the broker taking the packet is not evidence a mesh node heard it. ReliableRouter
+        // instead records it on the pending entry so the terminal MAX_RETRANSMIT NAK is suppressed once the retries
+        // run out; without that the client would show the message delivered and then flip it to failed.
         if (isFromUs(e.packet)) {
             auto pAck = routingModule->allocAckNak(meshtastic_Routing_Error_NONE, getFrom(e.packet), e.packet->id, ch.index);
             if (!pAck)

test/test_nexthop_routing/test_main.cpp

@@ -270,6 +270,13 @@ class ReliableRouterTestShim : public ReliableRouter
 
     void implicitAckForTest(const meshtastic_MeshPacket *p) { perhapsGenerateImplicitAckForOwnOverheard(p); }
 
+    bool mqttAckedForTest(NodeNum from, PacketId id)
+    {
+        PendingPacket *record = findPendingPacket(from, id);
+        TEST_ASSERT_NOT_NULL(record);
+        return record->mqttAcked;
+    }
+
     void clearPendingForTest()
     {
         while (!pending.empty())
@@ -822,6 +829,109 @@ void test_reliableAckStopsNormalPendingTransmission(void)
     TEST_ASSERT_EQUAL_UINT32(0, reliableShim->pendingCount());
 }
 
+// Builds the ACK that MQTT.cpp's onReceiveProto() mints locally when the broker echoes back a packet
+// we uplinked ourselves: addressed to us, sourced from us, and tagged TRANSPORT_MQTT.
+static meshtastic_MeshPacket makeMqttSelfAck(PacketId originalId, uint8_t channel)
+{
+    auto ack = makeBehaviorPacket(meshtastic_PortNum_ROUTING_APP, kLocalNode, kLocalNode, channel);
+    ack.decoded.request_id = originalId;
+    ack.hop_limit = 0;
+    ack.hop_start = 0;
+    ack.relay_node = 0;
+    ack.transport_mechanism = meshtastic_MeshPacket_TransportMechanism_TRANSPORT_MQTT;
+    return ack;
+}
+
+// An MQTT-only mesh (uplink/downlink on, no neighbor in range) is the whole point of this pair of
+// tests. The broker echo is not proof any LoRa node heard us, so retransmissions must continue - but
+// the client was already handed a successful ACK, so the terminal NAK has to be suppressed. Emitting
+// both is what turned every message on such a mesh into "Failed to deliver to mesh" after showing as
+// delivered first.
+void test_mqtt_self_ack_keeps_retransmitting(void)
+{
+    auto original = makeBehaviorPacket(meshtastic_PortNum_TEXT_MESSAGE_APP, kLocalNode, NODENUM_BROADCAST, 0, /*wantAck=*/true);
+    reliableShim->seedRetry(original, NextHopRouter::NUM_RELIABLE_RETX);
+    TEST_ASSERT_EQUAL_UINT32(1, reliableShim->pendingCount());
+    mockRoutingModule->ackNaks.clear();
+    reliableRadio->reset();
+
+    auto ack = makeMqttSelfAck(original.id, original.channel);
+    meshtastic_Routing routing = meshtastic_Routing_init_zero;
+    routing.error_reason = meshtastic_Routing_Error_NONE;
+
+    reliableShim->sniffForTest(&ack, &routing);
+
+    // Still pending, and flagged so the terminal NAK is suppressed later.
+    TEST_ASSERT_EQUAL_UINT32(1, reliableShim->pendingCount());
+    TEST_ASSERT_TRUE(reliableShim->mqttAckedForTest(kLocalNode, original.id));
+
+    // The next scheduled retry still goes out over the air.
+    reliableShim->makeRetryDue(kLocalNode, original.id);
+    reliableShim->runDueRetries();
+    TEST_ASSERT_EQUAL_UINT32(1, reliableRadio->sentPackets.size());
+
+    reliableShim->clearPendingForTest();
+}
+
+void test_mqtt_self_ack_suppresses_final_nak(void)
+{
+    auto original = makeBehaviorPacket(meshtastic_PortNum_TEXT_MESSAGE_APP, kLocalNode, NODENUM_BROADCAST, 0, /*wantAck=*/true);
+    // attempts=1 leaves zero retries, so the next due run takes the give-up branch.
+    reliableShim->seedRetry(original, /*attempts=*/1);
+    mockRoutingModule->ackNaks.clear();
+
+    auto ack = makeMqttSelfAck(original.id, original.channel);
+    meshtastic_Routing routing = meshtastic_Routing_init_zero;
+    routing.error_reason = meshtastic_Routing_Error_NONE;
+    reliableShim->sniffForTest(&ack, &routing);
+    mockRoutingModule->ackNaks.clear(); // ignore anything the sniff itself emitted
+
+    reliableShim->makeRetryDue(kLocalNode, original.id);
+    reliableShim->runDueRetries();
+
+    // Record is reaped as usual, but no MAX_RETRANSMIT reaches the client.
+    TEST_ASSERT_EQUAL_UINT32(0, reliableShim->pendingCount());
+    TEST_ASSERT_EQUAL_UINT32(0, mockRoutingModule->ackNaks.size());
+}
+
+// Control for the two above: without the MQTT echo, giving up must still NAK, otherwise a genuinely
+// undelivered message would silently look fine.
+void test_unacked_broadcast_still_naks(void)
+{
+    auto original = makeBehaviorPacket(meshtastic_PortNum_TEXT_MESSAGE_APP, kLocalNode, NODENUM_BROADCAST, 0, /*wantAck=*/true);
+    reliableShim->seedRetry(original, /*attempts=*/1);
+    mockRoutingModule->ackNaks.clear();
+
+    reliableShim->makeRetryDue(kLocalNode, original.id);
+    reliableShim->runDueRetries();
+
+    TEST_ASSERT_EQUAL_UINT32(0, reliableShim->pendingCount());
+    TEST_ASSERT_EQUAL_UINT32(1, mockRoutingModule->ackNaks.size());
+    const auto &nak = mockRoutingModule->ackNaks.front();
+    TEST_ASSERT_EQUAL(meshtastic_Routing_Error_MAX_RETRANSMIT, std::get<0>(nak));
+    TEST_ASSERT_EQUAL_UINT32(kLocalNode, std::get<1>(nak));
+    TEST_ASSERT_EQUAL_UINT32(original.id, std::get<2>(nak));
+}
+
+// The suppression is keyed on the ACK being one we minted for ourselves. A real ACK that merely
+// arrived over MQTT from another node is end-to-end proof of delivery and must still stop retries.
+void test_foreign_ack_over_mqtt_still_stops_retransmission(void)
+{
+    auto original = makeBehaviorPacket(meshtastic_PortNum_TEXT_MESSAGE_APP, kLocalNode, kRemoteNode, 1, /*wantAck=*/true);
+    reliableShim->seedRetry(original, NextHopRouter::NUM_RELIABLE_RETX);
+    TEST_ASSERT_EQUAL_UINT32(1, reliableShim->pendingCount());
+
+    auto ack = makeBehaviorPacket(meshtastic_PortNum_ROUTING_APP, kRemoteNode, kLocalNode, 1);
+    ack.decoded.request_id = original.id;
+    ack.transport_mechanism = meshtastic_MeshPacket_TransportMechanism_TRANSPORT_MQTT;
+    meshtastic_Routing routing = meshtastic_Routing_init_zero;
+    routing.error_reason = meshtastic_Routing_Error_NONE;
+
+    reliableShim->sniffForTest(&ack, &routing);
+
+    TEST_ASSERT_EQUAL_UINT32(0, reliableShim->pendingCount());
+}
+
 // A PKI DM we originated is encrypted to the recipient, so when we overhear it being rebroadcast we
 // cannot decode it. The routing auth gate classifies it opaque and returns before
 // shouldFilterReceived() runs, so the implicit ACK has to be reachable from the header alone -
@@ -1102,6 +1212,10 @@ void setup()
     RUN_TEST(test_reliableAckStopsNormalPendingTransmission);
 
     printf("\n=== pending retransmission bookkeeping ===\n");
+    RUN_TEST(test_mqtt_self_ack_keeps_retransmitting);
+    RUN_TEST(test_mqtt_self_ack_suppresses_final_nak);
+    RUN_TEST(test_unacked_broadcast_still_naks);
+    RUN_TEST(test_foreign_ack_over_mqtt_still_stops_retransmission);
     RUN_TEST(test_implicit_ack_for_opaque_own_packet);
     RUN_TEST(test_implicit_ack_ignores_foreign_pkt);
     RUN_TEST(test_pending_does_not_cancel_radio_queue_before_first_retry);