From 367e2377674146ae3d62c64096c6a057e214a833 Mon Sep 17 00:00:00 2001 From: James Rich <2199651+jamesarich@users.noreply.github.com> Date: Fri, 23 Oct 2026 07:20:56 -0500 Subject: [PATCH 1/2] fix: surface routing NAKs on remote admin and node requests A request the target or the mesh answers with a routing NAK was shown as unreachable after a timeout, or never answered at all. Remote admin to a node that refuses our key said "Could not reach node"; traceroute, telemetry, position, user info, neighbor info and metadata requests just sat on "Requesting". `MeshPacket.routingNakOrNull` reads a NAK off `meshPacketFlow`. The remote admin session ensure ends as `Rejected` when one answers its metadata request, and the snackbar names the refusal when the target sent it. `RequestNakWatcher` shows " from failed: " for the node requests. Awaited admin sends no longer treat a relay's implicit ACK as delivery: an ACK completes the wait only when it comes from the destination. fixes #7732 --- .../core/data/manager/CommandSenderImpl.kt | 6 +- .../core/data/manager/MeshDataHandlerImpl.kt | 7 +- .../core/data/manager/PacketHandlerImpl.kt | 5 +- .../core/data/manager/MeshDataHandlerTest.kt | 6 +- .../data/manager/PacketHandlerImplTest.kt | 32 ++++++ .../EnsureRemoteAdminSessionUseCase.kt | 66 +++++++------ .../usecase/session/EnsureSessionResult.kt | 5 + .../EnsureRemoteAdminSessionUseCaseTest.kt | 98 +++++++++++++++++-- .../org/meshtastic/core/model/RoutingNak.kt | 36 +++++++ .../meshtastic/core/model/RoutingNakTest.kt | 71 ++++++++++++++ .../core/repository/CommandSender.kt | 4 +- .../core/repository/PacketHandler.kt | 5 +- .../core/repository/QueryController.kt | 12 +-- .../composeResources/values/strings.xml | 1 + .../core/service/QueryControllerImpl.kt | 12 +-- .../core/testing/FakeCommandSender.kt | 4 +- .../core/testing/FakeRadioController.kt | 6 +- .../node/component/AdministrationSection.kt | 2 +- .../node/detail/CommonNodeRequestActions.kt | 17 ++-- .../feature/node/detail/HandleNodeAction.kt | 2 +- .../node/detail/NodeDetailViewModel.kt | 34 ++++++- .../feature/node/detail/RequestNakWatcher.kt | 75 ++++++++++++++ .../feature/node/model/NodeDetailAction.kt | 2 +- .../detail/CommonNodeRequestActionsTest.kt | 5 +- .../node/detail/HandleNodeActionTest.kt | 1 + .../node/detail/NodeDetailViewModelTest.kt | 27 ++++- .../node/detail/RecordingSnackbarManager.kt | 4 + .../node/detail/RequestNakWatcherTest.kt | 98 +++++++++++++++++++ .../feature/node/detail/TestNakWatcher.kt | 66 +++++++++++++ .../detail/NodeDetailCompassLifecycleTest.kt | 1 + 30 files changed, 625 insertions(+), 85 deletions(-) create mode 100644 core/model/src/commonMain/kotlin/org/meshtastic/core/model/RoutingNak.kt create mode 100644 core/model/src/commonTest/kotlin/org/meshtastic/core/model/RoutingNakTest.kt create mode 100644 feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/detail/RequestNakWatcher.kt create mode 100644 feature/node/src/commonTest/kotlin/org/meshtastic/feature/node/detail/RequestNakWatcherTest.kt create mode 100644 feature/node/src/commonTest/kotlin/org/meshtastic/feature/node/detail/TestNakWatcher.kt diff --git a/core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/CommandSenderImpl.kt b/core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/CommandSenderImpl.kt index 7b51d0d1b54..ae70f80f5db 100644 --- a/core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/CommandSenderImpl.kt +++ b/core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/CommandSenderImpl.kt @@ -271,7 +271,7 @@ class CommandSenderImpl( } } - override suspend fun requestPosition(destNum: Int, currentPosition: Position) { + override suspend fun requestPosition(destNum: Int, currentPosition: Position, requestId: Int) { // Firmware coarsens a phone-originated position to the channel precision, so an explicit 0,0 goes out as a // real-looking coordinate. Leave the fields absent when we have none. val meshPosition = @@ -288,6 +288,7 @@ class CommandSenderImpl( enqueueOrThrow( buildMeshPacket( to = destNum, + id = requestId.nonZeroRequestId(), channel = getChannelIndex(destNum), priority = MeshPacket.Priority.BACKGROUND, decoded = @@ -317,12 +318,13 @@ class CommandSenderImpl( } } - override suspend fun requestUserInfo(destNum: Int) { + override suspend fun requestUserInfo(destNum: Int, requestId: Int) { val myNum = nodeManager.myNodeNum.value ?: throw LocalNodeUnavailableException("User-info request") val myNode = nodeManager.nodeDBbyNodeNum[myNum] ?: throw LocalNodeUnavailableException("User-info request") enqueueOrThrow( buildMeshPacket( to = destNum, + id = requestId.nonZeroRequestId(), channel = getChannelIndex(destNum), decoded = Data.Builder() diff --git a/core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/MeshDataHandlerImpl.kt b/core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/MeshDataHandlerImpl.kt index 37696ec93b7..0ebed3c9905 100644 --- a/core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/MeshDataHandlerImpl.kt +++ b/core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/MeshDataHandlerImpl.kt @@ -389,7 +389,7 @@ class MeshDataHandlerImpl( } handleAckNak( packet.decoded?.request_id ?: 0, - nodeManager.toNodeID(packet.from), + packet.from, r.error_reason?.value ?: 0, dataPacket.relayNode, session, @@ -400,12 +400,13 @@ class MeshDataHandlerImpl( @Suppress("CyclomaticComplexMethod", "LongMethod") private fun handleAckNak( requestId: Int, - fromId: String, + fromNum: Int, routingError: Int, relayNode: Int?, session: RadioSessionContext, ackProofStatus: Int = MeshPacket.AckProofStatus.ACK_PROOF_ABSENT.value, ) { + val fromId = nodeManager.toNodeID(fromNum) radioInterfaceService.launchSessionWork(scope, session) { val isAck = routingError == Routing.Error.NONE.value val absentAckProof = MeshPacket.AckProofStatus.ACK_PROOF_ABSENT.value @@ -471,7 +472,7 @@ class MeshDataHandlerImpl( packetRepository.value.updateReaction(settled.copy(ackProofStatus = provenAckProof)) } } - packetHandler.completeDispatchedResponse(requestId, complete = isAck) + packetHandler.completeDispatchedResponse(requestId, complete = isAck, from = fromNum) } } diff --git a/core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/PacketHandlerImpl.kt b/core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/PacketHandlerImpl.kt index 8b48f3eb25c..5226193b166 100644 --- a/core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/PacketHandlerImpl.kt +++ b/core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/PacketHandlerImpl.kt @@ -41,6 +41,7 @@ import org.meshtastic.core.database.dao.shouldApplyOutgoingQueueStatus import org.meshtastic.core.model.ConnectionState import org.meshtastic.core.model.MeshLog import org.meshtastic.core.model.MessageStatus +import org.meshtastic.core.model.NodeAddress import org.meshtastic.core.model.RadioNotConnectedException import org.meshtastic.core.model.util.toOneLineString import org.meshtastic.core.model.util.toPIIString @@ -425,9 +426,11 @@ class PacketHandlerImpl( } } - override suspend fun completeDispatchedResponse(dataRequestId: Int, complete: Boolean) { + override suspend fun completeDispatchedResponse(dataRequestId: Int, complete: Boolean, from: Int?) { responseMutex.withLock { val pending = queueResponse[dataRequestId]?.takeIf { it.wasDispatched } ?: return@withLock + val to = pending.packet.to + if (complete && from != null && from != to && to != NodeAddress.NODENUM_BROADCAST) return@withLock pending.completeRouting(complete.toAwaitedSendStatus()) removePendingIfTerminalLocked(dataRequestId, pending) } diff --git a/core/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/MeshDataHandlerTest.kt b/core/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/MeshDataHandlerTest.kt index 7c1cbefaf21..a8f5e254916 100644 --- a/core/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/MeshDataHandlerTest.kt +++ b/core/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/MeshDataHandlerTest.kt @@ -773,7 +773,7 @@ class MeshDataHandlerTest { handler.handleReceivedData(routingPacket(Routing.Error.NONE), 123) advanceUntilIdle() - verifySuspend { packetHandler.completeDispatchedResponse(99, complete = true) } + verifySuspend { packetHandler.completeDispatchedResponse(99, complete = true, from = 456) } } private fun sentPacket(ackProofStatus: Int = 0) = DataPacket( @@ -915,7 +915,7 @@ class MeshDataHandlerTest { handler.handleReceivedData(routingPacket(Routing.Error.NO_ROUTE), 123) advanceUntilIdle() - verifySuspend { packetHandler.completeDispatchedResponse(99, complete = false) } + verifySuspend { packetHandler.completeDispatchedResponse(99, complete = false, from = 456) } } @Test @@ -952,7 +952,7 @@ class MeshDataHandlerTest { verifySuspend(exactly(0)) { packetRepository.findPacketsWithId(any()) } verifySuspend(exactly(0)) { packetRepository.findReactionsWithId(any()) } verifySuspend(exactly(0)) { packetRepository.update(any(), any()) } - verifySuspend(exactly(0)) { packetHandler.completeDispatchedResponse(any(), any()) } + verifySuspend(exactly(0)) { packetHandler.completeDispatchedResponse(any(), any(), any()) } } // --- Telemetry handling --- diff --git a/core/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/PacketHandlerImplTest.kt b/core/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/PacketHandlerImplTest.kt index 6f28c88d8ef..6849a3fc4b2 100644 --- a/core/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/PacketHandlerImplTest.kt +++ b/core/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/PacketHandlerImplTest.kt @@ -1126,6 +1126,38 @@ class PacketHandlerImplTest { assertTrue(rejected.dispatched) } + @Test + fun `a relay ack leaves the wait open for the destination nak`() = runTest(testDispatcher) { + connectionStateFlow.value = ConnectionState.Connected + val packet = + MeshPacket.Builder() + .also { wb -> + wb.id = 830 + wb.to = 500 + } + .build() + val result = async { handler.sendToRadioAndAwaitResult(packet) } + testScheduler.runCurrent() + handler.handleQueueStatus( + QueueStatus.Builder() + .also { wb -> + wb.mesh_packet_id = 830 + wb.res = 0 + wb.free = 16 + } + .build(), + ) + testScheduler.runCurrent() + + handler.completeDispatchedResponse(dataRequestId = 830, complete = true, from = 123) + testScheduler.runCurrent() + assertFalse(result.isCompleted) + + handler.completeDispatchedResponse(dataRequestId = 830, complete = false, from = 500) + testScheduler.runCurrent() + assertEquals(AwaitedSendStatus.RADIO_REJECTED, result.await().status) + } + @Test fun `late queue status cannot replace an already completed response`() = runTest(testDispatcher) { connectionStateFlow.value = ConnectionState.Connected diff --git a/core/domain/src/commonMain/kotlin/org/meshtastic/core/domain/usecase/session/EnsureRemoteAdminSessionUseCase.kt b/core/domain/src/commonMain/kotlin/org/meshtastic/core/domain/usecase/session/EnsureRemoteAdminSessionUseCase.kt index 9df3d09d93b..6b7921fb9a2 100644 --- a/core/domain/src/commonMain/kotlin/org/meshtastic/core/domain/usecase/session/EnsureRemoteAdminSessionUseCase.kt +++ b/core/domain/src/commonMain/kotlin/org/meshtastic/core/domain/usecase/session/EnsureRemoteAdminSessionUseCase.kt @@ -24,6 +24,7 @@ import kotlinx.coroutines.async import kotlinx.coroutines.coroutineScope import kotlinx.coroutines.flow.filter import kotlinx.coroutines.flow.first +import kotlinx.coroutines.flow.mapNotNull import kotlinx.coroutines.selects.select import kotlinx.coroutines.sync.Mutex import kotlinx.coroutines.sync.withLock @@ -33,6 +34,7 @@ import org.koin.core.annotation.Single import org.meshtastic.core.common.di.ServiceScope import org.meshtastic.core.model.ConnectionState import org.meshtastic.core.model.SessionStatus +import org.meshtastic.core.model.routingNakOrNull import org.meshtastic.core.repository.RadioController import org.meshtastic.core.repository.ServiceRepository import org.meshtastic.core.repository.SessionManager @@ -75,35 +77,34 @@ open class EnsureRemoteAdminSessionUseCase( return EnsureSessionResult.AlreadyActive } - val deferred = - inFlightMutex.withLock { - inFlight[destNum]?.takeIf { !it.isCompleted } - ?: run { - lateinit var newDeferred: Deferred - newDeferred = - serviceScope.async(start = CoroutineStart.LAZY) { - try { - runEnsure(destNum) - } finally { - // Cleanup belongs to the shared Deferred itself. NonCancellable guarantees that a - // service-scope cancellation cannot strand a completed entry in the dedupe map. - withContext(NonCancellable) { - inFlightMutex.withLock { - if (inFlight[destNum] === newDeferred) inFlight.remove(destNum) - } + val deferred = inFlightMutex.withLock { + inFlight[destNum]?.takeIf { !it.isCompleted } + ?: run { + lateinit var newDeferred: Deferred + newDeferred = + serviceScope.async(start = CoroutineStart.LAZY) { + try { + runEnsure(destNum) + } finally { + // Cleanup belongs to the shared Deferred itself. NonCancellable guarantees that a + // service-scope cancellation cannot strand a completed entry in the dedupe map. + withContext(NonCancellable) { + inFlightMutex.withLock { + if (inFlight[destNum] === newDeferred) inFlight.remove(destNum) } } } - // Register before the lazy deferred starts. The identity check above prevents an old completion - // from removing a newer ensure for the same node. - inFlight[destNum] = newDeferred - // The service scope, not the first awaiting caller, owns dispatch once the entry is visible. - // A lazy child can already be cancelled when its parent scope is shutting down; in that case - // its body never runs, so remove the entry here instead of relying on the body-level finally. - if (!newDeferred.start() && inFlight[destNum] === newDeferred) inFlight.remove(destNum) - newDeferred - } - } + } + // Register before the lazy deferred starts. The identity check above prevents an old completion + // from removing a newer ensure for the same node. + inFlight[destNum] = newDeferred + // The service scope, not the first awaiting caller, owns dispatch once the entry is visible. + // A lazy child can already be cancelled when its parent scope is shutting down; in that case + // its body never runs, so remove the entry here instead of relying on the body-level finally. + if (!newDeferred.start() && inFlight[destNum] === newDeferred) inFlight.remove(destNum) + newDeferred + } + } return deferred.await() } @@ -111,7 +112,7 @@ open class EnsureRemoteAdminSessionUseCase( Logger.d { "EnsureRemoteAdminSession dispatching metadata request to $destNum" } return withTimeoutOrNull(UX_TIMEOUT) { coroutineScope { - // Subscribe to both terminal events before dispatch so neither a synchronous response nor a + // Subscribe to every terminal event before dispatch so neither a synchronous response nor a // concurrent connection departure can be missed. UNDISPATCHED is deliberate: these children do no // work before suspending in first(), so starting inline only establishes the subscriptions. val refreshed = @@ -122,16 +123,25 @@ open class EnsureRemoteAdminSessionUseCase( async(start = CoroutineStart.UNDISPATCHED) { serviceRepository.connectionState.filter { it != ConnectionState.Connected }.first() } + val requestId = radioController.generatePacketId() + val rejected = + async(start = CoroutineStart.UNDISPATCHED) { + serviceRepository.meshPacketFlow + .mapNotNull { it.routingNakOrNull() } + .first { it.requestId == requestId } + } try { if (disconnected.isCompleted) return@coroutineScope EnsureSessionResult.Disconnected - radioController.refreshMetadata(destNum) + radioController.refreshMetadata(destNum, requestId) select { disconnected.onAwait { EnsureSessionResult.Disconnected } refreshed.onAwait { EnsureSessionResult.Refreshed } + rejected.onAwait { EnsureSessionResult.Rejected(it) } } } finally { disconnected.cancel() refreshed.cancel() + rejected.cancel() } } } ?: EnsureSessionResult.Timeout diff --git a/core/domain/src/commonMain/kotlin/org/meshtastic/core/domain/usecase/session/EnsureSessionResult.kt b/core/domain/src/commonMain/kotlin/org/meshtastic/core/domain/usecase/session/EnsureSessionResult.kt index 18b9ac12036..8e6c41dcac5 100644 --- a/core/domain/src/commonMain/kotlin/org/meshtastic/core/domain/usecase/session/EnsureSessionResult.kt +++ b/core/domain/src/commonMain/kotlin/org/meshtastic/core/domain/usecase/session/EnsureSessionResult.kt @@ -16,6 +16,8 @@ */ package org.meshtastic.core.domain.usecase.session +import org.meshtastic.core.model.RoutingNak + /** * Transient outcome of a single call to [EnsureRemoteAdminSessionUseCase]. This is the *event* the UI reacts to * (snackbar / navigate / disable button) — distinct from the durable `SessionStatus` flow used by chips and gates. @@ -33,6 +35,9 @@ sealed interface EnsureSessionResult { */ data object Timeout : EnsureSessionResult + /** A routing NAK answered the metadata request, such as the target refusing our admin key. */ + data class Rejected(val nak: RoutingNak) : EnsureSessionResult + /** The radio is not in [org.meshtastic.core.model.ConnectionState.Connected]; no packet was sent. */ data object Disconnected : EnsureSessionResult } diff --git a/core/domain/src/commonTest/kotlin/org/meshtastic/core/domain/usecase/session/EnsureRemoteAdminSessionUseCaseTest.kt b/core/domain/src/commonTest/kotlin/org/meshtastic/core/domain/usecase/session/EnsureRemoteAdminSessionUseCaseTest.kt index 9756319e84a..6688bf8fe20 100644 --- a/core/domain/src/commonTest/kotlin/org/meshtastic/core/domain/usecase/session/EnsureRemoteAdminSessionUseCaseTest.kt +++ b/core/domain/src/commonTest/kotlin/org/meshtastic/core/domain/usecase/session/EnsureRemoteAdminSessionUseCaseTest.kt @@ -36,12 +36,18 @@ import kotlinx.coroutines.test.advanceUntilIdle import kotlinx.coroutines.test.runCurrent import kotlinx.coroutines.test.runTest import okio.ByteString +import okio.ByteString.Companion.toByteString import org.meshtastic.core.common.di.asServiceScope import org.meshtastic.core.model.ConnectionState +import org.meshtastic.core.model.RoutingNak import org.meshtastic.core.model.SessionStatus import org.meshtastic.core.repository.RadioController import org.meshtastic.core.repository.ServiceRepository import org.meshtastic.core.repository.SessionManager +import org.meshtastic.proto.Data +import org.meshtastic.proto.MeshPacket +import org.meshtastic.proto.PortNum +import org.meshtastic.proto.Routing import kotlin.test.Test import kotlin.test.assertEquals import kotlin.time.Clock @@ -63,12 +69,34 @@ class EnsureRemoteAdminSessionUseCaseTest { return mgr } - private fun connectedRepo(state: ConnectionState = ConnectionState.Connected): ServiceRepository { + private fun connectedRepo( + state: ConnectionState = ConnectionState.Connected, + packets: MutableSharedFlow = MutableSharedFlow(extraBufferCapacity = 8), + ): ServiceRepository { val repo = mock(MockMode.autofill) every { repo.connectionState } returns MutableStateFlow(state) + every { repo.meshPacketFlow } returns packets return repo } + private fun routingNak(requestId: Int, error: Routing.Error): MeshPacket { + val routing = Routing.Builder().also { wb -> wb.error_reason = error }.build() + val data = + Data.Builder() + .also { wb -> + wb.portnum = PortNum.ROUTING_APP + wb.request_id = requestId + wb.payload = routing.encode().toByteString() + } + .build() + return MeshPacket.Builder() + .also { wb -> + wb.from = destNum + wb.decoded = data + } + .build() + } + @Test fun `returns Disconnected without dispatching when not connected`() = runTest { val sessionManager = stubSessionManager() @@ -105,7 +133,7 @@ class EnsureRemoteAdminSessionUseCaseTest { val sessionManager = stubSessionManager(refreshFlow = refresh) val controller = mock(MockMode.autofill) // Simulate the radio responding by emitting on the refresh flow when the metadata request fires. - everySuspend { controller.refreshMetadata(any()) } calls { refresh.tryEmit(destNum) } + everySuspend { controller.refreshMetadata(any(), any()) } calls { refresh.tryEmit(destNum) } val useCase = EnsureRemoteAdminSessionUseCase(sessionManager, controller, connectedRepo(), this.asServiceScope()) @@ -113,7 +141,54 @@ class EnsureRemoteAdminSessionUseCaseTest { val result = useCase(destNum) assertEquals(EnsureSessionResult.Refreshed, result) - verifySuspend { controller.refreshMetadata(destNum) } + verifySuspend { controller.refreshMetadata(destNum, any()) } + } + + @Test + fun `returns Rejected when a routing nak answers the metadata request`() = runTest { + val packets = MutableSharedFlow(extraBufferCapacity = 8) + val controller = mock(MockMode.autofill) + every { controller.generatePacketId() } returns REQUEST_ID + everySuspend { controller.refreshMetadata(any(), any()) } calls + { + packets.tryEmit(routingNak(REQUEST_ID, Routing.Error.ADMIN_PUBLIC_KEY_UNAUTHORIZED)) + } + val useCase = + EnsureRemoteAdminSessionUseCase( + stubSessionManager(), + controller, + connectedRepo(packets = packets), + this.asServiceScope(), + ) + + val result = useCase(destNum) + + val expected = RoutingNak(REQUEST_ID, destNum, Routing.Error.ADMIN_PUBLIC_KEY_UNAUTHORIZED.value) + assertEquals(EnsureSessionResult.Rejected(expected), result) + verifySuspend { controller.refreshMetadata(destNum, REQUEST_ID) } + } + + @Test + fun `ignores a nak for another request and a plain ack`() = runTest { + val refresh = MutableSharedFlow(extraBufferCapacity = 8) + val packets = MutableSharedFlow(extraBufferCapacity = 8) + val controller = mock(MockMode.autofill) + every { controller.generatePacketId() } returns REQUEST_ID + everySuspend { controller.refreshMetadata(any(), any()) } calls + { + packets.tryEmit(routingNak(REQUEST_ID + 1, Routing.Error.ADMIN_PUBLIC_KEY_UNAUTHORIZED)) + packets.tryEmit(routingNak(REQUEST_ID, Routing.Error.NONE)) + refresh.tryEmit(destNum) + } + val useCase = + EnsureRemoteAdminSessionUseCase( + stubSessionManager(refreshFlow = refresh), + controller, + connectedRepo(packets = packets), + this.asServiceScope(), + ) + + assertEquals(EnsureSessionResult.Refreshed, useCase(destNum)) } @Test @@ -121,7 +196,7 @@ class EnsureRemoteAdminSessionUseCaseTest { val refresh = MutableSharedFlow(extraBufferCapacity = 8) val sessionManager = stubSessionManager(refreshFlow = refresh) val controller = mock(MockMode.autofill) - everySuspend { controller.refreshMetadata(any()) } returns Unit + everySuspend { controller.refreshMetadata(any(), any()) } returns Unit val useCase = EnsureRemoteAdminSessionUseCase(sessionManager, controller, connectedRepo(), this.asServiceScope()) @@ -135,7 +210,7 @@ class EnsureRemoteAdminSessionUseCaseTest { job.join() assertEquals(EnsureSessionResult.Refreshed, observed) - verifySuspend { controller.refreshMetadata(destNum) } + verifySuspend { controller.refreshMetadata(destNum, any()) } } @Test @@ -143,7 +218,7 @@ class EnsureRemoteAdminSessionUseCaseTest { val refresh = MutableSharedFlow(extraBufferCapacity = 8) val sessionManager = stubSessionManager(refreshFlow = refresh) val controller = mock(MockMode.autofill) - everySuspend { controller.refreshMetadata(any()) } returns Unit + everySuspend { controller.refreshMetadata(any(), any()) } returns Unit val useCase = EnsureRemoteAdminSessionUseCase(sessionManager, controller, connectedRepo(), this.asServiceScope()) @@ -163,7 +238,7 @@ class EnsureRemoteAdminSessionUseCaseTest { val sessionManager = stubSessionManager(refreshFlow = refresh) val controller = mock(MockMode.autofill) var dispatches = 0 - everySuspend { controller.refreshMetadata(any()) } calls { dispatches++ } + everySuspend { controller.refreshMetadata(any(), any()) } calls { dispatches++ } val useCase = EnsureRemoteAdminSessionUseCase(sessionManager, controller, connectedRepo(), this.asServiceScope()) @@ -187,7 +262,7 @@ class EnsureRemoteAdminSessionUseCaseTest { val sessionManager = stubSessionManager(refreshFlow = refresh) val controller = mock(MockMode.autofill) var dispatches = 0 - everySuspend { controller.refreshMetadata(any()) } calls { dispatches++ } + everySuspend { controller.refreshMetadata(any(), any()) } calls { dispatches++ } val useCase = EnsureRemoteAdminSessionUseCase(sessionManager, controller, connectedRepo(), this.asServiceScope()) @@ -212,10 +287,11 @@ class EnsureRemoteAdminSessionUseCaseTest { val sessionManager = stubSessionManager(refreshFlow = refresh) val controller = mock(MockMode.autofill) var dispatches = 0 - everySuspend { controller.refreshMetadata(any()) } calls { dispatches++ } + everySuspend { controller.refreshMetadata(any(), any()) } calls { dispatches++ } val connectionState = MutableStateFlow(ConnectionState.Connected) val repository = mock(MockMode.autofill) every { repository.connectionState } returns connectionState + every { repository.meshPacketFlow } returns MutableSharedFlow() val useCase = EnsureRemoteAdminSessionUseCase(sessionManager, controller, repository, this.asServiceScope()) val departed = async { useCase(destNum) } @@ -234,4 +310,8 @@ class EnsureRemoteAdminSessionUseCaseTest { assertEquals(EnsureSessionResult.Refreshed, reconnected.await()) } + + private companion object { + const val REQUEST_ID = 0x1234 + } } diff --git a/core/model/src/commonMain/kotlin/org/meshtastic/core/model/RoutingNak.kt b/core/model/src/commonMain/kotlin/org/meshtastic/core/model/RoutingNak.kt new file mode 100644 index 00000000000..f122e6098d8 --- /dev/null +++ b/core/model/src/commonMain/kotlin/org/meshtastic/core/model/RoutingNak.kt @@ -0,0 +1,36 @@ +/* + * Copyright (c) 2026 Meshtastic LLC + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ +package org.meshtastic.core.model + +import org.meshtastic.core.model.util.decodeOrNull +import org.meshtastic.proto.MeshPacket +import org.meshtastic.proto.PortNum +import org.meshtastic.proto.Routing + +/** A routing NAK from node [from] carrying `Routing.Error` value [routingError] for our packet [requestId]. */ +data class RoutingNak(val requestId: Int, val from: Int, val routingError: Int) + +/** The routing NAK this packet carries, or null for any other packet, including a plain ACK. */ +fun MeshPacket.routingNakOrNull(): RoutingNak? { + val data = decoded?.takeIf { it.portnum == PortNum.ROUTING_APP && it.request_id != 0 } + val error = data?.let { Routing.ADAPTER.decodeOrNull(it.payload)?.error_reason } + return if (data == null || error == null || error == Routing.Error.NONE) { + null + } else { + RoutingNak(requestId = data.request_id, from = from, routingError = error.value) + } +} diff --git a/core/model/src/commonTest/kotlin/org/meshtastic/core/model/RoutingNakTest.kt b/core/model/src/commonTest/kotlin/org/meshtastic/core/model/RoutingNakTest.kt new file mode 100644 index 00000000000..ede70113d40 --- /dev/null +++ b/core/model/src/commonTest/kotlin/org/meshtastic/core/model/RoutingNakTest.kt @@ -0,0 +1,71 @@ +/* + * Copyright (c) 2026 Meshtastic LLC + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ +package org.meshtastic.core.model + +import okio.ByteString +import okio.ByteString.Companion.toByteString +import org.meshtastic.proto.Data +import org.meshtastic.proto.MeshPacket +import org.meshtastic.proto.PortNum +import org.meshtastic.proto.Routing +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertNull + +class RoutingNakTest { + private fun packet(portnum: PortNum, requestId: Int, payload: ByteString): MeshPacket { + val data = + Data.Builder() + .also { wb -> + wb.portnum = portnum + wb.request_id = requestId + wb.payload = payload + } + .build() + return MeshPacket.Builder() + .also { wb -> + wb.from = 77 + wb.decoded = data + } + .build() + } + + private fun routing(error: Routing.Error) = + Routing.Builder().also { wb -> wb.error_reason = error }.build().encode().toByteString() + + @Test + fun `a routing error is read with its request and sender`() { + val nak = packet(PortNum.ROUTING_APP, 9, routing(Routing.Error.NO_RESPONSE)).routingNakOrNull() + + assertEquals(RoutingNak(requestId = 9, from = 77, routingError = Routing.Error.NO_RESPONSE.value), nak) + } + + @Test + fun `an ack is not a nak`() { + assertNull(packet(PortNum.ROUTING_APP, 9, routing(Routing.Error.NONE)).routingNakOrNull()) + } + + @Test + fun `a packet answering no request is not a nak`() { + assertNull(packet(PortNum.ROUTING_APP, 0, routing(Routing.Error.NO_RESPONSE)).routingNakOrNull()) + } + + @Test + fun `another port is not a nak`() { + assertNull(packet(PortNum.TEXT_MESSAGE_APP, 9, routing(Routing.Error.NO_RESPONSE)).routingNakOrNull()) + } +} diff --git a/core/repository/src/commonMain/kotlin/org/meshtastic/core/repository/CommandSender.kt b/core/repository/src/commonMain/kotlin/org/meshtastic/core/repository/CommandSender.kt index 30807011434..28368c80038 100644 --- a/core/repository/src/commonMain/kotlin/org/meshtastic/core/repository/CommandSender.kt +++ b/core/repository/src/commonMain/kotlin/org/meshtastic/core/repository/CommandSender.kt @@ -109,13 +109,13 @@ interface CommandSender { suspend fun sendPosition(pos: ProtoPosition, destNum: Int? = null, wantResponse: Boolean = false) /** Requests the position of a specific node, or throws if the outbound queue rejects it. */ - suspend fun requestPosition(destNum: Int, currentPosition: ModelPosition) + suspend fun requestPosition(destNum: Int, currentPosition: ModelPosition, requestId: Int = 0) /** Sets a fixed position for a node, or throws if the outbound queue rejects the admin command. */ suspend fun setFixedPosition(destNum: Int, pos: ModelPosition) /** Requests user info from a specific node, or throws if the outbound queue rejects it. */ - suspend fun requestUserInfo(destNum: Int) + suspend fun requestUserInfo(destNum: Int, requestId: Int = 0) /** Requests a traceroute to a specific node, or throws if the outbound queue rejects it. */ suspend fun requestTraceroute(requestId: Int, destNum: Int) diff --git a/core/repository/src/commonMain/kotlin/org/meshtastic/core/repository/PacketHandler.kt b/core/repository/src/commonMain/kotlin/org/meshtastic/core/repository/PacketHandler.kt index 2ebfe749407..64ddef77eec 100644 --- a/core/repository/src/commonMain/kotlin/org/meshtastic/core/repository/PacketHandler.kt +++ b/core/repository/src/commonMain/kotlin/org/meshtastic/core/repository/PacketHandler.kt @@ -80,8 +80,11 @@ interface PacketHandler { * Completes the strict routing response for [dataRequestId] when an active transport already dispatched that * packet. Replies that arrive before dispatch are stale and ignored. The packet ID remains reserved until both the * firmware queue stage and this routing stage are terminal. + * + * An ACK [from] a node other than the packet's destination is a relay's implicit ACK and leaves the wait open; a + * NAK from any node ends it. A null [from] is treated as the destination. */ - suspend fun completeDispatchedResponse(dataRequestId: Int, complete: Boolean) + suspend fun completeDispatchedResponse(dataRequestId: Int, complete: Boolean, from: Int? = null) /** Stops the packet queue. */ fun stopPacketQueue() diff --git a/core/repository/src/commonMain/kotlin/org/meshtastic/core/repository/QueryController.kt b/core/repository/src/commonMain/kotlin/org/meshtastic/core/repository/QueryController.kt index 39f72bc879e..b222e484d03 100644 --- a/core/repository/src/commonMain/kotlin/org/meshtastic/core/repository/QueryController.kt +++ b/core/repository/src/commonMain/kotlin/org/meshtastic/core/repository/QueryController.kt @@ -28,14 +28,14 @@ import org.meshtastic.core.model.Position */ interface QueryController { - /** Requests device metadata from a remote node. */ - suspend fun refreshMetadata(destNum: Int) + /** Requests device metadata from a remote node. A [requestId] of 0 lets the sender assign one. */ + suspend fun refreshMetadata(destNum: Int, requestId: Int = 0) - /** Requests the current GPS position from a remote node. */ - suspend fun requestPosition(destNum: Int, currentPosition: Position) + /** Requests the current GPS position from a remote node. A [requestId] of 0 lets the sender assign one. */ + suspend fun requestPosition(destNum: Int, currentPosition: Position, requestId: Int = 0) - /** Requests detailed user info from a remote node. */ - suspend fun requestUserInfo(destNum: Int) + /** Requests detailed user info from a remote node. A [requestId] of 0 lets the sender assign one. */ + suspend fun requestUserInfo(destNum: Int, requestId: Int = 0) /** Initiates a traceroute request to a remote node. */ suspend fun requestTraceroute(requestId: Int, destNum: Int) diff --git a/core/resources/src/commonMain/composeResources/values/strings.xml b/core/resources/src/commonMain/composeResources/values/strings.xml index 96c24fd4b33..1a8572a7ff1 100644 --- a/core/resources/src/commonMain/composeResources/values/strings.xml +++ b/core/resources/src/commonMain/composeResources/values/strings.xml @@ -1799,6 +1799,7 @@ Power Metrics Request Telemetry Request User Info + %1$s from %2$s failed: %3$s Requesting %1$s from %2$s Resend Reset diff --git a/core/service/src/commonMain/kotlin/org/meshtastic/core/service/QueryControllerImpl.kt b/core/service/src/commonMain/kotlin/org/meshtastic/core/service/QueryControllerImpl.kt index 2137b71279d..5fca91e27d9 100644 --- a/core/service/src/commonMain/kotlin/org/meshtastic/core/service/QueryControllerImpl.kt +++ b/core/service/src/commonMain/kotlin/org/meshtastic/core/service/QueryControllerImpl.kt @@ -37,13 +37,13 @@ internal class QueryControllerImpl( private val myNodeNum: Int get() = nodeManager.myNodeNum.value ?: 0 - override suspend fun refreshMetadata(destNum: Int) { - commandSender.sendAdmin(destNum, wantResponse = true) { + override suspend fun refreshMetadata(destNum: Int, requestId: Int) { + commandSender.sendAdmin(destNum, requestId = requestId, wantResponse = true) { AdminMessage.Builder().also { wb -> wb.get_device_metadata_request = true }.build() } } - override suspend fun requestPosition(destNum: Int, currentPosition: Position) { + override suspend fun requestPosition(destNum: Int, currentPosition: Position, requestId: Int) { if (destNum == nodeManager.myNodeNum.value) return val provideLocation = uiPrefs.shouldProvideNodeLocation(myNodeNum).value // Position(0.0, 0.0, 0) means "attach no coordinates"; the sender leaves them off the wire. @@ -55,12 +55,12 @@ internal class QueryControllerImpl( } else { Position(0.0, 0.0, 0) } - commandSender.requestPosition(destNum, resolvedPosition) + commandSender.requestPosition(destNum, resolvedPosition, requestId) } - override suspend fun requestUserInfo(destNum: Int) { + override suspend fun requestUserInfo(destNum: Int, requestId: Int) { if (destNum != nodeManager.myNodeNum.value) { - commandSender.requestUserInfo(destNum) + commandSender.requestUserInfo(destNum, requestId) } } diff --git a/core/testing/src/commonMain/kotlin/org/meshtastic/core/testing/FakeCommandSender.kt b/core/testing/src/commonMain/kotlin/org/meshtastic/core/testing/FakeCommandSender.kt index 8aebd564944..9e69bfd7163 100644 --- a/core/testing/src/commonMain/kotlin/org/meshtastic/core/testing/FakeCommandSender.kt +++ b/core/testing/src/commonMain/kotlin/org/meshtastic/core/testing/FakeCommandSender.kt @@ -170,7 +170,7 @@ class FakeCommandSender : failCommandIfConfigured() } - override suspend fun requestPosition(destNum: Int, currentPosition: Position) { + override suspend fun requestPosition(destNum: Int, currentPosition: Position, requestId: Int) { failCommandIfConfigured() } @@ -178,7 +178,7 @@ class FakeCommandSender : failCommandIfConfigured() } - override suspend fun requestUserInfo(destNum: Int) { + override suspend fun requestUserInfo(destNum: Int, requestId: Int) { failCommandIfConfigured() } diff --git a/core/testing/src/commonMain/kotlin/org/meshtastic/core/testing/FakeRadioController.kt b/core/testing/src/commonMain/kotlin/org/meshtastic/core/testing/FakeRadioController.kt index 691d88e05db..423bc4c8789 100644 --- a/core/testing/src/commonMain/kotlin/org/meshtastic/core/testing/FakeRadioController.kt +++ b/core/testing/src/commonMain/kotlin/org/meshtastic/core/testing/FakeRadioController.kt @@ -274,7 +274,7 @@ class FakeRadioController : override suspend fun importContact(contact: org.meshtastic.proto.SharedContact) {} - override suspend fun refreshMetadata(destNum: Int) {} + override suspend fun refreshMetadata(destNum: Int, requestId: Int) {} override suspend fun setLocalConfig(config: Config) { if (throwOnSetLocalConfig) error("Fake local config write failure") @@ -420,9 +420,9 @@ class FakeRadioController : override suspend fun removeByNodenum(packetId: Int, nodeNum: Int) {} - override suspend fun requestPosition(destNum: Int, currentPosition: Position) {} + override suspend fun requestPosition(destNum: Int, currentPosition: Position, requestId: Int) {} - override suspend fun requestUserInfo(destNum: Int) {} + override suspend fun requestUserInfo(destNum: Int, requestId: Int) {} override suspend fun requestTraceroute(requestId: Int, destNum: Int) {} diff --git a/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/component/AdministrationSection.kt b/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/component/AdministrationSection.kt index 500bc0a1dc8..1655d4b98e1 100644 --- a/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/component/AdministrationSection.kt +++ b/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/component/AdministrationSection.kt @@ -133,7 +133,7 @@ fun AdministrationSection( leadingIcon = MeshtasticIcons.Memory, trailingIcon = null, enabled = !isEnsuringSession, - onClick = { onAction(NodeDetailAction.RefreshMetadata(node.num)) }, + onClick = { onAction(NodeDetailAction.RefreshMetadata(node.num, node.user.long_name)) }, ) } } diff --git a/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/detail/CommonNodeRequestActions.kt b/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/detail/CommonNodeRequestActions.kt index e97abe4f3fe..8b353b91f43 100644 --- a/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/detail/CommonNodeRequestActions.kt +++ b/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/detail/CommonNodeRequestActions.kt @@ -52,6 +52,7 @@ constructor( private val radioController: RadioController, private val snackbarManager: SnackbarManager, private val analytics: PlatformAnalytics, + private val nakWatcher: RequestNakWatcher, private val resolveUiText: suspend (UiText) -> String = { it.resolve() }, ) : NodeRequestActions { @@ -82,32 +83,27 @@ constructor( override suspend fun requestUserInfo(destNum: Int, longName: String) = runRequest { Logger.i { "Requesting UserInfo for '$destNum'" } - radioController.requestUserInfo(destNum) + nakWatcher.watch(Res.string.user_info, longName) { id -> radioController.requestUserInfo(destNum, id) } analytics.trackAction("user_info_request") showFeedback(UiText.Resource(Res.string.requesting_from, Res.string.user_info, longName)) } override suspend fun requestNeighborInfo(destNum: Int, longName: String) = runRequest { Logger.i { "Requesting NeighborInfo for '$destNum'" } - val packetId = radioController.generatePacketId() - radioController.requestNeighborInfo(packetId, destNum) + nakWatcher.watch(Res.string.neighbor_info, longName) { id -> radioController.requestNeighborInfo(id, destNum) } _lastRequestNeighborTimes.update { it + (destNum to nowMillis) } showFeedback(UiText.Resource(Res.string.requesting_from, Res.string.neighbor_info, longName)) } override suspend fun requestPosition(destNum: Int, longName: String, position: Position) = runRequest { Logger.i { "Requesting position for '$destNum'" } - radioController.requestPosition(destNum, position) + nakWatcher.watch(Res.string.position, longName) { id -> radioController.requestPosition(destNum, position, id) } analytics.trackAction("position_request") showFeedback(UiText.Resource(Res.string.requesting_from, Res.string.position, longName)) } override suspend fun requestTelemetry(destNum: Int, longName: String, type: TelemetryType) = runRequest { Logger.i { "Requesting telemetry for '$destNum'" } - val packetId = radioController.generatePacketId() - radioController.requestTelemetry(packetId, destNum, type.ordinal) - analytics.trackAction("telemetry_request", mapOf("telemetry_type" to type.name)) - val typeRes = when (type) { TelemetryType.DEVICE -> Res.string.request_device_metrics @@ -118,6 +114,8 @@ constructor( TelemetryType.HOST -> Res.string.request_host_metrics TelemetryType.PAX -> Res.string.request_pax_metrics } + nakWatcher.watch(typeRes, longName) { id -> radioController.requestTelemetry(id, destNum, type.ordinal) } + analytics.trackAction("telemetry_request", mapOf("telemetry_type" to type.name)) showFeedback(UiText.Resource(Res.string.requesting_from, typeRes, longName)) } @@ -137,8 +135,7 @@ constructor( Logger.i { "Requesting traceroute for '$destNum'" } var sent = false try { - val packetId = radioController.generatePacketId() - radioController.requestTraceroute(packetId, destNum) + nakWatcher.watch(Res.string.traceroute, longName) { id -> radioController.requestTraceroute(id, destNum) } sent = true } finally { // A failed or canceled send must not start the cooldown; only undo our own claim. diff --git a/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/detail/HandleNodeAction.kt b/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/detail/HandleNodeAction.kt index decfa298ae3..2cee8deaeee 100644 --- a/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/detail/HandleNodeAction.kt +++ b/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/detail/HandleNodeAction.kt @@ -42,7 +42,7 @@ internal fun handleNodeAction( is NodeDetailAction.OpenRemoteShell -> viewModel.openRemoteShell(action.nodeNum) - is NodeDetailAction.RefreshMetadata -> viewModel.refreshMetadata(action.nodeNum) + is NodeDetailAction.RefreshMetadata -> viewModel.refreshMetadata(action.nodeNum, action.longName) is NodeDetailAction.HandleNodeMenuAction -> { when (val menuAction = action.action) { diff --git a/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/detail/NodeDetailViewModel.kt b/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/detail/NodeDetailViewModel.kt index 4288b96a67e..3807f8855b0 100644 --- a/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/detail/NodeDetailViewModel.kt +++ b/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/detail/NodeDetailViewModel.kt @@ -30,6 +30,7 @@ import kotlinx.coroutines.flow.flatMapLatest import kotlinx.coroutines.flow.flowOf import kotlinx.coroutines.flow.map import kotlinx.coroutines.flow.receiveAsFlow +import org.jetbrains.compose.resources.StringResource import org.koin.core.annotation.KoinViewModel import org.meshtastic.core.domain.usecase.session.EnsureRemoteAdminSessionUseCase import org.meshtastic.core.domain.usecase.session.EnsureSessionResult @@ -37,7 +38,10 @@ import org.meshtastic.core.domain.usecase.session.ObserveRemoteAdminSessionStatu import org.meshtastic.core.model.ContactKey import org.meshtastic.core.model.Node import org.meshtastic.core.model.NodeAddress +import org.meshtastic.core.model.RoutingNak import org.meshtastic.core.model.SessionStatus +import org.meshtastic.core.model.getMessageRoutingErrorDetailResFrom +import org.meshtastic.core.model.getStringResFrom import org.meshtastic.core.navigation.NodeDetailRoute import org.meshtastic.core.navigation.Route import org.meshtastic.core.navigation.SettingsRoute @@ -49,6 +53,7 @@ import org.meshtastic.core.resources.Res import org.meshtastic.core.resources.UiText import org.meshtastic.core.resources.connect_radio_for_remote_admin import org.meshtastic.core.resources.remote_admin_unreachable +import org.meshtastic.core.resources.request_metadata import org.meshtastic.core.ui.util.SnackbarManager import org.meshtastic.core.ui.viewmodel.safeLaunch import org.meshtastic.core.ui.viewmodel.stateInWhileSubscribed @@ -57,11 +62,23 @@ import org.meshtastic.feature.node.domain.usecase.GetNodeDetailsUseCase import org.meshtastic.feature.node.metrics.EnvironmentMetricsState import org.meshtastic.feature.node.model.LogsType import org.meshtastic.feature.node.model.MetricsState +import org.meshtastic.proto.Routing private const val QUEUE_REJECTION_LOG_MESSAGE = "Node-detail request rejected by outbound packet queue" private const val LOCAL_NODE_UNAVAILABLE_LOG_MESSAGE = "Node-detail request deferred until local node identity is available" +// Raised by our own node when it cannot encrypt to the target, so they say more than "unreachable". +private val LOCAL_KEY_ERRORS = setOf(Routing.Error.PKI_FAILED.value, Routing.Error.PKI_SEND_FAIL_PUBLIC_KEY.value) + +/** A NAK from the target says why it refused; one from anywhere else means the target was never reached. */ +internal fun remoteAdminRefusalText(nak: RoutingNak, destNum: Int): StringResource = when { + nak.from == destNum || nak.routingError in LOCAL_KEY_ERRORS -> + getMessageRoutingErrorDetailResFrom(nak.routingError) ?: getStringResFrom(nak.routingError) + + else -> Res.string.remote_admin_unreachable +} + /** UI state for the Node Details screen. */ @androidx.compose.runtime.Stable data class NodeDetailUiState( @@ -103,6 +120,7 @@ class NodeDetailViewModel( private val ensureRemoteAdminSession: EnsureRemoteAdminSessionUseCase, private val observeRemoteAdminSessionStatus: ObserveRemoteAdminSessionStatusUseCase, private val snackbarManager: SnackbarManager, + private val nakWatcher: RequestNakWatcher, private val resolveUiText: suspend (UiText) -> String = { it.resolve() }, ) : ViewModel() { @@ -211,9 +229,11 @@ class NodeDetailViewModel( /** * Re-fetch device metadata (firmware/edition/role) for [destNum]. Refreshes the session passkey as a side effect. */ - fun refreshMetadata(destNum: Int) = safeLaunch(tag = "refreshMetadata") { + fun refreshMetadata(destNum: Int, longName: String) = safeLaunch(tag = "refreshMetadata") { try { - queryController.refreshMetadata(destNum) + nakWatcher.watch(Res.string.request_metadata, longName) { id -> + queryController.refreshMetadata(destNum, id) + } } catch (e: PacketQueueRejectedException) { showNodeRequestFailure(e, QUEUE_REJECTION_LOG_MESSAGE, snackbarManager, resolveUiText) } catch (e: LocalNodeUnavailableException) { @@ -223,7 +243,8 @@ class NodeDetailViewModel( /** * Ensure a remote-admin session passkey is fresh, then request navigation to the remote-admin screen. Surfaces a - * snackbar with the appropriate guidance on [EnsureSessionResult.Disconnected] or [EnsureSessionResult.Timeout]. + * snackbar with the appropriate guidance on [EnsureSessionResult.Disconnected], [EnsureSessionResult.Timeout] or + * [EnsureSessionResult.Rejected]. */ fun openRemoteAdmin(destNum: Int) = ensureSessionThenNavigate(destNum, "openRemoteAdmin") { SettingsRoute.Settings(destNum) } @@ -240,7 +261,7 @@ class NodeDetailViewModel( if (!isEnsuringSession.compareAndSet(expect = false, update = true)) return safeLaunch(tag = tag) { try { - when (ensureRemoteAdminSession(destNum)) { + when (val result = ensureRemoteAdminSession(destNum)) { EnsureSessionResult.AlreadyActive, EnsureSessionResult.Refreshed, -> _navigationEvents.trySend(destination()) @@ -254,6 +275,11 @@ class NodeDetailViewModel( snackbarManager.showSnackbar( resolveUiText(UiText.Resource(Res.string.remote_admin_unreachable)), ) + + is EnsureSessionResult.Rejected -> + snackbarManager.showSnackbar( + resolveUiText(UiText.Resource(remoteAdminRefusalText(result.nak, destNum))), + ) } } catch (e: PacketQueueRejectedException) { showNodeRequestFailure(e, QUEUE_REJECTION_LOG_MESSAGE, snackbarManager, resolveUiText) diff --git a/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/detail/RequestNakWatcher.kt b/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/detail/RequestNakWatcher.kt new file mode 100644 index 00000000000..143a4776c9e --- /dev/null +++ b/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/detail/RequestNakWatcher.kt @@ -0,0 +1,75 @@ +/* + * Copyright (c) 2026 Meshtastic LLC + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ +package org.meshtastic.feature.node.detail + +import kotlinx.coroutines.CoroutineStart +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.flow.mapNotNull +import kotlinx.coroutines.launch +import kotlinx.coroutines.withTimeoutOrNull +import org.jetbrains.compose.resources.StringResource +import org.koin.core.annotation.Single +import org.meshtastic.core.common.di.ServiceScope +import org.meshtastic.core.model.getStringResFrom +import org.meshtastic.core.model.routingNakOrNull +import org.meshtastic.core.repository.RadioController +import org.meshtastic.core.repository.ServiceRepository +import org.meshtastic.core.resources.Res +import org.meshtastic.core.resources.UiText +import org.meshtastic.core.resources.request_failed_from +import org.meshtastic.core.ui.util.SnackbarManager +import kotlin.time.Duration.Companion.minutes + +/** Shows a snackbar when a routing NAK answers a node request, which otherwise just never gets its reply. */ +@Single +class RequestNakWatcher( + private val radioController: RadioController, + private val serviceRepository: ServiceRepository, + private val serviceScope: ServiceScope, + private val snackbarManager: SnackbarManager, + private val resolveUiText: suspend (UiText) -> String = { it.resolve() }, +) { + /** Runs [send] with a fresh packet id, watching for a NAK to that id from before the send until [WINDOW] after. */ + suspend fun watch(request: StringResource, longName: String, send: suspend (requestId: Int) -> Unit) { + val requestId = radioController.generatePacketId() + val watcher = + serviceScope.launch(start = CoroutineStart.UNDISPATCHED) { + val nak = + withTimeoutOrNull(WINDOW) { + serviceRepository.meshPacketFlow + .mapNotNull { it.routingNakOrNull() } + .first { it.requestId == requestId } + } ?: return@launch + val reason = getStringResFrom(nak.routingError) + snackbarManager.showSnackbar( + resolveUiText(UiText.Resource(Res.string.request_failed_from, request, longName, reason)), + ) + } + var sent = false + try { + send(requestId) + sent = true + } finally { + if (!sent) watcher.cancel() + } + } + + companion object { + /** Long enough for a multi-hop request to exhaust its retransmissions on the slowest presets. */ + val WINDOW = 5.minutes + } +} diff --git a/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/model/NodeDetailAction.kt b/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/model/NodeDetailAction.kt index 4d10f8a3045..ba2f8032d88 100644 --- a/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/model/NodeDetailAction.kt +++ b/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/model/NodeDetailAction.kt @@ -33,7 +33,7 @@ sealed interface NodeDetailAction { data class OpenRemoteShell(val nodeNum: Int) : NodeDetailAction /** Force-refresh device metadata (firmware version, edition, role) for the given node. */ - data class RefreshMetadata(val nodeNum: Int) : NodeDetailAction + data class RefreshMetadata(val nodeNum: Int, val longName: String) : NodeDetailAction data object ShareContact : NodeDetailAction diff --git a/feature/node/src/commonTest/kotlin/org/meshtastic/feature/node/detail/CommonNodeRequestActionsTest.kt b/feature/node/src/commonTest/kotlin/org/meshtastic/feature/node/detail/CommonNodeRequestActionsTest.kt index a4b645a8012..f90b5c03a30 100644 --- a/feature/node/src/commonTest/kotlin/org/meshtastic/feature/node/detail/CommonNodeRequestActionsTest.kt +++ b/feature/node/src/commonTest/kotlin/org/meshtastic/feature/node/detail/CommonNodeRequestActionsTest.kt @@ -48,6 +48,7 @@ class CommonNodeRequestActionsTest { radioController = radioController, snackbarManager = snackbarManager, analytics = mock(), + nakWatcher = testNakWatcher(radioController), resolveUiText = resolveNodeRequestFailureUiText, ) @@ -58,7 +59,8 @@ class CommonNodeRequestActionsTest { @Test fun `missing local identity uses the same localized request feedback`() = runTest { - everySuspend { radioController.requestUserInfo(1234) } throws LocalNodeUnavailableException("local node") + every { radioController.generatePacketId() } returns 41 + everySuspend { radioController.requestUserInfo(1234, 41) } throws LocalNodeUnavailableException("local node") actions.requestUserInfo(destNum = 1234, longName = "Test Node") @@ -93,6 +95,7 @@ class CommonNodeRequestActionsTest { radioController = radioController, snackbarManager = snackbarManager, analytics = mock(MockMode.autofill), + nakWatcher = testNakWatcher(radioController), resolveUiText = { "feedback" }, ) diff --git a/feature/node/src/commonTest/kotlin/org/meshtastic/feature/node/detail/HandleNodeActionTest.kt b/feature/node/src/commonTest/kotlin/org/meshtastic/feature/node/detail/HandleNodeActionTest.kt index 90498725614..d1e4c9cff51 100644 --- a/feature/node/src/commonTest/kotlin/org/meshtastic/feature/node/detail/HandleNodeActionTest.kt +++ b/feature/node/src/commonTest/kotlin/org/meshtastic/feature/node/detail/HandleNodeActionTest.kt @@ -165,5 +165,6 @@ class HandleNodeActionTest { ensureRemoteAdminSession = ensureRemoteAdminSession, observeRemoteAdminSessionStatus = observeRemoteAdminSessionStatus, snackbarManager = snackbarManager, + nakWatcher = testNakWatcher(), ) } diff --git a/feature/node/src/commonTest/kotlin/org/meshtastic/feature/node/detail/NodeDetailViewModelTest.kt b/feature/node/src/commonTest/kotlin/org/meshtastic/feature/node/detail/NodeDetailViewModelTest.kt index 03e1481f47b..dfefd897f9c 100644 --- a/feature/node/src/commonTest/kotlin/org/meshtastic/feature/node/detail/NodeDetailViewModelTest.kt +++ b/feature/node/src/commonTest/kotlin/org/meshtastic/feature/node/detail/NodeDetailViewModelTest.kt @@ -41,6 +41,7 @@ import org.meshtastic.core.domain.usecase.session.EnsureRemoteAdminSessionUseCas import org.meshtastic.core.domain.usecase.session.EnsureSessionResult import org.meshtastic.core.domain.usecase.session.ObserveRemoteAdminSessionStatusUseCase import org.meshtastic.core.model.Node +import org.meshtastic.core.model.RoutingNak import org.meshtastic.core.model.SessionStatus import org.meshtastic.core.navigation.SettingsRoute import org.meshtastic.core.repository.LocalNodeUnavailableException @@ -49,6 +50,7 @@ import org.meshtastic.core.repository.PacketRepository import org.meshtastic.core.repository.QueryController import org.meshtastic.feature.node.component.NodeMenuAction import org.meshtastic.feature.node.domain.usecase.GetNodeDetailsUseCase +import org.meshtastic.proto.Routing import org.meshtastic.proto.User import kotlin.test.AfterTest import kotlin.test.BeforeTest @@ -91,6 +93,7 @@ class NodeDetailViewModelTest { ensureRemoteAdminSession = ensureRemoteAdminSession, observeRemoteAdminSessionStatus = observeRemoteAdminSessionStatus, snackbarManager = snackbarManager, + nakWatcher = testNakWatcher(), resolveUiText = resolveNodeDetailUiTextForTest, ) @@ -188,12 +191,34 @@ class NodeDetailViewModelTest { verifySuspend { ensureRemoteAdminSession(1234) } } + @Test + fun `openRemoteAdmin shows the refusal when the node rejects our admin key`() = runTest(testDispatcher) { + everySuspend { ensureRemoteAdminSession(1234) } returns + EnsureSessionResult.Rejected(RoutingNak(7, 1234, Routing.Error.ADMIN_PUBLIC_KEY_UNAUTHORIZED.value)) + + viewModel.openRemoteAdmin(1234) + runCurrent() + + assertEquals(listOf(ADMIN_KEY_UNAUTHORIZED_TEXT), snackbarManager.messages) + } + + @Test + fun `openRemoteAdmin treats a nak from another node as unreachable`() = runTest(testDispatcher) { + everySuspend { ensureRemoteAdminSession(1234) } returns + EnsureSessionResult.Rejected(RoutingNak(7, 99, Routing.Error.MAX_RETRANSMIT.value)) + + viewModel.openRemoteAdmin(1234) + runCurrent() + + assertEquals(listOf("Could not reach node — try again or move closer."), snackbarManager.messages) + } + @Test fun `refreshMetadata surfaces queue rejection without throwing from view model scope`() = runTest(testDispatcher) { everySuspend { queryController.refreshMetadata(1234) } throws PacketQueueRejectedException("Metadata request") - viewModel.refreshMetadata(1234) + viewModel.refreshMetadata(1234, "Test Node") runCurrent() assertEquals(listOf(NODE_REQUEST_SEND_FAILED_TEXT), snackbarManager.messages) diff --git a/feature/node/src/commonTest/kotlin/org/meshtastic/feature/node/detail/RecordingSnackbarManager.kt b/feature/node/src/commonTest/kotlin/org/meshtastic/feature/node/detail/RecordingSnackbarManager.kt index 76c65563065..e447758023c 100644 --- a/feature/node/src/commonTest/kotlin/org/meshtastic/feature/node/detail/RecordingSnackbarManager.kt +++ b/feature/node/src/commonTest/kotlin/org/meshtastic/feature/node/detail/RecordingSnackbarManager.kt @@ -20,6 +20,7 @@ import androidx.compose.material3.SnackbarDuration import org.meshtastic.core.resources.Res import org.meshtastic.core.resources.UiText import org.meshtastic.core.resources.connect_radio_for_remote_admin +import org.meshtastic.core.resources.message_routing_error_admin_public_key_unauthorized_detail import org.meshtastic.core.resources.node_request_send_failed import org.meshtastic.core.resources.remote_admin_unreachable import org.meshtastic.core.ui.util.SnackbarManager @@ -40,6 +41,8 @@ internal class RecordingSnackbarManager : SnackbarManager() { internal const val NODE_REQUEST_SEND_FAILED_TEXT = "Couldn't send request. Try again." +internal const val ADMIN_KEY_UNAUTHORIZED_TEXT = "The remote node does not authorize your admin key." + internal val resolveNodeRequestFailureUiText: suspend (UiText) -> String = { text -> when (text) { is UiText.DynamicString -> text.value @@ -61,6 +64,7 @@ internal val resolveNodeDetailUiTextForTest: suspend (UiText) -> String = { text when (text.res) { Res.string.connect_radio_for_remote_admin -> "Connect to a radio to administer remote nodes." Res.string.remote_admin_unreachable -> "Could not reach node — try again or move closer." + Res.string.message_routing_error_admin_public_key_unauthorized_detail -> ADMIN_KEY_UNAUTHORIZED_TEXT Res.string.node_request_send_failed -> NODE_REQUEST_SEND_FAILED_TEXT else -> error("Unexpected UiText resource in test: ${text.res}") } diff --git a/feature/node/src/commonTest/kotlin/org/meshtastic/feature/node/detail/RequestNakWatcherTest.kt b/feature/node/src/commonTest/kotlin/org/meshtastic/feature/node/detail/RequestNakWatcherTest.kt new file mode 100644 index 00000000000..c48653dfbb8 --- /dev/null +++ b/feature/node/src/commonTest/kotlin/org/meshtastic/feature/node/detail/RequestNakWatcherTest.kt @@ -0,0 +1,98 @@ +/* + * Copyright (c) 2026 Meshtastic LLC + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ +package org.meshtastic.feature.node.detail + +import dev.mokkery.MockMode +import dev.mokkery.answering.returns +import dev.mokkery.every +import dev.mokkery.mock +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.flow.MutableSharedFlow +import kotlinx.coroutines.test.TestScope +import kotlinx.coroutines.test.runCurrent +import kotlinx.coroutines.test.runTest +import org.meshtastic.core.model.getStringResFrom +import org.meshtastic.core.repository.RadioController +import org.meshtastic.core.resources.Res +import org.meshtastic.core.resources.UiText +import org.meshtastic.core.resources.request_failed_from +import org.meshtastic.core.resources.traceroute +import org.meshtastic.proto.MeshPacket +import org.meshtastic.proto.Routing +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFailsWith +import kotlin.test.assertTrue + +@OptIn(ExperimentalCoroutinesApi::class) +class RequestNakWatcherTest { + private val packets = MutableSharedFlow(extraBufferCapacity = 8) + private val resolved = mutableListOf() + private val radioController = + mock(MockMode.autofill).also { every { it.generatePacketId() } returns REQUEST_ID } + + private fun TestScope.watcher() = testNakWatcher( + radioController = radioController, + packets = packets, + resolveUiText = { + resolved += it + "" + }, + scope = backgroundScope, + ) + + @Test + fun `a nak to the request shows why it failed`() = runTest { + var sentId = 0 + watcher().watch(Res.string.traceroute, "Test Node") { sentId = it } + packets.emit(routingNakPacket(REQUEST_ID, from = 1234, error = Routing.Error.NOT_AUTHORIZED)) + runCurrent() + + assertEquals(REQUEST_ID, sentId) + val text = resolved.single() as UiText.Resource + assertEquals(Res.string.request_failed_from, text.res) + assertEquals( + listOf(Res.string.traceroute, "Test Node", getStringResFrom(Routing.Error.NOT_AUTHORIZED.value)), + text.args.toList(), + ) + } + + @Test + fun `a nak to another request or a plain ack shows nothing`() = runTest { + watcher().watch(Res.string.traceroute, "Test Node") {} + packets.emit(routingNakPacket(REQUEST_ID + 1, from = 1234, error = Routing.Error.NOT_AUTHORIZED)) + packets.emit(routingNakPacket(REQUEST_ID, from = 1234, error = Routing.Error.NONE)) + runCurrent() + + assertTrue(resolved.isEmpty()) + } + + @Test + fun `a failed send stops watching`() = runTest { + assertFailsWith { + watcher().watch(Res.string.traceroute, "Test Node") { error("send failed") } + } + packets.emit(routingNakPacket(REQUEST_ID, from = 1234, error = Routing.Error.NOT_AUTHORIZED)) + runCurrent() + + assertTrue(resolved.isEmpty()) + } + + private companion object { + const val REQUEST_ID = 0x4242 + } +} diff --git a/feature/node/src/commonTest/kotlin/org/meshtastic/feature/node/detail/TestNakWatcher.kt b/feature/node/src/commonTest/kotlin/org/meshtastic/feature/node/detail/TestNakWatcher.kt new file mode 100644 index 00000000000..0e397c22295 --- /dev/null +++ b/feature/node/src/commonTest/kotlin/org/meshtastic/feature/node/detail/TestNakWatcher.kt @@ -0,0 +1,66 @@ +/* + * Copyright (c) 2026 Meshtastic LLC + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ +package org.meshtastic.feature.node.detail + +import dev.mokkery.MockMode +import dev.mokkery.answering.returns +import dev.mokkery.every +import dev.mokkery.mock +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.MutableSharedFlow +import kotlinx.coroutines.test.UnconfinedTestDispatcher +import okio.ByteString.Companion.toByteString +import org.meshtastic.core.common.di.asServiceScope +import org.meshtastic.core.repository.RadioController +import org.meshtastic.core.repository.ServiceRepository +import org.meshtastic.core.resources.UiText +import org.meshtastic.core.ui.util.SnackbarManager +import org.meshtastic.proto.Data +import org.meshtastic.proto.MeshPacket +import org.meshtastic.proto.PortNum +import org.meshtastic.proto.Routing + +internal fun testNakWatcher( + radioController: RadioController = mock(MockMode.autofill), + packets: Flow = MutableSharedFlow(), + snackbarManager: SnackbarManager = SnackbarManager(), + resolveUiText: suspend (UiText) -> String = { "" }, + scope: CoroutineScope = CoroutineScope(UnconfinedTestDispatcher()), +): RequestNakWatcher { + val serviceRepository = mock(MockMode.autofill) + every { serviceRepository.meshPacketFlow } returns packets + return RequestNakWatcher(radioController, serviceRepository, scope.asServiceScope(), snackbarManager, resolveUiText) +} + +internal fun routingNakPacket(requestId: Int, from: Int, error: Routing.Error): MeshPacket { + val routing = Routing.Builder().also { wb -> wb.error_reason = error }.build() + val data = + Data.Builder() + .also { wb -> + wb.portnum = PortNum.ROUTING_APP + wb.request_id = requestId + wb.payload = routing.encode().toByteString() + } + .build() + return MeshPacket.Builder() + .also { wb -> + wb.from = from + wb.decoded = data + } + .build() +} diff --git a/feature/node/src/jvmTest/kotlin/org/meshtastic/feature/node/detail/NodeDetailCompassLifecycleTest.kt b/feature/node/src/jvmTest/kotlin/org/meshtastic/feature/node/detail/NodeDetailCompassLifecycleTest.kt index 1fc92e28205..6632b370dde 100644 --- a/feature/node/src/jvmTest/kotlin/org/meshtastic/feature/node/detail/NodeDetailCompassLifecycleTest.kt +++ b/feature/node/src/jvmTest/kotlin/org/meshtastic/feature/node/detail/NodeDetailCompassLifecycleTest.kt @@ -188,6 +188,7 @@ class NodeDetailCompassLifecycleTest { ensureRemoteAdminSession = mock(), observeRemoteAdminSessionStatus = observeSessionStatus, snackbarManager = SnackbarManager(), + nakWatcher = testNakWatcher(), ) } From 09ab2b9ed2e8558ebe582e23c90c4b2b3fce9b06 Mon Sep 17 00:00:00 2001 From: James Rich <2199651+jamesarich@users.noreply.github.com> Date: Fri, 23 Oct 2026 10:52:38 -0500 Subject: [PATCH 2/2] chore(resources): sort and index request_failed_from --- .skills/compose-ui/strings-index.txt | 1 + .../src/commonMain/composeResources/values/strings.xml | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/.skills/compose-ui/strings-index.txt b/.skills/compose-ui/strings-index.txt index ee85d59e4fb..74245d1cd8a 100644 --- a/.skills/compose-ui/strings-index.txt +++ b/.skills/compose-ui/strings-index.txt @@ -1726,6 +1726,7 @@ request request_air_quality_metrics request_device_metrics request_environment_metrics +request_failed_from request_host_metrics request_local_stats request_metadata diff --git a/core/resources/src/commonMain/composeResources/values/strings.xml b/core/resources/src/commonMain/composeResources/values/strings.xml index 1a8572a7ff1..25600364b9f 100644 --- a/core/resources/src/commonMain/composeResources/values/strings.xml +++ b/core/resources/src/commonMain/composeResources/values/strings.xml @@ -1792,6 +1792,7 @@ Air-Quality Metrics Device Metrics Environment Metrics + %1$s from %2$s failed: %3$s Host Metrics Local Stats Metadata @@ -1799,7 +1800,6 @@ Power Metrics Request Telemetry Request User Info - %1$s from %2$s failed: %3$s Requesting %1$s from %2$s Resend Reset